From f92b4d30cb2cd74d83ca1000da1ba9566787e14e Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 18:01:11 +0200 Subject: [PATCH 1/2] AutoFit Column Width measures text in the font the grid draws it with (fixes #1316) --- crates/engine/src/cmd/format.rs | 11 +++++++---- crates/engine/src/lib.rs | 7 +++++++ crates/ui-egui/src/lib.rs | 4 ++++ crates/ui-egui/src/theme.rs | 11 +++++++++++ crates/ui-egui/tests/header_autofit.rs | 27 ++++++++++++++++++++++++++ 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index 4712351ca..c7e970fa5 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -1008,7 +1008,7 @@ pub fn chars_to_points(c: f64) -> f64 { pub use gridcraft_model::points_to_chars; /// Approximate text width in points for autofit (no font metrics in the engine: average glyph -/// widths for the default font; the UI refines with real measurement when it autofits). +/// widths for the default font; a front end with fonts measures instead, see `Session::text_width`). pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { let mut w = 0.0; for c in text.chars() { @@ -1027,6 +1027,7 @@ pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { fn autofit_cols(s: &mut Session, p: &Json) -> Result { let lines = line_targets(s, p, false)?; + let measure = s.text_width.clone(); edit(s, |cx| { let si = cx.sheet_index(); let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; @@ -1044,9 +1045,11 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { let st = cx.wb.styles.get(sh.style_id(cell)); let text = crate::display::cell_text(&cx.wb, sh, cell); for line in text.lines() { - max = max.max( - approx_text_width(line, st.font.size, st.font.bold) + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell), - ); + let w = match &measure { + Some(m) => m(line, &st.font), + None => approx_text_width(line, st.font.size, st.font.bold), + }; + max = max.max(w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell)); } } if max > 0.0 { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 258a1cb72..daa9875e5 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -227,6 +227,10 @@ pub enum UiRequest { OpenUrl(String), } +/// Width in points of one line of text drawn in a cell font, measured by a front end that has +/// the font the grid draws with. +pub type TextWidth = Arc f32 + Send + Sync>; + #[derive(Default)] pub struct Session { docs: Vec, @@ -256,6 +260,9 @@ pub struct Session { /// Review › Notes › Show All Notes: every note is displayed. An application display setting /// (like Excel's comment indicator option), so it's not saved in the file. pub show_all_notes: bool, + /// Measures cell text for AutoFit Column Width. Without it (CLI, MCP) AutoFit uses + /// [`cmd::format::approx_text_width`]. + pub text_width: Option, } impl Session { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f96cdac3c..c9616bc0e 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -990,6 +990,10 @@ impl SheetApp { /// Per-frame logic (control channel, screenshots). Call before `ui`. pub fn logic(&mut self, ctx: &egui::Context) { + if self.session.text_width.is_none() { + let ctx = ctx.clone(); + self.session.text_width = Some(std::sync::Arc::new(move |text, font| theme::cell_text_width(&ctx, text, font))); + } // A keytip control request lives one frame: the ribbon consumes it during `ui`, and // anything that never reached a control (collapsed ribbon, tab switch) is dropped. self.ui.keytip_invoke = None; diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 485b9a2f3..23c7fd35e 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -192,6 +192,17 @@ pub fn cell_family(name: &str, bold: bool, italic: bool) -> FontFamily { ) } +/// Width in points, at 100% zoom, of one line of cell text drawn as the grid draws it: in the +/// face [`cell_family`] picks, which on a machine without the named font is a wider or narrower +/// substitute (#1316). A family the active font set doesn't have yet (bound fonts apply from the +/// next frame) falls back to the engine's estimate rather than letting egui panic on it. +pub fn cell_text_width(ctx: &egui::Context, text: &str, font: &gridcraft_engine::model::Font) -> f32 { + let family = cell_family(&font.name, font.bold, font.italic); + let id = FontId::new(font.size * 96.0 / 72.0, family); + ctx.fonts_mut(|f| f.families().contains(&id.family).then(|| f.layout_no_wrap(text.to_owned(), id, Color32::PLACEHOLDER).size().x)) + .unwrap_or_else(|| gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold)) +} + /// Whether epaint can open face `index` of this font file and it maps characters to glyphs. /// epaint panics (in `Context::set_fonts`, i.e. on startup) on a face it can't open, so every font /// file we did not ship is checked first with the same parser and the same call epaint makes diff --git a/crates/ui-egui/tests/header_autofit.rs b/crates/ui-egui/tests/header_autofit.rs index 5e123809f..b4f104f4b 100644 --- a/crates/ui-egui/tests/header_autofit.rs +++ b/crates/ui-egui/tests/header_autofit.rs @@ -191,3 +191,30 @@ fn table_filter_autofit_works_through_command_and_header_double_click() { } } } + +#[test] +fn header_double_click_fits_the_text_as_the_grid_draws_it() { + // Issue #1316: `@` and `%` are far wider in every real font than the engine's average-glyph + // estimate, so AutoFit has to measure with the font the grid draws the cell in. + let text = "sarmstrong@example.com %%% @@@"; + let mut h = harness("A1"); + h.state_mut().session.execute("range.setValues", json!({"range":"A1", "values":[[text]]})).unwrap(); + h.run_steps(3); + let (pos, _) = edge(&h, false, 0); + button(&mut h, pos, true); + button(&mut h, pos, false); + h.step(); + button(&mut h, pos, true); + button(&mut h, pos, false); + h.run_steps(2); + + let app = h.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let font = &app.session.active().unwrap().wb.styles.get(sheet.style_id(gridcraft_engine::core::CellRef::new(0, 0))).font; + let id = egui::FontId::new(font.size * 96.0 / 72.0, gridcraft_ui_egui::theme::cell_family(&font.name, font.bold, font.italic)); + let drawn = h.ctx.fonts_mut(|f| f.layout_no_wrap(text.to_string(), id, egui::Color32::BLACK)).size().x; + let estimate = gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold); + assert!(drawn > estimate + 6.0, "fixture: the drawn text ({drawn}) must be wider than the estimate ({estimate}) plus padding"); + // Two 2.5 px paddings and the gridline, as the grid lays out the cell. + assert!(sheet.col_width(0) >= drawn + 6.0, "column {} is narrower than the drawn text {drawn} + padding", sheet.col_width(0)); +} From f36bc100d4b72ca25800e7b55bd8860e1ae1e5df Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Wed, 14 Oct 2026 18:14:09 +0200 Subject: [PATCH 2/2] AutoFit measures each distinct text once, and quickly on large columns (#1316) --- crates/engine/src/cmd/format.rs | 22 ++++++++++++++++-- crates/engine/src/lib.rs | 7 ++++-- crates/engine/tests/autofit_filter_buttons.rs | 23 +++++++++++++++++++ crates/ui-egui/src/lib.rs | 2 +- crates/ui-egui/src/theme.rs | 19 ++++++++++++--- crates/ui-egui/tests/header_autofit.rs | 23 ++++++++++++++++++- 6 files changed, 87 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index c7e970fa5..585f68dc0 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -1025,6 +1025,10 @@ pub fn approx_text_width(text: &str, size: f32, bold: bool) -> f32 { w * size * if bold { 1.07 } else { 1.0 } * 96.0 / 72.0 } +/// Distinct texts AutoFit measures exactly; past this many, `Session::text_width` is asked for a +/// quick measure so a huge column doesn't lay out every value. +const EXACT_TEXT_WIDTHS: usize = 2_000; + fn autofit_cols(s: &mut Session, p: &Json) -> Result { let lines = line_targets(s, p, false)?; let measure = s.text_width.clone(); @@ -1032,6 +1036,8 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { let si = cx.sheet_index(); let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) }; let mut widths = Vec::new(); + let mut measured: std::collections::HashMap> = Default::default(); + let mut distinct = 0usize; let used = sh.used_range(); for (a, b) in lines { for c in a..=b.min(a + 16383) { @@ -1042,11 +1048,23 @@ fn autofit_cols(s: &mut Session, p: &Json) -> Result { if sh.merge_at(cell).is_some_and(|m| !m.is_single()) { continue; } - let st = cx.wb.styles.get(sh.style_id(cell)); + let id = sh.style_id(cell); + let st = cx.wb.styles.get(id); let text = crate::display::cell_text(&cx.wb, sh, cell); for line in text.lines() { let w = match &measure { - Some(m) => m(line, &st.font), + Some(m) => { + let known = measured.entry(id).or_default(); + match known.get(line) { + Some(w) => *w, + None => { + let w = m(line, &st.font, distinct < EXACT_TEXT_WIDTHS); + known.insert(line.to_owned(), w); + distinct += 1; + w + } + } + } None => approx_text_width(line, st.font.size, st.font.bold), }; max = max.max(w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell)); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index daa9875e5..f52984c4c 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -228,8 +228,11 @@ pub enum UiRequest { } /// Width in points of one line of text drawn in a cell font, measured by a front end that has -/// the font the grid draws with. -pub type TextWidth = Arc f32 + Send + Sync>; +/// the font the grid draws with. With `exact` false the front end may answer with a cheaper +/// measure that is never narrower (AutoFit asks for it on columns with many distinct values). +/// Commands call it while they run, so a front end must not run commands while holding a lock +/// the measure takes (egui's: see `theme::cell_text_width` in the UI crate). +pub type TextWidth = Arc f32 + Send + Sync>; #[derive(Default)] pub struct Session { diff --git a/crates/engine/tests/autofit_filter_buttons.rs b/crates/engine/tests/autofit_filter_buttons.rs index 307bb998d..dbf25eced 100644 --- a/crates/engine/tests/autofit_filter_buttons.rs +++ b/crates/engine/tests/autofit_filter_buttons.rs @@ -100,3 +100,26 @@ fn autofit_filter_width_is_undoable_without_changing_selection() { s.execute("edit.undo", json!({})).unwrap(); assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(0), 80.0); } + +#[test] +fn autofit_measures_each_distinct_text_once_and_quickly_past_the_limit() { + use std::sync::{Arc, Mutex}; + let mut s = Session::new(); + s.new_workbook(); + // 2,500 distinct texts, each in two cells. + let values: Vec<_> = (0..5000).map(|i| json!([format!("v{}", i % 2500)])).collect(); + s.execute("range.setValues", json!({"range":"A1", "values": values})).unwrap(); + let calls = Arc::new(Mutex::new(Vec::new())); + let seen = calls.clone(); + s.text_width = Some(Arc::new(move |text: &str, _: &gridcraft_engine::model::Font, exact: bool| { + seen.lock().unwrap().push((text.to_owned(), exact)); + if text == "v2499" { 300.0 } else { 10.0 } + })); + s.execute("home.autofitColumnWidth", json!({"cols":"A:A"})).unwrap(); + let calls = calls.lock().unwrap(); + assert_eq!(calls.len(), 2500, "one measure per distinct text"); + assert_eq!(calls.iter().filter(|(_, exact)| *exact).count(), 2000); + assert!(calls.iter().skip(2000).all(|(_, exact)| !exact)); + // The quick measures still size the column. + assert_eq!(s.doc().unwrap().wb.active().unwrap().col_width(0), 306.0); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index c9616bc0e..3bb674dce 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -992,7 +992,7 @@ impl SheetApp { pub fn logic(&mut self, ctx: &egui::Context) { if self.session.text_width.is_none() { let ctx = ctx.clone(); - self.session.text_width = Some(std::sync::Arc::new(move |text, font| theme::cell_text_width(&ctx, text, font))); + self.session.text_width = Some(std::sync::Arc::new(move |text, font, exact| theme::cell_text_width(&ctx, text, font, exact))); } // A keytip control request lives one frame: the ribbon consumes it during `ui`, and // anything that never reached a control (collapsed ribbon, tab switch) is dropped. diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index 23c7fd35e..46ac1ea4a 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -196,11 +196,24 @@ pub fn cell_family(name: &str, bold: bool, italic: bool) -> FontFamily { /// face [`cell_family`] picks, which on a machine without the named font is a wider or narrower /// substitute (#1316). A family the active font set doesn't have yet (bound fonts apply from the /// next frame) falls back to the engine's estimate rather than letting egui panic on it. -pub fn cell_text_width(ctx: &egui::Context, text: &str, font: &gridcraft_engine::model::Font) -> f32 { +/// +/// `exact` lays the text out (a galley kept until the end of the frame); otherwise the glyph +/// advances are summed without kerning, which is cheap and never narrower. This takes the +/// Context lock (`fonts_mut`): never call it, or run a command, while an egui lock is held. +pub fn cell_text_width(ctx: &egui::Context, text: &str, font: &gridcraft_engine::model::Font, exact: bool) -> f32 { let family = cell_family(&font.name, font.bold, font.italic); let id = FontId::new(font.size * 96.0 / 72.0, family); - ctx.fonts_mut(|f| f.families().contains(&id.family).then(|| f.layout_no_wrap(text.to_owned(), id, Color32::PLACEHOLDER).size().x)) - .unwrap_or_else(|| gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold)) + ctx.fonts_mut(|f| { + f.families().contains(&id.family).then(|| { + if exact { + f.layout_no_wrap(text.to_owned(), id, Color32::PLACEHOLDER).size().x + } else { + // Half a point covers the galley's rounding of its width up to the pixel grid. + text.chars().map(|c| f.glyph_width(&id, c)).sum::() + 0.5 + } + }) + }) + .unwrap_or_else(|| gridcraft_engine::cmd::format::approx_text_width(text, font.size, font.bold)) } /// Whether epaint can open face `index` of this font file and it maps characters to glyphs. diff --git a/crates/ui-egui/tests/header_autofit.rs b/crates/ui-egui/tests/header_autofit.rs index b4f104f4b..e5cf0cc21 100644 --- a/crates/ui-egui/tests/header_autofit.rs +++ b/crates/ui-egui/tests/header_autofit.rs @@ -199,7 +199,10 @@ fn header_double_click_fits_the_text_as_the_grid_draws_it() { let text = "sarmstrong@example.com %%% @@@"; let mut h = harness("A1"); h.state_mut().session.execute("range.setValues", json!({"range":"A1", "values":[[text]]})).unwrap(); - h.run_steps(3); + // Let the installed-font scan finish and bind its faces first, so the cell's font can't + // change between the AutoFit and the measurement below. + gridcraft_ui_egui::fonts::catalog(); + h.run_steps(6); let (pos, _) = edge(&h, false, 0); button(&mut h, pos, true); button(&mut h, pos, false); @@ -218,3 +221,21 @@ fn header_double_click_fits_the_text_as_the_grid_draws_it() { // Two 2.5 px paddings and the gridline, as the grid lays out the cell. assert!(sheet.col_width(0) >= drawn + 6.0, "column {} is narrower than the drawn text {drawn} + padding", sheet.col_width(0)); } + +#[test] +fn the_quick_measure_is_never_narrower_than_the_drawn_text() { + // Columns with many distinct values are measured by summing glyph advances; that must cover + // the laid-out text, or AutoFit would clip again on large columns. + let h = harness("A1"); + let app = h.state(); + let sheet = app.session.active().unwrap().wb.active().unwrap(); + let mut font = app.session.active().unwrap().wb.styles.get(sheet.style_id(gridcraft_engine::core::CellRef::new(0, 0))).font.clone(); + for bold in [false, true] { + font.bold = bold; + for text in ["sarmstrong@example.com", "Quarterly revenue summary", "WWW MMM Wide Headers", "AVATAR To Ty", "fi ffl 1,234.56"] { + let quick = gridcraft_ui_egui::theme::cell_text_width(&h.ctx, text, &font, false); + let exact = gridcraft_ui_egui::theme::cell_text_width(&h.ctx, text, &font, true); + assert!(exact > 0.0 && quick >= exact, "{text:?} bold={bold}: quick {quick} < drawn {exact}"); + } + } +}