collective
Browse / github.com/storytold/gridcraft / patch
Undo and redo keep each sheet's view (Normal, Page Layout, Page Break Preview)
Undo and redo keep each sheet's view (Normal, Page Layout, Page Break Preview) ## What happened Undoing an edit changed the sheet's view. Set A1, switch the sheet to Page Break Preview, then undo: A1 is cleared, and the sheet is back in Normal. Redo switches it to Page Break Preview again. The command-sequence fuzz reports the same thing as a failed undo check after one step, `view.pageBreakPreview {"sheet": 0}` on the `sales` sample (seed 177 with `GRIDCRAFT_FUZZ_SEEDS=300`): ``` invariant [undo]: undoing all 0 steps doesn't give back the starting workbook: Sales: other sheet properties differ ``` ## Why Switching between Normal, Page Layout and Page Break Preview
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- prxnvt (imported from pull request #2597, open)
- Base
ad10881cc3347c3a07bc5f04a4e876999647e839- Size
- +116 −3 in 4 files
- Review
- pending
- Shared
- 2026-10-28
- Id
0409bbf846f66369dd95377e
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/edit.rs crates/engine/src/cmd/extra.rs crates/engine/src/tests.rs crates/engine/tests/it/command_sequence_fuzz.rs
@@ -809,6 +809,8 @@ fn undo(s: &mut Session, p: &Json) -> Result<Json> { label = e.label.clone(); d.wb = e.wb; d.selection = e.selection;+ // Step by step: a rename and a new sheet undone together match up only one step apart.+ super::extra::keep_view_modes(d, &cur_wb); crate::push_history(&mut d.redo, label.clone(), cur_wb, &d.wb, cur_selection); } super::extra::keep_watch_list(&now, &mut d.wb);@@ -829,6 +831,7 @@ fn redo(s: &mut Session, p: &Json) -> Result<Json> { label = e.label.clone(); d.wb = e.wb; d.selection = e.selection;+ super::extra::keep_view_modes(d, &cur_wb); crate::push_history(&mut d.undo, label.clone(), cur_wb, &d.wb, cur_selection); } super::extra::keep_watch_list(&now, &mut d.wb);@@ -756,6 +756,35 @@ pub(crate) fn keep_watch_list(from: &Workbook, to: &mut std::sync::Arc<Workbook> wb.names.extend(keep); } +/// Normal, Page Layout and Page Break Preview belong to the window, not to the edits: Excel doesn't+/// undo a switch between them, and `set_view` is no undo step. So undo and redo keep each sheet's+/// current view when they swap in another snapshot of the workbook, and a view kept this way+/// doesn't make a saved workbook dirty.+pub(crate) fn keep_view_modes(d: &mut crate::DocState, now: &Workbook) {+ // Sheets have no stable id. A name missing from `now` is a rename being undone or redone, which+ // keeps the sheet count and position: the sheet is the one at the same index, under a name the+ // restored snapshot doesn't have.+ let renamed = |i: usize| {+ (now.sheets.len() == d.wb.sheets.len()).then(|| now.sheets.get(i)).flatten().filter(|sh| !d.wb.sheets.iter().any(|s| s.name == sh.name))+ };+ let current = |i: usize, name: &str| now.sheets.iter().find(|sh| sh.name == name).or_else(|| renamed(i)).map(|sh| sh.view);+ let changed: Vec<(usize, SheetViewMode)> =+ d.wb.sheets.iter().enumerate().filter_map(|(i, sh)| current(i, &sh.name).filter(|v| *v != sh.view).map(|v| (i, v))).collect();+ if changed.is_empty() {+ return;+ }+ let dirty = d.is_dirty();+ let wb = std::sync::Arc::make_mut(&mut d.wb);+ for (i, v) in changed {+ if let Some(sh) = wb.sheet_mut(i) {+ sh.view = v;+ }+ }+ if !dirty {+ d.saved = d.wb.clone();+ }+}+ fn custom_views(s: &mut Session, p: &Json) -> Result<Json> { let d = s.doc()?; let mut views = stash_map(&d.wb, VIEWS_NAME);@@ -4905,3 +4905,72 @@ fn a_saved_workbook_keeps_its_values_until_an_edit_reaches_them() { assert_eq!(v(&saved, "B1"), Value::Number(6.0)); assert_eq!(v(&saved, "C1"), Value::Number(7.0)); }++/// Normal, Page Layout and Page Break Preview are not undo steps (as in Excel), so undoing or+/// redoing an edit keeps each sheet in the view it is in now, instead of putting back the view+/// of the snapshot it swaps in. Found through the command-sequence fuzz (seed 177).+#[test]+fn undo_and_redo_keep_each_sheets_view() {+ use gridcraft_model::SheetViewMode;+ let view = |s: &Session, i: usize| s.doc().unwrap().wb.sheet(i).unwrap().view;+ let mut s = s();+ s.execute("cell.set", json!({"cell": "A1", "input": "1"})).unwrap();+ s.execute("view.pageBreakPreview", json!({})).unwrap();+ s.execute("edit.undo", json!({})).unwrap();+ assert_eq!(v(&s, "A1"), Value::Empty);+ assert_eq!(view(&s, 0), SheetViewMode::PageBreakPreview, "undo leaves the view alone");+ s.execute("view.pageLayout", json!({})).unwrap();+ s.execute("edit.redo", json!({})).unwrap();+ assert_eq!(v(&s, "A1"), Value::Number(1.0));+ assert_eq!(view(&s, 0), SheetViewMode::PageLayout, "redo leaves the view alone");++ // Undoing a rename: the sheet is found at its position under the restored name.+ s.execute("sheet.rename", json!({"sheet": 0, "name": "Renamed"})).unwrap();+ s.execute("view.normal", json!({})).unwrap();+ s.execute("edit.undo", json!({})).unwrap();+ assert_ne!(s.doc().unwrap().wb.sheet(0).unwrap().name, "Renamed");+ assert_eq!(view(&s, 0), SheetViewMode::Normal);++ // Undoing a new sheet: the other sheets keep theirs.+ s.execute("view.pageBreakPreview", json!({})).unwrap();+ s.execute("home.insertSheet", json!({"name": "Extra"})).unwrap();+ s.execute("edit.undo", json!({})).unwrap();+ assert_eq!(s.doc().unwrap().wb.sheets.len(), 1);+ assert_eq!(view(&s, 0), SheetViewMode::PageBreakPreview);+}++#[test]+fn a_multi_step_undo_keeps_views_through_each_step() {+ // A rename and a new sheet undone in one call: neither the name nor the sheet count matches+ // between the first and last workbook, so the view is carried step by step.+ let mut s = s();+ let first = s.doc().unwrap().wb.sheet(0).unwrap().name.clone();+ s.execute("sheet.rename", json!({"sheet": 0, "name": "Renamed"})).unwrap();+ s.execute("view.pageBreakPreview", json!({"sheet": 0})).unwrap();+ s.execute("home.insertSheet", json!({"name": "Extra"})).unwrap();+ s.execute("edit.undo", json!({"steps": 2})).unwrap();+ let d = s.doc().unwrap();+ assert_eq!(d.wb.sheets.len(), 1);+ assert_eq!(d.wb.sheet(0).unwrap().name, first);+ assert_eq!(d.wb.sheet(0).unwrap().view, gridcraft_model::SheetViewMode::PageBreakPreview);+ s.execute("edit.redo", json!({"steps": 2})).unwrap();+ let d = s.doc().unwrap();+ assert_eq!((d.wb.sheets.len(), d.wb.sheet(0).unwrap().name.as_str()), (2, "Renamed"));+ assert_eq!(d.wb.sheet(0).unwrap().view, gridcraft_model::SheetViewMode::PageBreakPreview);+}++#[test]+fn a_view_kept_through_redo_doesnt_make_the_saved_workbook_dirty() {+ let mut s = s();+ s.execute("cell.set", json!({"cell": "A1", "input": "1"})).unwrap();+ {+ let d = s.doc_mut().unwrap();+ d.saved = d.wb.clone();+ }+ s.execute("edit.undo", json!({})).unwrap();+ s.execute("view.pageBreakPreview", json!({})).unwrap();+ s.execute("edit.redo", json!({})).unwrap();+ let d = s.doc().unwrap();+ assert_eq!(d.wb.active().unwrap().view, gridcraft_model::SheetViewMode::PageBreakPreview);+ assert!(!d.is_dirty(), "back at the saved edits, the view alone isn't a change");+}@@ -34,7 +34,7 @@ use std::sync::Arc; use gridcraft_core::{CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value, col_to_letters}; use gridcraft_engine::calc::Calc;-use gridcraft_engine::model::{CalcMode, Formula, Sheet, Workbook};+use gridcraft_engine::model::{CalcMode, Formula, Sheet, SheetViewMode, Workbook}; use gridcraft_engine::{EngineError, Session, command_specs, find_command, io}; use serde_json::{Value as Json, json}; @@ -485,8 +485,9 @@ fn watch_list(wb: &Workbook) -> Option<&str> { wb.names.iter().find(|n| n.name == WATCH_NAME).map(|n| n.formula.as_str()) } -/// The workbook with view state (active sheet, scroll, active cell) and the Watch Window list-/// reset: undo restores view state as it was, but switching sheets isn't an undoable edit.+/// The workbook with view state (active sheet, scroll, active cell, Normal / Page Layout / Page+/// Break Preview) and the Watch Window list reset: undo restores view state as it was, but+/// switching sheets or views isn't an undoable edit. fn canonical(wb: &Workbook) -> Workbook { let mut wb = wb.clone(); wb.active_sheet = 0;@@ -495,6 +496,7 @@ fn canonical(wb: &Workbook) -> Workbook { let sh = Arc::make_mut(sh); sh.view_active = CellRef::default(); sh.view_top_left = CellRef::default();+ sh.view = SheetViewMode::Normal; } wb }@@ -1088,3 +1090,13 @@ fn params_docs_parse_into_keys() { assert!(find_command(id).is_some(), "CURATED names unknown command `{id}`"); } }++/// Seed 177: switching a sheet to Page Break Preview isn't an undo step (as in Excel), so undoing+/// everything leaves the view mode alone and the undo check must not count it as an edit.+#[test]+fn view_mode_switch_is_not_an_undo_difference() {+ for id in ["view.pageBreakPreview", "view.pageLayout"] {+ let failure = replay("sales", &[(id.to_string(), json!({"sheet": 0}))]);+ assert!(failure.is_none(), "`{id}`: {failure:?}");+ }+}