From a98239e1d12542fd7bc7440eba4b515d9e97cb72 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Sat, 10 Oct 2026 01:05:51 -0500 Subject: [PATCH 01/17] Format Cells: take the documented camelCase style keys home.formatCells documents {style: {numFmt?, ...}}, but the style patch was merged with the style's snake_case field names, so numFmt (and borders.diagUp/diagDown) were silently ignored. camelCase keys in the patch are now renamed to the style's own; snake_case keys still work. --- crates/engine/src/cmd/format.rs | 42 ++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index 4712351ca..fd3993e1d 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -488,7 +488,7 @@ fn borders(s: &mut Session, p: &Json) -> Result { } fn format_cells(s: &mut Session, p: &Json) -> Result { - let Some(patch) = p.get("style").cloned() else { + let Some(patch) = p.get("style").map(snake_case_keys) else { // Menu invocation without params: the UI opens the dialog. s.ui_requests.push(crate::UiRequest::Dialog("formatCells".into(), json!({}))); return ok(); @@ -506,6 +506,32 @@ fn format_cells(s: &mut Session, p: &Json) -> Result { }) } +/// The style patch with camelCase keys (`numFmt`, `diagDown`, as documented) renamed to the +/// style's own snake_case (`num_fmt`, `diag_down`). Keys starting with a capital (`Rgb`) stay. +fn snake_case_keys(v: &Json) -> Json { + match v { + Json::Object(m) => Json::Object( + m.iter() + .map(|(k, v)| { + let key = if k.starts_with(|c: char| c.is_ascii_lowercase()) { + k.chars().fold(String::new(), |mut out, c| { + if c.is_ascii_uppercase() { + out.push('_'); + } + out.push(c.to_ascii_lowercase()); + out + }) + } else { + k.clone() + }; + (key, snake_case_keys(v)) + }) + .collect(), + ), + other => other.clone(), + } +} + /// Deep-merges `patch` into `base` (objects merge, everything else replaces). pub(crate) fn merge_json(mut base: Json, patch: &Json) -> Json { match (&mut base, patch) { @@ -1149,6 +1175,20 @@ mod tests { assert_eq!(adjust_decimals("0%", &v, 1), "0.0%"); } + #[test] + fn format_cells_takes_the_documented_keys() { + let mut s = crate::Session::new(); + s.new_workbook(); + s.execute("cell.set", json!({"cell": "A1", "input": "5"})).unwrap(); + s.execute("home.formatCells", json!({"range": "A1", "style": {"numFmt": "0.0", "borders": {"diagUp": {"style": "Thin"}}}})).unwrap(); + let st = &s.execute("cell.get", json!({"cell": "A1"})).unwrap()["style"]; + assert_eq!(st["num_fmt"], "0.0"); + assert_eq!(st["borders"]["diag_up"]["style"], "Thin"); + // The style's own snake_case keys keep working. + s.execute("home.formatCells", json!({"range": "A1", "style": {"num_fmt": "0.00"}})).unwrap(); + assert_eq!(s.execute("cell.get", json!({"cell": "A1"})).unwrap()["style"]["num_fmt"], "0.00"); + } + #[test] fn width_units() { assert_eq!(chars_to_points(8.43), 64.0); From 260da4e7527bca8cf4651ce47f3101cd858bf317 Mon Sep 17 00:00:00 2001 From: nihilist-narwhal <240798405+nihilist-narwhal@users.noreply.github.com> Date: Mon, 12 Oct 2026 14:28:57 -0500 Subject: [PATCH 02/17] Recalc: an array that spills as before doesn't requeue its dependents After each round, every array formula that was evaluated queued the dependents of its spill area again, even when it spilled the same area with the same values. Two arrays that read each other's spill areas then queued each other in every round, up to the limit of 8: one edit evaluated 24 formulas instead of 3. An unchanged spill now leaves its dependents alone. last_recalc_cells counts the formulas of all rounds. --- crates/calc/src/recalc.rs | 25 ++++++++++++++++++++++++- crates/calc/src/tests.rs | 19 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index b391821d7..a3daee641 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -456,6 +456,20 @@ fn spill_blocked(host: &PassHost<'_>, (sheet, anchor): Key, a: &Array) -> bool { || sh.spill.range(range.start..=range.end).any(|(c, _)| range.contains(*c) && spilled_over(*c)) } +/// Whether `arr`, anchored at `anchor`, would spill exactly what the sheet already holds over +/// `range`: the same area, nothing new in the way, and the same values. +fn spills_as_before(sheet: &gridcraft_model::Sheet, anchor: CellRef, range: RangeRef, arr: &Array) -> bool { + let end = + CellRef::new(anchor.row.saturating_add((arr.rows as u32).saturating_sub(1)), anchor.col.saturating_add((arr.cols as u32).saturating_sub(1))); + RangeRef::new(anchor, end) == range + && !sheet.merges.iter().any(|m| m.intersects(&range)) + && !sheet.tables.iter().any(|t| t.range.contains(anchor)) + && range + .iter() + .enumerate() + .all(|(i, c)| c == anchor || (!sheet.cells.has(c) && sheet.spill.get(&c) == Some(arr.data.get(i).unwrap_or(&Value::Empty)))) +} + impl PassHost<'_> { fn compute(&mut self, k: Key) -> Value { if let Some(v) = self.results.get(&k) { @@ -950,6 +964,7 @@ impl Calc { /// Evaluates the dirty set and writes results (and spills) back. fn run(&mut self, wb: &mut Workbook, mut dirty: Vec, full: bool) { let mut dynamic_again = !self.graph.dynamic.is_empty(); + self.last_recalc_cells = 0; for _round in 0..8 { if dirty.is_empty() { break; @@ -984,7 +999,7 @@ impl Calc { eprintln!(" eval {} formulas {:.1} ms", results.len(), prof_now() - tp); } let tw = prof_now(); - self.last_recalc_cells = results.len(); + self.last_recalc_cells += results.len(); self.circular = cycles; // Write back. let mut spill_changes: Vec = Vec::new(); @@ -1004,6 +1019,14 @@ impl Calc { if old.is_none() && new.is_none() { continue; } + // An array that spills exactly as before changes nothing its dependents read, so + // they aren't queued again. Otherwise two arrays that read each other's spill areas + // queue each other in every round, up to the round limit. + if let (Some(r), Some(arr)) = (old, new) + && spills_as_before(sheet, k.1, r, arr) + { + continue; + } if let Some(r) = old { for c in r.iter() { sheet.spill.remove(&c); diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..4529e1c19 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -67,6 +67,25 @@ fn spill_range_operator() { assert_eq!(t.num("E1"), 100.0); } +#[test] +fn arrays_that_spill_as_before_do_not_requeue_their_dependents() { + // Two arrays that each read the other's spill area, and a formula reading both spills. + let mut t = T::new(); + t.set("A1", "=SEQUENCE(2)+0*B2"); + t.set("B1", "=SEQUENCE(2)+0*A2"); + t.set("C1", "=SUM(A1#)+SUM(B1#)"); + // An edit that spills the same values again: one round, not one per round up to the limit. + t.set("A1", "=SEQUENCE(2)+0*B2+0"); + assert_eq!((t.num("A2"), t.num("B2"), t.num("C1")), (2.0, 2.0, 6.0)); + assert!(t.calc.last_recalc_cells <= 3, "{} formulas evaluated for one edit", t.calc.last_recalc_cells); + // Only the anchor's value changes: formulas reading the whole spill still see it. + t.set("A1", "={7;2}+0*B2"); + assert_eq!((t.num("A1"), t.num("C1")), (7.0, 12.0)); + // A spill that does change still reaches its dependents. + t.set("A1", "=SEQUENCE(2)*10+0*B2"); + assert_eq!((t.num("A2"), t.num("C1")), (20.0, 33.0)); +} + #[test] fn arithmetic_and_dependencies() { let mut t = T::new(); From 08902d925a63dda7ae223729ae0d0c4e5db127d0 Mon Sep 17 00:00:00 2001 From: MarekSpodBiedry <73122968+MarekSpodBiedry@users.noreply.github.com> Date: Tue, 13 Oct 2026 18:02:24 +0200 Subject: [PATCH 03/17] xlsx: keep the first group's series kind when importing a combination chart (#1014) --- crates/xlsx/src/chart.rs | 5 ++- crates/xlsx/src/tests/chart_combo_kinds.rs | 46 ++++++++++++++++++++++ crates/xlsx/src/tests/mod.rs | 1 + 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 crates/xlsx/src/tests/chart_combo_kinds.rs diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index dee33c83d..cbe8a1eb1 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -193,8 +193,11 @@ pub fn read_chart(root: &El) -> Option { for (gi, (g, k)) in groups.iter().enumerate() { for s in g.kids("ser") { let mut ser = read_series(s); - if gi > 0 { + 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); + } + if gi > 0 { ser.secondary = axis_ids(g) != first_axes; } series.push(ser); diff --git a/crates/xlsx/src/tests/chart_combo_kinds.rs b/crates/xlsx/src/tests/chart_combo_kinds.rs new file mode 100644 index 000000000..6857683ad --- /dev/null +++ b/crates/xlsx/src/tests/chart_combo_kinds.rs @@ -0,0 +1,46 @@ +//! Importing a combination chart keeps each series' own group kind, the first group's included. + +use gridcraft_model::ChartKind; + +use crate::chart::read_chart; +use crate::xml::parse; + +fn ser(i: usize) -> String { + format!("Data!$A$1:$A$5") +} + +const AX: &str = ""; + +fn bar(sers: &str) -> String { + format!("{sers}{AX}") +} + +fn line(sers: &str) -> String { + format!("{sers}{AX}") +} + +fn kinds(first: &str, second: &str) -> Vec> { + let xml = format!( + "\ + {first}{second}" + ); + let root = parse(xml.as_bytes()).unwrap(); + let ch = read_chart(&root).unwrap(); + assert_eq!(ch.kind, ChartKind::Combo); + ch.series.iter().map(|s| s.kind).collect() +} + +#[test] +fn first_group_with_two_columns_stays_columns() { + let k = kinds(&bar(&(ser(0) + &ser(1))), &line(&ser(2))); + assert_eq!(k[0], Some(ChartKind::ColumnClustered)); + assert_eq!(k[1], Some(ChartKind::ColumnClustered)); + assert!(matches!(k[2], Some(ChartKind::Line | ChartKind::LineMarkers))); +} + +#[test] +fn line_first_group_stays_a_line() { + let k = kinds(&line(&ser(0)), &bar(&ser(1))); + assert!(matches!(k[0], Some(ChartKind::Line | ChartKind::LineMarkers))); + assert_eq!(k[1], Some(ChartKind::ColumnClustered)); +} diff --git a/crates/xlsx/src/tests/mod.rs b/crates/xlsx/src/tests/mod.rs index 517113d0f..218cc632f 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_combo_kinds; mod csv_tests; mod encrypted_tests; mod fixtures; From 1241fbf2609bb2d60acfda6cc31bfb864c2212cc Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Wed, 14 Oct 2026 13:08:42 -0500 Subject: [PATCH 04/17] calc: calculate long dependency chains with bounded stack A formula that reads one later in row-major order (further down, or on a later sheet) is calculated on demand, inside the formula that reads it. Each level costs stack, so real models with chains of about 1,000 such formulas overflowed the 1 MB main-thread stack on Windows and crashed on open. Past 2,000 levels the chain gave #CIRC! instead. On-demand evaluation now goes at most 64 formulas deep. A formula needed deeper suspends the evaluations in progress: it is calculated from the top, then the suspended ones again, reading the results kept so far. Chains of any length calculate, with the same results. A suspended formula read again is a circular reference, as a formula in progress is, so loops are found at the same cell as before. Regression tests build the chains in code: 20,000 formulas each reading the row below, the same hopping between two sheets, a 5,000-formula loop, and a loop closing into the middle of a deep chain. Co-Authored-By: Claude Opus 5.5 --- crates/calc/src/recalc.rs | 81 +++++++++++++++++++++++++++++++++-- crates/calc/src/tests.rs | 88 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 4 deletions(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index b391821d7..634e391ba 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -60,6 +60,12 @@ const WIDE: u32 = 64; /// Insertions an [`Intervals`] keeps aside (scanned by every query) before merging them in. const PENDING: usize = 32; +/// Formulas a formula may calculate on demand one inside the other (each needing one another's +/// value before it is done). Each level takes stack, so a chain deeper than this is suspended and +/// calculated from the top instead (see [`PassHost::calculate`]): long dependency chains calculate +/// whatever order they come in, with bounded stack. +const MAX_NESTED: usize = 64; + /// Formula evaluations iterative calculation may spend on one recalculation (passes × formulas /// evaluated again), however high Maximum Iterations is. const MAX_ITERATION_WORK: usize = 10_000_000; @@ -395,6 +401,17 @@ struct PassHost<'a> { cycle: bool, cycles: Vec, depth: usize, + /// A formula needed at [`MAX_NESTED`] levels deep: the evaluations in progress are abandoned + /// and it is calculated first. + suspend: Option, + /// Formulas whose evaluation was abandoned for another one, to be calculated again after it. + /// They all need it, so reading one of them while it is calculated is a circular reference, + /// as reading a formula in progress is. + waiting: HashSet, + /// The formulas abandoned since the last suspension. + abandoned: Vec, + /// The formulas in `cycles`. + cycle_seen: HashSet, /// Iterative calculation: a circular reference reads the cell's previous value instead of /// giving `#CIRC!`. iterate: bool, @@ -425,6 +442,10 @@ impl<'a> PassHost<'a> { cycle: false, cycles: vec![], depth: 0, + suspend: None, + waiting: HashSet::default(), + abandoned: Vec::new(), + cycle_seen: HashSet::default(), iterate: false, prev: HashMap::default(), stack: Vec::new(), @@ -457,6 +478,36 @@ fn spill_blocked(host: &PassHost<'_>, (sheet, anchor): Key, a: &Array) -> bool { } impl PassHost<'_> { + /// Calculates `k` (and the formulas it reads) with bounded stack: when [`Self::compute`] would + /// go deeper than [`MAX_NESTED`], it suspends what is in progress, the formula it needed is + /// calculated from here, then the suspended ones again (reading the results kept so far). + fn calculate(&mut self, k: Key) { + // Formulas to calculate, innermost last, each with the ones abandoned to calculate the + // next. + let mut work: Vec<(Key, Vec)> = vec![(k, Vec::new())]; + while let Some(&(top, _)) = work.last() { + self.compute(top); + match self.suspend.take() { + Some(next) => { + let held = std::mem::take(&mut self.abandoned); + self.waiting.extend(held.iter().copied()); + if let Some(entry) = work.last_mut() { + entry.1 = held; + } + work.push((next, Vec::new())); + } + None => { + work.pop(); + if let Some(entry) = work.last_mut() { + for a in entry.1.drain(..) { + self.waiting.remove(&a); + } + } + } + } + } + } + fn compute(&mut self, k: Key) -> Value { if let Some(v) = self.results.get(&k) { let v = v.clone(); @@ -468,14 +519,25 @@ impl PassHost<'_> { let Some(expr) = self.exprs.get(&k) else { return self.wb.sheet(k.0).map(|s| s.value(k.1)).unwrap_or_default(); }; - if self.in_progress.contains(&k) || self.depth > 2000 { + if self.suspend.is_some() { + // Unwinding to `calculate`: the value is thrown away. + return Value::Error(CellError::Calc); + } + if self.in_progress.contains(&k) || self.waiting.contains(&k) { self.cycle = true; if self.iterate { return self.circular_read(k); } - self.cycles.push(k); + // Once each, though a suspended evaluation meets it again. + if self.cycle_seen.insert(k) { + self.cycles.push(k); + } return Value::Error(CellError::Circ); } + if self.depth >= MAX_NESTED { + self.suspend = Some(k); + return Value::Error(CellError::Calc); + } self.in_progress.insert(k); self.depth += 1; let outer_taint = std::mem::replace(&mut self.taint, false); @@ -487,6 +549,17 @@ impl PassHost<'_> { let mut ev = Evaluator::new(self, k.0, k.1); ev.value(&expr) }; + if self.suspend.is_some() { + // Abandoned: `calculate` evaluates this formula again once the one it needs is done. + self.depth -= 1; + self.in_progress.remove(&k); + self.abandoned.push(k); + if self.iterate { + self.stack.pop(); + } + self.taint = outer_taint; + return Value::Error(CellError::Calc); + } // A legacy array formula fills the range it was entered in. let legacy = self.wb.sheet(k.0).and_then(|s| s.cell(k.1)).and_then(|c| c.formula.as_ref()).and_then(|f| f.array); let v = match legacy { @@ -577,7 +650,7 @@ impl PassHost<'_> { self.pending = set.clone(); self.spills.retain(|a, _| !set.contains(a)); for k in &again { - self.compute(*k); + self.calculate(*k); } passes += 1; } @@ -969,7 +1042,7 @@ impl Calc { let mut host = PassHost::new(wb, &exprs, &mut rng, now); host.iterate = wb.calc.iterative; for k in &dirty { - host.compute(*k); + host.calculate(*k); } if host.iterate && !host.members.is_empty() { let passes = host.iterate_loops(&dirty, wb.calc.max_iterations, wb.calc.max_change); diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..ef83116e2 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1266,3 +1266,91 @@ fn offset_extreme_arguments_return_ref() { assert_eq!(crate::evaluate(&wb, 0, c("D1"), formula), Value::Number(expected), "{formula}"); } } + +/// A formula reading one further down (or on a later sheet) is calculated on demand, inside the +/// formula that reads it. Chains of thousands of such formulas, common in financial models, used +/// to overflow the stack (or give `#CIRC!` past 2,000 levels); they now calculate in full. +#[test] +fn long_dependency_chains_against_row_order_calculate() { + const N: u32 = 20_000; + let mut t = T::new(); + { + let sh = t.wb.sheet_mut(0).unwrap(); + for r in 1..N { + sh.cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=A{}+1", r + 1)))); + } + sh.cells.set(c(&format!("A{N}")), Cell::value(Value::Number(1.0))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.num("A1"), N as f64); + assert_eq!(t.num("A2"), (N - 1) as f64); + assert!(t.calc.circular.is_empty()); + // An edit at the far end recalculates the whole chain. + t.set(&format!("A{N}"), "10"); + assert_eq!(t.num("A1"), (N + 9) as f64); + assert!(t.calc.circular.is_empty()); + + // Hopping between two sheets at every step, through a range read (SUM) as well. + let mut t = T::new(); + t.wb.sheets.push(std::sync::Arc::new(gridcraft_model::Sheet::new("Data"))); + for r in 1..N { + t.wb.sheet_mut(0).unwrap().cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=Data!A{r}+1")))); + let next = format!("=SUM(Sheet1!A{}:A{})", r + 1, r + 1); + t.wb.sheet_mut(1).unwrap().cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&next))); + } + t.wb.sheet_mut(0).unwrap().cells.set(c(&format!("A{N}")), Cell::value(Value::Number(0.0))); + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.num("A1"), (N - 1) as f64); + assert_eq!(t.wb.sheet(1).unwrap().value(c("A1")), Value::Number((N - 2) as f64)); + assert!(t.calc.circular.is_empty()); +} + +/// A loop longer than the on-demand nesting limit is still a circular reference (and still ends). +#[test] +fn long_circular_chain_is_circular() { + const N: u32 = 5_000; + let mut t = T::new(); + { + let sh = t.wb.sheet_mut(0).unwrap(); + for r in 1..N { + sh.cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=A{}+1", r + 1)))); + } + sh.cells.set(c(&format!("A{N}")), Cell::formula(Formula::new("=A1+1"))); + sh.cells.set(c("B1"), Cell::formula(Formula::new("=1+2"))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.get("A1"), Value::Error(CellError::Circ)); + assert_eq!(t.get(&format!("A{N}")), Value::Error(CellError::Circ)); + assert_eq!(t.num("B1"), 3.0); + assert!(!t.calc.circular.is_empty()); + // Each formula is listed once at most. + let mut seen = t.calc.circular.clone(); + seen.sort_by_key(|(s, c)| (*s, c.row, c.col)); + seen.dedup(); + assert_eq!(seen.len(), t.calc.circular.len()); + // With iterative calculation the loop settles to numbers instead (no hang, no crash). + t.wb.calc.iterative = true; + t.wb.calc.max_iterations = 3; + t.calc.recalc_all(&mut t.wb); + assert!(t.get("A1").as_f64().is_some()); +} + +/// A loop closing back into the middle of a chain deeper than the on-demand nesting limit is found +/// where calculating it in one go finds it: at the formula read while it was being calculated. +#[test] +fn deep_loop_is_found_where_it_closes() { + let mut t = T::new(); + { + let sh = t.wb.sheet_mut(0).unwrap(); + for r in 1..300 { + sh.cells.set(c(&format!("A{r}")), Cell::formula(Formula::new(&format!("=A{}+1", r + 1)))); + } + sh.cells.set(c("A300"), Cell::formula(Formula::new("=A30+1"))); + // Reads the loop through IFERROR, so its value shows where the loop was cut. + sh.cells.set(c("B1"), Cell::formula(Formula::new("=IFERROR(A1,-1)"))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.calc.circular, vec![(0, c("A30"))]); + assert_eq!(t.get("A1"), Value::Error(CellError::Circ)); + assert_eq!(t.num("B1"), -1.0); +} From a0ec1fb785ab17d084ac76d5ad41024169d32214 Mon Sep 17 00:00:00 2001 From: DCM4711 <107349915+DCM4711@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:24:56 +0200 Subject: [PATCH 05/17] Selection: whole columns and rows don't widen to merged cells Clicking column B's header on a sheet with A2:C2 merged selected A:C, because selection.set grows every area until merged cells are whole. Excel leaves header selections alone: column B stays B:B and row 3 stays 3:3. Cell ranges still take merged cells whole (B1:B3 -> A1:C3). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/selection.rs | 6 +++++- crates/engine/src/tests.rs | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/selection.rs b/crates/engine/src/selection.rs index 27418a21a..4df6fbe4a 100644 --- a/crates/engine/src/selection.rs +++ b/crates/engine/src/selection.rs @@ -63,9 +63,13 @@ impl Selection { pub fn a1(&self) -> String { self.ranges.iter().map(|r| r.a1()).collect::>().join(",") } - /// Expands areas so merged cells are included whole. + /// Expands areas so merged cells are included whole. Whole columns and whole rows stay as + /// they are: clicking column B's header selects just B even when B1:C1 is merged, as in Excel. pub fn expand_merges(&mut self, sheet: &Sheet) { for r in self.ranges.iter_mut() { + if r.is_full_cols() || r.is_full_rows() { + continue; + } let mut cur = *r; loop { let mut grown = cur; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..9051d29e4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2267,6 +2267,21 @@ fn whole_sheet_selection_stays_within_the_data() { assert!(s.execute("data.removeDuplicates", json!({"range": "A1:XFD1048576", "header": false})).is_ok()); } +#[test] +fn whole_columns_and_rows_ignore_merges_but_cell_ranges_take_them_whole() { + // As in Excel: a merged A2:C2 doesn't widen a click on column B's header to A:C, nor a + // merged B2:B4 a click on row 3's header to rows 2:4; a cell range still takes them whole. + let mut s = s(); + s.execute("home.mergeCenter", json!({"range": "A2:C2"})).unwrap(); + s.execute("home.mergeCenter", json!({"range": "E2:E4"})).unwrap(); + let sel = |s: &mut Session, range: &str| s.execute("selection.set", json!({"range": range})).unwrap()["selection"].clone(); + assert_eq!(sel(&mut s, "B:B"), "B:B"); + assert_eq!(sel(&mut s, "A:B"), "A:B"); + assert_eq!(sel(&mut s, "3:3"), "3:3"); + assert_eq!(sel(&mut s, "B1:B3"), "A1:C3"); + assert_eq!(sel(&mut s, "E3"), "E2:E4"); +} + #[test] fn parity_counts() { let (done, total) = crate::catalog::parity(); From d66ed11253bc750527c110355786987c28557ade Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Wed, 14 Oct 2026 23:58:57 +0300 Subject: [PATCH 06/17] xlsx: keep every property of the default cell format when importing a file --- crates/xlsx/src/read.rs | 6 ++++-- crates/xlsx/src/tests/fixtures.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 1a479081c..c65008bcc 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -123,9 +123,11 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { if let Some(x) = cx.optional_xml(&r.target)? { let StylesIn { xfs, dxfs, palette, named, num_fmts } = read_styles(&x, &checkboxes); cx.num_fmts = num_fmts; - // Cell format 0 is the Normal style: its font is what every unformatted cell shows. + // Cell format 0 is the Normal cell format: everything on it (font, number format, + // fill, borders, alignment, protection) is what a cell without its own format shows + // (#1013). if let Some(normal) = xfs.first() { - cx.styles = StyleTable::with_default(Style { font: normal.font.clone(), ..Style::default() }); + cx.styles = StyleTable::with_default(normal.clone()); } cx.xf_map = xfs.into_iter().enumerate().map(|(i, s)| if i == 0 { StyleId::DEFAULT } else { cx.styles.intern(s) }).collect(); cx.dxfs = dxfs; diff --git a/crates/xlsx/src/tests/fixtures.rs b/crates/xlsx/src/tests/fixtures.rs index 39ea30951..ee3297b3b 100644 --- a/crates/xlsx/src/tests/fixtures.rs +++ b/crates/xlsx/src/tests/fixtures.rs @@ -553,6 +553,36 @@ fn text_box_formatting_is_read_and_written() { assert_eq!(g.text_outline.map(|o| o.1), Some(1.5)); } +/// #1013: cell format 0 is the workbook's Normal cell format. A file that gives it a number +/// format, fill, alignment or protection must keep all of it for cells that omit their style (or +/// say s="0"), not only the font. +#[test] +fn the_default_cell_format_keeps_every_property_of_xf_zero() { + let styles = r#" + + + + + + "#; + let rels = r#""#; + let body = r#"55"#; + let (wb, _) = read_xlsx(&minimal(body, &[("xl/styles.xml", styles)], rels, "")).unwrap(); + let s = wb.sheet(0).unwrap(); + let check = |st: &gridcraft_model::Style, when: &str| { + assert_eq!(st.num_fmt.as_str(), "0.00", "number format {when}"); + assert_eq!(st.fill.pattern, PatternType::Solid, "fill {when}"); + assert_eq!(st.fill.fg, Color::Rgb(0x00B050), "fill colour {when}"); + assert_eq!(st.align.h, HAlign::Center, "alignment {when}"); + assert!(!st.protection.locked, "protection {when}"); + }; + check(wb.styles.get(StyleId::DEFAULT), "after reading"); + assert_eq!(s.cell(at("A1")).unwrap().style, StyleId::DEFAULT); + assert_eq!(s.cell(at("B1")).unwrap().style, StyleId::DEFAULT); + let (back, _) = read_xlsx(&crate::write_xlsx(&wb).unwrap()).unwrap(); + check(back.styles.get(StyleId::DEFAULT), "after saving"); +} + /// `_xludf.` marks a user-defined call even when a built-in has the same name; dropping it turns /// the call into the built-in, and `_xludf.EOMONTH()` into a call Excel repairs away (#726). #[test] From ced724323429691007a48fb6c2bf25e60005de1c Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Wed, 14 Oct 2026 17:36:36 -0500 Subject: [PATCH 07/17] calc: bounded references up to 1,048,576 cells keep their full shape (#1010) A reference over 16,384 cells was cut to the sheet's used range when it was read into an array. Next to a full-size operand the two no longer lined up: SUMPRODUCT(D3:AZ400*B3:B400,(A3:A400=1)*(D1:AZ1=2)) gave #VALUE!, D3:AZ400*B3:B400 filled with #N/A, and ROWS, COUNTBLANK and --(r="") lost the blanks past the data. Excel keeps the reference's size. Bounded references now keep their full shape up to one column's worth of cells (1,048,576). References that run to the edge of the sheet (A:A, A2:A1048576, several whole rows) and larger blocks still stop at the used range, so whole-column formulas stay fast. Co-Authored-By: Claude Opus 5.5 --- crates/calc/src/eval.rs | 16 ++++++++++---- crates/calc/src/tests.rs | 48 ++++++++++++++++++++++++++++++++++++++++ docs/function-parity.md | 5 +++-- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 0f9cfb03d..517f2fe9e 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -13,10 +13,14 @@ use gridcraft_model::Workbook; /// Largest array a reference may expand to. pub const MAX_CELLS: u64 = 16_000_000; -/// References up to this many cells are read at their full size; larger ones (`A:A`, ranges -/// reserved for data to come such as `$A$2:$A$50000`) stop at the sheet's used range, so that -/// formulas over them stay fast. +/// References up to this many cells (one whole row) are always read at their full size. const FULL_SIZE_CELLS: u64 = 16_384; +/// Bounded references up to this many cells (one column's worth) are read at their full size too, as +/// Excel does, so their shape lines up with the arrays next to them (`D3:AZ400*B3:B400` is 398 by +/// 49 even when the data ends at column F) and their blanks count. References that run to the edge +/// of the sheet (`A:A`, `A2:A1048576`, several whole rows) and larger blocks stop at the sheet's used +/// range, so that formulas over them stay fast. +const FULL_SIZE_BOUNDED_CELLS: u64 = 1_048_576; const MAX_DEPTH: usize = 400; #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] @@ -174,7 +178,11 @@ impl<'h> Evaluator<'h> { // A reference keeps its size, so the array has its shape and its blanks count // (`=A1:A10`, `SUMPRODUCT(--(A1:A10=""))`). let cells = u64::from(a.range.height()) * u64::from(a.range.width()); - let range = if cells > FULL_SIZE_CELLS { self.trim(a) } else { a.range }; + // `A:A`, `1:1` and `A2:A1048576` run to the edge of the sheet: they stand for "the rest of + // the column", not for a block of a chosen size. + let to_edge = a.range.end.row == MAX_ROWS - 1 || a.range.end.col == MAX_COLS - 1; + let full_size = cells <= FULL_SIZE_CELLS || (!to_edge && cells <= FULL_SIZE_BOUNDED_CELLS); + let range = if full_size { a.range } else { self.trim(a) }; let (h, w) = (range.height() as usize, range.width() as usize); if (h as u64) * (w as u64) > MAX_CELLS { return Value::Error(CellError::Num); diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..f623edbc0 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1266,3 +1266,51 @@ fn offset_extreme_arguments_return_ref() { assert_eq!(crate::evaluate(&wb, 0, c("D1"), formula), Value::Number(expected), "{formula}"); } } + +#[test] +fn large_bounded_references_keep_their_shape() { + // D3:AZ400 is 19,502 cells, past the size where a block used to be cut to the used range + // (which ends at column F here). Sliced that way it no longer lined up with the full-size + // operands next to it. Expected values observed in Excel 365 on the same layout. + let mut t = T::new(); + for (at, v) in [ + ("D1", "1"), + ("E1", "2"), + ("F1", "3"), + ("A3", "1"), + ("A4", "2"), + ("A5", "1"), + ("B3", "2"), + ("B4", "3"), + ("B5", "4"), + ("D3", "10"), + ("E3", "20"), + ("F3", "30"), + ("D4", "40"), + ("E4", "50"), + ("F4", "60"), + ("D5", "70"), + ("E5", "80"), + ("F5", "90"), + ] { + t.set(at, v); + } + let cases = [ + ("=SUMPRODUCT(D3:AZ400*B3:B400,(A3:A400=1)*(D1:AZ1=2))", Value::Number(360.0)), + ("=ROWS(D3:AZ400*1)", Value::Number(398.0)), + ("=COLUMNS(D3:AZ400*1)", Value::Number(49.0)), + ("=SUMPRODUCT(D3:AZ400,(A3:A400=1)*(D1:AZ1=2))", Value::Number(100.0)), + ("=SUMPRODUCT(D3:AZ400+0,D3:AZ401)", Value::Error(CellError::Value)), + ("=COUNTBLANK(D3:AZ400)", Value::Number(19493.0)), + ("=SUMPRODUCT(--(D3:AZ400=\"\"))", Value::Number(19493.0)), + ("=SUM(D3:AZ400*B3:B400)", Value::Number(1530.0)), + ("=COLUMNS(TRANSPOSE(D3:AZ400))", Value::Number(398.0)), + // Whole columns are still read up to the used range. + ("=SUMPRODUCT((A:A=1)*B:B)", Value::Number(6.0)), + ]; + for (i, (formula, want)) in cases.iter().enumerate() { + let at = format!("C{}", 1001 + i); + t.set(&at, formula); + assert_eq!(&t.get(&at), want, "{formula}"); + } +} diff --git a/docs/function-parity.md b/docs/function-parity.md index 760ffd6ac..5d5e80409 100644 --- a/docs/function-parity.md +++ b/docs/function-parity.md @@ -1,6 +1,6 @@ # Function parity: worksheet functions and formulas -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (USDOLLAR added as a compatibility function; MIRR, COUNTIF and INDIRECT edge cases match Excel 365) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** minor (bounded references up to 1,048,576 cells keep their full shape in array formulas, #1010) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Excel's worksheet functions, category by category, against GridCraft's function library. Part of [target-app-parity.md](target-app-parity.md); gaps feed [gaps.md](gaps.md). @@ -90,7 +90,7 @@ PIVOTBY, REGEXTEST…) are all present. **~82% ready.** The library is broad and has 96 test functions in `crates/functions` with many assertions each, but there is no conformance suite against Excel-computed results, and user reports keep finding edge cases: MIN() with no arguments (#138), COUNTBLANK past the used range -(#137), ROUND at 15 digits and DATE(1900,2,29) (fixed 10-09), SUMIF sum-range sizing (fixed +(#137, and past 16,384 cells #1010, fixed 10-14 for bounded references), ROUND at 15 digits and DATE(1900,2,29) (fixed 10-09), SUMIF sum-range sizing (fixed 10-09). Statistical distributions and financial functions with iterative solvers (RATE, IRR, XIRR, YIELD) are the highest-risk for last-digit differences. @@ -108,6 +108,7 @@ workbooks; results checked in as data, not Excel files) and fixing what it finds | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Bounded references up to 1,048,576 cells (`D3:AZ400`) keep their full shape instead of stopping at the used range, so `SUMPRODUCT(D3:AZ400*B3:B400,(A3:A400=x)*(D1:AZ1=y))` broadcasts like Excel and `ROWS`, `COUNTBLANK` and `--(r="")` count the blanks; references that run to the edge of the sheet (`A:A`) still stop at the used range (#1010) | | 2026-10-12 | minor | USDOLLAR added as a Compatibility function (not on Microsoft's list, so the counts don't change); MIRR with no positive flow is -1, COUNTIF's bare `"<="` / `">="` skip empty strings, INDIRECT with `a1` = FALSE gives `#REF!` for A1-style text (#738) | | 2026-10-11 | minor | TEXT / number formats fixed against Excel-cached results: fraction literals and zero fractions, `0` denominators zero padded, `?` pads the thousands separator, booleans use the text section (226 mismatches → 0 in 352 TEXT cells) | | 2026-10-11 | minor | Iterative calculation: circular references iterate up to Maximum Iterations / Maximum Change when enabled | From ac7595975a973c47ef8c0dd5f804b15f44d67059 Mon Sep 17 00:00:00 2001 From: David Ardila Date: Wed, 14 Oct 2026 21:05:00 -0500 Subject: [PATCH 08/17] Recalculation: formulas reading an array that loses a spill conflict see #SPILL! When two dynamic arrays of one recalculation want the same empty cells, both look free while the pass runs, so formulas evaluated in that pass read both arrays. The one laid out second becomes #SPILL! at write-back, but nothing evaluated its readers again, and they kept its values. With B1 `=SEQUENCE(3)` and A2 `=SEQUENCE(1,3)` (both want B2), a full recalculation (opening a file, Calculate Now) leaves A2 at #SPILL!, yet `=A2*10` showed 10, `=SUM(A2#)` 6, and `=C2` and `=SUM(C2:C3)` 3, though C2 is empty. Entering the same formulas one at a time gave the right values. An array blocked at write-back now sends the formulas that read its anchor or the area it would have spilled into to the next round, where they read #SPILL! and empty cells. Regression test in calc (`formulas_reading_an_array_that_loses_a_spill_conflict_read_spill_errors`); it fails without the fix. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +++- crates/calc/src/recalc.rs | 7 +++++++ crates/calc/src/tests.rs | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..3d021e17e 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** minor (formulas reading an array that loses a spill conflict see `#SPILL!`) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-14 | Recalculation: when two arrays of one recalculation want the same empty cells, the formulas that read the one laid out second (at its anchor or over the cells it would have spilled into) read `#SPILL!` and empty cells instead of its values | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Progress log: formulas reading an array that loses a spill conflict | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index b391821d7..2cd7b0b87 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -1023,6 +1023,13 @@ impl Calc { if let Some(cell) = sheet.cells.get_mut(k.1) { cell.value = Value::Error(CellError::Spill); } + // Not blocked while the pass ran (it would have been #SPILL! there), so + // blocked by another array of this pass: the formulas that read this one, + // at its anchor or over the area it would have spilled into, read #SPILL! + // or empty cells now. + let mut deps = Vec::new(); + self.graph.dependents_of_range(k.0, range, &mut deps); + spill_changes.extend(deps.into_iter().filter(|d| d != k)); } else { for (i, c) in range.iter().enumerate() { if c != k.1 { diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..6e0b7b616 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -368,6 +368,25 @@ fn spill_into_the_old_area_of_a_blocked_neighbour() { assert_eq!([t.num("C8"), t.num("D8"), t.num("C9"), t.num("D9")], [1.0, 2.0, 3.0, 4.0]); } +#[test] +fn formulas_reading_an_array_that_loses_a_spill_conflict_read_spill_errors() { + // Two arrays of one recalculation want the same empty cell: the one laid out second is + // #SPILL!, and the formulas that read it in that pass (at its anchor, through `A2#`, or over + // the cells it would have spilled into) kept its values. + let mut t = T::new(); + t.set("B1", "=SEQUENCE(3)"); + t.set("A2", "=SEQUENCE(1,3)"); + t.set("D2", "=A2*10"); + t.set("E2", "=C2"); + t.set("F2", "=SUM(A2#)"); + t.set("G2", "=SUM(C2:C3)"); + // Both arrays recalculated together: B1 is laid out first and takes B2. + t.calc.recalc_all(&mut t.wb); + let spill = Value::Error(CellError::Spill); + assert_eq!((t.num("B2"), t.get("A2"), t.get("D2")), (2.0, spill.clone(), spill)); + assert_eq!((t.get("C2"), t.num("E2"), t.get("F2"), t.num("G2")), (Value::Empty, 0.0, Value::Error(CellError::Ref), 0.0)); +} + #[test] fn arrays_do_not_spill_in_tables() { // Excel: spilled array formulas aren't supported in tables. From cd94a6d7b42091158809156a7768f2cb0453f960 Mon Sep 17 00:00:00 2001 From: David Ardila Date: Wed, 14 Oct 2026 21:02:40 -0500 Subject: [PATCH 09/17] Recalculation: a spill anchor given a formula that reads its old spill area sees it empty When a dynamic array's anchor gets a new formula, the formula is evaluated while the old array is still laid out, so it can read the old spilled values. The anchor was then never evaluated again: it is left out of its own spill area's dependents, so as not to read its own array. - `A1` was `=SEQUENCE(3)` and becomes `=SUM(A2:A3)+1`: it showed 6 (2 + 3 + 1) with A2:A3 empty; recalculating from scratch gives 1. - `A1` was `=SEQUENCE(3)` and becomes `=SEQUENCE(1,A2+1)`: it read A2 as 2 and spilled over A1:C1 instead of showing 1. The anchor is now evaluated again when its old array is gone or partly freed. It is still left out when its new array covers the old one, where it would read its own array. Regression test in calc (`an_anchor_given_a_formula_that_reads_its_old_spill_area_sees_it_empty`: the new formula stops spilling, spills elsewhere, or spills over part of the old area); it fails without the fix. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +++- crates/calc/src/recalc.rs | 10 +++++++++- crates/calc/src/tests.rs | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..931d22418 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-14 · **Change:** minor (a spill anchor given a formula that reads its old spill area sees it empty) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-14 | Recalculation: a dynamic array's anchor given a new formula that reads its old spill area (`A1` was `=SEQUENCE(3)`, becomes `=SUM(A2:A3)+1` or `=SEQUENCE(1,A2+1)`) sees that area empty instead of the old values | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Progress log: spill anchor reading its old spill area | | 2026-10-12 | minor | Rows auto-fit oversized fonts on open and whole-row/column formatting (#833); progress log row | | 2026-10-12 | minor | Progress log: multi-area row/column delete (#758) | | 2026-10-12 | trivial | Progress log: XLSX chart sheets kept on save (#727) | diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index b391821d7..0905b8fb2 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -1036,7 +1036,15 @@ impl Calc { if let Some(r) = changed_range { let mut deps = Vec::new(); self.graph.dependents_of_range(k.0, r, &mut deps); - spill_changes.extend(deps.into_iter().filter(|d| d != k)); + // Not the anchor itself (that would read its own array), unless its old array + // is gone or partly freed: its new formula may have read the old values, still + // laid out while it was evaluated. + let freed = match (old, sheet.spill_ranges.get(&k.1)) { + (Some(o), Some(n)) => !(n.contains(o.start) && n.contains(o.end)), + (_, None) => true, + (None, Some(_)) => false, + }; + spill_changes.extend(deps.into_iter().filter(|d| d != k || freed)); } if let Some(o) = old && !sheet.spill_ranges.get(&k.1).is_some_and(|n| n.contains(o.start) && n.contains(o.end)) diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 196c57de7..924c2db66 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -543,6 +543,25 @@ fn spills() { assert_eq!(t.num("D2"), 40.0); } +#[test] +fn an_anchor_given_a_formula_that_reads_its_old_spill_area_sees_it_empty() { + // The new formula was evaluated while the old array was still laid out, and only the + // anchor's dependents were evaluated again once it was gone. + let mut t = T::new(); + t.set("A1", "=SEQUENCE(3)"); + assert_eq!(t.num("A3"), 3.0); + t.set("A1", "=SUM(A2:A3)+1"); + assert_eq!((t.num("A1"), t.get("A3")), (1.0, Value::Empty)); + // Still an array, now spilling elsewhere: it read A2 as 2 and spilled three columns. + t.set("A1", "=SEQUENCE(3)"); + t.set("A1", "=SEQUENCE(1,A2+1)"); + assert_eq!((t.num("A1"), t.get("B1"), t.get("A2")), (1.0, Value::Empty, Value::Empty)); + // A smaller array inside the old area: it read A3 as 3 and spilled over A1:A2. + t.set("A1", "=SEQUENCE(3)"); + t.set("A1", "=SEQUENCE(MAX(1,A3-1))"); + assert_eq!((t.num("A1"), t.get("A2"), t.get("A3")), (1.0, Value::Empty, Value::Empty)); +} + #[test] fn references_keep_their_size_past_the_used_range() { let mut t = T::new(); From 0ec27a6101f8d0a16dbccc455c3179de2be5dfd0 Mon Sep 17 00:00:00 2001 From: MarekSpodBiedry <73122968+MarekSpodBiedry@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:15:12 +0200 Subject: [PATCH 10/17] Open the Insert Chart dialog from Recommended Charts instead of inserting a chart (#1399) --- crates/ui-egui/src/lib.rs | 5 +++++ crates/ui-egui/src/ribbon.rs | 9 ++------- crates/ui-egui/tests/interaction.rs | 13 +++++++++++++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f96cdac3c..a32dcd078 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -505,6 +505,11 @@ impl SheetApp { if id == "file.exportPdf" && self.services.pick_save.is_some() && params.get("path").and_then(Json::as_str).is_none_or(str::is_empty) { return self.export_pdf_with_picker(params); } + // Recommended Charts with no chart type named (ribbon button, keytip, command search) asks + // which chart to insert; programmatic callers of `run` still get the first recommendation. + if id == "insert.recommendedCharts" && params.get("type").is_none() { + return self.open_dialog("recommendedCharts", json!({})); + } if let Err(e) = self.run(id, params) { self.alert(e); } diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 9c6d89eeb..b9e7bfa06 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -1434,13 +1434,8 @@ fn insert(app: &mut SheetApp, ui: &mut Ui) { let __k = big_button(ui, Icon::Chart, "Recommended\nCharts", "Recommended Charts", false); control_keytip(app, ui, &__k, "insert.recommendedCharts"); if __k.clicked() { - match app.run("insert.recommendedCharts", json!({})) { - // The new chart is selected, as after inserting any other object (see insert.chart - // in dialogs.rs) — otherwise the prior cell/range selection is left in place, and - // Del right after inserting deletes the source data instead of the chart. - Ok(r) => app.selected_chart = r.get("chart").and_then(serde_json::Value::as_u64).and_then(|id| u32::try_from(id).ok()), - Err(e) => app.message = Some(("GridCraft".into(), crate::clean_error(&e))), - } + // Menu-style: with no chart type, run_or_alert opens the Insert Chart dialog. + act(app, "insert.recommendedCharts", json!({})); } ui.vertical(|ui| { ui.horizontal(|ui| { diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index b40769cf8..f36e5d411 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -485,6 +485,19 @@ fn recommended_charts_dialog_inserts_the_chosen_chart() { assert!(h.state().message.is_some()); } +#[test] +fn recommended_charts_command_without_a_type_opens_the_dialog() { + // #1399: the ribbon button and the NR keytip ran the command directly and inserted the first + // recommendation (a scatter plot) instead of showing the chart picker. + let mut s = blank(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Fruit", "Sales"], ["Apples", 5], ["Pears", 3], ["Plums", 2]]})).unwrap(); + s.execute("selection.set", json!({"range": "A1:B4"})).unwrap(); + let mut h = harness(s); + h.state_mut().run_or_alert("insert.recommendedCharts", json!({})); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("recommendedCharts")); + assert!(h.state().session.active().unwrap().wb.active().unwrap().charts.is_empty(), "no chart is inserted before one is picked"); +} + #[test] fn select_data_dialog_sets_the_category_axis_labels() { // #287: Chart Design › Select Data edits the Horizontal (Category) Axis Labels and removes the From 86b124fca68f2b28eddae404f8a37949cf54e7df Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Fri, 16 Oct 2026 15:17:19 +0300 Subject: [PATCH 11/17] Charts: build multi-area series from every selected area --- crates/engine/src/cmd/insert.rs | 62 ++++++++++++++++++++++++++------- crates/engine/src/tests.rs | 25 +++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index d878e4469..422698fcb 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -224,6 +224,17 @@ fn block(s: &Session, p: &Json) -> Result { Ok(current_region(sh, d.selection.active)) } +fn chart_areas(s: &Session, p: &Json) -> Result> { + if p.get("range").is_some() { + return target_ranges(s, p); + } + let d = s.doc()?; + if d.selection.ranges.len() > 1 { + return Ok(d.selection.ranges.clone()); + } + Ok(vec![block(s, p)?]) +} + // ---------------------------------------------------------------- tables fn insert_table(s: &mut Session, p: &Json) -> Result { @@ -626,6 +637,36 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec out } +fn series_from_areas(sh: &Sheet, areas: &[RangeRef]) -> Result<(RangeRef, bool, Vec)> { + let Some(first) = areas.first().copied() else { return Err(EngineError::Other("Select data for the chart first.".into())) }; + let bounds = areas.iter().skip(1).fold(first, |r, area| r.union(area)); + if areas.len() == 1 { + let by_rows = first.width() > first.height() + 1; + return Ok((bounds, by_rows, series_from_range(sh, first, by_rows))); + } + let valid_shape = first.width() == 1 + && first.height() > 1 + && areas.iter().skip(1).all(|area| area.width() == 1 && area.height() == first.height()) + && first.iter().all(|cell| sh.value(cell).is_text() || sh.value(cell).is_empty()); + if !valid_shape { + return Err(bad("insert.chart", "multi-area charts require a text category column followed by equal-height series columns")); + } + let category = series_from_range(sh, first, false).into_iter().next().map(|series| series.values); + let mut series = Vec::with_capacity(areas.len() - 1); + for area in &areas[1..] { + let mut area_series = series_from_range(sh, *area, false); + let Some(mut item) = area_series.pop() else { + return Err(EngineError::Other("Select data for the chart first.".into())); + }; + if item.name.is_none() { + return Err(bad("insert.chart", "multi-area series columns require headers")); + } + item.categories = category.clone(); + series.push(item); + } + Ok((bounds, false, series)) +} + /// A scatter plot of a block without category labels takes its first column (or row) as the X /// values of the others, as Excel does: two plain columns make one series of points, not two (#428). pub fn scatter_x_from_first(kind: ChartKind, series: &mut Vec) { @@ -646,15 +687,14 @@ const MAX_CHART_CELLS: u64 = 1_000_000; /// without labels → scatter; one short series → column, pie, bar; long series → line; several /// series → clustered and stacked columns and bars, line. pub fn recommend_charts(s: &Session, p: &Json) -> Result> { - let r = block(s, p)?; - if r.count() > MAX_CHART_CELLS { + let areas = chart_areas(s, p)?; + if areas.iter().fold(0_u64, |count, area| count.saturating_add(area.count())) > MAX_CHART_CELLS { return Err(bad("insert.chart", "range too large for a chart")); } let sheet = target_sheet(s, p)?; let d = s.doc()?; let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; - let by_rows = r.width() > r.height() + 1; - let series = series_from_range(sh, r, by_rows); + let (_, by_rows, series) = series_from_areas(sh, &areas)?; let ranges: Vec = series .iter() .filter_map(|x| match gridcraft_formula::parse(&x.values) { @@ -706,8 +746,8 @@ pub fn recommend_charts(s: &Session, p: &Json) -> Result Result<(usize, Chart)> { - let r = block(s, p)?; - if r.count() > MAX_CHART_CELLS { + let areas = chart_areas(s, p)?; + if areas.iter().fold(0_u64, |count, area| count.saturating_add(area.count())) > MAX_CHART_CELLS { return Err(bad("insert.chart", "range too large for a chart")); } let mut t = str_param(p, "type").unwrap_or("column").to_string(); @@ -720,18 +760,14 @@ pub fn build_chart(s: &Session, p: &Json) -> Result<(usize, Chart)> { let sheet = target_sheet(s, p)?; let d = s.doc()?; let sh = d.wb.sheet(sheet).ok_or(EngineError::NoDocument)?; - // Excel plots series by the longer dimension (more rows → series in columns). - let data_h = r.height(); - let data_w = r.width(); - let by_rows = data_w > data_h + 1; let kind = chart_kind(&t, &sub); - let mut series = series_from_range(sh, r, by_rows); + let (bounds, by_rows, mut series) = series_from_areas(sh, &areas)?; if series.is_empty() { return Err(EngineError::Other("Select data for the chart first.".into())); } scatter_x_from_first(kind, &mut series); let id = d.wb.next_object_id(); - let at = cell_param_on(s, p, "at", sheet)?.unwrap_or_else(|| CellRef::new(r.start.row, r.end.col + 2)); + let at = cell_param_on(s, p, "at", sheet)?.unwrap_or_else(|| CellRef::new(bounds.start.row, bounds.end.col + 2)); let title = str_param(p, "title").map(str::to_string).or_else(|| { if series.len() == 1 { series[0].name.as_ref().map(|n| gridcraft_calc::evaluate(&d.wb, sheet, at, n).display()) @@ -758,7 +794,7 @@ pub fn build_chart(s: &Session, p: &Json) -> Result<(usize, Chart)> { style: 1, x_title: None, y_title: None, - source: Some(format!("{}!{}", gridcraft_formula::quote_sheet(&sh.name), r.a1())), + source: (areas.len() == 1).then(|| format!("{}!{}", gridcraft_formula::quote_sheet(&sh.name), bounds.a1())), by_rows, pivot: None, }; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..57a0f1e55 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -125,6 +125,31 @@ fn chart_recommendations_follow_the_data_shape() { assert!(s.ui_requests.is_empty(), "programmatic calls never open the dialog"); } +#[test] +fn chart_uses_text_first_area_as_categories_for_later_areas() { + let mut s = s(); + s.execute( + "range.setValues", + json!({"range": "A1", "values": [["Mes", "x", "y", "z", "Ventas"], ["Ene", 1, 1, 1, 10], ["Feb", 1, 1, 1, 20], ["Mar", 1, 1, 1, 30], ["Abr", 1, 1, 1, 40]]}), + ) + .unwrap(); + + s.execute("insert.chart", json!({"range": "A1:A5,E1:E5", "type": "pie", "at": "G2"})).unwrap(); + let multi = s.doc().unwrap().wb.active().unwrap().charts.last().unwrap().clone(); + assert_eq!(multi.series.len(), 1); + assert_eq!(multi.series[0].name.as_deref(), Some("Sheet1!$E$1")); + assert_eq!(multi.series[0].categories.as_deref(), Some("Sheet1!$A$2:$A$5")); + assert_eq!(multi.series[0].values, "Sheet1!$E$2:$E$5"); + + s.execute("insert.chart", json!({"range": "D1:E5", "type": "pie", "at": "G20"})).unwrap(); + let contiguous = s.doc().unwrap().wb.active().unwrap().charts.last().unwrap().clone(); + assert_eq!(contiguous.series.len(), 2); + assert_eq!(contiguous.series[0].name.as_deref(), Some("Sheet1!$D$1")); + assert_eq!(contiguous.series[0].values, "Sheet1!$D$2:$D$5"); + assert_eq!(contiguous.series[1].name.as_deref(), Some("Sheet1!$E$1")); + assert_eq!(contiguous.series[1].values, "Sheet1!$E$2:$E$5"); +} + #[test] fn chart_category_axis_from_blank_corner_and_select_data() { // #287: numeric X values must be the category axis, not a series of their own. From ea3ef8faf7813489bf6332255cef203e9a054b6b Mon Sep 17 00:00:00 2001 From: webdevtodayjason <115739170+webdevtodayjason@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:25:48 -0500 Subject: [PATCH 12/17] Charts: honor box and whisker quartile methods Keep exclusive and inclusive quartiles per series when drawing and saving XLSX, and expose the setting through chart.set. Fixes #1453 --- crates/chart/src/render.rs | 44 +++++++++++++++++-- crates/chart/src/tests.rs | 32 ++++++++++++++ crates/engine/src/cmd/forecast.rs | 1 + crates/engine/src/cmd/insert.rs | 17 ++++++- crates/engine/src/pivot.rs | 1 + crates/engine/src/sample.rs | 1 + crates/engine/src/tests.rs | 27 ++++++++++++ crates/model/src/features.rs | 11 +++++ crates/xlsx/src/chart.rs | 1 + crates/xlsx/src/chartex.rs | 42 ++++++++++++++++-- .../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 | 5 ++- 15 files changed, 175 insertions(+), 11 deletions(-) diff --git a/crates/chart/src/render.rs b/crates/chart/src/render.rs index 037b9099e..af30df572 100644 --- a/crates/chart/src/render.rs +++ b/crates/chart/src/render.rs @@ -3,7 +3,7 @@ use std::collections::HashSet; use std::f32::consts::{PI, TAU}; -use gridcraft_model::{Chart, ChartKind, LegendPos, LineDash, Theme, TrendlineKind}; +use gridcraft_model::{Chart, ChartKind, LegendPos, LineDash, QuartileMethod, Theme, TrendlineKind}; use crate::axis::{Fmt, Scale, clean, fixed_scale, nice_scale}; use crate::resolve::{MAX_POINTS, MAX_SERIES, MAX_TRENDLINES, series_color}; @@ -1561,12 +1561,16 @@ fn radar(ctx: &mut Ctx, chart: &Chart, data: &ChartData, area: Bx) { // ---------------------------------------------------------------- box & whisker -fn quantile(sorted: &[f64], q: f64) -> f64 { +fn quantile(sorted: &[f64], q: f64, method: QuartileMethod) -> f64 { let n = sorted.len(); if n == 0 { return 0.0; } - let pos = q * (n - 1) as f64; + // Clamp exclusive ranks for short samples so one or two points remain drawable. + let pos = match method { + QuartileMethod::Inclusive => q * (n - 1) as f64, + QuartileMethod::Exclusive => (q * (n as f64 + 1.0) - 1.0).clamp(0.0, (n - 1) as f64), + }; let i = pos.floor() as usize; let frac = pos - i as f64; let a = sorted.get(i).copied().unwrap_or(0.0); @@ -1593,7 +1597,8 @@ fn box_whisker(ctx: &mut Ctx, chart: &Chart, data: &ChartData, area: Bx) { continue; } v.sort_by(f64::total_cmp); - let (q1, med, q3) = (quantile(&v, 0.25), quantile(&v, 0.5), quantile(&v, 0.75)); + let method = chart.series.get(i).map(|s| s.quartiles).unwrap_or_default(); + let (q1, med, q3) = (quantile(&v, 0.25, method), quantile(&v, 0.5, method), quantile(&v, 0.75, method)); let iqr = q3 - q1; let (flo, fhi) = (q1 - 1.5 * iqr, q3 + 1.5 * iqr); let wlo = v.iter().copied().find(|x| *x >= flo).unwrap_or(q1); @@ -1769,3 +1774,34 @@ mod smooth_tests { assert!(out.iter().all(|p| (0.0..=300.0).contains(&p[0]) && (10.0..=110.0).contains(&p[1]))); } } + +#[cfg(test)] +mod quartile_tests { + use super::*; + + #[test] + fn box_whisker_default_quartiles_are_exclusive() { + let values = [1.0, 2.0, 3.0, 4.0, 5.0, 6.0]; + assert_eq!(quantile(&values, 0.25, QuartileMethod::default()), 1.75); + assert_eq!(quantile(&values, 0.5, QuartileMethod::default()), 3.5); + assert_eq!(quantile(&values, 0.75, QuartileMethod::default()), 5.25); + } + + #[test] + fn quartile_methods_and_short_samples() { + for (values, exclusive, inclusive) in [ + (vec![1., 2., 3.], (1., 3.), (1.5, 2.5)), + (vec![1., 2., 3., 4.], (1.25, 3.75), (1.75, 3.25)), + (vec![1., 2., 3., 4., 5.], (1.5, 4.5), (2., 4.)), + (vec![1., 2., 3., 4., 5., 6., 7.], (2., 6.), (2.5, 5.5)), + (vec![1., 1., 2., 3., 5., 8.], (1., 5.75), (1.25, 4.5)), + (vec![1., 2.], (1., 2.), (1.25, 1.75)), + (vec![1.], (1., 1.), (1., 1.)), + (vec![], (0., 0.), (0., 0.)), + ] { + for (method, expected) in [(QuartileMethod::Exclusive, exclusive), (QuartileMethod::Inclusive, inclusive)] { + assert_eq!((quantile(&values, 0.25, method), quantile(&values, 0.75, method)), expected); + } + } + } +} diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 01f1aac49..07729c6c7 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -227,6 +227,7 @@ fn resolve_from_workbook() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }, @@ -238,6 +239,7 @@ fn resolve_from_workbook() { color: Some(Color::rgb(1, 2, 3)), secondary: true, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }, @@ -249,6 +251,7 @@ fn resolve_from_workbook() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }, @@ -287,6 +290,7 @@ fn resolve_from_workbook() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }, @@ -298,6 +302,7 @@ fn resolve_from_workbook() { color: None, secondary: true, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }, @@ -314,6 +319,7 @@ fn resolve_from_workbook() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }]; @@ -331,6 +337,7 @@ fn resolve_from_workbook() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }]; @@ -1001,6 +1008,7 @@ fn pie_points_follow_workbook_theme() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }]; @@ -1057,3 +1065,27 @@ fn area_charts_draw_source_value_labels_only_when_enabled() { assert!(first_label > last_poly, "{kind:?}: label drawn under a later area"); } } + +#[test] +fn box_whisker_draws_each_series_quartile_method() { + use gridcraft_model::QuartileMethod; + let mut c = chart(ChartKind::BoxWhisker); + c.legend = LegendPos::None; + c.series = vec![Series::default(), Series { quartiles: QuartileMethod::Inclusive, ..Default::default() }]; + let color = [12, 34, 56, 255]; + let d = ChartData { + series: vec![series("A", &[1., 2., 3., 4., 5., 6.], color, c.kind), series("B", &[1., 2., 3., 4., 5., 6.], color, c.kind)], + ..Default::default() + }; + let prims = render(&c, &d, 640., 400., &ApproxMeasure); + let heights: Vec = prims + .iter() + .filter_map(|p| match p { + Prim::Rect { h, fill, .. } if *fill == Some(color) => Some(*h), + _ => None, + }) + .collect(); + assert_eq!(heights.len(), 2); + // 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); +} diff --git a/crates/engine/src/cmd/forecast.rs b/crates/engine/src/cmd/forecast.rs index d25b367ba..139d6fc46 100644 --- a/crates/engine/src/cmd/forecast.rs +++ b/crates/engine/src/cmd/forecast.rs @@ -411,6 +411,7 @@ fn forecast_sheet(s: &mut Session, p: &Json) -> Result { color: None, secondary: false, kind: (opts.column_chart && c >= 3).then_some(ChartKind::Line), + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }) diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index d878e4469..b320fd7fe 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -90,7 +90,7 @@ pub fn specs() -> Vec { "Chart Properties", ["Chart Design"], None, - "{chart?: id, type?, title?, legend?: none|bottom|top|left|right, dataLabels?, gridlines?, style?, xTitle?, yTitle?, at?, width?, height?, dx?, dy?}", + "{chart?: id, type?, title?, legend?: none|bottom|top|left|right, dataLabels?, gridlines?, quartiles?: exclusive|inclusive, style?, xTitle?, yTitle?, at?, width?, height?, dx?, dy?}", has_doc, chart_set ), @@ -603,6 +603,7 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }); @@ -618,6 +619,7 @@ pub fn series_from_range(sh: &Sheet, r: RangeRef, by_rows: bool) -> Vec color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }); @@ -788,12 +790,25 @@ fn find_chart(s: &Session, p: &Json) -> Result<(usize, usize)> { fn chart_set(s: &mut Session, p: &Json) -> Result { let (si, ci) = find_chart(s, p)?; let at = cell_param_on(s, p, "at", si)?; + let quartiles = p + .get("quartiles") + .map(|v| match v.as_str() { + Some("exclusive") => Ok(gridcraft_model::QuartileMethod::Exclusive), + Some("inclusive") => Ok(gridcraft_model::QuartileMethod::Inclusive), + _ => Err(bad("quartiles", "expected exclusive or inclusive")), + }) + .transpose()?; edit(s, |cx| { let sh = cx.sheet_mut(si)?; let Some(c) = sh.charts.get_mut(ci) else { return Ok(Json::Null) }; if let Some(t) = str_param(p, "type") { c.kind = chart_kind(t, str_param(p, "subtype").unwrap_or("")); } + if let Some(method) = quartiles { + for series in &mut c.series { + series.quartiles = method; + } + } if let Some(t) = p.get("title") { c.title = t.as_str().map(str::to_string); } diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index 6146bd4f9..e8a33fd50 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -1567,6 +1567,7 @@ pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }) diff --git a/crates/engine/src/sample.rs b/crates/engine/src/sample.rs index 4d4822445..129fe2171 100644 --- a/crates/engine/src/sample.rs +++ b/crates/engine/src/sample.rs @@ -108,6 +108,7 @@ fn sales(s: &mut Session) -> bool { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }]; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..a650b8271 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3282,3 +3282,30 @@ 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 box_whisker_quartile_methods_survive_xlsx() { + use gridcraft_model::QuartileMethod; + let mut s = s(); + chart_data(&mut s); + s.execute("insert.chart", json!({"range": "A1:C5", "type": "boxWhisker"})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().charts[0].series[0].quartiles, QuartileMethod::Exclusive); + s.execute("chart.set", json!({"quartiles": "inclusive"})).unwrap(); + assert!(s.doc().unwrap().wb.active().unwrap().charts[0].series.iter().all(|s| s.quartiles == QuartileMethod::Inclusive)); + for invalid in [json!("median"), json!(null), json!(1)] { + assert!(s.execute("chart.set", json!({"quartiles": invalid, "title": "Changed"})).is_err()); + } + assert_ne!(s.doc().unwrap().wb.active().unwrap().charts[0].title.as_deref(), Some("Changed")); + // Imported series can differ; changing a chart's setting applies to all its series. + let mut wb = (*s.doc().unwrap().wb).clone(); + let sh = wb.sheet_mut(0).unwrap(); + assert!(sh.charts[0].series.len() > 1); + sh.charts[0].series[1].quartiles = QuartileMethod::Exclusive; + let bytes = gridcraft_xlsx::write_xlsx(&wb).unwrap(); + let (back, _) = gridcraft_xlsx::read_xlsx(&bytes).unwrap(); + let series = &back.active().unwrap().charts[0].series; + assert_eq!(series[0].quartiles, QuartileMethod::Inclusive); + assert_eq!(series[1].quartiles, QuartileMethod::Exclusive); + s.execute("chart.set", json!({"quartiles": "exclusive"})).unwrap(); + assert!(s.doc().unwrap().wb.active().unwrap().charts[0].series.iter().all(|s| s.quartiles == QuartileMethod::Exclusive)); +} diff --git a/crates/model/src/features.rs b/crates/model/src/features.rs index b51f7bc98..01c98425e 100644 --- a/crates/model/src/features.rs +++ b/crates/model/src/features.rs @@ -406,6 +406,14 @@ pub enum LegendPos { Right, } +/// Quartile calculation for a box & whisker series. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub enum QuartileMethod { + #[default] + Exclusive, + Inclusive, +} + #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] pub struct Series { /// Formula text for the series name (`Sheet1!$B$1`) or a literal. @@ -421,6 +429,9 @@ pub struct Series { /// Draw line segments as a smoothed (spline) curve. #[serde(default)] pub smooth: bool, + /// Box & whisker quartiles; Excel defaults to exclusive. + #[serde(default)] + pub quartiles: QuartileMethod, /// Trendlines fitted to this series (Excel allows several per series). #[serde(default)] pub trendlines: Vec, diff --git a/crates/xlsx/src/chart.rs b/crates/xlsx/src/chart.rs index dee33c83d..1b893a3d2 100644 --- a/crates/xlsx/src/chart.rs +++ b/crates/xlsx/src/chart.rs @@ -106,6 +106,7 @@ fn read_series(s: &El) -> Series { color, secondary: false, kind: None, + quartiles: Default::default(), smooth: bool_val(s, "smooth"), trendlines: s.kids("trendline").filter_map(read_trendline).collect(), } diff --git a/crates/xlsx/src/chartex.rs b/crates/xlsx/src/chartex.rs index 2ab8c221b..e32fc63d0 100644 --- a/crates/xlsx/src/chartex.rs +++ b/crates/xlsx/src/chartex.rs @@ -6,7 +6,7 @@ use std::fmt::Write as _; use gridcraft_core::RangeRef; use gridcraft_formula::{Expr, SheetSel}; -use gridcraft_model::{Chart, ChartKind, LegendPos, Series, Workbook}; +use gridcraft_model::{Chart, ChartKind, LegendPos, QuartileMethod, Series, Workbook}; use crate::drawing::{dml_color, srgb}; use crate::xml::{El, esc, esc_attr}; @@ -99,6 +99,10 @@ pub fn read_chartex(root: &El) -> Option { let (cat, val) = data.iter().find(|(i, _, _)| i == id).map(|(_, c, v)| (c.clone(), v.clone())).unwrap_or_default(); data_labels |= s.path(&["dataLabels", "visibility"]).is_some_and(|v| v.flag("value", false)); series.push(Series { + quartiles: match s.path(&["layoutPr", "statistics"]).and_then(|el| el.attr("quartileMethod")) { + Some("inclusive") => QuartileMethod::Inclusive, + _ => QuartileMethod::Exclusive, + }, name: s.child("tx").and_then(text), categories: cat, values: val.unwrap_or_default(), @@ -309,9 +313,16 @@ pub fn write_chartex(wb: &Workbook, si: usize, ch: &Chart, names: &mut Vec"); match ch.kind { ChartKind::Histogram => s.push_str(""), - ChartKind::BoxWhisker => s.push_str( - "", - ), + ChartKind::BoxWhisker => { + let method = match ser.quartiles { + QuartileMethod::Exclusive => "exclusive", + QuartileMethod::Inclusive => "inclusive", + }; + let _ = write!( + s, + "" + ); + } ChartKind::Treemap => s.push_str(""), _ => {} } @@ -364,3 +375,26 @@ pub fn write_chartex(wb: &Workbook, si: usize, ch: &Chart, names: &mut Vec", QuartileMethod::Exclusive), + (r#""#, QuartileMethod::Exclusive), + (r#""#, QuartileMethod::Exclusive), + (r#""#, QuartileMethod::Inclusive), + ] { + let xml = format!( + r#"{statistics}"# + ); + let root = crate::xml::parse(xml.as_bytes()).unwrap(); + let chart = read_chartex(&root).unwrap(); + assert_eq!(chart.series[0].quartiles, expected); + } + } +} diff --git a/crates/xlsx/src/tests/literal_category_roundtrip.rs b/crates/xlsx/src/tests/literal_category_roundtrip.rs index c818de221..e9f6a4461 100644 --- a/crates/xlsx/src/tests/literal_category_roundtrip.rs +++ b/crates/xlsx/src/tests/literal_category_roundtrip.rs @@ -28,6 +28,7 @@ fn literal_category_with_separators_survives_save() { color: None, secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }) diff --git a/crates/xlsx/src/tests/roundtrip.rs b/crates/xlsx/src/tests/roundtrip.rs index 901802905..bedf79e5b 100644 --- a/crates/xlsx/src/tests/roundtrip.rs +++ b/crates/xlsx/src/tests/roundtrip.rs @@ -380,6 +380,7 @@ fn sample() -> Workbook { color: Some(Color::Rgb(0xFF0000)), secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }], diff --git a/crates/xlsx/src/tests/smooth_roundtrip.rs b/crates/xlsx/src/tests/smooth_roundtrip.rs index 5a4f68fb2..0b4ebcef8 100644 --- a/crates/xlsx/src/tests/smooth_roundtrip.rs +++ b/crates/xlsx/src/tests/smooth_roundtrip.rs @@ -17,6 +17,7 @@ fn smooth_line_setting_round_trips() { color: Some(Color::Rgb(0x4472C4)), secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines: Vec::new(), }; diff --git a/crates/xlsx/src/tests/trendline_roundtrip.rs b/crates/xlsx/src/tests/trendline_roundtrip.rs index d954c9bc6..02948a6c7 100644 --- a/crates/xlsx/src/tests/trendline_roundtrip.rs +++ b/crates/xlsx/src/tests/trendline_roundtrip.rs @@ -47,6 +47,7 @@ fn chart(id: u32, kind: ChartKind, trendlines: Vec) -> Chart { color: Some(Color::Rgb(0x4472C4)), secondary: false, kind: None, + quartiles: Default::default(), smooth: false, trendlines, }], diff --git a/docs/chart-parity.md b/docs/chart-parity.md index 9058b0935..9bb87339a 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-16 · **Change:** minor (box & whisker quartile methods are read, written and drawn per series; 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 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). @@ -25,7 +25,7 @@ open PRs #158, #161, and issue #190. | Radar | radar, with markers, filled | 1 of 3 | | | Treemap, Sunburst | 1 each | 2 of 2 | lose their kind in XLSX (PR #161) | | Histogram, Pareto | 2 | 1 of 2 | no Pareto | -| Box & Whisker | 1 | 1 of 1 | loses its kind in XLSX (PR #161) | +| 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) | | Filled map | 1 | 0 | needs geographic boundary data (licensing decision) | | Combo | custom combinations, secondary axis | yes (line on secondary axis) | | @@ -80,6 +80,7 @@ back. | Date | Change | Summary | |---|---|---| +| 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-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 3f8167719ac836536e4697a8a61604cf2b93db6a Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Sat, 17 Oct 2026 05:15:25 +0200 Subject: [PATCH 13/17] Keep chart source range in step with inserted and deleted lines Found while testing crates/engine/src/cmd/sheet.rs:216-238 (rewrite never updates Chart.source); used by crates/engine/src/cmd/insert.rs:849 (chart_switch) Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- crates/engine/src/cmd/sheet.rs | 2 ++ crates/engine/src/tests.rs | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index 7870dee4e..4f66bb1d6 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -226,6 +226,8 @@ fn rewrite(wb: &mut gridcraft_model::Workbook, target: &str, e: &Edit) { } }; for ch in sh.charts.iter_mut() { + // The source block moves with its cells; one that was deleted is dropped. + ch.source = ch.source.as_deref().map(fix).filter(|s| !s.contains("#REF!")); for se in ch.series.iter_mut() { se.values = fix(&se.values); se.categories = se.categories.as_deref().map(fix); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..27caa0f4c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2370,6 +2370,28 @@ fn switch_row_column_with_whole_column_refs_is_bounded() { assert_eq!(s.doc().unwrap().wb.active().unwrap().charts[0].series.len(), 2); } +#[test] +fn switch_row_column_follows_inserted_and_deleted_lines() { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [["", "Q1", "Q2"], ["North", 1, 2], ["South", 3, 4]]})).unwrap(); + s.execute("insert.chart", json!({"range": "A1:C3", "type": "column", "at": "F1"})).unwrap(); + s.execute("home.insertRows", json!({"rows": "1:1"})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().charts[0].source.as_deref(), Some("Sheet1!A2:C4")); + s.execute("chart.switchRowColumn", json!({})).unwrap(); + let c = s.doc().unwrap().wb.active().unwrap().charts[0].clone(); + let values: Vec<&str> = c.series.iter().map(|x| x.values.as_str()).collect(); + assert_eq!(values, ["Sheet1!$B$3:$C$3", "Sheet1!$B$4:$C$4"]); + let names: Vec> = c.series.iter().map(|x| x.name.as_deref()).collect(); + assert_eq!(names, [Some("Sheet1!$A$3"), Some("Sheet1!$A$4")]); + assert_eq!(c.series[0].categories.as_deref(), Some("Sheet1!$B$2:$C$2")); + // Columns shift the source too. + s.execute("home.insertColumns", json!({"cols": "A:A"})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().charts[0].source.as_deref(), Some("Sheet1!B2:D4")); + // Deleting the whole block leaves no stale source behind. + s.execute("home.deleteRows", json!({"rows": "2:4"})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().charts[0].source, None); +} + #[test] fn ink_strokes_and_ink_to_shape() { let mut s = s(); From d890a1a7da6f9907992d2c1bb9d009d19387c013 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 15 Oct 2026 17:41:11 +0500 Subject: [PATCH 14/17] Calc: a dynamic array of one value still anchors a spill range (#1491) `C1#` gave `#REF!` while the dynamic-array formula in C1 happened to return a single value -- a FILTER matching one row, a SORT or UNIQUE over one item -- so `ROWS(C1#)` and `SUM(C1#)` failed and `COUNT(C1#)` gave 0. Every formula that counts or sums a filtered list therefore broke the moment the list had exactly one entry. In Excel the formula is the anchor of a 1 x 1 range and the three give 1, the value, and 1. A pass recorded the spill only for an array larger than one cell: if a.rows > 1 || a.cols > 1 { self.spills.insert(k, a.clone()); } so a 1 x 1 result left no range for `spill_range` to answer with, and `eval` turned the missing range into `#REF!`. It is recorded unconditionally now. The report suggested keying this on the cell carrying `cm`, which turned out not to be expressible: nothing in the model says "dynamic array" positively -- the reader tells one from a legacy CSE formula only by leaving `Formula::array` unset. The arm itself is the better test, because a formula whose result is a scalar never reaches it: `=1+1` does not become an anchor, which a test pins. It also matches Excel 365, where `=A1:A1` is a 1 x 1 anchor of its own. --- crates/calc/src/recalc.rs | 8 +++++--- crates/calc/src/tests.rs | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index b391821d7..aa9da716f 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -517,9 +517,11 @@ impl PassHost<'_> { let data = a.data.iter().map(|x| if matches!(x, Value::Empty) { Value::Number(0.0) } else { x.clone() }).collect(); Array::new(a.rows, a.cols, data).map(std::sync::Arc::new).unwrap_or(a) }; - if a.rows > 1 || a.cols > 1 { - self.spills.insert(k, a.clone()); - } + // A dynamic-array formula anchors a spill range even while its result is one + // value -- a FILTER matching one row, a SORT or UNIQUE over one item -- so `C1#` + // is the 1 x 1 range and `ROWS(C1#)` is 1, as in Excel (#1491). A formula that + // returns a scalar never reaches this arm, so it does not become an anchor. + self.spills.insert(k, a.clone()); a.data.first().cloned().unwrap_or_default() } Value::Empty => Value::Number(0.0), diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 97f7a8350..15562b7c5 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -1294,3 +1294,41 @@ fn median_large_values_recalculate_dependents() { assert!((t.num(cell) + 1.3).abs() < 1e-14); } } + +#[test] +fn a_dynamic_array_of_one_value_still_anchors_a_spill() { + // In Excel a dynamic-array formula anchors a spill range even while its result is a single + // value, so `C1#` is the 1 x 1 range and ROWS/SUM/COUNT of it work. GridCraft gave #REF! + // (and COUNT 0), so every formula counting a filtered list broke the moment the list had + // exactly one entry (#1491). Values and cells follow the reported fixture. + let mut t = T::new(); + for i in 1..=5 { + t.set(&format!("A{i}"), &i.to_string()); + t.set(&format!("B{i}"), "7"); + } + t.set("C1", "=FILTER(A1:A5,A1:A5>4)"); + t.set("C3", "=SORT(FILTER(A1:A5,A1:A5=3))"); + t.set("C5", "=UNIQUE(B1:B5)"); + // Control: the same FILTER matching two rows, which worked before. + t.set("C9", "=FILTER(A1:A5,A1:A5>3)"); + + t.set("E1", "=ROWS(C1#)"); + t.set("E2", "=SUM(C1#)"); + t.set("E3", "=ROWS(C3#)"); + t.set("E5", "=COUNT(C5#)"); + t.set("E9", "=ROWS(C9#)"); + assert_eq!(t.get("E1"), Value::from(1.0)); + assert_eq!(t.get("E2"), Value::from(5.0)); + assert_eq!(t.get("E3"), Value::from(1.0)); + assert_eq!(t.get("E5"), Value::from(1.0)); + assert_eq!(t.get("E9"), Value::from(2.0), "a spilling array is unchanged"); + + // The anchors still show their first value, as they did. + assert_eq!(t.get("C1"), Value::from(5.0)); + assert_eq!(t.get("C5"), Value::from(7.0)); + + // A formula whose result is a scalar never becomes an anchor: `#` on it is still #REF!. + t.set("D1", "=1+1"); + t.set("E11", "=ROWS(D1#)"); + assert_eq!(t.get("E11"), Value::Error(CellError::Ref)); +} From 4445aa6ddfb4d87892fe0eb9b3c0b2be81458382 Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Mon, 19 Oct 2026 13:31:04 -0500 Subject: [PATCH 15/17] calc: document deref's full-shape rule for bounded references The deref doc comment still said blocks are trimmed to the used range. Describe the actual rule: blocks up to FULL_SIZE_CELLS, or up to FULL_SIZE_BOUNDED_CELLS when they do not run to the sheet's edge, keep their full shape; the rest stop at the used range. Docs only. Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/eval.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 517f2fe9e..0bc68b7b7 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -158,8 +158,13 @@ impl<'h> Evaluator<'h> { Ok(self.resolve_sheet(&r.sheet)?.into_iter().map(|sheet| Area { sheet, range }).collect()) } - /// Dereferences: one cell → its value; a block → an array (trimmed to the used range); - /// several areas → `#VALUE!`. + /// Dereferences: one cell → its value; a block → an array; several areas → `#VALUE!`. + /// + /// A block keeps its full shape (blanks included) when it holds at most `FULL_SIZE_CELLS` + /// cells, or when it holds at most `FULL_SIZE_BOUNDED_CELLS` cells (one full column's worth) + /// and does not run to the sheet's last row or column. Every other block (larger, or a + /// bigger one that runs to the sheet's edge, such as `A:A` or `1:1`) is trimmed to the + /// sheet's used range before its values are read. An array over `MAX_CELLS` is `#NUM!`. pub fn deref(&mut self, ev: Ev) -> Value { match ev { Ev::V(v) => v, From 13e730d42a54eea14f4b53dc9c8294c004a96ef5 Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Mon, 19 Oct 2026 13:36:56 -0500 Subject: [PATCH 16/17] calc: calculate every formula read at the depth limit in one suspension A formula at the nesting limit reading many uncalculated formulas (for example SUM over a wide range) suspended the abandoned evaluations once per cell read, restarting them each time. The other formulas read at the limit are now collected and calculated together with the suspended one, before the abandoned evaluations restart. Results, circular-reference and iteration behaviour are unchanged. Adds a regression test (3000 suspensions before, 1 after). Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/recalc.rs | 59 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 634e391ba..86a97b472 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -410,6 +410,13 @@ struct PassHost<'a> { waiting: HashSet, /// The formulas abandoned since the last suspension. abandoned: Vec, + /// Other formulas read at [`MAX_NESTED`] levels deep by the evaluations being abandoned. They + /// are calculated along with the suspended one, rather than one per restart (a wide range + /// read at the limit would restart the abandoned evaluations once per cell in it). + also: Vec, + /// Suspensions so far (tests check that a wide read at the limit needs one, not one per cell). + #[cfg(test)] + suspensions: usize, /// The formulas in `cycles`. cycle_seen: HashSet, /// Iterative calculation: a circular reference reads the cell's previous value instead of @@ -445,6 +452,9 @@ impl<'a> PassHost<'a> { suspend: None, waiting: HashSet::default(), abandoned: Vec::new(), + also: Vec::new(), + #[cfg(test)] + suspensions: 0, cycle_seen: HashSet::default(), iterate: false, prev: HashMap::default(), @@ -489,11 +499,25 @@ impl PassHost<'_> { self.compute(top); match self.suspend.take() { Some(next) => { + #[cfg(test)] + { + self.suspensions += 1; + } let held = std::mem::take(&mut self.abandoned); self.waiting.extend(held.iter().copied()); if let Some(entry) = work.last_mut() { entry.1 = held; } + // The other formulas read at the limit are calculated after `next`, before the + // held evaluations restart. Each has nothing held of its own, so the held ones + // are released only when the last of them is done. + let mut seen: HashSet = HashSet::default(); + seen.insert(next); + for s in std::mem::take(&mut self.also) { + if seen.insert(s) { + work.push((s, Vec::new())); + } + } work.push((next, Vec::new())); } None => { @@ -520,7 +544,11 @@ impl PassHost<'_> { return self.wb.sheet(k.0).map(|s| s.value(k.1)).unwrap_or_default(); }; if self.suspend.is_some() { - // Unwinding to `calculate`: the value is thrown away. + // Unwinding to `calculate`: the value is thrown away. A formula read at the limit is + // calculated with the suspended one, so the evaluations are not restarted for each. + if self.depth >= MAX_NESTED && !self.in_progress.contains(&k) && !self.waiting.contains(&k) { + self.also.push(k); + } return Value::Error(CellError::Calc); } if self.in_progress.contains(&k) || self.waiting.contains(&k) { @@ -1395,6 +1423,35 @@ pub(crate) mod tests { assert_eq!(deps, vec![at("DA9"), at("DA9")]); } + /// A formula at the nesting limit reading thousands of formulas not calculated yet suspends the + /// evaluations in progress once, not once per formula read. + #[test] + fn wide_read_at_the_nesting_limit_suspends_once() { + const WIDTH: u32 = 3000; + let mut wb = Workbook::new(); + let mut exprs = HashMap::default(); + let mut put = |wb: &mut Workbook, at: String, text: String| { + let at = CellRef::parse(&at).unwrap(); + wb.sheet_mut(0).unwrap().cells.set(at, Cell::formula(Formula::new(&text))); + exprs.insert((0, at), std::sync::Arc::new(gridcraft_formula::parse(&text[1..]).unwrap())); + }; + // A1 reads A2, ..., A64 (the 64th formula deep) reads a wide range of formulas. + for r in 1..64 { + put(&mut wb, format!("A{r}"), format!("=A{}+1", r + 1)); + } + put(&mut wb, "A64".to_string(), format!("=SUM(B1:B{WIDTH})")); + for r in 1..=WIDTH { + put(&mut wb, format!("B{r}"), format!("={r}*2")); + } + let mut rng = 1u64; + let mut host = PassHost::new(&wb, &exprs, &mut rng, 0.0); + host.calculate((0, CellRef::parse("A1").unwrap())); + let total = (WIDTH * (WIDTH + 1)) as f64; + assert_eq!(host.results.get(&(0, CellRef::parse("A1").unwrap())), Some(&Value::Number(total + 63.0))); + assert!(host.cycles.is_empty()); + assert!(host.suspensions <= 3, "{} suspensions", host.suspensions); + } + /// For tests of `PassHost::visit_range`, which can't be reached from outside this module: the /// values it hands over for `range` while the formulas in `pending` (cell, formula) have not been /// calculated yet, or `None` when it declines the block. From e49b2d8830557ae72d725723813b4334bb6696a9 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 02:35:30 +0000 Subject: [PATCH 17/17] Integration: #1550 keytip NR expects the Insert Chart dialog The generator no longer probes insert.recommendedCharts (the engine command errors without a chart type; the UI opens the dialog) and emits an assertion that the recommendedCharts dialog is open. keytips_generated.rs updated to match. --- crates/ui-egui/tests/keytips_generated.rs | 2 +- xtask/src/keytips.rs | 13 ++++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/tests/keytips_generated.rs b/crates/ui-egui/tests/keytips_generated.rs index 653554857..5f2acdbb0 100644 --- a/crates/ui-egui/tests/keytips_generated.rs +++ b/crates/ui-egui/tests/keytips_generated.rs @@ -1276,7 +1276,7 @@ fn keytip_nq_() { fn keytip_nr_() { let mut h = harness(blank()); sequence(&mut h, "NR", false); - assert!(h.state().message.is_some() || h.state().toast.is_some(), "NR must invoke insert.recommendedCharts and surface its missing-params error"); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name.as_str()), Some("recommendedCharts"), "NR must open the recommendedCharts dialog"); } #[test] diff --git a/xtask/src/keytips.rs b/xtask/src/keytips.rs index 36847222f..e4d423199 100644 --- a/xtask/src/keytips.rs +++ b/xtask/src/keytips.rs @@ -17,6 +17,10 @@ const TABS: &[&str] = &["Home", "Insert", "Draw", "Page Layout", "Formulas", "Da /// something observable" so the checked-in file stays byte-identical everywhere. const ENV_SENSITIVE: &[&str] = &["review.spelling"]; +/// Commands that the UI turns into a dialog when invoked menu-style with no params (the engine +/// command itself errors without them). Never probed; their tests assert the dialog opens. +const UI_DIALOG_COMMANDS: &[(&str, &str)] = &[("insert.recommendedCharts", "recommendedCharts")]; + /// Format generated source through `rustfmt` so the checked-in file matches /// the emitter byte-for-byte. fn rustfmt(src: &str) -> Result { @@ -57,7 +61,7 @@ fn probe_live_commands(ids: &[String]) -> Probe { use gridcraft_engine::Session; let mut map = Probe::new(); for id in ids { - if ENV_SENSITIVE.contains(&id.as_str()) { + if ENV_SENSITIVE.contains(&id.as_str()) || UI_DIALOG_COMMANDS.iter().any(|(c, _)| c == id) { continue; } let mut s = Session::new(); @@ -269,6 +273,13 @@ fn render_tests(entries: &[Entry], probe: &Probe) -> String { "sequence(&mut h, {seq:?}, false);\n assert!(h.state().dialog.is_none(), \"{seq} must not open a dialog\");\n assert!(h.state().message.is_none(), \"{seq} must not open a message box\");\n assert!(h.state().toast.is_none(), \"{seq} must not raise a toast\");\n assert!(h.state().session.journal.is_empty(), \"{seq} must not journal\");", seq = e.seq, ), + "command" if UI_DIALOG_COMMANDS.iter().any(|(c, _)| *c == e.target) => { + let dialog = UI_DIALOG_COMMANDS.iter().find(|(c, _)| *c == e.target).map_or("", |(_, d)| d); + format!( + "sequence(&mut h, {seq:?}, false);\n assert_eq!(h.state().dialog.as_ref().map(|d| d.name.as_str()), Some({dialog:?}), \"{seq} must open the {dialog} dialog\");", + seq = e.seq, + ) + } "command" => match probe.get(&e.target).copied().unwrap_or(2) { 0 => format!( "sequence(&mut h, {seq:?}, false);\n assert!(h.state().session.journal.iter().any(|(id, _)| id == {id:?}), \"{seq} must journal {id}\");",