From 09a37a8f10a4c33a9407448dbdeb3661f61a5b59 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:59:59 -0400 Subject: [PATCH] Form controls: a kept button action comes back only on the same button The button actions xlsx has no place for (commands, fixed inputs, output cells) are kept in a hidden name keyed by sheet and the control's place on the sheet. When another application added, removed or reordered controls before saving, opening the file gave the action to whichever button now had that place, and could replace a macro the file itself gave that button. Each entry now also holds the button's name and anchor cell, and is restored only on a button with that name and cell whose own action from the file does not contradict it: a stored macro action only on a button that still runs that macro, a stored command only on a button the file gives no macro. --- crates/engine/src/cmd/controls.rs | 29 ++++++++++++++---- crates/engine/src/tests/controls.rs | 46 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/controls.rs b/crates/engine/src/cmd/controls.rs index 4a9ade919..d2cf17f01 100644 --- a/crates/engine/src/cmd/controls.rs +++ b/crates/engine/src/cmd/controls.rs @@ -563,14 +563,15 @@ fn plain(a: &ControlAction) -> bool { } /// A copy of `wb` for xlsx with the button actions xlsx can't express (commands, fixed inputs, -/// output cells) in a hidden name, keyed by sheet name and the control's place on the sheet. -/// `None` when there is nothing to store. +/// output cells) in a hidden name, keyed by sheet name and the control's place on the sheet, with +/// the button's name and anchor cell to check it is still the same button when the file comes +/// back. `None` when there is nothing to store. pub(crate) fn with_action_names(wb: &Workbook) -> Result> { let mut items = Vec::new(); for sh in &wb.sheets { for (i, c) in sh.controls.iter().enumerate() { if let Some(a) = c.action.as_ref().filter(|a| !plain(a)) { - items.push(json!({"sheet": sh.name, "index": i, "action": a})); + items.push(json!({"sheet": sh.name, "index": i, "name": c.display_name(), "at": c.anchor.cell.a1(), "action": a})); } } } @@ -587,7 +588,11 @@ pub(crate) fn with_action_names(wb: &Workbook) -> Result> { } /// After a file is opened: gives buttons back the actions kept in the hidden name, and removes -/// it. Entries that don't match a button, or don't parse, are dropped. +/// it. Entries that don't parse, or don't match a button, are dropped: the button at the stored +/// place must have the stored name and anchor cell (another application may have added, removed +/// or reordered controls), and must not have been given another macro since (a stored macro +/// action comes back only on a button that still runs that macro, a stored command only on one +/// the file gives no macro). pub(crate) fn load_action_names(wb: &mut Workbook) { let Some(items) = stash::get_json(wb, ACTIONS_KEY) else { return }; stash::remove(wb, ACTIONS_KEY); @@ -599,10 +604,22 @@ pub(crate) fn load_action_names(wb: &mut Workbook) { ControlAction::Command { id, params } => !id.is_empty() && id.len() <= 200 && params.is_object(), }; let Some(si) = wb.sheet_index(sheet) else { continue }; - if !valid || !wb.sheet(si).is_some_and(|sh| sh.controls.get(index as usize).is_some_and(|c| c.kind == ControlKind::Button)) { + let Ok(index) = usize::try_from(index) else { continue }; + let text = |k: &str| item.get(k).and_then(Json::as_str); + let same = |c: &FormControl| { + c.kind == ControlKind::Button + && text("name").is_none_or(|n| n == c.display_name()) + && text("at").is_none_or(|a| a == c.anchor.cell.a1()) + && match (&c.action, &action) { + (None, _) => true, + (Some(ControlAction::Macro { name: now, .. }), ControlAction::Macro { name: kept, .. }) => now.eq_ignore_ascii_case(kept), + _ => false, + } + }; + if !valid || !wb.sheet(si).is_some_and(|sh| sh.controls.get(index).is_some_and(same)) { continue; } - if let Some(c) = wb.sheet_mut(si).and_then(|sh| sh.controls.get_mut(index as usize)) { + if let Some(c) = wb.sheet_mut(si).and_then(|sh| sh.controls.get_mut(index)) { c.action = Some(action); } } diff --git a/crates/engine/src/tests/controls.rs b/crates/engine/src/tests/controls.rs index 61de5caa2..98b2404ae 100644 --- a/crates/engine/src/tests/controls.rs +++ b/crates/engine/src/tests/controls.rs @@ -351,6 +351,52 @@ fn reopen(s: &mut Session) { s.execute("file.open", json!({"name": "x.xlsx", "base64": b64})).unwrap(); } +#[test] +fn a_kept_button_action_comes_back_only_on_the_same_button() { + use crate::cmd::controls::{load_action_names, with_action_names}; + let mut s = s(); + s.execute("macro.save", json!({"name": "Mark", "steps": [{"command": "cell.set", "params": {"cell": "A1", "input": "m"}}]})).unwrap(); + s.execute("insert.button", json!({"at": "D2", "text": "Run", "command": "cell.set", "params": {"cell": "A9", "input": "cmd"}})).unwrap(); + s.execute("insert.button", json!({"at": "D6", "text": "Other"})).unwrap(); + s.execute("insert.button", json!({"at": "D9", "text": "Fixed", "macro": "Mark", "input": "F1:G2"})).unwrap(); + let wb = (*s.doc().unwrap().wb).clone(); + let stored = with_action_names(&wb).unwrap().unwrap(); + let actions = |wb: &gridcraft_model::Workbook| wb.sheet(0).unwrap().controls.iter().map(|c| c.action.clone()).collect::>(); + let kept = |wb: &gridcraft_model::Workbook| { + let mut wb = wb.clone(); + for c in &mut wb.sheet_mut(0).unwrap().controls { + // As xlsx carries them: a macro on the selection, or nothing for a command. + c.action = match c.action.take() { + Some(ControlAction::Macro { name, .. }) => Some(ControlAction::Macro { name, input: MacroInput::Selection, output: None }), + _ => None, + }; + } + wb + }; + // The same buttons in the same places get their actions back. + let mut same = kept(&stored); + load_action_names(&mut same); + assert_eq!(actions(&same), actions(&wb)); + // Another application removed the first button: the command must not move to "Other". + let mut shifted = kept(&stored); + shifted.sheet_mut(0).unwrap().controls.remove(0); + load_action_names(&mut shifted); + assert_eq!(shifted.sheets[0].controls[0].action, None, "the command stays with the removed button"); + // A button given another macro there keeps it; a command doesn't replace a macro either. + let mut reassigned = kept(&stored); + for i in [0, 2] { + reassigned.sheet_mut(0).unwrap().controls[i].action = + Some(ControlAction::Macro { name: "Other".into(), input: MacroInput::Selection, output: None }); + } + load_action_names(&mut reassigned); + for i in [0, 2] { + assert!( + matches!(&reassigned.sheets[0].controls[i].action, Some(ControlAction::Macro { name, input: MacroInput::Selection, .. }) if name == "Other"), + "{i}" + ); + } +} + #[test] fn every_kind_and_action_survives_xlsx() { let mut s = s();