From da97bde61c5b8f688976d2ade4a683c0b1dcfc29 Mon Sep 17 00:00:00 2001 From: bfenty <1398236+bfenty@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:21:50 +0000 Subject: [PATCH] fix(ui): seed Sort dialog column from the active cell The Sort dialog's 'Sort by' box was a free-text field hard-coded to 'A', so sorting a region in column B silently sorted column A unless the user noticed and retyped the letter. It now seeds from the column the cursor is in and renders a dropdown of the current region's columns, labelled with their header text. '+ Add Level' seeds the next region column instead of a hard-coded B. Closes #1906 Co-Authored-By: Claude --- crates/ui-egui/src/dialogs.rs | 39 ++++++++++++- crates/ui-egui/tests/sort_dialog_columns.rs | 63 +++++++++++++++++++++ 2 files changed, 99 insertions(+), 3 deletions(-) create mode 100644 crates/ui-egui/tests/sort_dialog_columns.rs diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index fb899c702..59c098343 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -668,7 +668,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!({})), @@ -1923,12 +1927,40 @@ 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 = 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); + let label_row = if header { region.start.row + 1 } else { 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)) @@ -1958,7 +1990,8 @@ 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 next = columns.get(levels.len()).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; diff --git a/crates/ui-egui/tests/sort_dialog_columns.rs b/crates/ui-egui/tests/sort_dialog_columns.rs new file mode 100644 index 000000000..9a0541b6a --- /dev/null +++ b/crates/ui-egui/tests/sort_dialog_columns.rs @@ -0,0 +1,63 @@ +//! #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 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("").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 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 = (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"); +}