diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index d85616c5e..041e5d65c 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 @@ -442,9 +445,9 @@ pub struct Session { /// Serialized size of each journal entry, measured once when it is pushed, and their sum. journal_sizes: Vec, journal_bytes: usize, - /// Entries the journal has dropped from its front to stay within its limits, ever; a dry run - /// that puts the journal back needs to know how many went while it ran. - journal_dropped: u64, + /// While a dry run runs (the footprint, `macro.check`), its commands are not journaled: the + /// dry run is put back, and journaling it would evict the person's own oldest entries. + pub(crate) journal_paused: u32, pub ui_requests: Vec, /// Format painter source style (sticky when double-clicked). pub format_painter: Option<(Arc, usize, RangeRef, bool)>, @@ -575,16 +578,10 @@ impl Session { self.journal.drain(..n); self.journal_sizes.drain(..n); self.journal_bytes = kept; - self.journal_dropped = self.journal_dropped.saturating_add(n as u64); self.recording = self.recording.map(|r| r.saturating_sub(n)); } } - /// Entries dropped from the journal's front so far, for a caller that puts the journal back. - pub(crate) fn journal_dropped(&self) -> u64 { - self.journal_dropped - } - /// Cuts the journal back to its first `keep` entries, with its size bookkeeping. pub(crate) fn truncate_journal(&mut self, keep: usize) { if self.journal_sizes.len() == self.journal.len() { @@ -695,6 +692,8 @@ impl Session { macros::debug::recover(self); self.trim_history(); self.group_step = None; + // A dry run the panic cut short never put journaling back on. + self.journal_paused = 0; } return Err(EngineError::Internal(id.to_string(), msg)); } @@ -720,7 +719,9 @@ impl Session { { rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params: cmd::journal_params(¶ms) }); } - self.push_journal(id, cmd::journal_params(¶ms)); + if self.journal_paused == 0 { + self.push_journal(id, cmd::journal_params(¶ms)); + } } // Inside an undo group the group owns the step. A command that ended a group open before // it leaves the group its step and makes one of its own for what it changed after the end. @@ -835,6 +836,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); @@ -931,6 +933,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 2c6b0ffb1..a2b1b3caa 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -116,6 +116,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/run.rs b/crates/engine/src/macros/run.rs index ea5973072..c122ba7aa 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -1053,10 +1053,9 @@ struct Saved { clipboard: Option, prefs: crate::Prefs, mode: crate::Mode, - /// The journal's length and how many entries it had dropped from its front; a dry run that - /// pushes past the journal's limits drops more, which a length alone cannot see. + /// The journal's length: the dry run journals nothing (`Session::journal_paused`), so nothing + /// of the person's is evicted, and the length puts back exactly what was there. journal: usize, - journal_dropped: u64, recording: Option, ui_requests: usize, format_painter: Option<(Arc, usize, RangeRef, bool)>, @@ -1069,9 +1068,10 @@ struct Saved { } impl Saved { + /// Takes the state and stops journaling until [`Saved::restore`]. fn take(s: &mut Session, doc: u64) -> Saved { let d = s.docs.iter().find(|d| d.uid == doc); - Saved { + let saved = Saved { active: s.active, docs: s.docs.iter().map(|d| d.uid).collect(), saved: d.map(|d| d.saved.clone()), @@ -1084,7 +1084,6 @@ impl Saved { prefs: s.prefs.clone(), mode: s.mode, journal: s.journal.len(), - journal_dropped: s.journal_dropped(), recording: s.recording, ui_requests: s.ui_requests.len(), format_painter: s.format_painter.clone(), @@ -1094,7 +1093,9 @@ impl Saved { draw: (s.draw_tool.clone(), s.draw_color.clone(), s.draw_width), comments_pane: s.comments_pane, show_all_notes: s.show_all_notes, - } + }; + s.journal_paused = s.journal_paused.saturating_add(1); + saved } fn restore(self, s: &mut Session, doc: u64) { @@ -1115,11 +1116,9 @@ impl Saved { s.clipboard = self.clipboard; s.prefs = self.prefs; s.mode = self.mode; - // Entries the dry run pushed go; entries the journal dropped meanwhile are gone for good, - // and the recording's start moves back with them. - let dropped = usize::try_from(s.journal_dropped().saturating_sub(self.journal_dropped)).unwrap_or(usize::MAX); - s.truncate_journal(self.journal.saturating_sub(dropped)); - s.recording = self.recording.map(|r| r.saturating_sub(dropped)); + s.truncate_journal(self.journal); + s.recording = self.recording; + s.journal_paused = s.journal_paused.saturating_sub(1); s.ui_requests.truncate(self.ui_requests); s.format_painter = self.format_painter; s.last_find = self.last_find; @@ -1137,9 +1136,10 @@ impl Saved { pub fn footprint(s: &mut Session, p: &Prepared, run: &Machine) -> Result { let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?; s.set_active(i); - let saved = Saved::take(s, p.doc); let before = s.doc()?.wb.clone(); let token = s.group_begin("Check Macro")?; + // After the calls that can fail, so nothing returns between taking and putting back. + let saved = Saved::take(s, p.doc); if let Some(d) = s.docs.iter_mut().find(|d| d.uid == p.doc) { d.touched = Some(Vec::new()); } @@ -1543,8 +1543,8 @@ fn conflict_rule(p: &Json, m: &Macro) -> Result { /// For `macro.check`, as the run would: shifts the data, checks the footprint again, and puts /// everything back. What still stands in the way, or `None`. fn shifted_conflicts(s: &mut Session, fp: &Footprint, p: &Prepared, budget: &Machine, down: bool) -> Result> { - let saved = Saved::take(s, p.doc); let token = s.group_begin("Check Macro")?; + let saved = Saved::take(s, p.doc); let mut spent = budget.clone(); (spent.executed, spent.param_bytes) = (fp.executed, fp.param_bytes); let again = make_room(s, fp, p, down).and_then(|()| footprint(s, p, &spent)); 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 67dcb1cfb..913e4f86f 100644 --- a/crates/engine/src/record.rs +++ b/crates/engine/src/record.rs @@ -68,4 +68,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/macros.rs b/crates/engine/src/tests/macros.rs index 13530cb9b..941a4a29f 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -718,33 +718,29 @@ fn a_dry_run_leaves_a_clean_workbook_clean() { } #[test] -fn a_dry_run_puts_the_journal_back_even_after_it_trimmed_it() { - // Every nested command a dry run executes is journaled, so a long macro pushes the journal - // past its limit and the journal drops its oldest entries. Putting the journal back by its - // old length would then keep entries from the dry run and point a recording at the wrong - // place; the restore counts what was dropped. +fn a_dry_run_leaves_the_journal_and_the_recording_start_as_they_were() { + // A dry run journals nothing: journaling its nested commands would push a full journal past + // its limit and evict the person's own oldest entries, and move Record Actions' start. let mut s = s(); data(&mut s); - s.execute("selection.set", json!({"range": "A1"})).unwrap(); - let journal = s.journal.clone(); - s.recording = Some(journal.len().saturating_sub(1)); + for i in 0..crate::JOURNAL_MAX_ENTRIES { + s.execute("selection.set", json!({"range": format!("A{}", i % 50 + 1)})).unwrap(); + } + s.recording = Some(s.journal.len().saturating_sub(100)); put( &mut s, json!({"name": "Long", "steps": [{"type": "repeat", "over": "A1:A200", "body": [ {"type": "repeat", "over": "A1:A60", "body": [{"command": "cell.set", "params": {"cell": "Row.cell(1,1)", "input": "x"}}]} ]}]}), ); - s.execute("macro.check", json!({"name": "Long"})).unwrap(); - let sets = |j: &[(String, Json)]| j.iter().filter(|(id, _)| id == "cell.set").count(); - assert!(sets(&s.journal) <= sets(&journal), "no dry-run entry stays"); - assert!(s.journal.len() <= journal.len(), "{} entries, had {}", s.journal.len(), journal.len()); - assert!(journal.ends_with(&s.journal), "what stays is the journal's own tail"); - assert!(s.recording.is_some_and(|r| r <= s.journal.len()), "{:?}", s.recording); - // A short dry run leaves the journal exactly as it was. - put(&mut s, json!({"name": "Short", "steps": [{"command": "cell.set", "params": {"cell": "H1", "input": "x"}}]})); let (journal, recording) = (s.journal.clone(), s.recording); - s.execute("macro.check", json!({"name": "Short"})).unwrap(); - assert_eq!((&s.journal, s.recording), (&journal, recording)); + let c = s.execute("macro.check", json!({"name": "Long"})).unwrap(); + assert!(c.get("footprint").is_some(), "{c}"); + assert_eq!((s.journal.len(), s.recording), (journal.len(), recording)); + assert!(s.journal == journal, "the journal is as it was"); + // The check itself is a query and isn't journaled; a command after it is. + s.execute("selection.set", json!({"range": "B1"})).unwrap(); + assert_eq!(s.journal.last().map(|(id, _)| id.as_str()), Some("selection.set")); } #[test] 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); }