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 1/2] 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 2/2] 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")] {