diff --git a/crates/engine/src/cmd/whatif.rs b/crates/engine/src/cmd/whatif.rs index d9d1ace89..318d0cb72 100644 --- a/crates/engine/src/cmd/whatif.rs +++ b/crates/engine/src/cmd/whatif.rs @@ -70,7 +70,7 @@ pub fn specs() -> Vec { "Advanced…", ["Data", "Sort & Filter"], None, - "{range? (list with headers; default current region), criteria: \"H1:I3\" (header row of field names; rows OR'ed, columns AND'ed; \">100\", \"=abc\", \"a*\", \"<>\"; blank-headed formula cells are computed criteria for the first data row), copyTo?: \"K1\" (else filter in place by hiding rows), unique?: bool}", + "{range? (list with headers; default current region), criteria?: \"H1:I3\" (omitted/blank matches all rows; header row of field names; rows OR'ed, columns AND'ed; \">100\", \"=abc\", \"a*\", \"<>\"; blank-headed formula cells are computed criteria for the first data row), copyTo?: \"K1\" (else filter in place by hiding rows), unique?: bool}", has_doc, advanced_filter ), @@ -795,41 +795,45 @@ fn advanced_filter(s: &mut Session, p: &Json) -> Result { (d.wb.active_sheet, crate::selection::current_region(sh, d.selection.active)) } }; - let (csi, crit) = range_arg(s, p, "criteria", CMD)?; + let criteria = str_param(p, "criteria").filter(|t| !t.trim().is_empty()).map(|t| parse_range(&d.wb, t, CMD)).transpose()?; if list.height() < 1 || list.count() > 50_000_000 { return Err(bad(CMD, "the list range is too large")); } - if crit.width() > 1024 || crit.height() > 10_000 { + if let Some((_, crit)) = criteria + && (crit.width() > 1024 || crit.height() > 10_000) + { return Err(bad(CMD, "the criteria range is too large")); } let wb = &d.wb; let sh = wb.sheet(si).ok_or(EngineError::NoDocument)?; - let csh = wb.sheet(csi).ok_or(EngineError::NoDocument)?; let headers: Vec = (list.start.col..=list.end.col).map(|c| crate::display::cell_text(wb, sh, CellRef::new(list.start.row, c)).trim().to_lowercase()).collect(); let first_data = list.start.row + 1; // Criteria rows → conditions. let mut rows: Vec> = Vec::new(); - for cr in crit.start.row + 1..=crit.end.row { - let mut conds = Vec::new(); - for cc in crit.start.col..=crit.end.col { - let at = CellRef::new(cr, cc); - let cell = csh.cell(at); - let v = csh.value(at); - let formula = cell.and_then(|c| c.formula.as_ref()); - if v.is_empty() && formula.is_none() { - continue; - } - let head = crate::display::cell_text(wb, csh, CellRef::new(crit.start.row, cc)).trim().to_lowercase(); - match headers.iter().position(|h| !head.is_empty() && *h == head) { - Some(off) => conds.push(Cond::Compare(off as u32, v)), - None => match formula.and_then(|f| f.expr()) { - Some(e) => conds.push(Cond::Computed(e, at)), - None => return Err(EngineError::Other(format!("The criteria label `{head}` doesn't match a column of the list."))), - }, + if let Some((csi, crit)) = criteria { + let csh = wb.sheet(csi).ok_or(EngineError::NoDocument)?; + for cr in crit.start.row + 1..=crit.end.row { + let mut conds = Vec::new(); + for cc in crit.start.col..=crit.end.col { + let at = CellRef::new(cr, cc); + let cell = csh.cell(at); + let v = csh.value(at); + let formula = cell.and_then(|c| c.formula.as_ref()); + if v.is_empty() && formula.is_none() { + continue; + } + let head = crate::display::cell_text(wb, csh, CellRef::new(crit.start.row, cc)).trim().to_lowercase(); + match headers.iter().position(|h| !head.is_empty() && *h == head) { + Some(off) => conds.push(Cond::Compare(off as u32, v)), + None => match formula.and_then(|f| f.expr()) { + Some(e) => conds.push(Cond::Computed(e, at)), + None => return Err(EngineError::Other(format!("The criteria label `{head}` doesn't match a column of the list."))), + }, + } } + rows.push(conds); } - rows.push(conds); } let matches_row = |row: u32| -> bool { if rows.is_empty() { @@ -845,7 +849,7 @@ fn advanced_filter(s: &mut Session, p: &Json) -> Result { } Cond::Computed(e, at) => { let shifted = gridcraft_formula::adjust::shift_relative(e.clone(), row as i64 - first_data as i64, 0); - let v = gridcraft_calc::recalc::evaluate_expr(wb, csi, *at, &shifted); + let v = gridcraft_calc::recalc::evaluate_expr(wb, criteria.map(|(si, _)| si).unwrap_or(si), *at, &shifted); matches!(v.scalar().to_bool(), Ok(true)) } }) @@ -1354,6 +1358,17 @@ mod tests { assert_eq!(v(&s, "C2"), Value::Number(1.0)); } + #[test] + fn advanced_filter_unique_without_criteria() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Name"], ["A"], ["A"], ["B"]]})).unwrap(); + let r = s.execute("data.advancedFilter", json!({"range": "A1:A4", "copyTo": "C1", "unique": true})).unwrap(); + assert_eq!(r["matched"], 2); + assert_eq!(r["range"], "C1:C3"); + assert_eq!(v(&s, "C2"), Value::from("A")); + assert_eq!(v(&s, "C3"), Value::from("B")); + } + #[test] fn criteria_semantics() { use super::criterion_matches as m; diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 2ea5cfae0..0bd14ebce 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -252,7 +252,7 @@ pub fn handle(app: &mut SheetApp, ctx: &egui::Context, method: &str, p: &Json) - "ui.dialog.confirm" => match app.dialog.take() { Some(d) => match d.command { Some(cmd) => { - let params = Json::Object(d.values.clone()); + let params = if d.name == "advancedFilter" { crate::dialogs::build_params(&d) } else { Json::Object(d.values.clone()) }; let r = app.run(cmd, params); if r.is_err() { app.dialog = Some(d); diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index fb899c702..b33041085 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -437,6 +437,26 @@ impl Dialog { vec![Check { key: "header", label: "My data has headers" }, Text { key: "columnsText", label: "Columns (blank = all, e.g. A,C):" }], json!({"header": true}), ), + "advancedFilter" => { + let list = app + .session + .active() + .and_then(|d| d.wb.active().map(|sh| gridcraft_engine::selection::current_region(sh, d.selection.active).a1())) + .unwrap_or(sel.clone()); + let mut d = Dialog::form( + "advancedFilter", + "Advanced Filter", + "data.advancedFilter", + vec![], + json!({"action": "inPlace", "range": list, "criteria": "", "copyTo": "", "unique": false}), + ); + for key in ["action", "range", "criteria", "copyTo", "unique"] { + if let Some(value) = params.get(key) { + d.set_value(key, value.clone()); + } + } + d + } "goalSeek" => Dialog::custom("goalSeek", "Goal Seek", json!({"set": active, "to": "0", "changing": ""})), "forecastSheet" => Dialog::form( "forecastSheet", @@ -938,10 +958,43 @@ fn data_validation_form(ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut bool, o ok_cancel(ui, confirm, open); } +fn advanced_filter_form(ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut bool, open: &mut bool) { + use Field::*; + let copy = d.values.get("action").and_then(Json::as_str) == Some("copy"); + d.fields = vec![ + Choice { + key: "action", + label: "Action", + options: vec![("inPlace", "Show matching rows in place"), ("copy", "Copy matching rows elsewhere")], + }, + Text { key: "range", label: "List range:" }, + Text { key: "criteria", label: "Criteria range (optional):" }, + ]; + if copy { + d.fields.push(Text { key: "copyTo", label: "Copy to:" }); + } + d.fields.push(Check { key: "unique", label: "Unique rows only" }); + form_fields(ui, d); + // Constrain the button row: the shared right-to-left layout otherwise + // consumes the window's remaining height instead of fitting the buttons. + ui.allocate_ui(vec2(ui.available_width(), ui.spacing().interact_size.y + 8.0), |ui| ok_cancel(ui, confirm, open)); +} + /// Builds the command params from form values for dialogs that need translation. -fn build_params(d: &Dialog) -> Json { +pub(crate) fn build_params(d: &Dialog) -> Json { let mut v = Json::Object(d.values.clone()); match d.name.as_str() { + "advancedFilter" => { + if let Some(m) = v.as_object_mut() { + m.remove("action"); + if d.values.get("action").and_then(Json::as_str) != Some("copy") { + m.remove("copyTo"); + } + if m.get("criteria").and_then(Json::as_str).is_some_and(|s| s.trim().is_empty()) { + m.remove("criteria"); + } + } + } "dataValidation" => { let kind = d.values.get("type").and_then(Json::as_str).unwrap_or("any"); let operator = d.values.get("operator").and_then(Json::as_str).unwrap_or("between"); @@ -1042,12 +1095,17 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { .default_width(width) .anchor(egui::Align2::CENTER_CENTER, vec2(0.0, -40.0)) .open(&mut win_open); - let window = if d.name == "insertPicture" { window.default_height(240.0) } else { window }; + let window = match d.name.as_str() { + "insertPicture" => window.default_height(240.0), + "advancedFilter" => window.default_height(0.0), + _ => window, + }; window.show(ctx, |ui| { ui.set_min_width(width - 20.0); match d.name.as_str() { "formatCells" => format_cells(app, ui, &mut d, &mut confirm), "dataValidation" => data_validation_form(ui, &mut d, &mut confirm, &mut open), + "advancedFilter" => advanced_filter_form(ui, &mut d, &mut confirm, &mut open), "insertFunction" => insert_function(app, ui, &mut d, &mut confirm), "find" => find(app, ui, &mut d), "goTo" => go_to(app, ui, &mut d, &mut confirm), diff --git a/crates/ui-egui/src/keytips/keytip_table/generated.rs b/crates/ui-egui/src/keytips/keytip_table/generated.rs index 79920b5b8..17f0ff0cd 100644 --- a/crates/ui-egui/src/keytips/keytip_table/generated.rs +++ b/crates/ui-egui/src/keytips/keytip_table/generated.rs @@ -291,7 +291,7 @@ pub const SEQUENCES: &[(&str, Entry)] = &[ ("APR", Entry::Finish(Action::Pending("data.recentSources"))), // pending; Excel: Recent Sources; gap #12; reserved until the command lands ("APS", Entry::Finish(Action::Command("data.queriesConnections"))), // Excel: Show Queries ("APT", Entry::Finish(Action::Pending("data.fromTable"))), // pending; Excel: From Table (query); gap #12; reserved until the command lands - ("AQ", Entry::Finish(Action::Command("data.advancedFilter"))), // Excel: Advanced Filter + ("AQ", Entry::Finish(Action::Dialog("advancedFilter"))), // Excel: Advanced Filter ("AR", Entry::Continue), // group: group: Queries & Connections ("ARA", Entry::Finish(Action::Command("data.refreshAll"))), // Excel: Refresh All ("ARC", Entry::Finish(Action::Pending("data.cancelRefresh"))), // pending; Excel: Cancel Refresh; gap #12; reserved until the command lands diff --git a/crates/ui-egui/src/keytips/keytips.json b/crates/ui-egui/src/keytips/keytips.json index 59aeadcfb..efe5c938b 100644 --- a/crates/ui-egui/src/keytips/keytips.json +++ b/crates/ui-egui/src/keytips/keytips.json @@ -1965,8 +1965,8 @@ }, { "seq": "AQ", - "kind": "command", - "target": "data.advancedFilter", + "kind": "dialog", + "target": "advancedFilter", "excel": "Advanced Filter", "note": "" }, @@ -2523,4 +2523,4 @@ "note": "Alt+Q search box does not exist yet (issue #49)" } ] -} \ No newline at end of file +} diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 9c6d89eeb..be1a9604b 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -1925,6 +1925,11 @@ fn data(app: &mut SheetApp, ui: &mut Ui) { if __k.clicked() { act(app, "data.reapply", json!({})); } + let __k = small_button(ui, Icon::Filter, "Advanced", "Advanced Filter", false); + control_keytip(app, ui, &__k, "dialog:advancedFilter"); + if __k.clicked() { + app.open_dialog("advancedFilter", json!({})); + } }); sep(ui); let __k = big_button(ui, Icon::TextColumns, "Text to\nColumns", "Text to Columns", false); diff --git a/crates/ui-egui/tests/advanced_filter.rs b/crates/ui-egui/tests/advanced_filter.rs new file mode 100644 index 000000000..449c6a47e --- /dev/null +++ b/crates/ui-egui/tests/advanced_filter.rs @@ -0,0 +1,72 @@ +//! Advanced Filter's dialog must route both actions to the same engine command. + +use egui::vec2; +use egui_kittest::kittest::Queryable; +use gridcraft_engine::Session; +use gridcraft_ui_egui::SheetApp; +use gridcraft_ui_egui::control::{Outcome, handle}; +use serde_json::{Value as Json, json}; + +fn harness() -> egui_kittest::Harness<'static, SheetApp> { + let mut session = Session::new(); + session.new_workbook(); + let mut h = egui_kittest::Harness::builder().with_size(vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(&ui.ctx().clone()); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(6); + h +} + +fn control(h: &mut egui_kittest::Harness<'static, SheetApp>, method: &str, params: Json) -> Json { + let ctx = h.ctx.clone(); + match handle(h.state_mut(), &ctx, method, ¶ms) { + Outcome::Done(v) => v, + Outcome::Screenshot { .. } => json!(null), + } +} + +#[test] +fn dialog_offers_copy_action_and_runs_unique_extract() { + let mut h = harness(); + h.state_mut().run("range.setValues", json!({"range": "A1", "values": [["Name"], ["A"], ["A"], ["B"]]})).unwrap(); + h.state_mut().open_dialog("advancedFilter", json!({})); + h.run_steps(6); + h.get_by_label("Copy matching rows elsewhere").click(); + h.run_steps(6); + let rect = h.ctx.memory(|m| m.area_rect(egui::Id::new("dialog").with("advancedFilter"))).unwrap(); + assert!(rect.width() < 500.0 && rect.height() < 400.0, "dialog must fit its controls: {rect:?}"); + assert!( + h.state() + .dialog + .as_ref() + .is_some_and(|d| d.fields.iter().any(|f| matches!(f, gridcraft_ui_egui::dialogs::Field::Text { key: "copyTo", .. }))) + ); + assert_eq!(h.state().dialog.as_ref().unwrap().values["range"], "A1:A4"); + control(&mut h, "ui.dialog.set", json!({"field": "copyTo", "value": "C1"})); + control(&mut h, "ui.dialog.set", json!({"field": "unique", "value": true})); + h.run_steps(6); + h.get_by_label(" OK ").click(); + h.run_steps(6); + assert!(h.state().dialog.is_none()); + assert!(h.state().message.is_none()); + assert_eq!(h.state().session.active().unwrap().wb.active().unwrap().value(gridcraft_engine::core::CellRef::parse("C3").unwrap()).display(), "B"); +} + +#[test] +fn in_place_action_does_not_use_stale_copy_destination() { + let mut h = harness(); + h.state_mut().run("range.setValues", json!({"range": "A1", "values": [["Name"], ["A"], ["B"]]})).unwrap(); + h.state_mut().run("range.setValues", json!({"range": "H1", "values": [["Name"], ["B"]]})).unwrap(); + control(&mut h, "ui.dialog", json!({"name": "advancedFilter"})); + control(&mut h, "ui.dialog.set", json!({"field": "range", "value": "A1:A3"})); + control(&mut h, "ui.dialog.set", json!({"field": "criteria", "value": "H1:H2"})); + control(&mut h, "ui.dialog.set", json!({"field": "copyTo", "value": "K1"})); + let r = control(&mut h, "ui.dialog.confirm", json!({})); + assert_eq!(r["ok"], true, "{r}"); + assert!(h.state().session.active().unwrap().wb.active().unwrap().is_row_hidden(1)); + assert!(h.state().session.active().unwrap().wb.active().unwrap().value(gridcraft_engine::core::CellRef::parse("K1").unwrap()).is_empty()); +} diff --git a/crates/ui-egui/tests/keytips_generated.rs b/crates/ui-egui/tests/keytips_generated.rs index 653554857..5d649c56d 100644 --- a/crates/ui-egui/tests/keytips_generated.rs +++ b/crates/ui-egui/tests/keytips_generated.rs @@ -2149,7 +2149,7 @@ fn keytip_apt_() { fn keytip_aq_() { let mut h = harness(blank()); sequence(&mut h, "AQ", false); - assert!(h.state().message.is_some() || h.state().toast.is_some(), "AQ must invoke data.advancedFilter and surface its missing-params error"); + assert!(h.state().dialog.is_some(), "AQ must open the advancedFilter dialog"); } #[test] diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 22e221e69..bbf5ee966 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -1,6 +1,6 @@ # UI parity -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-13 · **Change:** minor (horizontal scrollbar in the sheet tab bar, #840) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-16 · **Change:** minor (Advanced Filter dialog, #1622) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Interaction, controls and feel, against Excel. Part of [target-app-parity.md](target-app-parity.md). **UI/UX fidelity: ~55% ready, 50–80 h** (estimated). Ribbon command coverage is measured @@ -51,6 +51,7 @@ this logic, with one undo step. | Point mode (click/drag/arrow references), reference colouring, F4 | done | | AutoComplete in columns, function autocomplete and argument tips | done; cell helpers clip to the worksheet and hide when the editor scrolls out (#200) | | Header resize and double-click AutoFit | done (#42, #88 for header dividers) | +| Data › Advanced Filter | dialog offers in-place or copy-to filtering, list and optional criteria ranges, and unique records; Alt+A,Q opens it (#1622). The engine's copy-to path clears only the result footprint and rejects worksheet overflow (#1031). | | Outline symbols for grouped rows / columns (band beside the headers, brackets, − / +, level buttons 1…n+1) | done (#373): clicks run `data.hideDetail` / `data.showDetail` / `data.showLevels`, frozen panes and right-to-left sheets included; summary rows/columns are always below/right (the "Summary rows below detail" setting isn't modelled), no "Show outline symbols" option yet, and a block of cells still groups rows without the Rows / Columns dialog | | Reorder selected whole columns by dragging their headers | done (10-11) | | Freeze panes, split panes | freeze done; split to verify | @@ -161,6 +162,7 @@ scrollbar bugs 2–4 · pixel/feel pass against observed Excel 7–12. | Date | Change | Summary | |---|---|---| +| 2026-10-16 | minor | Advanced Filter ribbon dialog, optional criteria for unique-only extraction (#1622); safe copy-to output (#1031) | | 2026-10-12 | minor | The File view's recent list tells when each workbook was last changed, read off the UI thread | | 2026-10-13 | minor | Horizontal scrollbar in the sheet tab bar: drag, track click pages, frozen columns excluded from the page (#840) | | 2026-10-12 | minor | Ctrl+wheel zoom steps along a ladder of percentages about 20% apart that never skips 100%, gliding to each step with animations on, so a notch out undoes a notch in (it drifted: 100 → 122 → 101); Excel steps 15 points, see decisions.md |