collective
Browse / github.com/storytold/gridcraft / patch
Sort: identical merged blocks that tile the range sort as units, as in Excel
Sort: identical merged blocks that tile the range sort as units, as in Excel Issues 1543 and 1540 (notes, hyperlinks and merged areas not moving with Sort or Cut and Paste) were already fixed in main by the merged PRs 1535 and 1534; the issues were never closed. I checked the behaviour in real Excel and found one remaining gap, which this PR fixes. ## What I observed in Excel (COM, new temporary workbooks) - Sort ascending/descending, with and without header, by cell colour, and a whole-row sort: notes and hyperlinks move with their row. GridCraft already does this. - Sort with merges: Excel sorts only when identical merged blocks tile the entire sort range (full-widt
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Harrison-Booth (imported from pull request #2689, open)
- Base
11bb67386c9bb19b0717e21e98adc608162ae2e5- Size
- +85 −5 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
be428d2c0443598312f6f61f
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/data/sort.rs crates/engine/src/tests.rs
@@ -89,9 +89,27 @@ fn do_sort(s: &mut Session, sheet: usize, r: RangeRef, header: bool, keys: Vec<S } edit(s, |cx| { let Some(sh) = cx.wb.sheet(sheet) else { return Ok(Json::Null) };- if sh.merges.iter().any(|m| m.intersects(&body) && !m.is_single()) {- return Err(EngineError::Other("To do this, all the merged cells need to be the same size.".into()));- }+ // Excel sorts merged cells only when identical merged blocks tile the whole range; the+ // blocks then move as units, and since they are all alike the merges themselves stay put.+ let merged: Vec<&RangeRef> = sh.merges.iter().filter(|m| m.intersects(&body) && !m.is_single()).collect();+ let unit = match merged.first() {+ None => 1,+ Some(first) => {+ let (mh, mw) = (first.height(), first.width());+ let tiles = merged.iter().all(|m| {+ body.contains_range(m)+ && m.height() == mh+ && m.width() == mw+ && (m.start.row - body.start.row) % mh.max(1) == 0+ && (m.start.col - body.start.col) % mw.max(1) == 0+ });+ let area: u64 = merged.iter().map(|m| m.count()).sum();+ if !tiles || area != body.count() {+ return Err(EngineError::Other("To do this, all the merged cells need to be the same size.".into()));+ }+ (if by_cols { mw } else { mh }).max(1)+ }+ }; let n = if by_cols { body.width() } else { body.height() }; let line = |i: u32, k: u32| if by_cols { CellRef::new(k, body.start.col + i) } else { CellRef::new(body.start.row + i, k) }; let theme = cx.wb.theme.clone();@@ -110,8 +128,8 @@ fn do_sort(s: &mut Session, sheet: usize, r: RangeRef, header: bool, keys: Vec<S }; (v, color) };- let mut order: Vec<u32> = (0..n).collect();- let keyed: Vec<Vec<(Value, u32)>> = (0..n).map(|i| keys.iter().map(|k| key_val(i, k)).collect()).collect();+ let mut order: Vec<u32> = (0..n / unit).collect();+ let keyed: Vec<Vec<(Value, u32)>> = (0..n / unit).map(|i| keys.iter().map(|k| key_val(i * unit, k)).collect()).collect(); order.sort_by(|&a, &b| { for (ki, k) in keys.iter().enumerate() { let (va, ca) = &keyed[a as usize][ki];@@ -134,6 +152,8 @@ fn do_sort(s: &mut Session, sheet: usize, r: RangeRef, header: bool, keys: Vec<S } Ordering::Equal });+ // Back from blocks to lines.+ let order: Vec<u32> = order.iter().flat_map(|&b| (0..unit).map(move |j| b * unit + j)).collect(); if order.iter().enumerate().all(|(i, &o)| i as u32 == o) { return Ok(Json::Null); }@@ -3981,6 +3981,66 @@ fn sort_and_remove_duplicates_move_the_rows_notes_and_links() { assert!(notes.iter().all(Option::is_none), "a note on a removed row goes with it: {notes:?}"); } +/// Excel sorts merged cells only when identical merged blocks tile the whole range (observed in+/// Excel: a mix of merged and unmerged cells, merges of different sizes, or a merge sticking out+/// of the range all give "To do this, all the merged cells need to be the same size."). The+/// blocks move as units, with their notes and hyperlinks, and the merges stay where they are.+#[test]+fn sort_moves_identical_merged_blocks_and_refuses_anything_else() {+ let merges = |s: &Session| {+ let mut m: Vec<String> = s.doc().unwrap().wb.active().unwrap().merges.iter().map(|m| m.a1()).collect();+ m.sort();+ m+ };+ // Every row merged A:B, sorted on A: whole rows of blocks move, merges are unchanged.+ let mut s = s();+ for (i, t) in ["d", "b", "c", "a"].iter().enumerate() {+ s.execute("cell.set", json!({"cell": format!("A{}", i + 1), "input": t})).unwrap();+ s.execute("home.mergeCells", json!({"range": format!("A{0}:B{0}", i + 1)})).unwrap();+ }+ s.execute("review.newNote", json!({"cell": "A1", "text": "nd", "author": "Example"})).unwrap();+ s.execute("insert.link", json!({"cell": "A2", "target": "https://example.com/b"})).unwrap();+ s.execute("data.sort", json!({"range": "A1:B4", "keys": [{"column": "A"}]})).unwrap();+ assert_eq!(["A1", "A2", "A3", "A4"].map(|c| v(&s, c)), ["a", "b", "c", "d"].map(Value::text));+ assert_eq!(merges(&s), ["A1:B1", "A2:B2", "A3:B3", "A4:B4"]);+ let sh = s.doc().unwrap().wb.active().unwrap().clone();+ assert_eq!(sh.comments.get(&CellRef::parse("A4").unwrap()).map(|m| m.text.as_str()), Some("nd"));+ assert!(sh.hyperlinks.contains_key(&CellRef::parse("A2").unwrap()));+ assert!(!sh.hyperlinks.contains_key(&CellRef::parse("A1").unwrap()));++ // Two-row merges sort as blocks of two rows.+ let mut s = self::s();+ s.execute("cell.set", json!({"cell": "A1", "input": "d"})).unwrap();+ s.execute("cell.set", json!({"cell": "A3", "input": "b"})).unwrap();+ s.execute("home.mergeCells", json!({"range": "A1:A2"})).unwrap();+ s.execute("home.mergeCells", json!({"range": "A3:A4"})).unwrap();+ s.execute("data.sort", json!({"range": "A1:A4", "keys": [{"column": "A"}]})).unwrap();+ assert_eq!(v(&s, "A1"), Value::text("b"));+ assert_eq!(v(&s, "A3"), Value::text("d"));+ assert_eq!(merges(&s), ["A1:A2", "A3:A4"]);++ // Same-size merges that do not cover the whole range (column C is unmerged), one merged row+ // among unmerged ones, and a merge covering only part of the range are all refused, unchanged.+ for (range, merge, key) in [("A1:C4", "A1:B1", "C"), ("A1:A4", "A1:B1", "A"), ("A1:A4", "A2:A3", "A")] {+ let mut s = self::s();+ for (i, t) in ["d", "b", "c", "a"].iter().enumerate() {+ s.execute("cell.set", json!({"cell": format!("A{}", i + 1), "input": t})).unwrap();+ }+ s.execute("home.mergeCells", json!({"range": merge})).unwrap();+ let err = s.execute("data.sort", json!({"range": range, "keys": [{"column": key}]})).unwrap_err();+ assert!(err.to_string().contains("merged cells need to be the same size"), "{range}/{merge}: {err}");+ assert_eq!(v(&s, "A1"), Value::text("d"));+ }++ // Merges outside the range don't matter.+ let mut s = self::s();+ s.execute("cell.set", json!({"cell": "A1", "input": "b"})).unwrap();+ s.execute("cell.set", json!({"cell": "A2", "input": "a"})).unwrap();+ s.execute("home.mergeCells", json!({"range": "E1:F2"})).unwrap();+ s.execute("data.sort", json!({"range": "A1:A2", "keys": [{"column": "A"}]})).unwrap();+ assert_eq!(v(&s, "A1"), Value::text("a"));+}+ /// 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).