diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 23a09c9ee..ebc369962 100644 --- a/crates/engine/src/lib.rs +++ b/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, + /// 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)); diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs index 76d9150a3..70fda5833 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -115,6 +115,11 @@ pub fn with_names(wb: &Workbook) -> crate::Result> { 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). diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs index 6de2e212c..ea1365e46 100644 --- a/crates/engine/src/macros/tests.rs +++ b/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. - for file in ["book.xlsx", "book.json"] { - assert!(crate::io::save_bytes(&wb, file).is_err(), "{file}"); + // dropped the next time the file opens: too large, or past the model's limits. + let bad_name = Macro { name: "Two\nlines".into(), ..Macro::default() }; + 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}"); + } } } diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs index 4a5d7c4c9..f1a7e29e5 100644 --- a/crates/engine/src/record.rs +++ b/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); + } } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index e93116120..48d5abf4c 100644 --- a/crates/engine/src/tests/seams.rs +++ b/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); }