From 58a1f3df174ce9664563a72f2071bf5766129e04 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Tue, 10 Nov 2026 11:01:42 +0530 Subject: [PATCH] #2859: test the real marquee selection path The unit test re-implemented Marquee::selection's box/outline split, so it only covered `targets`. Move the anchor math into `Anchor::press` / `Anchor::selection` over a plain `View`, call it from the test, and add a scrolled-drag case that fails with a screen-fixed anchor. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/panels/timeline/marquee.rs | 54 +++++++++++++------ crates/ui-egui/src/panels/timeline/tests.rs | 25 +++++---- 2 files changed, 54 insertions(+), 25 deletions(-) diff --git a/crates/ui-egui/src/panels/timeline/marquee.rs b/crates/ui-egui/src/panels/timeline/marquee.rs index 1c64a7963..d4eb56c76 100644 --- a/crates/ui-egui/src/panels/timeline/marquee.rs +++ b/crates/ui-egui/src/panels/timeline/marquee.rs @@ -16,10 +16,32 @@ struct Marquee { rows: Arc>, graph_on: bool, layout: (Rect, f32, f32), + anchor: Anchor, + dragging: bool, +} + +/// The pressed corner in content coordinates: a time (or an outline x) and a scroll-free y. +#[derive(Clone, Copy)] +pub(super) struct Anchor { time: f64, y: f32, outline_x: Option, - dragging: bool, +} + +/// The timeline layout a marquee is projected through. +#[derive(Clone, Copy)] +pub(super) struct View { + pub tm: TMap, + /// Screen y of the first row at scroll 0. + pub top: f32, + pub graph_x0: f32, + pub graph_on: bool, +} + +impl View { + fn of(g: &Rw) -> Self { + Self { tm: g.tl.tm, top: g.tl.rows_rect.min.y, graph_x0: g.tl.graph_x0, graph_on: g.graph_on } + } } fn id() -> Id { @@ -65,9 +87,7 @@ pub(super) fn prepare(app: &EffectcraftApp, ui: &egui::Ui, g: &Rw) { rows: g.tl.rows.clone(), graph_on: g.graph_on, layout: (g.tl.rect, g.tl.rh, g.tl.graph_x0), - time: g.tl.tm.t(p.x.max(g.tl.graph_x0)), - y: p.y - g.tl.rows_rect.min.y + app.ui.timeline.scroll_y, - outline_x: (p.x < g.tl.graph_x0).then_some(p.x), + anchor: Anchor::press(&View::of(g), app.ui.timeline.scroll_y, p), dragging: false, }, ); @@ -98,19 +118,21 @@ pub(super) struct SelectionBox { pub outline: bool, } -impl Marquee { +impl Anchor { + /// Anchor a press at screen point `p` with the rows scrolled by `scroll`. + pub(super) fn press(v: &View, scroll: f32, p: Pos2) -> Self { + Self { time: v.tm.t(p.x.max(v.graph_x0)), y: p.y - v.top + scroll, outline_x: (p.x < v.graph_x0).then_some(p.x) } + } + /// Reproject the content anchor without clipping the logical selection bounds. - fn selection(&self, g: &Rw, scroll: f32, pointer: Pos2) -> (Rect, SelectionBox) { - let tm = g.tl.tm; - let anchor = pos2(self.outline_x.unwrap_or_else(|| tm.x(self.time)), g.tl.rows_rect.min.y + self.y - scroll); + pub(super) fn selection(&self, v: &View, scroll: f32, pointer: Pos2) -> (Rect, SelectionBox) { + let tm = v.tm; + let anchor = pos2(self.outline_x.unwrap_or_else(|| tm.x(self.time)), v.top + self.y - scroll); let paint = Rect::from_two_pos(anchor, pointer); - let graph = self.outline_x.is_none() || pointer.x >= g.tl.graph_x0; - let keys = if graph && !g.graph_on { - Rect::from_two_pos(pos2(tm.x(self.time), anchor.y), pos2(pointer.x.max(g.tl.graph_x0), pointer.y)) - } else { - Rect::NOTHING - }; - (paint, SelectionBox { keys, outline: self.outline_x.is_some() || pointer.x < g.tl.graph_x0 }) + let graph = self.outline_x.is_none() || pointer.x >= v.graph_x0; + let keys = + if graph && !v.graph_on { Rect::from_two_pos(pos2(tm.x(self.time), anchor.y), pos2(pointer.x.max(v.graph_x0), pointer.y)) } else { Rect::NOTHING }; + (paint, SelectionBox { keys, outline: self.outline_x.is_some() || pointer.x < v.graph_x0 }) } } @@ -151,7 +173,7 @@ pub(super) fn finish(app: &EffectcraftApp, ui: &egui::Ui, g: &mut Rw) { return; } let Some(pointer) = ctx.input(|i| i.pointer.latest_pos()) else { return }; - let (paint, selection) = drag.selection(g, app.ui.timeline.scroll_y, pointer); + let (paint, selection) = drag.anchor.selection(&View::of(g), app.ui.timeline.scroll_y, pointer); if dropped { ctx.data_mut(|d| d.remove::(id())); let (layers, keys) = targets(&g.tl.comp, &g.tl.rows, &g.tl.tm, g.tl.rows_rect.min.y - app.ui.timeline.scroll_y, g.tl.rh, paint, &selection); diff --git a/crates/ui-egui/src/panels/timeline/tests.rs b/crates/ui-egui/src/panels/timeline/tests.rs index d4a801160..e42923e4a 100644 --- a/crates/ui-egui/src/panels/timeline/tests.rs +++ b/crates/ui-egui/src/panels/timeline/tests.rs @@ -960,25 +960,32 @@ fn a_box_over_both_columns_selects_the_rows_it_crosses_and_the_keys_inside() { } // Rows are 20 pt and 400 pt wide: the outline is left of x 200, a key at 0 s sits at x 205. let tm = TMap { x0: 205.0, start: 0.0, pps: 100.0 }; - let go = |br: Rect| { - let keys = if br.max.x < 200.0 { Rect::NOTHING } else { Rect::from_min_max(pos2(br.min.x.max(200.0), br.min.y), br.max) }; - marquee::targets(comp, &rows, &tm, 0.0, 20.0, br, &marquee::SelectionBox { keys, outline: br.min.x < 200.0 }) + let view = marquee::View { tm, top: 0.0, graph_x0: 200.0, graph_on: false }; + // Press at `from` with the rows scrolled by `s0`, release at `to` scrolled by `s1`. + let drag = |from: Pos2, s0: f32, to: Pos2, s1: f32| { + let (paint, sel) = marquee::Anchor::press(&view, s0, from).selection(&view, s1, to); + marquee::targets(comp, &rows, &tm, -s1, 20.0, paint, &sel) }; + let go = |from: Pos2, to: Pos2| drag(from, 0.0, to, 0.0); let keyed = |ks: &[effectcraft_engine::KeyRef]| ks.iter().map(|k| k.layer.0).collect::>(); // From the outline across the boundary: layer A's row (y 0..20) and B's row, and their keys. - let (ls, ks) = go(Rect::from_min_max(pos2(10.0, 5.0), pos2(260.0, 65.0))); + let (ls, ks) = go(pos2(10.0, 5.0), pos2(260.0, 65.0)); assert_eq!(ls, [layers[0], layers[1]]); assert_eq!(keyed(&ks), [layers[0], layers[1]]); - // The same box dragged the other way is the same rectangle: the same rows, the same keys. - let (ls, ks) = go(Rect::from_two_pos(pos2(260.0, 65.0), pos2(10.0, 5.0))); + // The same box dragged the other way: the same rows, the same keys. + let (ls, ks) = go(pos2(260.0, 65.0), pos2(10.0, 5.0)); assert_eq!((ls, keyed(&ks)), (vec![layers[0], layers[1]], vec![layers[0], layers[1]])); // Only over the outline: rows, no keys; only over the graph: keys, and the rows crossed. - let (ls, ks) = go(Rect::from_min_max(pos2(10.0, 5.0), pos2(150.0, 65.0))); + let (ls, ks) = go(pos2(10.0, 5.0), pos2(150.0, 65.0)); assert_eq!((ls.len(), ks.len()), (2, 0), "a key under the outline isn't touched"); // Kept inside the time graph: keys only, no rows (a marquee over empty track area). - let (ls, ks) = go(Rect::from_min_max(pos2(204.0, 5.0), pos2(260.0, 65.0))); + let (ls, ks) = go(pos2(204.0, 5.0), pos2(260.0, 65.0)); assert_eq!((ls.len(), keyed(&ks)), (0, vec![layers[0], layers[1]])); // A locked layer is not selected as a row (the engine drops its keys in keys.select). - let (ls, _) = go(Rect::from_min_max(pos2(10.0, 5.0), pos2(260.0, 119.0))); + let (ls, _) = go(pos2(10.0, 5.0), pos2(260.0, 119.0)); + assert_eq!(ls, [layers[0], layers[1]]); + // Scrolled 20 pt mid-drag: the pressed corner stays on layer A's row (#2859), so A is still + // selected; a screen-fixed anchor would start the box below A and select only B. + let (ls, _) = drag(pos2(10.0, 5.0), 0.0, pos2(260.0, 45.0), 20.0); assert_eq!(ls, [layers[0], layers[1]]); }