Merge PR #1979: Seed the Sort dialog column from the active cell (#1906)

crates/ui-egui/src/dialogs.rs

@@ -710,7 +710,11 @@ impl Dialog {
                 }],
                 json!({"kind": "blanks"}),
             ),
-            "sort" => Dialog::custom("sort", "Sort", json!({"header": true, "levels": [{"column": "A", "order": "asc"}]})),
+            "sort" => {
+                // Seed "Sort by" from the column the user is actually in (#1906).
+                let col = app.session.active().map(|d| gridcraft_engine::core::col_to_letters(d.selection.active.col)).unwrap_or_else(|| "A".into());
+                Dialog::custom("sort", "Sort", json!({"header": true, "levels": [{"column": col, "order": "asc"}]}))
+            }
             "nameManager" => Dialog::custom("nameManager", "Name Manager", json!({})),
             "manageRules" => Dialog::custom("manageRules", "Conditional Formatting Rules Manager", json!({})),
             "commandSearch" => Dialog::custom("commandSearch", "Search Commands", json!({})),
@@ -2038,12 +2042,43 @@ fn sort(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut boo
     ui.checkbox(&mut header, "My list has headers");
     d.values.insert("header".into(), json!(header));
     let mut levels: Vec<Json> = d.values.get("levels").and_then(Json::as_array).cloned().unwrap_or_default();
+    // Columns of the region around the active cell, labelled with their header (or first value).
+    let columns: Vec<(String, String)> = app
+        .session
+        .active()
+        .and_then(|d| {
+            let sh = d.wb.active()?;
+            let region = gridcraft_engine::selection::current_region(sh, d.selection.active);
+            Some(
+                (region.start.col..=region.end.col)
+                    .map(|c| {
+                        let letter = gridcraft_engine::core::col_to_letters(c);
+                        // The region starts at the header row when one is present, so the
+                        // label always comes from start.row: header text with headers on,
+                        // first data value with them off.
+                        let label_row = region.start.row;
+                        let text = gridcraft_engine::display::cell_text(&d.wb, sh, gridcraft_engine::core::CellRef::new(label_row, c));
+                        let text: String = text.chars().take(24).collect();
+                        (letter.clone(), if text.is_empty() { letter } else { format!("{letter}: {text}") })
+                    })
+                    .collect(),
+            )
+        })
+        .unwrap_or_default();
     let mut remove = None;
     for (i, lv) in levels.iter_mut().enumerate() {
         ui.horizontal(|ui| {
             ui.label(if i == 0 { "Sort by" } else { "Then by" });
             let mut col = lv["column"].as_str().unwrap_or("A").to_string();
-            ui.add(egui::TextEdit::singleline(&mut col).desired_width(50.0));
+            if columns.is_empty() {
+                ui.add(egui::TextEdit::singleline(&mut col).desired_width(50.0));
+            } else {
+                egui::ComboBox::from_id_salt(("sort_col", i)).selected_text(col.clone()).show_ui(ui, |ui| {
+                    for (letter, label) in &columns {
+                        ui.selectable_value(&mut col, letter.clone(), label.as_str());
+                    }
+                });
+            }
             lv["column"] = json!(col.to_ascii_uppercase());
             let mut by = lv["by"].as_str().unwrap_or("values").to_string();
             egui::ComboBox::from_id_salt(("by", i))
@@ -2073,7 +2108,15 @@ fn sort(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut boo
         levels.remove(i);
     }
     if ui.button("+ Add Level").clicked() {
-        levels.push(json!({"column": "B", "order": "asc"}));
+        let used: Vec<String> = levels.iter().filter_map(|l| l.get("column").and_then(Json::as_str).map(str::to_string)).collect();
+        let next = columns
+            .iter()
+            .map(|(l, _)| l)
+            .find(|l| !used.contains(l))
+            .cloned()
+            .or_else(|| columns.last().map(|(l, _)| l.clone()))
+            .unwrap_or_else(|| "B".into());
+        levels.push(json!({"column": next, "order": "asc"}));
     }
     d.values.insert("levels".into(), Json::Array(levels.clone()));
     let mut open = true;

crates/ui-egui/tests/sort_dialog_columns.rs

@@ -0,0 +1,81 @@
+//! #1906: the Sort dialog seeds "Sort by" from the active column instead of a
+//! free-text box stuck on "A", and the column list offers the region's columns.
+
+use egui::{Key, Modifiers};
+use egui_kittest::kittest::Queryable;
+use gridcraft_engine::Session;
+use gridcraft_ui_egui::SheetApp;
+use serde_json::json;
+
+type Harness = egui_kittest::Harness<'static, SheetApp>;
+
+fn harness(session: Session) -> Harness {
+    let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state(
+        |ui, app: &mut SheetApp| {
+            let ctx = ui.ctx().clone();
+            app.logic(&ctx);
+            app.ui(ui);
+        },
+        SheetApp::new(session, Default::default()),
+    );
+    h.run_steps(4);
+    h
+}
+
+/// Header row + three rows; the user's cursor sits in column B (Revenue).
+fn sales() -> Session {
+    let mut s = Session::new();
+    s.new_workbook();
+    s.execute("range.setValues", json!({"range": "A1", "values": [["Month", "Revenue"], ["Mar", 30], ["Jan", 10], ["Feb", 20]]})).unwrap();
+    s.execute("selection.set", json!({"range": "B2"})).unwrap();
+    s
+}
+
+fn col(h: &Harness) -> String {
+    h.state().dialog.as_ref().unwrap().values["levels"][0]["column"].as_str().unwrap_or("<missing>").to_string()
+}
+
+#[test]
+fn sort_dialog_seeds_the_active_column() {
+    let mut h = harness(sales());
+    h.state_mut().open_dialog("sort", json!({}));
+    h.run_steps(2);
+    assert_eq!(col(&h), "B", "Sort by starts on the column the cursor is in, not A");
+}
+
+#[test]
+fn add_level_picks_a_column_no_level_uses_yet() {
+    let mut h = harness(sales());
+    h.state_mut().open_dialog("sort", json!({}));
+    h.run_steps(2);
+    // Level 1 seeded B; clicking "+ Add Level" must not default level 2 to B too (#1906 review).
+    h.run_steps(6);
+    h.get_by_label("+ Add Level").click();
+    h.run_steps(6);
+    let cols: Vec<String> = h.state().dialog.as_ref().unwrap().values["levels"]
+        .as_array()
+        .map(|a| a.iter().filter_map(|l| l["column"].as_str().map(str::to_string)).collect())
+        .unwrap_or_default();
+    assert_eq!(cols.len(), 2, "two levels after Add Level");
+    assert_ne!(cols[0], cols[1], "the second level defaults to a different column: {cols:?}");
+}
+
+#[test]
+fn sorting_by_the_seeded_column_orders_the_rows() {
+    let mut h = harness(sales());
+    h.state_mut().open_dialog("sort", json!({}));
+    h.run_steps(2);
+    // Accept the dialog: rows must come back ordered by Revenue (10, 20, 30).
+    h.input_mut().events.push(egui::Event::Key {
+        key: Key::Enter,
+        physical_key: Some(Key::Enter),
+        pressed: true,
+        repeat: false,
+        modifiers: Modifiers::NONE,
+    });
+    h.run_steps(2);
+    let d = h.state().session.active().unwrap();
+    let sh = d.wb.active().unwrap();
+    let nums: Vec<i64> = (1..4).map(|r| sh.value(gridcraft_engine::core::CellRef::new(r, 1))).filter_map(|v| v.as_f64().map(|n| n as i64)).collect();
+    assert_eq!(nums, vec![10, 20, 30], "sort ran on the seeded column B");
+}