From 8c1c7cfd27a22b366ce2ddb94120165b5978147e Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 5 Nov 2026 00:47:47 +0100 Subject: [PATCH 1/4] 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; From 51e242e1fa1ad79764115cb7f3427d1e539930ec Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 5 Nov 2026 00:52:15 +0100 Subject: [PATCH 2/4] Manage Rules test: check the dialog rect and which rule was deleted Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/tests/it/cf_rules_manager.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/tests/it/cf_rules_manager.rs b/crates/ui-egui/tests/it/cf_rules_manager.rs index 540b19403..d6f19a715 100644 --- a/crates/ui-egui/tests/it/cf_rules_manager.rs +++ b/crates/ui-egui/tests/it/cf_rules_manager.rs @@ -48,13 +48,17 @@ fn many_rules_scroll_inside_a_dialog_that_fits_the_window() { 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); + let window = h.get_by_label("Conditional Formatting Rules Manager").rect(); + assert!(window.bottom() <= SCREEN_H, "the dialog fits the window, bottom at {}", window.bottom()); // 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); + let first = h.state().session.active().unwrap().wb.active().unwrap().cond_formats[0].ranges[0].a1(); + // New rules go first, so the one deleted was A3000. + assert_eq!(first, format!("A{}", RULES - 1), "the first rule was the one removed"); } #[test] From d7c0158f0a6de45660750d1387969b36369025ac Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sat, 7 Nov 2026 17:08:44 +0100 Subject: [PATCH 3/4] Manage Rules: let the rule list scroll sideways A long range list pushed the move and Delete buttons out of the vertical-only scroll area, where they could not be clicked. Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/dialogs/cond_format_rules.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ui-egui/src/dialogs/cond_format_rules.rs b/crates/ui-egui/src/dialogs/cond_format_rules.rs index 23af88aae..79df9897f 100644 --- a/crates/ui-egui/src/dialogs/cond_format_rules.rs +++ b/crates/ui-egui/src/dialogs/cond_format_rules.rs @@ -14,7 +14,7 @@ pub(super) fn manage_rules(app: &mut SheetApp, ui: &mut egui::Ui) { } 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| { + egui::ScrollArea::both().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 }; From 684f051f0da03c797ee39f89bda3708bf81f026b Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 12:51:27 +0100 Subject: [PATCH 4/4] Manage Rules: test reaching rules by scrolling down and sideways Scroll the 3,000-rule list to a row that was off screen and move and delete that rule through its own buttons; give one rule a label of 200 ranges whose buttons start outside the dialog, scroll sideways, then move and delete it. Each test checks the intended rule changed. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/tests/it/cf_rules_manager.rs | 117 +++++++++++++++++++- 1 file changed, 114 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/tests/it/cf_rules_manager.rs b/crates/ui-egui/tests/it/cf_rules_manager.rs index d6f19a715..013a0b28f 100644 --- a/crates/ui-egui/tests/it/cf_rules_manager.rs +++ b/crates/ui-egui/tests/it/cf_rules_manager.rs @@ -2,7 +2,7 @@ //! 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::{Event, Modifiers, MouseWheelSource, MouseWheelUnit, PointerButton, Pos2, Rect, TouchPhase, Vec2, pos2, vec2}; use egui_kittest::kittest::Queryable; use gridcraft_engine::Session; use gridcraft_ui_egui::SheetApp; @@ -14,10 +14,15 @@ const RULES: usize = 3000; const SCREEN_H: f32 = 800.0; fn harness_with_rules(n: usize) -> Harness { + let ranges: Vec = (0..n).map(|i| format!("A{}", i + 1)).collect(); + harness_with_ranges(&ranges) +} + +/// One rule per entry of `ranges`, added in order (so the last one is listed first). +fn harness_with_ranges(ranges: &[String]) -> Harness { let mut session = Session::new(); session.new_workbook(); - for i in 0..n { - let range = format!("A{}", i + 1); + for range in ranges { session .execute( "home.conditionalFormat", @@ -38,6 +43,67 @@ fn harness_with_rules(n: usize) -> Harness { h } +fn dialog(h: &Harness) -> Rect { + h.get_by_label("Conditional Formatting Rules Manager").rect() +} + +/// The rows of the list shown inside the dialog: (rule number, row rectangle). +fn visible_rows(h: &Harness) -> Vec<(usize, Rect)> { + let window = dialog(h); + let mut rows: Vec<(usize, Rect)> = h + .query_all_by_label_contains("applies to") + .filter(|n| window.contains(n.rect().left_center())) + .filter_map(|n| Some((n.value()?.split('.').next()?.parse().ok()?, n.rect()))) + .collect(); + rows.sort_by(|a, b| a.1.top().total_cmp(&b.1.top())); + rows +} + +/// The row of rule `n`, wherever it is laid out (its label may reach past the dialog's edges). +fn row_of(h: &Harness, n: usize) -> Rect { + let prefix = format!("{n}. "); + h.query_all_by_label_contains("applies to") + .find(|r| r.value().is_some_and(|l| l.starts_with(&prefix))) + .expect("the rule's row is laid out") + .rect() +} + +/// The `label` button on the row at `row`, if it is inside the dialog. +fn row_button(h: &Harness, row: Rect, label: &str) -> Option { + let window = dialog(h); + h.query_all_by_label(label).map(|n| n.rect()).find(|r| window.contains_rect(*r) && (r.center().y - row.center().y).abs() < row.height() / 2.0) +} + +fn wheel(h: &mut Harness, at: Pos2, delta: Vec2) { + h.input_mut().events.push(Event::PointerMoved(at)); + h.run_steps(2); + h.input_mut().events.push(Event::MouseWheel { + unit: MouseWheelUnit::Point, + delta, + phase: TouchPhase::Move, + source: MouseWheelSource::Unknown, + modifiers: Modifiers::NONE, + }); + h.run_steps(30); +} + +fn click(h: &mut Harness, r: Rect) { + let pos = r.center(); + for pressed in [true, false] { + h.input_mut() + .events + .extend([Event::PointerMoved(pos), Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }]); + h.step(); + } + h.run_steps(4); +} + +/// The ranges of every rule on the sheet, in list order. +fn rule_ranges(h: &Harness) -> Vec { + let sh = h.state().session.active().unwrap().wb.active().unwrap(); + sh.cond_formats.iter().map(|cf| cf.ranges.iter().map(|r| r.a1()).collect::>().join(",")).collect() +} + #[test] fn many_rules_scroll_inside_a_dialog_that_fits_the_window() { let mut h = harness_with_rules(RULES); @@ -66,3 +132,48 @@ 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."); } + +#[test] +fn a_rule_scrolled_into_view_is_moved_and_deleted() { + let mut h = harness_with_rules(RULES); + let before = visible_rows(&h); + let last_shown = before.iter().map(|r| r.0).max().expect("the first rules are listed"); + // Scroll the list down, then act on a row in the middle of what is shown now. + let at = pos2(dialog(&h).left() + 40.0, before[before.len() / 2].1.center().y); + wheel(&mut h, at, vec2(0.0, -2000.0)); + let rows = visible_rows(&h); + let (n, row) = rows[rows.len() / 2]; + assert!(n > last_shown, "rule {n} was not on screen before scrolling (rules 1 to {last_shown} were)"); + // Rule n is the n-th listed; new rules go first, so it applies to A{RULES + 1 - n}. + let target = format!("A{}", RULES + 1 - n); + assert_eq!(rule_ranges(&h)[n - 1], target); + let down = row_button(&h, row, "▼").expect("the row's move-down button is in the dialog"); + click(&mut h, down); + let ranges = rule_ranges(&h); + assert_eq!(ranges[n], target, "rule {n} moved down one place"); + assert_eq!(ranges[n - 1], format!("A{}", RULES - n), "the rule below it moved up"); + // The moved rule is now rule n + 1, on the next row. + let delete = row_button(&h, row_of(&h, n + 1), "Delete").expect("the row's Delete button is in the dialog"); + click(&mut h, delete); + let ranges = rule_ranges(&h); + assert_eq!(ranges.len(), RULES - 1); + assert!(!ranges.contains(&target), "the rule scrolled to was deleted"); +} + +#[test] +fn a_rule_with_a_long_label_is_reached_by_scrolling_sideways() { + let long: Vec = (0..200).map(|i| format!("C{}", 2 * i + 1)).collect(); + let long = long.join(","); + let mut h = harness_with_ranges(&["A1".into(), long.clone(), "B1".into()]); + assert_eq!(rule_ranges(&h), ["B1", long.as_str(), "A1"]); + let row = row_of(&h, 2); + assert!(row_button(&h, row, "▼").is_none(), "the long rule's buttons start out of view"); + let window = dialog(&h); + wheel(&mut h, pos2(window.left() + 40.0, row.center().y), vec2(-20000.0, 0.0)); + let down = row_button(&h, row_of(&h, 2), "▼").expect("scrolling sideways brings the move-down button into the dialog"); + click(&mut h, down); + assert_eq!(rule_ranges(&h), ["B1", "A1", long.as_str()], "the long rule moved down"); + let delete = row_button(&h, row_of(&h, 3), "Delete").expect("the long rule's Delete button is in the dialog on its new row"); + click(&mut h, delete); + assert_eq!(rule_ranges(&h), ["B1", "A1"], "the long rule was deleted"); +}