From 8e30c095db6efd0137f25d405fd639fa8ee72ab2 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Tue, 13 Oct 2026 20:26:01 +0200 Subject: [PATCH 1/6] Charts: resolved data and rendering are cached between frames (#1139) Part of #1098. Charts were resolved and rendered on every frame. The primitives are now cached per document/sheet/chart and checked against a Weak (any write to the workbook moves or clones it), the zoom and pixels-per-point; capped, LRU, closed documents dropped. Co-Authored-By: Claude Sonnet 5.5 --- ROADMAP.md | 4 +- crates/ui-egui/src/chartcache.rs | 462 +++++++++++++++++++++++++++++++ crates/ui-egui/src/chartview.rs | 41 ++- crates/ui-egui/src/lib.rs | 7 + 4 files changed, 507 insertions(+), 7 deletions(-) create mode 100644 crates/ui-egui/src/chartcache.rs diff --git a/ROADMAP.md b/ROADMAP.md index 070552d07..f87ece42a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-12 · **Change:** minor (rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-13 · **Change:** trivial (charts are rendered once per change, not once per frame, #1139; previously minor: rows auto-fit oversized fonts on open and whole-row/column formatting) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -132,6 +132,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-13 | Performance: a chart is resolved and rendered once per change of its workbook, zoom or fonts instead of on every frame; idle frames, hovering and scrolling reuse it (#1139, part of #1098) | | 2026-10-12 | Rows auto-fit oversized fonts: opening a file and whole-row/column formatting grow non-custom rows to their tallest font (#833) | | 2026-10-12 | Number formats: parsing a format code is linear in its length (long runs of `[`, `,` or `/` no longer take quadratic time), and an unclosed `[` is a literal when splitting sections as well as when tokenizing (#810) | | 2026-10-12 | Number formats: a fraction denominator with 65,536 zeros no longer crashes; format codes are capped at 255 characters like Excel (longer codes display as General, `TEXT` gives `#VALUE!`) (#807) | @@ -193,6 +194,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-13 | trivial | Progress log: charts cached between frames (#1139) | | 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/ui-egui/src/chartcache.rs b/crates/ui-egui/src/chartcache.rs new file mode 100644 index 000000000..95455dd3b --- /dev/null +++ b/crates/ui-egui/src/chartcache.rs @@ -0,0 +1,462 @@ +//! Rendered charts kept between frames. +//! +//! Resolving a chart's series and laying it out is O(points). Doing it for every visible chart on +//! every frame kept the UI thread busy while scrolling a workbook with a few big charts (#1139, +//! part of #1098). The primitives a chart renders to depend on the workbook, the chart's place in +//! it, the zoom, the screen's pixels-per-point and the fonts text is measured with, so those are +//! what an entry is checked against. +//! +//! **Why "the workbook is unchanged" can be known without a change counter.** A document's +//! workbook is an `Arc` and its content is only reachable for writing through +//! `Arc::make_mut` (or by replacing the `Arc`), because the model has no interior mutability and +//! the code base forbids `unsafe`. While a `Weak` to the allocation exists, `make_mut` never +//! mutates it in place: it clones (other strong owners, e.g. an undo snapshot) or moves the value +//! to a new allocation (the `Weak` is cut loose). So if the document's current `Arc` still points +//! at the allocation the cache holds a `Weak` to, nothing was written since the entries were +//! built, however the write would have been made (command, undo, redo, recalculation, file load, +//! a direct edit from the UI, a future code path). The `Weak` also keeps the allocation from being +//! freed and reused, so a pointer match cannot be a stranger that landed on the same address. + +use std::collections::HashMap; +use std::sync::{Arc, Weak}; + +use gridcraft_chart::Prim; +use gridcraft_engine::model::Workbook; + +/// Most charts kept, over all documents. +const MAX_ENTRIES: usize = 64; +/// Most memory kept (estimated from the primitives), over all documents. +const MAX_BYTES: usize = 48 << 20; + +struct Entry { + /// Zoom and pixels-per-point the primitives were measured for (`f32::to_bits`). + z: u32, + ppp: u32, + prims: Arc<[Prim]>, + bytes: usize, + /// Tick of the last use; the smallest is evicted first. + used: u64, +} + +struct DocCharts { + /// The workbook the entries were built from. + wb: Weak, + /// (sheet index, index in the sheet's `charts`). The workbook is unchanged while the entries + /// live, so the pair names one chart. + charts: HashMap<(usize, usize), Entry>, +} + +#[derive(Default)] +pub(crate) struct ChartCache { + docs: HashMap, + tick: u64, + bytes: usize, + /// Charts built (resolved and rendered) so far; for tests. + #[cfg(test)] + pub(crate) builds: usize, +} + +fn prim_bytes(p: &Prim) -> usize { + std::mem::size_of::() + + match p { + Prim::Line { pts, .. } | Prim::Polygon { pts, .. } => pts.len().saturating_mul(std::mem::size_of::<[f32; 2]>()), + Prim::Text { text, .. } => text.len(), + _ => 0, + } +} + +impl ChartCache { + /// The primitives of chart `index` of `sheet` in document `uid` whose workbook is `wb`, for + /// zoom and pixels per point `(z, ppp)`. `build` renders the chart when there is no valid entry. + pub(crate) fn get( + &mut self, + uid: u64, + wb: &Arc, + sheet: usize, + index: usize, + (z, ppp): (f32, f32), + build: impl FnOnce() -> Vec, + ) -> Arc<[Prim]> { + self.tick = self.tick.wrapping_add(1); + let tick = self.tick; + let doc = self.docs.entry(uid).or_insert_with(|| DocCharts { wb: Arc::downgrade(wb), charts: HashMap::new() }); + if !std::ptr::eq(doc.wb.as_ptr(), Arc::as_ptr(wb)) { + // The document changed (or is another one with this uid): nothing built from the old + // workbook can be trusted. + let dropped: usize = doc.charts.values().map(|e| e.bytes).sum(); + doc.charts.clear(); + doc.wb = Arc::downgrade(wb); + self.bytes = self.bytes.saturating_sub(dropped); + } + let (zb, pb) = (z.to_bits(), ppp.to_bits()); + if let Some(e) = doc.charts.get_mut(&(sheet, index)) + && e.z == zb + && e.ppp == pb + { + e.used = tick; + return e.prims.clone(); + } + #[cfg(test)] + { + self.builds += 1; + } + let prims: Arc<[Prim]> = build().into(); + let bytes = prims.iter().map(prim_bytes).fold(0usize, usize::saturating_add); + let Some(doc) = self.docs.get_mut(&uid) else { return prims }; + if let Some(old) = doc.charts.remove(&(sheet, index)) { + self.bytes = self.bytes.saturating_sub(old.bytes); + } + // A chart that alone would fill a quarter of the budget is not worth keeping. + if bytes <= MAX_BYTES / 4 { + doc.charts.insert((sheet, index), Entry { z: zb, ppp: pb, prims: prims.clone(), bytes, used: tick }); + self.bytes = self.bytes.saturating_add(bytes); + self.evict(); + } + prims + } + + /// Drops least recently used entries until the caps hold. + fn evict(&mut self) { + loop { + let count: usize = self.docs.values().map(|d| d.charts.len()).sum(); + if count <= MAX_ENTRIES && self.bytes <= MAX_BYTES { + return; + } + let oldest = self.docs.iter().flat_map(|(uid, d)| d.charts.iter().map(move |(k, e)| (e.used, *uid, *k))).min(); + let Some((_, uid, key)) = oldest else { return }; + if let Some(e) = self.docs.get_mut(&uid).and_then(|d| d.charts.remove(&key)) { + self.bytes = self.bytes.saturating_sub(e.bytes); + } + } + } + + /// Forgets the documents that are not in `live` (closed workbooks). + pub(crate) fn retain_docs(&mut self, live: impl Iterator) { + let live: Vec = live.collect(); + let bytes = &mut self.bytes; + self.docs.retain(|uid, d| { + let keep = live.contains(uid); + if !keep { + *bytes = bytes.saturating_sub(d.charts.values().map(|e| e.bytes).sum()); + } + keep + }); + } + + /// Forgets everything (the fonts text was measured with changed). + pub(crate) fn clear(&mut self) { + self.docs.clear(); + self.bytes = 0; + } + + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.docs.values().map(|d| d.charts.len()).sum() + } + + #[cfg(test)] + pub(crate) fn docs(&self) -> usize { + self.docs.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::SheetApp; + use crate::chartview::tests::RENDERS; + use egui::{Event, vec2}; + use gridcraft_chart::ApproxMeasure; + use gridcraft_engine::Session; + use serde_json::json; + + type Harness = egui_kittest::Harness<'static, SheetApp>; + + fn wb() -> Arc { + Arc::new(Workbook::new()) + } + + fn rect_prim(w: f32) -> Vec { + vec![Prim::Rect { x: 0.0, y: 0.0, w, h: 1.0, fill: None, stroke: None, radius: 0.0 }] + } + + fn line_prim(points: usize) -> Vec { + vec![Prim::Line { pts: vec![[0.0, 0.0]; points], color: [0, 0, 0, 255], width: 1.0, dash: false }] + } + + #[test] + fn a_workbook_written_after_caching_is_never_taken_for_the_cached_one() { + let mut cache = ChartCache::default(); + let mut w = wb(); + let build = || rect_prim(1.0); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 1, "same workbook: a hit"); + // Sole owner of the Arc: without the Weak this write would happen in place, at the same address. + Arc::make_mut(&mut w).active_sheet = 0; + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 2, "a write through make_mut moved the workbook to a new allocation"); + // Shared with an undo snapshot: make_mut clones, the pointer changes as well. + let snapshot = w.clone(); + Arc::make_mut(&mut w).active_sheet = 0; + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 3); + // Replacing the Arc with an equal workbook (a reload) is a change too. + w = Arc::new((*snapshot).clone()); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 4); + // The old allocation is still held by the cache's Weak while its owner is gone, so a new + // workbook can't be mistaken for it by landing on the same address. + drop(snapshot); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + assert_eq!(cache.builds, 4); + } + + #[test] + fn zoom_scale_chart_and_document_are_part_of_the_key() { + let mut cache = ChartCache::default(); + let w = wb(); + let build = || rect_prim(1.0); + cache.get(1, &w, 0, 0, (1.0, 1.0), build); + cache.get(1, &w, 0, 0, (1.25, 1.0), build); + cache.get(1, &w, 0, 0, (1.25, 2.0), build); + cache.get(1, &w, 0, 1, (1.25, 2.0), build); + cache.get(1, &w, 1, 0, (1.25, 2.0), build); + cache.get(2, &w, 0, 0, (1.25, 2.0), build); + assert_eq!(cache.builds, 6); + cache.get(1, &w, 0, 1, (1.25, 2.0), build); + cache.get(2, &w, 0, 0, (1.25, 2.0), build); + assert_eq!(cache.builds, 6, "each of those is cached"); + } + + #[test] + fn the_cache_is_capped_in_entries_and_bytes_and_drops_the_least_recently_used() { + let mut cache = ChartCache::default(); + let w = wb(); + for i in 0..MAX_ENTRIES + 10 { + cache.get(1, &w, 0, i, (1.0, 1.0), || rect_prim(1.0)); + // Chart 0 is used all the time, so it survives. + cache.get(1, &w, 0, 0, (1.0, 1.0), || rect_prim(1.0)); + } + assert_eq!(cache.len(), MAX_ENTRIES); + let before = cache.builds; + cache.get(1, &w, 0, 0, (1.0, 1.0), || rect_prim(1.0)); + assert_eq!(cache.builds, before, "the busy chart is still there"); + cache.get(1, &w, 0, 1, (1.0, 1.0), || rect_prim(1.0)); + assert_eq!(cache.builds, before + 1, "the idle one was evicted"); + + // Bytes: each of these is ~4 MiB of points; only a handful fit. + let mut cache = ChartCache::default(); + let points = (4 << 20) / std::mem::size_of::<[f32; 2]>(); + for i in 0..40 { + cache.get(1, &w, 0, i, (1.0, 1.0), || line_prim(points)); + } + assert!(cache.bytes <= MAX_BYTES && cache.len() < 40, "{} entries, {} bytes", cache.len(), cache.bytes); + // One that would fill a quarter of the budget is not kept, and is still returned. + let mut cache = ChartCache::default(); + let big = cache.get(1, &w, 0, 0, (1.0, 1.0), || line_prim(MAX_BYTES / 4 / std::mem::size_of::<[f32; 2]>() + 1000)); + assert!(!big.is_empty()); + assert_eq!((cache.len(), cache.bytes), (0, 0)); + } + + #[test] + fn closed_documents_are_forgotten() { + let mut cache = ChartCache::default(); + let w = wb(); + cache.get(1, &w, 0, 0, (1.0, 1.0), || line_prim(10)); + cache.get(2, &w, 0, 0, (1.0, 1.0), || line_prim(10)); + cache.retain_docs([2].into_iter()); + assert_eq!((cache.docs(), cache.len()), (1, 1)); + cache.retain_docs(std::iter::empty()); + assert_eq!((cache.docs(), cache.len(), cache.bytes), (0, 0, 0)); + } + + /// A workbook with charts of several kinds over numbers, dates and categories. + fn corpus() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Date", "Sales", "Cost", "Share"]]})).unwrap(); + for i in 0..40u32 { + let date = format!("2024-{:02}-{:02}", 1 + i / 28, 1 + i % 28); + s.execute("cell.set", json!({"cell": format!("A{}", i + 2), "input": date})).unwrap(); + s.execute("cell.set", json!({"cell": format!("B{}", i + 2), "input": format!("{}", (i * 37) % 101)})).unwrap(); + s.execute("cell.set", json!({"cell": format!("C{}", i + 2), "input": format!("=B{}/2+{}", i + 2, i)})).unwrap(); + s.execute("cell.set", json!({"cell": format!("D{}", i + 2), "input": format!("{}", (i % 5) + 1)})).unwrap(); + } + for (i, (kind, range)) in [ + ("column", "A1:C41"), + ("line", "A1:C41"), + ("scatter", "B1:C41"), + ("pie", "A1:B6"), + ("combo", "A1:C41"), + ("area", "A1:B41"), + ("bar", "A1:D41"), + ("doughnut", "A1:B6"), + ] + .into_iter() + .enumerate() + { + s.execute("insert.chart", json!({"range": range, "type": kind, "at": format!("H{}", 2 + i * 16), "width": 320, "height": 220})).unwrap(); + } + s + } + + #[test] + fn cached_output_equals_a_fresh_render_for_a_corpus_of_charts() { + let s = corpus(); + let d = s.doc().unwrap(); + let sh = d.wb.active().unwrap(); + assert_eq!(sh.charts.len(), 8); + let mut cache = ChartCache::default(); + for (i, ch) in sh.charts.iter().enumerate() { + let fresh = || { + let data = gridcraft_chart::resolve(&d.wb, d.wb.active_sheet, ch); + gridcraft_chart::render(ch, &data, ch.anchor.width, ch.anchor.height, &ApproxMeasure) + }; + let expect = fresh(); + assert!(expect.len() > 3, "{:?} renders something", ch.kind); + let first = cache.get(d.uid, &d.wb, d.wb.active_sheet, i, (1.0, 1.0), fresh); + let second = cache.get(d.uid, &d.wb, d.wb.active_sheet, i, (1.0, 1.0), || panic!("must be a hit")); + assert_eq!(&*first, &expect[..], "{:?}: built entry", ch.kind); + assert_eq!(&*second, &expect[..], "{:?}: cached entry", ch.kind); + } + assert_eq!(cache.builds, 8); + } + + // --- frames --- + + fn harness(s: Session) -> Harness { + let mut h = egui_kittest::Harness::builder().with_size(vec2(1400.0, 900.0)).build_ui_state( + |ui, app: &mut SheetApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + SheetApp::new(s, Default::default()), + ); + h.run_steps(8); + h + } + + /// Two charts side by side, both in view, over A1:B12 (B3 is `=B2*2`). + fn two_charts() -> Session { + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "A1", "values": [["Day", "Sales"]]})).unwrap(); + for i in 2..=12u32 { + let input = if i == 3 { "=B2*2".to_string() } else { format!("{}", i * 3 % 17) }; + s.execute("cell.set", json!({"cell": format!("A{i}"), "input": format!("2024-03-{i:02}")})).unwrap(); + s.execute("cell.set", json!({"cell": format!("B{i}"), "input": input})).unwrap(); + } + s.execute("insert.chart", json!({"range": "A1:B12", "type": "column", "at": "D2", "width": 300, "height": 200})).unwrap(); + s.execute("insert.chart", json!({"range": "A1:B12", "type": "scatter", "at": "L2", "width": 300, "height": 200})).unwrap(); + s + } + + fn builds() -> usize { + RENDERS.with(|c| c.get()) + } + + /// Runs `change`, lets a few frames pass and returns how many charts were rendered meanwhile. + fn rendered_after(h: &mut Harness, change: impl FnOnce(&mut Harness)) -> usize { + let before = builds(); + change(h); + h.run_steps(4); + builds() - before + } + + fn exec(h: &mut Harness, id: &str, params: serde_json::Value) { + h.state_mut().session.execute(id, params).unwrap(); + } + + #[test] + fn frames_without_a_change_render_each_chart_once() { + let mut h = harness(two_charts()); + assert!(builds() >= 2, "the charts were drawn"); + let n = rendered_after(&mut h, |h| h.run_steps(30)); + assert_eq!(n, 0, "30 idle frames render nothing again"); + // Pointer movement and wheel scrolling (view state only) change no workbook. + let scroll_before = h.state().view().scroll; + let n = rendered_after(&mut h, |h| { + for i in 0..10 { + h.input_mut().events.push(Event::PointerMoved(egui::pos2(300.0 + i as f32 * 7.0, 400.0))); + h.input_mut().events.push(Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: vec2(0.0, -2.0), + phase: egui::TouchPhase::Move, + modifiers: egui::Modifiers::NONE, + }); + h.run_steps(1); + } + }); + assert_ne!(h.state().view().scroll, scroll_before, "the sheet did scroll"); + assert_eq!(n, 0, "hover and scrolling reuse the rendered charts"); + assert_eq!(h.state().chart_cache.lock().unwrap().len(), 2); + } + + #[test] + fn every_kind_of_change_renders_the_charts_again_exactly_once() { + let mut h = harness(two_charts()); + let cases: Vec<(&str, Box)> = vec![ + ("cell edit", Box::new(|h| exec(h, "cell.set", json!({"cell": "B5", "input": "99"})))), + ("recalc through a dependency", Box::new(|h| exec(h, "cell.set", json!({"cell": "B2", "input": "7"})))), + ("calculate now", Box::new(|h| exec(h, "formulas.calculateNow", json!({})))), + ("undo", Box::new(|h| exec(h, "edit.undo", json!({})))), + ("redo", Box::new(|h| exec(h, "edit.redo", json!({})))), + ("row hide", Box::new(|h| exec(h, "home.hideRows", json!({"rows": "6:7"})))), + ("chart resize", Box::new(|h| exec(h, "chart.set", json!({"chart": 1, "width": 360, "height": 240})))), + ("format change", Box::new(|h| exec(h, "home.bold", json!({"range": "A1:B1"})))), + ("number format", Box::new(|h| exec(h, "home.numberFormat", json!({"range": "B2:B12", "format": "0.00"})))), + ( + "document theme", + Box::new(|h| { + exec( + h, + "pageLayout.themeColors", + json!({"colors": ["#000000", "#FFFFFF", "#44546A", "#E7E6E6", "#C00000", "#ED7D31", "#A5A5A5", "#FFC000", "#5B9BD5", "#70AD47", "#0563C1", "#954F72"]}), + ) + }), + ), + ]; + for (what, change) in cases { + let n = rendered_after(&mut h, |h| change(h)); + assert_eq!(n, 2, "{what}: both visible charts rendered once"); + let n = rendered_after(&mut h, |h| h.run_steps(5)); + assert_eq!(n, 0, "{what}: and then nothing more"); + } + // Zoom is part of the key too. + let n = rendered_after(&mut h, |h| exec(h, "view.zoom", json!({"percent": 150}))); + assert_eq!(n, 2, "zoom"); + } + + #[test] + fn switching_sheets_and_loading_files_render_the_new_charts_once() { + let mut h = harness(two_charts()); + exec(&mut h, "home.insertSheet", json!({"before": 1})); + h.run_steps(4); + // The new sheet has no charts; coming back renders this sheet's two once. + let n = rendered_after(&mut h, |h| exec(h, "sheet.activate", json!({"sheet": 0}))); + assert_eq!(n, 2, "sheet switch"); + let n = rendered_after(&mut h, |h| h.run_steps(5)); + assert_eq!(n, 0); + + // File load: the same workbook opened from bytes is a new document with its own charts. + let bytes = h.state_mut().session.execute("file.saveBytes", json!({})).unwrap(); + let b64 = bytes["base64"].as_str().unwrap().to_string(); + let n = rendered_after(&mut h, |h| exec(h, "file.open", json!({"name": "again.xlsx", "base64": b64}))); + assert_eq!(n, 2, "file load"); + assert_eq!(h.state().session.documents().len(), 2); + let n = rendered_after(&mut h, |h| h.run_steps(5)); + assert_eq!(n, 0); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 2, "both open documents are cached"); + + // Back in the first document nothing is rendered: its entries were kept. + let n = rendered_after(&mut h, |h| h.state_mut().session.set_active(0)); + assert_eq!(n, 0, "the first document's charts were still cached"); + // Closing a document forgets its entries; the other one is shown (and rendered) now. + let n = rendered_after(&mut h, |h| exec(h, "file.close", json!({"force": true}))); + assert_eq!(h.state().session.documents().len(), 1); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); + assert_eq!(n, 0, "the remaining document's charts were cached too"); + } +} diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 180f15c21..56ec63898 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -2,7 +2,7 @@ //! we paint with egui), pictures and shapes. use std::collections::HashMap; -use std::sync::Mutex; +use std::sync::{Arc, Mutex}; use egui::epaint::{PathShape, PathStroke}; use egui::{Align2, Color32, FontId, Painter, Pos2, Rect, Stroke, StrokeKind, pos2, vec2}; @@ -243,6 +243,27 @@ fn paint_shape_text(p: &Painter, wb: &Workbook, sp: &gridcraft_engine::model::Sh clip.galley(pos, g, color); } +/// Resolves and renders `ch` (chart `ci` of sheet `si`), at zoom `z`. +fn render_chart(p: &Painter, z: f32, wb: &Workbook, si: usize, ch: &gridcraft_engine::model::Chart) -> Vec { + #[cfg(test)] + tests::RENDERS.with(|c| c.set(c.get() + 1)); + let data = gridcraft_chart::resolve(wb, si, ch); + let m = EguiMeasure { painter: p, z }; + gridcraft_chart::render(ch, &data, ch.anchor.width, ch.anchor.height, &m) +} + +/// The primitives of chart `ci` of sheet `si`: from the frame cache when the workbook, zoom and +/// screen scale are those it was rendered for, else rendered now (see [`crate::chartcache`]). +fn chart_prims(app: &SheetApp, p: &Painter, z: f32, wb: &Arc, si: usize, ci: usize, ch: &gridcraft_engine::model::Chart) -> Arc<[Prim]> { + // Only the active document's own workbook is cached: anything else cannot be tied to a uid. + let Some(d) = app.session.active().filter(|d| Arc::ptr_eq(&d.wb, wb)) else { + return render_chart(p, z, wb, si, ch).into(); + }; + let mut cache = app.chart_cache.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + cache.retain_docs(app.session.documents().iter().map(|d| d.uid)); + cache.get(d.uid, wb, si, ci, (z, p.ctx().pixels_per_point()), || render_chart(p, z, wb, si, ch)) +} + pub(crate) fn anchor_rect(geo: &Geo, sh: &Sheet, a: &Anchor) -> Rect { let x = geo.x(sh, a.cell.col) + a.dx * geo.z; let y = geo.y(sh, a.cell.row) + a.dy * geo.z; @@ -258,7 +279,7 @@ pub(crate) struct PaintedObject { pub rect: Rect, } -pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workbook, si: usize, sh: &Sheet) -> Vec { +pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Arc, si: usize, sh: &Sheet) -> Vec { let mut painted = Vec::new(); let mut done = |start: egui::layers::ShapeIdx, rect: Rect| painted.push(PaintedObject { shapes: start.0..p.add(egui::Shape::Noop).0, rect }); for sp in &sh.shapes { @@ -353,7 +374,7 @@ pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workboo } done(start, r); } - for ch in &sh.charts { + for (ci, ch) in sh.charts.iter().enumerate() { let r = anchor_rect(geo, sh, &ch.anchor); if !r.intersects(p.clip_rect()) { continue; @@ -362,9 +383,7 @@ pub(crate) fn paint_objects(app: &SheetApp, p: &Painter, geo: &Geo, wb: &Workboo // Soft shadow + white card. p.rect_filled(r.translate(vec2(0.0, 1.5)), 2.0, Color32::from_black_alpha(18)); p.rect_filled(r, 0.0, Color32::WHITE); - let data = gridcraft_chart::resolve(wb, si, ch); - let m = EguiMeasure { painter: p, z: geo.z }; - let prims = gridcraft_chart::render(ch, &data, ch.anchor.width, ch.anchor.height, &m); + let prims = chart_prims(app, p, geo.z, wb, si, ci, ch); paint_prims(&p.with_clip_rect(r.intersect(p.clip_rect())), r.min, &prims, geo.z); if app.selected_chart == Some(ch.id) { selection_frame(p, r); @@ -533,3 +552,13 @@ pub fn hit(app: &SheetApp, geo: &Geo, sh: &Sheet, p: Pos2) -> Option<(&'static s } None } + +#[cfg(test)] +pub(crate) mod tests { + use std::cell::Cell; + + thread_local! { + /// Charts resolved and rendered on this thread (a test runs on its own thread). + pub(crate) static RENDERS: Cell = const { Cell::new(0) }; + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f96cdac3c..20e9aa99f 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -9,6 +9,7 @@ pub mod backstage; mod border_preview; mod cell_pictures; +mod chartcache; pub mod chartview; pub mod control; pub mod credits; @@ -254,6 +255,8 @@ pub struct SheetApp { pub(crate) shots: control::Shots, /// Chart selected on the sheet (id). pub selected_chart: Option, + /// Rendered charts kept between frames ([`chartcache`]). + pub(crate) chart_cache: std::sync::Mutex, /// The system clipboard's text when GridCraft last copied without publishing to it (a /// control-channel or MCP `edit.copy`/`edit.cut`). While the system clipboard still holds this /// text, GridCraft's copy is the newer one and Paste uses it. @@ -303,6 +306,7 @@ impl SheetApp { keytips: keytips::KeyTips::default(), shots: control::Shots::default(), selected_chart: None, + chart_cache: Default::default(), clipboard_text_at_copy: None, cut_text: None, started: now_ms(), @@ -1001,6 +1005,8 @@ impl SheetApp { // New fonts apply from the next frame on: paint nothing until then. if self.fonts_set { self.fonts_ready = true; + // The new set is active now: chart text was measured with the old one. + self.chart_cache.get_mut().unwrap_or_else(std::sync::PoisonError::into_inner).clear(); } else { SheetApp::setup_context_for_language(ctx, self.ui.dark, self.ui.language); self.fonts_set = true; @@ -1013,6 +1019,7 @@ impl SheetApp { } else if self.fonts_rebind { // A font-set change made last frame is active now. self.fonts_rebind = false; + self.chart_cache.get_mut().unwrap_or_else(std::sync::PoisonError::into_inner).clear(); } else if fonts::scan_done() { // Bind the workbook's installed fonts once the background scan has finished. Adding // fonts re-applies the font set, which egui activates at the next pass, so skip this From 97e43b84fab993b16a1de8ed4f8d375f8a88a3ea Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 25 Oct 2026 23:59:30 +0100 Subject: [PATCH 2/6] Chart cache test: set the MouseWheel source field added upstream Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/chartcache.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ui-egui/src/chartcache.rs b/crates/ui-egui/src/chartcache.rs index 95455dd3b..b68a7d2a1 100644 --- a/crates/ui-egui/src/chartcache.rs +++ b/crates/ui-egui/src/chartcache.rs @@ -382,6 +382,7 @@ mod tests { h.input_mut().events.push(Event::PointerMoved(egui::pos2(300.0 + i as f32 * 7.0, 400.0))); h.input_mut().events.push(Event::MouseWheel { unit: egui::MouseWheelUnit::Point, + source: egui::MouseWheelSource::Unknown, delta: vec2(0.0, -2.0), phase: egui::TouchPhase::Move, modifiers: egui::Modifiers::NONE, From 4f372ff470e9b46ee018e64acc69c696f3813c7d Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 29 Oct 2026 23:09:29 +0100 Subject: [PATCH 3/6] Name the test case type (clippy type_complexity) --- crates/ui-egui/src/chartcache.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/chartcache.rs b/crates/ui-egui/src/chartcache.rs index 0917c22c9..2eb8510ad 100644 --- a/crates/ui-egui/src/chartcache.rs +++ b/crates/ui-egui/src/chartcache.rs @@ -398,7 +398,8 @@ mod tests { #[test] fn every_kind_of_change_renders_the_charts_again_exactly_once() { let mut h = harness(two_charts()); - let cases: Vec<(&str, Box)> = vec![ + type Case<'a> = (&'a str, Box); + let cases: Vec = vec![ ("cell edit", Box::new(|h| exec(h, "cell.set", json!({"cell": "B5", "input": "99"})))), ("recalc through a dependency", Box::new(|h| exec(h, "cell.set", json!({"cell": "B2", "input": "7"})))), ("calculate now", Box::new(|h| exec(h, "formulas.calculateNow", json!({})))), From 8e3fba8a25376f4852dba1b0c8f1edda021d7354 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sat, 7 Nov 2026 16:36:02 +0100 Subject: [PATCH 4/6] Chart cache: drop closed documents once per frame, skip volatile series Move retain_docs out of the per-chart paint path into SheetApp::logic so closed documents are forgotten even when no chart is painted. Charts whose series call NOW, TODAY, RAND, RANDBETWEEN or RANDARRAY bypass the cache because their resolved values change between evaluations. Co-Authored-By: Claude Sonnet 5.5 --- crates/chart/src/lib.rs | 2 +- crates/chart/src/resolve.rs | 20 ++++++++++++++++++++ crates/chart/src/tests.rs | 29 +++++++++++++++++++++++++++++ crates/ui-egui/src/app.rs | 1 + crates/ui-egui/src/chartcache.rs | 31 +++++++++++++++++++++++++++++++ crates/ui-egui/src/chartview.rs | 5 ++++- 6 files changed, 86 insertions(+), 2 deletions(-) diff --git a/crates/chart/src/lib.rs b/crates/chart/src/lib.rs index 4b68b8d17..f4e2895fe 100644 --- a/crates/chart/src/lib.rs +++ b/crates/chart/src/lib.rs @@ -26,7 +26,7 @@ use serde::{Deserialize, Serialize}; pub use axis::{Scale, format_number, nice_scale}; pub use raster::rasterize; pub use render::{Swatch, legend_entries, render, supports_trendline}; -pub use resolve::{default_palette, resolve, series_color, series_plot_kind}; +pub use resolve::{default_palette, has_volatile_series, resolve, series_color, series_plot_kind}; pub use sparkline::render_sparkline; /// Straight (non-premultiplied) RGBA colour. diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index de3cdef1b..53dc177f1 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -104,6 +104,26 @@ fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, depth: u32) -> Vec bool { + const VOLATILE: [&str; 5] = ["NOW", "TODAY", "RAND", "RANDBETWEEN", "RANDARRAY"]; + let calls_volatile = |f: &str| { + // Formulas read from a file may carry the `_xlfn.` prefix of newer functions. + let up = f.to_ascii_uppercase().replace("_XLFN.", ""); + VOLATILE.iter().any(|name| { + up.match_indices(name).any(|(i, _)| { + let before = up[..i].chars().next_back(); + let after = up[i + name.len()..].trim_start(); + before.is_none_or(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) && after.starts_with('(') + }) + }) + }; + ch.series.iter().any(|s| { + [s.name.as_deref(), s.categories.as_deref(), Some(s.values.as_str()), s.bubble_sizes.as_deref()].into_iter().flatten().any(calls_volatile) + }) +} + fn to_num(v: &Value) -> Option { match v { Value::Number(n) if n.is_finite() => Some(*n), diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 6e423802c..2e08f3d68 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1330,3 +1330,32 @@ fn a_series_named_by_a_name_on_a_sheet_with_a_bang_resolves() { let d = resolve(&wb, 0, &c); assert_eq!(d.series.first().map(|s| s.name.as_str()), Some("Takings")); } + +#[test] +fn volatile_series_formulas_are_detected() { + let with = |values: &str, name: Option<&str>| { + let mut c = chart(ChartKind::Line); + c.series.push(Series { + name: name.map(Into::into), + categories: None, + values: values.into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + smooth: false, + quartiles: Default::default(), + trendlines: vec![], + }); + c + }; + assert!(has_volatile_series(&with("=NOW()", None))); + assert!(has_volatile_series(&with("=SUM(A1:A3)*rand ()", None))); + assert!(has_volatile_series(&with("Sheet1!$B$2:$B$9", Some("=TODAY()")))); + assert!(has_volatile_series(&with("=RANDARRAY(5)", None))); + assert!(has_volatile_series(&with("=_xlfn.RANDARRAY(5)", None))); + assert!(!has_volatile_series(&with("Sheet1!$B$2:$B$9", Some("Sheet1!$B$1")))); + assert!(!has_volatile_series(&with("=MYRAND(A1)", None))); + assert!(!has_volatile_series(&with("=Sheet1!$NOW", None))); + assert!(!has_volatile_series(&chart(ChartKind::Line))); +} diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index b67e37d99..535c91a5c 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -115,6 +115,7 @@ impl SheetApp { // Floating pictures' textures go with their workbook, even when no sheet is painted. let open: Vec = self.session.documents().iter().map(|d| d.uid).collect(); floating_pictures::forget_closed(ctx, &open); + self.chart_cache.get_mut().unwrap_or_else(std::sync::PoisonError::into_inner).retain_docs(open.iter().copied()); self.apply_font_state(ctx); if let Some(answer) = self.desktop_theme.take_change() { self.desktop_dark = Some(answer); diff --git a/crates/ui-egui/src/chartcache.rs b/crates/ui-egui/src/chartcache.rs index 2eb8510ad..82e9a96a2 100644 --- a/crates/ui-egui/src/chartcache.rs +++ b/crates/ui-egui/src/chartcache.rs @@ -461,4 +461,35 @@ mod tests { assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); assert_eq!(n, 0, "the remaining document's charts were cached too"); } + + #[test] + fn a_chart_with_a_volatile_series_is_rendered_on_every_frame() { + let mut h = harness(two_charts()); + { + let d = h.state_mut().session.active_mut().unwrap(); + let sh = Arc::make_mut(&mut d.wb).sheet_mut(0).unwrap(); + sh.charts[0].series[0].values = "=RAND()".into(); + } + h.run_steps(4); + let n = rendered_after(&mut h, |h| h.run_steps(6)); + assert!(n >= 6, "the volatile chart is resolved again on each frame, got {n}"); + assert!(n < 12, "the other chart still comes from the cache, got {n}"); + } + + #[test] + fn closed_documents_are_forgotten_without_painting_a_chart() { + let mut h = harness(two_charts()); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); + exec(&mut h, "file.new", json!({})); + h.run_steps(4); + assert_eq!(h.state().session.documents().len(), 2); + // The charts' document is no longer shown, but still open. + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 1); + h.state_mut().session.set_active(0); + h.run_steps(2); + exec(&mut h, "file.close", json!({"force": true, "document": true})); + h.run_steps(2); + assert_eq!(h.state().session.documents().len(), 1); + assert_eq!(h.state().chart_cache.lock().unwrap().docs(), 0, "no chart is painted, yet the closed document's entries are gone"); + } } diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 0db26f4da..c769dbc1c 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -258,8 +258,11 @@ fn chart_prims(app: &SheetApp, p: &Painter, z: f32, wb: &Arc, si: usiz let Some(d) = app.session.active().filter(|d| Arc::ptr_eq(&d.wb, wb)) else { return render_chart(p, z, wb, si, ch).into(); }; + // A series that calls NOW or RAND must be evaluated again on every paint. + if gridcraft_chart::has_volatile_series(ch) { + return render_chart(p, z, wb, si, ch).into(); + } let mut cache = app.chart_cache.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - cache.retain_docs(app.session.documents().iter().map(|d| d.uid)); cache.get(d.uid, wb, si, ci, (z, p.ctx().pixels_per_point()), || render_chart(p, z, wb, si, ch)) } From 8605fb02f0da1c6fd96a45bf13999f28e157d157 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 12:46:16 +0100 Subject: [PATCH 5/6] Charts: follow defined names when deciding a chart is volatile A series like =FreshClock, where the workbook name FreshClock is NOW(), was cached because only the literal series text was searched for volatile calls. The check now also follows defined names the formulas use (workbook or sheet scope, bare or sheet-qualified, LAMBDA names), the way the evaluator looks them up. Nesting deeper than 8, cycles and more than 64 lookups per chart count as volatile, so a chart whose stability isn't established is not cached. Co-Authored-By: Claude Opus 5.5 --- crates/chart/src/resolve.rs | 143 +++++++++++++++++++++++++++----- crates/chart/src/tests.rs | 82 ++++++++++++++++-- crates/ui-egui/src/chartview.rs | 5 +- 3 files changed, 198 insertions(+), 32 deletions(-) diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index 92ce0eb23..29158ebc7 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -155,26 +155,131 @@ fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, limit: usize, depth: } } -/// Whether a series formula of `ch` calls a function whose result changes between evaluations -/// (`NOW`, `TODAY`, `RAND`, `RANDBETWEEN`, `RANDARRAY`), so a result resolved earlier is stale. -pub fn has_volatile_series(ch: &Chart) -> bool { - const VOLATILE: [&str; 5] = ["NOW", "TODAY", "RAND", "RANDBETWEEN", "RANDARRAY"]; - let calls_volatile = |f: &str| { - // Formulas read from a file may carry the `_xlfn.` prefix of newer functions. - let up = f.to_ascii_uppercase().replace("_XLFN.", ""); - VOLATILE.iter().any(|name| { - up.match_indices(name).any(|(i, _)| { - let before = up[..i].chars().next_back(); - let after = up[i + name.len()..].trim_start(); - before.is_none_or(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) && after.starts_with('(') - }) - }) - }; +/// Whether a series formula of `ch` (chart of `sheet`) calls a function whose result changes +/// between evaluations (`NOW`, `TODAY`, `RAND`, `RANDBETWEEN`, `RANDARRAY`), so a result resolved +/// earlier is stale. Defined names the formulas use (`=FreshClock`, `Sheet1!Rates`, a LAMBDA name) +/// are followed into their own formulas, the way the evaluator looks them up; each name once per +/// chart, so a cycle ends. Names nested deeper than [`MAX_NAME_DEPTH`], or more than +/// [`MAX_NAMES_FOLLOWED`] distinct names, count as volatile: a chart whose stability isn't +/// established is not cached. +pub fn has_volatile_series(wb: &Workbook, sheet: usize, ch: &Chart) -> bool { + let mut seen = Seen::new(); ch.series.iter().any(|s| { - [s.name.as_deref(), s.categories.as_deref(), Some(s.values.as_str()), s.bubble_sizes.as_deref()].into_iter().flatten().any(calls_volatile) + [s.name.as_deref(), s.categories.as_deref(), Some(s.values.as_str()), s.bubble_sizes.as_deref()] + .into_iter() + .flatten() + .any(|f| formula_volatile(wb, sheet, f, 0, &mut seen)) }) } +/// Names followed inside names before a formula counts as volatile. +const MAX_NAME_DEPTH: u32 = 8; +/// Distinct defined names followed per chart before the chart counts as volatile. +const MAX_NAMES_FOLLOWED: usize = 64; + +/// Defined names already followed for one chart, each with the sheet its formula is read on. +type Seen<'a> = Vec<(&'a gridcraft_model::DefinedName, usize)>; + +fn formula_volatile<'a>(wb: &'a Workbook, sheet: usize, f: &str, depth: u32, seen: &mut Seen<'a>) -> bool { + if calls_volatile(f) { + return true; + } + name_tokens(f).into_iter().any(|t| { + let Some((n, at)) = defined_name(wb, sheet, t) else { return false }; + // Followed already, or being followed (a cycle): a volatile call in it is found there. + if seen.iter().any(|&(m, s)| std::ptr::eq(m, n) && s == at) { + return false; + } + if depth >= MAX_NAME_DEPTH || seen.len() >= MAX_NAMES_FOLLOWED { + return true; + } + seen.push((n, at)); + formula_volatile(wb, at, &n.formula, depth + 1, seen) + }) +} + +/// Whether formula text `f` calls one of the volatile functions by name. +fn calls_volatile(f: &str) -> bool { + const VOLATILE: [&str; 5] = ["NOW", "TODAY", "RAND", "RANDBETWEEN", "RANDARRAY"]; + // Formulas read from a file may carry the `_xlfn.` prefix of newer functions. + let up = f.to_ascii_uppercase().replace("_XLFN.", ""); + VOLATILE.iter().any(|name| { + up.match_indices(name).any(|(i, _)| { + let before = up[..i].chars().next_back(); + let after = up[i + name.len()..].trim_start(); + before.is_none_or(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) && after.starts_with('(') + }) + }) +} + +fn is_ident_char(c: char) -> bool { + c.is_alphanumeric() || c == '_' || c == '.' || c == '\\' +} + +/// End of the identifier starting at byte `from` of `f`. +fn ident_end(f: &str, from: usize) -> usize { + f[from..].char_indices().find(|&(_, c)| !is_ident_char(c)).map_or(f.len(), |(j, _)| from + j) +} + +/// End (past the closing quote) of the `q`-quoted text opening at byte `from`; a doubled quote +/// is an escaped one. `None` when it is not closed. +fn quoted_end(f: &str, from: usize, q: char) -> Option { + let mut i = from + q.len_utf8(); + loop { + let j = i + f[i..].find(q)?; + if f[j + q.len_utf8()..].starts_with(q) { + i = j + 2 * q.len_utf8(); + } else { + return Some(j + q.len_utf8()); + } + } +} + +/// The words of formula text `f` that may be defined names: `Name`, `Sheet1!Name`, +/// `'Bob''s'!Name`. String literals and numbers are skipped; function names, column letters and +/// the like come along too and simply aren't found as names. +fn name_tokens(f: &str) -> Vec<&str> { + let mut out = Vec::new(); + let mut i = 0; + while let Some(c) = f[i..].chars().next() { + let start = i; + let end = match c { + '"' => { + i = quoted_end(f, i, '"').unwrap_or(f.len()); + continue; + } + '\'' => quoted_end(f, i, '\'').unwrap_or(f.len()), + c if is_ident_char(c) => ident_end(f, i), + c => { + i += c.len_utf8(); + continue; + } + }; + i = end; + if f[end..].starts_with('!') { + let e = ident_end(f, end + 1); + if e > end + 1 { + out.push(&f[start..e]); + } + i = e; + } else if c != '\'' && !c.is_ascii_digit() { + out.push(&f[start..end]); + } + } + out +} + +/// The defined name `t` (`Name` or `Sheet1!Name`) seen from `sheet`, and the sheet its own +/// formula is read on. +fn defined_name<'a>(wb: &'a Workbook, sheet: usize, t: &str) -> Option<(&'a gridcraft_model::DefinedName, usize)> { + // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). + let (at, bare) = match gridcraft_core::split_sheet(t) { + (Some(s), bare) => (wb.sheet_index(&s)?, bare), + (None, bare) => (sheet, bare), + }; + wb.name(bare, at).map(|n| (n, n.scope.unwrap_or(at))) +} + fn to_num(v: &Value) -> Option { match v { Value::Number(n) if n.is_finite() => Some(*n), @@ -278,11 +383,7 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str, limi /// Whether `t` names a defined name seen from `sheet` (`Name`, or `Sheet1!Name` looked up on /// that sheet), the way the evaluator looks names up. fn is_defined_name(wb: &Workbook, sheet: usize, t: &str) -> bool { - // `'Bob''s'!Name`: the sheet is unquoted as everywhere else (#2012). - match gridcraft_core::split_sheet(t) { - (Some(s), bare) => wb.sheet_index(&s).is_some_and(|i| wb.name(bare, i).is_some()), - (None, _) => wb.name(t, sheet).is_some(), - } + defined_name(wb, sheet, t).is_some() } /// A series name: a reference/formula or a defined name (`[0]!Title` in a file) is evaluated, diff --git a/crates/chart/src/tests.rs b/crates/chart/src/tests.rs index 1ecabd997..a6d845387 100644 --- a/crates/chart/src/tests.rs +++ b/crates/chart/src/tests.rs @@ -1683,6 +1683,7 @@ fn a_series_named_by_a_name_on_a_sheet_with_a_bang_resolves() { #[test] fn volatile_series_formulas_are_detected() { + let wb = Workbook::new(); let with = |values: &str, name: Option<&str>| { let mut c = chart(ChartKind::Line); c.series.push(Series { @@ -1699,13 +1700,76 @@ fn volatile_series_formulas_are_detected() { }); c }; - assert!(has_volatile_series(&with("=NOW()", None))); - assert!(has_volatile_series(&with("=SUM(A1:A3)*rand ()", None))); - assert!(has_volatile_series(&with("Sheet1!$B$2:$B$9", Some("=TODAY()")))); - assert!(has_volatile_series(&with("=RANDARRAY(5)", None))); - assert!(has_volatile_series(&with("=_xlfn.RANDARRAY(5)", None))); - assert!(!has_volatile_series(&with("Sheet1!$B$2:$B$9", Some("Sheet1!$B$1")))); - assert!(!has_volatile_series(&with("=MYRAND(A1)", None))); - assert!(!has_volatile_series(&with("=Sheet1!$NOW", None))); - assert!(!has_volatile_series(&chart(ChartKind::Line))); + assert!(has_volatile_series(&wb, 0, &with("=NOW()", None))); + assert!(has_volatile_series(&wb, 0, &with("=SUM(A1:A3)*rand ()", None))); + assert!(has_volatile_series(&wb, 0, &with("Sheet1!$B$2:$B$9", Some("=TODAY()")))); + assert!(has_volatile_series(&wb, 0, &with("=RANDARRAY(5)", None))); + assert!(has_volatile_series(&wb, 0, &with("=_xlfn.RANDARRAY(5)", None))); + assert!(!has_volatile_series(&wb, 0, &with("Sheet1!$B$2:$B$9", Some("Sheet1!$B$1")))); + assert!(!has_volatile_series(&wb, 0, &with("=MYRAND(A1)", None))); + assert!(!has_volatile_series(&wb, 0, &with("=Sheet1!$NOW", None))); + assert!(!has_volatile_series(&wb, 0, &chart(ChartKind::Line))); +} + +#[test] +fn volatile_defined_names_are_followed() { + let with = |values: &str| { + let mut c = chart(ChartKind::Line); + c.series.push(Series { + name: None, + categories: None, + values: values.into(), + bubble_sizes: None, + color: None, + secondary: false, + kind: None, + smooth: false, + quartiles: Default::default(), + trendlines: vec![], + }); + c + }; + let name = |name: &str, scope: Option, formula: &str| gridcraft_model::DefinedName { + name: name.into(), + scope, + formula: formula.into(), + comment: String::new(), + hidden: false, + }; + let mut wb = Workbook::new(); + wb.sheet_mut(0).unwrap().name = "Bob's".into(); + wb.names.push(name("FreshClock", None, "NOW()")); + wb.names.push(name("Later", None, "FreshClock+1")); + wb.names.push(name("Dice", Some(0), "RANDBETWEEN(1,6)")); + wb.names.push(name("Rates", None, "'Bob''s'!$A$1:$A$3")); + wb.names.push(name("Loop", None, "Loop+1")); + wb.names.push(name("Spin", None, "Spin+NOW()")); + wb.names.push(name("Roll", None, "LAMBDA(n,n*RAND())")); + // A workbook-scoped name, a name used inside another, and a sheet-scoped one, bare or qualified. + assert!(has_volatile_series(&wb, 0, &with("=FreshClock"))); + assert!(has_volatile_series(&wb, 0, &with("=SUM(Later, 1)"))); + assert!(has_volatile_series(&wb, 0, &with("=Dice*2"))); + assert!(has_volatile_series(&wb, 0, &with("='Bob''s'!Dice"))); + assert!(has_volatile_series(&wb, 0, &with("=Roll(3)"))); + // A self-referencing name ends (it evaluates to an error, not a changing value); one that + // also calls NOW is volatile. + assert!(!has_volatile_series(&wb, 0, &with("=Loop"))); + assert!(has_volatile_series(&wb, 0, &with("=Spin"))); + // Many series over the same stable name stay cacheable: each name is followed once. + let mut many = with("=Rates"); + let first = many.series[0].clone(); + many.series.extend(std::iter::repeat_n(first, 100)); + assert!(!has_volatile_series(&wb, 0, &many)); + // Names nested deeper than the limit are not known to be stable. + for i in 0..10 { + wb.names.push(name(&format!("Deep{i}"), None, &format!("Deep{}", i + 1))); + } + assert!(has_volatile_series(&wb, 0, &with("=Deep0"))); + // A name that refers to cells, a name inside a string and a name of another scope are stable. + assert!(!has_volatile_series(&wb, 0, &with("=Rates"))); + assert!(!has_volatile_series(&wb, 0, &with("=\"FreshClock\""))); + // Dice belongs to sheet 0: seen from another sheet it isn't defined. + wb.sheets.push(Arc::new(gridcraft_model::Sheet::new("Other"))); + assert!(!has_volatile_series(&wb, 1, &with("=Dice"))); + assert!(has_volatile_series(&wb, 1, &with("=FreshClock"))); } diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index 6196878b4..a95b97d57 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -258,8 +258,9 @@ fn chart_prims(app: &SheetApp, p: &Painter, z: f32, wb: &Arc, si: usiz let Some(d) = app.session.active().filter(|d| Arc::ptr_eq(&d.wb, wb)) else { return render_chart(p, z, wb, si, ch).into(); }; - // A series that calls NOW or RAND must be evaluated again on every paint. - if gridcraft_chart::has_volatile_series(ch) { + // A series that calls NOW or RAND, directly or through a defined name, must be evaluated + // again on every paint. + if gridcraft_chart::has_volatile_series(wb, si, ch) { return render_chart(p, z, wb, si, ch).into(); } let mut cache = app.chart_cache.lock().unwrap_or_else(std::sync::PoisonError::into_inner); From bf76e6e63b352350079bbd1187e44adceb54f0c0 Mon Sep 17 00:00:00 2001 From: Codex Date: Thu, 12 Nov 2026 19:41:19 +0100 Subject: [PATCH 6/6] test: keep ref-edit three-sheet check clippy-clean --- crates/ui-egui/tests/it/ref_edit.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/tests/it/ref_edit.rs b/crates/ui-egui/tests/it/ref_edit.rs index fb3bf0cb6..b14a75da0 100644 --- a/crates/ui-egui/tests/it/ref_edit.rs +++ b/crates/ui-egui/tests/it/ref_edit.rs @@ -1064,8 +1064,8 @@ fn the_windows_cancel_with_an_unsavable_draft_closes_the_dialog_and_says_why() { assert_eq!(draft_text(&h).as_deref(), Some("Picked note"), "the draft is kept"); assert_eq!(selection(&h).0, 2, "the selection stays where it is"); let wb = &h.state().session.active().unwrap().wb; - for sheet in 0..3 { - assert!(wb.sheets[sheet].value(CellRef::parse("H1").unwrap()).is_empty(), "nothing ran on sheet {sheet}"); + for (index, sheet) in wb.sheets[..3].iter().enumerate() { + assert!(sheet.value(CellRef::parse("H1").unwrap()).is_empty(), "nothing ran on sheet {index}"); } }