From 4668fc30ae985bdc028371a84f496ea863276498 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:57:14 -0400 Subject: [PATCH] Engine: the name commands leave GridCraft's data names alone `formulas.defineName {"name": "_gc_macro_1_0"}` replaced the hidden chunk of a stored item with a visible name: the item vanished from `stash::get` and `stash::keys`, and its other chunks stayed behind as orphan hidden names. `formulas.deleteName` could remove chunks the same way, and `formulas.createFromSelection` with `replace` could replace one from a cell label. `stash::set` refused a clash with a visible name, but nothing guarded the other direction. Defining, renaming or deleting a name that `stash::item_key` recognises (`_gc__` and legacy `_sc_`) is now refused with an error that names `force: true`, which lets the call go ahead. Create from Selection lists such labels in `skipped` unless `force` is true. The params docs of the three commands say so. --- crates/engine/src/cmd/formulas.rs | 75 +++++++++++++++++++-- crates/engine/tests/it/stored_list_shape.rs | 3 +- docs/architecture.md | 2 + 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/formulas.rs b/crates/engine/src/cmd/formulas.rs index 5334784fa..c5f8a8bf8 100644 --- a/crates/engine/src/cmd/formulas.rs +++ b/crates/engine/src/cmd/formulas.rs @@ -32,18 +32,26 @@ pub fn specs() -> Vec { "Define Name…", ["Formulas", "Defined Names"], None, - "{name, refersTo?: \"=Sheet1!$A$1:$A$10\" (default: the selection), scope?: \"Workbook\"|sheet name, comment?, oldName?, oldScope?} (oldName: edit that name (Name Manager › Edit); a new name also renames it in formulas)", + "{name, refersTo?: \"=Sheet1!$A$1:$A$10\" (default: the selection), scope?: \"Workbook\"|sheet name, comment?, oldName?, oldScope?, force?: bool} (oldName: edit that name (Name Manager › Edit); a new name also renames it in formulas; names GridCraft keeps its own data in, `_gc__` and `_sc_`, are refused unless force is true)", has_doc, define_name ), - cmd!("formulas.deleteName", "Delete Name", [], None, "{name, scope?: \"Workbook\"|sheet name (default: every scope)}", has_doc, delete_name), + cmd!( + "formulas.deleteName", + "Delete Name", + [], + None, + "{name, scope?: \"Workbook\"|sheet name (default: every scope), force?: bool} (names GridCraft keeps its own data in, `_gc__` and `_sc_`, are refused unless force is true)", + has_doc, + delete_name + ), cmd!(query "formulas.nameManager", "Name Manager", ["Formulas", "Defined Names"], Some("Cmd+F3"), "{} → names with values", has_doc, name_manager), cmd!( "formulas.createFromSelection", "Create from Selection", ["Formulas", "Defined Names"], Some("Cmd+Shift+F3"), - "{range?, top?: true, left?: false, bottom?: false, right?: false, replace?: false (existing names are kept and listed in `skipped`)}", + "{range?, top?: true, left?: false, bottom?: false, right?: false, replace?: false (existing names are kept and listed in `skipped`), force?: false (labels of the shape GridCraft keeps its own data in, `_gc__` and `_sc_`, are listed in `skipped` unless force is true)}", has_doc, create_from_selection ), @@ -268,6 +276,9 @@ fn define_name(s: &mut Session, p: &Json) -> Result { if !valid_name(&name) { return Err(EngineError::Other("The name that you entered is not valid.".into())); } + for n in [Some(name.as_str()), str_param(p, "oldName")].into_iter().flatten() { + refuse_stash_name(p, n)?; + } let d = s.doc()?; let scope = match str_param(p, "scope") { None | Some("Workbook") => None, @@ -374,8 +385,21 @@ fn rename_name_in_formulas(wb: &mut gridcraft_model::Workbook, old: &str, old_sc } } +/// Refuses a name of the shape GridCraft keeps its own workbook data in ([`super::stash`]): +/// defining, renaming or deleting one would replace or remove part of an item (a stored macro, +/// script or view) and leave the rest behind. `force: true` lets the call go ahead. +fn refuse_stash_name(p: &Json, name: &str) -> Result<()> { + if stash::item_key(name).is_some() && !bool_param(p, "force").unwrap_or(false) { + return Err(EngineError::Other(format!( + "'{name}' is a name GridCraft keeps its own data in (stored macros, scripts or views). Pass `force: true` to change it anyway." + ))); + } + Ok(()) +} + fn delete_name(s: &mut Session, p: &Json) -> Result { let name = str_param(p, "name").ok_or_else(|| bad("formulas.deleteName", "missing `name`"))?.to_string(); + refuse_stash_name(p, &name)?; let scope = match str_param(p, "scope") { None => None, Some("Workbook") => Some(None), @@ -461,7 +485,11 @@ fn create_from_selection(s: &mut Session, p: &Json) -> Result { // Excel asks before replacing an existing name; here it's kept unless `replace` is set, and // reported. let exists = |n: &str| d.wb.names.iter().any(|x| x.name.eq_ignore_ascii_case(n) && x.scope.is_none()); - let (made, skipped): (Vec<_>, Vec<_>) = made.into_iter().partition(|(n, _)| replace || !exists(n)); + // A label of the shape GridCraft keeps its own data in would replace part of an item, or + // stand in the way of one (see `refuse_stash_name`). + let force = bool_param(p, "force").unwrap_or(false); + let ours = |n: &str| !force && stash::item_key(n).is_some(); + let (made, skipped): (Vec<_>, Vec<_>) = made.into_iter().partition(|(n, _)| !ours(n) && (replace || !exists(n))); let skipped: Vec = skipped.into_iter().map(|(n, _)| n).collect(); let n = made.len(); edit(s, |cx| { @@ -742,6 +770,45 @@ mod tests { assert_eq!(r["refersTo"], "=Sheet1!$A$1"); } + #[test] + fn the_name_commands_leave_gridcraft_data_alone() { + // Defining, renaming or deleting a name of a stored item's shape would replace or remove + // one of its chunks and leave the rest behind: refused unless `force` is true. + let mut s = Session::new(); + s.new_workbook(); + let wb = std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb); + let text = "m".repeat(9_000); + stash::set(wb, "macro_1", &text).unwrap(); + wb.names.push(DefinedName { name: "_sc_watch".into(), scope: None, formula: "\"[]\"".into(), comment: String::new(), hidden: true }); + let names = |s: &Session| s.doc().unwrap().wb.names.clone(); + let before = names(&s); + assert!(before.len() >= 3, "the item takes two chunks"); + for params in [ + json!({"name": "_gc_macro_1_0", "refersTo": "=1"}), + json!({"name": "_SC_Watch", "refersTo": "=1"}), + json!({"name": "Mine", "refersTo": "=1", "oldName": "_gc_macro_1_1"}), + json!({"name": "_gc_macro_1_0", "refersTo": "=1", "force": false}), + ] { + let err = s.execute("formulas.defineName", params.clone()).unwrap_err().to_string(); + assert!(err.contains("force"), "{params}: {err}"); + } + for name in ["_gc_macro_1_1", "_sc_watch"] { + let err = s.execute("formulas.deleteName", json!({"name": name})).unwrap_err().to_string(); + assert!(err.contains("force"), "{name}: {err}"); + } + s.execute("range.setValues", json!({"range": "A1:B2", "values": [["_gc_macro_1_0", "Rate"], [1, 2]]})).unwrap(); + let r = s.execute("formulas.createFromSelection", json!({"range": "A1:B2", "top": true, "replace": true})).unwrap(); + assert_eq!((r["created"].clone(), r["skipped"].clone()), (json!(1), json!(["_gc_macro_1_0"]))); + let after = names(&s); + assert_eq!(after.iter().filter(|n| n.name != "Rate").cloned().collect::>(), before); + assert_eq!(stash::get(&s.doc().unwrap().wb, "macro_1").as_deref(), Some(text.as_str())); + // Ordinary names and forced calls go ahead. + s.execute("formulas.deleteName", json!({"name": "Rate"})).unwrap(); + s.execute("formulas.defineName", json!({"name": "_gc_macro_1_1", "refersTo": "=1", "force": true})).unwrap(); + s.execute("formulas.deleteName", json!({"name": "_gc_macro_1_1", "force": true})).unwrap(); + assert!(!names(&s).iter().any(|n| n.name == "_gc_macro_1_1")); + } + #[test] fn arabic_defined_names_count_characters_not_bytes() { let mut s = Session::new(); diff --git a/crates/engine/tests/it/stored_list_shape.rs b/crates/engine/tests/it/stored_list_shape.rs index 0d38cd5f9..fd17a4806 100644 --- a/crates/engine/tests/it/stored_list_shape.rs +++ b/crates/engine/tests/it/stored_list_shape.rs @@ -6,7 +6,8 @@ const NOT_OBJECTS: [&str; 4] = ["\"[1,2]\"", "\"7\"", "\"true\"", "\"\"\"hi\"\"\ fn with_name(name: &str, refers_to: &str) -> Session { let mut s = Session::new(); s.new_workbook(); - s.execute("formulas.defineName", json!({"name": name, "refersTo": refers_to})).unwrap(); + // `force`: the name commands refuse GridCraft's own data names otherwise. + s.execute("formulas.defineName", json!({"name": name, "refersTo": refers_to, "force": true})).unwrap(); s } diff --git a/docs/architecture.md b/docs/architecture.md index 2e80f5606..3fc6cb0b1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -85,6 +85,8 @@ Actions made of many commands (macros, a debugger that runs one step per call) u U+FDD0 and four hex digits, as is U+FDD0 itself, and read back as they were. `stash::items` and `stash::keys` list the items under a key prefix in one pass over the names, at most 10,000 of them (`stash::MAX_ITEMS`). The older single-name `_sc_` items are read as well. + `formulas.defineName` and `formulas.deleteName` refuse names of either shape, and + `formulas.createFromSelection` skips labels of either shape, unless `force: true` is passed. ## Agent control