Merge #2151 (0aae1ffe) into #2144

crates/engine/src/lib.rs

@@ -102,6 +102,9 @@ struct OpenGroup {
     selection: Selection,
     /// The redo stack when the group began (a rollback puts it back).
     redo: Vec<HistoryEntry>,
+    /// Where the recording was when the group began (`None` when nothing was recording): a
+    /// rollback takes back the steps recorded since, as it takes back their edits.
+    recorded: Option<(usize, usize)>,
 }
 
 /// Where the last [`Session::group_end`] or [`Session::group_rollback`] left its document: the
@@ -789,6 +792,7 @@ impl Session {
             wb: d.wb.clone(),
             selection: d.selection.clone(),
             redo: d.redo.clone(),
+            recorded: self.recorder.as_ref().map(record::Recorder::mark),
         };
         let token = GroupToken { id: g.id, doc: g.doc };
         self.next_group = self.next_group.saturating_add(1);
@@ -885,6 +889,11 @@ impl Session {
         d.undo.truncate(g.start);
         d.redo = g.redo;
         d.revision += 1;
+        // The steps recorded while the group was open go with their edits (all of them when the
+        // recording started inside the group).
+        if let Some(rec) = self.recorder.as_mut() {
+            rec.cut_back(g.recorded.unwrap_or((0, 0)));
+        }
         // The record of the last group step stays when the document is back exactly where that
         // step left it (this group began there); otherwise the rollback is where the group ended.
         let kept = self.group_step.as_ref().is_some_and(|st| st.doc == g.doc && st.len == g.start && Arc::ptr_eq(&st.after, &d.wb));

crates/engine/src/macros/mod.rs

@@ -115,6 +115,11 @@ pub fn with_names(wb: &Workbook) -> crate::Result<Option<Workbook>> {
     if wb.macros.is_empty() && wb.macros_unread.is_empty() && !wb.names.iter().any(stored) {
         return Ok(None);
     }
+    // A macro that skipped `check` (built in code) fails the save, as the JSON save does, rather
+    // than being written and dropped the next time the file opens.
+    for m in wb.macros.iter() {
+        check(m).map_err(crate::EngineError::Other)?;
+    }
     let mut out = wb.clone();
     // One pass drops the items of the last save, and one writes the macros (a file can hold a
     // thousand macros and many thousands of names).

crates/engine/src/macros/tests.rs

@@ -211,12 +211,16 @@ fn a_macro_built_in_code_goes_through_the_size_check_too() {
     };
     assert!(big.check_limits().is_ok(), "the model's limits say nothing about the size");
     assert!(check(&big).unwrap_err().contains("too large"));
-    let mut wb = Workbook::new();
-    wb.macros.push(big);
     // A macro that fails `check` fails the save in either format, rather than being written and
-    // dropped the next time the file opens.
+    // dropped the next time the file opens: too large, or past the model's limits.
-    for file in ["book.xlsx", "book.json"] {
+    let bad_name = Macro { name: "Two\nlines".into(), ..Macro::default() };
-        assert!(crate::io::save_bytes(&wb, file).is_err(), "{file}");
+    assert!(check(&bad_name).is_err());
+    for m in [big, bad_name] {
+        let mut wb = Workbook::new();
+        wb.macros.push(m);
+        for file in ["book.xlsx", "book.json"] {
+            assert!(crate::io::save_bytes(&wb, file).is_err(), "{file}");
+        }
     }
 }
 

crates/engine/src/record.rs

@@ -67,4 +67,19 @@ impl Recorder {
             self.dropped = self.dropped.saturating_add(1);
         }
     }
+
+    /// Where the recording is now, for [`Recorder::cut_back`].
+    pub(crate) fn mark(&self) -> (usize, usize) {
+        (self.steps.len(), self.dropped)
+    }
+
+    /// Takes back the steps recorded since `mark` (an undo group that recorded them was rolled
+    /// back, so playing them back would make edits the workbook no longer has).
+    pub(crate) fn cut_back(&mut self, (len, dropped): (usize, usize)) {
+        if self.steps.len() > len {
+            self.steps.truncate(len);
+            self.bytes = self.steps.iter().map(|s| serde_json::to_vec(s).map_or(0, |v| v.len())).sum();
+        }
+        self.dropped = self.dropped.min(dropped);
+    }
 }

crates/engine/src/tests/seams.rs

@@ -837,12 +837,13 @@ fn stop_action_runs_only_on_its_own_and_is_not_recorded() {
     r.unwrap();
     assert_eq!(v(&s, "A2").display(), "two");
     assert_eq!(undo_labels(&s), ["Outer"], "the action kept its group");
-    // On its own it stops a group left open; the recording keeps the edits and not the stop.
+    // On its own it stops a group left open; the recording keeps neither the stop nor the edits
+    // the stop rolled back, so playing it back makes only the edits the workbook has.
     let _left = s.group_begin("Left open").unwrap();
     set(&mut s, "A3", "three");
     assert_eq!(s.execute("automate.stopAction", json!({})).unwrap(), json!({"stopped": true}));
     assert!(v(&s, "A3").is_empty());
-    assert_eq!(recorded_ids(&s), ["cell.set", "cell.set", "cell.set"]);
+    assert_eq!(recorded_ids(&s), ["cell.set", "cell.set"]);
     assert_eq!(s.stop_recording().unwrap().dropped, 0);
 }