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}"); + } + } +}