From f6a6010baed9e6164a1fcc7ecd58256a562827d7 Mon Sep 17 00:00:00 2001 From: "sorokin.oleg.edu" Date: Wed, 23 Sep 2026 01:53:11 -0700 Subject: [PATCH] fix: keep saved diagram viewport stable across group selection --- .../oa-desktop/src/app/diagram_focus_tests.rs | 99 ++++++++- crates/oa-desktop/src/views.rs | 87 +++++--- harness/multiselect-native-review-20260923.md | 198 ++++++++++++++++++ 3 files changed, 351 insertions(+), 33 deletions(-) create mode 100644 harness/multiselect-native-review-20260923.md diff --git a/crates/oa-desktop/src/app/diagram_focus_tests.rs b/crates/oa-desktop/src/app/diagram_focus_tests.rs index e4952c1f..345cfa83 100644 --- a/crates/oa-desktop/src/app/diagram_focus_tests.rs +++ b/crates/oa-desktop/src/app/diagram_focus_tests.rs @@ -154,9 +154,6 @@ async fn mounted_f2_requires_canvas_focus_and_yields_to_clean_inspector() { } } } -// Append to app/diagram_focus_tests.rs after the frozen desktop gate. -// Reuses its imports and FocusField, plus Mounted/key from canvas_workspace tests. - async fn modifier_focus_fixture() -> (App, Mounted, Id, Id, Id) { let mut project = Project::empty(); let a = project @@ -498,3 +495,99 @@ async fn mounted_window_unfocus_resets_held_shift_before_next_canvas_press() { assert_eq!(app.document.bundle(), &before.bundle); assert!(!app.document.can_undo()); } + +#[tokio::test] +async fn mounted_selection_strip_keeps_canvas_and_fitted_camera_fixed_for_empty_single_group_and_mixed() + { + use crate::diagram_usage::Target; + let mut project = Project::empty(); + let a = project + .add_element(Kind::Activity, Layer::Oa, "First activity", None) + .unwrap(); + let b = project + .add_element( + Kind::Activity, + Layer::Oa, + &format!( + "Second activity {}", + "with a very long primary name ".repeat(20) + ), + None, + ) + .unwrap(); + let edge = project.relate(RelationKind::Interaction, "Activity exchange", a, b); + let diagram = project.ensure_diagram(Layer::Oa); + project.diagrams.get_mut(&diagram).unwrap().projection = ProjectionKind::Overview; + let first = project.place(diagram, a, Position { x: 80., y: 80. }); + let second = project.place(diagram, b, Position { x: 500., y: 80. }); + let mut app = App::with_project(project); + app.page = Page::Diagram; + app.layer = Layer::Oa; + app.active_diagram = Some(diagram); + app.window_size = Size::new(1442., 932.); + let _ = app.update(Message::Select(diagram)); + app.fit_to_view = true; + let mut mounted = Mounted::new(&mut app).await; + let baseline = app.document.snapshot(); + let bounds = mounted + .geometry(&app) + .bounds(crate::allocation_input::CANVAS_ID); + let camera = (app.zoom, app.pan, app.fit_to_view, app.canvas_size); + assert!(camera.2, "exercise the native resize-triggered refit path"); + assert!(app.multiselect_snapshot().unwrap().targets().is_empty()); + assert!(bounds.width > 500. && bounds.height > 300.); + for (target, expected) in [ + ( + Some(Target::Placement(first)), + vec![Target::Placement(first)], + ), + ( + Some(Target::Placement(second)), + vec![Target::Placement(first), Target::Placement(second)], + ), + ( + Some(Target::Edge(edge)), + vec![ + Target::Placement(first), + Target::Placement(second), + Target::Edge(edge), + ], + ), + ( + Some(Target::Edge(edge)), + vec![Target::Placement(first), Target::Placement(second)], + ), + ( + Some(Target::Placement(second)), + vec![Target::Placement(first)], + ), + (None, vec![]), + ] { + let context = app.multiselect_context().unwrap(); + let event = match target { + Some(target) => { + multiselect::Event::Choose(context, target, multiselect::Choice::Toggle) + } + None => multiselect::Event::Clear(context), + }; + let task = app.update(Message::DiagramSelection(event)); + mounted.perform(&mut app, task).await; + mounted.settle(&mut app).await; + assert_eq!(app.multiselect_snapshot().unwrap().targets(), expected); + assert_eq!( + mounted + .geometry(&app) + .bounds(crate::allocation_input::CANVAS_ID), + bounds, + "selection feedback must not change the measured canvas rectangle" + ); + assert_eq!( + (app.zoom, app.pan, app.fit_to_view, app.canvas_size), + camera + ); + assert_eq!(app.project(), &baseline.decoded.project); + assert_eq!(app.document.bundle(), &baseline.bundle); + assert!(!app.document.can_undo()); + assert!(!app.draft_dirty()); + } +} diff --git a/crates/oa-desktop/src/views.rs b/crates/oa-desktop/src/views.rs index 79870af7..b8cac778 100644 --- a/crates/oa-desktop/src/views.rs +++ b/crates/oa-desktop/src/views.rs @@ -429,32 +429,7 @@ fn diagram(app: &App) -> View<'_, Message> { chooser = chooser.push(action("View properties", Message::Select(id))); controls = controls.push(action("Auto-layout", Message::Command(Command::Layout(id)))); } - let mut toolbar = column![chooser, controls].spacing(6); - if let Some(snapshot) = app.multiselect_snapshot().filter(|s| s.targets().len() > 1) { - let mut group_controls = row![label(format!("{} selected", snapshot.targets().len()))] - .spacing(7) - .align_y(iced::Center); - if crate::app::multiselect::placements(&snapshot).is_some() { - group_controls = group_controls.push(action( - "Hide selected", - Message::DiagramSelection(crate::app::multiselect::Event::Hide(snapshot.clone())), - )); - } - group_controls = group_controls.push(action( - "Clear selection", - Message::DiagramSelection(crate::app::multiselect::Event::Clear(snapshot.context())), - )); - toolbar = toolbar.push( - column![ - group_controls, - label(format!( - "Properties edit only {}", - short(app.project().named(app.selected), 36) - )) - ] - .spacing(3), - ); - } + let toolbar = column![chooser, controls].spacing(6); let projection = app .active_diagram .and_then(|id| app.project().diagrams.get(&id)) @@ -634,6 +609,37 @@ fn diagram(app: &App) -> View<'_, Message> { ) .is_some_and(|(source, target)| source != edge.source || target != edge.target) }); + let selection = app.multiselect_snapshot(); + let count = selection.as_ref().map_or(0, |s| s.targets().len()); + let mut selection_controls = + row![label(format!("{count} selected")).wrapping(widget::text::Wrapping::None)] + .spacing(7) + .align_y(iced::Center) + .height(32); + if let Some(snapshot) = selection.as_ref().filter(|s| s.targets().len() > 1) { + if crate::app::multiselect::placements(snapshot).is_some() { + selection_controls = selection_controls.push(action( + "Hide selected", + Message::DiagramSelection(crate::app::multiselect::Event::Hide( + snapshot.clone(), + )), + )); + } + selection_controls = selection_controls + .push(action( + "Clear selection", + Message::DiagramSelection(crate::app::multiselect::Event::Clear( + snapshot.context(), + )), + )) + .push( + label(format!( + "Properties edit only {}", + short(app.project().named(app.selected), 36) + )) + .wrapping(widget::text::Wrapping::None), + ); + } let hint = if let Some(feedback) = &app.allocation_drag.feedback { feedback.text.as_str() } else if let Some(session) = &app.connection { @@ -646,16 +652,37 @@ fn diagram(app: &App) -> View<'_, Message> { } else { session.tool.hint() } + } else if count > 1 { + if selection + .as_ref() + .is_some_and(|s| crate::app::multiselect::placements(s).is_some()) + && projection != Some(ProjectionKind::Allocation) + { + "Shift-click to add/remove · drag selected to move · right-click for actions · scroll to zoom" + } else { + "Shift-click to add/remove · right-click for selection actions · Esc clears selection" + } } else if promoted { "Expand endpoint containers to edit this connection route" } else if edge.is_some() { "Right-click for routing · drag line / bend / label · Insert adds a bend · Esc cancels" } else { - "Right-click for actions · drag to arrange · scroll to zoom" + "Shift-click to add/remove · right-click for actions · drag to arrange · scroll to zoom" }; - // Status is outside the measured canvas. Clipping prevents long text - // from painting over work headers or the dedicated destination slot. - tools = tools.push(container(label(hint)).height(30).width(Fill).clip(true)); + // Selection changes must not resize the measured canvas or refit its + // camera. Reserve both rows even when no group actions are available. + tools = tools.push( + container( + column![ + selection_controls, + label(hint).wrapping(widget::text::Wrapping::None) + ] + .spacing(3), + ) + .height(56) + .width(Fill) + .clip(true), + ); } column![container(tools).padding(14), canvas].into() } diff --git a/harness/multiselect-native-review-20260923.md b/harness/multiselect-native-review-20260923.md new file mode 100644 index 00000000..e34bcbf9 --- /dev/null +++ b/harness/multiselect-native-review-20260923.md @@ -0,0 +1,198 @@ +# Independent native evaluator review — 2026-09-23 + +Read-only review of `crates/oa-core/examples/multiselect_native.rs` at +`12a7352ddef5f69a144ead8219a50916233d63a9`, the local native preparation, +snapshot and capture helpers, and the owning session's proposed UI sequence. +Only this note was edited by the reviewer. No builds, tests, UI actions or +Gitea operations were run. The parent reports core, full desktop and lint gates +passed; no native run was available for acceptance at this review checkpoint. + +## Finding and correction + +**P2, corrected in the helper during review:** the original +`.local/multiselect-native/prepare-native.py:38–41` recorded the GUI exit code +and binary/fixture immutability booleans, then returned normal helper success +even if the GUI crashed or a recorded input changed. This could mislead a +consumer using process exit status as the gate. The parent added assertions at +lines 42–43 requiring GUI exit zero and all three immutability conditions after +writing the durable receipt. Re-reading the helper confirmed the correction. +No production source or build input changed for this correction, and it was +not runtime-tested by this reviewer. + +No additional actionable checker correctness defect was established. A native +acceptance result still requires the actual UI sequence and evidence below; +successful launcher completion alone is insufficient. + +## Independence and falsification coverage + +- The expected Project starts from the loaded baseline and changes explicitly + enumerated fixture records. Movement expects A, B and the child to move once, + translates only the internal route's pins, and preserves C, one-ended route + pins, attachments, labels, all semantic records, traces and other views. + Hide removes only A/B placements. It preserves the unselected child, stored + routes and model objects. Neither expectation invokes the group planner, + descendant traversal or routing implementation. +- `equal` compares the complete Project, not a selected geometry subset. + Validation, JSON roundtrip and source compilation/roundtrip supplement that + comparison. Completeness warnings in this deliberately partial fixture are + allowed; source discovery errors and compilation failures are rejected. +- The ten checker tests include positive generation/source/command examples + and multiple valid-model corruption falsifiers. The negative cases cover + an omitted or double-moved child, stranded internal pins, moved one-ended + pins, attachment/label drift, semantic and trace drift, other-view and fixed + node changes, invalid movement evidence, destructive Hide and unchanged-mode + semantic drift. Corruptions are validated before comparison and most tests + require the specific whole-Project mismatch, avoiding a fake pass caused + only by malformed input rejection. Planner use is confined to generating + positive/corrupted test candidates, not expected evaluator output. +- Fixture UUIDs are deterministic, generation refuses an existing output + directory, and evidence writes are exclusive. Preparation validates binary + hashes against successful unchanged-input build receipts and requires the + GUI and checker input-manifest hashes to match. The inspected checker + receipt names the reviewed commit; the GUI's then-current started receipt + records the same input hash. Final build/run results remain the parent's gate. +- The snapshot helper copies source into a unique directory, compares the + source file set and hashes before/after copying and after checking, pins the + checker and baseline, and propagates evaluator failure. Source snapshot + evaluation is separate from UI capture, as it should be. + +## Acceptance limits for the planned sequence + +The parent plans native context Add for A/B, actual drag, saved-model checks, +one-step Undo/Redo with exact source-byte comparison, undo back to baseline, +group Hide with Undo/Redo, and reopen. That is appropriate evidence for the +ordinary saved-view path with these explicit limits: + +1. **The movement checker is an invariant evaluator.** It infers the common + delta from saved A and checks bounds, nontrivial length and anchor grid + alignment. A wrong drag direction or magnitude can pass if all required + records share the same admissible delta. The code and manifest already + state this limit. Record native press/release and viewport observations if + claiming intended displacement accuracy; otherwise claim only the verified + common translation and preservation invariants. Existing pointer/gesture + tests provide separate evidence and are not native coordinate evidence. +2. **Model equality does not prove an action happened.** Capture one actual + Undo or Redo and completed Save between the appropriate snapshots. An + unchanged check alone cannot distinguish successful Undo from never making + an edit. Reopen must actually load the saved working source in the GUI and + demonstrate its state; checker roundtrip alone does not verify GUI reopening. +3. **Exact source-byte restoration is a separate comparison.** The snapshot + helper records each source inventory but does not compare different stages. + Explicitly compare all relative paths and hashes: Undo movement to the + original fixture source, Redo movement to the first moved snapshot, and + corresponding baseline/hidden inventories for Hide history. A Project PASS + does not establish identical source formatting or file layout. +4. **Join provenance rather than assuming it.** Tie the captured native window + to the launched PID/executable in `started.json`, identify the opened working + entry, retain the build/input receipts, and require the final fixture and + binary immutability checks. The checker verifies the expected fixture project + ID, not a baked-in baseline hash; baseline authenticity comes from the + generated fixture inventory and unchanged final receipt. Capture JSON and + screenshots alone do not establish which binary produced saved source. +5. **This fixture is OA Activities on an ordinary Overview.** Native context + Add verifies that explicit alternative, not held-Shift injection (unavailable + through the chosen native tool). Mounted focus tests separately exercise + Shift before canvas focus. The native run does not establish an SA Function + workflow, selected ancestor-plus-child deduplication, mixed-port/edge refusal, + busy suspension behavior or every cancellation path. Existing targeted tests + remain separate evidence for those cases; no additional broad native claims + should be inferred. +6. **Scope remains incomplete beyond ordinary saved views.** State, Allocation, + Process/Scenario and aggregate model deletion are not accepted by this + checker or planned native sequence. Their absence is not a new finding in + this bounded review. + +These limits qualify what the evidence proves. They do not require replacing +the independently enumerated Project oracle or broadening this native fixture +into the unfinished adapter families. + +## Native layout follow-up: selection changes move the canvas + +**P2 — group feedback changes the diagram viewport while merely selecting.** +The parent subsequently supplied native run-1 captures. I independently viewed +`.local/multiselect-native/run-1/06-select-a-0.jpg` and +`08-group-selected-0.jpg`; both capture the same native window at 1442 by 932. +The canvas begins near y=314 in the singleton capture and y=370 after forming +the group. A's header moves from approximately y=484 to y=511. The group count, +Hide/Clear actions and primary-properties explanation are readable, but adding +them has moved the diagram under the user's pointer. This is a presentation +finding; it does not assert that selecting changed saved geometry. + +The source cause is explicit: `views.rs:434–461` conditionally appends a two-row +group block above the View content control and canvas when the target count +exceeds one. The additional block is about 55 pixels high. The observed node +shift is smaller than the viewport-top shift, consistent with the canvas resize +and current fitted camera reacting to the reduced height. Removing the final +group member or clearing selection reverses the same conditional layout. + +The ordinary idle hint (`views.rs:654`) also mentions right-click, drag and zoom +but gives no visible Shift-click discovery cue; neither inspected screenshot +shows such a cue. Context Add is usable, but the user's requested modifier +gesture should be discoverable directly in the ordinary selection UI. + +### Smallest space-conscious correction + +Move the group feedback into an **always-mounted, fixed-height selection/status +strip at the existing fixed 30-pixel hint location** near `views.rs:658`. Remove +the conditional group block from the toolbar. A roughly 56-pixel strip can +contain: + +- A fixed approximately 32-pixel first row with selection count, the existing + group Hide/Clear controls when applicable, and the short explicit disclosure + `Properties edit only`. Use a bounded/nonwrapping primary label so + long names or a mixed selection cannot increase strip height. Empty and + singleton states keep the same row allocation even with fewer controls. +- A short second line: `Shift-click to add/remove · drag selected to move · + right-click for actions`. Existing active connection, allocation and route + instructions retain precedence when those modes own input; ordinary idle + selection should expose the modifier hint. + +This reuses the existing status location and consumes about 26 more vertical +pixels than today's singleton layout, while recovering about 29 pixels from +today's group layout. The shown central workspace is approximately 860 pixels +wide, sufficient for these compact controls and a bounded primary label without +adding a third row. A fixed outer height, fixed first-row height and nonwrapping +text keep the canvas rectangle identical for zero, one and multiple targets. +Keep this strip outside the canvas; an overlay would obscure diagram content +and complicate hit testing. Simply reserving the current extra 55-pixel toolbar +block would also stop the jump, but spends more permanent vertical space than +reusing the existing status strip. + +The narrow verification target is mounted canvas-bounds equality across +0→1→2→1→0 selection transitions, including mixed selection and a long primary +name, with unchanged model/history for those selection changes. A native +before/after capture should confirm stationary node screen positions and readable +controls at the same 1442-pixel window width. No source changes, build or native +interaction were performed by this reviewer for this follow-up. + +### Delegated layout correction + +The parent subsequently authorized edits to `views.rs`, +`app/diagram_focus_tests.rs` and this note. The conditional toolbar group block +is removed. Every active ordinary diagram now reserves a 56-pixel strip in the +former hint location with a fixed 32-pixel controls row. Count is always shown; +the existing group Hide/Clear intents and primary-only property explanation are +shown for groups. The primary name remains shortened and all strip text is +nonwrapping. Hide still requires whole-set placement admission and all App +event guards are unchanged. + +Idle selection hints expose Shift-click add/remove. Supported ordinary placement +groups advertise dragging selected elements together; mixed groups and Allocation +groups receive selection guidance without claiming a movement adapter they do +not have. Active allocation feedback and connection instructions retain priority; +singleton route/promoted-edge hints are preserved. The strip remains outside the +canvas with clipping, and no overlay or global layout policy was introduced. + +One mounted regression was added in source: +`mounted_selection_strip_keeps_canvas_and_fitted_camera_fixed_for_empty_single_group_and_mixed`. +It mounts the real App at 1442 by 932 with fitting enabled, a very long primary +name and a visible edge, then checks 0→1→2→3 mixed→2→1→0 selection transitions. +Each step must retain the exact measured canvas rectangle, zoom, pan, fit flag, +observed canvas size, complete Project, source bundle and empty history. +The previous implementation changes the canvas rectangle on the 1→2 transition, +so this verifies the observed regression through production layout and sensors. + +Only a static inspection and `git diff --check` were performed for these edits. +Formatting, compilation, regression execution and a fresh native before/after +capture remain owned by the parent. Native run-1 continues against its unchanged +pinned binary and cannot serve as verification of this subsequent source edit.