diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 383624b70..357e0e207 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -295,6 +295,21 @@ fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_co } // Move whole lines (cells with formulas adjusted for relative references). let lanes: Vec = if by_cols { (body.start.row..=body.end.row).collect() } else { (body.start.col..=body.end.col).collect() }; + // Notes and hyperlinks hang off a cell, not a row, so they travel with the row they + // describe — the way cell pictures and row heights do. Collect them before inserting, + // so a swap can't resurrect one from a destination that is also a source. + let side: Vec<(CellRef, Comment)> = order + .iter() + .enumerate() + .flat_map(|(dst_i, &src_i)| lanes.iter().map(move |&k| (src_i, dst_i as u32, k))) + .filter_map(|(src_i, dst_i, k)| sh.comments.get(&line(src_i, k)).map(|cm| (line(dst_i, k), cm.clone()))) + .collect(); + let links: Vec<(CellRef, Hyperlink)> = order + .iter() + .enumerate() + .flat_map(|(dst_i, &src_i)| lanes.iter().map(move |&k| (src_i, dst_i as u32, k))) + .filter_map(|(src_i, dst_i, k)| sh.hyperlinks.get(&line(src_i, k)).map(|h| (line(dst_i, k), h.clone()))) + .collect(); let mut moved: Vec<(CellRef, Option)> = Vec::with_capacity(body.count() as usize); let mut pictures = Vec::new(); for (dst_i, &src_i) in order.iter().enumerate() { @@ -354,6 +369,18 @@ fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_co } } } + // Every cell of the body is the destination of exactly one source cell, so clearing the + // body and inserting the permuted entries is the same permutation the cells went through. + for c in body.iter() { + shm.comments.remove(&c); + shm.hyperlinks.remove(&c); + } + for (at, cm) in side { + shm.comments.insert(at, cm); + } + for (at, h) in links { + shm.hyperlinks.insert(at, h); + } shm.cell_pictures.extend(pictures); for (i, h) in heights.into_iter().enumerate() { let row = body.start.row + i as u32; @@ -774,6 +801,24 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result { .filter_map(move |col| sh.cell_pictures.get(&CellRef::new(*row, col)).map(|p| (CellRef::new(start + i as u32, col), p.clone()))) }) .collect(); + // Rows a filter keeps are compacted to `start + i`, so the notes and hyperlinks that + // describe them move with them; the ones on removed rows are dropped with the row. + let notes: Vec<_> = keep + .iter() + .enumerate() + .flat_map(|(i, row)| { + (r.start.col..=r.end.col) + .filter_map(move |col| sh.comments.get(&CellRef::new(*row, col)).map(|cm| (CellRef::new(start + i as u32, col), cm.clone()))) + }) + .collect(); + let links: Vec<_> = keep + .iter() + .enumerate() + .flat_map(|(i, row)| { + (r.start.col..=r.end.col) + .filter_map(move |col| sh.hyperlinks.get(&CellRef::new(*row, col)).map(|h| (CellRef::new(start + i as u32, col), h.clone()))) + }) + .collect(); let rows: Vec>> = keep.iter().map(|row| (r.start.col..=r.end.col).map(|c| sh.cell(CellRef::new(*row, c)).cloned()).collect()).collect(); let shm = cx.sheet_mut(sheet)?; @@ -782,6 +827,13 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result { shm.remove_cell(CellRef::new(row, c)); } } + for row in start..=r.end.row { + for c in r.start.col..=r.end.col { + let at = CellRef::new(row, c); + shm.comments.remove(&at); + shm.hyperlinks.remove(&at); + } + } for (i, cells) in rows.into_iter().enumerate() { for (j, cell) in cells.into_iter().enumerate() { if let Some(cell) = cell { @@ -790,6 +842,12 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result { } } shm.cell_pictures.extend(pictures); + for (at, cm) in notes { + shm.comments.insert(at, cm); + } + for (at, h) in links { + shm.hyperlinks.insert(at, h); + } for c in RangeRef::new(CellRef::new(start, r.start.col), r.end).iter() { cx.changed.push((sheet, c)); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..54c348606 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2925,6 +2925,50 @@ fn notes_comments_and_links_survive_a_json_save() { assert_eq!(after.cells, before.cells); } +/// Sorting and removing duplicates rewrite the rows of a range. The cells, pictures, row heights +/// and anchors travel with them, but the notes and hyperlinks hung off their old cells: a note +/// describing one record ended up annotating whatever landed on that row, and on a removed row it +/// annotated blank cells. +#[test] +fn sort_and_remove_duplicates_move_the_rows_notes_and_links() { + fn setup(values: [&str; 5]) -> Session { + let mut s = s(); + for (row, text) in values.iter().enumerate() { + s.execute("cell.set", json!({"cell": format!("A{}", row + 1), "input": text})).unwrap(); + } + s.execute("review.newNote", json!({"cell": "B5", "text": "late", "author": "Example"})).unwrap(); + s.execute("insert.link", json!({"cell": "B4", "target": "https://example.com/4"})).unwrap(); + s + } + let notes_and_links = |s: &Session| { + let sh = s.doc().unwrap().wb.active().unwrap().clone(); + let cells = ["B2", "B3", "B4", "B5"].map(|c| CellRef::parse(c).unwrap()); + ( + cells.map(|c| sh.comments.get(&c).map(|m| m.text.clone())), + cells.map(|c| sh.hyperlinks.get(&c).map(|h| h.target.clone())), + ) + }; + + // Sort ascending with unique keys d,a,e,b,c: the order becomes a(2), b(4), c(5), d(1), e(3). + // The note on B5 describes row 5's record ("c"), which lands on row 3; the link on B4 + // describes row 4's record ("b"), which lands on row 2. Both must travel with their record. + let mut s = setup(["d", "a", "e", "b", "c"]); + s.execute("data.sort", json!({"range": "A1:B5", "keys": [{"column": "A"}]})).unwrap(); + assert_eq!(v(&s, "A3"), Value::text("c")); + assert_eq!(v(&s, "A5"), Value::text("e")); + let (notes, links) = notes_and_links(&s); + assert_eq!(notes[1], Some("late".to_string()), "the note moved with its record: {notes:?}"); + assert_eq!(links[0], Some("https://example.com/4".to_string()), "the link moved with its record: {links:?}"); + + // Remove duplicates keeps the first of each value: rows 3 and 5 go, and row 4's record moves + // to row 3. The note on B5 described row 5 and goes with it. + let mut s = setup(["a", "b", "a", "c", "a"]); + s.execute("data.removeDuplicates", json!({"range": "A1:B5"})).unwrap(); + assert_eq!(v(&s, "A3"), Value::text("c")); + let (notes, _) = notes_and_links(&s); + assert!(notes.iter().all(Option::is_none), "a note on a removed row goes with it: {notes:?}"); +} + /// Insert › Link sorts a target as Excel's dialog does: a sheet reference, a defined name or `#…` is /// a place in this document, anything else an existing file or web page. A file saved as a /// `location` opens nothing in Excel (#800).