Merge #2151 (0533480c) into #2144

crates/engine/src/lib.rs

@@ -104,7 +104,7 @@ struct OpenGroup {
     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)>,
+    recorded: Option<(u64, u64)>,
 }
 
 /// Where the last [`Session::group_end`] or [`Session::group_rollback`] left its document: the

crates/engine/src/record.rs

@@ -38,8 +38,11 @@ pub struct RecordedStep {
     pub params: Json,
 }
 
+/// Numbers each recording, so an undo group's mark from an earlier one isn't read as this one's.
+static NEXT_RECORDING: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1);
+
 /// The steps recorded since recording started.
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug)]
 pub struct Recorder {
     pub steps: Vec<RecordedStep>,
     /// Steps not kept because the recording reached [`RECORD_LIMIT`] or [`RECORD_MAX_BYTES`].
@@ -48,6 +51,18 @@ pub struct Recorder {
     pub dropped: usize,
     /// The size of `steps` serialized as JSON.
     bytes: usize,
+    /// Which recording this is, and the number the next kept step gets: each step's number is in
+    /// `seqs`, beside it, so a mark stays right however many steps rollbacks take out.
+    generation: u64,
+    next_seq: u64,
+    seqs: Vec<u64>,
+}
+
+impl Default for Recorder {
+    fn default() -> Self {
+        let generation = NEXT_RECORDING.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        Recorder { steps: Vec::new(), dropped: 0, bytes: 0, generation, next_seq: 0, seqs: Vec::new() }
+    }
 }
 
 impl Recorder {
@@ -63,28 +78,38 @@ impl Recorder {
         if self.bytes.saturating_add(size) <= RECORD_MAX_BYTES {
             self.bytes = self.bytes.saturating_add(size);
             self.steps.push(step);
+            self.seqs.push(self.next_seq);
+            self.next_seq = self.next_seq.saturating_add(1);
         } else {
             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)
+    /// Where the recording is now, for [`Recorder::cut_back`]: this recording and the number its
+    /// next step gets.
+    pub(crate) fn mark(&self) -> (u64, u64) {
+        (self.generation, self.next_seq)
     }
 
     /// Takes back the steps recorded in the document with uid `doc` since `mark` (an undo group
     /// on it was rolled back, so playing them back would make edits it no longer has). Steps in
-    /// other documents stay: their edits do too. Steps the buffer dropped stay counted.
-    pub(crate) fn cut_back(&mut self, (len, _): (usize, usize), doc: u64) {
-        let before = self.steps.len();
-        let mut i = 0;
-        self.steps.retain(|s| {
-            i += 1;
-            i <= len || s.doc != doc
-        });
-        if self.steps.len() != before {
-            self.bytes = self.steps.iter().map(|s| serde_json::to_vec(s).map_or(0, |v| v.len())).sum();
+    /// other documents stay: their edits do too. A mark from another recording (the recording
+    /// started again inside the group) means every step of this one came after it. Steps the
+    /// buffer dropped stay counted.
+    pub(crate) fn cut_back(&mut self, (generation, seq): (u64, u64), doc: u64) {
+        let from = if generation == self.generation { seq } else { 0 };
+        let gone = |i: usize, step: &RecordedStep| step.doc == doc && self.seqs.get(i).is_some_and(|&n| n >= from);
+        if !self.steps.iter().enumerate().any(|(i, s)| gone(i, s)) {
+            return;
         }
+        let (mut steps, mut seqs) = (Vec::with_capacity(self.steps.len()), Vec::with_capacity(self.seqs.len()));
+        for (i, step) in self.steps.iter().enumerate() {
+            if !gone(i, step) {
+                steps.push(step.clone());
+                seqs.push(self.seqs.get(i).copied().unwrap_or(0));
+            }
+        }
+        (self.steps, self.seqs) = (steps, seqs);
+        self.bytes = self.steps.iter().map(|s| serde_json::to_vec(s).map_or(0, |v| v.len())).sum();
     }
 }

crates/engine/src/tests/seams.rs

@@ -857,6 +857,34 @@ fn stop_action_runs_only_on_its_own_and_is_not_recorded() {
     assert_eq!(s.stop_recording().unwrap().dropped, 0);
 }
 
+#[test]
+fn rollbacks_take_back_their_own_recorded_steps_in_any_order() {
+    // Groups open on two workbooks, each with a recorded edit: rolling back the first takes out
+    // only its edit, and the second's mark still finds its own after that.
+    let mut s = s();
+    s.start_recording();
+    let a = s.active_index();
+    let _in_a = s.group_begin("A").unwrap();
+    set(&mut s, "A1", "a");
+    s.new_workbook();
+    let b = s.active_index();
+    let _in_b = s.group_begin("B").unwrap();
+    set(&mut s, "B1", "b");
+    s.set_active(a);
+    s.execute("automate.stopAction", json!({})).unwrap();
+    let cells = |s: &Session| s.recorder.as_ref().unwrap().steps.iter().map(|st| st.params["cell"].clone()).collect::<Vec<_>>();
+    assert_eq!(cells(&s), [json!("B1")]);
+    s.set_active(b);
+    s.execute("automate.stopAction", json!({})).unwrap();
+    assert!(cells(&s).is_empty());
+    // A recording started again inside a group: every step of it came after the group began.
+    let _again = s.group_begin("Again").unwrap();
+    s.start_recording();
+    set(&mut s, "C1", "c");
+    s.execute("automate.stopAction", json!({})).unwrap();
+    assert!(cells(&s).is_empty(), "{:?}", cells(&s));
+}
+
 #[test]
 fn a_run_that_edits_and_opens_a_dialog_is_recorded() {
     // A script with an edit before a menu-style step: the edit happened, so the run is a step of