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();