From ad79b071ce8dfa22647cb4910530735c7cbb988b Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:40:21 -0400 Subject: [PATCH] Engine: a macro's inputs, output and Repeat ranges follow their cells when a step moves them Inputs, the output and a Repeat's range were bound once, by sheet index and coordinates. A step that inserted a sheet in front of an input's sheet sent every later token to the new sheet, and rows inserted above an input left the tokens on the cells that moved into its place; the run wrote into the wrong cells and reported success. - Commands record the structural edits they make as they rewrite references (`Ctx::edits`; while a macro step runs, `DocState::edits` collects them). - After each step the machine matches the sheets as the run's guard does and moves the inputs, the output and the ranges of the Repeats in progress by those edits, with the formula engine's own adjustment. - A step that deletes an input's or the output's cells, or their sheet, or changes the size of a range a Repeat is going over, stops the run, which rolls back. - Test: `inputs_and_the_output_follow_their_cells_when_a_step_moves_them` (a sheet inserted in front, rows inserted above, an input deleted, a Repeat's range grown). --- crates/engine/src/cmd/edit.rs | 1 + crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/mod.rs | 18 +++++++- crates/engine/src/cmd/sheet.rs | 18 ++++---- crates/engine/src/lib.rs | 4 ++ crates/engine/src/macros/run.rs | 68 ++++++++++++++++++++++++++++++- crates/engine/src/tests/macros.rs | 53 ++++++++++++++++++++++++ docs/macros.md | 5 ++- 8 files changed, 155 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 102edc2d3..556d41759 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1386,6 +1386,7 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { gridcraft_formula::adjust::Edit::MoveToSheet { from: src, to_sheet: dest_name, to_row: at.row, to_col: at.col } }; rewrite_all_formulas(&mut cx.wb, &src_sheet_name, &e); + cx.edits.push((src_sheet_name.clone(), e)); cx.structural = true; } let end = at.offset_clamped((tiles_r * h) as i64 - 1, (tiles_c * w) as i64 - 1); diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 3c45031af..0078cb4fe 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -294,7 +294,7 @@ fn insert_table(s: &mut Session, p: &Json) -> Result { let e = Edit::InsertCells { axis: Axis::Rows, range: RangeRef::new(r.start, CellRef::new(r.start.row, r.end.col)) }; shift_cell_features(sh, &e); let name = sh.name.clone(); - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); let sh = cx.sheet_mut(sheet)?; for (i, c) in (r.start.col..=r.end.col).enumerate() { sh.set_value(CellRef::new(r.start.row, c), gridcraft_core::Value::text(format!("Column{}", i + 1))); diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 6f58f18ee..55da0377e 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -340,6 +340,9 @@ pub struct Ctx<'a> { /// The command applied sheet protection itself (row/column inserts and deletes the /// protection allows), so cells it shifts aren't refused as edits of locked cells. pub protection_checked: bool, + /// The structural edits the command made, in order (the sheet's name, the edit), as + /// references were rewritten for them. + pub edits: Vec<(String, gridcraft_formula::adjust::Edit)>, } pub(crate) fn edit(s: &mut Session, f: impl FnOnce(&mut Ctx) -> Result) -> Result { @@ -357,15 +360,26 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) #[allow(clippy::disallowed_methods)] let t0 = if cfg!(target_arch = "wasm32") { None } else { std::env::var_os("GRIDCRAFT_PROFILE").map(|_| std::time::Instant::now()) }; let mut sel = d.selection.clone(); - let mut ctx = Ctx { wb: (*d.wb).clone(), changed: Vec::new(), structural: false, sel: &mut sel, fit_rows: Vec::new(), protection_checked: false }; + let mut ctx = Ctx { + wb: (*d.wb).clone(), + changed: Vec::new(), + structural: false, + sel: &mut sel, + fit_rows: Vec::new(), + protection_checked: false, + edits: Vec::new(), + }; let r = f(&mut ctx)?; - let Ctx { mut wb, changed, structural, mut fit_rows, protection_checked, .. } = ctx; + let Ctx { mut wb, changed, structural, mut fit_rows, protection_checked, edits, .. } = ctx; if !protection_checked { check_protection(&d.wb, &wb, &d.unlocked_ranges)?; } if !structural { check_arrays(&d.wb, &wb)?; } + if let Some(log) = d.edits.as_mut() { + log.extend(edits); + } if changed.len() <= 200_000 { fit_rows.extend(changed.iter().map(|(s, c)| (*s, c.row))); } diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index e6caa1dc7..dfab93736 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -223,7 +223,9 @@ fn shift_sheet_features(sh: &mut Sheet, axis: Axis, at: u32, count: u32, insert: } /// Rewrites every formula reference (and chart series) for a structural edit on `target`. -pub(super) fn rewrite(wb: &mut gridcraft_model::Workbook, target: &str, e: &Edit) { +pub(super) fn rewrite(cx: &mut Ctx, target: &str, e: &Edit) { + cx.edits.push((target.to_string(), e.clone())); + let wb = &mut cx.wb; super::edit::rewrite_all_formulas(wb, target, e); for si in 0..wb.sheets.len() { let host = wb.sheets.get(si).map(|s| s.name.clone()).unwrap_or_default(); @@ -307,7 +309,7 @@ fn insert_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } } } - rewrite(&mut cx.wb, &name, &Edit::Insert { axis, at, count }); + rewrite(cx, &name, &Edit::Insert { axis, at, count }); cx.structural = true; Ok(Json::Null) })?; @@ -372,7 +374,7 @@ fn delete_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } shift_sheet_features(sh, axis, at, count, false); } - rewrite(&mut cx.wb, &name, &Edit::Delete { axis, at, count }); + rewrite(cx, &name, &Edit::Delete { axis, at, count }); } cx.structural = true; Ok(Json::Null) @@ -424,7 +426,7 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { sh.cells.shift_cols(before, count as i64); shift_sheet_features(sh, Axis::Cols, before, count, true); } - rewrite(&mut cx.wb, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); + rewrite(cx, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); let mut touched = Vec::new(); { @@ -462,14 +464,14 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { for cell in touched { cx.touch(sheet, cell); } - rewrite(&mut cx.wb, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); + rewrite(cx, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); { let sh = cx.sheet_mut(sheet)?; sh.cells.shift_cols(delete_at + count, -(count as i64)); shift_sheet_features(sh, Axis::Cols, delete_at, count, false); } - rewrite(&mut cx.wb, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); + rewrite(cx, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); *cx.sel = crate::Selection { active: CellRef::new(active_row, final_start), anchor: CellRef::new(active_row, final_start), @@ -514,7 +516,7 @@ fn insert_cells(s: &mut Session, p: &Json) -> Result { } shift_cell_features(sh, &e); let name = cx.wb.sheet(sheet).map(|s| s.name.clone()).unwrap_or_default(); - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); cx.structural = true; Ok(Json::Null) })?; @@ -549,7 +551,7 @@ fn delete_cells(s: &mut Session, p: &Json) -> Result { shift_cell_features(sh, &e); } // References into the deleted cells become #REF!; the rest move. - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); cx.structural = true; Ok(Json::Null) })?; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 151c2bc06..23a09c9ee 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -206,6 +206,9 @@ pub struct DocState { /// workbook is opened from a file, carried by content pasted from such a workbook, and never /// changed by a trust decision. pub trust: Trust, + /// While a macro step runs: the structural edits its command made (the sheet's name, the + /// edit), so the macro's inputs, output and Repeat ranges follow their cells as formulas do. + pub edits: Option>, /// The references `edit.goTo` with `record: true` went to in this document, newest first, /// without names, each area written with its sheet (`Sheet1!B14:C22,Sheet1!F19:G30`). pub go_to_recent: Vec, @@ -299,6 +302,7 @@ impl DocState { content_kind: None, scripts_trusted, trust, + edits: None, go_to_recent: vec![], } } diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 8ae1e3496..36c4cae00 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -22,6 +22,7 @@ use std::collections::BTreeSet; use std::sync::Arc; use gridcraft_core::{CellRef, RangeRef, Value}; +use gridcraft_formula::adjust::Edit; use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, Sheet, Step, StepKind, Workbook}; use serde::Serialize; use serde_json::{Value as Json, json}; @@ -645,6 +646,34 @@ impl Machine { } } + /// After a step that inserted, deleted or moved sheets, rows, columns or cells, moves the + /// inputs, the output and the ranges of the Repeats in progress with their cells, as the + /// references in formulas move. A step that deletes one of them, or changes the size of a + /// range a Repeat is going over, stops the run. + fn follow(&mut self, before: &Workbook, after: &Workbook, edits: &[(String, Edit)]) -> std::result::Result<(), String> { + let same_sheets = before.sheets.len() == after.sheets.len() && before.sheets.iter().zip(&after.sheets).all(|(b, a)| b.name == a.name); + if same_sheets && edits.is_empty() { + return Ok(()); + } + let (pairs, _) = pair_sheets(before, after); + let follow = |what: &str, p: Place| follow_place(before, after, &pairs, edits, p).ok_or_else(|| format!("It deleted the cells of {what}.")); + for (name, place, _) in self.inputs.iter_mut() { + *place = follow(name, *place)?; + } + if let Some(o) = self.output { + self.output = Some(follow("the output anchor", o)?); + } + for f in self.frames.iter_mut() { + let Some(l) = f.repeat.as_mut() else { continue }; + let over = follow(&format!("the range Repeat {} goes over", f.prefix), l.over)?; + if (over.range.height(), over.range.width()) != (l.over.range.height(), l.over.range.width()) { + return Err(format!("It changed the size of the range Repeat {} goes over.", f.prefix)); + } + l.over = over; + } + Ok(()) + } + fn env<'a>(&'a self, wb: &'a Workbook) -> Env<'a> { Env { wb, active_sheet: wb.active_sheet, inputs: &self.inputs, output: self.output, item: self.item() } } @@ -670,9 +699,13 @@ impl Machine { (_, Policy::Allow | Policy::Ask(_)) => {} } self.last_params = Some(resolved.clone()); - let prev = self.guard.as_ref().and_then(|_| s.docs.iter().find(|d| d.uid == self.doc)).map(|d| d.wb.clone()); + let prev = s.docs.iter_mut().find(|d| d.uid == self.doc).map(|d| { + d.edits = Some(Vec::new()); + d.wb.clone() + }); let queued = s.ui_requests.len(); let r = s.execute(id, resolved); + let edits = s.docs.iter_mut().find(|d| d.uid == self.doc).and_then(|d| d.edits.take()).unwrap_or_default(); // Macros never open dialogs or web pages, and never start editing a cell; messages for // the person stay queued. let mut blocked = None; @@ -689,8 +722,11 @@ impl Machine { if let Some(b) = blocked { return Err(fail(b)); } + let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; + if let Some(prev) = &prev { + self.follow(prev, &after, &edits).map_err(fail)?; + } if let (Some(g), Some(prev)) = (self.guard.as_mut(), prev) { - let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { let msg = format!("It would write over data ({what})."); @@ -701,6 +737,34 @@ impl Machine { } } +/// Where the cells of `p` are after a step: on the same sheet wherever it moved (`pairs`, from +/// [`pair_sheets`]), and moved by each structural edit the step made there, or `None` when the +/// step deleted them (or their sheet). +fn follow_place(before: &Workbook, after: &Workbook, pairs: &[(usize, Option)], edits: &[(String, Edit)], p: Place) -> Option { + let sheet = pairs.iter().find(|(b, _)| *b == p.sheet).and_then(|(_, a)| *a)?; + let mut name = after.sheet(sheet)?.name.clone(); + let old_name = before.sheet(p.sheet).map(|s| s.name.as_str()).unwrap_or_default(); + let mut text = format!("{}!{}", resolve::quote_sheet(&name), p.range.a1()); + for (target, e) in edits { + if target != &name && target != old_name { + continue; + } + let expr = gridcraft_formula::parse(&text).ok()?; + text = gridcraft_formula::print(&gridcraft_formula::adjust::adjust(expr, target, target, e)); + if text.contains("#REF!") { + return None; + } + // A cut and paste can take the cells to another sheet. + let env = Env { wb: after, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + name = after.sheet(resolve::resolve_ref(&text, &env).ok()?.sheet)?.name.clone(); + } + let env = Env { wb: after, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + if text.contains('$') { + text = text.replace('$', ""); + } + resolve::resolve_ref(&text, &env).ok() +} + /// If's comparison of a cell's value with a step's `value`, the way Excel's `=`, `<`… compare. fn compare(v: &Value, op: CompareOp, want: &Json) -> bool { let empty = v.is_empty() || v.as_text() == Some(""); diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 262f50af9..68d0b428a 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -3,6 +3,7 @@ use std::sync::Arc; +use gridcraft_core::CellRef; use serde_json::{Value as Json, json}; use super::{s, v}; @@ -176,6 +177,58 @@ fn conflicts_ask_fail_or_overwrite() { assert_eq!(v(&s, "H1").display(), "400"); } +/// The value of `cell` on the sheet called `sheet`. +fn on(s: &Session, sheet: &str, cell: &str) -> String { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap().value(CellRef::parse(cell).unwrap()).display() +} + +#[test] +fn inputs_and_the_output_follow_their_cells_when_a_step_moves_them() { + // A sheet inserted in front: the input is still on Sheet1, not on whatever sheet is first now. + let mut s = s(); + let steps = json!([ + {"command": "home.insertSheet", "params": {"name": "Summary", "before": 0}}, + {"command": "cell.set", "params": {"cell": "Input", "input": "x"}} + ]); + put(&mut s, json!({"name": "Front", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "overwrite"}, "steps": steps})); + s.execute("macro.run", json!({"name": "Front", "inputs": "Sheet1!B2"})).unwrap(); + assert_eq!((on(&s, "Sheet1", "B2"), on(&s, "Summary", "B2")), ("x".into(), "".into())); + + // Rows inserted above: the input and the output move down with their cells. + let mut s = super::s(); + set(&mut s, "B5", "in"); + let steps = json!([ + {"command": "home.insertRows", "params": {"rows": "2:3"}}, + {"command": "cell.set", "params": {"cell": "Input", "input": "x"}}, + {"command": "cell.set", "params": {"cell": "Output", "input": "y"}} + ]); + put( + &mut s, + json!({"name": "Down", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"anchor": "D5", "onConflict": "overwrite"}, "steps": steps}), + ); + s.execute("macro.run", json!({"name": "Down", "inputs": "B5"})).unwrap(); + assert_eq!((v(&s, "B7").display(), v(&s, "D7").display()), ("x".into(), "y".into())); + assert_eq!((v(&s, "B5").display(), v(&s, "D5").display()), ("".into(), "".into())); + + // A step that deletes an input's cells stops the run. + let mut s = super::s(); + set(&mut s, "B5", "in"); + let steps = json!([{"command": "home.deleteRows", "params": {"rows": "5"}}, {"command": "cell.set", "params": {"cell": "Input", "input": "x"}}]); + put(&mut s, json!({"name": "Gone", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "overwrite"}, "steps": steps})); + let start = s.doc().unwrap().wb.clone(); + let err = s.execute("macro.run", json!({"name": "Gone", "inputs": "B5"})).unwrap_err().to_string(); + assert!(err.contains("Step 1 (home.deleteRows): It deleted the cells of Input."), "{err}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); + + // A step inside a Repeat that changes the size of the range it goes over stops the run. + let mut s = super::s(); + let body = json!([{"command": "home.insertRows", "params": {"rows": "2"}}]); + put(&mut s, json!({"name": "Grow", "output": {"onConflict": "overwrite"}, "steps": [{"type": "repeat", "over": "A1:A3", "body": body}]})); + let err = s.execute("macro.run", json!({"name": "Grow"})).unwrap_err().to_string(); + assert!(err.contains("Step 1.1 (home.insertRows): It changed the size of the range Repeat 1 goes over."), "{err}"); +} + #[test] fn each_step_is_checked_for_writes_over_data() { let dir = std::env::temp_dir().join(format!("gc-macro-guard-{}", std::process::id())); diff --git a/docs/macros.md b/docs/macros.md index daf0a2c35..184cc16ad 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -117,7 +117,10 @@ refused rather than read as the header. runs, which this version doesn't work out, so those rules stop the run as `fail` does. 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside - an If). + an If). When a step inserts, deletes or moves sheets, rows, columns or cells, the inputs, the + output and the range of a Repeat in progress move with their cells, as references in formulas + do; a step that deletes one of them, or changes the size of a range a Repeat is going over, + stops the run. ## Storage