From 1688b43ae225fc4f89879d57ebacba66c7908a5a Mon Sep 17 00:00:00 2001 From: Zinadin Zidan Date: Sat, 7 Nov 2026 00:27:07 +0600 Subject: [PATCH] View: a sheet keeps its own scroll position when sheets move (#3485) Scroll positions were stored by (window, sheet index), and nothing re-keyed them when sheets were inserted, deleted or moved. A new PivotTable sheet, which goes in front of its source, opened at the source's position, and the source took over whatever was stored at its new index. Positions are now keyed by (window, sheet uid), so they follow the sheet through inserts, deletes, moves and undo. A closed window's positions are dropped. Saving the active cell still happens when no window is active. Co-Authored-By: Olivier <5681030+tutis@users.noreply.github.com> --- ROADMAP.md | 2 + crates/ui-egui/src/app/commands.rs | 1 + crates/ui-egui/src/app/view.rs | 16 +++-- crates/ui-egui/src/lib.rs | 5 +- crates/ui-egui/tests/it/scroll_position.rs | 83 ++++++++++++++++++++++ 5 files changed, 101 insertions(+), 6 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 59b3bf2d8..7a794e06e 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -135,6 +135,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-11-06 | A sheet keeps its own scroll position when sheets are inserted, deleted or moved before it: a new PivotTable or inserted sheet opens at the top instead of where its neighbour was scrolled; a closed window's positions are let go (#3485) | | 2026-11-02 | Form controls: Insert › Controls places buttons, sliders (scroll bars) and spin buttons on the sheet. A slider or spin button holds a whole number from 0 to 30000 in a linked cell (a drag or a held arrow is one undo step, and editing the cell moves the control); a button runs a macro or one command through the macro policy, asking first in a workbook opened from a file; Assign Action and Format Control edit them, and XLSX files read and write them (control properties, `` and VML), alongside the form controls GridCraft keeps unchanged | | 2026-11-02 | Dialogs take ranges by selection on the grid: reference boxes (RefEdit) with a collapse button that shrinks the dialog while the person selects, in Goal Seek, Solver (including the Model Range of Load/Save) and Data Analysis; while collapsed, the arrow keys move the selection and Shift+arrow extends it, and F4 cycles a reference's anchors | | 2026-11-02 | Python mode in the formula bar: Ctrl+Alt+Shift+P, `=PY` then Tab, or editing a Python cell edits its code in the formula bar, with Enter for a new line, Tab to indent and Ctrl+Enter to commit through `python.set`. The code still doesn't run | @@ -259,6 +260,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-11-06 | trivial | Progress log: scroll positions follow their sheet through sheet inserts, deletes and moves (#3485) | | 2026-11-02 | minor | Form controls; catalog 277 / 292 → 281 / 296 (Developer › Controls); progress log row | | 2026-11-02 | minor | Progress log: RefEdit reference boxes in dialogs | | 2026-11-02 | minor | Progress log: Python mode in the formula bar | diff --git a/crates/ui-egui/src/app/commands.rs b/crates/ui-egui/src/app/commands.rs index 4aba80024..6dff45cb2 100644 --- a/crates/ui-egui/src/app/commands.rs +++ b/crates/ui-egui/src/app/commands.rs @@ -302,6 +302,7 @@ impl SheetApp { // The requests belong to the window the command left active. self.follow_active_window(); self.prune_selected_object(); + self.prune_views(); for req in self.session.take_ui_requests() { match req { UiRequest::Dialog(name, params) => self.open_dialog(&name, params), diff --git a/crates/ui-egui/src/app/view.rs b/crates/ui-egui/src/app/view.rs index aea36abc2..482372448 100644 --- a/crates/ui-egui/src/app/view.rs +++ b/crates/ui-egui/src/app/view.rs @@ -7,8 +7,8 @@ impl SheetApp { } /// The active window and sheet: each window scrolls on its own. - fn scroll_key(&self) -> Option<(u64, usize)> { - Some((self.session.active_window()?, self.session.active()?.wb.active_sheet())) + fn scroll_key(&self) -> Option<(u64, u64)> { + Some((self.session.active_window()?, self.session.active()?.wb.active()?.uid.get())) } pub fn view(&self) -> SheetView { @@ -41,8 +41,10 @@ impl SheetApp { let window = self.session.active_window(); let Some(d) = self.session.active_mut() else { return }; let mut tops = Vec::new(); - for (&(w, si), v) in &self.views { - let Some(sh) = d.wb.sheets.get(si).filter(|sh| Some(w) == window && sh.view == gridcraft_engine::model::SheetViewMode::Normal) else { + for (si, sh) in d.wb.sheets.iter().enumerate() { + let Some(v) = + window.and_then(|w| self.views.get(&(w, sh.uid.get()))).filter(|_| sh.view == gridcraft_engine::model::SheetViewMode::Normal) + else { continue; }; let (fr, fc) = sh.freeze.unwrap_or((0, 0)); @@ -53,4 +55,10 @@ impl SheetApp { } d.store_view(&tops); } + + /// Drops the scroll positions of windows that have closed: window ids are never reused. + pub(super) fn prune_views(&mut self) { + let session = &self.session; + self.views.retain(|&(w, _), _| session.window(w).is_some()); + } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index e773a207d..dffe3ed89 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -222,8 +222,9 @@ pub struct SheetApp { pub ui: UiState, /// The window chrome to draw (see [`Chrome`]). pub chrome: Chrome, - /// Scroll position per (window id, sheet index). - pub views: HashMap<(u64, usize), SheetView>, + /// Scroll position per (window id, sheet uid): each sheet keeps its own when sheets are + /// inserted, deleted or moved. + pub views: HashMap<(u64, u64), SheetView>, pub editor: Option, pub text_box_editor: Option, pub dialog: Option, diff --git a/crates/ui-egui/tests/it/scroll_position.rs b/crates/ui-egui/tests/it/scroll_position.rs index 520bb1691..ffcb56dd6 100644 --- a/crates/ui-egui/tests/it/scroll_position.rs +++ b/crates/ui-egui/tests/it/scroll_position.rs @@ -1,4 +1,6 @@ //! The saved scroll position (`sheetView topLeftCell`) is restored on open and saved back (#378). +//! Each window keeps each sheet's own scroll position while sheets are inserted, deleted or +//! moved (#3485). use gridcraft_engine::core::{CellRef, Value}; use gridcraft_engine::model::Workbook; @@ -58,3 +60,84 @@ fn saved_scroll_position_is_restored_and_saved_back() { assert!(!d.is_dirty(), "scrolling and saving view state does not dirty the workbook"); } } + +#[test] +fn a_pivottable_sheet_opens_at_the_top_and_its_source_sheet_keeps_its_place() { + // A new PivotTable sheet goes in front of its source sheet. Scroll positions were kept by + // sheet index, so the new sheet took over the source sheet's position and opened at row 51. + let mut h = harness(None); + h.state_mut().run("range.setValues", json!({"range": "C1:D3", "values": [["Region", "Sales"], ["North", 10], ["South", 20]]})).unwrap(); + { + let app = h.state_mut(); + let sh = app.session.active().unwrap().wb.active().unwrap().clone(); + app.view_mut().unwrap().scroll.y = sh.row_top(50) as f32; + } + h.run_steps(2); + assert_eq!(first_scrolled_row(&h), 50, "setup: Sheet1 is scrolled to row 51"); + h.state_mut().run("insert.pivotTable", json!({"source": "Sheet1!$C$1:$D$3", "rows": ["Region"]})).unwrap(); + h.run_steps(4); + let wb = &h.state().session.active().unwrap().wb; + assert!(!wb.active().unwrap().pivots.is_empty(), "setup: the PivotTable's new sheet is active"); + assert_eq!(first_scrolled_row(&h), 0, "the new sheet opens at row 1"); + h.state_mut().run("sheet.next", json!({})).unwrap(); + h.run_steps(4); + assert_eq!(h.state().session.active().unwrap().wb.active().unwrap().name, "Sheet1"); + assert_eq!(first_scrolled_row(&h), 50, "Sheet1 is still scrolled to row 51"); +} + +/// Scrolls the active sheet so `row` (0-based) is the first one in view. +fn scroll_to_row(h: &mut Harness, row: u32) { + let app = h.state_mut(); + let sh = app.session.active().unwrap().wb.active().unwrap().clone(); + app.view_mut().unwrap().scroll.y = sh.row_top(row) as f32; + h.run_steps(2); +} + +fn activate(h: &mut Harness, sheet: &str) { + h.state_mut().run("sheet.activate", json!({"sheet": sheet})).unwrap(); + h.run_steps(4); + assert_eq!(h.state().session.active().unwrap().wb.active().unwrap().name, sheet); +} + +#[test] +fn a_sheet_keeps_its_scroll_position_when_sheets_are_inserted_deleted_or_moved() { + let mut h = harness(None); + scroll_to_row(&mut h, 50); + h.state_mut().run("home.insertSheet", json!({"name": "Two", "before": 0})).unwrap(); + h.run_steps(4); + assert_eq!(first_scrolled_row(&h), 0, "the inserted sheet opens at row 1"); + scroll_to_row(&mut h, 10); + activate(&mut h, "Sheet1"); + assert_eq!(first_scrolled_row(&h), 50, "Sheet1 moved one place right and is still at row 51"); + h.state_mut().run("home.deleteSheet", json!({"sheet": "Two"})).unwrap(); + h.run_steps(4); + assert_eq!(first_scrolled_row(&h), 50, "Sheet1 moved back and is still at row 51"); + h.state_mut().run("edit.undo", json!({})).unwrap(); + h.run_steps(4); + activate(&mut h, "Two"); + assert_eq!(first_scrolled_row(&h), 10, "the sheet undo brings back is where it was left"); + h.state_mut().run("sheet.move", json!({"sheet": 0, "to": 1})).unwrap(); + h.run_steps(4); + activate(&mut h, "Two"); + assert_eq!(first_scrolled_row(&h), 10, "a moved sheet keeps its place"); + activate(&mut h, "Sheet1"); + assert_eq!(first_scrolled_row(&h), 50, "and so does the sheet it moved past"); +} + +#[test] +fn each_window_scrolls_on_its_own_and_a_closed_windows_positions_are_dropped() { + let mut h = harness(None); + scroll_to_row(&mut h, 50); + let first = h.state().session.active_window().unwrap(); + h.state_mut().run("view.newWindow", json!({})).unwrap(); + h.run_steps(4); + let second = h.state().session.active_window().unwrap(); + assert_ne!(first, second); + scroll_to_row(&mut h, 10); + assert!(h.state().views.keys().any(|(w, _)| *w == second), "setup: the second window has its own position"); + h.state_mut().run("window.close", json!({"window": second})).unwrap(); + h.run_steps(4); + assert_eq!(h.state().session.active_window(), Some(first)); + assert_eq!(first_scrolled_row(&h), 50, "the first window kept its own position"); + assert!(h.state().views.keys().all(|(w, _)| *w == first), "nothing is kept for the closed window: {:?}", h.state().views.keys()); +}