From 9ab297ced1115c2088bb0065588a4500563476f2 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 04:06:21 +0100 Subject: [PATCH] perf(engine): rewrite only the formulas a structural edit changes rewrite_all_formulas borrows each tree with Formula::with_expr and calls adjust_if_changed, so a formula the edit leaves alone is neither copied nor printed, and a changed one is printed once. A legacy array whose range moves while its formula stays the same still gets the new range. Sheet rename and delete compare the adjusted tree with the old one instead of printed text, and replace only formulas that changed. Delete no longer rebuilds every formula in the workbook, so unrelated formulas keep their array range and dynamic-array mark. A stored non-canonical formula text is no longer re-printed by an edit that does not change the formula; a test pins this. Part of #3576 Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/edit/paste_special.rs | 18 ++-- crates/engine/src/cmd/sheet/tabs.rs | 25 +++--- crates/engine/tests/it/main.rs | 1 + .../tests/it/structural_edit_rewrite.rs | 84 +++++++++++++++++++ 4 files changed, 106 insertions(+), 22 deletions(-) create mode 100644 crates/engine/tests/it/structural_edit_rewrite.rs diff --git a/crates/engine/src/cmd/edit/paste_special.rs b/crates/engine/src/cmd/edit/paste_special.rs index df2b0de61..c90dd9ec8 100644 --- a/crates/engine/src/cmd/edit/paste_special.rs +++ b/crates/engine/src/cmd/edit/paste_special.rs @@ -567,19 +567,19 @@ pub(crate) fn rewrite_all_formulas(wb: &mut gridcraft_model::Workbook, target: & let on_target = moves_arrays && host.eq_ignore_ascii_case(target); let Some(sheet) = wb.sheet_mut(si) else { continue }; for (c, f) in keys { - let Some(expr) = f.expr() else { continue }; - let new = gridcraft_formula::adjust::adjust(expr, &host, target, e); - let text = gridcraft_formula::print(&new); + // Borrows the tree and copies and prints only the formulas the edit changes. A formula + // the edit leaves alone keeps its stored text, even if that text is not canonical. + let Some(new) = f.with_expr(|x| gridcraft_formula::adjust::adjust_if_changed(x, &host, target, e)) else { continue }; let array = match f.array { Some(a) if on_target => crate::cmd::sheet::map_range(a, e), a => a, }; - if (text != f.text || array != f.array) - && let Some(cell) = sheet.cells.get_mut(c) - { - let mut nf = Formula::from_expr(new); - nf.array = array; - nf.dynamic = f.dynamic; + let nf = match new { + Some(new) => Formula { array, dynamic: f.dynamic, ..Formula::from_expr(new) }, + None if array != f.array => Formula { array, ..(*f).clone() }, + None => continue, + }; + if let Some(cell) = sheet.cells.get_mut(c) { cell.formula = Some(std::sync::Arc::new(nf)); } } diff --git a/crates/engine/src/cmd/sheet/tabs.rs b/crates/engine/src/cmd/sheet/tabs.rs index d21851553..2c1657ef9 100644 --- a/crates/engine/src/cmd/sheet/tabs.rs +++ b/crates/engine/src/cmd/sheet/tabs.rs @@ -55,11 +55,12 @@ pub(super) fn delete_sheet(s: &mut Session, p: &Json) -> Result { cx.wb.sheets.get(si).map(|s| s.cells.iter().filter_map(|(c, x)| x.formula.clone().map(|f| (c, f))).collect()).unwrap_or_default(); let Some(sh) = cx.wb.sheet_mut(si) else { continue }; for (c, f) in keys { - if let Some(e) = f.expr() { - let ne = gridcraft_formula::adjust::delete_sheet(e, &name, &order); - if let Some(cell) = sh.cells.get_mut(c) { - cell.formula = Some(Arc::new(gridcraft_model::Formula::from_expr(ne))); - } + // Only formulas whose tree changes are rewritten; the others keep their text. + let changed = f.with_expr(|e| Some(gridcraft_formula::adjust::delete_sheet(e.clone(), &name, &order)).filter(|ne| ne != e)); + if let Some(Some(ne)) = changed + && let Some(cell) = sh.cells.get_mut(c) + { + cell.formula = Some(Arc::new(gridcraft_model::Formula::from_expr(ne))); } } } @@ -109,14 +110,12 @@ pub(super) fn rename_sheet(s: &mut Session, p: &Json) -> Result { cx.wb.sheets.get(si).map(|s| s.cells.iter().filter_map(|(c, x)| x.formula.clone().map(|f| (c, f))).collect()).unwrap_or_default(); let Some(sh) = cx.wb.sheet_mut(si) else { continue }; for (c, f) in keys { - if let Some(e) = f.expr() { - let ne = gridcraft_formula::adjust::rename_sheet(e, &old, &name); - let text = gridcraft_formula::print(&ne); - if text != f.text - && let Some(cell) = sh.cells.get_mut(c) - { - cell.formula = Some(Arc::new(gridcraft_model::Formula::from_expr(ne))); - } + // Compares trees, so unchanged formulas are not printed; they keep their text. + let changed = f.with_expr(|e| Some(gridcraft_formula::adjust::rename_sheet(e.clone(), &old, &name)).filter(|ne| ne != e)); + if let Some(Some(ne)) = changed + && let Some(cell) = sh.cells.get_mut(c) + { + cell.formula = Some(Arc::new(gridcraft_model::Formula::from_expr(ne))); } } for ch in sh.charts.iter_mut() { diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index 0ba7c0db8..ca960d759 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -29,6 +29,7 @@ mod sheet_copy_spill; mod sheet_name_quoting; mod sparse_clear_invalidation; mod stored_list_shape; +mod structural_edit_rewrite; mod table_resize_overlap; mod table_total_row_last_row; mod trace_error; diff --git a/crates/engine/tests/it/structural_edit_rewrite.rs b/crates/engine/tests/it/structural_edit_rewrite.rs new file mode 100644 index 000000000..bb87ba3c4 --- /dev/null +++ b/crates/engine/tests/it/structural_edit_rewrite.rs @@ -0,0 +1,84 @@ +//! Structural edits (insert and delete of lines, sheet rename and delete) rewrite only the formulas +//! whose tree they change. A formula they leave alone keeps its stored text and its flags. + +use std::sync::Arc; + +use gridcraft_core::{CellRef, RangeRef}; +use gridcraft_engine::Session; +use gridcraft_model::{Cell, Formula}; +use serde_json::{Value as Json, json}; + +fn run(s: &mut Session, command: &str, params: Json) { + s.execute(command, params).unwrap(); +} + +fn workbook() -> Session { + let mut s = Session::new(); + s.new_workbook(); + run(&mut s, "home.insertSheet", json!({"name": "Other"})); + run(&mut s, "home.insertSheet", json!({"name": "Spare"})); + run(&mut s, "sheet.activate", json!({"sheet": "Sheet1"})); + s +} + +fn formula(s: &Session, sheet: &str, cell: &str) -> Arc { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap().cell(CellRef::parse(cell).unwrap()).unwrap().formula.clone().unwrap() +} + +/// Stores a formula with this exact text and no cached tree, as a JSON workbook loads it. +fn put_raw(s: &mut Session, cell: &str, text: &str) { + let wb = Arc::make_mut(&mut s.doc_mut().unwrap().wb); + let i = wb.sheet_index("Sheet1").unwrap(); + let f = Formula { text: text.to_string(), expr: None, array: None, dynamic: false }; + wb.sheet_mut(i).unwrap().cells.set(CellRef::parse(cell).unwrap(), Cell::formula(f)); +} + +#[test] +fn an_edit_that_leaves_a_formula_alone_keeps_its_stored_text() { + // The rewrite compares trees, not printed text, so a stored non-canonical text is not + // re-printed by an edit that does not change the formula. An edit that changes it prints it. + let mut s = workbook(); + put_raw(&mut s, "C1", "sum(a1, Other!b2)"); + run(&mut s, "home.insertRows", json!({"rows": "5:5"})); + run(&mut s, "home.insertColumns", json!({"cols": "F:F"})); + run(&mut s, "sheet.rename", json!({"sheet": "Spare", "name": "Spare2"})); + run(&mut s, "home.deleteSheet", json!({"sheet": "Spare2"})); + assert_eq!(formula(&s, "Sheet1", "C1").text, "sum(a1, Other!b2)"); + + run(&mut s, "home.insertRows", json!({"rows": "1:1"})); + assert_eq!(formula(&s, "Sheet1", "C2").text, "SUM(A2,Other!B2)"); +} + +#[test] +fn renaming_a_referenced_sheet_rewrites_the_formula() { + let mut s = workbook(); + put_raw(&mut s, "C1", "sum(a1, Other!b2)"); + run(&mut s, "sheet.rename", json!({"sheet": "Other", "name": "Data"})); + assert_eq!(formula(&s, "Sheet1", "C1").text, "SUM(A1,Data!B2)"); + run(&mut s, "home.deleteSheet", json!({"sheet": "Data"})); + assert_eq!(formula(&s, "Sheet1", "C1").text, "SUM(A1,#REF!)"); +} + +#[test] +fn inserting_rows_above_a_legacy_array_moves_its_range_when_the_formula_is_unchanged() { + let mut s = workbook(); + run(&mut s, "range.setValues", json!({"range": "B1", "values": [[1], [2]]})); + run(&mut s, "selection.set", json!({"range": "C5:C6"})); + run(&mut s, "cell.set", json!({"cell": "C5", "input": "=B1:B2*2", "array": true})); + assert_eq!(formula(&s, "Sheet1", "C5").array, Some(RangeRef::parse("C5:C6").unwrap())); + run(&mut s, "home.insertRows", json!({"rows": "3:3"})); + let f = formula(&s, "Sheet1", "C6"); + assert_eq!(f.text, "B1:B2*2"); + assert_eq!(f.array, Some(RangeRef::parse("C6:C7").unwrap())); +} + +#[test] +fn deleting_an_unrelated_sheet_keeps_legacy_arrays() { + let mut s = workbook(); + run(&mut s, "range.setValues", json!({"range": "B1", "values": [[1], [2]]})); + run(&mut s, "selection.set", json!({"range": "C1:C2"})); + run(&mut s, "cell.set", json!({"cell": "C1", "input": "=B1:B2*2", "array": true})); + run(&mut s, "home.deleteSheet", json!({"sheet": "Spare"})); + assert_eq!(formula(&s, "Sheet1", "C1").array, Some(RangeRef::parse("C1:C2").unwrap())); +}