From bc0d3035fb69af237fcc086688cbbab122c78abf Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 03:18:26 -0400 Subject: [PATCH] Engine: kept macros count toward the macro limit and keep their names; saving over a macro needs replace: true; renaming checks the size - macro.save and a stopped recording count the macros kept in a newer format with the readable ones, so a save never holds more than the next open keeps; the message names the kept count (kept_macros_count_toward_the_limit_and_keep_their_names, which saves and reopens as xlsx and JSON). - The names of kept macros are taken: macro.save and macro.rename refuse them, unique_name and next_name step around them (same test). - macro.save no longer replaces by default: `replace` defaults to false, the error names the existing macro and says to pass `replace: true`, and a `replace` that is not a boolean is refused (saving_over_a_macro_needs_replace). - macro.rename runs the whole macro check, size included, so a longer name can't take a macro past what an xlsx file holds (renaming_checks_the_macro_size). - Docs: macros.md, the management commands and kept macros. --- crates/engine/src/cmd/macros.rs | 39 +++++++++++++---- crates/engine/src/macros/mod.rs | 48 ++++++++++++++++++-- crates/engine/src/tests/macros.rs | 73 +++++++++++++++++++++++++++++++ docs/macros.md | 10 ++++- 4 files changed, 156 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/macros.rs b/crates/engine/src/cmd/macros.rs index a7b9fa9aa..0023cdaf9 100644 --- a/crates/engine/src/cmd/macros.rs +++ b/crates/engine/src/cmd/macros.rs @@ -16,7 +16,7 @@ pub fn specs() -> Vec { "Save Macro", [], None, - "{macro: {name, description?, shortcut?, inputs?: [{name, kind: cell|range, rows: [min, max], cols: [min, max], header, colTypes: [number|text|date|bool|any], nonEmpty, defaultRef?}], steps: [{type: command, id, params, enabled?, breakpoint?, note?} | {type: repeat, over, of: rows|columns, max, body} | {type: if, cell, op: eq|ne|gt|ge|lt|le|empty|notEmpty|contains, value, then, else} | {type: stop, message, rollback?}], output?: {anchor?, onConflict: ask|fail|overwrite|shiftDown|shiftRight}, permissions?: [command ids]} (or those fields at the top level), replace?: bool (default true)} → {name, steps}", + "{macro: {name, description?, shortcut?, inputs?: [{name, kind: cell|range, rows: [min, max], cols: [min, max], header, colTypes: [number|text|date|bool|any], nonEmpty, defaultRef?}], steps: [{type: command, id, params, enabled?, breakpoint?, note?} | {type: repeat, over, of: rows|columns, max, body} | {type: if, cell, op: eq|ne|gt|ge|lt|le|empty|notEmpty|contains, value, then, else} | {type: stop, message, rollback?}], output?: {anchor?, onConflict: ask|fail|overwrite|shiftDown|shiftRight}, permissions?: [command ids]} (or those fields at the top level), replace?: bool (default false: saving over a macro with the name is refused)} → {name, steps}", has_doc, save ), @@ -102,26 +102,44 @@ fn get(s: &mut Session, p: &Json) -> Result { fn save(s: &mut Session, p: &Json) -> Result { let v = p.get("macro").filter(|m| m.is_object()).unwrap_or(p); let m = crate::macros::parse_macro(v).map_err(|e| bad("macro.save", e))?; - let replace = bool_param(p, "replace").unwrap_or(true); + // Replacing is asked for, never assumed: a save with a name in use is refused. + let replace = match p.get("replace") { + None | Some(Json::Null) => false, + Some(Json::Bool(b)) => *b, + Some(_) => return Err(bad("macro.save", "`replace` must be true or false")), + }; let (name, steps) = (m.name.clone(), m.step_count()); edit(s, |cx| { + if let Some(kept) = crate::macros::kept_name(&cx.wb, &m.name) { + return Err(EngineError::Other(taken_by_kept(&kept))); + } match crate::macros::find(&cx.wb, &m.name) { - Some(_) if !replace => return Err(EngineError::Other(format!("A macro called '{}' already exists.", m.name))), + Some(i) if !replace => { + let existing = cx.wb.macros.get(i).map_or(m.name.as_str(), |o| o.name.as_str()); + return Err(EngineError::Other(format!("A macro called '{existing}' already exists. Pass `replace: true` to replace it."))); + } Some(i) => { if let Some(old) = cx.wb.macros.get_mut(i) { *old = m; } } - None if cx.wb.macros.len() >= gridcraft_model::MAX_MACROS => { - return Err(EngineError::Other(format!("A workbook can hold at most {} macros.", gridcraft_model::MAX_MACROS))); + None => { + crate::macros::check_room(&cx.wb).map_err(EngineError::Other)?; + cx.wb.macros.push(m); } - None => cx.wb.macros.push(m), } Ok(()) })?; Ok(json!({"name": name, "steps": steps})) } +/// The refusal of a name that a macro kept in a newer format has. +fn taken_by_kept(name: &str) -> String { + format!( + "A macro called '{name}' made by a newer version of GridCraft is kept in this workbook; this version can't change it. Choose another name." + ) +} + fn delete(s: &mut Session, p: &Json) -> Result { let i = index_of(s, name_param("macro.delete", p)?)?; edit(s, |cx| { @@ -138,14 +156,19 @@ fn rename(s: &mut Session, p: &Json) -> Result { if to.chars().any(char::is_control) { return Err(bad("macro.rename", "a macro name can't hold line breaks or other control characters")); } - if crate::macros::find(&s.doc()?.wb, &to).is_some_and(|j| j != i) { + let wb = &s.doc()?.wb; + if crate::macros::find(wb, &to).is_some_and(|j| j != i) { return Err(EngineError::Other(format!("A macro called '{to}' already exists."))); } + if let Some(kept) = crate::macros::kept_name(wb, &to) { + return Err(EngineError::Other(taken_by_kept(&kept))); + } edit(s, |cx| { let Some(m) = cx.wb.macros.get_mut(i) else { return Ok(()) }; let mut renamed = m.clone(); renamed.name = to.clone(); - renamed.check_limits().map_err(EngineError::Other)?; + // The whole check, size included: a longer name can take a macro past what a file holds. + crate::macros::check(&renamed).map_err(EngineError::Other)?; *m = renamed; Ok(()) })?; diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs index 66886125f..1fc0ab674 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -54,7 +54,8 @@ pub fn check(m: &Macro) -> Result<(), String> { } /// Refuses a macro too large to keep in an xlsx file ([`stash::MAX_ITEM_CHARS`] characters of -/// JSON), so a macro's size never makes a save fail. (A clashing visible name doesn't either: +/// JSON). Every command that adds or changes a macro calls [`check`], so the size of a macro kept +/// in the workbook doesn't make a save fail. (A clashing visible name doesn't either: /// [`with_names`] stores the macro under another index.) fn check_size(m: &Macro) -> Result<(), String> { if macro_json(m).to_string().chars().count() > stash::MAX_ITEM_CHARS { @@ -63,6 +64,42 @@ fn check_size(m: &Macro) -> Result<(), String> { Ok(()) } +/// How many macros the workbook holds: the readable ones and the ones kept in a newer format +/// (`Workbook::macros_unread`), which a save writes back and the next open counts too. +pub fn count(wb: &Workbook) -> usize { + wb.macros.len().saturating_add(wb.macros_unread.len()) +} + +/// Refuses one more macro in a workbook that holds [`gridcraft_model::MAX_MACROS`] already, +/// counting the kept ones, so a save never holds more than the next open keeps. +pub fn check_room(wb: &Workbook) -> Result<(), String> { + let max = gridcraft_model::MAX_MACROS; + if count(wb) < max { + return Ok(()); + } + Err(match wb.macros_unread.len() { + 0 => format!("A workbook can hold at most {max} macros."), + kept => format!( + "A workbook can hold at most {max} macros, and {kept} of the {} in this one {} made by a newer version of GridCraft and kept unchanged.", + count(wb), + if kept == 1 { "was" } else { "were" } + ), + }) +} + +/// The name of the macro kept in a newer format called `name` (compared as [`find`] compares), +/// as the file held it. Its name is taken: a new or renamed macro with it would clash when the +/// newer version reads the file. +pub fn kept_name(wb: &Workbook, name: &str) -> Option { + let name = name.trim(); + wb.macros_unread + .iter() + .filter_map(|v| v.get("name").and_then(Json::as_str)) + .map(str::trim) + .find(|k| k.eq_ignore_ascii_case(name)) + .map(str::to_string) +} + /// Rewrites the short step form into the tagged one, in place, refusing nesting past the limit /// before the steps are deserialized. fn normalize_steps(steps: &mut Json, level: usize) -> Result<(), String> { @@ -284,7 +321,7 @@ fn macro_item(n: &DefinedName) -> Option<(String, Option)> { Some((key, i)) } -/// `name`, or `name (2)`, `name (3)`… if a macro already has it. A free name comes back as it is; +/// `name`, or `name (2)`, `name (3)`… if a macro already has it, a kept one too. A free name comes back as it is; /// a renamed one is shortened to make room for the suffix, so it stays within /// [`gridcraft_model::MAX_MACRO_NAME`] characters. The result still goes through [`check`]. pub fn unique_name(wb: &Workbook, name: &str) -> String { @@ -292,6 +329,9 @@ pub fn unique_name(wb: &Workbook, name: &str) -> String { for m in &wb.macros { taken.insert(&m.name); } + for kept in wb.macros_unread.iter().filter_map(|v| v.get("name").and_then(Json::as_str)) { + taken.insert(kept); + } taken.unique(name) } @@ -350,9 +390,9 @@ impl Taken { } } -/// The first free `Macro1`, `Macro2`… name. +/// The first free `Macro1`, `Macro2`… name, also free among the kept macros ([`kept_name`]). pub fn next_name(wb: &Workbook) -> String { - (1..).map(|n| format!("Macro{n}")).find(|n| find(wb, n).is_none()).unwrap_or_else(|| "Macro".into()) + (1..).map(|n| format!("Macro{n}")).find(|n| find(wb, n).is_none() && kept_name(wb, n).is_none()).unwrap_or_else(|| "Macro".into()) } /// Every command step of `steps` (inside Repeat and If too), with its path. diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 168680c51..33a6f6249 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -1728,3 +1728,76 @@ fn a_recording_waits_while_a_macro_in_its_workbook_is_debugged() { assert_eq!(r["saved"], "Typed"); assert_eq!(recorded(&mut s, "Typed"), ["cell.set {\"cell\":\"Z1\",\"input\":\"typed\"}"]); } + +/// Opens a JSON workbook whose `macros` field holds `macros`, as a file the person opened. +fn open_with_macros(s: &mut Session, macros: Vec) -> Vec { + let mut file = serde_json::to_value(gridcraft_model::Workbook::new()).unwrap(); + file["macros"] = json!(macros); + let r = s.execute("file.open", json!({"name": "kept.json", "base64": crate::io::base64_encode(file.to_string().as_bytes())})).unwrap(); + r["warnings"].as_array().map(|w| w.iter().map(|x| x.as_str().unwrap_or_default().to_string()).collect()).unwrap_or_default() +} + +#[test] +fn kept_macros_count_toward_the_limit_and_keep_their_names() { + // Macros in a newer format count toward the limit when the file opens again, so they count + // when one is added; their names are taken. + let mut s = s(); + let newer = |name: &str| json!({"name": name, "v": gridcraft_model::MACRO_VERSION + 1}); + let mut list: Vec = (0..gridcraft_model::MAX_MACROS - 2).map(|i| json!({"name": format!("M{i}")})).collect(); + list.extend([newer("Future"), newer("Macro1")]); + open_with_macros(&mut s, list); + assert_eq!((s.doc().unwrap().wb.macros.len(), s.doc().unwrap().wb.macros_unread.len()), (gridcraft_model::MAX_MACROS - 2, 2)); + let err = s.execute("macro.save", json!({"name": "New"})).unwrap_err().to_string(); + assert!(err.contains("at most 1000 macros") && err.contains("2 of the 1000"), "{err}"); + s.execute("macro.delete", json!({"name": "M0"})).unwrap(); + for (cmd, p) in [("macro.save", json!({"name": " future "})), ("macro.rename", json!({"name": "M1", "to": "MACRO1"}))] { + let err = s.execute(cmd, p).unwrap_err().to_string(); + assert!(err.contains("newer version"), "{cmd}: {err}"); + } + // A recording takes the next free name, around the kept one, and fills the last place. + s.execute("macro.record", json!({"relative": false})).unwrap(); + set(&mut s, "A1", "typed"); + assert_eq!(s.execute("macro.record", json!({"stop": true})).unwrap()["saved"], "Macro2"); + assert_eq!(crate::macros::unique_name(&s.doc().unwrap().wb, "Future"), "Future (2)"); + s.execute("macro.record", json!({"relative": false})).unwrap(); + set(&mut s, "A2", "more"); + let err = s.execute("macro.record", json!({"stop": true, "name": "Late"})).unwrap_err().to_string(); + assert!(err.contains("2 of the 1000"), "{err}"); + s.execute("macro.record", json!({"cancel": true})).unwrap(); + // Saved and opened again: every macro, the kept ones too, is still there. + for format in ["xlsx", "json"] { + let b64 = s.execute("file.saveBytes", json!({"format": format})).unwrap()["base64"].as_str().unwrap().to_string(); + s.execute("file.open", json!({"name": format!("again.{format}"), "base64": b64})).unwrap(); + let wb = &s.doc().unwrap().wb; + assert_eq!((wb.macros.len(), wb.macros_unread.len()), (gridcraft_model::MAX_MACROS - 2, 2), "{format}"); + assert_eq!(*wb.macros_unread, [newer("Future"), newer("Macro1")], "{format}"); + } +} + +#[test] +fn renaming_checks_the_macro_size() { + // A longer name can take a macro past the size a file holds: renaming is refused, as saving is. + let mut s = s(); + let mut m = json!({"name": "A", "steps": [{"command": "cell.set", "params": {"cell": "A1", "input": ""}}]}); + let size = |m: &Json| crate::macros::macro_json(&crate::macros::parse_macro(m).unwrap()).to_string().chars().count(); + let pad = crate::cmd::stash::MAX_ITEM_CHARS - size(&m); + m["steps"][0]["params"]["input"] = json!("x".repeat(pad)); + assert_eq!(size(&m), crate::cmd::stash::MAX_ITEM_CHARS); + s.execute("macro.save", json!({"macro": m})).unwrap(); + let err = s.execute("macro.rename", json!({"name": "A", "to": "Longer"})).unwrap_err().to_string(); + assert!(err.contains("too large"), "{err}"); + s.execute("macro.rename", json!({"name": "A", "to": "B"})).unwrap(); + crate::io::save_bytes(&s.doc().unwrap().wb, "book.xlsx").unwrap(); +} + +#[test] +fn saving_over_a_macro_needs_replace() { + let mut s = s(); + s.execute("macro.save", json!({"name": "Tidy", "description": "first"})).unwrap(); + let err = s.execute("macro.save", json!({"name": "TIDY", "description": "second"})).unwrap_err().to_string(); + assert!(err.contains("'Tidy' already exists") && err.contains("replace: true"), "{err}"); + assert!(s.execute("macro.save", json!({"name": "Tidy", "replace": "yes"})).is_err()); + assert_eq!(s.execute("macro.get", json!({"name": "Tidy"})).unwrap()["description"], "first"); + s.execute("macro.save", json!({"name": "TIDY", "description": "second", "replace": true})).unwrap(); + assert_eq!(s.execute("macro.get", json!({"name": "Tidy"})).unwrap()["description"], "second"); +} diff --git a/docs/macros.md b/docs/macros.md index 1e2817abf..b7c196956 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -178,7 +178,9 @@ it ran: editing or deleting the script before the recording stops changes nothin ## Other commands and storage `macro.list`, `macro.get`, `macro.save {macro, replace?}`, `macro.delete` and `macro.rename -{name, to}`. Saving, renaming and deleting are undoable. +{name, to}`. Saving under a name a macro has is refused unless `replace: true` is passed, and +renaming to one is refused; names compare ignoring the case of the letters A to Z. A renamed macro is checked +against every limit again, its size included. Saving, renaming and deleting are undoable. Macros are saved in the workbook: as a field in GridCraft's JSON format, and in xlsx as hidden defined names (`_gc_macro__`, the macro's JSON split into string literals within Excel's @@ -190,7 +192,11 @@ Each macro carries its format version, `v` (1 in this version; a missing `v` rea file is opened, a macro in a newer format (a `v` above 1) is kept unread, with a warning: this version can't show or run it, and saving writes it back unchanged, so the newer version gets it back whole. It is never read in part. Kept macros count toward the 1,000-macro -limit after the readable ones, and one too large for an xlsx file is left out. A macro that is +limit after the readable ones, and one too large for an xlsx file is left out. They count when a +macro is added too (saving, or stopping a recording, is refused once readable and kept macros +together reach 1,000, and the message says how many are kept), so a save holds no more macros +than the next open keeps. Their names are taken: saving or renaming a macro to one is refused, and +a recording gets a suffix. A macro that is damaged or over the limits is left out with one warning, and the rest of the workbook and its other macros open; a macro that was left out is not in the saved file.