From 8846dadc529497d17e1e5c2a53b554b65eed27d1 Mon Sep 17 00:00:00 2001 From: Harrison O'Connor-Hoover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Sat, 10 Oct 2026 06:11:29 -0400 Subject: [PATCH] Fix #128: preserve references when cutting cells across sheets --- ROADMAP.md | 3 +- crates/engine/src/cmd/edit.rs | 30 +++-- crates/engine/tests/cross_sheet_cut.rs | 146 +++++++++++++++++++++++++ crates/formula/src/adjust.rs | 22 ++++ crates/formula/src/tests.rs | 23 ++++ 5 files changed, 216 insertions(+), 8 deletions(-) create mode 100644 crates/engine/tests/cross_sheet_cut.rs diff --git a/ROADMAP.md b/ROADMAP.md index 1b0cd34..ea6c342 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -81,7 +81,8 @@ by open equivalents or left out, not cloned. autocomplete and argument hints, copy/cut/paste and Paste Special (values, formats, transpose, operations, link, skip blanks), AutoFill series (numbers, dates, months, weekdays, custom lists, text+number), Fill Series, Flash Fill, find/replace with wildcards, Go To / Go To Special, - undo/redo with history, format painter. + undo/redo with history, format painter. Cross-sheet cuts retarget dependent cell formulas + and qualified defined names while preserving references inside the moved formulas. - Formatting: fonts, fills, all border styles, alignment (wrap, indent, rotation, merge), number formats, 47 cell styles, 60 table styles, conditional formatting (cell rules, text, dates, duplicates, top/bottom, averages, data bars, colour scales, icon sets, formulas), automatic diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index cffe154..98d8f39 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -814,7 +814,22 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { let f = cell.formula.as_ref()?; let e = f.expr()?; if cut_move { - return Some(std::sync::Arc::new(Formula::from_expr(e))); + // A moved formula still reads cells on its original sheet. Qualify + // those references before the workbook-wide move adjustment below. + let e = if dest_sheet != clip.sheet { + e.map(&mut |x| match x { + gridcraft_formula::Expr::Ref(mut r) if r.sheet == gridcraft_formula::SheetSel::Current => { + r.sheet = gridcraft_formula::SheetSel::Named(src_sheet_name.clone()); + gridcraft_formula::Expr::Ref(r) + } + other => other, + }) + } else { + e + }; + let mut moved = Formula::from_expr(e); + moved.array = f.array; + return Some(std::sync::Arc::new(moved)); } let moved = gridcraft_formula::adjust::shift_relative(e, dest.row as i64 - sc.row as i64, dest.col as i64 - sc.col as i64); @@ -929,11 +944,12 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { if cut_move { // References elsewhere to the moved block now point to its new place. let dest_name = cx.wb.sheet(dest_sheet).map(|s| s.name.clone()).unwrap_or_default(); - if dest_sheet == clip.sheet { - let e = gridcraft_formula::adjust::Edit::Move { from: src, to_row: at.row, to_col: at.col }; - rewrite_all_formulas(&mut cx.wb, &src_sheet_name, &e); - } - let _ = dest_name; + let e = if dest_sheet == clip.sheet { + gridcraft_formula::adjust::Edit::Move { from: src, to_row: at.row, to_col: at.col } + } else { + 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.structural = true; } let end = at.offset_clamped((tiles_r * h) as i64 - 1, (tiles_c * w) as i64 - 1); @@ -946,7 +962,7 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { Ok(json!({"pasted": s.doc()?.selection.a1()})) } -/// Applies a reference adjustment to every formula in the workbook (and names, CF, DV). +/// Applies a reference adjustment to every cell formula and defined name in the workbook. pub(crate) fn rewrite_all_formulas(wb: &mut gridcraft_model::Workbook, target: &str, e: &gridcraft_formula::adjust::Edit) { for si in 0..wb.sheets.len() { let host = wb.sheets.get(si).map(|s| s.name.clone()).unwrap_or_default(); diff --git a/crates/engine/tests/cross_sheet_cut.rs b/crates/engine/tests/cross_sheet_cut.rs new file mode 100644 index 0000000..3f2baaa --- /dev/null +++ b/crates/engine/tests/cross_sheet_cut.rs @@ -0,0 +1,146 @@ +use gridcraft_core::{CellRef, Value}; +use gridcraft_engine::Session; +use serde_json::{Value as Json, json}; + +fn workbook() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s +} + +fn run(s: &mut Session, command: &str, params: Json) { + s.execute(command, params).unwrap(); +} + +fn set(s: &mut Session, cell: &str, input: &str) { + run(s, "cell.set", json!({"cell": cell, "input": input})); +} + +fn activate(s: &mut Session, sheet: &str) { + run(s, "sheet.activate", json!({"sheet": sheet})); +} + +fn value(s: &Session, sheet: &str, cell: &str) -> Value { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap().value(CellRef::parse(cell).unwrap()) +} + +fn formula(s: &Session, sheet: &str, cell: &str) -> String { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap().cell(CellRef::parse(cell).unwrap()).unwrap().formula.as_ref().unwrap().text.clone() +} + +fn cut_to(s: &mut Session, source: &str, range: &str, destination: &str, at: &str) { + activate(s, source); + run(s, "edit.cut", json!({"range": range})); + activate(s, destination); + run(s, "edit.paste", json!({"at": at})); +} + +#[test] +fn cross_sheet_cut_preserves_total_through_undo_redo_and_xlsx() { + let mut s = workbook(); + run(&mut s, "range.setValues", json!({"range": "A1", "values": [[10], [20]]})); + set(&mut s, "B1", "=SUM(A1:A2)"); + run(&mut s, "home.insertSheet", json!({"name": "Sheet2"})); + cut_to(&mut s, "Sheet1", "A1:A2", "Sheet2", "C3"); + + let assert_moved = |s: &Session| { + assert_eq!(value(s, "Sheet1", "A1"), Value::Empty); + assert_eq!(value(s, "Sheet1", "A2"), Value::Empty); + assert_eq!(value(s, "Sheet2", "C3"), Value::Number(10.0)); + assert_eq!(value(s, "Sheet2", "C4"), Value::Number(20.0)); + assert_eq!(value(s, "Sheet1", "B1"), Value::Number(30.0)); + assert_eq!(formula(s, "Sheet1", "B1"), "SUM(Sheet2!C3:C4)"); + }; + assert_moved(&s); + run(&mut s, "edit.undo", json!({})); + assert_eq!(value(&s, "Sheet1", "A1"), Value::Number(10.0)); + assert_eq!(value(&s, "Sheet1", "A2"), Value::Number(20.0)); + assert_eq!(value(&s, "Sheet2", "C3"), Value::Empty); + assert_eq!(value(&s, "Sheet2", "C4"), Value::Empty); + assert_eq!(value(&s, "Sheet1", "B1"), Value::Number(30.0)); + assert_eq!(formula(&s, "Sheet1", "B1"), "SUM(A1:A2)"); + run(&mut s, "edit.redo", json!({})); + assert_moved(&s); + + let saved = s.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap(); + run(&mut s, "file.open", json!({"name": "moved.xlsx", "base64": saved["base64"]})); + assert_moved(&s); +} + +#[test] +fn references_on_all_sheets_follow_cut_but_destination_local_refs_do_not() { + let mut s = workbook(); + run(&mut s, "sheet.rename", json!({"name": "Source Data"})); + run(&mut s, "range.setValues", json!({"range": "A1", "values": [[10], [20]]})); + set(&mut s, "B1", "=SUM($A$1:$A$2)"); + run(&mut s, "formulas.defineName", json!({"name": "MovedData", "refersTo": "='Source Data'!$A$1:$A$2"})); + run(&mut s, "home.insertSheet", json!({"name": "Target's Data"})); + run(&mut s, "range.setValues", json!({"range": "A1", "values": [[100], [200]]})); + set(&mut s, "B1", "=SUM('Source Data'!$A$1:$A$2)"); + set(&mut s, "B2", "=SUM(A1:A2)"); + run(&mut s, "home.insertSheet", json!({"name": "Summary"})); + set(&mut s, "A1", "='Source Data'!$A$1+'Source Data'!A$2"); + set(&mut s, "A2", "=SUM(MovedData)"); + cut_to(&mut s, "Source Data", "A1:A2", "Target's Data", "C3"); + + for (sheet, cell) in [("Source Data", "B1"), ("Target's Data", "B1"), ("Summary", "A1"), ("Summary", "A2")] { + assert_eq!(value(&s, sheet, cell), Value::Number(30.0), "{sheet}!{cell}"); + } + assert_eq!(formula(&s, "Source Data", "B1"), "SUM('Target''s Data'!$C$3:$C$4)"); + assert_eq!(formula(&s, "Summary", "A1"), "'Target''s Data'!$C$3+'Target''s Data'!C$4"); + assert_eq!(value(&s, "Target's Data", "B2"), Value::Number(300.0)); + assert_eq!(formula(&s, "Target's Data", "B2"), "SUM(A1:A2)"); + + // Recalculation must keep tracking the destination after the move, not just retain a cache. + activate(&mut s, "Target's Data"); + set(&mut s, "C3", "40"); + for (sheet, cell) in [("Source Data", "B1"), ("Target's Data", "B1"), ("Summary", "A1"), ("Summary", "A2")] { + assert_eq!(value(&s, sheet, cell), Value::Number(60.0), "{sheet}!{cell}"); + } +} + +#[test] +fn moved_formulas_keep_external_source_refs_and_move_internal_refs_once() { + let mut s = workbook(); + run(&mut s, "sheet.rename", json!({"name": "Source Data"})); + set(&mut s, "A1", "10"); + set(&mut s, "D1", "7"); + set(&mut s, "A2", "=$A$1+$D$1+'Source Data'!A1"); + set(&mut s, "A3", "=A2*2"); + run(&mut s, "home.insertSheet", json!({"name": "Destination"})); + set(&mut s, "D1", "1000"); + // Overlapping coordinates on different sheets catch accidental double adjustment. + cut_to(&mut s, "Source Data", "A1:A3", "Destination", "A2"); + + assert_eq!(value(&s, "Destination", "A2"), Value::Number(10.0)); + assert_eq!(value(&s, "Destination", "A3"), Value::Number(27.0)); + assert_eq!(value(&s, "Destination", "A4"), Value::Number(54.0)); + assert_eq!(value(&s, "Source Data", "D1"), Value::Number(7.0)); + assert!(formula(&s, "Destination", "A3").contains("'Source Data'!$D$1")); + for cell in ["A1", "A2", "A3"] { + assert_eq!(value(&s, "Source Data", cell), Value::Empty); + } + activate(&mut s, "Source Data"); + set(&mut s, "D1", "8"); + assert_eq!(value(&s, "Destination", "A3"), Value::Number(28.0)); + assert_eq!(value(&s, "Destination", "A4"), Value::Number(56.0)); +} + +#[test] +fn same_sheet_overlapping_cut_still_updates_references_once() { + let mut s = workbook(); + run(&mut s, "range.setValues", json!({"range": "A1", "values": [[10], [20]]})); + set(&mut s, "B1", "=SUM(A1:A2)"); + set(&mut s, "A3", "=A1+A2"); + run(&mut s, "edit.cut", json!({"range": "A1:A3"})); + run(&mut s, "edit.paste", json!({"at": "A2"})); + assert_eq!(value(&s, "Sheet1", "A1"), Value::Empty); + assert_eq!(value(&s, "Sheet1", "A2"), Value::Number(10.0)); + assert_eq!(value(&s, "Sheet1", "A3"), Value::Number(20.0)); + assert_eq!(value(&s, "Sheet1", "A4"), Value::Number(30.0)); + assert_eq!(value(&s, "Sheet1", "B1"), Value::Number(30.0)); + assert_eq!(formula(&s, "Sheet1", "A4"), "A2+A3"); + assert_eq!(formula(&s, "Sheet1", "B1"), "SUM(A2:A3)"); +} diff --git a/crates/formula/src/adjust.rs b/crates/formula/src/adjust.rs index 734ff5c..c1e03fc 100644 --- a/crates/formula/src/adjust.rs +++ b/crates/formula/src/adjust.rs @@ -56,6 +56,8 @@ pub enum Edit { Delete { axis: Axis, at: u32, count: u32 }, /// Cut-paste of a block: references into `from` now point into the block at `to` (top-left). Move { from: RangeRef, to_row: u32, to_col: u32 }, + /// Cut-paste to another sheet; fully contained references follow the destination sheet too. + MoveToSheet { from: RangeRef, to_sheet: String, to_row: u32, to_col: u32 }, } /// Adjusts a formula living on sheet `host` after `edit` on sheet `target`. @@ -176,6 +178,26 @@ fn adjust_ref(r: &Reference, edit: &Edit) -> Option { }; Some(Reference { sheet: r.sheet.clone(), kind }) } + Edit::MoveToSheet { from, to_sheet, to_row, to_col } => { + if !from.contains_range(&r.range()) { + return Some(r.clone()); + } + let mut moved = adjust_ref(r, &Edit::Move { from: *from, to_row: *to_row, to_col: *to_col })?; + // Whole-row/column references have no anchors, but their indices must move too. + moved.kind = match &r.kind { + RefKind::Rows(a, aa, b, ba) => { + let dr = *to_row as i64 - from.start.row as i64; + RefKind::Rows(shift_line(*a, false, dr, MAX_ROWS)?, *aa, shift_line(*b, false, dr, MAX_ROWS)?, *ba) + } + RefKind::Cols(a, aa, b, ba) => { + let dc = *to_col as i64 - from.start.col as i64; + RefKind::Cols(shift_line(*a, false, dc, MAX_COLS)?, *aa, shift_line(*b, false, dc, MAX_COLS)?, *ba) + } + _ => moved.kind, + }; + moved.sheet = SheetSel::Named(to_sheet.clone()); + Some(moved) + } } } diff --git a/crates/formula/src/tests.rs b/crates/formula/src/tests.rs index ff84a5b..a283c05 100644 --- a/crates/formula/src/tests.rs +++ b/crates/formula/src/tests.rs @@ -129,6 +129,29 @@ fn moves_and_renames() { assert_eq!(print(&rename_sheet(e, "Old", "New Name")), "'New Name'!A1+'New Name'!B2"); } +#[test] +fn moves_between_sheets() { + let mv = Edit::MoveToSheet { from: gridcraft_core::RangeRef::parse("A1:B2").unwrap(), to_sheet: "New Name".into(), to_row: 9, to_col: 3 }; + for (host, input, expected) in [ + ("Source", "A1+$B$2+C3+SUM(A1:B2)", "'New Name'!D10+'New Name'!$E$11+C3+SUM('New Name'!D10:E11)"), + ("Other", "A1+sOuRcE!$B2+Other!B2", "A1+'New Name'!$E11+Other!B2"), + ("Source", "SUM(A1:C3)+SUM(Source:Other!A1)", "SUM(A1:C3)+SUM(Source:Other!A1)"), + ] { + assert_eq!(print(&adjust(parse(input).unwrap(), host, "Source", &mv)), expected); + } + let edge = Edit::MoveToSheet { + from: gridcraft_core::RangeRef::parse("A1:B2").unwrap(), + to_sheet: "New Name".into(), + to_row: gridcraft_core::MAX_ROWS - 1, + to_col: 3, + }; + assert_eq!(print(&adjust(parse("B2").unwrap(), "Source", "Source", &edge)), "#REF!"); + for (from, input, row, col, expected) in [("A:A", "$A:$A", 0, 2, "Dest!$C:$C"), ("1:1", "$1:$1", 2, 0, "Dest!$3:$3")] { + let mv = Edit::MoveToSheet { from: gridcraft_core::RangeRef::parse(from).unwrap(), to_sheet: "Dest".into(), to_row: row, to_col: col }; + assert_eq!(print(&adjust(parse(input).unwrap(), "Source", "Source", &mv)), expected); + } +} + #[test] fn r1c1() { let e = parse("A1+$B$2+SUM(C:C)").unwrap();