From e89ba799e916d60fa3286c84f5dd859d5e5b0cbf Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Tue, 3 Nov 2026 01:49:01 -0600 Subject: [PATCH 1/2] #2012: chart names and macro examples unquote sheet names like formulas do Three places still split a `Sheet!Name` reference and stripped the sheet's quotes by hand: the chart resolver (is a series name a defined name?), the XLSX chart writer (the name's cache) and the macro editor's example references. The first two left a doubled quote in the name, so a series named `'Bob''s'!Title` (a name on the sheet "Bob's") wasn't found and showed that text instead of the name's value; they also split at the first `!`, so a quoted sheet holding one broke. All three now use core's split_sheet, the one reading formulas use. Co-Authored-By: Claude Opus 5.5 --- crates/chart/src/resolve.rs | 7 +++--- crates/chart/src/tests.rs | 36 ++++++++++++++++++++++++++++++ crates/ui-egui/src/macro_editor.rs | 14 ++++++++---- crates/xlsx/src/chart.rs | 9 ++++---- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index 6eee201ae..de3cdef1b 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -179,9 +179,10 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str) -> ( /// Whether `t` names a defined name seen from `sheet` (`Name`, or `Sheet1!Name` looked up on /// that sheet), the way the evaluator looks names up. fn is_defined_name(wb: &Workbook, sheet: usize, t: &str) -> bool { - match t.split_once('!') { - Some((s, bare)) => wb.sheet_index(s.trim_matches('\'')).is_some_and(|i| wb.name(bare, i).is_some()), - None => wb.name(t, sheet).is_some(), + // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). + match gridcraft_core::split_sheet(t) { + (Some(s), bare) => wb.sheet_index(&s).is_some_and(|i| wb.name(bare, i).is_some()), + (None, _) => wb.name(t, sheet).is_some(), } } diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 46d047afc..5d5876af0 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1258,3 +1258,39 @@ fn box_whisker_draws_each_series_quartile_method() { // The boxes span 3.5 and 2.5 data units on the same value axis. assert!((heights[0] / heights[1] - 1.4).abs() < 0.001); } + +#[test] +fn a_series_named_by_a_name_on_a_sheet_with_an_apostrophe_resolves() { + // #2012: `'Bob''s'!Title` kept the doubled quote, so the sheet wasn't found and the series + // was named by the literal text. + let mut wb = Workbook::new(); + { + let s = wb.sheet_mut(0).unwrap(); + s.name = "Bob's".into(); + s.set_value(CellRef::new(0, 0), Value::from("Takings")); + s.set_value(CellRef::new(1, 0), Value::Number(4.0)); + s.set_value(CellRef::new(2, 0), Value::Number(9.0)); + } + wb.names.push(gridcraft_model::DefinedName { + name: "Title".into(), + scope: Some(0), + formula: "'Bob''s'!$A$1".into(), + comment: String::new(), + hidden: false, + }); + let mut c = chart(ChartKind::ColumnClustered); + c.series = vec![Series { + name: Some("'Bob''s'!Title".into()), + categories: None, + values: "'Bob''s'!$A$2:$A$3".into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + quartiles: Default::default(), + smooth: false, + trendlines: Vec::new(), + }]; + let d = resolve(&wb, 0, &c); + assert_eq!(d.series.first().map(|s| s.name.as_str()), Some("Takings")); +} diff --git a/crates/ui-egui/src/macro_editor.rs b/crates/ui-egui/src/macro_editor.rs index 1f64d55ad..2dc52edf5 100644 --- a/crates/ui-egui/src/macro_editor.rs +++ b/crates/ui-egui/src/macro_editor.rs @@ -459,10 +459,7 @@ pub fn parse_refs(text: &str) -> Vec<(Option, RangeRef)> { text.split(',') .filter_map(|part| { let part = part.trim(); - let (sheet, body) = match part.rsplit_once('!') { - Some((s, b)) => (Some(s.trim().trim_matches('\'').replace("''", "'")), b), - None => (None, part), - }; + let (sheet, body) = gridcraft_engine::core::split_sheet(part); RangeRef::parse(&body.replace('$', "")).map(|r| (sheet, r)) }) .take(500) @@ -2399,6 +2396,15 @@ pub fn paint_overlay(p: &egui::Painter, geo: &crate::grid::Geo, sh: &gridcraft_e mod tests { use super::*; + #[test] + fn example_references_unquote_sheet_names_as_formulas_do() { + // #2012: one shared reading of `'Bob''s'!A1` and `'a!b'!A1`. + let refs = parse_refs("'Bob''s'!$A$1:$B$2, 'a!b'!C3, D4"); + let sheets: Vec> = refs.iter().map(|(s, _)| s.clone()).collect(); + assert_eq!(sheets, [Some("Bob's".to_string()), Some("a!b".to_string()), None]); + assert_eq!(refs.iter().map(|(_, r)| r.a1()).collect::>(), ["A1:B2", "C3", "D4"]); + } + #[test] fn an_edited_input_keeps_a_shape_a_macro_may_declare() { let range = NamedInput { rows: [2, 10], cols: [3, 3], header: true, col_types: vec![ColType::Number; 3], ..NamedInput::default() }; diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index 6fa9e0144..c36420043 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -366,12 +366,13 @@ fn is_ref(f: &str) -> bool { /// when it isn't a name, or names a sheet the workbook doesn't have. fn chart_name<'a>(wb: &'a Workbook, si: usize, f: &str) -> Option<(Option, String, Option<&'a DefinedName>)> { let Ok(Expr::Name(n)) = gridcraft_formula::parse(f.trim()) else { return None }; - match n.split_once('!') { - Some((s, bare)) => { - let i = wb.sheet_index(s.trim_matches('\''))?; + // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). + match gridcraft_core::split_sheet(&n) { + (Some(s), bare) => { + let i = wb.sheet_index(&s)?; Some((Some(i), bare.to_string(), wb.name(bare, i))) } - None => { + (None, _) => { let def = wb.name(&n, si); Some((None, n, def)) } From 2caf435a38a2e2d62625c47b7ccd64befca2ea66 Mon Sep 17 00:00:00 2001 From: Hector Villarreal Date: Tue, 3 Nov 2026 07:21:22 -0600 Subject: [PATCH 2/2] #2012: split a parsed sheet-qualified name at the last `!`, sheet as is Addresses review comments 4455115540 and 4455115557. The parser stores `'Sheet'!Name` as `Sheet!Name` with the sheet already unquoted: - Evaluator::name split it at the first `!`, so a series named `'a!b'!Title` showed #NAME?. - chart_name unquoted it again, trimming spaces, so a chart reference to `' Sales '!Data` was saved as `Sales!Data`. Both now split at the last `!` and look the sheet up as is. Tests: chart tests::a_series_named_by_a_name_on_a_sheet_with_a_bang_resolves, xlsx tests::chart_book_names::chart_names_keep_spaces_around_a_quoted_sheet_name. Co-Authored-By: Claude Opus 5.5 --- crates/calc/src/eval.rs | 7 +++-- crates/chart/src/tests.rs | 36 +++++++++++++++++++++ crates/xlsx/src/chart.rs | 11 ++++--- crates/xlsx/src/tests/chart_book_names.rs | 38 +++++++++++++++++++++++ 4 files changed, 84 insertions(+), 8 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 4972623fc..7a957f3a4 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -359,9 +359,10 @@ impl<'h> Evaluator<'h> { return v.clone(); } let wb = self.host.workbook(); - // Sheet-qualified `Sheet1!Name`. - let (scope_sheet, bare) = match n.split_once('!') { - Some((s, b)) => match wb.sheet_index(&gridcraft_core::unquote_sheet(s)) { + // Sheet-qualified `Sheet1!Name`. The parser stores the sheet unquoted, so it is used as + // is (`' Sales '` keeps its spaces) and split at the last `!` (`'a!b'!Name`). + let (scope_sheet, bare) = match n.rsplit_once('!') { + Some((s, b)) => match wb.sheet_index(s) { Some(i) => (i, b), None => return err(CellError::Name), }, diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 5d5876af0..6e423802c 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1294,3 +1294,39 @@ fn a_series_named_by_a_name_on_a_sheet_with_an_apostrophe_resolves() { let d = resolve(&wb, 0, &c); assert_eq!(d.series.first().map(|s| s.name.as_str()), Some("Takings")); } + +#[test] +fn a_series_named_by_a_name_on_a_sheet_with_a_bang_resolves() { + // `'a!b'!Title`: the sheet name holds a `!`, so the evaluator must split the parsed name at + // the last one, not the first. + let mut wb = Workbook::new(); + { + let s = wb.sheet_mut(0).unwrap(); + s.name = "a!b".into(); + s.set_value(CellRef::new(0, 0), Value::from("Takings")); + s.set_value(CellRef::new(1, 0), Value::Number(4.0)); + s.set_value(CellRef::new(2, 0), Value::Number(9.0)); + } + wb.names.push(gridcraft_model::DefinedName { + name: "Title".into(), + scope: Some(0), + formula: "'a!b'!$A$1".into(), + comment: String::new(), + hidden: false, + }); + let mut c = chart(ChartKind::ColumnClustered); + c.series = vec![Series { + name: Some("'a!b'!Title".into()), + categories: None, + values: "'a!b'!$A$2:$A$3".into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + quartiles: Default::default(), + smooth: false, + trendlines: Vec::new(), + }]; + let d = resolve(&wb, 0, &c); + assert_eq!(d.series.first().map(|s| s.name.as_str()), Some("Takings")); +} diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index c36420043..a408da5fa 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -366,13 +366,14 @@ fn is_ref(f: &str) -> bool { /// when it isn't a name, or names a sheet the workbook doesn't have. fn chart_name<'a>(wb: &'a Workbook, si: usize, f: &str) -> Option<(Option, String, Option<&'a DefinedName>)> { let Ok(Expr::Name(n)) = gridcraft_formula::parse(f.trim()) else { return None }; - // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). - match gridcraft_core::split_sheet(&n) { - (Some(s), bare) => { - let i = wb.sheet_index(&s)?; + // The parser has already unquoted the sheet (`'Bob''s'!Name` → `Bob's!Name`, #2012): split + // at the last `!` (`'a!b'!Name`) and keep the name as is (`' Sales '` keeps its spaces). + match n.rsplit_once('!') { + Some((s, bare)) => { + let i = wb.sheet_index(s)?; Some((Some(i), bare.to_string(), wb.name(bare, i))) } - (None, _) => { + None => { let def = wb.name(&n, si); Some((None, n, def)) } diff --git a/crates/xlsx/src/tests/chart_book_names.rs b/crates/xlsx/src/tests/chart_book_names.rs index 14dc9d243..10edf45a4 100644 --- a/crates/xlsx/src/tests/chart_book_names.rs +++ b/crates/xlsx/src/tests/chart_book_names.rs @@ -139,3 +139,41 @@ fn chart_names_are_written_qualified() { let refs = |x: &str| x.split("").skip(1).filter_map(|p| p.split_once("").map(|(f, _)| f.to_string())).collect::>(); assert_eq!(refs(&again), refs(&xml)); } + +/// A quoted sheet name keeps its outer spaces: `' Sales '!Data` is not `Sales!Data`, so the +/// chart doesn't switch to the other sheet's name on save. +#[test] +fn chart_names_keep_spaces_around_a_quoted_sheet_name() { + let mut wb = Workbook::default(); + let mut s = Sheet::new("Sales"); + s.charts.push(Chart { + id: 1, + kind: ChartKind::BarStacked, + anchor: Anchor { cell: gridcraft_core::CellRef::new(0, 3), width: 480.0, height: 300.0, ..Anchor::default() }, + title: None, + series: vec![series("' Sales '!Label", "' Sales '!Data")], + legend: LegendPos::Bottom, + data_labels: false, + data_table: None, + gridlines: true, + style: 2, + x_title: None, + y_title: None, + source: None, + by_rows: false, + pivot: None, + }); + wb.sheets = vec![Arc::new(s), Arc::new(Sheet::new(" Sales "))]; + wb.names = vec![ + name("Items", None, "Sales!$A$2:$A$4"), + name("Data", Some(0), "Sales!$B$2:$B$4"), + name("Data", Some(1), "' Sales '!$B$2:$B$4"), + name("Label", Some(0), "Sales!$B$1"), + name("Label", Some(1), "' Sales '!$B$1"), + ]; + let bytes = write_xlsx(&wb).unwrap(); + let mut xml = String::new(); + zip::ZipArchive::new(std::io::Cursor::new(&bytes)).unwrap().by_name("xl/charts/chart1.xml").unwrap().read_to_string(&mut xml).unwrap(); + assert!(xml.contains("' Sales '!Data"), "{xml}"); + assert!(xml.contains("' Sales '!Label"), "{xml}"); +}