fix(progressive): make the filling vectors window interactive — locked tree + intent latch - #132
Draft
CSSFrancis wants to merge 6 commits into
Draft
fix(progressive): make the filling vectors window interactive — locked tree + intent latch#132CSSFrancis wants to merge 6 commits into
CSSFrancis wants to merge 6 commits into
Conversation
…guard The progressive vectors preview installs its own slice function wholesale (sel.children[child] = preview.slice_fn), so update_from_navigation_selection -- and with it PR #126's _nav_readable_data skip -- is out of the drag path for the result window's navigator reads. Pin it from both sides: with the deepcopy placeholder parked on .data for an entire drive (and under a rebind hammer) every dispatcher read at a computed position still serves with zero 'nav read skipped' records, and the guard re-engages only on the default path the selector falls back to after preview.close(). Written while investigating the progressive_signal_preview.spec.ts CI failure (zero served frames over a drag): the guard was the suspected mechanism and is structurally ruled out here.
Dragging a Find-Vectors result window's navigator over a region the batch has already computed now shows that position's diffraction pattern, and keeps showing what the user navigated to. Three parts. LOCK. The result tree is locked for the duration of the batch (lifecycle.lock_tree / refuse_if_locked): no actions and no new nodes on it until the vectors attach. That is what makes ProgressiveSignalPreview.install() correct BY CONSTRUCTION -- with the link set frozen, its one-shot snapshot of the navigator->signal links cannot go stale, so the Live-Display nav read gets no per-read re-check. ONE helper gates all three entry points (the toolbar invoker, the staged dispatch, BaseSignalTree.add_node/add_transformation), so a new entry point either calls it or is visibly missing it, and a refusal is a user-facing error rather than a silent no-op. Released where the vectors attach (_finalize) AND in the batch teardown, since only the latter runs on a cancelled or failed run. Read-only re-emits and teardown verbs (*_query/*_close/*_stop/*_cancel) are exempt: the renderer fires overlay_query itself on every focus change, so gating it put an error in the status bar on every progressive fill (seen), and a lock that can trap a caret open is worse than no lock. LATCH. The first crosshair MOVE ends the auto-sample flash for the rest of the run -- the signal panel is the user's from then on. The 2-second hold it replaces gave the panel back exactly when the user paused to look at what they had navigated to. A forced re-fire at an UNCHANGED index (the parked-position refresh, the selector's settle timer) is not a move and does not latch; the reference position is seeded at install so the first real move counts. THE E2E's ZERO-SERVED DRAG WAS NOT A ROUTING BUG. Tracing the dispatcher through a real run showed install() lands on exactly the link the drag uses (CrosshairSelector -> the tree's signal plot -> _slice_fn) and that reads at ready positions serve. The drag produced no dispatcher submits at all: the harness had parked the crosshair by writing the widget from the backend, and the press then hit-tested against a renderer state that no longer matched -- measured, the crosshair's entire travel across the drag was the aim's own displacement, reversed. So test_aim_ready_position now REPORTS a walk target as a nav-index delta and moves nothing; the spec converts it to pixels from the crosshair guide lines' own extent (they span the drawn image) and drives the whole walk with the mouse, the path vectors_dp_follows_nav.spec.ts already proves works on this window. Part (a) also stops sampling once the hold parks, which was spending the hold's 120 s budget before part (b) ran. The gated [NAV-IDX] trace now carries each child's update-function name, which is the line that answers "did the swap land on the link I dragged?" directly. Verified: progressive_signal_preview.spec.ts passes -- crosshair moved 119 px of the 119 asked for, served 0 -> 23 over the drag, 5 distinct diffraction patterns, batch still running; the mid-drag screenshot shows a real pattern at the aimed position, not the zeros placeholder.
The result-tree compute lock already refused a toolbar click on a window a find-vectors batch was still filling, but the button looked perfectly clickable until you pressed it and got an error. Show it instead. Contract change, in the existing toolbar_config message: each action descriptor gains `disabled` + `disabled_reason` while the plot's tree is locked (get_toolbar_config_for_plot). Per-action rather than a message-level flag because ToolbarAction[] is what already flows into FloatingToolbar -- a tree-level flag would have to be threaded through the reducer and window state as a separate prop. DISABLED, not hidden: the requires_vectors family hides because those actions do not apply to the data, where the lock means "not yet", and a button that vanishes and comes back reads as a glitch. Every toolbar action is disabled because _dispatch_toolbar_action refuses every one of them; registry.is_lock_exempt covers STAGED verbs, which are caret-internal and never appear in this config. lock_tree/unlock_tree now re-send the config for every plot of the tree, so the buttons grey at the START of the fill and restore when the vectors attach -- not only when something else happens to re-send. Every plot, not just the signal ones: the navigator's Add Selector would add the very navigator->signal link the lock exists to freeze. Both are transition-only, so the second unlock (the batch teardown's belt-and-braces call) costs nothing. The renderer dims + greyscales the icon, sets cursor:not-allowed, puts the reason in the tooltip, marks the button `disabled`, and drops the :hover highlight for disabled buttons (a disabled button still matches :hover, so without that they kept lighting up and reading as clickable). click() also early-returns on `disabled`, and the backend refusal stays -- a renderer holding a stale config must not be able to land a node on a half-built tree. Verified in the real app (progressive_signal_preview.spec.ts, extended): while the batch is parked mid-fill all 4 of the result window's buttons assert `toBeDisabled`, and after `[fv-batch] finalized` the first one asserts `toBeEnabled`. Screenshots 95-toolbar-greyed-while-filling.png (dim, greyscale, against the source window's bright toolbar in the same frame) and 96-toolbar-restored.png (7 bright buttons -- the requires_vectors actions have appeared too).
# Conflicts: # spyde/actions/find_vectors_action.py # spyde/backend/_session_actions.py
…hide them Reported: "all of the vector actions aren't visible ... I'd like the vectors signal with some actions greyed out while computing." Measured in the app on sped_ag: the result window showed 4 greyed buttons for the whole run (FFT, Rebin, Crop, Line Profile) and gained 3 more the instant `[fv-batch] finalized` landed (Vector Virtual Imaging, Vector Orientation Mapping, Strain Mapping). The window is NOT reporting a wrong or temporary signal type -- checked in the running app, Plot Control's Signal type field reads `spyde_diffraction_vectors_image` from the moment the window opens and never changes. What hid the actions is `requires_vectors`, which gates on `tree.diffraction_vectors` and so is false until `_finalize` attaches the container. That gate has two meanings and no way to tell them apart: on an ordinary signal it means "this action does not apply to this data" (hide is right); on the window a Find-Vectors batch is filling it means "the result is still computing" (hide is wrong -- the user cannot tell that waiting will bring the buttons back, and when it does they pop in). The compute lock is exactly the signal that separates the two, so it now relaxes that one gate: while a tree is LOCKED a `requires_vectors` action is included in the toolbar config and carries the same `disabled` + `disabled_reason` as every other action, and un-greys when the container attaches. The button SET no longer changes when the run ends. Every other gate stays absolute -- a lock never makes an action appear on data it does not apply to. A cancelled or failed batch unlocks with no container, so they disappear again, correctly: that window really has no vectors. Only `get_toolbar_config_for_plot` (what the renderer draws) knows about the lock. `get_toolbar_actions_for_plot`, which RESOLVES the action functions, does not need to: a locked tree refuses every action before it is resolved.
…from the read
Reported: "while the vectors are computing I can drag around, but it still tries
to update when I let go."
The suspected cause -- `_user_owns` latching only on a SERVED read, so a drag
entirely over uncomputed data never latches -- is NOT what the code did:
`_note_read_position` ran BEFORE the readiness check, so a declined read latched
too. Three instrumented runs against the real app (sped_ag, real Dask, mid-fill
drag + release, sampling the DP every 250 ms for 8-15 s and tracking every
renderer->main action) could not reproduce a repaint on release: the settle
re-fire returns None for an uncomputed position and `_run_update` paints
nothing, and no action is sent on pointer-up at all.
What IS wrong is that ownership was INFERRED from the reads ("a read arrived at
a new nav index, so the user must have dragged"), and that inference has holes
which all surface as the same symptom -- a landing block repainting the panel
just as the user lets go:
* `_run_update` short-circuits a repeat position, so a grab-and-release, or a
drag that comes back to where it started, or a release at the index the
crosshair was already on, produces NO read at a new index at all;
* the dispatcher coalesces, so intermediate positions never reach the preview;
* a read that raises inside `_resolve_index` is swallowed and counts as
nothing.
So latch on the pointer event instead of guessing from its consequences.
`BaseSelector` grows `interaction_hooks`, fired from `update_data(..., user=True)`
-- passed ONLY by the widget handlers in selector1d/selector2d -- before the read
is even queued. Default False, so multiplot_manager wiring a new plot, the CSB
width change and the strain reference selector can never be mistaken for a human.
Nothing else about the update path changes: same dispatcher, same latest-wins
coalescing, same settle re-fire.
`ProgressiveSignalPreview` registers one hook per captured navigator (both
sub-selectors of a CompositeSelector, which owns no widget of its own and
delegates to whichever is active) and removes it on close. The changed-index
check stays as a BACKSTOP for a crosshair moved without a pointer event (a
programmatic set, a linked selector) -- it can only latch later than the hook,
never instead of it.
Verified in the real app (progressive_signal_preview.spec.ts, new part (c)):
mid-fill, with the batch parked, the crosshair is dragged to the far right and
released over an uncomputed position; the DP's content signature is identical
across the baseline and all 8 samples over the following 2.4 s, and the backend's
only ownership line is "navigator grabbed" -- the pointer path, not the backstop.
The spec also now asserts the vector actions are present-and-greyed during the
fill and that the button SET is unchanged after finalize (7 either way).
Migrated tests: a pointer grab with no read at a new index still latches and the
next landing block paints nothing; only a pointer-driven update fires
`interaction_hooks`; close unregisters it; a release over an uncomputed position
paints nothing from either the settle re-fire or a landing block.
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.
Finishes a feature that was half-wired: while a find-vectors batch streams, dragging the result window's navigator now shows the diffraction pattern for any position already computed. Two design decisions from the maintainer: the result tree is locked for the duration of the compute, and user intent wins permanently once they touch the crosshair.
The instrumentation corrected the working diagnosis. The suspicion was that
ProgressiveSignalPreview.install()'s one-shot swap missed the dragged link. A gated[nav-route]trace disproved it — install landed on exactly the selector/plot pairBaseSelector._run_updateiterates, and reads at ready positions served:The drag produced zero dispatcher submits — pointer events never reached the backend, because the test-only aim parked the crosshair by writing the widget server-side (
w.set(_notify=False, …)), leaving renderer hit-testing out of sync. Proof by pixel measurement: total crosshair travel over a 130 px drag was 17.9 px = exactly the aim's own displacement, reversed. So the half-wired parts were the latch and the lock; the read path itself worked.Result-tree lock (
lifecycle.lock_tree/unlock_tree/tree_lock/refuse_if_locked) — one helper, user-facingemit_error, never a silent no-op. Gated: toolbar-action invoke, staged dispatch,BaseSignalTree.add_node,add_transformation. Set beforeattach_signal_preview; cleared in_finalizeand in batch teardown (a cancelled batch would otherwise strand the lock for the session).Intent latch: first navigator read at a new index permanently ends auto-sampling for that run. Index-change is the criterion because our own re-fires (parked-position refresh, settle timer) are forced reads at an unchanged index — latching on those would kill auto-sampling the first time a block landed under a resting crosshair.
Aim rewritten: the backend now reports a nav-index delta and moves nothing; the spec converts indices→pixels via the crosshair guide extent and seeks inside the same pointer-down. Also stops part (a)'s sampling once the hold parks — it was burning the 120 s hold budget before part (b) ran (3.1 min → 47 s).
Evidence: full suite 2664 passed / 25 skipped / 1 xfailed. The e2e passes twice with different ready sets (
dix=-6/dix=-39), proving the aim adapts to run-to-run fill order: served 0 → 23, 5 distinct DP signatures, batch still running. Screenshots read (progressive_signal_shots/83,84): crosshair inside the computed band, DP showing a real disk pattern, not the zeros placeholder. Typecheck clean.One carve-out to review (flagged, not silently taken): the Plot Control dock fires
overlay_queryitself on focus change, so the strict lock put a refusal in the status bar on every progressive fill.registry.is_lock_exemptnow lets*_query/*_close/*_stop/*_cancelthrough — none can add a node or a link, and a lock that can trap a caret open is worse than no lock. This is a deviation from a literal "no actions"; happy to gate them anyway.Also noted: only the Find-Vectors result tree is locked (Orientation/EBSD progressive windows have no navigator, so nothing to protect —
lock_treeis generic and ready); the source tree stays unlocked (pre-existing); refused toolbar buttons still look enabled — greying them out while locked is a small follow-up.