From 8c1c7cfd27a22b366ce2ddb94120165b5978147e Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 5 Nov 2026 00:47:47 +0100 Subject: [PATCH] Manage Rules: scroll the rule list and lay out only the visible rules (#3321) The dialog laid out one row per rule with no height limit and ran home.manageRules (copying and serialising every rule, and journaling a step) on every frame. A sheet with thousands of rules made the window as tall as the whole list and the application window go blank. Co-Authored-By: Claude Sonnet 5.5 --- .../ui-egui/src/dialogs/cond_format_rules.rs | 54 ++++++++++------ crates/ui-egui/tests/it/cf_rules_manager.rs | 64 +++++++++++++++++++ crates/ui-egui/tests/it/main.rs | 1 + 3 files changed, 98 insertions(+), 21 deletions(-) create mode 100644 crates/ui-egui/tests/it/cf_rules_manager.rs diff --git a/crates/ui-egui/src/dialogs/cond_format_rules.rs b/crates/ui-egui/src/dialogs/cond_format_rules.rs index 4d8a22375..23af88aae 100644 --- a/crates/ui-egui/src/dialogs/cond_format_rules.rs +++ b/crates/ui-egui/src/dialogs/cond_format_rules.rs @@ -1,28 +1,40 @@ use super::{Json, SheetApp, json}; +/// The list scrolls inside the dialog: a sheet read from a file can carry thousands of rules, and +/// a list laid out in full made the window as tall as all of them (issue #3321). +const LIST_HEIGHT: f32 = 360.0; + pub(super) fn manage_rules(app: &mut SheetApp, ui: &mut egui::Ui) { - let list = app.session.run("home.manageRules", json!({})).unwrap_or(Json::Null); - let rules = list["rules"].as_array().cloned().unwrap_or_default(); - if rules.is_empty() { + // Read the rules in place: running `home.manageRules` for them each frame copied and + // serialised every rule and journaled a step 60 times a second. + let count = app.session.active().and_then(|d| d.wb.active()).map_or(0, |sh| sh.cond_formats.len()); + if count == 0 { ui.label("There are no conditional formatting rules on this sheet."); + return; } - for (i, r) in rules.iter().enumerate() { - ui.horizontal(|ui| { - let kind = r["rule"].as_object().and_then(|o| o.keys().next().cloned()).unwrap_or_default(); - ui.label(format!( - "{}. {kind} applies to {}", - i + 1, - r["ranges"].as_array().map(|a| a.iter().filter_map(|x| x.as_str()).collect::>().join(",")).unwrap_or_default() - )); - if ui.small_button("▲").clicked() { - app.run_or_alert("home.manageRules", json!({"moveUp": i})); - } - if ui.small_button("▼").clicked() { - app.run_or_alert("home.manageRules", json!({"moveDown": i})); - } - if ui.small_button("Delete").clicked() { - app.run_or_alert("home.manageRules", json!({"delete": i})); - } - }); + let mut action: Option = None; + let row_height = ui.spacing().interact_size.y; + egui::ScrollArea::vertical().max_height(LIST_HEIGHT).auto_shrink([false, true]).show_rows(ui, row_height, count, |ui, rows| { + let Some(sh) = app.session.active().and_then(|d| d.wb.active()) else { return }; + for i in rows { + let Some(cf) = sh.cond_formats.get(i) else { break }; + let kind = serde_json::to_value(&cf.rule).ok().and_then(|r| r.as_object().and_then(|o| o.keys().next().cloned())).unwrap_or_default(); + let ranges = cf.ranges.iter().map(|r| r.a1()).collect::>().join(","); + ui.horizontal(|ui| { + ui.label(format!("{}. {kind} applies to {ranges}", i + 1)); + if ui.small_button("▲").clicked() { + action = Some(json!({"moveUp": i})); + } + if ui.small_button("▼").clicked() { + action = Some(json!({"moveDown": i})); + } + if ui.small_button("Delete").clicked() { + action = Some(json!({"delete": i})); + } + }); + } + }); + if let Some(params) = action { + app.run_or_alert("home.manageRules", params); } } diff --git a/crates/ui-egui/tests/it/cf_rules_manager.rs b/crates/ui-egui/tests/it/cf_rules_manager.rs new file mode 100644 index 000000000..540b19403 --- /dev/null +++ b/crates/ui-egui/tests/it/cf_rules_manager.rs @@ -0,0 +1,64 @@ +//! Manage Rules on a sheet with thousands of conditional formats (issue #3321): the list scrolls +//! inside the dialog, and only the visible rules are laid out, instead of one row per rule +//! making the window as tall as the whole list and the application window go blank. + +use egui::vec2; +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>; + +const RULES: usize = 3000; +const SCREEN_H: f32 = 800.0; + +fn harness_with_rules(n: usize) -> Harness { + let mut session = Session::new(); + session.new_workbook(); + for i in 0..n { + let range = format!("A{}", i + 1); + session + .execute( + "home.conditionalFormat", + json!({"range": range, "rule": {"type": "cellIs", "operator": "greaterThan", "value": "5", "preset": "yellowFill"}}), + ) + .unwrap(); + } + let mut h = crate::common::builder().with_size(vec2(1200.0, SCREEN_H)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(ui.ctx()); + app.ui(ui); + }, + crate::common::app(session), + ); + h.run_steps(4); + h.state_mut().open_dialog("manageRules", json!({})); + h.run_steps(6); + h +} + +#[test] +fn many_rules_scroll_inside_a_dialog_that_fits_the_window() { + let mut h = harness_with_rules(RULES); + let deletes: Vec<_> = h.query_all_by_label("Delete").collect(); + assert!(!deletes.is_empty(), "the first rules are listed"); + // One more Delete sits in the ribbon. + assert!(deletes.len() < 100, "only the visible rules are laid out, got {}", deletes.len()); + let lowest = deletes.iter().map(|d| d.rect().bottom()).fold(0.0, f32::max); + assert!(lowest <= SCREEN_H, "the list stays inside the window, lowest button at {lowest}"); + drop(deletes); + // Deleting through the dialog removes the first rule and the dialog stays usable. + // (The ribbon has a Delete button of its own, outside the dialog's window.) + let window = h.get_by_label("Conditional Formatting Rules Manager").rect(); + h.query_all_by_label("Delete").find(|d| window.contains_rect(d.rect())).unwrap().click(); + h.run_steps(4); + let left = h.state().session.active().unwrap().wb.active().unwrap().cond_formats.len(); + assert_eq!(left, RULES - 1); +} + +#[test] +fn an_empty_sheet_says_so() { + let h = harness_with_rules(0); + h.get_by_label("There are no conditional formatting rules on this sheet."); +} diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index efc4fdc23..9cb395e5d 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -15,6 +15,7 @@ mod borders; mod cell_editor_caret; mod cell_pictures; mod cf_rule_pick; +mod cf_rules_manager; mod chart_data_table; mod chart_resize; mod chart_source_layering;