Merge the footprint pull request (361fff98, with #2144 and #2151) into #2145

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
@@ -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<usize>,
     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<UiRequest>,
     /// Format painter source style (sticky when double-clicked).
     pub format_painter: Option<(Arc<Workbook>, 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(&params) });
             }
-            self.push_journal(id, cmd::journal_params(&params));
+            if self.journal_paused == 0 {
+                self.push_journal(id, cmd::journal_params(&params));
+            }
         }
         // 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));

crates/engine/src/macros/mod.rs

@@ -116,6 +116,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/run.rs

@@ -1053,10 +1053,9 @@ struct Saved {
     clipboard: Option<crate::Clipboard>,
     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<usize>,
     ui_requests: usize,
     format_painter: Option<(Arc<Workbook>, 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<Footprint> {
     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<OnConflict> {
 /// 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<Option<String>> {
-    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));

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}");
+        }
     }
 }
 

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);
+    }
 }

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]

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);
 }