diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 27e217474..ba3bf8ac4 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -183,6 +183,8 @@ fn undo(s: &mut Session, p: &Json) -> Result { 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); @@ -203,6 +205,7 @@ fn redo(s: &mut Session, p: &Json) -> Result { 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); diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index c9a1582a2..70640bf27 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -28,7 +28,7 @@ use objects::{align, arrange, rotate, selection_pane, shape_fill, shape_outline} pub(crate) use scripts::has_scripts; use scripts::{all_scripts, delete_script, new_script, record, run_script, stop_action, trust_scripts}; use window::{arrange_all, custom_views, hide_window, new_window, ruler, set_view, split, switch_windows, unhide_window, watch_window}; -pub(crate) use window::{keep_watch_list, window_param}; +pub(crate) use window::{keep_view_modes, keep_watch_list, window_param}; pub fn specs() -> Vec { vec![ diff --git a/crates/engine/src/cmd/extra/window.rs b/crates/engine/src/cmd/extra/window.rs index b20a00a61..b8129805f 100644 --- a/crates/engine/src/cmd/extra/window.rs +++ b/crates/engine/src/cmd/extra/window.rs @@ -38,6 +38,35 @@ pub(crate) fn keep_watch_list(from: &Workbook, to: &mut std::sync::Arc 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(); + } +} + pub(super) fn custom_views(s: &mut Session, p: &Json) -> Result { let d = s.doc()?; let mut views = stash_map(&d.wb, VIEWS_NAME); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f99f246df..b99e01248 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -5234,3 +5234,72 @@ fn unique_nonce() -> u64 { static N: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); N.fetch_add(1, std::sync::atomic::Ordering::Relaxed) } + +/// 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"); +}