From 103f53724fd9708cdcfcab88fc248aa121e83f53 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 02:55:30 -0400 Subject: [PATCH] LR-FILMSTRIP: grid and filmstrip keep the user's scroll position; wheel scrolls the filmstrip Fixes #11. Root cause: the grid and the filmstrip remembered the last active photo in egui temp data with mismatched types (inserted `Option`, read back as `PhotoId` because of the `let last: Option = get_temp(..)` annotation), so the read always missed, "the active photo changed" was true on every frame and both called `scroll_to_rect` on the active photo every frame. Any wheel or scroll-bar scroll was undone as soon as the user stopped (the animated scroll back read as a delayed snap). - `grid::follow_active` (shared by grid and filmstrip): scroll to the active photo only when it changed since the strip was last drawn, or when the strip was not drawn on the previous pass (view switch). Time passing, finished renders, edits and a released scroll bar no longer move either strip. - Filmstrip: a vertical mouse wheel scrolls it sideways (horizontal trackpad scrolling still works); it recentres on a new active photo only when that is off screen, so a click on a visible thumbnail leaves it alone. - `ui.inspect` reports `scroll: {grid, filmstrip}`; headless tests in `tests_scroll.rs`. - docs/parity.md (LR-VIEW-PHOTOGRID, LR-VIEW-FILMSTRIP), docs/control-protocol.md. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/control.rs | 1 + crates/ui-egui/src/lib.rs | 8 ++ crates/ui-egui/src/panels/detail.rs | 13 ++-- crates/ui-egui/src/panels/grid.rs | 28 +++++-- crates/ui-egui/src/tests_scroll.rs | 117 ++++++++++++++++++++++++++++ docs/control-protocol.md | 2 +- docs/parity.md | 4 +- 7 files changed, 159 insertions(+), 14 deletions(-) create mode 100644 crates/ui-egui/src/tests_scroll.rs diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 613b17e7..33a7a497 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -73,6 +73,7 @@ pub fn inspect(app: &LightcraftApp, ctx: &egui::Context) -> Value { "pixelsPerPoint": ctx.pixels_per_point(), "canvasRect": app.canvas_rect.map(rect_json), "imageRect": app.image_rect.map(rect_json), + "scroll": {"grid": app.grid_scroll, "filmstrip": app.film_scroll}, "active": app.session.active().map(|p| p.0), "selection": app.session.selection.ids.iter().map(|p| p.0).collect::>(), "activeMask": app.session.active_mask, diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 6a67bd16..13a0e09d 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -24,6 +24,8 @@ pub mod widgets; #[cfg(test)] mod tests_masking; +#[cfg(test)] +mod tests_scroll; use std::sync::mpsc::{Receiver, Sender}; @@ -114,6 +116,10 @@ pub struct LightcraftApp { /// Rect of the photo canvas and the displayed image (screen points) from the last frame. pub canvas_rect: Option, pub image_rect: Option, + /// Scroll offsets (points) of the photo grid (vertical) and the filmstrip (horizontal) as + /// drawn last (`ui.inspect` → `scroll`). + pub grid_scroll: Option, + pub film_scroll: Option, /// Widget registry from the last frame (automation ids → rects). pub widgets: Vec<(String, egui::Rect)>, /// In-progress on-canvas gesture (brush stroke points, gradient drag…). @@ -169,6 +175,8 @@ impl LightcraftApp { last_time: 0.0, canvas_rect: None, image_rect: None, + grid_scroll: None, + film_scroll: None, widgets: vec![], gesture: None, loupe_shown: None, diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index a869dbd6..81cee455 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -1201,17 +1201,20 @@ pub(crate) fn filmstrip(app: &mut LightcraftApp, ui: &mut egui::Ui, r: Rect) { let cell_w = 120.0; let ppp = ui.ctx().pixels_per_point(); let mut child = ui.new_child(egui::UiBuilder::new().max_rect(r.shrink2(vec2(0.0, 4.0)))); - let scroll_key = egui::Id::new("film-last-active"); - let last: Option = child.data(|d| d.get_temp(scroll_key)); - child.data_mut(|d| d.insert_temp(scroll_key, active)); + // a vertical mouse wheel scrolls the strip sideways (horizontal trackpad scrolls still work) + child.style_mut().always_scroll_the_only_direction = true; + // only a new active photo (or the strip appearing) scrolls it; see `grid::follow_active` + let follow = super::grid::follow_active(child.ctx(), egui::Id::new("film-follow-active"), active); egui::ScrollArea::horizontal().id_salt("filmstrip").auto_shrink([false, false]).show_viewport(&mut child, |ui, vp| { let (area, _) = ui.allocate_exact_size(vec2(ids.len() as f32 * cell_w, r.height() - 16.0), Sense::hover()); + app.film_scroll = Some(vp.left()); for (i, id) in ids.iter().enumerate() { let cr = Rect::from_min_size(pos2(area.left() + i as f32 * cell_w, area.top()), vec2(cell_w, area.height())); - if Some(*id) == active && last != active { + let local = Rect::from_min_size(pos2(i as f32 * cell_w, 0.0), cr.size()); + // centred when it is (partly) off screen; a click on a visible thumbnail leaves the strip alone + if follow && Some(*id) == active && !(local.left() >= vp.left() && local.right() <= vp.right()) { ui.scroll_to_rect(cr, Some(egui::Align::Center)); } - let local = Rect::from_min_size(pos2(i as f32 * cell_w, 0.0), cr.size()); if !local.intersects(vp.expand2(vec2(cell_w * 4.0, 0.0))) { continue; } diff --git a/crates/ui-egui/src/panels/grid.rs b/crates/ui-egui/src/panels/grid.rs index 115f6c77..23db3687 100644 --- a/crates/ui-egui/src/panels/grid.rs +++ b/crates/ui-egui/src/panels/grid.rs @@ -102,18 +102,19 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { let cells: Vec = ids.iter().zip(&lay.cells).map(|(id, r)| Cell { id: *id, rect: *r }).collect(); let total_h = lay.height + 12.0; let active = app.session.selection.active; - // keep the active photo in view when it changes by keyboard - let scroll_to: Option = { - let key = egui::Id::new("grid-last-active"); - let last: Option = ui.data(|d| d.get_temp(key)); - ui.data_mut(|d| d.insert_temp(key, active)); - if last != active { active.and_then(|a| cells.iter().find(|c| c.id == a).map(|c| c.rect)) } else { None } + // bring the active photo into view when it changes (keyboard, click, command) or the grid + // comes back on screen; otherwise the scroll position is the user's + let scroll_to: Option = if follow_active(ui.ctx(), egui::Id::new("grid-follow-active"), active) { + active.and_then(|a| cells.iter().find(|c| c.id == a).map(|c| c.rect)) + } else { + None }; let stacks = app.session.catalog.stack_index(); let mut visible_ids = HashSet::new(); egui::ScrollArea::vertical().id_salt("grid-scroll").auto_shrink([false, false]).show_viewport(ui, |ui, viewport| { let (area, _) = ui.allocate_exact_size(vec2(ui.available_width(), total_h), Sense::hover()); let origin = area.min; + app.grid_scroll = Some(viewport.top()); if let Some(r) = scroll_to { ui.scroll_to_rect(r.translate(origin.to_vec2()), None); } @@ -154,6 +155,21 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { let _ = (Color32::BLACK, StrokeKind::Inside, Stroke::NONE); } +/// Should a scrolling photo strip (`key`: the grid, the filmstrip) bring the active photo into +/// view on this pass? Yes when the active photo changed since the strip was last drawn, or when +/// the strip was not drawn on the previous pass (view switch, panel shown again). Otherwise the +/// scroll position is the user's: time passing, finished renders, edits to the photo or a released +/// scroll bar never move it (issue #11). +pub(crate) fn follow_active(ctx: &egui::Context, key: egui::Id, active: Option) -> bool { + let pass = ctx.cumulative_pass_nr(); + let last: Option<(Option, u64)> = ctx.data(|d| d.get_temp(key)); + ctx.data_mut(|d| d.insert_temp(key, (active, pass))); + match last { + Some((was, drawn)) => was != active || drawn + 1 < pass, + None => true, + } +} + /// Height of a date header row (points). pub const HEADER_H: f32 = 40.0; diff --git a/crates/ui-egui/src/tests_scroll.rs b/crates/ui-egui/src/tests_scroll.rs new file mode 100644 index 00000000..05167c47 --- /dev/null +++ b/crates/ui-egui/src/tests_scroll.rs @@ -0,0 +1,117 @@ +//! Headless tests of the photo grid's and the filmstrip's scrolling (issue #11): the user's +//! scroll position stays put until the active photo changes; the mouse wheel scrolls the +//! filmstrip sideways. + +use std::time::Duration; + +use serde_json::json; + +use crate::headless::Headless; +use crate::{LightcraftApp, Services}; + +const T: Duration = Duration::from_secs(20); +const SETTLE: Duration = Duration::from_secs(120); + +fn demo(view: &str) -> Headless { + let services = Services { png: None, ..Default::default() }; + let app = LightcraftApp::new(lightcraft_engine::Session::with_demo(), services); + let mut h = Headless::new(app, [1200.0, 800.0], 1.0); + // big thumbnails: the demo library is several screens tall + let r = h.request("ui.set", json!({"view": view, "thumbSize": 480.0}), T); + assert_eq!(r["ok"], true, "{r}"); + let first = h.app.session.visible_cloned()[0].0; + let r = h.request("engine.execute", json!({"command": "library.select", "params": {"ids": [first]}}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + h +} + +/// Run `n` frames (plus a settle: thumbnails and renders finishing must not move anything). +fn idle(h: &mut Headless, n: usize) { + h.settle(SETTLE); + for _ in 0..n { + h.step(); + } +} + +fn grid_y(h: &Headless) -> f32 { + h.app.grid_scroll.expect("grid drawn") +} + +fn film_x(h: &Headless) -> f32 { + h.app.film_scroll.expect("filmstrip drawn") +} + +fn widget(h: &Headless, id: &str) -> egui::Rect { + h.app.widgets.iter().find(|(w, _)| w == id).map(|(_, r)| *r).unwrap_or_else(|| panic!("no widget {id}")) +} + +#[test] +fn grid_keeps_the_users_scroll_position() { + let mut h = demo("photoGrid"); + assert_eq!(grid_y(&h), 0.0); + let canvas = h.app.canvas_rect.expect("grid canvas"); + // the mouse wheel scrolls the grid away from the active (first) photo… + h.request("ui.move", json!({"x": canvas.center().x, "y": canvas.center().y}), T); + for _ in 0..4 { + h.request("ui.scroll", json!({"dy": -300.0}), T); + } + idle(&mut h, 120); + let y = grid_y(&h); + assert!(y > 600.0, "the wheel scrolled the grid ({y})"); + // …and it stays there: no snap back after a second and more of frames and finished renders + idle(&mut h, 120); + assert_eq!(grid_y(&h), y, "the grid snapped back"); + // dragging the scroll bar back up a bit, then releasing it + let x = canvas.right() - 3.0; + let r = h.request("ui.drag", json!({"x": x, "y": canvas.top() + 300.0, "toX": x, "toY": canvas.top() + 200.0}), T); + assert_eq!(r["ok"], true, "{r}"); + idle(&mut h, 120); + let y2 = grid_y(&h); + assert!(y2 > 0.0 && (y2 - y).abs() > 1.0, "the scroll bar moved the grid ({y} → {y2})"); + // editing the active photo does not yank the grid back either + let r = h.request("engine.execute", json!({"command": "develop.set", "params": {"control": "light.exposure", "value": 0.5}}), T); + assert_eq!(r["ok"], true, "{r}"); + idle(&mut h, 60); + assert_eq!(grid_y(&h), y2, "an edit snapped the grid back"); + // a new active photo (keyboard) brings it into view + h.request("ui.key", json!({"key": "right"}), T); + idle(&mut h, 60); + let second = h.app.session.visible_cloned()[1].0; + assert_eq!(h.app.session.active().map(|p| p.0), Some(second)); + let cell = widget(&h, &format!("thumb:{second}")); + assert!(canvas.contains_rect(cell) || cell.intersects(canvas), "the new active photo is in view ({cell:?} vs {canvas:?})"); + assert!(grid_y(&h) < y2, "scrolled up to the second photo"); +} + +#[test] +fn filmstrip_wheel_scrolls_and_keeps_its_position() { + let mut h = demo("detail"); + let first = h.app.session.visible_cloned()[0].0; + let x0 = film_x(&h); + let film = widget(&h, &format!("film:{first}")); + // a vertical wheel over the filmstrip scrolls it sideways + h.request("ui.move", json!({"x": 600.0, "y": film.center().y}), T); + for _ in 0..3 { + h.request("ui.scroll", json!({"dy": -200.0}), T); + } + idle(&mut h, 60); + let x = film_x(&h); + assert!(x > x0 + 300.0, "the wheel scrolled the filmstrip ({x0} → {x})"); + // a horizontal (trackpad) scroll still works + h.request("ui.scroll", json!({"dx": 100.0}), T); + idle(&mut h, 60); + let x1 = film_x(&h); + assert!(x1 < x - 50.0, "a horizontal scroll moved it back ({x} → {x1})"); + // it stays there + idle(&mut h, 120); + assert_eq!(film_x(&h), x1, "the filmstrip snapped back"); + // the next photo by keyboard brings the filmstrip to it + h.request("ui.key", json!({"key": "right"}), T); + idle(&mut h, 60); + let second = h.app.session.visible_cloned()[1].0; + assert_eq!(h.app.session.active().map(|p| p.0), Some(second)); + assert!(film_x(&h) < x1, "scrolled back to the active photo"); + let cell = widget(&h, &format!("film:{second}")); + assert!(cell.left() >= 0.0 && cell.right() <= 1200.0, "the new active photo is in view ({cell:?})"); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 2b4c9884..186adf40 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -21,7 +21,7 @@ mode ([mcp.md](mcp.md)) is a thin layer over this channel. Implementation: | `engine.commands` | — | Engine + UI commands: id, label, menu, shortcut, params doc, enabled | | `ui.menu.list` | — | Menu entries (flat: id, label, menu path, shortcut, enabled) | | `ui.menu.tree` | — | The menu bar as shown (File … Help): items `{id, params?, label, shortcut?, enabled, checked?}`, separators, submenus — the model behind the native macOS menu bar and the in-window menus | -| `ui.inspect` | — | UI state, window, canvas/image rects, active photo, selection, perf, status, memory (bytes per cache, see `library.memory`; plus stage caches and textures), `export: {running: {total, done, current} \| null, last}` | +| `ui.inspect` | — | UI state, window, canvas/image rects, `scroll: {grid, filmstrip}` (scroll offsets in points, `null` until drawn), active photo, selection, perf, status, memory (bytes per cache, see `library.memory`; plus stage caches and textures), `export: {running: {total, done, current} \| null, last}` | | `ui.widgets` | `{filter?}` | On-screen widgets `{id, rect: [x, y, w, h]}` (screen points) | | `ui.clickWidget` / `ui.dragWidget` / `ui.hoverWidget` | `{id, count?, fx?, fy?}` / `{id, toX?, toY?, dx?, dy?, steps?}` / `{id, fx?, fy?}` | Real egui input on a widget (hover: the pointer rests on it, e.g. for preset/profile previews) | | `ui.move` / `ui.click` / `ui.drag` | `{x, y, count?, button?}` / `{x, y, toX, toY, steps?}` | Raw pointer input, screen points | diff --git a/docs/parity.md b/docs/parity.md index 171b5aa0..be341962 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -149,12 +149,12 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | Id | Feature | Tier | Status | Evidence | Missing / notes | |---|---|---|---|---|---| -| LR-VIEW-PHOTOGRID | Justified photo grid | P0 | ✅ | `cmd:view.photoGrid`, `cmd:library.groups`, `cmd:library.sort` (`group`), `crates/ui-egui/src/panels/grid.rs`, `crates/catalog/src/dates.rs` | grouped by capture / import / edit date (day headers “Wednesday, 30 September 2026 · 12 photos”, months or years when zoomed out, or a fixed day/month/year/none choice in the sort menu); sticky header; clicking a header selects its photos; stacks never split | +| LR-VIEW-PHOTOGRID | Justified photo grid | P0 | ✅ | `cmd:view.photoGrid`, `cmd:library.groups`, `cmd:library.sort` (`group`), `crates/ui-egui/src/panels/grid.rs`, `crates/catalog/src/dates.rs` | grouped by capture / import / edit date (day headers “Wednesday, 30 September 2026 · 12 photos”, months or years when zoomed out, or a fixed day/month/year/none choice in the sort menu); sticky header; clicking a header selects its photos; stacks never split; scrolls to the active photo only when it changes (or the grid comes back on screen), never on its own | | LR-VIEW-SQUAREGRID | Square grid | P0 | ✅ | `cmd:view.squareGrid` | same date headers as the photo grid | | LR-VIEW-DETAIL | Single-photo view | P0 | ✅ | `cmd:view.detail`, `crates/ui-egui/src/panels/detail.rs` | | | LR-VIEW-EDIT | Edit view | P0 | ✅ | `cmd:panel.edit` | | | LR-VIEW-FULLSCREEN | Full-screen preview | P1 | ✅ | `cmd:view.fullScreenPreview`, `cmd:view.enterFullScreen` | photo on black, arrows step, Esc exits; ⇧⌘F window full screen | -| LR-VIEW-FILMSTRIP | Filmstrip | P0 | ✅ | `cmd:view.filmstrip` | | +| LR-VIEW-FILMSTRIP | Filmstrip | P0 | ✅ | `cmd:view.filmstrip`, `crates/ui-egui/src/panels/detail.rs` | the mouse wheel scrolls it sideways; it follows the active photo only when that changes (centred if off screen), so a scrolled strip stays put | | LR-VIEW-ZOOM | Zoom & pan | P0 | ✅ | `cmd:view.zoomFit`, `cmd:view.zoom100`, `cmd:view.zoomIn`, `cmd:view.zoomOut`, `cmd:view.zoomToggle`, `cmd:view.clickZoom` | steps 25–800 % (not 6–1600 %); Fill only in the bottom bar; a click on the photo (and Z / Space) eases to the click-zoom ratio (1:1 default, 2:1, 3:1, 4:1, 8:1 in the bottom bar) | | LR-VIEW-NAVIGATOR | Navigator mini map | P1 | ✅ | `cmd:view.navigator`, `crates/ui-egui/src/panels/detail.rs` | shown while zoomed (bottom right); click/drag pans | | LR-VIEW-BEFOREAFTER | Before / after | P0 | ✅ | `cmd:view.showOriginal`, `cmd:view.beforeAfter`, `cmd:view.beforeAfterSplit`, `cmd:view.beforeAfterTopBottom`, `cmd:view.beforeAfterSplitTopBottom`, `cmd:beforeAfter.setBefore`, `cmd:beforeAfter.copyAfterToBefore`, `cmd:beforeAfter.copyBeforeToAfter`, `cmd:beforeAfter.swap`, `cmd:beforeAfter.resetBefore` | all four layouts; before = the import state (defaults + import preset) or a chosen history step / version / the current settings; copy and swap (View → Before/After Settings, History row menu). The chosen before lasts for the session |