From 0533480c970ac558fabff66bbaa11fe380eb0b80 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:48:03 -0400 Subject: [PATCH] Engine: recorded steps carry a number, so a group's rollback finds its own after others A group remembered the recording's length when it began, which another workbook's rollback (or a recording started again inside the group) made point at the wrong steps. Each kept step now has a number, and each recording one of its own; a group remembers both, and its rollback takes back the steps of its workbook numbered from its mark, or all of them when the recording started inside the group. - Test: `rollbacks_take_back_their_own_recorded_steps_in_any_order` (groups on two workbooks rolled back in turn, and a recording started again inside a group). --- crates/engine/src/lib.rs | 2 +- crates/engine/src/record.rs | 53 +++++++++++++++++++++++--------- crates/engine/src/tests/seams.rs | 28 +++++++++++++++++ 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index a0305b5a8..d7aa1bfe1 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -98,7 +98,7 @@ struct OpenGroup { 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)>, + recorded: Option<(u64, u64)>, } /// Where the last [`Session::group_end`] or [`Session::group_rollback`] left its document: the diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs index b0d28ef40..b6ccae410 100644 --- a/crates/engine/src/record.rs +++ b/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, /// 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, +} + +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(); } } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index 3fa37b100..a9692d45b 100644 --- a/crates/engine/src/tests/seams.rs +++ b/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::>(); + 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