diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 4dad22d02..3288d3e7b 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -541,6 +541,13 @@ impl Session { // made or undid, and the redo history they cleared, come back. A length is not enough, // because a nested `edit.undo` removes entries. let history_before = before.as_ref().and_then(|(uid, ..)| self.docs.iter().find(|d| d.uid == *uid).map(|d| (d.undo.clone(), d.redo.clone()))); + // The groups open on the document before the command (usually none), for the same reason. + let groups_before: Vec<(u64, String, Arc, Selection)> = match &before { + Some((uid, ..)) => { + self.groups.iter().filter(|g| g.doc == *uid).map(|g| (g.id, g.label.clone(), g.wb.clone(), g.selection.clone())).collect() + } + None => Vec::new(), + }; // The recorder keeps top-level calls with the place they ran in. let place = (self.depth == 0 && self.recorder.is_some()).then(|| match self.active() { Some(d) => (d.uid, d.wb.active_sheet, d.wb.active().map(|sh| sh.name.clone()).unwrap_or_default(), d.selection.clone()), @@ -563,6 +570,7 @@ impl Session { // Restore the workbook and its history as they were, and drop the undo groups the // command opened. self.groups.retain(|g| g.id < first_group); + let group_open = before.as_ref().is_some_and(|(uid, ..)| self.groups.iter().any(|g| g.doc == *uid)); if let Some((uid, wb, sel)) = &before && let Some(d) = self.docs.iter_mut().find(|d| d.uid == *uid) { @@ -572,6 +580,16 @@ impl Session { d.undo = undo; d.redo = redo; } + // A group open before the command and ended or rolled back inside it stays + // ended (its token went with the command), and keeps a step from its start to + // the restored workbook, so Undo still returns to the state before the group. + if !group_open + && let Some((_, label, wb, selection)) = groups_before.into_iter().min_by_key(|g| g.0) + && !Arc::ptr_eq(&wb, &d.wb) + { + push_entry(&mut d.undo, label, wb, &d.wb, selection); + d.redo.clear(); + } d.calc.rebuild(&d.wb); } if self.depth == 0 { @@ -667,20 +685,26 @@ impl Session { pub fn grouped(&mut self, label: &str, f: impl FnOnce(&mut Session) -> Result) -> Result { let token = self.group_begin(label)?; let doc = token.doc(); - match f(self) { - Ok(r) => match self.group_end(token) { + // A panic in `f` rolls the group back before it goes on, so a caller that catches it + // (outside `execute`, which drops a command's groups itself) finds no group left open. + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self))) { + Ok(Ok(r)) => match self.group_end(token) { Ok(_) => Ok(r), // `f` closed the group's document (`file.close`, or `file.open` taking the place of // an untouched new workbook), and the group went with it: nothing is left to undo. Err(_) if !self.docs.iter().any(|d| d.uid == doc) => Ok(r), Err(e) => Err(e), }, - Err(e) => { + Ok(Err(e)) => { // The error from `f` is the one to report. A rollback fails only when the group // has already ended, and then there is nothing left to undo. let _ = self.group_rollback(token); Err(e) } + Err(panic) => { + let _ = self.group_rollback(token); + std::panic::resume_unwind(panic) + } } } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index 9c9aeb6ad..0f1bc3072 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -579,6 +579,70 @@ fn a_command_that_panics_leaves_the_history_as_it_found_it() { assert_eq!(v(&s, "A1").display(), "1"); } +#[test] +fn a_grouped_action_that_panics_is_rolled_back_before_the_panic_goes_on() { + // Outside a command, a caller that catches the panic finds the group rolled back and no + // group left open, so Undo is enabled and the next grouped action does not nest. + let mut s = s(); + set(&mut s, "A1", "before"); + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + s.grouped::<()>("Doomed", |s| { + set(s, "A1", "half done"); + panic!("inside the group"); + }) + })); + assert!(caught.is_err(), "the panic goes on to the caller"); + let uid = s.doc().unwrap().uid; + assert!(!s.in_group(uid)); + assert_eq!(v(&s, "A1").display(), "before"); + assert_eq!(undo_labels(&s), ["Enter Cell"]); + s.grouped("Next", |s| { + set(s, "A1", "after"); + Ok(()) + }) + .unwrap(); + assert_eq!(undo_labels(&s), ["Enter Cell", "Next"]); +} + +#[test] +fn a_group_ended_by_a_command_that_then_panics_keeps_its_step() { + // The command takes a token the test left for it, ends or rolls back that group, and panics. + // The group's token went with the command, so the group stays ended; its step runs from the + // group's start to the restored workbook, so Undo still returns to the state before the group. + static TOKEN: std::sync::Mutex> = std::sync::Mutex::new(None); + crate::cmd::register_undoable_test_command("test.endGroupThenPanic", |s, _| { + let token = TOKEN.lock().map_err(|_| EngineError::Other("lock".into()))?.take(); + if let Some(token) = token { + s.group_end(token)?; + } + panic!("after ending a group"); + }); + crate::cmd::register_undoable_test_command("test.rollBackGroupThenPanic", |s, _| { + let token = TOKEN.lock().map_err(|_| EngineError::Other("lock".into()))?.take(); + if let Some(token) = token { + s.group_rollback(token)?; + } + panic!("after rolling back a group"); + }); + for cmd in ["test.endGroupThenPanic", "test.rollBackGroupThenPanic"] { + let mut s = s(); + set(&mut s, "A1", "before"); + let token = s.group_begin("Outer").unwrap(); + set(&mut s, "A1", "inside"); + set(&mut s, "A2", "inside too"); + *TOKEN.lock().unwrap() = Some(token); + assert!(s.execute(cmd, json!({})).is_err()); + let uid = s.doc().unwrap().uid; + assert!(!s.in_group(uid), "{cmd}: the group stays ended"); + assert_eq!(undo_labels(&s), ["Enter Cell", "Outer"], "{cmd}"); + assert_eq!(v(&s, "A1").display(), "inside", "{cmd}: the workbook is as the command found it"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!((v(&s, "A1").display(), v(&s, "A2").display()), ("before".into(), String::new()), "{cmd}"); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(v(&s, "A2").display(), "inside too", "{cmd}"); + } +} + // ---------------------------------------------------------------- recording buffer fn recorded_ids(s: &Session) -> Vec { diff --git a/docs/architecture.md b/docs/architecture.md index aa5e1e64d..f51f3a61b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -66,8 +66,11 @@ Actions made of many commands (macros, a debugger that runs one step per call) u Commands run by other commands are not recorded, and the buffer is independent of the journal (Automate › Record Actions still records through the journal). - **Document trust.** `DocState::trust` is `Local` for workbooks made in the session and - `FromFile` for workbooks opened from a file, and for workbooks that received formulas or - validation pasted from one. Active content in a `FromFile` workbook asks before it runs. This + `FromFile` for workbooks opened from a file, and for workbooks that received a paste from one + that writes formulas or validation: Paste, Paste Special › All, All Except Borders, Formulas, + Formulas and Number Formats or Validation, and any linked paste other than column widths or + notes. Values, formats, column widths and notes carry nothing active. Active content in a + `FromFile` workbook asks before it runs. This is provenance, which no decision changes; `DocState::scripts_trusted` is the separate per-session consent for stored `automate.*` scripts (`automate.trustScripts`). - **Workbook data in hidden names** (`cmd/stash.rs`). An item is split over hidden