From 151a279da835ded9150ac278ae4c1cb790be357a Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 22:58:21 -0400 Subject: [PATCH 01/22] Conditional formats: hex colours in a custom style, no silently empty rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit home.conditionalFormat deserialized `style` straight into the model's Style, so a programmatic caller had to write colours as {"Rgb": 16773836}: "#FFF2CC", which format_range, colorScale and dataBar take, was refused with a serde message naming internal types. A key the struct doesn't have (`style: {preset: …}`, `fillColor`) was dropped, which saved a rule with an empty format that matches and shows nothing. And a cellIs `value` already written as a string literal ("\"Closed\"", as in the file and openpyxl) was quoted again into the text "Closed" with its quotes, so it never matched. `style` colours (font, fill fg/bg, borders) may now be "#RRGGBB", {theme, tint} or "auto", and `fill` may be a colour alone (a solid fill); a fill colour with no pattern is solid. Unknown keys and a non-object `style` are refused. A cellIs value that is one string literal is kept. The model's own shape keeps working. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/insert.rs | 69 +++++++++++++++++++++++++++++++-- crates/engine/src/tests.rs | 58 +++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 392c5eebf..0d58b0da5 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -179,7 +179,7 @@ pub fn specs() -> Vec { "Conditional Formatting", ["Home", "Styles"], None, - "{range?, rule: {type: cellIs|expression|containsText|beginsWith|endsWith|blanks|noBlanks|errors|noErrors|duplicate|unique|top10|aboveAverage|timePeriod|colorScale|dataBar|iconSet, operator?, value?, value2?, formula?, text?, rank?, bottom?, percent?, below?, period?, colors?: [..], color?, set?, style?: {font..fill..}|preset: lightRedFill|redText|yellowFill|greenFill|redBorder|lightRedFillDarkRedText…}}", + "{range?, rule: {type: cellIs|expression|containsText|beginsWith|endsWith|blanks|noBlanks|errors|noErrors|duplicate|unique|top10|aboveAverage|timePeriod|colorScale|dataBar|iconSet, operator?, value?, value2?, formula?, text?, rank?, bottom?, percent?, below?, period?, colors?: [..], color?, set?, style?: {font?: {bold?, italic?, color?}, fill?: \"#FFC7CE\" | {pattern?, fg?, bg?}, borders?, num_fmt?} (colours \"#RRGGBB\") | preset?: lightRedFill|redText|yellowFill|greenFill|redBorder|lightRedFillDarkRedText…}; a text `value` is matched as text, or kept when already a quoted literal}", has_doc, add_cf ), @@ -1266,6 +1266,64 @@ fn preset_style(name: &str) -> Style { s } +/// Whether `v` is one formula string literal: `"…"` with every inner quote doubled. +fn is_string_literal(v: &str) -> bool { + v.len() >= 2 && v.starts_with('"') && v.ends_with('"') && v.get(1..v.len() - 1).is_some_and(|inner| !inner.replace("\"\"", "").contains('"')) +} + +/// A conditional format's `style` as the model's `Style` JSON (`font`, `fill`, `borders`, `align`, +/// `num_fmt`, `protection`, merged over the default by the caller), where a colour may also be +/// written as `format_range` takes it (`"#RRGGBB"`, `{theme, tint}`, `"auto"`) and `fill` may be a +/// colour alone (a solid fill). An unknown key is an error rather than dropped: dropped, it left a +/// rule with an empty format that matches and shows nothing. +fn cf_style_json(st: &Json) -> Result { + const KEYS: [&str; 6] = ["font", "fill", "borders", "align", "num_fmt", "protection"]; + let err = |m: String| bad("home.conditionalFormat", m); + let Some(obj) = st.as_object() else { return Err(err("`style` must be an object, e.g. {\"fill\": \"#FFC7CE\"}".into())) }; + let mut st = obj.clone(); + if let Some(k) = st.keys().find(|k| !KEYS.contains(&k.as_str())) { + let hint = if k == "preset" { "; a preset goes in `rule.preset`" } else { "" }; + return Err(err(format!("unknown key `{k}` in `style` (known: {}){hint}", KEYS.join(", ")))); + } + // A colour written the way the other formatting commands take it, as the model stores it. + let color = |v: &mut Json, what: &str| -> Result<()> { + if v.is_string() || v.get("theme").is_some() { + let c = super::format::color_param(Some(v)).ok_or_else(|| err(format!("invalid colour {v} for `{what}`")))?; + *v = serde_json::to_value(c).map_err(|e| EngineError::Other(e.to_string()))?; + } + Ok(()) + }; + if let Some(f) = st.get_mut("font").and_then(Json::as_object_mut) + && let Some(c) = f.get_mut("color") + { + color(c, "font.color")?; + } + if let Some(fill) = st.get_mut("fill") { + if fill.is_string() || fill.get("theme").is_some() { + *fill = json!({"pattern": "Solid", "fg": fill.clone()}); + } + if let Some(f) = fill.as_object_mut() { + for k in ["fg", "bg"] { + if let Some(c) = f.get_mut(k) { + color(c, &format!("fill.{k}"))?; + } + } + // A colour with no pattern is a solid fill, not an invisible one. + if f.contains_key("fg") && !f.contains_key("pattern") { + f.insert("pattern".into(), json!("Solid")); + } + } + } + if let Some(b) = st.get_mut("borders").and_then(Json::as_object_mut) { + for (side, line) in b.iter_mut() { + if let Some(c) = line.get_mut("color") { + color(c, &format!("borders.{side}.color"))?; + } + } + } + Ok(Json::Object(st)) +} + fn add_cf(s: &mut Session, p: &Json) -> Result { let ranges = target_ranges(s, p)?; let sheet = target_sheet(s, p)?; @@ -1275,9 +1333,10 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { }; let g = |k: &str| rule.get(k).and_then(Json::as_str).map(str::to_string); let style = Box::new(match rule.get("style") { - Some(st) if st.is_object() => { + Some(st) if !st.is_null() => { + let st = cf_style_json(st)?; let base = serde_json::to_value(Style::default()).map_err(|e| EngineError::Other(e.to_string()))?; - serde_json::from_value(super::format::merge_json(base, st)).map_err(|e| bad("home.conditionalFormat", e.to_string()))? + serde_json::from_value(super::format::merge_json(base, &st)).map_err(|e| bad("home.conditionalFormat", e.to_string()))? } _ => preset_style(g("preset").as_deref().unwrap_or("lightRedFillDarkRedText")), }); @@ -1316,8 +1375,12 @@ fn add_cf(s: &mut Session, p: &Json) -> Result { let a = rule.get("value").map(super::edit::json_to_input).unwrap_or_default(); let b = rule.get("value2").map(super::edit::json_to_input); let quote = |v: String| { + // A number, a formula, or text already written as a string literal (`"Closed"`, as + // in the file and in openpyxl's `CellIsRule`) is kept; other text is quoted. if gridcraft_core::parse::parse_number_text(&v).is_some() || v.starts_with('=') { v.trim_start_matches('=').to_string() + } else if is_string_literal(&v) { + v } else { format!("\"{}\"", v.replace('"', "\"\"")) } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 33fdc113e..7b3fff840 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2612,6 +2612,64 @@ fn icon_set_thresholds_from_files_pick_the_icon() { assert_eq!([icon(&s, "A1"), icon(&s, "A2"), icon(&s, "A3"), icon(&s, "A4")], [Some(0), Some(1), Some(1), Some(2)]); } +/// `home.conditionalFormat` styles take colours the way the other formatting commands do, refuse +/// keys they would drop, and keep a text criterion already written as a string literal. +#[test] +fn conditional_format_styles_take_hex_colours_and_quoted_values() { + use gridcraft_model::{CfRule, Color, Fill, PatternType}; + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Open"], ["Closed"], ["Blocked"]]})).unwrap(); + let rule = |s: &Session, i: usize| s.doc().unwrap().wb.active().unwrap().cond_formats[i].rule.clone(); + let style = |r: &CfRule| match r { + CfRule::Expression { style, .. } | CfRule::CellIs { style, .. } => (**style).clone(), + other => panic!("{other:?}"), + }; + s.execute( + "home.conditionalFormat", + json!({"range": "A1:A3", "rule": {"type": "expression", "formula": "=A1=\"Open\"", "style": {"fill": "#FFF2CC", "font": {"bold": true, "color": "#9C0006"}}}}), + ) + .unwrap(); + let st = style(&rule(&s, 0)); + assert_eq!(st.fill, Fill::solid(Color::Rgb(0xFFF2CC))); + assert_eq!((st.font.bold, st.font.color), (true, Color::Rgb(0x9C0006))); + // A fill colour without a pattern is solid; the model's own shape still works. + s.execute( + "home.conditionalFormat", + json!({"range": "A1", "rule": {"type": "expression", "formula": "=TRUE", "style": {"fill": {"fg": "C6EFCE"}}}}), + ) + .unwrap(); + assert_eq!(style(&rule(&s, 0)).fill, Fill { pattern: PatternType::Solid, fg: Color::Rgb(0xC6EFCE), bg: Color::Auto }); + let internal = json!({"fill": {"pattern": "Solid", "fg": {"Rgb": 16773836}}, "borders": {"left": {"style": "Thin", "color": "#C00000"}}}); + s.execute("home.conditionalFormat", json!({"range": "A1", "rule": {"type": "expression", "formula": "=TRUE", "style": internal}})).unwrap(); + let st = style(&rule(&s, 0)); + assert_eq!((st.fill.fg, st.borders.left.color), (Color::Rgb(0xFFF2CC), Color::Rgb(0xC00000))); + // Keys that would be dropped, leaving a rule that shows nothing, are refused. + let n = s.doc().unwrap().wb.active().unwrap().cond_formats.len(); + for bad in [json!({"preset": "greenFill"}), json!({"fillColor": "#FFF2CC"}), json!({"fill": "#nothex"}), json!("bold")] { + let e = s.execute("home.conditionalFormat", json!({"range": "A1", "rule": {"type": "expression", "formula": "=TRUE", "style": bad}})); + assert!(e.is_err(), "{bad} should be refused"); + } + assert_eq!(s.doc().unwrap().wb.active().unwrap().cond_formats.len(), n); + + // `"Closed"` (a string literal, as in the file) and `Closed` both match the text Closed. + for value in ["\"Closed\"", "Closed"] { + s.execute( + "home.conditionalFormat", + json!({"range": "A1:A3", "rule": {"type": "cellIs", "operator": "equal", "value": value, "preset": "greenFill"}}), + ) + .unwrap(); + assert!(matches!(rule(&s, 0), CfRule::CellIs { ref a, .. } if a == "\"Closed\""), "{value}"); + let wb = s.doc().unwrap().wb.clone(); + let mut cache = crate::cf::CfCache::new(); + let mut fill = |c: &str| cache.look(&wb, 0, CellRef::parse(c).unwrap()).and_then(|l| l.style).map(|st| st.fill.fg); + assert_eq!(fill("A2"), Some(Color::Rgb(0xC6EFCE)), "{value}"); + s.execute("edit.undo", json!({})).unwrap(); + } + // Text with quotes inside that isn't one literal is still quoted as text. + s.execute("home.conditionalFormat", json!({"range": "A1", "rule": {"type": "cellIs", "operator": "equal", "value": "say \"hi\""}})).unwrap(); + assert!(matches!(rule(&s, 0), CfRule::CellIs { ref a, .. } if a == "\"say \"\"hi\"\"\"")); +} + #[test] fn row_height_and_column_width_take_a_list_of_areas() { // Issue #341: header drags on a multi-area selection size every selected line in one step. From 2a580f2902cd2d36c3c7c9b4acb10561a6e1b1ba Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 23:34:14 -0400 Subject: [PATCH 02/22] Conditional formats: a style fill of "none" means no fill `style: {fill: "none"}` (or "auto") went through the colour pass and came out as a solid fill in the automatic colour. `home.fillColor` reads the same word as no fill, so the rule style does too now. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/insert.rs | 9 ++++++--- crates/engine/src/tests.rs | 8 ++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 0d58b0da5..985bb2a8a 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -1274,8 +1274,8 @@ fn is_string_literal(v: &str) -> bool { /// A conditional format's `style` as the model's `Style` JSON (`font`, `fill`, `borders`, `align`, /// `num_fmt`, `protection`, merged over the default by the caller), where a colour may also be /// written as `format_range` takes it (`"#RRGGBB"`, `{theme, tint}`, `"auto"`) and `fill` may be a -/// colour alone (a solid fill). An unknown key is an error rather than dropped: dropped, it left a -/// rule with an empty format that matches and shows nothing. +/// colour alone (a solid fill) or `"none"` (no fill). An unknown key is an error rather than +/// dropped: dropped, it left a rule with an empty format that matches and shows nothing. fn cf_style_json(st: &Json) -> Result { const KEYS: [&str; 6] = ["font", "fill", "borders", "align", "num_fmt", "protection"]; let err = |m: String| bad("home.conditionalFormat", m); @@ -1299,7 +1299,10 @@ fn cf_style_json(st: &Json) -> Result { color(c, "font.color")?; } if let Some(fill) = st.get_mut("fill") { - if fill.is_string() || fill.get("theme").is_some() { + // `"none"` (or `"auto"`) is no fill, as `home.fillColor` reads it, not a solid automatic one. + if fill.is_string() && super::format::color_param(Some(fill)) == Some(Color::Auto) { + *fill = serde_json::to_value(Fill::default()).map_err(|e| EngineError::Other(e.to_string()))?; + } else if fill.is_string() || fill.get("theme").is_some() { *fill = json!({"pattern": "Solid", "fg": fill.clone()}); } if let Some(f) = fill.as_object_mut() { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 7b3fff840..1af74e67e 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2643,6 +2643,14 @@ fn conditional_format_styles_take_hex_colours_and_quoted_values() { s.execute("home.conditionalFormat", json!({"range": "A1", "rule": {"type": "expression", "formula": "=TRUE", "style": internal}})).unwrap(); let st = style(&rule(&s, 0)); assert_eq!((st.fill.fg, st.borders.left.color), (Color::Rgb(0xFFF2CC), Color::Rgb(0xC00000))); + // `"none"` is no fill (the format only sets the font), as `home.fillColor` reads it. + s.execute( + "home.conditionalFormat", + json!({"range": "A1", "rule": {"type": "expression", "formula": "=TRUE", "style": {"fill": "none", "font": {"bold": true}}}}), + ) + .unwrap(); + let st = style(&rule(&s, 0)); + assert_eq!((st.fill, st.font.bold), (Fill::default(), true)); // Keys that would be dropped, leaving a rule that shows nothing, are refused. let n = s.doc().unwrap().wb.active().unwrap().cond_formats.len(); for bad in [json!({"preset": "greenFill"}), json!({"fillColor": "#FFF2CC"}), json!({"fill": "#nothex"}), json!("bold")] { From 4ec6a373b015d1456855b97800cbf4d7823f3959 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Tue, 13 Oct 2026 10:35:59 +0200 Subject: [PATCH 03/22] Ribbon: Font Size is a box you type into, the sizes behind its arrow (fixes #932) --- crates/ui-egui/src/ribbon.rs | 109 ++++++++++++++-------- crates/ui-egui/tests/font_size_box.rs | 125 ++++++++++++++++++++++++++ crates/ui-egui/tests/keytips.rs | 5 +- 3 files changed, 203 insertions(+), 36 deletions(-) create mode 100644 crates/ui-egui/tests/font_size_box.rs diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 9c6d89eeb..99b3b01e2 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -380,8 +380,6 @@ pub(crate) enum ComboResult { None, /// A row was applied: clicked or Enter on the highlighted row. Picked(String), - /// Enter with free text that matches no row (e.g. a custom font size). - Entered(String), } /// Excel-style dropdown: a filter field (type to narrow the list) plus arrow/Enter selection. @@ -394,6 +392,24 @@ pub(crate) fn editable_combo( current_value: &str, items: &[(&str, String)], forced: bool, +) -> ComboResult { + combo_popup(ui, anchor, salt, current_value, items, forced, true) +} + +/// The dropdown of [`editable_combo`] without the filter field: the whole list, picked by click or +/// arrows and Enter. +fn list_combo(ui: &mut Ui, anchor: &egui::Response, salt: &'static str, current_value: &str, items: &[(&str, String)]) -> ComboResult { + combo_popup(ui, anchor, salt, current_value, items, false, false) +} + +fn combo_popup( + ui: &mut Ui, + anchor: &egui::Response, + salt: &'static str, + current_value: &str, + items: &[(&str, String)], + forced: bool, + filter: bool, ) -> ComboResult { let popup_id = anchor.id.with("popup"); let state_id = anchor.id.with(salt).with("__combo_state"); @@ -424,15 +440,17 @@ pub(crate) fn editable_combo( }); }); } - let text_id = anchor.id.with(salt).with("__combo_text"); - let te = ui.add(egui::TextEdit::singleline(&mut st.text).id(text_id).desired_width(220.0).hint_text("Type to filter…")); - if open_now && !te.has_focus() { - ui.ctx().memory_mut(|m| m.request_focus(text_id)); - // Select all once focus lands so typing replaces the filter. - if let Some(mut s) = egui::text_edit::TextEditState::load(ui.ctx(), text_id) { - let len = st.text.chars().count(); - s.cursor.set_char_range(Some(egui::text::CCursorRange::two(egui::text::CCursor::new(0), egui::text::CCursor::new(len)))); - s.store(ui.ctx(), text_id); + if filter { + let text_id = anchor.id.with(salt).with("__combo_text"); + let te = ui.add(egui::TextEdit::singleline(&mut st.text).id(text_id).desired_width(220.0).hint_text("Type to filter…")); + if open_now && !te.has_focus() { + ui.ctx().memory_mut(|m| m.request_focus(text_id)); + // Select all once focus lands so typing replaces the filter. + if let Some(mut s) = egui::text_edit::TextEditState::load(ui.ctx(), text_id) { + let len = st.text.chars().count(); + s.cursor.set_char_range(Some(egui::text::CCursorRange::two(egui::text::CCursor::new(0), egui::text::CCursor::new(len)))); + s.store(ui.ctx(), text_id); + } } } let lower = st.text.to_lowercase(); @@ -446,18 +464,13 @@ pub(crate) fn editable_combo( st.sel = (st.sel - 1).rem_euclid(n); } } - if enter { - if filtered.is_empty() { - let t = st.text.trim().to_string(); - if !t.is_empty() { - result = ComboResult::Entered(t); - } - } else { - let idx = filtered[st.sel.clamp(0, filtered.len() as i32 - 1) as usize]; - result = ComboResult::Picked(items[idx].0.to_string()); - } + if enter && !filtered.is_empty() { + let idx = filtered[st.sel.clamp(0, filtered.len() as i32 - 1) as usize]; + result = ComboResult::Picked(items[idx].0.to_string()); + } + if filter { + ui.separator(); } - ui.separator(); egui::ScrollArea::vertical().max_height(240.0).show(ui, |ui| { for i in &filtered { let (value, disp) = &items[*i]; @@ -1092,25 +1105,51 @@ pub const FONTS: &[&str] = &[ "Verdana", ]; +/// Excel's size box: type a size and press Enter to apply exactly that number (the command refuses +/// sizes outside 1 to 409), Escape or clicking away reverts; the arrow opens the standard sizes. fn size_combo(app: &mut SheetApp, ui: &mut Ui, st: &Style) { + let t = Tokens::get(ui.ctx()); let size = st.font.size; - let label = if size.fract() == 0.0 { format!("{}", size as i32) } else { format!("{size}") }; - let resp = ui.add_sized([52.0, 24.0], egui::Button::new(egui::RichText::new(&label).font(theme::ui_font(12.5)))); + let current = if size.fract() == 0.0 { format!("{}", size as i32) } else { format!("{size}") }; + let field = egui::Id::new("gridcraft.font_size_box"); + let draft = field.with("draft"); + let mut text = ui.data_mut(|d| d.get_temp::(draft)).unwrap_or_else(|| current.clone()); + let gap = std::mem::replace(&mut ui.spacing_mut().item_spacing.x, 0.0); + let out = egui::TextEdit::singleline(&mut text).id(field).font(theme::ui_font(12.5)).desired_width(30.0).show(ui); + let resp = out.response; control_keytip(app, ui, &resp, "home.fontSize"); - let forced = invoke(app, "home.fontSize"); + if invoke(app, "home.fontSize") { + ui.memory_mut(|m| m.request_focus(field)); + } + if resp.gained_focus() { + // Typing replaces the size, as in Excel, whether the box was clicked or reached by keytip. + let mut state = out.state; + state.cursor.set_char_range(Some(egui::text::CCursorRange::two(egui::text::CCursor::new(0), egui::text::CCursor::new(text.chars().count())))); + state.store(ui.ctx(), field); + } + if resp.lost_focus() { + ui.data_mut(|d| d.remove::(draft)); + // Consume the Enter so the grid behind does not also move the active cell down. + if ui.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Enter)) + && let Some(v) = text.trim().parse::().ok().filter(|v| v.is_finite()) + { + act(app, "home.fontSize", json!({"size": v})); + } + } else if resp.has_focus() { + ui.data_mut(|d| d.insert_temp(draft, text)); + } else { + ui.data_mut(|d| d.remove::(draft)); + } + let arrow = icon_button(ui, Icon::Chevron, t.text_dim, "Font Size", vec2(14.0, 24.0)); + ui.spacing_mut().item_spacing.x = gap; + arrow.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, ui.is_enabled(), "Font Size")); let sizes = [8.0, 9.0, 10.0, 11.0, 12.0, 14.0, 16.0, 18.0, 20.0, 22.0, 24.0, 26.0, 28.0, 36.0, 48.0, 72.0]; let labels: Vec = sizes.iter().map(|s| format!("{}", *s as i32)).collect(); let items: Vec<(&str, String)> = labels.iter().map(|s| (s.as_str(), s.clone())).collect(); - let current = if size.fract() == 0.0 { format!("{}", size as i32) } else { format!("{size}") }; - let mut apply = |s: &str| { - if let Ok(v) = s.parse::() { - act(app, "home.fontSize", json!({"size": v})); - } - }; - match editable_combo(ui, &resp, "size", ¤t, &items, forced) { - ComboResult::Picked(value) => apply(&value), - ComboResult::Entered(text) => apply(&text), - _ => {} + if let ComboResult::Picked(value) = list_combo(ui, &arrow, "size", ¤t, &items) + && let Ok(v) = value.parse::() + { + act(app, "home.fontSize", json!({"size": v})); } } diff --git a/crates/ui-egui/tests/font_size_box.rs b/crates/ui-egui/tests/font_size_box.rs new file mode 100644 index 000000000..0337543ad --- /dev/null +++ b/crates/ui-egui/tests/font_size_box.rs @@ -0,0 +1,125 @@ +//! Home ▸ Font Size is a box you type a size into, with the standard sizes behind its arrow (#932). + +use egui::{Event, Key, Modifiers, accesskit::Role}; +use egui_kittest::kittest::Queryable as _; +use gridcraft_engine::Session; +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, SheetApp>; + +fn harness() -> Harness { + let mut s = Session::new(); + s.new_workbook(); + s.execute("selection.set", json!({"range": "A1:B2"})).unwrap(); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(ui.ctx()); + app.ui(ui); + }, + SheetApp::new(s, Default::default()), + ); + h.run_steps(4); + h +} + +fn key(h: &mut Harness, key: Key) { + h.input_mut().events.extend([ + Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE }, + Event::Key { key, physical_key: None, pressed: false, repeat: false, modifiers: Modifiers::NONE }, + ]); + h.run_steps(2); +} + +/// Alt, H, F, S focuses the size box, then `text` is typed and confirmed with Enter. +fn type_size(h: &mut Harness, text: &str) { + key(h, Key::F10); + for k in [Key::H, Key::F, Key::S] { + h.input_mut().events.push(Event::Key { key: k, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE }); + h.input_mut().events.push(Event::Text(k.name().to_ascii_lowercase())); + h.run_steps(2); + } + h.run_steps(3); + for c in text.chars() { + h.input_mut().events.push(Event::Text(c.to_string())); + h.run_steps(1); + } + key(h, Key::Enter); + h.run_steps(3); +} + +fn size(h: &Harness, cell: &str) -> f32 { + let d = h.state().session.active().unwrap(); + let sh = d.wb.active().unwrap(); + d.wb.styles.get(sh.style_id(CellRef::parse(cell).unwrap())).font.size +} + +#[test] +fn typed_size_is_applied_exactly_to_the_selection() { + for (typed, want) in [("6", 6.0), ("2", 2.0), ("10.5", 10.5)] { + let mut h = harness(); + type_size(&mut h, typed); + assert!(h.state().message.is_none(), "typing {typed} must not raise an error"); + for cell in ["A1", "B2"] { + assert_eq!(size(&h, cell), want, "typing {typed} + Enter sets {cell} to {want}"); + } + assert_eq!( + h.state().session.active().unwrap().wb.active().unwrap().value(CellRef::parse("A1").unwrap()), + Value::Empty, + "the digits go to the box, not the cell" + ); + } +} + +#[test] +fn out_of_range_or_not_a_number_changes_nothing() { + let mut h = harness(); + type_size(&mut h, "500"); + let (_, text) = h.state().message.clone().expect("500 is refused with a message"); + assert!(text.contains("between 1 and 409"), "{text}"); + assert_eq!(size(&h, "A1"), 11.0); + + let mut h = harness(); + type_size(&mut h, "abc"); + assert!(h.state().message.is_none()); + assert_eq!(size(&h, "A1"), 11.0, "text that is not a number reverts"); +} + +#[test] +fn escape_cancels_the_typed_size() { + let mut h = harness(); + key(&mut h, Key::F10); + for k in [Key::H, Key::F, Key::S] { + h.input_mut().events.push(Event::Key { key: k, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE }); + h.input_mut().events.push(Event::Text(k.name().to_ascii_lowercase())); + h.run_steps(2); + } + h.run_steps(3); + h.input_mut().events.push(Event::Text("6".into())); + h.run_steps(1); + key(&mut h, Key::Escape); + h.run_steps(3); + assert_eq!(size(&h, "A1"), 11.0); + assert!(h.get_all_by_role(Role::TextInput).any(|n| n.value().as_deref() == Some("11")), "the box shows the cell's size again"); +} + +#[test] +fn the_arrow_opens_the_plain_list_of_sizes() { + let mut h = harness(); + h.get_by_label("Font Size").click(); + h.run_steps(3); + assert!(h.query_by_label("Type to filter…").is_none(), "the size list has no filter field"); + // The grid's row headers carry the same numbers as labels: pick the list's button. + h.get_by_role_and_label(Role::Button, "16").click(); + h.run_steps(3); + assert_eq!(size(&h, "B2"), 16.0, "clicking a size applies it"); + + let mut h = harness(); + h.get_by_label("Font Size").click(); + h.run_steps(3); + key(&mut h, Key::ArrowDown); + key(&mut h, Key::Enter); + h.run_steps(3); + assert_eq!(size(&h, "A1"), 12.0, "Down from 11 then Enter picks 12"); +} diff --git a/crates/ui-egui/tests/keytips.rs b/crates/ui-egui/tests/keytips.rs index 24be742a5..1d96b32df 100644 --- a/crates/ui-egui/tests/keytips.rs +++ b/crates/ui-egui/tests/keytips.rs @@ -495,7 +495,10 @@ fn editable_combos_filter_by_typing_and_move_by_arrows() { key(&mut h, Key::Enter); h.run_steps(3); assert!(h.state().message.is_none() && h.state().toast.is_none(), "custom sizes must apply"); - assert_eq!(h.query_all_by_label("13.5").count(), 1, "the button must show the custom size"); + assert!( + h.get_all_by_role(egui::accesskit::Role::TextInput).any(|n| n.value().as_deref() == Some("13.5")), + "the size box must show the custom size" + ); // Closing without applying (Escape) and reopening must reset the filter to the full list. let mut h = harness(blank()); From 5a7fbfa5b8f4a2f697d82de8df966a36e13c6f96 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Tue, 13 Oct 2026 10:46:07 +0200 Subject: [PATCH 04/22] Contributors: add my real name --- contributors/people.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index d522b84e0..b92d6bb77 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -65,3 +65,7 @@ sync_github_name = true [people.Silvertree2010] display_name = "Netter" sync_github_name = false + +[people.marcoarnulfo] +real_name = "Marco Arnulfo" +sync_github_name = false From 4bcc5c87d175b449931f56c4bccea5b32665418e Mon Sep 17 00:00:00 2001 From: Richard Date: Tue, 13 Oct 2026 20:09:54 +1100 Subject: [PATCH 05/22] Ribbon: Font Size box matches the Font name box, list drops under it The size box is one frame with the Font name box's fill, border, height and corners, with the text field and the dropdown arrow inside it (focus shows the selection stroke). The size list drops directly below the box and is as wide as it, instead of opening 220 pt wide under the arrow. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Ut3hr3a13szpQdQ7gbYbfc --- crates/ui-egui/src/ribbon.rs | 46 +++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 99b3b01e2..d2d5fe101 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -393,13 +393,14 @@ pub(crate) fn editable_combo( items: &[(&str, String)], forced: bool, ) -> ComboResult { - combo_popup(ui, anchor, salt, current_value, items, forced, true) + combo_popup(ui, anchor, salt, current_value, items, forced, true, None) } /// The dropdown of [`editable_combo`] without the filter field: the whole list, picked by click or -/// arrows and Enter. -fn list_combo(ui: &mut Ui, anchor: &egui::Response, salt: &'static str, current_value: &str, items: &[(&str, String)]) -> ComboResult { - combo_popup(ui, anchor, salt, current_value, items, false, false) +/// arrows and Enter. It drops directly below `under` and is as wide as it, like Excel's Font Size +/// list under its box. +fn list_combo(ui: &mut Ui, anchor: &egui::Response, under: Rect, salt: &'static str, current_value: &str, items: &[(&str, String)]) -> ComboResult { + combo_popup(ui, anchor, salt, current_value, items, false, false, Some(under)) } fn combo_popup( @@ -410,6 +411,7 @@ fn combo_popup( items: &[(&str, String)], forced: bool, filter: bool, + under: Option, ) -> ComboResult { let popup_id = anchor.id.with("popup"); let state_id = anchor.id.with(salt).with("__combo_state"); @@ -424,7 +426,12 @@ fn combo_popup( } st.was_open = open_now; let mut result = ComboResult::None; - egui::Popup::menu(anchor).open_memory(popup_command(forced, anchor.clicked())).show(|ui| { + let mut popup = egui::Popup::menu(anchor).open_memory(popup_command(forced, anchor.clicked())); + if let Some(r) = under { + popup = popup.anchor(r).align(egui::RectAlign::BOTTOM_START).width(r.width()); + } + let row_w = under.map_or(220.0, |r| r.width() - 12.0); + popup.show(|ui| { // Arrows and Enter navigate the list, not the filter field's cursor. Capture them and // strip them from the input so the TextEdit never sees them. let (mut down, mut up, mut enter) = (false, false, false); @@ -474,7 +481,7 @@ fn combo_popup( egui::ScrollArea::vertical().max_height(240.0).show(ui, |ui| { for i in &filtered { let (value, disp) = &items[*i]; - let row = ui.add(egui::Button::selectable(*i as i32 == st.sel, egui::RichText::new(disp)).min_size(vec2(220.0, 20.0))); + let row = ui.add(egui::Button::selectable(*i as i32 == st.sel, egui::RichText::new(disp)).min_size(vec2(row_w, 20.0))); if row.clicked() { result = ComboResult::Picked(value.to_string()); } @@ -1114,8 +1121,21 @@ fn size_combo(app: &mut SheetApp, ui: &mut Ui, st: &Style) { let field = egui::Id::new("gridcraft.font_size_box"); let draft = field.with("draft"); let mut text = ui.data_mut(|d| d.get_temp::(draft)).unwrap_or_else(|| current.clone()); - let gap = std::mem::replace(&mut ui.spacing_mut().item_spacing.x, 0.0); - let out = egui::TextEdit::singleline(&mut text).id(field).font(theme::ui_font(12.5)).desired_width(30.0).show(ui); + // One frame like the Font name box beside it (same fill, border, height and corners), with the + // text field and the dropdown arrow inside it, as in Excel. + let (frame, _) = ui.allocate_exact_size(vec2(52.0, 24.0), egui::Sense::hover()); + // Painted once the field's focus and hover are known, but beneath the field and the arrow. + let bg = ui.painter().add(egui::Shape::Noop); + let text_rect = Rect::from_min_max(frame.min, egui::pos2(frame.right() - 16.0, frame.bottom())); + let arrow_rect = Rect::from_min_max(egui::pos2(frame.right() - 16.0, frame.top() + 1.0), egui::pos2(frame.right() - 1.0, frame.bottom() - 1.0)); + let mut field_ui = + ui.new_child(egui::UiBuilder::new().max_rect(text_rect.shrink2(vec2(6.0, 0.0))).layout(egui::Layout::left_to_right(egui::Align::Center))); + let out = egui::TextEdit::singleline(&mut text) + .id(field) + .font(theme::ui_font(12.5)) + .frame(egui::Frame::NONE) + .desired_width(text_rect.width() - 12.0) + .show(&mut field_ui); let resp = out.response; control_keytip(app, ui, &resp, "home.fontSize"); if invoke(app, "home.fontSize") { @@ -1140,13 +1160,17 @@ fn size_combo(app: &mut SheetApp, ui: &mut Ui, st: &Style) { } else { ui.data_mut(|d| d.remove::(draft)); } - let arrow = icon_button(ui, Icon::Chevron, t.text_dim, "Font Size", vec2(14.0, 24.0)); - ui.spacing_mut().item_spacing.x = gap; + let mut arrow_ui = ui.new_child(egui::UiBuilder::new().max_rect(arrow_rect)); + let arrow = icon_button(&mut arrow_ui, Icon::Chevron, t.text_dim, "Font Size", arrow_rect.size()); + let visuals = ui.style().visuals.clone(); + let looks = if resp.has_focus() || ui.rect_contains_pointer(frame) { &visuals.widgets.hovered } else { &visuals.widgets.inactive }; + let stroke = if resp.has_focus() { visuals.selection.stroke } else { looks.bg_stroke }; + ui.painter().set(bg, egui::epaint::RectShape::new(frame, looks.corner_radius, looks.weak_bg_fill, stroke, egui::StrokeKind::Inside)); arrow.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, ui.is_enabled(), "Font Size")); let sizes = [8.0, 9.0, 10.0, 11.0, 12.0, 14.0, 16.0, 18.0, 20.0, 22.0, 24.0, 26.0, 28.0, 36.0, 48.0, 72.0]; let labels: Vec = sizes.iter().map(|s| format!("{}", *s as i32)).collect(); let items: Vec<(&str, String)> = labels.iter().map(|s| (s.as_str(), s.clone())).collect(); - if let ComboResult::Picked(value) = list_combo(ui, &arrow, "size", ¤t, &items) + if let ComboResult::Picked(value) = list_combo(ui, &arrow, frame, "size", ¤t, &items) && let Ok(v) = value.parse::() { act(app, "home.fontSize", json!({"size": v})); From 1539500b7fa0b3b46f96194e6a2823bcdb8e2df2 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Tue, 13 Oct 2026 20:11:02 +0200 Subject: [PATCH 06/22] Conditional formats: statistics only for rules that read them, borrowed not cloned per cell (#1098) Co-Authored-By: Claude Sonnet 5.5 --- ROADMAP.md | 4 +- crates/engine/src/cf.rs | 179 +++++++++++++++++++++++++++++++++++----- 2 files changed, 161 insertions(+), 22 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..516a54f90 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-13 · **Change:** minor (conditional formatting no longer scans its whole range for rules that read no statistics) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-13 | Conditional formatting: a rule gathers range statistics only when it reads them (top/bottom, above average, duplicates, scales, bars, icons) and borrows them instead of copying them per cell, so the per-frame lookup of an expression or cell-value rule no longer visits its whole range (#1098, part 1) | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-13 | minor | Progress log: conditional-format lookups no longer scan the range for rules without statistics (#1098) | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/crates/engine/src/cf.rs b/crates/engine/src/cf.rs index b71ddb8ca..1b0a1fda7 100644 --- a/crates/engine/src/cf.rs +++ b/crates/engine/src/cf.rs @@ -34,7 +34,43 @@ struct RuleStats { counts: HashMap, } -fn numbers_in(sh: &Sheet, ranges: &[RangeRef]) -> (Vec, HashMap) { +/// Which statistics a rule reads. Scanning the rule's whole range for them costs O(range), so +/// rules that read none (cell value, formula, text, blanks, errors, dates) never scan. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum StatsNeed { + /// Sorted numbers, their mean and deviation (top/bottom N, above average, scales, bars, icons). + Numbers, + /// How often each displayed text occurs (duplicate / unique values). + Counts, +} + +fn stats_need(rule: &CfRule) -> Option { + match rule { + CfRule::Duplicate { .. } | CfRule::Unique { .. } => Some(StatsNeed::Counts), + CfRule::Top10 { .. } | CfRule::AboveAverage { .. } | CfRule::ColorScale { .. } | CfRule::DataBar { .. } | CfRule::IconSet { .. } => { + Some(StatsNeed::Numbers) + } + _ => None, + } +} + +impl RuleStats { + fn build(sh: &Sheet, ranges: &[RangeRef], need: StatsNeed) -> RuleStats { + let (sorted, counts) = numbers_in(sh, ranges, need); + let n = sorted.len().max(1) as f64; + let avg = sorted.iter().sum::() / n; + let std = (sorted.iter().map(|x| (x - avg).powi(2)).sum::() / n).sqrt(); + RuleStats { sorted, avg, std, counts } + } +} + +#[cfg(test)] +thread_local! { + /// Cells `numbers_in` visited on this thread (tests count the work a lookup does). + static SCANNED: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +fn numbers_in(sh: &Sheet, ranges: &[RangeRef], need: StatsNeed) -> (Vec, HashMap) { let mut nums = Vec::new(); let mut counts = HashMap::new(); for r in ranges { @@ -43,12 +79,20 @@ fn numbers_in(sh: &Sheet, ranges: &[RangeRef]) -> (Vec, HashMap continue, }; for c in r.iter().take(2_000_000) { + #[cfg(test)] + SCANNED.with(|n| n.set(n.get() + 1)); let v = sh.value(c); - if let Value::Number(n) = v { - nums.push(n); - } - if !v.is_empty() { - *counts.entry(v.display().to_lowercase()).or_insert(0) += 1; + match need { + StatsNeed::Numbers => { + if let Value::Number(n) = v { + nums.push(n); + } + } + StatsNeed::Counts => { + if !v.is_empty() { + *counts.entry(v.display().to_lowercase()).or_insert(0) += 1; + } + } } } } @@ -109,19 +153,13 @@ impl CfCache { sh.cond_formats.iter().enumerate().filter(|(_, cf)| cf.ranges.iter().any(|r| r.contains(c))).collect(); rules.sort_by_key(|(_, cf)| cf.priority); let v = sh.value(c); + let no_stats = RuleStats::default(); for (idx, cf) in rules { let first = cf.ranges.first().map(|r| r.start).unwrap_or(c); - let st = self - .stats - .entry(idx) - .or_insert_with(|| { - let (sorted, counts) = numbers_in(sh, &cf.ranges); - let n = sorted.len().max(1) as f64; - let avg = sorted.iter().sum::() / n; - let std = (sorted.iter().map(|x| (x - avg).powi(2)).sum::() / n).sqrt(); - RuleStats { sorted, avg, std, counts } - }) - .clone(); + let st: &RuleStats = match stats_need(&cf.rule) { + Some(need) => &*self.stats.entry(idx).or_insert_with(|| RuleStats::build(sh, &cf.ranges, need)), + None => &no_stats, + }; let text = v.display(); let matched_style: Option<&Style> = match &cf.rule { CfRule::CellIs { op, a, b, style } => { @@ -218,7 +256,7 @@ impl CfCache { if let Some(n) = v.as_f64() { let pts: Vec<(f64, [u8; 3])> = stops .iter() - .map(|(k, col)| (threshold(wb, sheet, c, &st, k), col.resolve(&wb.theme).unwrap_or([255, 255, 255]))) + .map(|(k, col)| (threshold(wb, sheet, c, st, k), col.resolve(&wb.theme).unwrap_or([255, 255, 255]))) .collect(); let rgb = match pts.as_slice() { [(a, ca), (b, cb)] => lerp(*ca, *cb, if b > a { (n - a) / (b - a) } else { 0.0 }), @@ -240,8 +278,8 @@ impl CfCache { } CfRule::DataBar { min, max, color, gradient, show_value } => { if let Some(n) = v.as_f64() { - let lo = threshold(wb, sheet, c, &st, min); - let hi = threshold(wb, sheet, c, &st, max); + let lo = threshold(wb, sheet, c, st, min); + let hi = threshold(wb, sheet, c, st, max); let lo = if lo > 0.0 { 0.0 } else { lo }; let frac = if hi > lo { ((n - lo) / (hi - lo)).clamp(0.0, 1.0) } else { 1.0 }; if out.bar.is_none() { @@ -270,7 +308,7 @@ impl CfCache { defaults.get(1..).unwrap_or_default() } }; - let cuts: Vec = lower.iter().map(|t| threshold(wb, sheet, c, &st, t)).collect(); + let cuts: Vec = lower.iter().map(|t| threshold(wb, sheet, c, st, t)).collect(); let mut idx = cuts.iter().filter(|cut| n >= **cut).count(); if *reverse { idx = k - 1 - idx.min(k - 1); @@ -397,6 +435,105 @@ mod tests { assert_eq!([look("A1"), look("A2"), look("A4")].map(border), [true, false, false]); } + /// The statistics as every rule used to get them: one pass collecting both parts. + fn all_stats_before(sh: &Sheet, ranges: &[RangeRef]) -> RuleStats { + let mut nums = Vec::new(); + let mut counts = HashMap::new(); + for r in ranges { + let Some(r) = sh.used_range().and_then(|u| r.intersection(&u)) else { continue }; + for c in r.iter() { + let v = sh.value(c); + if let Value::Number(n) = v { + nums.push(n); + } + if !v.is_empty() { + *counts.entry(v.display().to_lowercase()).or_insert(0) += 1; + } + } + } + nums.sort_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal)); + let n = nums.len().max(1) as f64; + let avg = nums.iter().sum::() / n; + let std = (nums.iter().map(|x| (x - avg).powi(2)).sum::() / n).sqrt(); + RuleStats { sorted: nums, avg, std, counts } + } + + #[test] + fn needed_statistics_equal_the_full_ones() { + let mut s = crate::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [[3, "x"], [7, "X"], [3, null], ["=NA()", 2.5], [" ", 9], [-1, "y"]]})).unwrap(); + let d = s.doc().unwrap(); + let sh = d.wb.sheet(0).unwrap(); + for ranges in ["A1:B6", "A1:A3", "B2:B9", "D1:E4", "A1:A2"].map(|r| vec![RangeRef::parse(r).unwrap()]) { + let full = all_stats_before(sh, &ranges); + let numbers = RuleStats::build(sh, &ranges, StatsNeed::Numbers); + assert_eq!((numbers.sorted, numbers.avg.to_bits(), numbers.std.to_bits()), (full.sorted, full.avg.to_bits(), full.std.to_bits())); + assert_eq!(RuleStats::build(sh, &ranges, StatsNeed::Counts).counts, full.counts); + } + } + + #[test] + fn rules_that_read_statistics_still_match() { + let mut s = crate::Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [[1, "a"], [2, "A"], [3, "b"], [10, "c"], [2, null]]})).unwrap(); + let rule = |range: &str, r: serde_json::Value| json!({"range": range, "rule": r}); + s.execute("home.conditionalFormat", rule("B1:B5", json!({"type": "duplicate", "preset": "lightRedFill"}))).unwrap(); + s.execute("home.conditionalFormat", rule("A1:A5", json!({"type": "top10", "rank": 1, "preset": "redBorder"}))).unwrap(); + s.execute("home.conditionalFormat", rule("A1:A5", json!({"type": "colorScale"}))).unwrap(); + let d = s.doc().unwrap(); + let mut cache = CfCache::new(); + let mut look = |a: &str| cache.look(&d.wb, 0, CellRef::parse(a).unwrap()); + let fill = |l: Option| l.and_then(|l| l.style).is_some_and(|st| st.fill.pattern == PatternType::Solid); + // Duplicates are case-insensitive texts: "a" and "A". + assert_eq!(["B1", "B2", "B3", "B4", "B5"].map(|a| fill(look(a))), [true, true, false, false, false]); + // The top value gets the border, the scale runs from the minimum to the maximum. + assert!(look("A4").and_then(|l| l.style).is_some_and(|st| !st.borders.top.is_none())); + assert!(look("A1").and_then(|l| l.style).is_none_or(|st| st.borders.top.is_none())); + assert_eq!(look("A1").and_then(|l| l.scale_fill), Some([0xF8, 0x69, 0x6B])); + assert_eq!(look("A4").and_then(|l| l.scale_fill), Some([0x63, 0xBE, 0x7B])); + } + + /// Looking cells up does work in proportion to the cells asked for, not the rule's range: the + /// statistics are only gathered for rules that read them, and once per rule. + #[test] + fn lookups_do_not_scan_the_range_for_rules_without_statistics() { + let mut s = crate::Session::new(); + s.new_workbook(); + let rows: Vec = (1..=10_000).map(|i| json!([i, i % 7])).collect(); + s.execute("range.setValues", json!({"range": "A1", "values": rows})).unwrap(); + s.execute( + "home.conditionalFormat", + json!({"range": "A1:B10000", "rule": {"type": "expression", "formula": "=MOD(ROW(),2)=0", "preset": "lightRedFill"}}), + ) + .unwrap(); + s.execute( + "home.conditionalFormat", + json!({"range": "A1:B10000", "rule": {"type": "cellIs", "operator": "greater", "value": "5", "preset": "redBorder"}}), + ) + .unwrap(); + s.execute("home.conditionalFormat", json!({"range": "A1:B10000", "rule": {"type": "noBlanks", "preset": "redBorder"}})).unwrap(); + let d = s.doc().unwrap(); + let mut cache = CfCache::new(); + SCANNED.with(|n| n.set(0)); + for r in 1..=50 { + for col in ["A", "B"] { + cache.look(&d.wb, 0, CellRef::parse(&format!("{col}{r}")).unwrap()); + } + } + assert_eq!(SCANNED.with(|n| n.get()), 0); + // A rule that does read them scans its range once, however many cells are looked up. + s.execute("home.conditionalFormat", json!({"range": "A1:A10000", "rule": {"type": "top10", "preset": "redBorder"}})).unwrap(); + let d = s.doc().unwrap(); + let mut cache = CfCache::new(); + SCANNED.with(|n| n.set(0)); + for r in 1..=50 { + cache.look(&d.wb, 0, CellRef::parse(&format!("A{r}")).unwrap()); + } + assert_eq!(SCANNED.with(|n| n.get()), 10_000); + } + #[test] fn cell_value_operator_names() { let mut s = crate::Session::new(); From f519fd67f68fef644bcf691fa932e3cb942f6d09 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 04:49:33 +0200 Subject: [PATCH 07/22] XLSX: keep a dxf's protection, font size and face, and the font parts it turns off (part of #1209) --- crates/model/src/style.rs | 21 +++++++++ crates/xlsx/src/styles.rs | 76 +++++++++++++++++++++++-------- crates/xlsx/src/tests/fixtures.rs | 35 ++++++++++++++ 3 files changed, 112 insertions(+), 20 deletions(-) diff --git a/crates/model/src/style.rs b/crates/model/src/style.rs index d3976876e..800bb34d1 100644 --- a/crates/model/src/style.rs +++ b/crates/model/src/style.rs @@ -339,6 +339,27 @@ pub struct Style { /// Number format code; "General" by default. pub num_fmt: NumFmt, pub protection: Protection, + /// Differential formats only. + #[serde(default)] + pub dxf_stated: DxfStated, +} + +/// The parts a differential format states at a value its `Style` can't tell from unset: a dxf's +/// default parts are unset, so a bold turned off, a face, a size or a protection that matches the +/// defaults would otherwise read as nothing. The values are the style's own; this records that +/// the format states them, and only where they match the defaults, so a format built in the app +/// and the same format read back from a file stay equal. +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub struct DxfStated { + pub bold: bool, + pub italic: bool, + pub strike: bool, + pub underline: bool, + pub size: bool, + pub name: bool, + /// ``: the theme font the face follows (`major`, `minor` or `none`). + pub scheme: Option, + pub protection: bool, } #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] diff --git a/crates/xlsx/src/styles.rs b/crates/xlsx/src/styles.rs index 01f1fa632..49ef72ca5 100644 --- a/crates/xlsx/src/styles.rs +++ b/crates/xlsx/src/styles.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use std::fmt::Write as _; -use gridcraft_model::style::Protection; +use gridcraft_model::style::{DxfStated, Protection}; use gridcraft_model::{ Alignment, BorderLine, BorderStyle, Borders, Color, Fill, Font, HAlign, NumFmt, PatternType, Style, Underline, VAlign, VertAlign, Workbook, }; @@ -348,10 +348,23 @@ pub fn read_styles(root: &El, checkboxes: &[bool]) -> StylesIn { d.kids("dxf") .map(|x| { let mut s = Style::default(); - if let Some(f) = x.child("font") { - let f = read_font(f, &palette); - // Differential fonts rarely name a face; keep the default face and size. - s.font = Font { name: s.font.name.clone(), size: s.font.size, ..f }; + if let Some(e) = x.child("font") { + let f = read_font(e, &palette); + // Differential fonts rarely name a face; unless stated, keep the default face and size. + let has_size = e.child("sz").and_then(|n| n.attr_f64("val")).is_some(); + let has_name = e.child("name").and_then(|n| n.attr("val")).is_some(); + let name = if has_name { f.name.clone() } else { s.font.name.clone() }; + let size = if has_size { f.size } else { s.font.size }; + let def = Font::default(); + let st = &mut s.dxf_stated; + st.bold = e.child("b").is_some() && !f.bold; + st.italic = e.child("i").is_some() && !f.italic; + st.strike = e.child("strike").is_some() && !f.strike; + st.underline = e.child("u").is_some() && f.underline == Underline::None; + st.size = has_size && size == def.size; + st.name = has_name && name == def.name; + st.scheme = e.child_val("scheme").map(str::to_string); + s.font = Font { name, size, ..f }; } if let Some(n) = x.child("numFmt") { if let Some(code) = n.attr("formatCode") { @@ -371,6 +384,7 @@ pub fn read_styles(root: &El, checkboxes: &[bool]) -> StylesIn { } if let Some(p) = x.child("protection") { s.protection = read_protection(p); + s.dxf_stated.protection = s.protection == Protection::default(); } s }) @@ -396,19 +410,20 @@ pub fn color_attrs(c: &Color) -> String { } } -/// `minor`: the font follows the theme's body font (``). -fn font_xml(f: &Font, dxf: bool, minor: bool) -> String { +/// `minor`: the font follows the theme's body font (``). `dxf`: a +/// differential format's font, which writes only what it states. +fn font_xml(f: &Font, dxf: Option<&DxfStated>, minor: bool) -> String { + let stated = dxf.cloned().unwrap_or_default(); let mut s = String::from(""); - if f.bold { - s.push_str(""); - } - if f.italic { - s.push_str(""); - } - if f.strike { - s.push_str(""); + for (on, said, tag) in [(f.bold, stated.bold, "b"), (f.italic, stated.italic, "i"), (f.strike, stated.strike, "strike")] { + if on { + let _ = write!(s, "<{tag}/>"); + } else if said { + let _ = write!(s, "<{tag} val=\"0\"/>"); + } } match f.underline { + Underline::None if stated.underline => s.push_str(""), Underline::None => {} Underline::Single => s.push_str(""), Underline::Double => s.push_str(""), @@ -420,19 +435,27 @@ fn font_xml(f: &Font, dxf: bool, minor: bool) -> String { VertAlign::Superscript => s.push_str(""), VertAlign::Subscript => s.push_str(""), } - if !dxf { + let def = Font::default(); + if dxf.is_none() || stated.size || f.size != def.size { let _ = write!(s, "", num(f.size as f64)); } if f.color != Color::Auto { let _ = write!(s, "", color_attrs(&f.color)); - } else if !dxf { + } else if dxf.is_none() { s.push_str(""); } - if !dxf { + if dxf.is_none() { let _ = write!(s, "", esc_attr(&f.name)); if minor { s.push_str(""); } + } else { + if stated.name || f.name != def.name { + let _ = write!(s, "", esc_attr(&f.name)); + } + if let Some(sc) = &stated.scheme { + let _ = write!(s, "", esc_attr(sc)); + } } s.push_str(""); s @@ -664,14 +687,24 @@ pub fn write_styles(wb: &Workbook, dxfs: &[Style], extra_fmts: &[String]) -> (St for d in dxfs { dx.push_str(""); let def = Style::default(); + let st = &d.dxf_stated; if d.font.bold || d.font.italic || d.font.strike || d.font.underline != Underline::None || d.font.color != Color::Auto || d.font.vert != VertAlign::Baseline + || d.font.size != def.font.size + || d.font.name != def.font.name + || st.bold + || st.italic + || st.strike + || st.underline + || st.size + || st.name + || st.scheme.is_some() { - dx.push_str(&font_xml(&d.font, true, false)); + dx.push_str(&font_xml(&d.font, Some(st), false)); } if d.num_fmt != def.num_fmt { let id = nf.id(d.num_fmt.as_str()); @@ -683,6 +716,9 @@ pub fn write_styles(wb: &Workbook, dxfs: &[Style], extra_fmts: &[String]) -> (St if let Some(a) = alignment_xml(&d.align) { dx.push_str(&a); } + if st.protection || d.protection != def.protection { + let _ = write!(dx, ""); } for f in extra_fmts { diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index b15f4da7d..19a6335b8 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -224,6 +224,32 @@ fn dxf_font_and_parts_stated_at_their_defaults_survive_save() { assert_eq!(written, Some(format!(r#" count="4">{dxfs}"#).as_str())); } +/// A dxf writes its parts in schema order: the border comes before the protection. +#[test] +fn dxf_border_is_written_before_protection() { + let dxfs = r#""#; + let styles = format!( + r#" + + + + + {dxfs} + "# + ); + let rels = r#""#; + let body = r#"1 + 0"#; + let bytes = minimal(body, &[("xl/styles.xml", &styles)], rels, ""); + let (wb, _) = read_xlsx(&bytes).unwrap(); + let out = crate::write_xlsx(&wb).unwrap(); + let st = crate::package::Package::open(&out).unwrap().read("xl/styles.xml").unwrap().unwrap(); + let st = String::from_utf8(st).unwrap(); + let dx = st.split_once("")).map(|(d, _)| d).unwrap_or_default(); + let (b, p) = (dx.find(" Date: Thu, 22 Oct 2026 02:35:41 +0000 Subject: [PATCH 22/22] Integration: #1365 kittest for the font dropdown's filtered highlight and cleared filter --- crates/ui-egui/tests/keytips.rs | 34 +++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/crates/ui-egui/tests/keytips.rs b/crates/ui-egui/tests/keytips.rs index 887721cdb..663ea8b1b 100644 --- a/crates/ui-egui/tests/keytips.rs +++ b/crates/ui-egui/tests/keytips.rs @@ -627,3 +627,37 @@ fn every_sequence_terminates_without_panicking() { } assert_eq!(SEQUENCES.len(), SEQUENCES.len(), "every sequence ran"); } + +/// The font list's highlight is a position in the filtered rows (#1365): with "ar" typed the rows +/// are Aptos Narrow, Arial, Candara, Garamond, and the highlight starts on the current font's +/// row (index 3, clamped to the filtered list). Down twice lands on the second row, Arial. Clearing +/// the filter brings the other fonts back. +#[test] +fn font_dropdown_highlight_follows_the_filtered_rows() { + let mut h = harness(blank()); + sequence(&mut h, &[Key::H, Key::F, Key::F], false); + h.run_steps(3); + events(&mut h, [Event::Text("ar".into())]); + h.run_steps(3); + assert_eq!(h.query_all_by_label("Georgia").count(), 0, "typing must narrow the list"); + key(&mut h, Key::ArrowDown); + key(&mut h, Key::ArrowDown); + key(&mut h, Key::Enter); + h.run_steps(3); + let d = h.state().session.active().unwrap(); + let sh = d.wb.active().unwrap(); + let name = d.wb.styles.get(sh.style_id(CellRef::parse("A1").unwrap())).font.name.clone(); + assert_eq!(name, "Arial", "the highlight is the second filtered row, not a full-list index"); + + // Clearing the filter shows the whole list again. + let mut h = harness(blank()); + sequence(&mut h, &[Key::H, Key::F, Key::F], false); + h.run_steps(3); + events(&mut h, [Event::Text("ar".into())]); + h.run_steps(3); + assert_eq!(h.query_all_by_label("Georgia").count(), 0); + key(&mut h, Key::Backspace); + key(&mut h, Key::Backspace); + h.run_steps(3); + assert!(h.query_all_by_label("Georgia").count() >= 1, "an emptied filter lists every font again"); +}