Merge #462 (712bb766) into #465

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<Workbook>, 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<R>(&mut self, label: &str, f: impl FnOnce(&mut Session) -> Result<R>) -> Result<R> {
         let token = self.group_begin(label)?;
         let doc = token.doc();
-        match f(self) {
+        // A panic in `f` rolls the group back before it goes on, so a caller that catches it
-            Ok(r) => match self.group_end(token) {
+        // (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)
+            }
         }
     }
 

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<Option<crate::GroupToken>> = 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<String> {

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
+  `FromFile` for workbooks opened from a file, and for workbooks that received a paste from one
-  validation pasted from one. Active content in a `FromFile` workbook asks before it runs. This
+  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