From 9354fdb0c4d155b1124facda01142564b24d7887 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sat, 7 Nov 2026 17:37:20 +0100 Subject: [PATCH] Chart: fit labels with a line break by a linear scan The width of a prefix plus an ellipsis is not monotone when the text has a line break, so the binary search could drop a label that fits. Also pass the series index to plot_kind in the data table (upstream/main does not compile without it). Co-Authored-By: Claude Sonnet 5.5 --- crates/chart/src/render.rs | 6 ++++ crates/chart/src/render/table.rs | 2 +- crates/chart/src/render_perf_tests.rs | 41 +++++++++++++++++++++++++++ crates/ui-egui/src/chartview.rs | 27 ++++++++++++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) diff --git a/crates/chart/src/render.rs b/crates/chart/src/render.rs index 1175ffc19..65b6d4f1a 100644 --- a/crates/chart/src/render.rs +++ b/crates/chart/src/render.rs @@ -71,6 +71,8 @@ impl Ctx<'_> { /// shrink as the prefix grows, which holds for per-character advance sums (`ApproxMeasure`, the /// PDF metrics) and for the UI's text layout up to kerning of a few hundredths of a point; with /// such a non-monotone measure the result may differ from the old linear scan by a character. + /// A label with a line break is not monotone (the ellipsis moves to a new line and the widest + /// line can get shorter), so it takes the linear scan, longest prefix first. fn fit(&self, s: &str, size: f32, maxw: f32) -> String { if self.tw(s, size) <= maxw { return s.to_string(); @@ -86,6 +88,10 @@ impl Ctx<'_> { t.push('…'); self.tw(&t, size) <= maxw }; + if s.contains('\n') { + let k = (1..=top).rev().find(|&k| fits(k)); + return k.map_or_else(String::new, |k| format!("{}…", s.get(..starts.get(k).copied().unwrap_or(s.len())).unwrap_or(""))); + } let (mut lo, mut hi) = (0usize, top); while lo < hi { let mid = lo + (hi - lo).div_ceil(2); diff --git a/crates/chart/src/render/table.rs b/crates/chart/src/render/table.rs index f0839dcfe..4f641afd2 100644 --- a/crates/chart/src/render/table.rs +++ b/crates/chart/src/render/table.rs @@ -70,7 +70,7 @@ pub(super) fn draw(ctx: &mut Ctx, chart: &Chart, data: &ChartData, plot: Bx, lay let y = top + (layout.headers as f32 + i as f32 + 0.5) * layout.row_height; let name_x = if settings.show_keys { x0 + 25.0 } else { x0 + 4.0 }; if settings.show_keys { - let swatch = match plot_kind(chart, series) { + let swatch = match plot_kind(chart, series, i) { ChartKind::Line | ChartKind::LineStacked => Swatch::Line, ChartKind::LineMarkers => Swatch::LineMarker, _ => Swatch::Box, diff --git a/crates/chart/src/render_perf_tests.rs b/crates/chart/src/render_perf_tests.rs index ce15e7861..9d2d7177e 100644 --- a/crates/chart/src/render_perf_tests.rs +++ b/crates/chart/src/render_perf_tests.rs @@ -60,6 +60,30 @@ impl Measure for Counting<'_> { } } +/// Like a text layout that breaks at `\n`: the width is that of the widest line. Adding a +/// newline can make the text narrower, so the width of `prefix + "…"` is not monotone. +struct WidestLine; + +impl Measure for WidestLine { + fn text_width(&self, text: &str, size: f32, _bold: bool) -> f32 { + text.split('\n') + .map(|line| { + line.chars() + .map(|c| { + size * if c == 'i' { + 0.25 + } else if c == 'M' { + 0.9 + } else { + 0.5 + } + }) + .sum::() + }) + .fold(0.0, f32::max) + } +} + fn corpus() -> Vec { let mut v: Vec = ["", "a", "ab", "Hello, world", "AVAVAV tight", "日本語のラベルです", "👨‍👩‍👧 family 🎉 party", "e\u{301}e\u{301}e\u{301} combining", "…", "……"] @@ -71,6 +95,9 @@ fn corpus() -> Vec { v.push("x".repeat(401)); v.push("é🎉".repeat(500)); v.push("long label ".repeat(100)); + for s in ["iiii\nMM", "iiii\n", "\niiii", "ab\ncd\nef", "iiiiiiii\niiiiMMMM\nMM", "line one\nline two is longer\nx"] { + v.push(s.to_string()); + } v } @@ -89,6 +116,20 @@ fn run_fit(m: &dyn Measure) { fn fit_matches_linear_reference() { run_fit(&ApproxMeasure); run_fit(&Varied { calls: Cell::new(0) }); + run_fit(&WidestLine); +} + +#[test] +fn a_multiline_label_keeps_its_fitting_prefix() { + // The whole label is 18 wide and "iii…" is 12.5, both too wide for 10.5, but "iiii\n…" is + // 10 wide and fits. A binary search that stops at the failing midpoint would return nothing. + let ctx = Ctx { out: vec![], m: &WidestLine }; + let s = "iiii\nMM"; + let size = 10.0; + let maxw = 10.5; + assert!(ctx.tw(s, size) > maxw); + assert!(ctx.tw("iii…", size) > maxw && ctx.tw("iiii\n…", size) <= maxw); + assert_eq!(ctx.fit(s, size, maxw), "iiii\n…"); } #[test] diff --git a/crates/ui-egui/src/chartview.rs b/crates/ui-egui/src/chartview.rs index f1a366d9e..dbfba039c 100644 --- a/crates/ui-egui/src/chartview.rs +++ b/crates/ui-egui/src/chartview.rs @@ -581,3 +581,30 @@ pub fn hit(app: &SheetApp, geo: &Geo, sh: &Sheet, p: Pos2) -> Option<(ObjRef, Re pub(crate) fn ink_color(line: Color32, width_pt: f32) -> Color32 { if width_pt > 8.0 { line.gamma_multiply(0.45) } else { line } } + +#[cfg(test)] +mod tests { + use gridcraft_chart::Measure; + + use super::*; + + /// The production measure is not monotone for labels with a line break: egui lays the text out + /// line by line, so ending a prefix with `\n…` can be narrower than the shorter `iii…`. The + /// chart's label fitting must not assume otherwise (#1152). + #[test] + fn egui_measure_shrinks_when_a_prefix_ends_in_a_line_break() { + let ctx = egui::Context::default(); + ctx.set_fonts(theme::pinned_font_definitions()); + let mut checked = false; + // Fonts are ready from the second pass on. + for _ in 0..2 { + let mut out = ctx.run_ui(Default::default(), |ui| { + let m = EguiMeasure { painter: ui.painter(), z: 1.0 }; + let (short, long) = (m.text_width("iii…", 11.0, false), m.text_width("iiii\n…", 11.0, false)); + checked = long < short; + }); + out.textures_delta.clear(); + } + assert!(checked); + } +}