63 lines
10 KiB
Markdown
63 lines
10 KiB
Markdown
# Independent ordinary saved-view multi-selection review — 2026-09-23
|
||
|
||
Read-only review of the in-progress ordinary saved-view adapter against `multiselect-contract-20260923.md`. The owning session was actively wiring UI during inspection; references describe the inspected code and must be rechecked after fixes. No production edits, Cargo, GUI, or runtime/test acceptance claims. Missing State, Allocation, derived Process/Scenario and aggregate model-delete adapters are explicitly outside this review, not findings below.
|
||
|
||
Reviewed primary files: `crates/oa-desktop/src/app/multiselect.rs`, `diagram/multiselect.rs`, `diagram/group_gesture.rs`, `diagram_selection.rs`, plus relevant `app.rs`, `diagram.rs`, `diagram/focus.rs`, `views.rs` and menu paths.
|
||
|
||
## Verified findings
|
||
|
||
### P2 — surviving selection is discarded when legacy sync removes the primary
|
||
|
||
`app/multiselect.rs:78–97` decides whether the group held the primary by comparing the old selection against **post-sync** `selected_node`/`selected`. But `sync_with_alias_editor` clears nonexistent semantic selection and calls `refresh_draft`, which recomputes/clears the placement (`app.rs:800–810`, `:829–853`). Consequently the reducer's correct survivor fallback is overwritten.
|
||
|
||
Concrete path: select placement A and B, with B primary; undo the model operation that created B, or remove B's placement through a singleton-target action. `sync` first makes `selected_node = None` (and may select the project). At final reconciliation, `held_primary` is false. `Selection::reconcile` prunes B and chooses A, but `legacy_target` is None, so line 95 clears A too. The same problem occurs when an external accepted model/source change removes the primary while other exact selected placements remain.
|
||
|
||
Preserve provenance of the pre-change primary across sync, and distinguish genuine explicit navigation from model/history reconciliation. The latter should prune only invalid members and inspect its surviving fallback. Do not recover by mapping an old semantic UUID to an arbitrary replacement placement. Regression: select A/B, undo B creation; expect A selected and inspected, no additional history. Repeat for placement deletion and semantic deletion, plus deletion of a non-primary member and ordinary group-move Undo/Redo.
|
||
|
||
### P2 — mixed selection cannot collapse through the specified stationary click
|
||
|
||
`diagram/multiselect.rs:83–97` tests whole-set movement capability on pointer press. For a legal selected set containing placement A and edge E (or a port), a plain press on either member immediately publishes `Refuse`, stores no click gesture, and captures the press. A stationary release therefore cannot execute the contract's "select that member alone" behavior. This is a selection defect inside ordinary saved views, not a request to implement mixed-target movement.
|
||
|
||
Track a deferred click for every selected exact target, independently of movement capability. On a stationary release replace with that target; only crossing the drag threshold should refuse unsupported movement while retaining the set. Regression: A+E, click A, expect exactly A and no model/history mutation; recreate A+E and drag A, expect refusal and both retained. Repeat with a port and with pointer displacement that never exceeds the threshold.
|
||
|
||
### P2 — a left press can start group movement while middle-button pan owns the pointer
|
||
|
||
The new `group_input` runs before the existing singleton left-press branch (`diagram.rs:724` versus the middle-pan guard around `:858`). It has no `state.pan_start` admission check. Reproducible event path from inspection: select A/B, middle-button press sets `pan_start`; while holding middle, left-button press on selected A enters `GroupGesture::new`; subsequent cursor motion is consumed by the group and left release emits a model move even though the middle pan remains active. The legacy singleton handler explicitly captures this left press without moving nodes.
|
||
|
||
Keep gesture ownership exclusive before starting or toggling group selection. Reject/defer left presses while middle pan is active, and ensure starting a group cannot coexist with `drag`, `route_drag`, `port_drag`, or work gesture state. Regression should replay middle-down → left-down on selected node → move → left-up → middle-up and assert no geometry/history/selection mutation attributable to a group drag.
|
||
|
||
### P2 — Delete hides the group during an active drag
|
||
|
||
The active-group dispatcher handles only cursor movement and left release (`diagram/multiselect.rs:27–45`). Delete falls through into the unconditional multi-selection Delete branch at `:54–63`, emitting `Hide(snapshot)` while the live preview exists. App validates that snapshot and hides the placements. By contrast the established singleton Delete branch requires every move/route/port/pan/work gesture to be idle (`diagram.rs:803–813`). Group selection should preserve this input-safety convention.
|
||
|
||
Require no live pointer gesture before publishing Hide, or define and test an explicit cancel-then-delete policy. Do not silently delete while the drag's release is still pending. Regression: A/B selected, start group drag and move, press Delete before release; expect no deletion/model history, retain the selection, and verify the subsequent release follows the chosen policy. Cover Delete during middle pan too: the same new branch also lacks `pan_start` gating.
|
||
|
||
## Performance observation (P3, no latency measurement)
|
||
|
||
Every accepted `App::update` calls `reconcile_multiselect` (`app.rs:1383`), which unconditionally calls `projection::project_view` (`app/multiselect.rs:76`) while on Page::Diagram. That rebuilds `ViewIndex::new`, including the global BehaviorIndex/evidence and all edge facts (`oa-core/src/view_options.rs:484–543`). Thus each pan delta, inspector keystroke and unrelated asynchronous completion now incurs global semantic indexing plus projection, even when surface/model visibility and selection are unchanged. Explicit Choose additionally projects before calling reconciliation and again at end of update.
|
||
|
||
Reuse `document.view_index()` via `project_view_with_index`, and separate revision/surface/visibility reconciliation from explicit legacy selection changes. Cache or skip the former when authority and visibility inputs have not changed. This is a verified unnecessary work path; responsiveness has not been measured and no timing claim is made.
|
||
|
||
## Sound portions and boundaries
|
||
|
||
- `Selection` stores insertion-ordered exact typed targets, maintains primary membership, prevents Whole admission, and uses a generation plus exact snapshot/context comparison to reject remove/re-add and primary ABA cycles. The reducer's survivor fallback itself is correct; the defect is App's later legacy reconciliation.
|
||
- `placements` collects an `Option<BTreeSet<_>>` over the full target set, so movement/hide cannot silently filter ports or edges. Selection events are checked before App's general allocation/camera boundaries, and operation events require the exact captured selection generation.
|
||
- `GroupGesture` freezes viewport and press geometry, consumes release position, latches threshold crossing, preserves a group after return-to-origin or snap-to-zero, and permanently retires invalid/outside pointers. Ordinary group input checks current selection snapshot and viewport before processing it. Preview applies only the geometry planner's emitted records, avoiding the full session-per-frame identity work identified in the core review.
|
||
- The existing ordered `state.pointer` normalization occurs before `group_input`, preserving native event-order handling rather than using the final batch cursor for earlier presses/releases.
|
||
- Escape, viewport/context invalidation, window focus loss, cursor leave, wheel, and right/middle press cancel an existing group before committing. App independently denies dirty drafts, busy/modal ownership, active architecture editors and connections. The newly passive modifier path is appropriate for Shift held before canvas focus; it does not grant keyboard command focus.
|
||
- Model/source transactions still flow through `App::execute` and `Document::execute`. Successful group movement restores the exact primary after the command's implementation-selected last record; zero movement returns before execution. Source failure is not accepted as a successful group operation.
|
||
|
||
Future verification should use mounted App and native-ordered canvas-event tests for the four findings, then actual GUI exercise. Reducer and gesture unit tests alone cannot expose the cross-layer sync and competing-gesture paths above. Existing known incomplete adapters are neither accepted nor reopened by this review.
|
||
|
||
## Follow-up inspection while desktop-1 gate runs
|
||
|
||
The owner has addressed the reported paths in source. This status is static inspection only; the review findings above remain useful regression definitions and are not asserted to be outstanding in the updated source:
|
||
|
||
- App reconciliation now detects an invalid exact primary before pruning and retains the reducer's survivor fallback when legacy sync has no target.
|
||
- A typed deferred `multiselect::Press` preserves mixed-selection stationary clicks; movement refusal occurs only after threshold crossing.
|
||
- An active group captures keyboard input, so Delete does not fall through to Hide; existing Escape cancellation still precedes that capture.
|
||
- Group admission yields to an existing `pan_start`, preserving the legacy middle-pan exclusion and Delete guard.
|
||
- App reconciliation is conditional on its selection/model/surface boundary and uses the document's cached view index.
|
||
|
||
Eleven production-canvas event regression tests have been drafted in the local-only `.local/multiselect-canvas-tests.rs` for later authorized incorporation into `diagram/multiselect_tests.rs`. They exercise actual `DiagramCanvas::update`, including ordered native batches, captured status, preview records, snapshots and independent literal geometry expectations. Coverage includes Shift hit identity, 0.5/1/2 zoom grouped movement, mixed click versus drag, Escape, Delete during drag, middle-pan exclusivity, seven viewport/context changes, membership ABA, release-only displacement and return-to-origin. The parent owns App-level survivor tests, module registration, compilation and native verification. No draft test has been compiled or run by this reviewer; no production test file was created during the frozen-input gate.
|