From d665499055deb924903e625748f07e9b4042fbd6 Mon Sep 17 00:00:00 2001 From: lloyd Date: Sun, 11 Oct 2026 17:02:39 +0200 Subject: [PATCH] Clipboard: copy lined-up multiple selections; say why others can't be copied (#348) Copying a multi-area selection always failed in the engine, and the UI dropped the error, so Ctrl+C did nothing and Ctrl+V had nothing to paste. Now, as in Excel: - Areas that cover the same rows (or the same columns) and don't overlap are copied as one block, slid together left to right (top to bottom). The clipboard keeps a snapshot with the block packed at the first area's place; formulas travel as their values, and column widths or row heights, comments, pictures and merges come along. - Other layouts, Cut of several areas and Paste Link of a multi-area copy are refused with "This action won't work on multiple selections."; Copy and Cut from the keyboard, ribbon and context menus now show that message instead of failing silently. - The marching ants go around each copied area. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/edit.rs | 111 +++++++++++++++++++++++++++--- crates/engine/src/lib.rs | 3 + crates/engine/src/tests.rs | 61 ++++++++++++++++ crates/ui-egui/src/grid.rs | 12 ++-- crates/ui-egui/src/lib.rs | 20 ++++-- crates/ui-egui/tests/clipboard.rs | 19 +++++ 6 files changed, 205 insertions(+), 21 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index bb54fe0..d1d6837 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -604,25 +604,34 @@ fn redo(s: &mut Session, p: &Json) -> Result { // ---------------------------------------------------------------- clipboard fn copy_impl(s: &mut Session, p: &Json, cut: bool) -> Result { + let multiple = || EngineError::Other("This action won't work on multiple selections.".into()); let sheet = target_sheet(s, p)?; let ranges = target_ranges(s, p)?; - if ranges.len() > 1 { - return Err(EngineError::Other("This action won't work on multiple selections.".into())); + if ranges.len() > 1 && cut { + return Err(multiple()); } - let range = ranges.first().copied().unwrap_or_default(); let d = s.doc()?; let Some(sh) = d.wb.sheet(sheet) else { return Err(EngineError::NoDocument) }; - let range = if range.is_full_cols() || range.is_full_rows() { - let used = sh.used_range().unwrap_or(RangeRef::cell(range.start)); - range.intersection(&used.union(&RangeRef::cell(range.start))).unwrap_or(range) - } else { - range + let trim = |range: RangeRef| { + if range.is_full_cols() || range.is_full_rows() { + let used = sh.used_range().unwrap_or(RangeRef::cell(range.start)); + range.intersection(&used.union(&RangeRef::cell(range.start))).unwrap_or(range) + } else { + range + } }; - let text = crate::display::range_text(&d.wb, sheet, range); + let (wb, range, areas) = if ranges.len() > 1 { + let areas: Vec = ranges.iter().map(|r| trim(*r)).collect(); + let (wb, block) = slide_together(&d.wb, sheet, &areas).ok_or_else(multiple)?; + (std::sync::Arc::new(wb), block, areas) + } else { + (d.wb.clone(), trim(ranges.first().copied().unwrap_or_default()), Vec::new()) + }; + let text = crate::display::range_text(&wb, sheet, range); // The HTML rendering is for the host clipboard (UI); it can reach 4 MB, so agents and // scripts get it only when they ask for it. - let html = if p.get("html").and_then(Json::as_bool) == Some(true) { crate::io::range_html(&d.wb, sheet, range, &text) } else { None }; - let clip = Clipboard { wb: d.wb.clone(), sheet, range, cut, doc_uid: d.uid, text: text.clone() }; + let html = if p.get("html").and_then(Json::as_bool) == Some(true) { crate::io::range_html(&wb, sheet, range, &text) } else { None }; + let clip = Clipboard { wb, sheet, range, cut, doc_uid: d.uid, text: text.clone(), areas }; s.clipboard = Some(clip); let mut result = json!({"range": range.a1(), "text": text}); if let Some(html) = html { @@ -631,6 +640,83 @@ fn copy_impl(s: &mut Session, p: &Json, cut: bool) -> Result { Ok(result) } +/// Excel copies a multi-area selection only when the areas line up: all cover the same rows (they +/// slide together side by side, left to right) or all cover the same columns (stacked, top to +/// bottom), without overlapping. Returns a copy of the workbook whose sheet holds the areas packed +/// into one block at the first area's place, and that block. Formulas travel as their values, as +/// in Excel. `None` when the areas don't line up. +fn slide_together(wb: &gridcraft_model::Workbook, sheet: usize, areas: &[RangeRef]) -> Option<(gridcraft_model::Workbook, RangeRef)> { + let first = *areas.first()?; + let across = areas.iter().all(|a| a.start.row == first.start.row && a.end.row == first.end.row); + let down = areas.iter().all(|a| a.start.col == first.start.col && a.end.col == first.end.col); + let mut sorted = areas.to_vec(); + if across { + sorted.sort_by_key(|a| a.start.col); + } else if down { + sorted.sort_by_key(|a| a.start.row); + } else { + return None; + } + if sorted.windows(2).any(|w| matches!(w, [a, b] if a.intersects(b))) { + return None; + } + let anchor = sorted.first()?.start; + let (h, w) = if across { + (first.height(), sorted.iter().map(RangeRef::width).fold(0u32, u32::saturating_add)) + } else { + (sorted.iter().map(RangeRef::height).fold(0u32, u32::saturating_add), first.width()) + }; + let block = RangeRef::new(anchor, anchor.offset(i64::from(h) - 1, i64::from(w) - 1)?); + let src = wb.sheet(sheet)?; + let mut out = wb.clone(); + let dst = out.sheet_mut(sheet)?; + dst.take_cells(block); + dst.spill.retain(|c, _| !block.contains(*c)); + dst.spill_ranges.retain(|c, _| !block.contains(*c)); + dst.comments.retain(|c, _| !block.contains(*c)); + dst.merges.retain(|m| !m.intersects(&block)); + if across { + dst.cols.retain(|c, _| !(block.start.col..=block.end.col).contains(c)); + } else { + dst.rows.retain(|r, _| !(block.start.row..=block.end.row).contains(r)); + } + let mut next = if across { anchor.col } else { anchor.row }; + for a in &sorted { + let (dr, dc) = if across { (0, i64::from(next) - i64::from(a.start.col)) } else { (i64::from(next) - i64::from(a.start.row), 0) }; + let cells = src.cells.iter_range(*a).map(|(c, _)| c).chain(src.spill.keys().copied().filter(|c| a.contains(*c))); + for c in cells { + let Some(to) = c.offset(dr, dc) else { continue }; + let style = src.cell(c).map(|x| x.style).unwrap_or_default(); + dst.set_cell(to, Cell { value: src.value(c), formula: None, style }); + if let Some(picture) = src.cell_pictures.get(&c) { + dst.cell_pictures.insert(to, picture.clone()); + } + } + for (c, comment) in src.comments.iter().filter(|(c, _)| a.contains(**c)) { + if let Some(to) = c.offset(dr, dc) { + dst.comments.insert(to, comment.clone()); + } + } + for m in src.merges.iter().filter(|m| a.contains_range(m)) { + if let (Some(s), Some(e)) = (m.start.offset(dr, dc), m.end.offset(dr, dc)) { + dst.merges.push(RangeRef::new(s, e)); + } + } + // Column widths (side by side) or row heights and hidden rows (stacked) follow their cells. + if across { + for (c, info) in src.cols.range(a.start.col..=a.end.col) { + dst.cols.insert(next.saturating_add(c.saturating_sub(a.start.col)), *info); + } + } else { + for (r, info) in src.rows.range(a.start.row..=a.end.row) { + dst.rows.insert(next.saturating_add(r.saturating_sub(a.start.row)), *info); + } + } + next = next.saturating_add(if across { a.width() } else { a.height() }); + } + Some((out, block)) +} + fn copy(s: &mut Session, p: &Json) -> Result { copy_impl(s, p, false) } @@ -769,6 +855,9 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { let skip_blanks = bool_param(p, "skipBlanks").unwrap_or(false); let transpose = bool_param(p, "transpose").unwrap_or(false); let link = bool_param(p, "link").unwrap_or(false); + if link && !clip.areas.is_empty() { + return Err(EngineError::Other("This action won't work on multiple selections.".into())); + } let dest_sheet = target_sheet(s, p)?; let sel = s.doc()?.selection.current(); let at = cell_param(p, "at").unwrap_or(sel.start); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index de786a1..267ef83 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -70,6 +70,9 @@ pub struct Clipboard { pub doc_uid: u64, /// Plain text (tab-separated) for the system clipboard. pub text: String, + /// A multi-area copy: the areas as selected (for the marquee). `wb` then holds them slid + /// together into one block at `range`. Empty for a single-range copy. + pub areas: Vec, } /// In-cell edit mode as shown in the status bar. diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6bd03d5..228612a 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1633,3 +1633,64 @@ fn notes_comments_and_links_survive_a_json_save() { assert_eq!(after.hyperlinks, before.hyperlinks); assert_eq!(after.cells, before.cells); } + +#[test] +fn copying_areas_that_share_rows_or_columns_pastes_them_as_one_block() { + // Issue #348: Excel slides lined-up areas together; formulas travel as their values. + let mut s = s(); + s.execute( + "range.setValues", + json!({"range": "A1", "values": [["a1", "b1", "c1", "d1"], [1, 2, 3, 4], ["a3", "b3", "c3", "d3"], ["a4", "b4", "c4", "d4"]]}), + ) + .unwrap(); + s.execute("cell.set", json!({"cell": "C2", "input": "=A2+B2"})).unwrap(); + + // Same rows, selected right to left: pasted left to right, B left out. + s.execute("selection.set", json!({"range": "C1:C2,A1:A2"})).unwrap(); + let copied = s.execute("edit.copy", json!({})).unwrap(); + assert_eq!(copied["text"], "a1\tc1\n1\t3\n"); + s.execute("edit.paste", json!({"at": "F1"})).unwrap(); + assert_eq!(v(&s, "F1"), Value::text("a1")); + assert_eq!(v(&s, "G1"), Value::text("c1")); + assert_eq!(v(&s, "F2"), Value::Number(1.0)); + assert_eq!(v(&s, "G2"), Value::Number(3.0)); + assert!(s.doc().unwrap().wb.active().unwrap().cell(CellRef::parse("G2").unwrap()).unwrap().formula.is_none()); + assert!(v(&s, "H1").is_empty()); + assert_eq!(s.doc().unwrap().selection.current().a1(), "F1:G2"); + // The source is untouched. + assert_eq!(v(&s, "B1"), Value::text("b1")); + assert!(s.doc().unwrap().wb.active().unwrap().cell(CellRef::parse("C2").unwrap()).unwrap().formula.is_some()); + + // Same columns: stacked, row 2 left out. + s.execute("selection.set", json!({"range": "A1:B1,A3:B4"})).unwrap(); + assert_eq!(s.execute("edit.copy", json!({})).unwrap()["text"], "a1\tb1\na3\tb3\na4\tb4\n"); + s.execute("edit.paste", json!({"at": "F5"})).unwrap(); + for (cell, want) in [("F5", "a1"), ("G5", "b1"), ("F6", "a3"), ("G7", "b4")] { + assert_eq!(v(&s, cell), Value::text(want), "{cell}"); + } + assert!(v(&s, "F8").is_empty()); + + // Whole columns are trimmed to the used rows like a single-area copy. + s.execute("edit.undo", json!({"steps": 2})).unwrap(); + s.execute("selection.set", json!({"range": "B:B,D:D"})).unwrap(); + assert_eq!(s.execute("edit.copy", json!({})).unwrap()["text"], "b1\td1\n2\t4\nb3\td3\nb4\td4\n"); +} + +#[test] +fn copying_areas_that_do_not_line_up_is_refused_with_a_message() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [[1, 2, 3], [4, 5, 6], [7, 8, 9]]})).unwrap(); + s.execute("selection.set", json!({"range": "A1"})).unwrap(); + s.execute("edit.copy", json!({})).unwrap(); + for (range, cut) in [("A1:A2,C2:C3", false), ("A1:A2,C1:D3", false), ("A1:A2,A2:A3", false), ("A1:A2,A1:A2", false), ("A1:A2,C1:C2", true)] { + s.execute("selection.set", json!({"range": range})).unwrap(); + let e = s.execute(if cut { "edit.cut" } else { "edit.copy" }, json!({})).unwrap_err().to_string(); + assert!(e.contains("This action won't work on multiple selections."), "{range}: {e}"); + assert_eq!(s.clipboard.as_ref().map(|c| c.range.a1()), Some("A1".into()), "{range}: the earlier copy stays"); + } + // Paste Link can't point at a slid-together block. + s.execute("selection.set", json!({"range": "A1:A2,C1:C2"})).unwrap(); + s.execute("edit.copy", json!({})).unwrap(); + assert!(s.execute("edit.pasteSpecial", json!({"what": "all", "link": true, "at": "E1"})).is_err()); + assert!(v(&s, "E1").is_empty()); +} diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 72d33e2..122f863 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -966,10 +966,14 @@ fn paint_selection(p: &Painter, geo: &Geo, sh: &Sheet, sel: &gridcraft_engine::S if let Some(clip) = &app.session.clipboard && app.session.active().is_some_and(|d| d.uid == clip.doc_uid && d.wb.active_sheet == clip.sheet) { - let rr = geo.range_rect(sh, clip.range).expand(0.5); - let phase = app.grid.marching_phase; - let pts = [rr.left_top(), rr.right_top(), rr.right_bottom(), rr.left_bottom(), rr.left_top()]; - p.extend(egui::Shape::dashed_line_with_offset(&pts, Stroke::new(2.0, t.sel_border), &[4.0], &[4.0], phase)); + // A multi-area copy marks each area as selected. + let areas = if clip.areas.is_empty() { std::slice::from_ref(&clip.range) } else { clip.areas.as_slice() }; + for area in areas { + let rr = geo.range_rect(sh, *area).expand(0.5); + let phase = app.grid.marching_phase; + let pts = [rr.left_top(), rr.right_top(), rr.right_bottom(), rr.left_bottom(), rr.left_top()]; + p.extend(egui::Shape::dashed_line_with_offset(&pts, Stroke::new(2.0, t.sel_border), &[4.0], &[4.0], phase)); + } } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1225eec..650483f 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -250,7 +250,10 @@ impl SheetApp { /// their existing internal clipboard behavior and do not write to the host clipboard. pub fn copy_to_clipboard(&mut self, ctx: &egui::Context, command: &str) { // Only ask the engine for HTML when the host can publish it. - let Ok(result) = self.run(command, json!({"html": self.services.copy_html.is_some()})) else { return }; + let result = match self.run(command, json!({"html": self.services.copy_html.is_some()})) { + Ok(result) => result, + Err(e) => return self.alert(e), + }; let Some(text) = result.get("text").and_then(Json::as_str) else { return }; if let Some(html) = result.get("html").and_then(Json::as_str) && let Some(copy_html) = &mut self.services.copy_html @@ -279,11 +282,16 @@ impl SheetApp { /// Runs a command and shows its error in a message box (for menu/ribbon clicks). pub fn run_or_alert(&mut self, id: &str, params: Json) { if let Err(e) = self.run(id, params) { - if e.contains("is not available right now") { - self.toast = Some((e, now_ms())); - } else { - self.message = Some(("GridCraft".into(), clean_error(&e))); - } + self.alert(e); + } + } + + /// Shows a command error: a toast for an unavailable command, else a message box. + fn alert(&mut self, e: String) { + if e.contains("is not available right now") { + self.toast = Some((e, now_ms())); + } else { + self.message = Some(("GridCraft".into(), clean_error(&e))); } } diff --git a/crates/ui-egui/tests/clipboard.rs b/crates/ui-egui/tests/clipboard.rs index 188dbdc..3922708 100644 --- a/crates/ui-egui/tests/clipboard.rs +++ b/crates/ui-egui/tests/clipboard.rs @@ -83,3 +83,22 @@ fn programmatic_copy_does_not_write_to_the_system_clipboard() { assert_eq!(*calls.borrow(), 0); assert!(!h.output().platform_output.commands.iter().any(|c| matches!(c, OutputCommand::CopyText(_)))); } + +#[test] +fn copying_multiple_areas_copies_lined_up_areas_and_explains_the_rest() { + // Issue #348: lined-up areas copy as one block; others get Excel's message instead of nothing. + let mut h = harness(Services::default()); + h.state_mut().session.execute("selection.set", json!({"range": "B1:B2,A1:A2"})).unwrap(); + h.input_mut().events.push(Event::Copy); + h.step(); + assert!(h.output().platform_output.commands.iter().any(|c| matches!(c, OutputCommand::CopyText(text) if text == "Name\tScore\nAda\t42\n"))); + assert!(h.state().message.is_none()); + assert_eq!(h.state().session.clipboard.as_ref().map(|c| c.areas.len()), Some(2)); + + h.state_mut().session.execute("selection.set", json!({"range": "A1,B2"})).unwrap(); + h.input_mut().events.push(Event::Copy); + h.step(); + assert!(!h.output().platform_output.commands.iter().any(|c| matches!(c, OutputCommand::CopyText(_)))); + assert_eq!(h.state().message.as_ref().map(|(_, text)| text.as_str()), Some("This action won't work on multiple selections.")); + assert_eq!(h.state().session.clipboard.as_ref().map(|c| c.areas.len()), Some(2), "the earlier copy stays"); +}