Merge PR #1535: engine: sort and remove duplicates move the rows' notes and hyperlinks

# Conflicts:
#	crates/engine/src/tests.rs

crates/engine/src/cmd/data.rs

@@ -295,6 +295,21 @@ fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec<SortKey>, by_co
         }
         // Move whole lines (cells with formulas adjusted for relative references).
         let lanes: Vec<u32> = 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<Cell>)> = 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<SortKey>, 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;
@@ -776,6 +803,24 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result<Json> {
                     .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<Vec<Option<Cell>>> =
             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)?;
@@ -784,6 +829,13 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result<Json> {
                 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 {
@@ -792,6 +844,12 @@ fn remove_duplicates(s: &mut Session, p: &Json) -> Result<Json> {
             }
         }
         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));
         }

crates/engine/src/tests.rs

@@ -2988,6 +2988,50 @@ fn print_titles_keeps_the_axis_it_is_not_given() {
     assert_eq!(titles(&s), (Some((1, 1)), None));
 }
 
+/// 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).