Replace the legacy Qt viewer with a more comprehensive Qt application - #573
Draft
mscheltienne wants to merge 19 commits into
Draft
Replace the legacy Qt viewer with a more comprehensive Qt application#573mscheltienne wants to merge 19 commits into
mscheltienne wants to merge 19 commits into
Conversation
Add the channel model and the Channels page, the presentation half of the channels-to-traces contract whose display half shipped with the trace display. 'controller/_model.py' holds 'ChannelModel', a 'QAbstractListModel' owning the presentation order and the visibility while 'stream.info' stays the only owner of the name, type, unit and bad state. Its five metadata fields are a cache of 'info' overwritten wholesale after every write, so the cache cannot diverge and no paint path reads the stream. 'visible_acq_indices()' is the single visible-row to acquisition-index translation site and doubles as the 'get_data' picks argument. Two coarse signals cross to the display: 'layout_changed' for order and visibility, 'metadata_changed' for the identity-preserving edits. Both are emitted once per user action, never per row -- a per-row emission made a 256-channel "hide all" cost 95 ms against 2.3 ms. 'controller/_channels.py' holds 'ChannelsPage' and 'ChannelDelegate': a single-column list whose leading eye glyph toggles visibility, a search box and three filters that hide rows without touching model state, and a contextual inspector shown only for a selection. 'widgets/_segmented.py' implements the animated segmented control backing the Order commands. The unit editor writes the multiplier only, and the Type control owns the unit kind, which is what MNE's data model supports: a channel type determines its kind, and there is no inverse. Labels are generated from the kind symbol and the multiplier rather than tabulated, so the offered ladder cannot drift from the pair it stands for; the ladders are filtered through the multipliers MNE accepts, which is why a Volt channel offers V, 100 mV, 10 mV, mV and uV with nothing between mV and uV -- 1e-4 and 1e-5 are not FIFF multipliers and cannot be written at all. Also fix two defects in the trace display, both shared with the new page: a close dropped its theme connection and nothing restored it on the next show, so a reopened widget kept the previous mode's baked icons, and a display whose render clock was stopped by the close never resumed it. The read-only stream fixtures move up to 'tests/viewer/conftest.py' now that two subpackages need a connected stream, and the factory gained channel name, type and unit overrides so a test can publish a mixed-type stream or a duplicate name deliberately.
The viewer runs: 'mne-lsl viewer' opens a window, discovers streams, connects to a selection and renders them, and 'BaseStream.plot()' opens one on a stream the caller owns. StreamDocument composes the layers below it. It builds the channel model, the Channels page and the trace display, pushes the initial layout once, then forwards two coarse signals: an order or visibility change re-reads the visible acquisition indices and pushes them, and an identity-preserving edit refreshes the display metadata. The display never receives the model, so the rule that 'display/' cannot import 'controller/' stays structural rather than conventional. ViewerWindow owns the Qt-ADS dock manager, the application toolbar, the landing-page and document stack, the connect path and a status bar which follows the focused document. EmptyStatePage is passive: it lists what discovery found and reports a selection, and resolves and connects nothing itself. Viewer is the public facade, with a non-blocking 'show()' and a blocking 'start()'. The Qt-ADS configuration flags are set by a new 'configure_docking()' in '_bootstrap', before any dock manager exists. Setting one afterwards segfaults the process on the next 'addDockWidget', and the binding offers no way to ask whether a manager already exists, so 'Viewer' documents that it sets process-wide flags and must be built before an embedder's own manager. 'backend' gains 'derive_bufsize' and 'stream_identity'. The buffer is derived from the widest window the display can select rather than the current one: a window wider than the buffer is not an error, it silently returns the shorter buffer and the display then draws over only part of its time axis for the rest of the session. That costs memory the status bar reports honestly. Freeze now keeps the frame it froze on. A stopped clock meant nothing repainted, so hiding a channel while frozen advanced the viewport to the newest samples, and changing the row count left the plot empty while the status bar still claimed the rows were shown. The retained window is reindexed onto the current picks, since a frozen interaction is exactly what changes them. Also fixed, each with a test that fails without it: a second open while a batch was in flight stranded the first stream for the life of the process, because the connector supersedes a batch rather than queueing it; a batch which mixed a failure with a success discarded the failure message, the only error surface there is; rebuilding the stream table under a live selection retargeted it, so the viewer could connect to a stream the user had not picked; a stream name reaching a label was interpreted as rich text, and a name is remote input; showing a viewer whose window had been closed returned that closed window and then blocked forever; the borrowed-stream path accepted an irregularly sampled stream the display cannot draw; a closed window kept re-theming a dead toolbar; and the command returned no exit code. 'BaseStream.plot()' blocks, which diverges from MNE's non-blocking default: a live viewer opened from a script must block or the script exits and takes the viewer with it. 'Viewer(stream=...).show()' is the non-blocking path. Every section-divider comment of the viewer is padded to the line length, which is why a few files outside the phase appear in the diff carrying that and nothing else.
A workspace can now be saved, listed with its live availability, renamed, deleted
and reopened. No new module: every piece lands where its knowledge already lives.
'backend/_config.py' gains the pure availability evaluation, the shared channel
comparison and the rename verb, and stays free of Qt and of LSL, so the largest
test block costs no stream connection. 'backend/_discovery.py' gains a 'Prober'
worker beside 'Discovery' and 'Connector', deduplicating channel probes by
identity and caching them on the outlet instance. 'ChannelModel' gains
'set_order', 'acquisition_names' and 'rename_many'; 'StreamDocument' gains
'capture_state' and 'apply_state'; the landing page gains the card region; and
'ViewerWindow' orchestrates availability, save, load, rollback and the manage
verbs.
Two decisions are load-bearing and easy to undo by accident.
The load applies saved edits *through the channel model*, never onto the stream
before the model exists. A channel's acquisition baseline is re-captured when a
model is built, so applying to the stream first would make the first load look
perfect and the next save write empty deltas, silently discarding the whole
configuration. A round-trip test pins it, asserting every delta container is
non-empty *before* comparing, because an equality between two empty dictionaries
passes for exactly that defect.
Renames are applied as one grouped write. Row by row cannot express a
permutation: the underlying operation refuses a target still held by another
channel, so {'A': 'B', 'B': 'A'} fails on its first write while the same mapping
in one call succeeds. The old per-row loop lost such a rename to a caught and
logged error, i.e. silently, on every load of a configuration that merely
exchanged two names.
Fixes found by the review pass over the first working implementation, each
reproduced before being fixed:
- a hand-edited layout string could abort the process: 'restoreState'
dereferences the first container once the root element and version match, so a
well-formed document carrying none segfaults inside C++, where the surrounding
'except' cannot reach it
- three raises escaped the load's guarded region into the exception policy, which
logs and swallows, leaving the load neither finished nor rolled back: every
later load silently refused, Refresh dead, an orphan document still rendering,
a live inlet leaked and no dialog. One needed no hand-edited file, only a
device unplugged while connecting
- the mid-load lockout was implemented per widget and per action, so opening a
stream during a load added a document the saved layout could not name; the
restore closed it through 'viewToggled' rather than 'closed', so nothing tore
it down, and the next plain Save wrote it into the user's configuration
- a stream with no source identifier saved a file the reader then rejected: the
identifier is optional in the protocol and such a stream connects normally
- a failed probe was cached under the outlet instance, so Refresh could never
retry it and the card stayed unreachable for the session
- probes already in flight were resubmitted, doubling inlet traffic against a
live device while the card never settled
- six message boxes rendered a configuration name as markup, so a name of
'<img src=...>' embedded a local file in a dialog
- a repeated identity in a saved file built two documents over one stream
- a slot name of 'stream-²' turned a cosmetic value into a failed load, because
'str.isdigit' accepts characters 'int' refuses
- the channel comparison existed twice and the copies already disagreed on
hoisting the set, which is both quadratic and a card that can say available
above a load that refuses
Also moves 'pytest-qt' out of the base test dependency group. It is an
entry-point plugin, so pytest imports it before any conftest runs and it aborts
collection outright when no Qt binding is present, which no 'collect_ignore_glob'
can prevent: every job installing neither binding was failing to run any test at
all. It now ships in a 'test-viewer' group installed alongside a Qt extra.
A stream can be disconnected by its own acquisition thread, on any exception raised while it pulls, and the three ways of asking about that state were all unusable from outside the thread. 'connected' asserted that its four backing attributes were either all set or all unset, while '_reset_variables()' clears them one at a time from the acquisition thread. Reading the property mid-teardown therefore raised 'AssertionError' -- from the connected gate itself, i.e. from the guard callers use to stay safe. 'StreamLSL.__repr__' already worked around it with an 'except AssertionError'. A partially reset stream now simply reads as not connected, which is what it is; a genuinely partial initialization still surfaces as the clear 'RuntimeError' the other methods raise. The workaround in '__repr__' goes with the defect it existed for. Fixed in all three classes, as 'StreamLSL' and 'EpochsStream' both extend the property with attributes of their own and 'StreamLSL._reset_variables' clears the base four first, so a fix in 'BaseStream' alone would only have moved the raise. 'disconnect()' raised through '_check_connected()' when the stream was already disconnected, so a caller which owns a stream could not release it after the acquisition thread had reset it -- exactly the state that thread leaves behind. It is now idempotent and silent: a warning would be an error under the test configuration of this repository, and this is a teardown, where doing nothing is the correct answer. 'disconnect_reason' is new, and is what makes the difference between a lost source, a bad callback and a user-requested disconnection visible at all. The exception is recorded before '_reset_variables()' runs and is deliberately kept out of it, exactly as '_recorder' already is, so that it survives the teardown that produced it. 'connect()' clears it, so a reconnected stream never reports a stale reason -- which is also why 'disconnect()' does not clear it, since doing so would erase a reason the acquisition thread had recorded moments earlier. The traceback is stripped from the stored exception, after it has been logged. Keeping it alive pins the frame of 'StreamInlet.pull_chunk' and therefore its pull buffers: measured at 31 MB for 256 channels at 1024 Hz over a 30 s buffer, for the lifetime of the object, and nothing reads the traceback. Stripping it before the log instead would delete the traceback from the log as well, since 'sys.exc_info()' derives it from the exception object.
The parts with no state of their own, so that the machine which drives them is its own change. Nothing here alters existing behaviour except the last item. 'StreamSignature' and 'signature_mismatch' in 'backend/_identity.py': the auto-resume rule, as a pure comparison over plain values in the one module which imports neither Qt nor LSL, so the whole rule is tested without connecting a stream. It is deliberately stricter than, and must never be unified with, the availability rule which decides whether a saved configuration can be opened at all: that one tolerates extra channels and ignores order, because a configuration describes a desired workspace, while this one asks whether it is safe to keep drawing into an existing display whose every index is already bound. Any tolerance there is a silent mis-mapping -- an extra channel shifts every index after it, and the operator then watches one channel's samples under another's label with nothing on screen to say so. The comparison is therefore ordered, and it excludes the uid and the hostname, which identify an outlet instance and so differ for every restarted source. The rule does not require a non-empty source ID. It looks like an impostor defence and is not one: the identity is re-resolved exactly, so the check reads the document's own construction-time identity and a stream published without a source ID -- legal, discoverable, drawable -- would refuse itself and become terminally unresumable after a five second gap. And it buys nothing: two outlets publishing an identical full triple were measured, and every connection attempt landed on whichever answered first. The ordered channel comparison is the defence. 'reconnect_stream', 'stream_signature' and 'disconnect_text' in 'backend/_source.py', which keeps 'recover=False' and the name 'LostError' inside the one module allowed to know about LSL. The reconnection disconnects first: on an already-connected stream 'connect()' warns and returns, and a warning is an error under the test configuration of this repository. 'submit_reconnect' in 'backend/_discovery.py', on the global thread pool rather than as a fifth 'QThread' facade, since a reconnection is per-document and one-shot. The outcome is emitted on a separate object, never on the dock widget, whose C++ half the docking framework may already have destroyed; the callback is connected before the task starts, because Qt resolves receivers at emit time and a task which finishes first would otherwise emit into nothing. Every refusal path releases the stream it opened: dropping a connected stream leaks a live inlet and its acquisition thread for the life of the process, and 'wait_for_reconnects' exists so that a closing window does not skip that release. 'widgets/_banner.py' is the notice strip: one per document, re-texted in place and never rebuilt, which is the whole anti-spam rule. Its label is plain text, because it renders an error message derived from the network. The only behaviour change: an un-filled buffer region is now drawn as a gap. A fresh or reconnected stream allocates its buffer with zeros, so the region reads as 0.0 rather than as absent, and the clipping kept a single point of it at the row baseline and joined it to the first real sample -- a line across an outage which never carried data. Timestamps are used to find it, as a real one is never zero, and the region is always a leading run. An integer stream is promoted before the write, since a NaN cannot be stored in an integer array and that raised on every tick of the render clock. It also stops the un-filled zeros from being read as a rising trigger edge.
liblsl exposes no connection status: a lost stream surfaces only as an error from a pull, and only with recovery disabled, which in turn disables liblsl's own stall watchdog. So a source which hangs without dropping its connection produces no error anywhere. A document therefore polls two things from the render clock it already runs -- whether the stream is connected, and how long ago its newest sample arrived -- and moves through four states. Interrupted freezes the viewport on the last frame it drew, shows the notice strip, and reconnects in the background on a 1, 2, 5, 10 second ladder. A source which returns must match on identity, sampling rate, format and the ordered list of wire channel names, or the document becomes mismatched: terminal, with an explicit retry, because a mismatch does not resolve itself and closing the document to escape it would cost every channel edit, display setting and layout position -- which is what retry exists to preserve. A match re-applies the operator's edits and only then waits for one real window before declaring the document live, so a source which returns and vanishes again does not flash a live viewport. The edits are re-applied through the channel model, in the order a configuration load uses, because the model re-baselines its acquisition baseline from whatever the stream declares: writing to the stream first would make the edited values that baseline and the next save would then write empty deltas, destroying the workspace by the act of saving it. A changed channel count would reach that re-baselining, and cannot: it is a mismatch, and a mismatch never resumes. The confirmation carries a deadline of its own. Without one, a source which comes back and stays silent -- the hung sender the stall detector exists for, and the likeliest state of acquisition software which has just restarted -- left the document waiting on a first sample forever, having stopped reconnecting, with a notice reading "reconnecting" and no way out but closing it. The same wait also has to tolerate a display with every trace hidden, which fetches nothing at all and so cannot confirm anything. A borrowed stream, i.e. one the viewer was handed rather than opened, is never reconnected on a timer. Reconnecting a stream in place destroys the filters, the callbacks, the reference channels and the acquisition delay its owner set on it, and the stall path fires on a source which merely went quiet -- so the viewer would silently rebuild a working stream that nobody had lost. It offers retry instead, and acts only when asked. The clock is stopped for the whole of an attempt, since the worker holds the stream briefly connected with a channel set the display has not been rebuilt against. Stopping the clock is not sufficient on its own: a scroll or a visibility change repaints on demand, reaching the stream with the old picks, so the display now carries an explicit suspension and one method writes both.
'disconnect_reason' and the reworked 'connected' and 'disconnect' docstrings never reached 'base.pyi', 'epochs.pyi' or 'stream_lsl.pyi', so the surface an IDE and any static consumer read still described the pre-viewer API: the property the changelog advertises did not exist there at all, and the two rewritten contracts -- an idempotent disconnection and a partially reset stream reading as not connected -- were absent. Half of this file set was already hand-updated for 'plot()' and half left to 'stubs.yaml', which is the state that hides the drift. Written to the convention of the surrounding file rather than to the runtime '__doc__': 'tools/stubgen.py' injects the raw docstring and then runs 'ruff format' over the result, so every docstring already in these files is indented where the runtime one is not, and the generator normalises the whole set on its next run.
Six defects, each with a test which fails without its fix. '_on_connected' built the document unguarded, so a construction which raises released nothing: the connector transfers the ownership of the stream with its signal and the exception policy merely logs what escapes a Qt slot, leaving a subscribed inlet and its acquisition thread alive, unreferenced, for the life of the process. Reachable -- the document refuses a stream declaring no sampling rate, which is what a source re-provisioned as an event stream between the discovery pass and the connection arrives as. Both non-document exits now go through 'release_stream', the helper the duplicate branch was open-coding. 'status_fields' and 'TraceDisplay._poll' both checked 'connected' and then read the stream, which the acquisition thread resets on its own account: the check answers for an instant that has passed by the time the read runs. The status bar now reads the connection and the two fields it gates in one guarded block, before reporting anything, so a read which fails is a disconnection like any other rather than 'Connected • Live' next to five blanks; the display catches the same failure and re-raises it whenever the stream is still connected, so the one 'get_data' logs a bug report for is never swallowed. Without this a routine outage produced a CRITICAL traceback out of the 33 ms render tick. The toolbar indicator branched on the freeze alone, so it kept its green '● Live' through an outage -- and 'MISMATCHED' stops the render clock outright, which made it a claim of liveness over a viewport that is never going to advance again, directly contradicting the banner above it. The state now overrides the label, and the refresh happens in '_apply_clock', the one seam every transition passes through, so a future state cannot update the clock and forget the indicator. 'controller_width' reported 0 for a panel the user dragged onto the splitter's edge: shown, and zero wide, which the hidden-panel fallback does not cover. Saving it wrote a configuration which opens with no Channels page in every session that loads it, while the collapse it came from lasts as long as the window and is undone by the toggle. A configuration card whose name a pass no longer reports was removed from the layout and 'deleteLater()'-ed but never hidden, and 'QLayout::removeWidget' does neither, so the orphan kept painting at its last geometry over the cards re-sorted underneath it. And 'set_streams' re-anchored the selection one 'select()' at a time under a live 'itemSelectionChanged' connection, so one discovery pass re-evaluated the Open action -- re-materialising every selected index to do it -- once per selected stream; measured at two extra emissions for the two-row case. The table's signals are now blocked for the rebuild, leaving the explicit emission at the end as the only one. Also carried here, both in files above: the note in '_on_document_closed' claimed 'deleteDockWidget' does not exist in the PyQt6 distribution while two methods of that same file call it, which pointed the recorded upgrade at the 'removeDockWidget' plus 'deleteLater' pattern the file elsewhere warns crashes the process; and 'refresh()' now records why it has no empty-model guard, since the rows are empty only while the stream is disconnected -- which the check above it already returned on -- and a connected stream never reports zero channels. One line of cleanup: 'capture_state' copied 'DisplayControls.state', which builds a fresh dict on every read.
'install_exception_policy' documented the traceback as logged at the ERROR level while '_excepthook', eight lines above, logs at CRITICAL and carries its own comment saying why ERROR would be wrong: an unhandled exception must not become invisible under 'MNE_LSL_LOG_LEVEL=CRITICAL'. A reader filtering the viewer's log against the docstring would conclude the policy is broken. And 'pytest-qt' does not ship with the viewer extras: it is its own 'test-viewer' dependency group, which the CI job requests next to one of the 'pyqt6' / 'pyside6' extras. Following the comment leads either to adding a test-only plugin to every user install of the viewer, or to dropping the flag the job depends on.
The nine viewer entries cited ':pr:`566`', a number guessed before the pull request existed and recorded as needing a check once it opened. The check never ran and the guess went stale exactly as expected: 566 is now the merged "Bump pypa/cibuildwheel from 4.1.0 to 4.1.1", so the whole viewer rewrite, 'plot()' and the four 'BaseStream' changes were attributed to a dependency bump, and the entries had grown from five to nine in the meantime.
The centred content column was added to its layout with stretch 0 between two stretch-1 spacers, which take the extra width first: the column never grew past its ~300 px size hint on any window, its maximum width was unreachable dead code, and the stream table sat at 256 px behind a permanent horizontal scrollbar with four of its six columns out of view. Flip the stretches, drop the group box which framed an already-framed table, and let the table take the free height instead of a fixed cap. The header now stretches Name and sizes the short columns to their content, so every column fits without a scrollbar down to a ~600 px window; the source ID stays interactive at a fixed default, since sizing a uuid to its content is what crowded the other columns out. The table is hidden for the duration of a fill, because a 'ResizeToContents' section recomputes on every 'setItem' and each recompute walks every row -- 51 ms at 30 streams and 531 ms at 100, against 0.7 and 2.1 ms hidden. Selection moves to 'MultiSelection': a plain left click toggles one row, where 'ExtendedSelection' made the second stream cost a modifier nobody discovers. The page already selected through the selection model with explicit flags, which bypasses the mode, so nothing in it changes; the one test which reached for 'selectRow' now takes the same path, since a view-level call toggles under the new mode. With nothing to show, the table and the event line hide rather than paint an empty grid and a line of copy stating that nothing was found -- the progress label above already carries that. A pass which found only event sources says so, because discovery counts them as streams and would otherwise report "Updated just now" over a page whose table just vanished.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.