From b18f6ea008ea1efbd07da65ebb25d89b6e20cfa0 Mon Sep 17 00:00:00 2001 From: nrq6993-a11y Date: Thu, 15 Oct 2026 05:50:54 +0300 Subject: [PATCH 1/3] History panel: scroll to the current state on document change. Fixes storytold/photocraft#5909 when new edits land off-screen in a short dock. Co-authored-by: Cursor --- .../ui-egui/src/history_panel_select_tests.rs | 28 +++++++++++++++++++ crates/ui-egui/src/panels.rs | 11 ++++++++ 2 files changed, 39 insertions(+) diff --git a/crates/ui-egui/src/history_panel_select_tests.rs b/crates/ui-egui/src/history_panel_select_tests.rs index 92c458e37f..7c97dca106 100644 --- a/crates/ui-egui/src/history_panel_select_tests.rs +++ b/crates/ui-egui/src/history_panel_select_tests.rs @@ -58,3 +58,31 @@ fn clicking_a_state_jumps_there_with_one_command() { h.run_steps(2); assert_eq!(state(&h), (4, 0, rev + 2)); } + +#[test] +fn new_history_steps_scroll_the_current_state_into_view() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.run("file.new", json!({"width": 64, "height": 48})).unwrap(); + for n in 0..18 { + app.run("layer.new.layer", json!({"name": format!("Layer {n}")})).unwrap(); + } + let mut ready = false; + let mut h = Harness::builder().with_size(vec2(280.0, 120.0)).build_ui_state( + move |ui, app: &mut PhotocraftApp| { + if !ready { + PhotocraftApp::setup_context(ui.ctx(), theme::ThemeKind::ProMedium); + ready = true; + return; + } + history(app, ui); + }, + app, + ); + h.run_steps(4); + let row = h.get_all_by_label("New Layer").last().expect("the latest New Layer state").rect(); + let view = h.ctx.input(|i| i.viewport_rect()); + assert!( + view.contains_rect(row), + "the current history row must scroll into view: row {row:?} view {view:?}", + ); +} diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index 522c74dc15..46a0d4a66b 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -3088,6 +3088,7 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { return; }; let doc = st.doc.clone(); + let revision = st.revision; let entries = st.history.entries(); let current = entries.len() - 1; let redo: Vec = st.history.redo_labels().map(str::to_string).collect(); @@ -3118,10 +3119,13 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { // The dock gives History a fixed height: the rows scroll above the footer. let footer = if t.pro { widgets::footer_height(ui) + ui.spacing().item_spacing.y } else { 0.0 }; let max_h = (ui.available_height() - footer).max(40.0); + let follow_id = ui.id().with(("history-follow-rev", doc.id)); + let follow = ui.ctx().data(|d| d.get_temp::(follow_id)) != Some(revision); egui::ScrollArea::vertical().id_salt("history-rows").max_height(max_h).auto_shrink([false, false]).show(ui, |ui| { // Snapshots head the list and scroll with the states, so many of them never push the // footer out of a short dock. snap_cmd = snapshot_rows(app, ui, selected_snap); + let mut current_rect = None; for (i, (e, is_redo)) in all.enumerate() { // The current state shows selected unless a snapshot is (Photoshop highlights one row). let selected = i == current && selected_snap.is_none(); @@ -3132,6 +3136,9 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { if resp.drag_started() { ui.ctx().data_mut(|d| d.insert_temp(history_drag_key(), HistoryDrag::State(i))); } + if i == current { + current_rect = Some(rect); + } if selected { ui.painter().rect_filled(rect, if t.pro { 0.0 } else { t.radius_sm }, if t.pro { t.row_selected } else { t.accent_soft }); } else if resp.hovered() { @@ -3161,6 +3168,10 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { ctx.data_mut(|d| d.remove::(selected_snapshot_key(doc.id))); } } + if follow && let Some(rect) = current_rect { + ui.scroll_to_rect(rect, Some(egui::Align::BOTTOM)); + } + ui.ctx().data_mut(|d| d.insert_temp(follow_id, revision)); }); if let Some(drag) = drag && let Some(p) = ctx.input(|i| i.pointer.interact_pos()) From 8be3269fe1bf3baaa6cf76fbabf4c16de9b25974 Mon Sep 17 00:00:00 2001 From: nrq6993-a11y Date: Thu, 15 Oct 2026 13:51:51 +0300 Subject: [PATCH 2/3] style: rustfmt --- crates/ui-egui/src/history_panel_select_tests.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/ui-egui/src/history_panel_select_tests.rs b/crates/ui-egui/src/history_panel_select_tests.rs index 7c97dca106..5e744794f3 100644 --- a/crates/ui-egui/src/history_panel_select_tests.rs +++ b/crates/ui-egui/src/history_panel_select_tests.rs @@ -81,8 +81,5 @@ fn new_history_steps_scroll_the_current_state_into_view() { h.run_steps(4); let row = h.get_all_by_label("New Layer").last().expect("the latest New Layer state").rect(); let view = h.ctx.input(|i| i.viewport_rect()); - assert!( - view.contains_rect(row), - "the current history row must scroll into view: row {row:?} view {view:?}", - ); + assert!(view.contains_rect(row), "the current history row must scroll into view: row {row:?} view {view:?}",); } From f18e6189c72d05415f9a8f7e1d2354b084dbc9e9 Mon Sep 17 00:00:00 2001 From: David Mann Date: Wed, 21 Oct 2026 15:31:09 +0200 Subject: [PATCH 3/3] Integration: History follows the history, not every revision, and never moves a visible row (#5940) The panel scrolled with `scroll_to_rect(current, Some(Align::BOTTOM))` on every revision change. In egui an explicit alignment always realigns, and the revision also changes when nothing in the history does (selecting a layer, a channel eye). So clicking a visible older state slid it to the bottom under the pointer, Undo hid the greyed redo states, a layer click threw a user who had scrolled up back to the bottom, and selecting a snapshot scrolled it out of view. Now the panel follows the history position (document, past/redo counts, current name, and the newest past state, which also changes at the state limit), skips it while a snapshot is selected, and scrolls without alignment, so only a hidden current row moves into view. New tests: a clicked visible state stays put, Undo keeps the redo state in view, a layer click keeps the scroll, a selected snapshot stays in view, a new state at the limit is revealed; the first three fail on the PR head, the others without the snapshot guard / the newest-state key. Co-authored-by: nrq6993-a11y <342501791+nrq6993-a11y@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .../ui-egui/src/history_panel_select_tests.rs | 142 ++++++++++++++++++ crates/ui-egui/src/panels.rs | 21 ++- 2 files changed, 158 insertions(+), 5 deletions(-) diff --git a/crates/ui-egui/src/history_panel_select_tests.rs b/crates/ui-egui/src/history_panel_select_tests.rs index 5e744794f3..14bfb344ca 100644 --- a/crates/ui-egui/src/history_panel_select_tests.rs +++ b/crates/ui-egui/src/history_panel_select_tests.rs @@ -83,3 +83,145 @@ fn new_history_steps_scroll_the_current_state_into_view() { let view = h.ctx.input(|i| i.viewport_rect()); assert!(view.contains_rect(row), "the current history row must scroll into view: row {row:?} view {view:?}",); } + +/// Open plus 18 New Layer states in a dock that shows only a few rows. +fn long_harness() -> Harness<'static, PhotocraftApp> { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.run("file.new", json!({"width": 64, "height": 48})).unwrap(); + for n in 0..18 { + app.run("layer.new.layer", json!({"name": format!("Layer {n}")})).unwrap(); + } + let mut ready = false; + let mut h = Harness::builder().with_size(vec2(280.0, 220.0)).build_ui_state( + move |ui, app: &mut PhotocraftApp| { + if !ready { + PhotocraftApp::setup_context(ui.ctx(), theme::ThemeKind::ProMedium); + ready = true; + return; + } + history(app, ui); + }, + app, + ); + h.run_steps(4); + h +} + +/// Top of the `n`th New Layer row from the end (0 = the last). +fn row_top(h: &Harness<'_, PhotocraftApp>, n: usize) -> f32 { + let rows: Vec<_> = h.get_all_by_label("New Layer").map(|r| r.rect()).collect(); + rows[rows.len() - 1 - n].top() +} + +/// The rows' visible band: below the window top, above the footer. +fn shown(h: &Harness<'_, PhotocraftApp>, n: usize) -> bool { + let footer = h.get_by_label("Delete current state").rect().top(); + let top = row_top(h, n); + top >= h.ctx.input(|i| i.viewport_rect()).top() && top + 24.0 <= footer + 0.5 +} + +#[test] +fn clicking_a_visible_state_leaves_the_list_still() { + let mut h = long_harness(); + assert!(shown(&h, 0) && shown(&h, 1), "the newest states are in view"); + let before = row_top(&h, 1); + // A click on the state above the current one goes back one step; the row stays under the pointer. + let rows: Vec<_> = h.get_all_by_label("New Layer").collect(); + rows[rows.len() - 2].click(); + h.run_steps(4); + assert_eq!(h.state().session.active().unwrap().history.redo_labels().count(), 1); + assert_eq!(row_top(&h, 1), before, "the clicked row must not move"); +} + +#[test] +fn undo_and_a_layer_click_keep_the_redo_state_in_view() { + let mut h = long_harness(); + h.state_mut().run("edit.undo", json!({})).unwrap(); + h.run_steps(4); + assert!(shown(&h, 0), "the greyed redo state stays in view after Undo"); + let before = row_top(&h, 0); + // Selecting a layer bumps the revision but adds no state: the list stays where it is. + let id = h.state().session.active().unwrap().doc.layers[0].id; + h.state_mut().session.select_layer(id).unwrap(); + h.run_steps(4); + assert_eq!(row_top(&h, 0), before, "a layer click must not scroll History"); +} + +#[test] +fn scrolling_away_survives_a_change_that_adds_no_state() { + let mut h = long_harness(); + let view = h.ctx.input(|i| i.viewport_rect()); + h.event(egui::Event::PointerMoved(view.center())); + for _ in 0..3 { + h.event(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Line, + delta: vec2(0.0, 3.0), + phase: egui::TouchPhase::Move, + modifiers: Default::default(), + }); + h.run_steps(1); + } + h.run_steps(4); + let scrolled = row_top(&h, 0); + assert!(!shown(&h, 0), "the wheel scrolled the current state out of view"); + let id = h.state().session.active().unwrap().doc.layers[0].id; + h.state_mut().session.select_layer(id).unwrap(); + h.run_steps(4); + assert_eq!(row_top(&h, 0), scrolled, "the user's scroll position is kept"); + // A new state brings the current row back into view. + h.state_mut().run("layer.new.layer", json!({})).unwrap(); + h.run_steps(4); + assert!(shown(&h, 0), "a new state scrolls into view"); +} + +#[test] +fn clicking_a_snapshot_keeps_it_in_view() { + let mut h = long_harness(); + h.state_mut().run("history.newSnapshot", json!({})).unwrap(); + h.run_steps(2); + // Scroll to the top, where the snapshots are. + let view = h.ctx.input(|i| i.viewport_rect()); + h.event(egui::Event::PointerMoved(view.center())); + for _ in 0..10 { + h.event(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Line, + delta: vec2(0.0, 5.0), + phase: egui::TouchPhase::Move, + modifiers: Default::default(), + }); + h.run_steps(1); + } + h.run_steps(4); + let before = h.get_all_by_label("Snapshot 1").next().expect("the snapshot row").rect().top(); + // Selecting it adds a "Snapshot 1" state at the bottom; the selected snapshot row must not scroll away. + h.get_all_by_label("Snapshot 1").next().expect("the snapshot row").click(); + h.run_steps(4); + assert_eq!(h.state().session.active().unwrap().history.entries().last().map(String::as_str), Some("Snapshot 1")); + assert_eq!(h.get_all_by_label("Snapshot 1").next().expect("the snapshot row").rect().top(), before, "the clicked snapshot stays put"); +} + +#[test] +fn a_new_state_at_the_history_limit_still_scrolls_into_view() { + let mut h = long_harness(); + // At the limit a new step drops the oldest: the count and the (same) name stay as they were. + let st = h.state_mut().session.active_mut().unwrap(); + st.history.max_states = st.history.past_len(); + let view = h.ctx.input(|i| i.viewport_rect()); + h.event(egui::Event::PointerMoved(view.center())); + for _ in 0..3 { + h.event(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Line, + delta: vec2(0.0, 3.0), + phase: egui::TouchPhase::Move, + modifiers: Default::default(), + }); + h.run_steps(1); + } + h.run_steps(4); + assert!(!shown(&h, 0), "the wheel scrolled the current state out of view"); + let past = h.state().session.active().unwrap().history.past_len(); + h.state_mut().run("layer.new.layer", json!({})).unwrap(); + assert_eq!(h.state().session.active().unwrap().history.past_len(), past, "the oldest state was dropped"); + h.run_steps(4); + assert!(shown(&h, 0), "a new state at the limit scrolls into view"); +} diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index 46a0d4a66b..6a22b41f8e 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -3088,11 +3088,14 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { return; }; let doc = st.doc.clone(); - let revision = st.revision; let entries = st.history.entries(); let current = entries.len() - 1; let redo: Vec = st.history.redo_labels().map(str::to_string).collect(); let selected_snap = selected_snapshot(ui.ctx(), st); + // The newest past state is a new document on every step, Undo and Redo, also at the state + // limit, where the counts and a repeated name stay the same. + let newest = st.history.past_len().checked_sub(1).and_then(|i| st.history.state(i)).map(|d| std::sync::Arc::as_ptr(&d) as usize); + let spot = (doc.id, history_spot(st), newest); let ctx = ui.ctx().clone(); // A row dragged onto the trash (#5324); a drag that never saw its release must not outlive it. if !ctx.input(|i| i.pointer.primary_down() || i.pointer.any_released()) { @@ -3119,8 +3122,16 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { // The dock gives History a fixed height: the rows scroll above the footer. let footer = if t.pro { widgets::footer_height(ui) + ui.spacing().item_spacing.y } else { 0.0 }; let max_h = (ui.available_height() - footer).max(40.0); - let follow_id = ui.id().with(("history-follow-rev", doc.id)); - let follow = ui.ctx().data(|d| d.get_temp::(follow_id)) != Some(revision); + // Follow the current state when the history moves (a new step, Undo, a state row) or another + // document comes up (`spot`), scrolling only if its row is hidden (#5909). Other changes (a + // layer click, a channel eye) and a selected snapshot leave the list where the user put it. + let follow_id = egui::Id::new("history-follow"); + let moved = ui.ctx().data_mut(|d| { + let moved = d.get_temp::<(photocraft_doc::DocId, HistorySpot, Option)>(follow_id).as_ref() != Some(&spot); + d.insert_temp(follow_id, spot); + moved + }); + let follow = moved && selected_snap.is_none(); egui::ScrollArea::vertical().id_salt("history-rows").max_height(max_h).auto_shrink([false, false]).show(ui, |ui| { // Snapshots head the list and scroll with the states, so many of them never push the // footer out of a short dock. @@ -3169,9 +3180,9 @@ fn history(app: &mut PhotocraftApp, ui: &mut egui::Ui) { } } if follow && let Some(rect) = current_rect { - ui.scroll_to_rect(rect, Some(egui::Align::BOTTOM)); + // No alignment: a row already in view stays put. No animation, as the Layers panel reveal. + ui.scroll_to_rect_animation(rect, None, egui::style::ScrollAnimation::none()); } - ui.ctx().data_mut(|d| d.insert_temp(follow_id, revision)); }); if let Some(drag) = drag && let Some(p) = ctx.input(|i| i.pointer.interact_pos())