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

@@ -184,10 +184,20 @@ fn start_recording(s: &mut Session, relative: bool) -> Result<Json> {
     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<Json> {
     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<RecordingStart>) -> Result<Json> {
     let d = s.doc()?;
     let start = start.unwrap_or_else(|| RecordingStart {
         doc: d.uid,

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<String> {
+    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\"}"]);
+}

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