Merge pull request #3507 from ZIDAN44/fix/3485-scroll-per-window

View: a sheet keeps its own scroll position when sheets move (#3485)

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, `<controls>` 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 |

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),

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());
+    }
 }

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<editor::EditState>,
     pub text_box_editor: Option<text_box::EditState>,
     pub dialog: Option<dialogs::Dialog>,

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());
+}