From 6874e0b6892cd622a2a3c525198291435b958b4f Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Tue, 3 Nov 2026 00:13:27 -0600 Subject: [PATCH] #3136: range.move pastes on the range's sheet and asks before replacing spilled values Addresses CodeRabbit review on #3216: - "Pass the resolved sheet to the paste step": a sheet-qualified move (`Two!B1:B2` with Sheet1 active) was checked on Two but pasted on the active sheet, replacing its data unasked. range.move now calls paste_special with the range's sheet. - "Check spilled values in the drop destination": cells showing a dynamic array's spilled values hold no cell of their own, so the drop replaced them without the prompt. They now count as data, as in Excel (ISBLANK of a spill cell is FALSE, COUNTA counts it). Tests: drag_and_drop_on_another_sheet_stays_on_that_sheet, drag_and_drop_asks_before_replacing_spilled_values. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/edit/clipboard.rs | 10 ++++--- crates/engine/src/cmd/edit/tests.rs | 36 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/edit/clipboard.rs b/crates/engine/src/cmd/edit/clipboard.rs index d416d7b12..78057691f 100644 --- a/crates/engine/src/cmd/edit/clipboard.rs +++ b/crates/engine/src/cmd/edit/clipboard.rs @@ -165,8 +165,8 @@ pub(super) fn paste(s: &mut Session, p: &Json) -> Result { } /// Drag and drop (dragging the selection's border): moves `range` so its top-left cell lands on -/// `to`, as Cut and Paste would. Like Excel, it asks before replacing cells that hold data (other -/// than the moved cells themselves); run it again with `overwrite: true` to replace them. A +/// `to`, as Cut and Paste would. Like Excel, it asks before replacing cells that hold data or show a +/// spilled value (other than the moved cells themselves); run it again with `overwrite: true` to replace them. A /// refused or failed move leaves the clipboard as it was. pub(super) fn range_move(s: &mut Session, p: &Json) -> Result { let sheet = target_sheet(s, p)?; @@ -178,11 +178,13 @@ pub(super) fn range_move(s: &mut Session, p: &Json) -> Result { if bool_param(p, "overwrite") != Some(true) && let Some(dest) = dest && let Some(sh) = s.doc()?.wb.sheet(sheet) - && sh.cells.iter_range(dest).any(|(c, cell)| !src.contains(c) && cell.has_content()) + && (sh.cells.iter_range(dest).any(|(c, cell)| !src.contains(c) && cell.has_content()) + || sh.spill.range(dest.start..=dest.end).any(|(c, v)| dest.contains(*c) && !src.contains(*c) && *v != Value::Empty)) { return Err(EngineError::Confirm("There's already data here. Do you want to replace it?".into())); } - paste(s, &json!({"at": to.a1()})) + // On the range's sheet, which `paste` would leave to the active one. + paste_special(s, &json!({"what": "all", "at": to.a1(), "sheet": sheet})) }); if moved.is_err() { s.clipboard = kept; diff --git a/crates/engine/src/cmd/edit/tests.rs b/crates/engine/src/cmd/edit/tests.rs index 232a9955a..e5b223a91 100644 --- a/crates/engine/src/cmd/edit/tests.rs +++ b/crates/engine/src/cmd/edit/tests.rs @@ -381,6 +381,42 @@ fn drag_and_drop_asks_before_replacing_data() { assert!(s.execute("range.move", json!({"range": "C2"})).is_err(), "`to` is required"); } +#[test] +fn drag_and_drop_asks_before_replacing_spilled_values() { + // Cells showing a dynamic array's spilled values hold no cell of their own, but they are data. + let mut s = Session::new(); + s.new_workbook(); + s.execute("cell.set", json!({"cell": "A1", "input": "=SEQUENCE(3)"})).unwrap(); + s.execute("range.setValues", json!({"range": "C2:C3", "values": [["Kiwi"], ["Plum"]]})).unwrap(); + let r = s.execute("range.move", json!({"range": "C2:C3", "to": "A2"})); + let Err(EngineError::Confirm(q)) = r else { panic!("{r:?}") }; + assert_eq!(q, "There's already data here. Do you want to replace it?"); + let sh = s.doc().unwrap().wb.active().unwrap(); + assert_eq!(sh.value(CellRef::new(2, 2)), Value::text("Plum")); + s.execute("range.move", json!({"range": "C2:C3", "to": "A2", "overwrite": true})).unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + assert_eq!(sh.value(CellRef::new(1, 0)), Value::text("Kiwi")); +} + +#[test] +fn drag_and_drop_on_another_sheet_stays_on_that_sheet() { + // The move was checked on the range's sheet but pasted on the active one, replacing its data + // without asking. + let mut s = Session::new(); + s.new_workbook(); + s.execute("home.insertSheet", json!({"name": "Two"})).unwrap(); + s.execute("range.setValues", json!({"range": "B1:B2", "values": [["Kiwi"], ["Plum"]]})).unwrap(); + s.execute("sheet.activate", json!({"sheet": 0})).unwrap(); + s.execute("cell.set", json!({"cell": "A1", "input": "Keep"})).unwrap(); + s.execute("range.move", json!({"range": "Two!B1:B2", "to": "A1"})).unwrap(); + let wb = &s.doc().unwrap().wb; + let two = wb.sheet_index("Two").unwrap(); + assert_eq!(wb.sheet(0).unwrap().value(CellRef::new(0, 0)), Value::text("Keep")); + assert_eq!(wb.sheet(two).unwrap().value(CellRef::new(0, 0)), Value::text("Kiwi")); + assert_eq!(wb.sheet(two).unwrap().value(CellRef::new(1, 0)), Value::text("Plum")); + assert_eq!(wb.sheet(two).unwrap().value(CellRef::new(0, 1)), Value::Empty); +} + #[test] fn replace_case_insensitive() { assert_eq!(replace_ci("Apple apple APPLE", "apple", "pear"), "pear pear pear");