From 374ab0f127e4d1e08ffb32d86dacfeea6c537d18 Mon Sep 17 00:00:00 2001 From: neilcawse <8085959+neilcawse@users.noreply.github.com> Date: Mon, 12 Oct 2026 07:50:52 -0400 Subject: [PATCH 01/12] XLSX: PivotCharts plot their cached points; deleted automatic titles stay off Excel often saves a PivotChart series with formulas that only approximate its PivotTable: every formula pointing at the table corner cell, or a block that takes in the grand total row. Excel draws the chart from the PivotTable, so the cached points are what it shows; GridCraft evaluated the formulas and drew empty charts or a stray grand total bar. When a PivotChart series references don't cover their cached points (numCache, strCache or the innermost multiLvlStrCache level), the series now plots the cached points and cached name as array constants. Refreshing the PivotTable rebinds the chart to live cells as before. A chart with and no title now opens with its automatic title off, instead of showing the lone series name ("Series1"). Array-constant series also survive a save: literals are read by index and ptCount (gaps stay #N/A instead of shifting later points), and written by parsing the array constant, so text with quotes, commas or semicolons is kept. Literal values keep their number format: Series gains number_format, read from the numLit or numCache formatCode and written as the numLit formatCode (it was always General). The renderer uses it only for array constants; references keep using their cells' format. Cached PivotChart points now label as "$185" or "5.36%", as in Excel. Co-Authored-By: Claude Opus 5.5 --- crates/chart/src/resolve.rs | 5 +- crates/chart/src/tests.rs | 10 + crates/engine/src/cmd/forecast.rs | 1 + crates/engine/src/cmd/insert.rs | 2 + crates/engine/src/pivot.rs | 1 + crates/engine/src/sample.rs | 1 + crates/model/src/features.rs | 4 + crates/xlsx/src/chart.rs | 179 +++++++++++++++--- crates/xlsx/src/tests/chart_read_tests.rs | 113 +++++++++++ .../src/tests/literal_category_roundtrip.rs | 1 + crates/xlsx/src/tests/mod.rs | 1 + crates/xlsx/src/tests/roundtrip.rs | 1 + crates/xlsx/src/tests/smooth_roundtrip.rs | 1 + crates/xlsx/src/tests/trendline_roundtrip.rs | 1 + docs/chart-parity.md | 5 +- 15 files changed, 301 insertions(+), 25 deletions(-) create mode 100644 crates/xlsx/src/tests/chart_read_tests.rs diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index c007e3e90..c3f8b8ed5 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -236,7 +236,10 @@ pub fn resolve(wb: &Workbook, sheet: usize, chart: &Chart) -> ChartData { secondary: s.secondary, smooth: s.smooth, trendlines, - number_format: ref_format(wb, sheet, &s.values), + number_format: match &s.number_format { + Some(code) if s.values.trim_start().starts_with('{') => code.clone(), + _ => ref_format(wb, sheet, &s.values), + }, }); } let n = series.iter().map(|s| s.values.len()).max().unwrap_or(0); diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 01f1aac49..de8f47285 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -229,6 +229,8 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + // References use their cells' format, whatever the series says. + number_format: Some("0%".into()), }, Series { name: None, @@ -240,6 +242,7 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }, Series { name: Some("Literal".into()), @@ -251,6 +254,7 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: Some("0.0%".into()), }, ]; let d = resolve(&wb, 0, &c); @@ -270,6 +274,7 @@ fn resolve_from_workbook() { assert!(s1.secondary); assert_eq!(d.series[2].name, "Literal"); assert_eq!(d.series[2].values.len(), 4); + assert_eq!(d.series[2].number_format, "0.0%"); // Default categories. c.series.remove(0); @@ -289,6 +294,7 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }, Series { name: None, @@ -300,6 +306,7 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }, ]; let d = resolve(&wb, 0, &c); @@ -316,6 +323,7 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }]; let d = resolve(&wb, 0, &c); assert_eq!(d.series[0].x.as_ref().map(|x| x.len()), Some(3)); @@ -333,6 +341,7 @@ fn resolve_from_workbook() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }]; let d = resolve(&wb, 7, &c); assert_eq!(d.series.len(), 1); @@ -1003,6 +1012,7 @@ fn pie_points_follow_workbook_theme() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }]; let d = resolve(&wb, 0, &c); let expected = vec![series_color(&wb.theme, 0), series_color(&wb.theme, 1)]; diff --git a/crates/engine/src/cmd/forecast.rs b/crates/engine/src/cmd/forecast.rs index d25b367ba..fbed59aad 100644 --- a/crates/engine/src/cmd/forecast.rs +++ b/crates/engine/src/cmd/forecast.rs @@ -413,6 +413,7 @@ fn forecast_sheet(s: &mut Session, p: &Json) -> Result { kind: (opts.column_chart && c >= 3).then_some(ChartKind::Line), smooth: false, trendlines: Vec::new(), + number_format: None, }) .collect(); sheet.charts.push(Chart { diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index d878e4469..f6ab29591 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -605,6 +605,7 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }); } } else { @@ -620,6 +621,7 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }); } } diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index 6146bd4f9..cc167fb7e 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -1569,6 +1569,7 @@ pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }) }) .collect() diff --git a/crates/engine/src/sample.rs b/crates/engine/src/sample.rs index 4d4822445..f3dee6489 100644 --- a/crates/engine/src/sample.rs +++ b/crates/engine/src/sample.rs @@ -110,6 +110,7 @@ fn sales(s: &mut Session) -> bool { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }]; } } diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index b51f7bc98..4ed6bdb7c 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -424,6 +424,10 @@ pub struct Series { /// Trendlines fitted to this series (Excel allows several per series). #[serde(default)] pub trendlines: Vec, + /// Number format of literal values (an array constant, such as a PivotChart's cached + /// points). Values that reference cells use the cells' format instead. + #[serde(default)] + pub number_format: Option, } /// Regression a trendline fits (`c:trendlineType`). diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index dee33c83d..363c83793 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -72,18 +72,111 @@ fn data_ref(e: Option<&El>) -> Option { // Literal values become an array constant. for k in ["numLit", "strLit"] { if let Some(l) = e.child(k) { - let pts: Vec = l - .kids("pt") - .filter_map(|p| p.child("v")) - .map(|v| if k == "numLit" { v.text.trim().to_string() } else { format!("\"{}\"", v.text.replace('"', "\"\"")) }) - .collect(); - return Some(format!("{{{}}}", pts.join(","))); + return points_literal(l, l, k == "numLit").map(|(_, lit)| lit); } } None } -fn read_series(s: &El) -> Series { +/// Cells a series reference covers: `Sheet!$A$1:$A$9`, or a union `(Sheet!$A$1:$A$3,Sheet!$A$5)`. +fn ref_cells(f: &str) -> Option { + let f = f.trim(); + let inner = f.strip_prefix('(').and_then(|x| x.strip_suffix(')')).unwrap_or(f); + let mut parts = Vec::new(); + let (mut quoted, mut start) = (false, 0); + for (i, c) in inner.char_indices() { + match c { + '\'' => quoted = !quoted, + ',' if !quoted => { + parts.push(inner.get(start..i)?); + start = i + 1; + } + _ => {} + } + } + parts.push(inner.get(start..)?); + let mut total = 0u64; + for p in parts { + let addr = p.rsplit_once('!').map_or(p, |(_, a)| a).trim(); + let r = gridcraft_core::RangeRef::parse(addr)?; + total = total.checked_add(u64::from(r.width()) * u64::from(r.height()))?; + } + Some(total) +} + +/// Most cached points a PivotChart series keeps (the chart renderer's own cap). +const MAX_CACHED_POINTS: u64 = 100_000; + +/// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s) as an array +/// constant, with the point count. Missing points become `#N/A` (numbers) or `""` (text) so +/// positions line up. Without a `ptCount`, the highest index sets the count. +fn points_literal(c: &El, pts: &El, numeric: bool) -> Option<(u64, String)> { + let idx = |p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()); + let count = match c.child_val("ptCount") { + Some(v) => v.trim().parse::().ok()?, + None => pts.kids("pt").filter_map(idx).max().map_or(0, |m| m as u64 + 1), + }; + if !(1..=MAX_CACHED_POINTS).contains(&count) { + return None; + } + let mut vals: Vec> = vec![None; usize::try_from(count).ok()?]; + for p in pts.kids("pt") { + if let (Some(slot), Some(v)) = (idx(p).and_then(|i| vals.get_mut(i)), p.child("v")) { + *slot = Some(v.text.as_str()); + } + } + let items: Vec = vals + .iter() + .map(|v| match (v, numeric) { + (Some(t), true) => t.trim().parse::().ok().filter(|n| n.is_finite()).map_or_else(|| "#N/A".to_string(), num), + (None, true) => "#N/A".to_string(), + (Some(t), false) => format!("\"{}\"", t.replace('"', "\"\"")), + (None, false) => "\"\"".to_string(), + }) + .collect(); + Some((count, format!("{{{}}}", items.join(",")))) +} + +/// The points cached under a series reference: `numCache`, `strCache`, or the innermost level +/// of a `multiLvlStrCache`. +fn cache_literal(r: &El) -> Option<(u64, String)> { + if let Some(c) = r.child("numCache") { + points_literal(c, c, true) + } else if let Some(c) = r.child("strCache") { + points_literal(c, c, false) + } else { + let c = r.child("multiLvlStrCache")?; + points_literal(c, c.child("lvl")?, false) + } +} + +/// A PivotChart's `` formulas only approximate its PivotTable: often just the table's corner +/// cell, or a block that takes in the grand total row. Excel draws the chart from the PivotTable, +/// and the cached points are what it shows. When a series' categories or values don't cover their +/// cached points, the series plots the cached points (and its cached name) instead; refreshing +/// the PivotTable rebinds the chart to live cells. +fn use_pivot_cache(s: &El, ser: &mut Series) { + let cached = |tag: &str, f: Option<&str>| -> Option { + let e = s.child(tag)?; + let r = ["numRef", "strRef", "multiLvlStrRef"].iter().find_map(|k| e.child(k))?; + let (count, lit) = cache_literal(r)?; + (f.and_then(ref_cells) != Some(count)).then_some(lit) + }; + let categories = cached("cat", ser.categories.as_deref()); + let values = cached("val", Some(ser.values.as_str())); + if categories.is_none() && values.is_none() { + return; + } + ser.categories = categories.or(ser.categories.take()); + if let Some(v) = values { + ser.values = v; + } + if let Some(name) = s.path(&["tx", "strRef", "strCache", "pt", "v"]) { + ser.name = Some(format!("\"{}\"", name.text.replace('"', "\"\""))); + } +} + +fn read_series(s: &El, pivot: bool) -> Series { let name = s.child("tx").and_then(|tx| { if let Some(f) = tx.path(&["strRef", "f"]) { let t = f.text.trim(); @@ -98,7 +191,7 @@ fn read_series(s: &El) -> Series { .and_then(dml_color) .or_else(|| sp.and_then(|p| p.path(&["ln", "solidFill"])).and_then(dml_color)) .or_else(|| s.path(&["marker", "spPr", "solidFill"]).and_then(dml_color)); - Series { + let mut ser = Series { name, categories: data_ref(s.child("cat")).or_else(|| data_ref(s.child("xVal"))), values: data_ref(s.child("val")).or_else(|| data_ref(s.child("yVal"))).unwrap_or_default(), @@ -108,7 +201,22 @@ fn read_series(s: &El) -> Series { kind: None, smooth: bool_val(s, "smooth"), trendlines: s.kids("trendline").filter_map(read_trendline).collect(), - } + number_format: None, + }; + if pivot { + use_pivot_cache(s, &mut ser); + } + // Literal values (a `numLit`, or the cached points above) keep the format code saved with them. + if ser.values.starts_with('{') { + let v = s.child("val").or_else(|| s.child("yVal")); + ser.number_format = v + .and_then(|v| v.child("numLit").or_else(|| v.path(&["numRef", "numCache"]))) + .and_then(|c| c.child("formatCode")) + .map(|c| c.text.trim()) + .filter(|c| !c.is_empty() && !c.eq_ignore_ascii_case("General")) + .map(str::to_string); + } + ser } const DASHES: [(LineDash, &str); 11] = [ @@ -189,10 +297,11 @@ pub fn read_chart(root: &El) -> Option { let groups: Vec<(&El, ChartKind)> = plot.children.iter().filter_map(|g| group_kind(g).map(|k| (g, k))).collect(); let (first, first_kind) = *groups.first()?; let first_axes = axis_ids(first); + let pivot = root.path(&["pivotSource", "name"]).map(|n| n.text.trim().to_string()).filter(|n| !n.is_empty()); let mut series = vec![]; for (gi, (g, k)) in groups.iter().enumerate() { for s in g.kids("ser") { - let mut ser = read_series(s); + let mut ser = read_series(s, pivot.is_some()); if gi > 0 { ser.kind = Some(*k); ser.secondary = axis_ids(g) != first_axes; @@ -226,7 +335,12 @@ pub fn read_chart(root: &El) -> Option { id: 0, kind, anchor: Default::default(), - title: chart.child("title").and_then(rich_title), + // No ``: an automatic title (a lone series' name), unless the file deleted it. + title: match chart.child("title") { + Some(t) => rich_title(t), + None if bool_val(chart, "autoTitleDeleted") => Some(String::new()), + None => None, + }, series, legend, data_labels, @@ -236,7 +350,7 @@ pub fn read_chart(root: &El) -> Option { y_title: y_ax.and_then(|a| a.child("title")).and_then(rich_title), source: None, by_rows: false, - pivot: root.path(&["pivotSource", "name"]).map(|n| n.text.trim().to_string()).filter(|n| !n.is_empty()), + pivot, }) } @@ -306,16 +420,35 @@ fn is_ref(f: &str) -> bool { matches!(gridcraft_formula::parse(f), Ok(Expr::Ref(_))) } -fn num_data(tag: &str, f: Option<&str>, wb: &Workbook, si: usize) -> String { +/// The items of an array constant (`{1,"a";#N/A,2}`), row-major; nothing for anything else. +fn array_items(f: &str) -> Vec { + match gridcraft_formula::parse(f) { + Ok(Expr::Array(rows)) => rows.into_iter().flatten().collect(), + _ => Vec::new(), + } +} + +/// `fmt` is the format code saved with literal values (`numLit`); references save their cells'. +fn num_data(tag: &str, f: Option<&str>, fmt: Option<&str>, wb: &Workbook, si: usize) -> String { let f = f.unwrap_or("").trim(); if f.is_empty() { return format!(""); } if f.starts_with('{') { - let vals: Vec<&str> = f.trim_matches(|c| c == '{' || c == '}').split([',', ';']).collect(); - let mut s = format!("General", vals.len()); - for (i, v) in vals.iter().enumerate() { - if let Ok(n) = v.trim().parse::() { + let items = array_items(f); + let fmt = esc(fmt.unwrap_or("General")); + let mut s = format!("{fmt}", items.len()); + for (i, e) in items.iter().enumerate() { + let n = match e { + Expr::Number(n) => Some(*n), + Expr::Unary(gridcraft_formula::UnOp::Neg, x) => match **x { + Expr::Number(n) => Some(-n), + _ => None, + }, + _ => None, + }; + // Anything else (`#N/A`, text) is a gap: its index is left out. + if let Some(n) = n { let _ = write!(s, "{}", num(n)); } } @@ -441,11 +574,11 @@ fn series_xml(s: &Series, idx: usize, g: G, wb: &Workbook, si: usize) -> String match g { G::Scatter { .. } | G::Bubble => { if let Some(c) = &s.categories { - x.push_str(&num_data("xVal", Some(c), wb, si)); + x.push_str(&num_data("xVal", Some(c), None, wb, si)); } - x.push_str(&num_data("yVal", Some(&s.values), wb, si)); + x.push_str(&num_data("yVal", Some(&s.values), s.number_format.as_deref(), wb, si)); if g == G::Bubble { - x.push_str(&num_data("bubbleSize", s.bubble_sizes.as_deref().or(Some(&s.values)), wb, si)); + x.push_str(&num_data("bubbleSize", s.bubble_sizes.as_deref().or(Some(&s.values)), None, wb, si)); x.push_str(""); } else { let _ = write!(x, "", s.smooth as u8); @@ -455,7 +588,7 @@ fn series_xml(s: &Series, idx: usize, g: G, wb: &Workbook, si: usize) -> String if let Some(c) = &s.categories { x.push_str(&str_data("cat", c, wb, si)); } - x.push_str(&num_data("val", Some(&s.values), wb, si)); + x.push_str(&num_data("val", Some(&s.values), s.number_format.as_deref(), wb, si)); if matches!(g, G::Line { .. }) { let _ = write!(x, "", s.smooth as u8); } @@ -623,12 +756,14 @@ pub fn write_chart(wb: &Workbook, si: usize, ch: &Chart) -> String { let _ = write!(s, "{}", esc(link)); } s.push_str(""); + // `None` is the automatic title; an empty title is one that was turned off. match &ch.title { Some(t) if !t.is_empty() => { s.push_str(&title_xml(t)); s.push_str(""); } - _ => s.push_str(""), + Some(_) => s.push_str(""), + None => s.push_str(""), } s.push_str(""); let all: Vec<(usize, &Series)> = ch.series.iter().enumerate().collect(); diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs new file mode 100644 index 000000000..c4c694a16 --- /dev/null +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -0,0 +1,113 @@ +//! Reading charts as Excel saved them: PivotChart series and deleted automatic titles. + +use gridcraft_model::{Chart, ChartKind, Workbook}; + +const NS: &str = + r#"xmlns:c="http://schemas.openxmlformats.org/drawingml/2006/chart" xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main""#; + +fn read(xml: &str) -> Chart { + crate::chart::read_chart(&crate::xml::parse(xml.as_bytes()).unwrap()).unwrap() +} + +fn str_cache(f: &str, pts: &[&str]) -> String { + let p: String = pts.iter().enumerate().map(|(i, v)| format!(r#"{v}"#)).collect(); + format!(r#"{f}{p}"#, pts.len()) +} + +/// A numeric cache of `count` points; `pts` are (idx, value), so gaps can be left out. +fn num_cache(f: &str, count: usize, pts: &[(usize, &str)]) -> String { + let p: String = pts.iter().map(|(i, v)| format!(r#"{v}"#)).collect(); + format!(r#"{f}General{p}"#) +} + +fn ser(tx: &str, cat: &str, val: &str) -> String { + format!("{tx}{cat}{val}") +} + +fn chart_xml(pivot: bool, plot: &str, title: &str) -> String { + let src = if pivot { "[Book.xlsx]Graph!PivotTable1" } else { "" }; + format!( + r#"{src}{title}{plot}"# + ) +} + +#[test] +fn pivot_charts_plot_their_cached_points_when_the_references_do_not_cover_them() { + // Excel points every formula of this combo PivotChart at the PivotTable's corner cell and + // draws it from the PivotTable; the cached points are what it shows. + let cats = ["Truck 1", "Van \"B\"", "Car"]; + let bar = + ser(&str_cache("Graph!$J$2", &["Events"]), &str_cache("Graph!$J$2", &cats), &num_cache("Graph!$J$2", 3, &[(0, "18"), (1, "12"), (2, "5")])); + // A point the cache leaves out stays a gap, so later points keep their positions. + let line = ser(&str_cache("Graph!$J$2", &["Share"]), &str_cache("Graph!$J$2", &cats), &num_cache("Graph!$J$2", 3, &[(0, "5.3E-2"), (2, "0.5")])); + let plot = format!( + r#"{bar}{line}"# + ); + let c = read(&chart_xml(true, &plot, "")); + assert_eq!(c.pivot.as_deref(), Some("[Book.xlsx]Graph!PivotTable1")); + assert_eq!(c.kind, ChartKind::Combo); + let s = &c.series; + assert_eq!(s[0].name.as_deref(), Some("\"Events\"")); + assert_eq!(s[0].categories.as_deref(), Some(r#"{"Truck 1","Van ""B""","Car"}"#)); + assert_eq!(s[0].values, "{18,12,5}"); + assert_eq!(s[1].name.as_deref(), Some("\"Share\"")); + assert_eq!(s[1].values, "{0.053,#N/A,0.5}"); + + // The block Excel writes takes in the grand total row (4 cells for 3 points): the total is + // not plotted. + let totals = ser( + &str_cache("Summary!$B$4", &["Total"]), + &str_cache("Summary!$A$5:$A$8", &cats), + &num_cache("Summary!$B$5:$B$8", 3, &[(0, "1010"), (1, "185.14"), (2, "120")]).replace("General", "\"$\"#,##0"), + ); + let c = read(&chart_xml(true, &format!(r#"{totals}"#), "")); + assert_eq!(c.series[0].values, "{1010,185.14,120}"); + assert_eq!(c.series[0].categories.as_deref(), Some(r#"{"Truck 1","Van ""B""","Car"}"#)); + // The cached points keep the cache's number format (data labels show "$185"), and so does + // a save. + assert_eq!(c.series[0].number_format.as_deref(), Some("\"$\"#,##0")); + let back = read(&crate::chart::write_chart(&Workbook::new(), 0, &c)); + assert_eq!(back.series[0].number_format, c.series[0].number_format); + + // References that cover the cached points stay live, and so do an ordinary chart's. + let live = ser(&str_cache("Summary!$B$4", &["Total"]), &str_cache("Summary!$A$5:$A$7", &cats), &num_cache("Summary!$B$5:$B$7", 3, &[(0, "1")])); + let c = read(&chart_xml(true, &format!(r#"{live}"#), "")); + assert_eq!((c.series[0].name.as_deref(), c.series[0].values.as_str()), (Some("Summary!$B$4"), "Summary!$B$5:$B$7")); + assert_eq!(c.series[0].number_format, None, "live references use their cells' format"); + let plain = ser(&str_cache("Graph!$J$2", &["Events"]), &str_cache("Graph!$J$2", &cats), &num_cache("Graph!$J$2", 3, &[(0, "18")])); + let c = read(&chart_xml(false, &format!(r#"{plain}"#), "")); + assert_eq!(c.series[0].values, "Graph!$J$2"); + + // Saved again, the cached points come back as they were: quotes, gaps and all. (A literal + // name is saved as plain text, which names the series the same way.) + let c = read(&chart_xml(true, &plot, "")); + let back = read(&crate::chart::write_chart(&Workbook::new(), 0, &c)); + assert_eq!(back.pivot, c.pivot); + let parts = |c: &Chart| { + c.series + .iter() + .map(|s| (s.name.as_deref().map(|n| n.trim_matches('"').to_string()), s.categories.clone(), s.values.clone())) + .collect::>() + }; + assert_eq!(parts(&back), parts(&c)); +} + +#[test] +fn a_deleted_automatic_title_stays_deleted() { + let one = ser("Vehicles", &str_cache("S!$A$1:$A$2", &["a", "b"]), &num_cache("S!$B$1:$B$2", 2, &[])); + let pie = format!("{one}"); + // A lone series names an automatic title unless the file turned it off. + assert_eq!(read(&chart_xml(false, &pie, "")).title, None); + assert_eq!(read(&chart_xml(false, &pie, r#""#)).title, None); + let deleted = read(&chart_xml(false, &pie, r#""#)); + assert_eq!(deleted.title.as_deref(), Some("")); + // An explicit title wins over the flag. + let t = r#"Risk"#; + assert_eq!(read(&chart_xml(false, &pie, t)).title.as_deref(), Some("Risk")); + + // Both states survive a save. + let wb = Workbook::new(); + assert_eq!(read(&crate::chart::write_chart(&wb, 0, &deleted)).title.as_deref(), Some("")); + let auto = read(&chart_xml(false, &pie, "")); + assert_eq!(read(&crate::chart::write_chart(&wb, 0, &auto)).title, None); +} diff --git a/crates/xlsx/src/tests/literal_category_roundtrip.rs b/crates/xlsx/src/tests/literal_category_roundtrip.rs index c818de221..1cb40f1c9 100644 --- a/crates/xlsx/src/tests/literal_category_roundtrip.rs +++ b/crates/xlsx/src/tests/literal_category_roundtrip.rs @@ -30,6 +30,7 @@ fn literal_category_with_separators_survives_save() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }) .collect(); s.charts.push(Chart { diff --git a/crates/xlsx/src/tests/mod.rs b/crates/xlsx/src/tests/mod.rs index 517113d0f..68a2af172 100644 --- a/crates/xlsx/src/tests/mod.rs +++ b/crates/xlsx/src/tests/mod.rs @@ -2,6 +2,7 @@ mod array_export; mod chart_bool_default; +mod chart_read_tests; mod csv_tests; mod encrypted_tests; mod fixtures; diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 901802905..1d44c1a38 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -382,6 +382,7 @@ fn sample() -> Workbook { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }], legend: LegendPos::Bottom, data_labels: true, diff --git a/crates/xlsx/src/tests/smooth_roundtrip.rs b/crates/xlsx/src/tests/smooth_roundtrip.rs index 5a4f68fb2..bb10a7c80 100644 --- a/crates/xlsx/src/tests/smooth_roundtrip.rs +++ b/crates/xlsx/src/tests/smooth_roundtrip.rs @@ -19,6 +19,7 @@ fn smooth_line_setting_round_trips() { kind: None, smooth: false, trendlines: Vec::new(), + number_format: None, }; s.charts.push(Chart { id: 1, diff --git a/crates/xlsx/src/tests/trendline_roundtrip.rs b/crates/xlsx/src/tests/trendline_roundtrip.rs index d954c9bc6..49a6b07b7 100644 --- a/crates/xlsx/src/tests/trendline_roundtrip.rs +++ b/crates/xlsx/src/tests/trendline_roundtrip.rs @@ -49,6 +49,7 @@ fn chart(id: u32, kind: ChartKind, trendlines: Vec) -> Chart { kind: None, smooth: false, trendlines, + number_format: None, }], legend: LegendPos::Bottom, data_labels: false, diff --git a/docs/chart-parity.md b/docs/chart-parity.md index 9058b0935..a3340584c 100644 --- a/docs/chart-parity.md +++ b/docs/chart-parity.md @@ -1,6 +1,6 @@ # Chart parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** trivial (chart sheets are kept on save, still not editable (#727); previously minor: trendlines: model, XLSX round-trip, drawing, Add Chart Element) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-13 · **Change:** minor (PivotCharts from files plot their cached points; deleted automatic titles stay off) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's chart types and chart elements against GridCraft. Part of [target-app-parity.md](target-app-parity.md). **Breadth ~50%, ready ~30%, 40–60 h** (estimated). @@ -55,7 +55,7 @@ open PRs #158, #161, and issue #190. | Recommended Charts | yes | partial (Insert ▸ Recommended) | | Switch Row/Column, Select Data | yes | yes (Switch on charts read from files landed in #116); Select Data dialog: data range, remove series, Horizontal (Category) Axis Labels (#287). No series add/edit or hidden/empty cell options | | Sparklines (line, column, win/loss, markers, axis) | yes | line, column, win/loss | -| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only | +| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose references don't cover its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed; data labels and the value axis use the cache's number format, which is kept through a save | | Animation on data change | yes (Windows) | no | ## XLSX round-trip @@ -80,6 +80,7 @@ back. | Date | Change | Summary | |---|---|---| +| 2026-10-13 | minor | XLSX: PivotChart series whose references don't cover their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format; `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save | | 2026-10-12 | trivial | chart sheets are kept on save, still not editable (#727) | | 2026-10-11 | minor | Trendlines: all six types modelled, round-tripped through `c:trendline`, drawn with forecast and equation/R² labels; `chart.addElement` `trendline` | | 2026-10-11 | minor | PivotCharts (`insert.pivotChart`) | From df4599d2cdf62a636ec4cb44bc3a835d641dd172 Mon Sep 17 00:00:00 2001 From: neilcawse <8085959+neilcawse@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:01:42 -0400 Subject: [PATCH 02/12] XLSX: PivotChart series keep live cells only when they hold the cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PivotChart series stayed live whenever its reference had as many cells as the cache had points. With one row item, every formula pointing at the PivotTable's corner cell passed that test, so the chart plotted the header text. Multi-level category references never passed it (width × height cells for N points), so a correct reference was replaced by a leaf-only literal and its outer level was lost on save. The check now compares values, once every sheet is read (read_chart returns each PivotChart series' cache; plot_pivot_caches runs after the sheets load): a series plots its cache unless the referenced cells hold the cached points, numbers within rounding and text as is. For a multiLvlStrRef, the innermost level's column (or row) is compared, so a reference that matches stays live and is saved as a reference. Literals read before this branch keep their points again: a missing, unreadable or too small ptCount gives way to the points present, and points without an idx go in document order. Co-Authored-By: Claude Opus 5.5 --- crates/xlsx/src/chart.rs | 286 +++++++++++++------ crates/xlsx/src/drawing.rs | 12 +- crates/xlsx/src/read.rs | 6 + crates/xlsx/src/tests/chart_bool_default.rs | 2 +- crates/xlsx/src/tests/chart_read_tests.rs | 112 +++++++- crates/xlsx/src/tests/trendline_roundtrip.rs | 2 +- docs/chart-parity.md | 4 +- 7 files changed, 319 insertions(+), 105 deletions(-) diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index 363c83793..198487c96 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -3,8 +3,8 @@ use std::fmt::Write as _; -use gridcraft_core::Value; -use gridcraft_formula::{Expr, SheetSel}; +use gridcraft_core::{CellRef, RangeRef, Value}; +use gridcraft_formula::{BinOp, Expr, Reference, SheetSel}; use gridcraft_model::{Chart, ChartKind, Color, LegendPos, LineDash, Series, Trendline, TrendlineKind, Workbook}; use crate::drawing::{dml_color, srgb}; @@ -72,111 +72,207 @@ fn data_ref(e: Option<&El>) -> Option { // Literal values become an array constant. for k in ["numLit", "strLit"] { if let Some(l) = e.child(k) { - return points_literal(l, l, k == "numLit").map(|(_, lit)| lit); + return read_points(l, l).map(|p| literal(&p, k == "numLit")); } } None } -/// Cells a series reference covers: `Sheet!$A$1:$A$9`, or a union `(Sheet!$A$1:$A$3,Sheet!$A$5)`. -fn ref_cells(f: &str) -> Option { - let f = f.trim(); - let inner = f.strip_prefix('(').and_then(|x| x.strip_suffix(')')).unwrap_or(f); - let mut parts = Vec::new(); - let (mut quoted, mut start) = (false, 0); - for (i, c) in inner.char_indices() { - match c { - '\'' => quoted = !quoted, - ',' if !quoted => { - parts.push(inner.get(start..i)?); - start = i + 1; - } - _ => {} - } - } - parts.push(inner.get(start..)?); - let mut total = 0u64; - for p in parts { - let addr = p.rsplit_once('!').map_or(p, |(_, a)| a).trim(); - let r = gridcraft_core::RangeRef::parse(addr)?; - total = total.checked_add(u64::from(r.width()) * u64::from(r.height()))?; - } - Some(total) -} - /// Most cached points a PivotChart series keeps (the chart renderer's own cap). -const MAX_CACHED_POINTS: u64 = 100_000; - -/// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s) as an array -/// constant, with the point count. Missing points become `#N/A` (numbers) or `""` (text) so -/// positions line up. Without a `ptCount`, the highest index sets the count. -fn points_literal(c: &El, pts: &El, numeric: bool) -> Option<(u64, String)> { - let idx = |p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()); - let count = match c.child_val("ptCount") { - Some(v) => v.trim().parse::().ok()?, - None => pts.kids("pt").filter_map(idx).max().map_or(0, |m| m as u64 + 1), - }; - if !(1..=MAX_CACHED_POINTS).contains(&count) { +const MAX_CACHED_POINTS: usize = 100_000; + +/// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s), by index; missing +/// points are `None`. Points without a valid `idx` (schema-invalid, but some writers omit it) go +/// in document order. A missing, unreadable or too small `ptCount` gives way to the points present. +fn read_points<'a>(c: &El, pts: &'a El) -> Option>> { + let at = |i: usize, p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()).unwrap_or(i); + let placed = || pts.kids("pt").enumerate().map(|(i, p)| (at(i, p), p)).filter(|(i, _)| *i < MAX_CACHED_POINTS); + let highest = placed().map(|(i, _)| i + 1).max().unwrap_or(0); + let count = + c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).filter(|n| (highest..=MAX_CACHED_POINTS).contains(n)).unwrap_or(highest); + if count == 0 { return None; } - let mut vals: Vec> = vec![None; usize::try_from(count).ok()?]; - for p in pts.kids("pt") { - if let (Some(slot), Some(v)) = (idx(p).and_then(|i| vals.get_mut(i)), p.child("v")) { + let mut vals = vec![None; count]; + for (i, p) in placed() { + if let (Some(slot), Some(v)) = (vals.get_mut(i), p.child("v")) { *slot = Some(v.text.as_str()); } } - let items: Vec = vals + Some(vals) +} + +/// Points as an array constant. Missing points become `#N/A` (numbers) or `""` (text) so +/// positions line up. +fn literal>(points: &[Option], numeric: bool) -> String { + let items: Vec = points .iter() - .map(|v| match (v, numeric) { + .map(|v| match (v.as_ref().map(AsRef::as_ref), numeric) { (Some(t), true) => t.trim().parse::().ok().filter(|n| n.is_finite()).map_or_else(|| "#N/A".to_string(), num), (None, true) => "#N/A".to_string(), (Some(t), false) => format!("\"{}\"", t.replace('"', "\"\"")), (None, false) => "\"\"".to_string(), }) .collect(); - Some((count, format!("{{{}}}", items.join(",")))) + format!("{{{}}}", items.join(",")) +} + +/// The points cached under a PivotChart series reference. +#[derive(Clone, Debug)] +struct CachedPoints { + points: Vec>, + numeric: bool, + /// A `multiLvlStrRef`: one column (or row) of cells per level, the innermost last. The + /// cache holds the innermost level. + multi_level: bool, +} + +/// What a PivotChart series saved with its references, plotted when the referenced cells don't +/// hold it (see [`plot_cached`]). +#[derive(Clone, Debug, Default)] +pub(crate) struct PivotSeriesCache { + name: Option, + categories: Option, + values: Option, + /// The values' `numCache` format code. + number_format: Option, } -/// The points cached under a series reference: `numCache`, `strCache`, or the innermost level -/// of a `multiLvlStrCache`. -fn cache_literal(r: &El) -> Option<(u64, String)> { - if let Some(c) = r.child("numCache") { - points_literal(c, c, true) - } else if let Some(c) = r.child("strCache") { - points_literal(c, c, false) +/// `numCache`, `strCache`, or the innermost level of a `multiLvlStrCache`, under `cat` or `val`. +fn cached_points(e: Option<&El>) -> Option { + let e = e?; + let owned = |p: Vec>| p.into_iter().map(|v| v.map(str::to_string)).collect(); + if let Some(c) = e.path(&["numRef", "numCache"]) { + Some(CachedPoints { points: owned(read_points(c, c)?), numeric: true, multi_level: false }) + } else if let Some(c) = e.path(&["strRef", "strCache"]) { + Some(CachedPoints { points: owned(read_points(c, c)?), numeric: false, multi_level: false }) } else { - let c = r.child("multiLvlStrCache")?; - points_literal(c, c.child("lvl")?, false) + let c = e.path(&["multiLvlStrRef", "multiLvlStrCache"])?; + Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?)?), numeric: false, multi_level: true }) + } +} + +fn pivot_series_cache(s: &El) -> PivotSeriesCache { + PivotSeriesCache { + name: s.path(&["tx", "strRef", "strCache", "pt", "v"]).map(|v| v.text.clone()), + categories: cached_points(s.child("cat")), + values: cached_points(s.child("val")), + number_format: s + .path(&["val", "numRef", "numCache", "formatCode"]) + .map(|c| c.text.trim()) + .filter(|c| !c.is_empty() && !c.eq_ignore_ascii_case("General")) + .map(str::to_string), + } +} + +/// The references of a series formula: one area, or a union `(Sheet!$A$1:$A$3,Sheet!$A$5)`. +fn ref_areas<'e>(e: &'e Expr, out: &mut Vec<&'e Reference>) -> bool { + match e { + Expr::Ref(r) => { + out.push(r); + true + } + Expr::Paren(x) => ref_areas(x, out), + Expr::Binary(BinOp::Union, a, b) => ref_areas(a, out) && ref_areas(b, out), + _ => false, + } +} + +/// Whether a cell shows the cached point: numbers within rounding, text as is. A number cache +/// leaves out cells that hold no number. +fn point_matches(v: &Value, p: Option<&str>, numeric: bool) -> bool { + match (v, p) { + (Value::Number(n), Some(t)) => t.trim().parse::().is_ok_and(|c| c == *n || (c - n).abs() <= 1e-9 * c.abs().max(n.abs())), + (Value::Number(_), None) => false, + (_, None) => numeric || v.is_empty(), + (_, Some(_)) if numeric => false, + (Value::Text(s), Some(t)) => **s == *t, + (Value::Bool(b), Some(t)) => t.eq_ignore_ascii_case(if *b { "TRUE" } else { "FALSE" }), + (Value::Error(e), Some(t)) => e.as_str() == t, + (Value::Empty, Some(t)) => t.is_empty(), + (Value::Array(_), Some(_)) => false, + } +} + +/// Whether the cells a series formula refers to (on sheet `home` when unqualified) hold the +/// cached points, one cell per point. +fn live_matches(wb: &Workbook, home: usize, f: &str, p: &CachedPoints) -> bool { + let Ok(e) = gridcraft_formula::parse(f) else { return false }; + let mut refs = Vec::new(); + if !ref_areas(&e, &mut refs) { + return false; + } + let mut areas = Vec::with_capacity(refs.len()); + for r in refs { + let si = match &r.sheet { + SheetSel::Current => Some(home), + SheetSel::Named(n) => wb.sheet_index(n), + SheetSel::Span(..) => None, + }; + let Some(sheet) = si.and_then(|i| wb.sheet(i)) else { return false }; + areas.push((sheet, r.range())); + } + let n = p.points.len(); + if p.multi_level { + // Only the innermost level is cached: the last column of a block with a row per point, + // or the last row of one with a column per point. + let [(sheet, r)] = areas[..] else { return false }; + let leaf = if r.height() as usize == n { + RangeRef::new(CellRef::new(r.start.row, r.end.col), r.end) + } else if r.width() as usize == n { + RangeRef::new(CellRef::new(r.end.row, r.start.col), r.end) + } else { + return false; + }; + areas = vec![(sheet, leaf)]; + } + if areas.iter().try_fold(0u64, |t, (_, r)| t.checked_add(r.count())) != Some(n as u64) { + return false; } + areas.iter().flat_map(|(sheet, r)| r.iter().map(|c| sheet.value(c))).zip(&p.points).all(|(v, pt)| point_matches(&v, pt.as_deref(), p.numeric)) } /// A PivotChart's `` formulas only approximate its PivotTable: often just the table's corner /// cell, or a block that takes in the grand total row. Excel draws the chart from the PivotTable, -/// and the cached points are what it shows. When a series' categories or values don't cover their -/// cached points, the series plots the cached points (and its cached name) instead; refreshing -/// the PivotTable rebinds the chart to live cells. -fn use_pivot_cache(s: &El, ser: &mut Series) { - let cached = |tag: &str, f: Option<&str>| -> Option { - let e = s.child(tag)?; - let r = ["numRef", "strRef", "multiLvlStrRef"].iter().find_map(|k| e.child(k))?; - let (count, lit) = cache_literal(r)?; - (f.and_then(ref_cells) != Some(count)).then_some(lit) - }; - let categories = cached("cat", ser.categories.as_deref()); - let values = cached("val", Some(ser.values.as_str())); - if categories.is_none() && values.is_none() { - return; - } - ser.categories = categories.or(ser.categories.take()); - if let Some(v) = values { - ser.values = v; +/// and the cached points are what it shows. When the cells a series' categories or values refer +/// to (on sheet `si`) don't hold their cached points, the series plots the cached points (and its +/// cached name) instead; refreshing the PivotTable rebinds the chart to live cells. +pub(crate) fn plot_cached(wb: &Workbook, si: usize, ch: &mut Chart, caches: &[PivotSeriesCache]) { + for (ser, c) in ch.series.iter_mut().zip(caches) { + let stale = |f: Option<&str>, p: &&CachedPoints| !f.is_some_and(|f| live_matches(wb, si, f, p)); + let categories = c.categories.as_ref().filter(|p| stale(ser.categories.as_deref(), p)).map(|p| literal(&p.points, p.numeric)); + let values = c.values.as_ref().filter(|p| stale(Some(&ser.values), p)).map(|p| literal(&p.points, p.numeric)); + if categories.is_none() && values.is_none() { + continue; + } + if categories.is_some() { + ser.categories = categories; + } + if let Some(v) = values { + ser.values = v; + ser.number_format.clone_from(&c.number_format); + } + if let Some(name) = &c.name { + ser.name = Some(format!("\"{}\"", name.replace('"', "\"\""))); + } } - if let Some(name) = s.path(&["tx", "strRef", "strCache", "pt", "v"]) { - ser.name = Some(format!("\"{}\"", name.text.replace('"', "\"\""))); +} + +/// [`plot_cached`] for the PivotCharts read from a workbook (by chart id), once every sheet's +/// cells are in. +pub(crate) fn plot_pivot_caches(wb: &mut Workbook, pending: &[(u32, Vec)]) { + for (id, caches) in pending { + let at = wb.sheets.iter().enumerate().find_map(|(si, sh)| sh.charts.iter().position(|c| c.id == *id).map(|ci| (si, ci))); + let Some((si, ci)) = at else { continue }; + let Some(mut ch) = wb.sheet(si).and_then(|sh| sh.charts.get(ci)).cloned() else { continue }; + plot_cached(wb, si, &mut ch, caches); + if let Some(slot) = wb.sheet_mut(si).and_then(|sh| sh.charts.get_mut(ci)) { + *slot = ch; + } } } -fn read_series(s: &El, pivot: bool) -> Series { +fn read_series(s: &El) -> Series { let name = s.child("tx").and_then(|tx| { if let Some(f) = tx.path(&["strRef", "f"]) { let t = f.text.trim(); @@ -191,7 +287,8 @@ fn read_series(s: &El, pivot: bool) -> Series { .and_then(dml_color) .or_else(|| sp.and_then(|p| p.path(&["ln", "solidFill"])).and_then(dml_color)) .or_else(|| s.path(&["marker", "spPr", "solidFill"]).and_then(dml_color)); - let mut ser = Series { + let values = s.child("val").or_else(|| s.child("yVal")); + Series { name, categories: data_ref(s.child("cat")).or_else(|| data_ref(s.child("xVal"))), values: data_ref(s.child("val")).or_else(|| data_ref(s.child("yVal"))).unwrap_or_default(), @@ -201,22 +298,14 @@ fn read_series(s: &El, pivot: bool) -> Series { kind: None, smooth: bool_val(s, "smooth"), trendlines: s.kids("trendline").filter_map(read_trendline).collect(), - number_format: None, - }; - if pivot { - use_pivot_cache(s, &mut ser); - } - // Literal values (a `numLit`, or the cached points above) keep the format code saved with them. - if ser.values.starts_with('{') { - let v = s.child("val").or_else(|| s.child("yVal")); - ser.number_format = v - .and_then(|v| v.child("numLit").or_else(|| v.path(&["numRef", "numCache"]))) - .and_then(|c| c.child("formatCode")) + // Literal values keep the format code saved with them. + number_format: values + .filter(|v| v.child("numRef").is_none()) + .and_then(|v| v.path(&["numLit", "formatCode"])) .map(|c| c.text.trim()) .filter(|c| !c.is_empty() && !c.eq_ignore_ascii_case("General")) - .map(str::to_string); + .map(str::to_string), } - ser } const DASHES: [(LineDash, &str); 11] = [ @@ -291,7 +380,9 @@ fn axis_ids(g: &El) -> Vec { g.kids("axId").filter_map(|a| a.attr("val")).map(str::to_string).collect() } -pub fn read_chart(root: &El) -> Option { +/// A chart part, with the cached points of each series when it is a PivotChart (empty otherwise), +/// for [`plot_cached`] once the workbook's cells are read. +pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { let chart = root.child("chart")?; let plot = chart.child("plotArea")?; let groups: Vec<(&El, ChartKind)> = plot.children.iter().filter_map(|g| group_kind(g).map(|k| (g, k))).collect(); @@ -299,14 +390,18 @@ pub fn read_chart(root: &El) -> Option { let first_axes = axis_ids(first); let pivot = root.path(&["pivotSource", "name"]).map(|n| n.text.trim().to_string()).filter(|n| !n.is_empty()); let mut series = vec![]; + let mut caches = vec![]; for (gi, (g, k)) in groups.iter().enumerate() { for s in g.kids("ser") { - let mut ser = read_series(s, pivot.is_some()); + let mut ser = read_series(s); if gi > 0 { ser.kind = Some(*k); ser.secondary = axis_ids(g) != first_axes; } series.push(ser); + if pivot.is_some() { + caches.push(pivot_series_cache(s)); + } } } let kind = if groups.len() > 1 { ChartKind::Combo } else { first_kind }; @@ -331,7 +426,7 @@ pub fn read_chart(root: &El) -> Option { _ => LegendPos::Right, }, }; - Some(Chart { + let ch = Chart { id: 0, kind, anchor: Default::default(), @@ -351,7 +446,8 @@ pub fn read_chart(root: &El) -> Option { source: None, by_rows: false, pivot, - }) + }; + Some((ch, caches)) } // ---------------------------------------------------------------- writing diff --git a/crates/xlsx/src/drawing.rs b/crates/xlsx/src/drawing.rs index 6dbd17e7e..9460fe1be 100644 --- a/crates/xlsx/src/drawing.rs +++ b/crates/xlsx/src/drawing.rs @@ -166,10 +166,18 @@ pub fn read_drawing(cx: &mut Ctx<'_>, part: &str, sheet: &mut Sheet) -> Result<( let target = rel.target.clone(); if let Some(cx_xml) = cx.optional_xml(&target)? { let chartex = uri.contains("chartex"); - match if chartex { crate::chartex::read_chartex(&cx_xml) } else { crate::chart::read_chart(&cx_xml) } { - Some(mut ch) => { + let read = if chartex { + crate::chartex::read_chartex(&cx_xml).map(|c| (c, Vec::new())) + } else { + crate::chart::read_chart(&cx_xml) + }; + match read { + Some((mut ch, caches)) => { ch.id = cx.next_object_id(); ch.anchor = anchor; + if !caches.is_empty() { + cx.pivot_charts.push((ch.id, caches)); + } sheet.charts.push(ch); } None => cx.warn(format!("chart {target} has no supported plot and was skipped")), diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 1a479081c..7b991d8b0 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -30,6 +30,9 @@ pub struct Ctx<'a> { pub cell_pictures: Vec>>, /// x14 conditional-format ids of the current sheet's data bars, with the index of the rule in `cond_formats`. pub cf_ext_ids: Vec<(String, usize)>, + /// PivotCharts (by chart id) and their series' cached points, checked against the cells once + /// every sheet is read. + pub pivot_charts: Vec<(u32, Vec)>, } impl Ctx<'_> { @@ -82,6 +85,7 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { num_fmts: Default::default(), cell_pictures: vec![], cf_ext_ids: vec![], + pivot_charts: vec![], }; let root_rels = cx.pkg.rels("")?; let wb_part = find_rel(&root_rels, "officeDocument").map(|r| r.target.clone()).unwrap_or_else(|| "xl/workbook.xml".into()); @@ -246,6 +250,8 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { wb.sheets.push(Arc::new(Sheet::new("Sheet1"))); } wb.active_sheet = file_to_model.get(active_tab).copied().flatten().unwrap_or(0); + // PivotChart series whose references don't hold their cached points plot the cache. + crate::chart::plot_pivot_caches(&mut wb, &cx.pivot_charts); // Defined names. Chartex parts refer to their data through hidden `_xlchart.*` names. let mut chart_names: Vec<(String, String)> = vec![]; diff --git a/crates/xlsx/src/tests/chart_bool_default.rs b/crates/xlsx/src/tests/chart_bool_default.rs index e9a074cbe..f48454486 100644 --- a/crates/xlsx/src/tests/chart_bool_default.rs +++ b/crates/xlsx/src/tests/chart_bool_default.rs @@ -13,7 +13,7 @@ fn flags(ser: &str, group: &str) -> (bool, bool) { " ); let root = parse(xml.as_bytes()).unwrap(); - let ch = read_chart(&root).unwrap(); + let (ch, _) = read_chart(&root).unwrap(); (ch.series[0].smooth, ch.data_labels) } diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index c4c694a16..3a139738a 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -1,12 +1,34 @@ //! Reading charts as Excel saved them: PivotChart series and deleted automatic titles. -use gridcraft_model::{Chart, ChartKind, Workbook}; +use std::sync::Arc; + +use gridcraft_core::{CellRef, Value}; +use gridcraft_model::{Chart, ChartKind, Sheet, Workbook}; const NS: &str = r#"xmlns:c="http://schemas.openxmlformats.org/drawingml/2006/chart" xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main""#; +/// Reads a chart part as part of `wb` (sheet 0): PivotChart series whose cells don't hold their +/// cached points plot the cache. +fn read_in(wb: &Workbook, xml: &str) -> Chart { + let (mut c, caches) = crate::chart::read_chart(&crate::xml::parse(xml.as_bytes()).unwrap()).unwrap(); + crate::chart::plot_cached(wb, 0, &mut c, &caches); + c +} + fn read(xml: &str) -> Chart { - crate::chart::read_chart(&crate::xml::parse(xml.as_bytes()).unwrap()).unwrap() + read_in(&Workbook::new(), xml) +} + +/// A workbook with one sheet holding `cells` (A1 address, value). +fn book(name: &str, cells: &[(&str, Value)]) -> Workbook { + let mut sheet = Sheet::new(name); + for (a, v) in cells { + sheet.set_value(CellRef::parse(a).unwrap(), v.clone()); + } + let mut wb = Workbook::new(); + wb.sheets = vec![Arc::new(sheet)]; + wb } fn str_cache(f: &str, pts: &[&str]) -> String { @@ -69,9 +91,11 @@ fn pivot_charts_plot_their_cached_points_when_the_references_do_not_cover_them() let back = read(&crate::chart::write_chart(&Workbook::new(), 0, &c)); assert_eq!(back.series[0].number_format, c.series[0].number_format); - // References that cover the cached points stay live, and so do an ordinary chart's. + // References whose cells hold the cached points stay live, and so do an ordinary chart's. let live = ser(&str_cache("Summary!$B$4", &["Total"]), &str_cache("Summary!$A$5:$A$7", &cats), &num_cache("Summary!$B$5:$B$7", 3, &[(0, "1")])); - let c = read(&chart_xml(true, &format!(r#"{live}"#), "")); + let mut cells = vec![("B4", Value::text("Total")), ("B5", Value::Number(1.0))]; + cells.extend(["A5", "A6", "A7"].into_iter().zip(cats).map(|(a, t)| (a, Value::text(t)))); + let c = read_in(&book("Summary", &cells), &chart_xml(true, &format!(r#"{live}"#), "")); assert_eq!((c.series[0].name.as_deref(), c.series[0].values.as_str()), (Some("Summary!$B$4"), "Summary!$B$5:$B$7")); assert_eq!(c.series[0].number_format, None, "live references use their cells' format"); let plain = ser(&str_cache("Graph!$J$2", &["Events"]), &str_cache("Graph!$J$2", &cats), &num_cache("Graph!$J$2", 3, &[(0, "18")])); @@ -92,6 +116,86 @@ fn pivot_charts_plot_their_cached_points_when_the_references_do_not_cover_them() assert_eq!(parts(&back), parts(&c)); } +#[test] +fn a_one_point_pivot_chart_at_the_corner_cell_plots_its_cache() { + // A PivotTable with one row item: every formula points at the corner cell, which holds the + // table's header, and each cache has one point, like the cell count. Excel plots the cache. + let bar = ser(&str_cache("Data!$J$2", &["Events"]), &str_cache("Data!$J$2", &["Truck 1"]), &num_cache("Data!$J$2", 1, &[(0, "18")])); + let chart = chart_xml(true, &format!(r#"{bar}"#), ""); + let drawing = r#"0040"#; + let rel = |kind: &str, target: &str| { + format!( + r#""# + ) + }; + let (srels, drels) = (rel("drawing", "../drawings/drawing1.xml"), rel("chart", "../charts/chart1.xml")); + let body = r#"Sum of Events"#; + let parts = [ + ("xl/worksheets/_rels/sheet1.xml.rels", srels.as_str()), + ("xl/drawings/drawing1.xml", drawing), + ("xl/drawings/_rels/drawing1.xml.rels", drels.as_str()), + ("xl/charts/chart1.xml", chart.as_str()), + ]; + let (wb, _) = crate::read_xlsx(&super::minimal(body, &parts, "", "")).unwrap(); + let s = &wb.sheets[0].charts[0].series[0]; + assert_eq!((s.name.as_deref(), s.categories.as_deref(), s.values.as_str()), (Some("\"Events\""), Some(r#"{"Truck 1"}"#), "{18}")); +} + +#[test] +fn multi_level_pivot_categories_stay_live_when_their_cells_hold_the_cache() { + // Two levels of row labels in A4:B7, the outer one (country) on the left; the cache keeps + // the innermost level first. + let cities = ["Ottawa", "Toronto", "Boston", "NYC"]; + let leaf: String = cities.iter().enumerate().map(|(i, v)| format!(r#"{v}"#)).collect(); + let outer = r#"CanadaUSA"#; + let cat = format!( + r#"Graph!$A$4:$B$7{leaf}{outer}"# + ); + let val = num_cache("Graph!$C$4:$C$7", 4, &[(0, "1"), (1, "2"), (2, "3"), (3, "4")]); + let xml = chart_xml( + true, + &format!( + r#"{}"#, + ser(&str_cache("Graph!$C$3", &["Total"]), &cat, &val) + ), + "", + ); + let mut cells = vec![("A4", Value::text("Canada")), ("A6", Value::text("USA")), ("C3", Value::text("Total"))]; + for (i, city) in cities.iter().enumerate() { + cells.push((["B4", "B5", "B6", "B7"][i], Value::text(*city))); + cells.push((["C4", "C5", "C6", "C7"][i], Value::Number(i as f64 + 1.0))); + } + let wb = book("Graph", &cells); + let c = read_in(&wb, &xml); + let s = &c.series[0]; + assert_eq!((s.name.as_deref(), s.categories.as_deref(), s.values.as_str()), (Some("Graph!$C$3"), Some("Graph!$A$4:$B$7"), "Graph!$C$4:$C$7")); + // A save keeps the reference. + let back = read_in(&wb, &crate::chart::write_chart(&wb, 0, &c)); + assert_eq!(back.series[0].categories.as_deref(), Some("Graph!$A$4:$B$7")); + + // When the innermost level's cells show other labels, the cache is plotted. + cells.push(("B5", Value::text("Montreal"))); + let c = read_in(&book("Graph", &cells), &xml); + assert_eq!(c.series[0].categories.as_deref(), Some(r#"{"Ottawa","Toronto","Boston","NYC"}"#)); +} + +#[test] +fn literal_series_keep_their_points_without_a_valid_pt_count() { + // Not valid by the schema, but readable: the points present set the count, and points + // without an index go in document order. + let values = |lit: &str| { + let s = format!("{lit}"); + read(&chart_xml(false, &format!("{s}"), "")).series[0].values.clone() + }; + let pts = r#"12"#; + assert_eq!(values(&format!(r#"{pts}"#)), "{1,2}"); + assert_eq!(values(&format!(r#"{pts}"#)), "{1,2}"); + assert_eq!(values(&format!(r#"{pts}"#)), "{1,2}"); + assert_eq!(values("12"), "{1,2}"); + // A valid count still keeps trailing gaps. + assert_eq!(values(&format!(r#"{pts}"#)), "{1,2,#N/A}"); +} + #[test] fn a_deleted_automatic_title_stays_deleted() { let one = ser("Vehicles", &str_cache("S!$A$1:$A$2", &["a", "b"]), &num_cache("S!$B$1:$B$2", 2, &[])); diff --git a/crates/xlsx/src/tests/trendline_roundtrip.rs b/crates/xlsx/src/tests/trendline_roundtrip.rs index 49a6b07b7..8354db586 100644 --- a/crates/xlsx/src/tests/trendline_roundtrip.rs +++ b/crates/xlsx/src/tests/trendline_roundtrip.rs @@ -161,7 +161,7 @@ fn excel_style_trendlines_are_read() { // type that is skipped rather than failing the chart. let xml = r#"Data!$B$2:$B$6"#; let root = crate::xml::parse(xml.as_bytes()).unwrap(); - let c = crate::chart::read_chart(&root).unwrap(); + let (c, _) = crate::chart::read_chart(&root).unwrap(); let t = &c.series[0].trendlines; assert_eq!(t.len(), 2); assert_eq!(t[0].kind, TrendlineKind::Polynomial(3)); diff --git a/docs/chart-parity.md b/docs/chart-parity.md index a3340584c..6c9743800 100644 --- a/docs/chart-parity.md +++ b/docs/chart-parity.md @@ -55,7 +55,7 @@ open PRs #158, #161, and issue #190. | Recommended Charts | yes | partial (Insert ▸ Recommended) | | Switch Row/Column, Select Data | yes | yes (Switch on charts read from files landed in #116); Select Data dialog: data range, remove series, Horizontal (Category) Axis Labels (#287). No series add/edit or hidden/empty cell options | | Sparklines (line, column, win/loss, markers, axis) | yes | line, column, win/loss | -| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose references don't cover its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed; data labels and the value axis use the cache's number format, which is kept through a save | +| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose referenced cells don't hold its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed; data labels and the value axis use the cache's number format, which is kept through a save | | Animation on data change | yes (Windows) | no | ## XLSX round-trip @@ -80,7 +80,7 @@ back. | Date | Change | Summary | |---|---|---| -| 2026-10-13 | minor | XLSX: PivotChart series whose references don't cover their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format; `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save | +| 2026-10-13 | minor | XLSX: PivotChart series whose referenced cells don't hold their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format; `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save | | 2026-10-12 | trivial | chart sheets are kept on save, still not editable (#727) | | 2026-10-11 | minor | Trendlines: all six types modelled, round-tripped through `c:trendline`, drawn with forecast and equation/R² labels; `chart.addElement` `trendline` | | 2026-10-11 | minor | PivotCharts (`insert.pivotChart`) | From e16b95eb821e9bb70f809eaad85e3cbcdac0f046 Mon Sep 17 00:00:00 2001 From: Local contributor Date: Wed, 14 Oct 2026 08:22:29 -0400 Subject: [PATCH 03/12] XLSX: Multi-level PivotChart categories plot one label per point; quoted series names survive a save A PivotChart whose multiLvlStrRef categories cover two levels (Graph!$A$4:$B$7 for 4 points) stayed live when the innermost column matched the cache. The renderer reads every cell of that block, so the chart drew 8 labels for 4 points, and a save flattened it into an 8-point strRef. Such a block is now compared cell for cell like any other reference: its cell count isn't the point count, so the innermost level is plotted from the cache, one label per point, and saved as a strLit. A one-level reference whose cells hold the cache still stays live. A literal series name is stored as a quoted formula literal with doubled quotes ("Sales ""East"""), which stale PivotChart series now always get. The chart and chartex writers only stripped the outer quotes, so the name came back as Sales ""East"" after a save. Both writers now unescape the literal through one helper. --- crates/xlsx/src/chart.rs | 38 +++++------ crates/xlsx/src/chartex.rs | 3 +- crates/xlsx/src/tests/chart_read_tests.rs | 78 +++++++++++++++-------- docs/chart-parity.md | 4 +- 4 files changed, 71 insertions(+), 52 deletions(-) diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index 198487c96..f591fec93 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -3,7 +3,7 @@ use std::fmt::Write as _; -use gridcraft_core::{CellRef, RangeRef, Value}; +use gridcraft_core::Value; use gridcraft_formula::{BinOp, Expr, Reference, SheetSel}; use gridcraft_model::{Chart, ChartKind, Color, LegendPos, LineDash, Series, Trendline, TrendlineKind, Workbook}; @@ -122,9 +122,6 @@ fn literal>(points: &[Option], numeric: bool) -> String { struct CachedPoints { points: Vec>, numeric: bool, - /// A `multiLvlStrRef`: one column (or row) of cells per level, the innermost last. The - /// cache holds the innermost level. - multi_level: bool, } /// What a PivotChart series saved with its references, plotted when the referenced cells don't @@ -139,16 +136,18 @@ pub(crate) struct PivotSeriesCache { } /// `numCache`, `strCache`, or the innermost level of a `multiLvlStrCache`, under `cat` or `val`. +/// A `multiLvlStrRef` block of more than one level has a cell per level for each point, so its +/// cell count isn't the point count and the innermost level is plotted from the cache. fn cached_points(e: Option<&El>) -> Option { let e = e?; let owned = |p: Vec>| p.into_iter().map(|v| v.map(str::to_string)).collect(); if let Some(c) = e.path(&["numRef", "numCache"]) { - Some(CachedPoints { points: owned(read_points(c, c)?), numeric: true, multi_level: false }) + Some(CachedPoints { points: owned(read_points(c, c)?), numeric: true }) } else if let Some(c) = e.path(&["strRef", "strCache"]) { - Some(CachedPoints { points: owned(read_points(c, c)?), numeric: false, multi_level: false }) + Some(CachedPoints { points: owned(read_points(c, c)?), numeric: false }) } else { let c = e.path(&["multiLvlStrRef", "multiLvlStrCache"])?; - Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?)?), numeric: false, multi_level: true }) + Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?)?), numeric: false }) } } @@ -213,19 +212,6 @@ fn live_matches(wb: &Workbook, home: usize, f: &str, p: &CachedPoints) -> bool { areas.push((sheet, r.range())); } let n = p.points.len(); - if p.multi_level { - // Only the innermost level is cached: the last column of a block with a row per point, - // or the last row of one with a column per point. - let [(sheet, r)] = areas[..] else { return false }; - let leaf = if r.height() as usize == n { - RangeRef::new(CellRef::new(r.start.row, r.end.col), r.end) - } else if r.width() as usize == n { - RangeRef::new(CellRef::new(r.end.row, r.start.col), r.end) - } else { - return false; - }; - areas = vec![(sheet, leaf)]; - } if areas.iter().try_fold(0u64, |t, (_, r)| t.checked_add(r.count())) != Some(n as u64) { return false; } @@ -626,14 +612,22 @@ fn str_data(tag: &str, f: &str, wb: &Workbook, si: usize) -> String { s } +/// The text of a literal series name: a quoted formula literal (`"Sales ""East"""`) without its +/// quotes and with `""` read as `"`; bare text as is. +pub(crate) fn literal_text(n: &str) -> std::borrow::Cow<'_, str> { + match n.trim().strip_prefix('"').and_then(|t| t.strip_suffix('"')) { + Some(t) => t.replace("\"\"", "\"").into(), + None => n.into(), + } +} + fn series_xml(s: &Series, idx: usize, g: G, wb: &Workbook, si: usize) -> String { let mut x = format!(""); if let Some(n) = &s.name { if is_ref(n) { x.push_str(&str_data("tx", n, wb, si)); } else { - let lit = n.trim().strip_prefix('"').and_then(|t| t.strip_suffix('"')).unwrap_or(n); - let _ = write!(x, "{}", esc(lit)); + let _ = write!(x, "{}", esc(&literal_text(n))); } } let color = s.color.as_ref().and_then(|c: &Color| srgb(c, &wb.theme)); diff --git a/crates/xlsx/src/chartex.rs b/crates/xlsx/src/chartex.rs index 2ab8c221b..40f7d0956 100644 --- a/crates/xlsx/src/chartex.rs +++ b/crates/xlsx/src/chartex.rs @@ -169,8 +169,7 @@ fn tx_xml(t: &str, wb: &Workbook, si: usize, names: &mut Vec) -> String let v = crate::chart::ref_values(wb, si, t).and_then(|v| v.into_iter().next()).map(|v| v.display()).unwrap_or_default(); format!("{}{}", f_xml(wb, si, t, names), esc(&v)) } else { - let lit = t.trim().strip_prefix('"').and_then(|x| x.strip_suffix('"')).unwrap_or(t); - format!("{}", esc(lit)) + format!("{}", esc(&crate::chart::literal_text(t))) } } diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index 3a139738a..2b56ed619 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -107,12 +107,7 @@ fn pivot_charts_plot_their_cached_points_when_the_references_do_not_cover_them() let c = read(&chart_xml(true, &plot, "")); let back = read(&crate::chart::write_chart(&Workbook::new(), 0, &c)); assert_eq!(back.pivot, c.pivot); - let parts = |c: &Chart| { - c.series - .iter() - .map(|s| (s.name.as_deref().map(|n| n.trim_matches('"').to_string()), s.categories.clone(), s.values.clone())) - .collect::>() - }; + let parts = |c: &Chart| c.series.iter().map(|s| (shown(s.name.as_deref()), s.categories.clone(), s.values.clone())).collect::>(); assert_eq!(parts(&back), parts(&c)); } @@ -142,41 +137,72 @@ fn a_one_point_pivot_chart_at_the_corner_cell_plots_its_cache() { } #[test] -fn multi_level_pivot_categories_stay_live_when_their_cells_hold_the_cache() { +fn multi_level_pivot_categories_plot_their_innermost_level() { // Two levels of row labels in A4:B7, the outer one (country) on the left; the cache keeps // the innermost level first. let cities = ["Ottawa", "Toronto", "Boston", "NYC"]; let leaf: String = cities.iter().enumerate().map(|(i, v)| format!(r#"{v}"#)).collect(); let outer = r#"CanadaUSA"#; - let cat = format!( - r#"Graph!$A$4:$B$7{leaf}{outer}"# - ); - let val = num_cache("Graph!$C$4:$C$7", 4, &[(0, "1"), (1, "2"), (2, "3"), (3, "4")]); - let xml = chart_xml( - true, - &format!( - r#"{}"#, - ser(&str_cache("Graph!$C$3", &["Total"]), &cat, &val) - ), - "", - ); + let xml = |f: &str, levels: &str| { + let cat = + format!(r#"{f}{levels}"#); + let val = num_cache("Graph!$C$4:$C$7", 4, &[(0, "1"), (1, "2"), (2, "3"), (3, "4")]); + chart_xml( + true, + &format!( + r#"{}"#, + ser(&str_cache("Graph!$C$3", &["Total"]), &cat, &val) + ), + "", + ) + }; let mut cells = vec![("A4", Value::text("Canada")), ("A6", Value::text("USA")), ("C3", Value::text("Total"))]; for (i, city) in cities.iter().enumerate() { cells.push((["B4", "B5", "B6", "B7"][i], Value::text(*city))); cells.push((["C4", "C5", "C6", "C7"][i], Value::Number(i as f64 + 1.0))); } let wb = book("Graph", &cells); - let c = read_in(&wb, &xml); + // The two-level block would draw both levels' cells (8 labels for 4 points): the innermost + // level is plotted from the cache, one label per point. The values' cells hold their cache. + let two_levels = xml("Graph!$A$4:$B$7", &format!("{leaf}{outer}")); + let c = read_in(&wb, &two_levels); let s = &c.series[0]; - assert_eq!((s.name.as_deref(), s.categories.as_deref(), s.values.as_str()), (Some("Graph!$C$3"), Some("Graph!$A$4:$B$7"), "Graph!$C$4:$C$7")); - // A save keeps the reference. + let labels = r#"{"Ottawa","Toronto","Boston","NYC"}"#; + assert_eq!((s.categories.as_deref(), s.values.as_str()), (Some(labels), "Graph!$C$4:$C$7")); + // A save keeps one label per point. let back = read_in(&wb, &crate::chart::write_chart(&wb, 0, &c)); - assert_eq!(back.series[0].categories.as_deref(), Some("Graph!$A$4:$B$7")); + assert_eq!(back.series[0].categories.as_deref(), Some(labels)); - // When the innermost level's cells show other labels, the cache is plotted. + // A one-level reference whose cells hold the cache stays live; when a cell shows another + // label, the cache is plotted. + let one_level = xml("Graph!$B$4:$B$7", &format!("{leaf}")); + assert_eq!(read_in(&wb, &one_level).series[0].categories.as_deref(), Some("Graph!$B$4:$B$7")); cells.push(("B5", Value::text("Montreal"))); - let c = read_in(&book("Graph", &cells), &xml); - assert_eq!(c.series[0].categories.as_deref(), Some(r#"{"Ottawa","Toronto","Boston","NYC"}"#)); + assert_eq!(read_in(&book("Graph", &cells), &one_level).series[0].categories.as_deref(), Some(labels)); +} + +/// A series name as the chart shows it: a quoted literal without its quotes, `""` read as `"`. +fn shown(name: Option<&str>) -> Option { + name.map(|n| n.strip_prefix('"').and_then(|q| q.strip_suffix('"')).map_or_else(|| n.to_string(), |q| q.replace("\"\"", "\""))) +} + +#[test] +fn series_names_with_quotes_survive_a_save() { + // A stale PivotChart series takes its cached name, quotes and all. + let s = + ser(&str_cache("Graph!$J$2", &["Sales \"East\""]), &str_cache("Graph!$J$2", &["a", "b"]), &num_cache("Graph!$J$2", 2, &[(0, "1"), (1, "2")])); + let c = read(&chart_xml(true, &format!("{s}"), "")); + assert_eq!(shown(c.series[0].name.as_deref()).as_deref(), Some("Sales \"East\"")); + let back = read(&crate::chart::write_chart(&Workbook::new(), 0, &c)); + assert_eq!(shown(back.series[0].name.as_deref()).as_deref(), Some("Sales \"East\"")); + + // So does a literal name on a chartex chart. + let mut c = Chart { kind: ChartKind::Waterfall, ..c }; + c.series[0].categories = None; + let mut names = Vec::new(); + let xml = crate::chartex::write_chartex(&Workbook::new(), 0, &c, &mut names); + let back = crate::chartex::read_chartex(&crate::xml::parse(xml.as_bytes()).unwrap()).unwrap(); + assert_eq!(shown(back.series[0].name.as_deref()).as_deref(), Some("Sales \"East\"")); } #[test] diff --git a/docs/chart-parity.md b/docs/chart-parity.md index 6c9743800..3b0881141 100644 --- a/docs/chart-parity.md +++ b/docs/chart-parity.md @@ -55,7 +55,7 @@ open PRs #158, #161, and issue #190. | Recommended Charts | yes | partial (Insert ▸ Recommended) | | Switch Row/Column, Select Data | yes | yes (Switch on charts read from files landed in #116); Select Data dialog: data range, remove series, Horizontal (Category) Axis Labels (#287). No series add/edit or hidden/empty cell options | | Sparklines (line, column, win/loss, markers, axis) | yes | line, column, win/loss | -| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose referenced cells don't hold its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed; data labels and the value axis use the cache's number format, which is kept through a save | +| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose referenced cells don't hold its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed, as does a multi-level category block (its innermost level, one label per point); data labels and the value axis use the cache's number format, which is kept through a save | | Animation on data change | yes (Windows) | no | ## XLSX round-trip @@ -80,7 +80,7 @@ back. | Date | Change | Summary | |---|---|---| -| 2026-10-13 | minor | XLSX: PivotChart series whose referenced cells don't hold their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format; `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save | +| 2026-10-13 | minor | XLSX: PivotChart series whose referenced cells don't hold their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format, and multi-level category blocks plot their innermost level (they drew every level's cells); `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save, and literal series names keep their quotes | | 2026-10-12 | trivial | chart sheets are kept on save, still not editable (#727) | | 2026-10-11 | minor | Trendlines: all six types modelled, round-tripped through `c:trendline`, drawn with forecast and equation/R² labels; `chart.addElement` `trendline` | | 2026-10-11 | minor | PivotCharts (`insert.pivotChart`) | From ef6776db8b56493acc6d60d2ae40d83829e78750 Mon Sep 17 00:00:00 2001 From: Local contributor Date: Wed, 14 Oct 2026 12:45:59 -0400 Subject: [PATCH 04/12] XLSX: A matching multi-level PivotChart category block keeps its references and draws its innermost level Since the previous commit, a multiLvlStrRef block of more than one level was always stale, so a save replaced Graph!$A$4:$B$7 with an innermost-only strLit and the name ref Graph!$C$3 with a literal, losing the outer level. The block is again compared by its innermost column (or row). When that holds the cache, the series stays live, its references kept and saved as before, and the new Series.innermost_categories flag makes the renderer draw only the innermost level, one label per point. A block whose innermost level doesn't hold the cache still plots its cache. --- crates/chart/src/resolve.rs | 14 ++++-- crates/chart/src/tests.rs | 43 +++++++++++++++++++ crates/engine/src/cmd/forecast.rs | 1 + crates/engine/src/cmd/insert.rs | 2 + crates/engine/src/pivot.rs | 1 + crates/engine/src/sample.rs | 1 + crates/model/src/features.rs | 5 +++ crates/xlsx/src/chart.rs | 36 ++++++++++++---- crates/xlsx/src/tests/chart_read_tests.rs | 25 +++++++---- .../src/tests/literal_category_roundtrip.rs | 1 + crates/xlsx/src/tests/roundtrip.rs | 1 + crates/xlsx/src/tests/smooth_roundtrip.rs | 1 + crates/xlsx/src/tests/trendline_roundtrip.rs | 1 + docs/chart-parity.md | 4 +- 14 files changed, 115 insertions(+), 21 deletions(-) diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index c3f8b8ed5..d1c789898 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -133,13 +133,21 @@ fn value_text(v: &Value) -> String { } } -/// Category labels, formatted with each source cell's number format (dates, currency…). -fn category_texts(wb: &Workbook, sheet: usize, formula: &str) -> Vec { +/// Category labels, formatted with each source cell's number format (dates, currency…). With +/// `innermost` (the point count of a multi-level block), only the block's last column (a row +/// per point) or last row (a column per point) is read: its innermost level. +fn category_texts(wb: &Workbook, sheet: usize, formula: &str, innermost: Option) -> Vec { let vals = eval_list(wb, sheet, formula); let cells: Option<(usize, RangeRef)> = parse_ref(wb, sheet, formula); let src = cells.and_then(|(sh, r)| wb.sheet(sh).map(|s| (s, r))); + let keep = |i: usize| match (innermost, cells) { + (Some(n), Some((_, r))) if r.width() > 1 && r.height() as usize == n => i % r.width() as usize == r.width() as usize - 1, + (Some(n), Some((_, r))) if r.height() > 1 && r.width() as usize == n => i / r.width() as usize == r.height() as usize - 1, + _ => true, + }; vals.iter() .enumerate() + .filter(|(i, _)| keep(*i)) .map(|(i, v)| { if let (Some((s, r)), Value::Number(_)) = (src, v) { let w = r.width().max(1) as usize; @@ -211,7 +219,7 @@ pub fn resolve(wb: &Workbook, sheet: usize, chart: &Chart) -> ChartData { if categories.is_none() && let Some(f) = &s.categories { - let c = category_texts(wb, sheet, f); + let c = category_texts(wb, sheet, f, s.innermost_categories.then_some(values.len())); if !c.is_empty() { categories = Some(c); } diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index de8f47285..61fea79a4 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -231,6 +231,7 @@ fn resolve_from_workbook() { trendlines: Vec::new(), // References use their cells' format, whatever the series says. number_format: Some("0%".into()), + innermost_categories: false, }, Series { name: None, @@ -243,6 +244,7 @@ fn resolve_from_workbook() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }, Series { name: Some("Literal".into()), @@ -255,6 +257,7 @@ fn resolve_from_workbook() { smooth: false, trendlines: Vec::new(), number_format: Some("0.0%".into()), + innermost_categories: false, }, ]; let d = resolve(&wb, 0, &c); @@ -295,6 +298,7 @@ fn resolve_from_workbook() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }, Series { name: None, @@ -307,6 +311,7 @@ fn resolve_from_workbook() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }, ]; let d = resolve(&wb, 0, &c); @@ -324,6 +329,7 @@ fn resolve_from_workbook() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }]; let d = resolve(&wb, 0, &c); assert_eq!(d.series[0].x.as_ref().map(|x| x.len()), Some(3)); @@ -342,6 +348,7 @@ fn resolve_from_workbook() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }]; let d = resolve(&wb, 7, &c); assert_eq!(d.series.len(), 1); @@ -1013,6 +1020,7 @@ fn pie_points_follow_workbook_theme() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }]; let d = resolve(&wb, 0, &c); let expected = vec![series_color(&wb.theme, 0), series_color(&wb.theme, 1)]; @@ -1067,3 +1075,38 @@ fn area_charts_draw_source_value_labels_only_when_enabled() { assert!(first_label > last_poly, "{kind:?}: label drawn under a later area"); } } + +#[test] +fn a_multi_level_category_block_draws_its_innermost_level() { + // A PivotChart's two levels of row labels in A1:B4, the outer one (country) on the left: + // one label per point, from the innermost column. + let mut wb = Workbook::new(); + { + let s = wb.sheet_mut(0).unwrap(); + s.name = "Graph".into(); + for (r, (outer, inner)) in [("Canada", "Ottawa"), ("", "Toronto"), ("USA", "Boston"), ("", "NYC")].iter().enumerate() { + if !outer.is_empty() { + s.set_value(CellRef::new(r as u32, 0), Value::from(*outer)); + } + s.set_value(CellRef::new(r as u32, 1), Value::from(*inner)); + s.set_value(CellRef::new(r as u32, 2), Value::Number(r as f64 + 1.0)); + s.set_value(CellRef::new(5, r as u32), Value::from(*inner)); + s.set_value(CellRef::new(6, r as u32), Value::from(*outer)); + } + } + let mut c = chart(ChartKind::ColumnClustered); + c.series = vec![Series { + categories: Some("Graph!$A$1:$B$4".into()), + values: "Graph!$C$1:$C$4".into(), + innermost_categories: true, + ..Series::default() + }]; + assert_eq!(resolve(&wb, 0, &c).categories, vec!["Ottawa", "Toronto", "Boston", "NYC"]); + // A block with a column per point: its last row is the innermost level. + c.series[0].categories = Some("Graph!$A$6:$D$7".into()); + assert_eq!(resolve(&wb, 0, &c).categories, vec!["Canada", "", "USA", ""]); + // Without the flag, every cell is a label, as for any other chart. + c.series[0].categories = Some("Graph!$A$1:$B$4".into()); + c.series[0].innermost_categories = false; + assert_eq!(resolve(&wb, 0, &c).categories.len(), 8); +} diff --git a/crates/engine/src/cmd/forecast.rs b/crates/engine/src/cmd/forecast.rs index fbed59aad..70d5a3c90 100644 --- a/crates/engine/src/cmd/forecast.rs +++ b/crates/engine/src/cmd/forecast.rs @@ -414,6 +414,7 @@ fn forecast_sheet(s: &mut Session, p: &Json) -> Result { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }) .collect(); sheet.charts.push(Chart { diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index f6ab29591..95e88e4b4 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -606,6 +606,7 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }); } } else { @@ -622,6 +623,7 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }); } } diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index cc167fb7e..77fa8b235 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -1570,6 +1570,7 @@ pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }) }) .collect() diff --git a/crates/engine/src/sample.rs b/crates/engine/src/sample.rs index f3dee6489..775a8bed3 100644 --- a/crates/engine/src/sample.rs +++ b/crates/engine/src/sample.rs @@ -111,6 +111,7 @@ fn sales(s: &mut Session) -> bool { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }]; } } diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index 4ed6bdb7c..f334dfcc9 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -428,6 +428,11 @@ pub struct Series { /// points). Values that reference cells use the cells' format instead. #[serde(default)] pub number_format: Option, + /// The categories are a multi-level block (a PivotChart's `multiLvlStrRef`): a column (or + /// row) of cells per level, the innermost last. Only the innermost level is drawn, one + /// label per point; the reference itself is kept and saved as is. + #[serde(default)] + pub innermost_categories: bool, } /// Regression a trendline fits (`c:trendlineType`). diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index f591fec93..280586b05 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -3,7 +3,7 @@ use std::fmt::Write as _; -use gridcraft_core::Value; +use gridcraft_core::{CellRef, RangeRef, Value}; use gridcraft_formula::{BinOp, Expr, Reference, SheetSel}; use gridcraft_model::{Chart, ChartKind, Color, LegendPos, LineDash, Series, Trendline, TrendlineKind, Workbook}; @@ -122,6 +122,9 @@ fn literal>(points: &[Option], numeric: bool) -> String { struct CachedPoints { points: Vec>, numeric: bool, + /// A `multiLvlStrRef` of more than one level: one column (or row) of cells per level, the + /// innermost last. The cache holds the innermost level. + multi_level: bool, } /// What a PivotChart series saved with its references, plotted when the referenced cells don't @@ -136,18 +139,17 @@ pub(crate) struct PivotSeriesCache { } /// `numCache`, `strCache`, or the innermost level of a `multiLvlStrCache`, under `cat` or `val`. -/// A `multiLvlStrRef` block of more than one level has a cell per level for each point, so its -/// cell count isn't the point count and the innermost level is plotted from the cache. fn cached_points(e: Option<&El>) -> Option { let e = e?; let owned = |p: Vec>| p.into_iter().map(|v| v.map(str::to_string)).collect(); if let Some(c) = e.path(&["numRef", "numCache"]) { - Some(CachedPoints { points: owned(read_points(c, c)?), numeric: true }) + Some(CachedPoints { points: owned(read_points(c, c)?), numeric: true, multi_level: false }) } else if let Some(c) = e.path(&["strRef", "strCache"]) { - Some(CachedPoints { points: owned(read_points(c, c)?), numeric: false }) + Some(CachedPoints { points: owned(read_points(c, c)?), numeric: false, multi_level: false }) } else { let c = e.path(&["multiLvlStrRef", "multiLvlStrCache"])?; - Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?)?), numeric: false }) + let multi_level = c.kids("lvl").nth(1).is_some(); + Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?)?), numeric: false, multi_level }) } } @@ -194,7 +196,7 @@ fn point_matches(v: &Value, p: Option<&str>, numeric: bool) -> bool { } /// Whether the cells a series formula refers to (on sheet `home` when unqualified) hold the -/// cached points, one cell per point. +/// cached points, one cell per point. Of a multi-level block, the innermost level is compared. fn live_matches(wb: &Workbook, home: usize, f: &str, p: &CachedPoints) -> bool { let Ok(e) = gridcraft_formula::parse(f) else { return false }; let mut refs = Vec::new(); @@ -212,6 +214,19 @@ fn live_matches(wb: &Workbook, home: usize, f: &str, p: &CachedPoints) -> bool { areas.push((sheet, r.range())); } let n = p.points.len(); + if p.multi_level { + // The last column of a block with a row per point, or the last row of one with a + // column per point. + let [(sheet, r)] = areas[..] else { return false }; + let leaf = if r.height() as usize == n { + RangeRef::new(CellRef::new(r.start.row, r.end.col), r.end) + } else if r.width() as usize == n { + RangeRef::new(CellRef::new(r.end.row, r.start.col), r.end) + } else { + return false; + }; + areas = vec![(sheet, leaf)]; + } if areas.iter().try_fold(0u64, |t, (_, r)| t.checked_add(r.count())) != Some(n as u64) { return false; } @@ -222,12 +237,16 @@ fn live_matches(wb: &Workbook, home: usize, f: &str, p: &CachedPoints) -> bool { /// cell, or a block that takes in the grand total row. Excel draws the chart from the PivotTable, /// and the cached points are what it shows. When the cells a series' categories or values refer /// to (on sheet `si`) don't hold their cached points, the series plots the cached points (and its -/// cached name) instead; refreshing the PivotTable rebinds the chart to live cells. +/// cached name) instead; refreshing the PivotTable rebinds the chart to live cells. A live +/// multi-level category block keeps its reference and draws only its innermost level. pub(crate) fn plot_cached(wb: &Workbook, si: usize, ch: &mut Chart, caches: &[PivotSeriesCache]) { for (ser, c) in ch.series.iter_mut().zip(caches) { let stale = |f: Option<&str>, p: &&CachedPoints| !f.is_some_and(|f| live_matches(wb, si, f, p)); let categories = c.categories.as_ref().filter(|p| stale(ser.categories.as_deref(), p)).map(|p| literal(&p.points, p.numeric)); let values = c.values.as_ref().filter(|p| stale(Some(&ser.values), p)).map(|p| literal(&p.points, p.numeric)); + if categories.is_none() && c.categories.as_ref().is_some_and(|p| p.multi_level) { + ser.innermost_categories = true; + } if categories.is_none() && values.is_none() { continue; } @@ -291,6 +310,7 @@ fn read_series(s: &El) -> Series { .map(|c| c.text.trim()) .filter(|c| !c.is_empty() && !c.eq_ignore_ascii_case("General")) .map(str::to_string), + innermost_categories: false, } } diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index 2b56ed619..851a2ed11 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -162,23 +162,32 @@ fn multi_level_pivot_categories_plot_their_innermost_level() { cells.push((["C4", "C5", "C6", "C7"][i], Value::Number(i as f64 + 1.0))); } let wb = book("Graph", &cells); - // The two-level block would draw both levels' cells (8 labels for 4 points): the innermost - // level is plotted from the cache, one label per point. The values' cells hold their cache. + // The innermost column of the two-level block holds the cache: the series stays live, its + // references (categories and name) kept as saved, and only the innermost level is drawn. let two_levels = xml("Graph!$A$4:$B$7", &format!("{leaf}{outer}")); let c = read_in(&wb, &two_levels); let s = &c.series[0]; + assert_eq!( + (s.name.as_deref(), s.categories.as_deref(), s.values.as_str(), s.innermost_categories), + (Some("Graph!$C$3"), Some("Graph!$A$4:$B$7"), "Graph!$C$4:$C$7", true) + ); + // A save keeps the references, outer level and all. + let saved = crate::chart::write_chart(&wb, 0, &c); + assert!(saved.contains("Graph!$A$4:$B$7") && saved.contains("Graph!$C$3"), "{saved}"); + + // When the innermost column shows another label, the block is stale: the innermost level + // is plotted from the cache, one label per point. let labels = r#"{"Ottawa","Toronto","Boston","NYC"}"#; - assert_eq!((s.categories.as_deref(), s.values.as_str()), (Some(labels), "Graph!$C$4:$C$7")); - // A save keeps one label per point. - let back = read_in(&wb, &crate::chart::write_chart(&wb, 0, &c)); - assert_eq!(back.series[0].categories.as_deref(), Some(labels)); + let mut moved = cells.clone(); + moved.push(("B5", Value::text("Montreal"))); + let s = read_in(&book("Graph", &moved), &two_levels).series[0].clone(); + assert_eq!((s.categories.as_deref(), s.innermost_categories), (Some(labels), false)); // A one-level reference whose cells hold the cache stays live; when a cell shows another // label, the cache is plotted. let one_level = xml("Graph!$B$4:$B$7", &format!("{leaf}")); assert_eq!(read_in(&wb, &one_level).series[0].categories.as_deref(), Some("Graph!$B$4:$B$7")); - cells.push(("B5", Value::text("Montreal"))); - assert_eq!(read_in(&book("Graph", &cells), &one_level).series[0].categories.as_deref(), Some(labels)); + assert_eq!(read_in(&book("Graph", &moved), &one_level).series[0].categories.as_deref(), Some(labels)); } /// A series name as the chart shows it: a quoted literal without its quotes, `""` read as `"`. diff --git a/crates/xlsx/src/tests/literal_category_roundtrip.rs b/crates/xlsx/src/tests/literal_category_roundtrip.rs index 1cb40f1c9..7601df18e 100644 --- a/crates/xlsx/src/tests/literal_category_roundtrip.rs +++ b/crates/xlsx/src/tests/literal_category_roundtrip.rs @@ -31,6 +31,7 @@ fn literal_category_with_separators_survives_save() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }) .collect(); s.charts.push(Chart { diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 1d44c1a38..2bd08d180 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -383,6 +383,7 @@ fn sample() -> Workbook { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }], legend: LegendPos::Bottom, data_labels: true, diff --git a/crates/xlsx/src/tests/smooth_roundtrip.rs b/crates/xlsx/src/tests/smooth_roundtrip.rs index bb10a7c80..f6a1b2542 100644 --- a/crates/xlsx/src/tests/smooth_roundtrip.rs +++ b/crates/xlsx/src/tests/smooth_roundtrip.rs @@ -20,6 +20,7 @@ fn smooth_line_setting_round_trips() { smooth: false, trendlines: Vec::new(), number_format: None, + innermost_categories: false, }; s.charts.push(Chart { id: 1, diff --git a/crates/xlsx/src/tests/trendline_roundtrip.rs b/crates/xlsx/src/tests/trendline_roundtrip.rs index 8354db586..8b1545866 100644 --- a/crates/xlsx/src/tests/trendline_roundtrip.rs +++ b/crates/xlsx/src/tests/trendline_roundtrip.rs @@ -50,6 +50,7 @@ fn chart(id: u32, kind: ChartKind, trendlines: Vec) -> Chart { smooth: false, trendlines, number_format: None, + innermost_categories: false, }], legend: LegendPos::Bottom, data_labels: false, diff --git a/docs/chart-parity.md b/docs/chart-parity.md index 3b0881141..04a1a811f 100644 --- a/docs/chart-parity.md +++ b/docs/chart-parity.md @@ -55,7 +55,7 @@ open PRs #158, #161, and issue #190. | Recommended Charts | yes | partial (Insert ▸ Recommended) | | Switch Row/Column, Select Data | yes | yes (Switch on charts read from files landed in #116); Select Data dialog: data range, remove series, Horizontal (Category) Axis Labels (#287). No series add/edit or hidden/empty cell options | | Sparklines (line, column, win/loss, markers, axis) | yes | line, column, win/loss | -| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose referenced cells don't hold its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed, as does a multi-level category block (its innermost level, one label per point); data labels and the value axis use the cache's number format, which is kept through a save | +| PivotCharts | yes | Insert ▸ PivotChart (`insert.pivotChart`): bound to a PivotTable's report, rebuilt on refresh and layout changes, `c:pivotSource` round trip; no field buttons, innermost category level only. Opened from a file, a series whose referenced cells don't hold its cached points (Excel often points them at the PivotTable's corner cell, or takes in the grand total row) plots the cached points until the PivotTable is refreshed. A multi-level category block is compared by its innermost column (or row): when that holds the cache, the reference is kept and only the innermost level is drawn, one label per point; otherwise the innermost level is plotted from the cache; data labels and the value axis use the cache's number format, which is kept through a save | | Animation on data change | yes (Windows) | no | ## XLSX round-trip @@ -80,7 +80,7 @@ back. | Date | Change | Summary | |---|---|---| -| 2026-10-13 | minor | XLSX: PivotChart series whose referenced cells don't hold their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format, and multi-level category blocks plot their innermost level (they drew every level's cells); `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save, and literal series names keep their quotes | +| 2026-10-13 | minor | XLSX: PivotChart series whose referenced cells don't hold their cached points plot the cached points (they drew empty or with the grand total), in the cache's number format, and multi-level category blocks draw only their innermost level, one label per point (they drew every level's cells); `autoTitleDeleted` keeps a deleted automatic title off; array-constant series keep quoted text, gaps and their number format through a save, and literal series names keep their quotes | | 2026-10-12 | trivial | chart sheets are kept on save, still not editable (#727) | | 2026-10-11 | minor | Trendlines: all six types modelled, round-tripped through `c:trendline`, drawn with forecast and equation/R² labels; `chart.addElement` `trendline` | | 2026-10-11 | minor | PivotCharts (`insert.pivotChart`) | From 1ba5d323d0fe3930a352abef3ad91808051e484e Mon Sep 17 00:00:00 2001 From: Local contributor Date: Wed, 14 Oct 2026 13:14:38 -0400 Subject: [PATCH 05/12] XLSX: Multi-level PivotChart categories save as a multiLvlStrRef and still draw one label per point after a reopen innermost_categories wasn't saved: the block was written as a 2-column strRef with an interleaved 8-point cache, so after save and reopen the chart drew 8 labels again. Flagged categories are now written as a multiLvlStrRef whose multiLvlStrCache has ptCount = the point count and one lvl of cell texts per level, innermost first, which a PivotChart reads back with the flag set. Categories without the flag are written as before. --- crates/engine/src/tests.rs | 49 ++++++++++++++++++++++ crates/model/src/features.rs | 3 +- crates/xlsx/src/chart.rs | 51 ++++++++++++++++++++++- crates/xlsx/src/tests/chart_read_tests.rs | 10 ++++- 4 files changed, 109 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..9b99a5ac8 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3282,3 +3282,52 @@ fn paste_special_validation_moves_relative_references() { assert!(!ok(&mut s, "C3", "5"), "the pasted rule checks C3, not A1"); assert!(ok(&mut s, "C3", "20")); } + +#[test] +fn a_multi_level_pivot_chart_draws_one_label_per_point_after_saves() { + use gridcraft_model::{Anchor, Chart, ChartKind, LegendPos, Series, Workbook}; + // Two levels of row labels in A4:B7, the outer one (country) on the left, as a PivotChart + // opened from a file keeps them when the innermost column holds its cache. + let mut wb = Workbook::new(); + let sh = wb.sheet_mut(0).unwrap(); + sh.name = "Graph".into(); + sh.set_value(CellRef::new(2, 2), Value::from("Total")); + for (i, (outer, inner)) in [("Canada", "Ottawa"), ("", "Toronto"), ("USA", "Boston"), ("", "NYC")].iter().enumerate() { + let r = 3 + i as u32; + if !outer.is_empty() { + sh.set_value(CellRef::new(r, 0), Value::from(*outer)); + } + sh.set_value(CellRef::new(r, 1), Value::from(*inner)); + sh.set_value(CellRef::new(r, 2), Value::Number(i as f64 + 1.0)); + } + sh.charts.push(Chart { + id: 1, + kind: ChartKind::ColumnClustered, + anchor: Anchor { cell: CellRef::new(0, 5), width: 480.0, height: 300.0, ..Anchor::default() }, + title: None, + series: vec![Series { + name: Some("Graph!$C$3".into()), + categories: Some("Graph!$A$4:$B$7".into()), + values: "Graph!$C$4:$C$7".into(), + innermost_categories: true, + ..Series::default() + }], + legend: LegendPos::Bottom, + data_labels: false, + gridlines: true, + style: 2, + x_title: None, + y_title: None, + source: None, + by_rows: false, + pivot: Some("Graph!PivotTable1".into()), + }); + let labels = ["Ottawa", "Toronto", "Boston", "NYC"]; + for _ in 0..2 { + wb = gridcraft_xlsx::read_xlsx(&gridcraft_xlsx::write_xlsx(&wb).unwrap()).unwrap().0; + let ch = &wb.sheet(0).unwrap().charts[0]; + let s = &ch.series[0]; + assert_eq!((s.name.as_deref(), s.categories.as_deref(), s.innermost_categories), (Some("Graph!$C$3"), Some("Graph!$A$4:$B$7"), true)); + assert_eq!(gridcraft_chart::resolve(&wb, 0, ch).categories, labels); + } +} diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index f334dfcc9..70353ad66 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -430,7 +430,8 @@ pub struct Series { pub number_format: Option, /// The categories are a multi-level block (a PivotChart's `multiLvlStrRef`): a column (or /// row) of cells per level, the innermost last. Only the innermost level is drawn, one - /// label per point; the reference itself is kept and saved as is. + /// label per point. The reference is kept, and XLSX saves it as a `multiLvlStrRef` with a + /// cache of every level; a PivotChart reads that back with this flag set again. #[serde(default)] pub innermost_categories: bool, } diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index 280586b05..5a33c5df8 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -607,6 +607,54 @@ fn split_array_items(inner: &str) -> Vec { items } +/// The point count of a series' values: an array constant's items or a reference's cells. +fn point_count(f: &str, wb: &Workbook, si: usize) -> Option { + let f = f.trim(); + if f.starts_with('{') { Some(array_items(f).len()) } else { ref_values(wb, si, f).map(|v| v.len()) } +} + +/// Multi-level categories ([`Series::innermost_categories`]) as a `multiLvlStrRef`, so they read +/// back as such: one `` of cell texts per column (a block with a row per point) or row (a +/// column per point), innermost (last) first. `None` when `f` isn't one area of `points` rows or +/// columns. +fn multi_level_data(f: &str, points: usize, wb: &Workbook, si: usize) -> Option { + let f = f.trim(); + let Expr::Ref(r) = gridcraft_formula::parse(f).ok()? else { return None }; + let sheet = match &r.sheet { + SheetSel::Current => wb.sheet(si), + SheetSel::Named(n) => wb.sheet_index(n).and_then(|i| wb.sheet(i)), + SheetSel::Span(..) => None, + }?; + let range = r.range(); + if range.count() > MAX_CACHE { + return None; + } + let (rows, cols) = (range.height() as usize, range.width() as usize); + // (level, point) to the cell's offset in the block. + let (levels, at): (usize, fn(usize, usize) -> (usize, usize)) = if rows == points && cols > 1 { + (cols, |l, p| (p, l)) + } else if cols == points && rows > 1 { + (rows, |l, p| (l, p)) + } else { + return None; + }; + let mut s = + format!("{}", esc(&crate::fmla::text_to_file(f))); + for l in (0..levels).rev() { + s.push_str(""); + for p in 0..points { + let (dr, dc) = at(l, p); + let v = range.start.offset(dr as i64, dc as i64).map(|c| sheet.value(c)).unwrap_or_default(); + if !v.is_empty() { + let _ = write!(s, "{}", esc(&v.display())); + } + } + s.push_str(""); + } + s.push_str(""); + Some(s) +} + fn str_data(tag: &str, f: &str, wb: &Workbook, si: usize) -> String { let f = f.trim(); if f.starts_with('{') { @@ -696,7 +744,8 @@ fn series_xml(s: &Series, idx: usize, g: G, wb: &Workbook, si: usize) -> String } _ => { if let Some(c) = &s.categories { - x.push_str(&str_data("cat", c, wb, si)); + let multi = s.innermost_categories.then(|| point_count(&s.values, wb, si).and_then(|n| multi_level_data(c, n, wb, si))).flatten(); + x.push_str(&multi.unwrap_or_else(|| str_data("cat", c, wb, si))); } x.push_str(&num_data("val", Some(&s.values), s.number_format.as_deref(), wb, si)); if matches!(g, G::Line { .. }) { diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index 851a2ed11..7f9a5e21c 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -171,9 +171,15 @@ fn multi_level_pivot_categories_plot_their_innermost_level() { (s.name.as_deref(), s.categories.as_deref(), s.values.as_str(), s.innermost_categories), (Some("Graph!$C$3"), Some("Graph!$A$4:$B$7"), "Graph!$C$4:$C$7", true) ); - // A save keeps the references, outer level and all. + // A save keeps the references and writes the block as a `multiLvlStrRef` whose cache has + // both levels, innermost first, so it reads back the same way. let saved = crate::chart::write_chart(&wb, 0, &c); - assert!(saved.contains("Graph!$A$4:$B$7") && saved.contains("Graph!$C$3"), "{saved}"); + let cat = format!( + r#"Graph!$A$4:$B$7{leaf}{outer}"# + ); + assert!(saved.contains(&cat) && saved.contains("Graph!$C$3"), "{saved}"); + let back = read_in(&wb, &saved).series[0].clone(); + assert_eq!((back.name, back.categories, back.innermost_categories), (s.name.clone(), s.categories.clone(), true)); // When the innermost column shows another label, the block is stale: the innermost level // is plotted from the cache, one label per point. From b56c2ddc7ead16cc1b7cb50cdcb0e82258f55be4 Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 05:20:59 +0000 Subject: [PATCH 06/12] Fix PivotChart innermost category labels --- crates/chart/src/resolve.rs | 9 +++++---- crates/chart/src/tests.rs | 12 +++++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index fa147d717..6b2a0a5cc 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -137,7 +137,7 @@ fn value_text(v: &Value) -> String { /// the outer levels of a multi-level axis: a category range wider than a column of values (or /// taller than a row of values) groups its innermost labels, year beside month. Each outer level /// has one entry per category, non-empty where a group starts, as the cells are. -fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str) -> (Vec, Vec>) { +fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str, innermost: bool) -> (Vec, Vec>) { let vals = eval_list(wb, sheet, formula); let cells: Option<(usize, RangeRef)> = parse_ref(wb, sheet, formula); let src = cells.and_then(|(sh, r)| wb.sheet(sh).map(|s| (s, r))); @@ -177,11 +177,12 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str) -> ( levels.by_columns }; let outer = if by_columns { w.saturating_sub(1).min(3) } else { h.saturating_sub(1).min(3) }; - if by_columns { + let result = if by_columns { (column(w - 1), (1..=outer).filter_map(|k| (w - 1).checked_sub(k)).map(column).collect()) } else { (row(h - 1), (1..=outer).filter_map(|k| (h - 1).checked_sub(k)).map(row).collect()) - } + }; + if innermost { (result.0, vec![]) } else { result } } /// Whether `t` names a defined name seen from `sheet` (`Name`, or `Sheet1!Name` looked up on @@ -250,7 +251,7 @@ pub fn resolve(wb: &Workbook, sheet: usize, chart: &Chart) -> ChartData { if categories.is_none() && let Some(f) = &s.categories { - let (c, outer) = category_table(wb, sheet, f, &s.values); + let (c, outer) = category_table(wb, sheet, f, &s.values, s.innermost_categories); if !c.is_empty() { categories = Some(c); category_levels = outer; diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 1fbdc75fd..01a7a143f 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1140,14 +1140,20 @@ fn a_multi_level_category_block_draws_its_innermost_level() { innermost_categories: true, ..Series::default() }]; - assert_eq!(resolve(&wb, 0, &c).categories, vec!["Ottawa", "Toronto", "Boston", "NYC"]); + let data = resolve(&wb, 0, &c); + assert_eq!(data.categories, vec!["Ottawa", "Toronto", "Boston", "NYC"]); + assert!(data.category_levels.is_empty(), "innermost PivotChart must not expose outer labels"); // A block with a column per point: its last row is the innermost level. c.series[0].categories = Some("Graph!$A$6:$D$7".into()); - assert_eq!(resolve(&wb, 0, &c).categories, vec!["Canada", "", "USA", ""]); + let data = resolve(&wb, 0, &c); + assert_eq!(data.categories, vec!["Canada", "", "USA", ""]); + assert!(data.category_levels.is_empty(), "innermost PivotChart must not expose outer labels"); // Without the flag, every cell is a label, as for any other chart. c.series[0].categories = Some("Graph!$A$1:$B$4".into()); c.series[0].innermost_categories = false; - assert_eq!(resolve(&wb, 0, &c).categories.len(), 4); + let data = resolve(&wb, 0, &c); + assert_eq!(data.categories, vec!["Ottawa", "Toronto", "Boston", "NYC"]); + assert_eq!(data.category_levels, vec![vec!["Canada", "", "USA", ""]]); } #[test] From 4f92fc417baae2f078cdc86fe450f50d66485986 Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 05:23:40 +0000 Subject: [PATCH 07/12] Fix chart parity chart type notes --- docs/chart-parity.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/chart-parity.md b/docs/chart-parity.md index 1a9e1349f..7ebb8113a 100644 --- a/docs/chart-parity.md +++ b/docs/chart-parity.md @@ -1,6 +1,6 @@ # Chart parity -> **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** trivial (stale chart notes: stock/combo, treemap/sunburst, box and waterfall kinds survive XLSX round-trip; PRs #158 and #161 merged) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (PivotCharts from files plot their cached points; deleted automatic titles stay off; stale chart notes: stock/combo, treemap/sunburst, box and waterfall kinds survive XLSX round-trip; PRs #158 and #161 merged) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's chart types and chart elements against GridCraft. Part of [target-app-parity.md](target-app-parity.md). **Breadth ~50%, ready ~30%, 40–60 h** (estimated). @@ -26,9 +26,7 @@ Sources: `crates/model/src/features.rs` (`ChartKind`, `Chart`, `Series`), `crate | Treemap, Sunburst | 1 each | 2 of 2 | | | Histogram, Pareto | 2 | 1 of 2 | no Pareto | | Box & Whisker | 1 | 1 of 1 | exclusive (default) and inclusive quartiles per series; XLSX round-trip; `chart.set {quartiles}` changes all series | -| Waterfall, Funnel | 2 | 2 of 2 | lose their kind in XLSX (PR #161) | -| Box & Whisker | 1 | 1 of 1 | | -| Waterfall, Funnel | 2 | 2 of 2 | | +| Waterfall, Funnel | 2 | 2 of 2 | kinds survive XLSX round-trip (PR #161) | | Filled map | 1 | 0 | needs geographic boundary data (licensing decision) | | Combo | custom combinations, secondary axis | yes (line on secondary axis) | | | **Total** | ~60 subtypes | **25 (~42%)** | usage-weighted breadth higher (~65%): column, bar, line, pie and scatter dominate | From 8338a35da6035688e026ec0efcbb88bf34e09b32 Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 05:41:58 +0000 Subject: [PATCH 08/12] Keep main's chart revision row text --- docs/chart-parity.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/chart-parity.md b/docs/chart-parity.md index 7ebb8113a..35f1ddbb2 100644 --- a/docs/chart-parity.md +++ b/docs/chart-parity.md @@ -88,7 +88,7 @@ are (#1686). Such series get no `numCache`/`strCache` on save (#1690). | Date | Change | Summary | |---|---|---| -| 2026-10-18 | trivial | Stale notes: stock/combo, treemap/sunburst, box and waterfall kinds survive XLSX round-trip (PRs #158, #161 merged) | +| 2026-10-18 | trivial | Stale notes: stock/combo, treemap/sunburst, box and waterfall kinds survive XLSX round-trip (PRs #158, #161) | | 2026-10-16 | trivial | Series that refer to defined names (`[0]!Name`, `Sheet!Name`) resolve instead of showing `#NAME?`, and are saved qualified, as Excel needs (#1686) | | 2026-10-16 | minor | Box & whisker quartiles default to exclusive, honor each series on draw and XLSX round-trip; `chart.set {quartiles}` changes the method (#1453) | | 2026-10-13 | minor | Multi-level category axes: read, drawn (outer levels under or beside the labels), written back with level caches | From 38696b93a4e90e467257ccc0a576d33a1f9af61b Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 06:11:32 +0000 Subject: [PATCH 09/12] Bound chart import expansion --- crates/chart/src/resolve.rs | 4 ++-- crates/chart/src/tests.rs | 16 ++++++++++++++++ crates/xlsx/src/chart.rs | 11 ++++++++--- crates/xlsx/src/tests/chart_read_tests.rs | 13 +++++++++++++ 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index 6b2a0a5cc..27668f3bc 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -169,9 +169,9 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str, inne let column = |c: usize| (0..h).map(|row| at(row, c)).collect::>(); let row = |rr: usize| (0..w).map(|c| at(rr, c)).collect::>(); let value_count = eval_list(wb, sheet, values).len(); - let by_columns = if value_count == h && w > 1 { + let by_columns = if value_count == h && w > 1 && h != w { true - } else if value_count == w && h > 1 { + } else if value_count == w && h > 1 && h != w { false } else { levels.by_columns diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 01a7a143f..0fd2a1301 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1230,6 +1230,22 @@ fn two_level_categories_resolve_and_draw() { assert!(d.category_levels.is_empty()); } +#[test] +fn square_category_blocks_keep_their_declared_orientation_on_ties() { + let mut wb = Workbook::new(); + let s = wb.sheet_mut(0).unwrap(); + s.name = "Data".into(); + for (i, value) in ["A1", "B1", "A2", "B2", "A3", "B3"].iter().enumerate() { + s.set_value(CellRef::parse(value).unwrap(), Value::from(*value)); + s.set_value(CellRef::new(3, i as u32), Value::Number(i as f64 + 1.0)); + } + let mut c = chart(ChartKind::Line); + c.series = vec![Series { categories: Some("Data!$A$1:$B$2".into()), values: "Data!$A$3:$B$3".into(), ..Series::default() }]; + let d = resolve(&wb, 0, &c); + assert_eq!(d.categories, vec!["A2", "B2"]); + assert_eq!(d.category_levels, vec![vec!["A1", "B1"]]); +} + #[test] fn a_huge_trendline_forecast_does_not_collapse_the_columns() { let m = ApproxMeasure; diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index c1d17bffd..f00d20dcf 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -117,6 +117,10 @@ fn data_ref(e: Option<&El>) -> Option { /// Most cached points a PivotChart series keeps (the chart renderer's own cap). const MAX_CACHED_POINTS: usize = 100_000; +/// Trailing gaps after the last present point are retained only within this bound. +const MAX_TRAILING_GAPS: usize = 16; +/// Maximum series retained from one chart, matching the renderer's series cap. +const MAX_SERIES: usize = 255; /// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s), by index; missing /// points are `None`. Points without a valid `idx` (schema-invalid, but some writers omit it) go @@ -125,8 +129,8 @@ fn read_points<'a>(c: &El, pts: &'a El) -> Option>> { let at = |i: usize, p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()).unwrap_or(i); let placed = || pts.kids("pt").enumerate().map(|(i, p)| (at(i, p), p)).filter(|(i, _)| *i < MAX_CACHED_POINTS); let highest = placed().map(|(i, _)| i + 1).max().unwrap_or(0); - let count = - c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).filter(|n| (highest..=MAX_CACHED_POINTS).contains(n)).unwrap_or(highest); + let limit = highest.saturating_add(MAX_TRAILING_GAPS).min(MAX_CACHED_POINTS); + let count = c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).filter(|n| (highest..=limit).contains(n)).unwrap_or(highest); if count == 0 { return None; } @@ -431,7 +435,8 @@ pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { let mut series = vec![]; let mut caches = vec![]; for (gi, (g, k)) in groups.iter().enumerate() { - for s in g.kids("ser") { + let remaining = MAX_SERIES.saturating_sub(series.len()); + for s in g.kids("ser").take(remaining) { let mut ser = read_series(s); if groups.len() > 1 { // Every series of a combo chart keeps its own group's kind, the first group's included. diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index aba9f2b1e..5d4a8b8b6 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -237,6 +237,19 @@ fn literal_series_keep_their_points_without_a_valid_pt_count() { assert_eq!(values(&format!(r#"{pts}"#)), "{1,2,#N/A}"); } +#[test] +fn chart_cache_counts_and_series_are_bounded() { + let sparse = r#"2"#; + let chart = read(&chart_xml(false, &format!("{sparse}"), "")); + assert_eq!(chart.series[0].values, "{#N/A,2}"); + + let series: String = (0..300) + .map(|i| format!(r#"{i}"#)) + .collect(); + let chart = read(&chart_xml(false, &format!("{series}"), "")); + assert_eq!(chart.series.len(), 255); +} + #[test] fn a_deleted_automatic_title_stays_deleted() { let one = ser("Vehicles", &str_cache("S!$A$1:$A$2", &["a", "b"]), &num_cache("S!$B$1:$B$2", 2, &[])); From 78cf72ae0f3b981315f73b17680f06db1c17ff44 Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 06:16:22 +0000 Subject: [PATCH 10/12] Preserve chart cache tails and series --- crates/xlsx/src/chart.rs | 14 ++++---------- crates/xlsx/src/tests/chart_read_tests.rs | 10 +++++++--- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index f00d20dcf..020703605 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -115,12 +115,8 @@ fn data_ref(e: Option<&El>) -> Option { None } -/// Most cached points a PivotChart series keeps (the chart renderer's own cap). -const MAX_CACHED_POINTS: usize = 100_000; -/// Trailing gaps after the last present point are retained only within this bound. -const MAX_TRAILING_GAPS: usize = 16; -/// Maximum series retained from one chart, matching the renderer's series cap. -const MAX_SERIES: usize = 255; +/// Excel's maximum worksheet row count bounds cache expansion while preserving declared trailing gaps. +const MAX_CACHED_POINTS: usize = gridcraft_core::MAX_ROWS as usize; /// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s), by index; missing /// points are `None`. Points without a valid `idx` (schema-invalid, but some writers omit it) go @@ -129,8 +125,7 @@ fn read_points<'a>(c: &El, pts: &'a El) -> Option>> { let at = |i: usize, p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()).unwrap_or(i); let placed = || pts.kids("pt").enumerate().map(|(i, p)| (at(i, p), p)).filter(|(i, _)| *i < MAX_CACHED_POINTS); let highest = placed().map(|(i, _)| i + 1).max().unwrap_or(0); - let limit = highest.saturating_add(MAX_TRAILING_GAPS).min(MAX_CACHED_POINTS); - let count = c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).filter(|n| (highest..=limit).contains(n)).unwrap_or(highest); + let count = c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).map(|n| n.min(MAX_CACHED_POINTS).max(highest)).unwrap_or(highest); if count == 0 { return None; } @@ -435,8 +430,7 @@ pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { let mut series = vec![]; let mut caches = vec![]; for (gi, (g, k)) in groups.iter().enumerate() { - let remaining = MAX_SERIES.saturating_sub(series.len()); - for s in g.kids("ser").take(remaining) { + for s in g.kids("ser") { let mut ser = read_series(s); if groups.len() > 1 { // Every series of a combo chart keeps its own group's kind, the first group's included. diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index 5d4a8b8b6..e23ec49f6 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -239,15 +239,19 @@ fn literal_series_keep_their_points_without_a_valid_pt_count() { #[test] fn chart_cache_counts_and_series_are_bounded() { - let sparse = r#"2"#; + let sparse = r#"110"#; let chart = read(&chart_xml(false, &format!("{sparse}"), "")); - assert_eq!(chart.series[0].values, "{#N/A,2}"); + assert_eq!(chart.series[0].values.split(',').count(), 40); + + let huge = r#"2"#; + let chart = read(&chart_xml(false, &format!("{huge}"), "")); + assert_eq!(chart.series[0].values.split(',').count(), gridcraft_core::MAX_ROWS as usize); let series: String = (0..300) .map(|i| format!(r#"{i}"#)) .collect(); let chart = read(&chart_xml(false, &format!("{series}"), "")); - assert_eq!(chart.series.len(), 255); + assert_eq!(chart.series.len(), 300); } #[test] From 5b8c2cde8defe9c2bfb70824be301122867a3669 Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 06:23:54 +0000 Subject: [PATCH 11/12] Bound total chart cache imports --- crates/xlsx/src/chart.rs | 64 ++++++++++++++++------- crates/xlsx/src/drawing.rs | 9 ++-- crates/xlsx/src/tests/chart_read_tests.rs | 12 +++++ 3 files changed, 62 insertions(+), 23 deletions(-) diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index 020703605..c9acf83d7 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -98,18 +98,29 @@ pub(crate) fn chart_ref_from_file(text: &str) -> String { } out } +const MAX_CHART_CACHED_POINTS: usize = 16_777_216; -fn data_ref(e: Option<&El>) -> Option { +struct ChartReadBudget { + remaining: usize, + exceeded: bool, +} + +impl ChartReadBudget { + fn new() -> Self { + Self { remaining: MAX_CHART_CACHED_POINTS, exceeded: false } + } +} + +fn data_ref(e: Option<&El>, budget: &mut ChartReadBudget) -> Option { let e = e?; for k in ["numRef", "strRef", "multiLvlStrRef"] { if let Some(f) = e.child(k).and_then(|r| r.child("f")) { return Some(chart_ref_from_file(&f.text)); } } - // Literal values become an array constant. for k in ["numLit", "strLit"] { if let Some(l) = e.child(k) { - return read_points(l, l).map(|p| literal(&p, k == "numLit")); + return read_points(l, l, budget).map(|p| literal(&p, k == "numLit")); } } None @@ -121,11 +132,19 @@ const MAX_CACHED_POINTS: usize = gridcraft_core::MAX_ROWS as usize; /// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s), by index; missing /// points are `None`. Points without a valid `idx` (schema-invalid, but some writers omit it) go /// in document order. A missing, unreadable or too small `ptCount` gives way to the points present. -fn read_points<'a>(c: &El, pts: &'a El) -> Option>> { +fn read_points<'a>(c: &El, pts: &'a El, budget: &mut ChartReadBudget) -> Option>> { let at = |i: usize, p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()).unwrap_or(i); let placed = || pts.kids("pt").enumerate().map(|(i, p)| (at(i, p), p)).filter(|(i, _)| *i < MAX_CACHED_POINTS); let highest = placed().map(|(i, _)| i + 1).max().unwrap_or(0); - let count = c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).map(|n| n.min(MAX_CACHED_POINTS).max(highest)).unwrap_or(highest); + let requested = + c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).map(|n| n.min(MAX_CACHED_POINTS).max(highest)).unwrap_or(highest); + let count = if requested > budget.remaining { + budget.exceeded = true; + highest.min(budget.remaining) + } else { + requested + }; + budget.remaining = budget.remaining.saturating_sub(count); if count == 0 { return None; } @@ -175,25 +194,25 @@ pub(crate) struct PivotSeriesCache { } /// `numCache`, `strCache`, or the innermost level of a `multiLvlStrCache`, under `cat` or `val`. -fn cached_points(e: Option<&El>) -> Option { +fn cached_points(e: Option<&El>, budget: &mut ChartReadBudget) -> Option { let e = e?; let owned = |p: Vec>| p.into_iter().map(|v| v.map(str::to_string)).collect(); if let Some(c) = e.path(&["numRef", "numCache"]) { - Some(CachedPoints { points: owned(read_points(c, c)?), numeric: true, multi_level: false }) + Some(CachedPoints { points: owned(read_points(c, c, budget)?), numeric: true, multi_level: false }) } else if let Some(c) = e.path(&["strRef", "strCache"]) { - Some(CachedPoints { points: owned(read_points(c, c)?), numeric: false, multi_level: false }) + Some(CachedPoints { points: owned(read_points(c, c, budget)?), numeric: false, multi_level: false }) } else { let c = e.path(&["multiLvlStrRef", "multiLvlStrCache"])?; let multi_level = c.kids("lvl").nth(1).is_some(); - Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?)?), numeric: false, multi_level }) + Some(CachedPoints { points: owned(read_points(c, c.child("lvl")?, budget)?), numeric: false, multi_level }) } } -fn pivot_series_cache(s: &El) -> PivotSeriesCache { +fn pivot_series_cache(s: &El, budget: &mut ChartReadBudget) -> PivotSeriesCache { PivotSeriesCache { name: s.path(&["tx", "strRef", "strCache", "pt", "v"]).map(|v| v.text.clone()), - categories: cached_points(s.child("cat")), - values: cached_points(s.child("val")), + categories: cached_points(s.child("cat"), budget), + values: cached_points(s.child("val"), budget), number_format: s .path(&["val", "numRef", "numCache", "formatCode"]) .map(|c| c.text.trim()) @@ -313,7 +332,7 @@ pub(crate) fn plot_pivot_caches(wb: &mut Workbook, pending: &[(u32, Vec Series { +fn read_series(s: &El, budget: &mut ChartReadBudget) -> Series { let name = s .child("tx") .and_then(|tx| tx.path(&["strRef", "f"]).map(|f| chart_ref_from_file(&f.text)).or_else(|| tx.child("v").map(|v| v.text.clone()))); @@ -326,9 +345,9 @@ fn read_series(s: &El) -> Series { let values = s.child("val").or_else(|| s.child("yVal")); Series { name, - categories: data_ref(s.child("cat")).or_else(|| data_ref(s.child("xVal"))), - values: data_ref(s.child("val")).or_else(|| data_ref(s.child("yVal"))).unwrap_or_default(), - bubble_sizes: data_ref(s.child("bubbleSize")), + categories: data_ref(s.child("cat"), budget).or_else(|| data_ref(s.child("xVal"), budget)), + values: data_ref(s.child("val"), budget).or_else(|| data_ref(s.child("yVal"), budget)).unwrap_or_default(), + bubble_sizes: data_ref(s.child("bubbleSize"), budget), color, secondary: false, kind: None, @@ -419,8 +438,12 @@ fn axis_ids(g: &El) -> Vec { } /// A chart part, with the cached points of each series when it is a PivotChart (empty otherwise), -/// for [`plot_cached`] once the workbook's cells are read. +#[allow(dead_code)] pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { + read_chart_with_budget(root).map(|(chart, caches, _)| (chart, caches)) +} + +pub(crate) fn read_chart_with_budget(root: &El) -> Option<(Chart, Vec, bool)> { let chart = root.child("chart")?; let plot = chart.child("plotArea")?; let groups: Vec<(&El, ChartKind)> = plot.children.iter().filter_map(|g| group_kind(g).map(|k| (g, k))).collect(); @@ -429,9 +452,10 @@ pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { let pivot = root.path(&["pivotSource", "name"]).map(|n| strip_book(n.text.trim()).to_string()).filter(|n| !n.is_empty()); let mut series = vec![]; let mut caches = vec![]; + let mut budget = ChartReadBudget::new(); for (gi, (g, k)) in groups.iter().enumerate() { for s in g.kids("ser") { - let mut ser = read_series(s); + let mut ser = read_series(s, &mut budget); if groups.len() > 1 { // Every series of a combo chart keeps its own group's kind, the first group's included. ser.kind = Some(*k); @@ -441,7 +465,7 @@ pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { } series.push(ser); if pivot.is_some() { - caches.push(pivot_series_cache(s)); + caches.push(pivot_series_cache(s, &mut budget)); } } } @@ -488,7 +512,7 @@ pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { by_rows: false, pivot, }; - Some((ch, caches)) + Some((ch, caches, budget.exceeded)) } // ---------------------------------------------------------------- writing diff --git a/crates/xlsx/src/drawing.rs b/crates/xlsx/src/drawing.rs index e18624a10..ce08d8466 100644 --- a/crates/xlsx/src/drawing.rs +++ b/crates/xlsx/src/drawing.rs @@ -167,14 +167,17 @@ pub fn read_drawing(cx: &mut Ctx<'_>, part: &str, sheet: &mut Sheet) -> Result<( if let Some(cx_xml) = cx.optional_xml(&target)? { let chartex = uri.contains("chartex"); let read = if chartex { - crate::chartex::read_chartex(&cx_xml).map(|c| (c, Vec::new())) + crate::chartex::read_chartex(&cx_xml).map(|c| (c, Vec::new(), false)) } else { - crate::chart::read_chart(&cx_xml) + crate::chart::read_chart_with_budget(&cx_xml) }; match read { - Some((mut ch, caches)) => { + Some((mut ch, caches, budget_exceeded)) => { ch.id = cx.next_object_id(); ch.anchor = anchor; + if budget_exceeded { + cx.warn("chart cached points exceeded the per-chart import budget; trailing gaps were truncated"); + } if !caches.is_empty() { cx.pivot_charts.push((ch.id, caches)); } diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index e23ec49f6..5477d972e 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -254,6 +254,18 @@ fn chart_cache_counts_and_series_are_bounded() { assert_eq!(chart.series.len(), 300); } +#[test] +fn chart_cache_budget_bounds_many_empty_series() { + let series: String = (0..1_000) + .map(|i| format!(r#""#)) + .collect(); + let root = crate::xml::parse(chart_xml(false, &format!("{series}"), "").as_bytes()).unwrap(); + let (chart, _, warned) = crate::chart::read_chart_with_budget(&root).unwrap(); + assert_eq!(chart.series.len(), 1_000); + assert!(chart.series.iter().map(|s| s.values.split(',').count()).sum::() <= 16_777_216 + chart.series.len()); + assert!(warned); +} + #[test] fn a_deleted_automatic_title_stays_deleted() { let one = ser("Vehicles", &str_cache("S!$A$1:$A$2", &["a", "b"]), &num_cache("S!$B$1:$B$2", 2, &[])); From 3d82a9cf4ee6d2da5b89444c359b546a74deaf2e Mon Sep 17 00:00:00 2001 From: Neil Cawse Date: Thu, 22 Oct 2026 06:30:27 +0000 Subject: [PATCH 12/12] Clarify chart cache budget drops --- crates/xlsx/src/chart.rs | 15 ++++++++++----- crates/xlsx/src/drawing.rs | 2 +- crates/xlsx/src/tests/chart_read_tests.rs | 13 +++++++++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index c9acf83d7..8c9f77f63 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -132,17 +132,22 @@ const MAX_CACHED_POINTS: usize = gridcraft_core::MAX_ROWS as usize; /// The points of a cache or literal (`c` holds `ptCount`, `pts` the ``s), by index; missing /// points are `None`. Points without a valid `idx` (schema-invalid, but some writers omit it) go /// in document order. A missing, unreadable or too small `ptCount` gives way to the points present. +/// +/// The per-chart budget is checked before allocation. If the requested count would exceed the +/// remaining budget, all present points are retained when they fit; only trailing gaps are +/// clamped. If present points extend past the remaining budget, points beyond that boundary are +/// dropped as well, and the caller is warned. fn read_points<'a>(c: &El, pts: &'a El, budget: &mut ChartReadBudget) -> Option>> { let at = |i: usize, p: &El| p.attr("idx").and_then(|v| v.trim().parse::().ok()).unwrap_or(i); let placed = || pts.kids("pt").enumerate().map(|(i, p)| (at(i, p), p)).filter(|(i, _)| *i < MAX_CACHED_POINTS); let highest = placed().map(|(i, _)| i + 1).max().unwrap_or(0); let requested = c.child_val("ptCount").and_then(|v| v.trim().parse::().ok()).map(|n| n.min(MAX_CACHED_POINTS).max(highest)).unwrap_or(highest); - let count = if requested > budget.remaining { - budget.exceeded = true; - highest.min(budget.remaining) - } else { + let count = if requested <= budget.remaining { requested + } else { + budget.exceeded = true; + if highest <= budget.remaining { highest } else { budget.remaining } }; budget.remaining = budget.remaining.saturating_sub(count); if count == 0 { @@ -438,7 +443,7 @@ fn axis_ids(g: &El) -> Vec { } /// A chart part, with the cached points of each series when it is a PivotChart (empty otherwise), -#[allow(dead_code)] +#[cfg(test)] pub(crate) fn read_chart(root: &El) -> Option<(Chart, Vec)> { read_chart_with_budget(root).map(|(chart, caches, _)| (chart, caches)) } diff --git a/crates/xlsx/src/drawing.rs b/crates/xlsx/src/drawing.rs index ce08d8466..ebe00bc7a 100644 --- a/crates/xlsx/src/drawing.rs +++ b/crates/xlsx/src/drawing.rs @@ -176,7 +176,7 @@ pub fn read_drawing(cx: &mut Ctx<'_>, part: &str, sheet: &mut Sheet) -> Result<( ch.id = cx.next_object_id(); ch.anchor = anchor; if budget_exceeded { - cx.warn("chart cached points exceeded the per-chart import budget; trailing gaps were truncated"); + cx.warn("cached chart points were dropped after the per-chart import budget was exceeded"); } if !caches.is_empty() { cx.pivot_charts.push((ch.id, caches)); diff --git a/crates/xlsx/src/tests/chart_read_tests.rs b/crates/xlsx/src/tests/chart_read_tests.rs index 5477d972e..b9f82f171 100644 --- a/crates/xlsx/src/tests/chart_read_tests.rs +++ b/crates/xlsx/src/tests/chart_read_tests.rs @@ -266,6 +266,19 @@ fn chart_cache_budget_bounds_many_empty_series() { assert!(warned); } +#[test] +fn chart_cache_budget_drops_points_beyond_boundary() { + let series: String = (0..17) + .map(|i| format!(r#"{i}"#)) + .collect(); + let root = crate::xml::parse(chart_xml(false, &format!("{series}"), "").as_bytes()).unwrap(); + let (chart, _, warned) = crate::chart::read_chart_with_budget(&root).unwrap(); + assert_eq!(chart.series.len(), 17); + assert!(chart.series.iter().take(16).all(|s| !s.values.is_empty())); + assert_eq!(chart.series[16].values, ""); + assert!(warned); +} + #[test] fn a_deleted_automatic_title_stays_deleted() { let one = ser("Vehicles", &str_cache("S!$A$1:$A$2", &["a", "b"]), &num_cache("S!$B$1:$B$2", 2, &[]));