From 252ffa95b6a9ee8c67b1a738770348bc081295d4 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:21:46 -0400 Subject: [PATCH] Engine: macro recorder keeps a recording when saving it fails Stopping a recording took the recorded commands and where recording started before saving the macro, so a failed save threw both away. For example, stopping with a name that holds a control character, or in a workbook at its macro limit, ended the recording, and stopping again with a good name found nothing to save. A failed save now puts the recording back, so stopping again saves what was recorded. --- crates/engine/src/cmd/macros.rs | 12 ++++++++- crates/engine/src/tests/macros.rs | 43 +++++++++++++++++++++++++++++++ docs/macros.md | 14 +++++----- 3 files changed, 62 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/macros.rs b/crates/engine/src/cmd/macros.rs index e68dd92e6..953be1982 100644 --- a/crates/engine/src/cmd/macros.rs +++ b/crates/engine/src/cmd/macros.rs @@ -184,10 +184,20 @@ fn start_recording(s: &mut Session, relative: bool) -> Result { Ok(if relative { json!({"recording": true, "relative": true, "input": input}) } else { json!({"recording": true, "relative": false}) }) } -/// Stops recording and saves what was recorded as a macro. +/// Stops recording and saves what was recorded as a macro. When saving fails (a bad name, a full +/// workbook) the recording goes on, so stopping again keeps what was recorded. fn stop_recording(s: &mut Session, p: &Json) -> Result { let Some(rec) = s.stop_recording() else { return Ok(json!({"recording": false})) }; let start = s.macro_recording.take(); + let saved = save_recording(s, p, &rec, start.clone()); + if saved.is_err() { + s.recorder = Some(rec); + s.macro_recording = start; + } + saved +} + +fn save_recording(s: &mut Session, p: &Json, rec: &crate::record::Recorder, start: Option) -> Result { let d = s.doc()?; let start = start.unwrap_or_else(|| RecordingStart { doc: d.uid, diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 941abe906..c32fa2d43 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -967,3 +967,46 @@ fn only_the_workbooks_a_debugged_run_touched_wait() { debug(&mut s, json!({"action": "stop"})); set(&mut s, "A1", "free"); } + +/// The commands of saved macro `name`, as `id params`. +fn recorded(s: &mut Session, name: &str) -> Vec { + let m = crate::macros::parse_macro(&s.execute("macro.get", json!({"name": name})).unwrap()).unwrap(); + crate::macros::command_steps(&m.steps).iter().map(|(_, id, p)| format!("{id} {p}")).collect() +} + +#[test] +fn a_recording_is_kept_when_saving_it_fails() { + let mut s = s(); + s.execute("macro.record", json!({"relative": false})).unwrap(); + set(&mut s, "A1", "typed"); + // A name the macro check refuses: still recording, and a good name saves what was recorded. + assert!(s.execute("macro.record", json!({"stop": true, "name": "Bad\u{1}name"})).is_err()); + assert!(s.is_recording()); + set(&mut s, "A2", "more"); + let r = s.execute("macro.record", json!({"stop": true, "name": "Good"})).unwrap(); + assert_eq!(r["saved"], "Good"); + assert!(!s.is_recording()); + assert_eq!(recorded(&mut s, "Good"), ["cell.set {\"cell\":\"A1\",\"input\":\"typed\"}", "cell.set {\"cell\":\"A2\",\"input\":\"more\"}"]); + + // A workbook at its macro limit: freeing a slot and stopping again saves it. + s.execute("macro.record", json!({"relative": false})).unwrap(); + set(&mut s, "B1", "full"); + let d = s.doc_mut().unwrap(); + let mut wb = (*d.wb).clone(); + let filler = wb.macros[0].clone(); + while wb.macros.len() < gridcraft_model::MAX_MACROS { + let mut m = filler.clone(); + m.name = format!("Filler{}", wb.macros.len()); + wb.macros.push(m); + } + d.wb = Arc::new(wb); + let err = s.execute("macro.record", json!({"stop": true, "name": "Late"})).unwrap_err().to_string(); + assert!(err.contains("at most"), "{err}"); + assert!(s.is_recording()); + let d = s.doc_mut().unwrap(); + let mut wb = (*d.wb).clone(); + wb.macros.pop(); + d.wb = Arc::new(wb); + s.execute("macro.record", json!({"stop": true, "name": "Late"})).unwrap(); + assert_eq!(recorded(&mut s, "Late"), ["cell.set {\"cell\":\"B1\",\"input\":\"full\"}"]); +} diff --git a/docs/macros.md b/docs/macros.md index 79e6c02c5..8587ca877 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -119,12 +119,14 @@ of the step that just ran, and the cells changed so far. ## Recording `macro.record` toggles recording (`relative: false` for absolute references; `name` and -`description` when stopping). With relative references the selection when recording starts is the -example input: references in and near it become tokens, and the input's shape, header and column -types are inferred. Toggles such as Bold are recorded with the state they set (`"on": true`), so -playing them back sets them rather than flipping them. Commands a macro may not run are left out -and listed in the result; a macro run while recording is replaced by its steps when it has no -inputs, and an Automate script run by the script's steps. +`description` when stopping). When saving the recording fails (a name the check refuses, a workbook +at its macro limit), recording goes on, so stopping again keeps what was recorded. With relative +references the selection when recording starts is the example input: references in and near it +become tokens, and the input's shape, header and column types are inferred. Toggles such as Bold +are recorded with the state they set (`"on": true`), so playing them back sets them rather than +flipping them. Commands a macro may not run are left out and listed in the result; a macro run +while recording is replaced by its steps when it has no inputs, and an Automate script run by the +script's steps. ## Other commands and storage