diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index 009d77f2a..02d45febb 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -1099,6 +1099,11 @@ fn run_script(s: &mut Session, p: &Json) -> Result { /// Automate › Stop Action: rolls back the undo groups open on the active workbook, so Undo and /// Redo work there again. fn stop_action(s: &mut Session, _: &Json) -> Result { + // Inside another command the open groups are that command's own (an action in progress): + // a step of it that stopped them would pull the workbook from under the rest of its run. + if s.depth() > 1 { + return Err(EngineError::Disabled("automate.stopAction".into(), "Stop Action runs on its own, not as a step of a script or macro.".into())); + } let uid = s.doc()?.uid; Ok(json!({"stopped": s.abort_groups(uid)})) } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 1827da375..856f1777e 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -623,7 +623,7 @@ impl Session { // file, `home.formatCells` with no style) is not recorded either: the dialog's own // call is, so playing the recording back runs the command once, with its parameters. let opened_dialog = self.ui_requests.get(requests_before..).is_some_and(|new| new.iter().any(|q| matches!(q, UiRequest::Dialog(..)))); - if let Some((doc, sheet, sheet_name, selection)) = place.filter(|_| !opened_dialog) + if let Some((doc, sheet, sheet_name, selection)) = place.filter(|_| !opened_dialog && !record::NOT_RECORDED.contains(&id)) && let Some(rec) = self.recorder.as_mut() { rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params: cmd::journal_params(¶ms) }); @@ -870,6 +870,16 @@ impl Session { self.depth } + /// Runs a command as a command in progress would (one level deeper), for tests of what a + /// nested call may do. + #[cfg(test)] + pub(crate) fn execute_nested(&mut self, id: &str, params: Json) -> Result { + self.depth = self.depth.saturating_add(1); + let r = self.execute(id, params); + self.depth = self.depth.saturating_sub(1); + r + } + /// Whether an undo group is open on the document with this uid. pub fn in_group(&self, doc: u64) -> bool { self.groups.iter().any(|g| g.doc == doc) diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs index 7804be6c9..4a5d7c4c9 100644 --- a/crates/engine/src/record.rs +++ b/crates/engine/src/record.rs @@ -18,6 +18,10 @@ pub const RECORD_LIMIT: usize = 100_000; /// without a size cap a recording of a few opens of a large workbook keeps tens of MB alive. pub const RECORD_MAX_BYTES: usize = 64 * 1024 * 1024; +/// Commands never recorded: recovery actions, not steps of the person's work. A recording that +/// replayed Stop Action would stop the run it is a step of. +pub const NOT_RECORDED: [&str; 1] = ["automate.stopAction"]; + /// One recorded command and where it ran. #[derive(Clone, Debug, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index b445377b7..45d57e6f6 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -820,6 +820,32 @@ fn a_call_that_only_opens_a_dialog_is_not_recorded() { assert_eq!(s.stop_recording().unwrap().dropped, 0); } +#[test] +fn stop_action_runs_only_on_its_own_and_is_not_recorded() { + // A step of an action in progress may not stop the action: nested, the command is refused + // and the action's group stays. Nor is Stop Action a step of the person's work: a recording + // leaves it out, so playing the recording back never stops the run it is part of. + let mut s = s(); + s.start_recording(); + let r: crate::Result<()> = s.grouped("Outer", |s| { + set(s, "A1", "one"); + let err = s.execute_nested("automate.stopAction", json!({})).unwrap_err().to_string(); + assert!(err.contains("on its own"), "{err}"); + set(s, "A2", "two"); + Ok(()) + }); + 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. + 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!(s.stop_recording().unwrap().dropped, 0); +} + #[test] fn the_recorder_keeps_top_level_calls_with_where_they_ran() { let mut s = s(); diff --git a/docs/architecture.md b/docs/architecture.md index d243643a4..60e095611 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -65,6 +65,8 @@ Actions made of many commands (macros, a debugger that runs one step per call) u every group it opened, as `group_rollback` does. `Session::abort_groups(doc)`, and the command `automate.stopAction` on the active workbook, roll back the groups left open on a document (a debugger the person abandons, a caller that dropped its token), so Undo works there again. The + command runs on its own: a step of a script or macro may not stop the action it is part of, and + a recording leaves it out, so playing the recording back never stops the run. The history limit is applied when the outermost command finishes; until then the steps made inside it are bounded on their own. - **Recording buffer** (`record.rs`). `Session::start_recording` collects each successful