From 61f17d46c34c40a1f0012c95e82f810b09d5d7e1 Mon Sep 17 00:00:00 2001 From: Sam Sung <51686767+samwhosung@users.noreply.github.com> Date: Tue, 29 Sep 2026 19:08:28 +0700 Subject: [PATCH] who_sort: the class, race and zone arms order a miss after every resolved name and tie two misses, a total order sort_by can take (#626) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dbc_name_cmp`, behind the who list's class, race and zone keys, answered `Equal` whenever either name was empty. So `"" = "Mage"` and `"" = "Warrior"` while `"Mage" < "Warrior"`: not a total order, and since Rust 1.81 `slice::sort_by` may panic on one. Empty names are real input, since `who_row` writes `""` for any id with no name. The reference's arms (`0x5ada7b`, `0x5adadf`, `0x5adb40`) look both ids up and, when either row is missing, jump to the chain loop's continue label (`0x5adbb2`, from `0x5adab6`): a tie. Its `qsort` tolerates that; ours cannot. Now: - two resolved names compare as before; - two misses tie, so the next key decides, as in the reference; - a miss orders after every resolved name. That case is marked as a deviation, since no total order can tie there. `the_dbc_arms_are_a_total_order` checks antisymmetry and transitivity over a mixed 21-row set for each key. It fails with the old arm, as do `an_unresolved_dbc_name_sorts_after_the_resolved_ones` and `sorting_a_mixed_list_puts_misses_last_without_panicking`. Built and gated on macOS. Fixes #620 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/benilla-ui/src/script/who_sort.rs | 110 ++++++++++++++++++++--- 1 file changed, 97 insertions(+), 13 deletions(-) diff --git a/crates/benilla-ui/src/script/who_sort.rs b/crates/benilla-ui/src/script/who_sort.rs index 7ba19abf..4e3baf6b 100644 --- a/crates/benilla-ui/src/script/who_sort.rs +++ b/crates/benilla-ui/src/script/who_sort.rs @@ -151,14 +151,19 @@ fn ascii_ci_cmp(a: &str, b: &str) -> Ordering { } /// The class, race and zone arms (`0x5ada7b`, `0x5adadf`, `0x5adb40`): an id with no DBC row, an -/// empty name here, ties and passes to the next key (`0x5adbb2`) rather than sorting as `""`. The -/// empty name is the miss marker: the reference's `GetWhoInfo` (`0x5ad6e0`) shows `UNKNOWN` for -/// it, benilla's an empty cell, and a row that carried `UNKNOWN` would sort on that word. +/// empty name here, sorts after every resolved name, not first as `""`. Two misses tie and pass +/// to the next key (`0x5adbb2`), as there. The empty name is the miss marker: the reference's +/// `GetWhoInfo` (`0x5ad6e0`) shows `UNKNOWN` for it, benilla's an empty cell, and a row that +/// carried `UNKNOWN` would sort on that word. fn dbc_name_cmp(a: &str, b: &str) -> Ordering { - if a.is_empty() || b.is_empty() { - return Ordering::Equal; + match (a.is_empty(), b.is_empty()) { + (true, true) => Ordering::Equal, + // Deviation: the reference ties a miss against a hit (`0x5adab6`); no total order can, and + // `sort_by` may panic on one that is not, so the miss goes last. + (true, false) => Ordering::Greater, + (false, true) => Ordering::Less, + (false, false) => ascii_ci_cmp(a, b), } - ascii_ci_cmp(a, b) } #[cfg(test)] @@ -304,22 +309,101 @@ mod tests { } #[test] - fn an_unresolved_dbc_name_ties() { - let mut chain = WhoSortChain::default(); - chain.promote("zone"); + fn an_unresolved_dbc_name_sorts_after_the_resolved_ones() { + // Zone is the seeded front key, so the promote flips it: the default chain already ascends. + let chain = WhoSortChain::default(); let known = row("Bbb", 10, "Mage", "Westfall"); let unknown = row("Aaa", 10, "Mage", ""); + assert_eq!(chain.compare(&known, &unknown), Ordering::Less); + assert_eq!(chain.compare(&unknown, &known), Ordering::Greater); + let other_unknown = row("Ccc", 10, "Mage", ""); assert_eq!( - chain.compare(&known, &unknown), - Ordering::Greater, - "the zone key ties, so the name decides" + chain.compare(&unknown, &other_unknown), + Ordering::Less, + "two misses tie on zone, so the name decides" ); - assert_eq!(chain.compare(&unknown, &known), Ordering::Less); // A guild is not a DBC lookup: an empty guild is a real value and sorts first. assert_eq!(ascii_ci_cmp("", "Legacy"), Ordering::Less); } + type Setter = fn(&mut WhoInfo, &str); + + /// Each DBC-backed key as a single-key comparator, with the setter for its field. + fn dbc_keys() -> [(&'static str, Setter); 3] { + [ + ("class", |r, v| r.class = v.to_string()), + ("race", |r, v| r.race = v.to_string()), + ("zone", |r, v| r.zone = v.to_string()), + ] + } + + fn mixed_rows(set: Setter, n: usize) -> Vec { + let values = [ + "", + "Mage", + "Warrior", + "", + "Elwynn Forest", + "Durotar", + "Mage", + ]; + (0..n) + .map(|i| { + let mut r = row(&format!("N{i:03}"), 1 + (i % 7) as u32, "Mage", "Z"); + set(&mut r, values[(i * 5 + i / 7) % values.len()]); + r + }) + .collect() + } + + #[test] + fn the_dbc_arms_are_a_total_order() { + for (key, set) in dbc_keys() { + let key = WhoSortKey::from_sort_type(key); + let rows = mixed_rows(set, 21); + let le = |a: &WhoInfo, b: &WhoInfo| key.order(a, b) != Ordering::Greater; + for a in &rows { + for b in &rows { + assert_eq!(key.order(a, b), key.order(b, a).reverse(), "{key:?}"); + for c in &rows { + if le(a, b) && le(b, c) { + assert!(le(a, c), "{key:?} not transitive"); + } + if key.order(a, b) == Ordering::Equal && key.order(b, c) == Ordering::Equal + { + assert_eq!(key.order(a, c), Ordering::Equal, "{key:?} tie"); + } + } + } + } + } + } + + #[test] + fn sorting_a_mixed_list_puts_misses_last_without_panicking() { + for (key, set) in dbc_keys() { + let mut chain = WhoSortChain::default(); + chain.promote("level"); // so the key below is promoted from behind, ascending + chain.promote(key); + let mut rows = mixed_rows(set, 64); + chain.sort(&mut rows); + let field = |r: &WhoInfo| match key { + "class" => r.class.clone(), + "race" => r.race.clone(), + _ => r.zone.clone(), + }; + let first_miss = rows.iter().position(|r| field(r).is_empty()).unwrap(); + assert!( + rows[first_miss..].iter().all(|r| field(r).is_empty()), + "{key}" + ); + assert!(rows[..first_miss] + .windows(2) + .all(|w| { ascii_ci_cmp(&field(&w[0]), &field(&w[1])) != Ordering::Greater })); + } + } + #[test] fn the_group_key_displaces_but_orders_nothing() { let mut chain = WhoSortChain::default();