From 691c33016a415128504ac91f9c696648bacf25c4 Mon Sep 17 00:00:00 2001 From: JDonaghy Date: Tue, 1 Sep 2026 18:35:08 -0500 Subject: [PATCH 1/2] fix(#728): VS Code-parity minimap width, stop rebuilding the buffer per frame, reconcile status-row predicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four defects filed against the minimap in #728 (the fourth, MinimapSizing::FixedPitch, is deferred — see below): 1. Width: `minimap_reserved_width`'s `want` was `rect_width * MINIMAP_WIDTH_FRACTION` alone, letting an ordinary wide GTK pane reach the 240px ceiling — roughly twice VS Code's own ~120px strip. `want` is now `min(MINIMAP_TARGET_COLS, rect_width * MINIMAP_WIDTH_FRACTION)` (120 = VS Code's `minimap.maxColumn`), so the strip settles at a fixed VS Code-parity width instead of scaling up with the pane; the fraction still shrinks it for a pane too narrow to afford the full target. 2. Performance: `build_minimap_data` materialised a `String` for every line in the buffer on every frame regardless of how many it actually samples. It now computes the sampled *indices* first (`minimap_sample_indices`, mirroring `quadraui::sample_lines`'s own stride formula) and fetches only those ~`target_lines` lines from the rope. Measured on a 10,000-line file, 300 simulated scroll frames: 24.23s (~80.8ms/frame) before, 0.40s (~1.34ms/frame) after — ~60x, and no longer scales with buffer size. Also capped `to_col`'s per-span char-count scan at `MINIMAP_COL_SCAN_LIMIT`, since `aggregate_spans` never looks past `MINIMAP_SPAN_COLS` cells and the old scan was unbounded on a long (e.g. minified) line. 3. Status-row overdraw: `build_screen_layout` and GTK's `h_scrollbar_geometry` answered "is a per-window status row painted here" with two different, independently-wrong predicates — one accounted for `separate_status`, the other for `terminal_maximized`, neither for both. Both now go through one shared `render::window_status_row_reserved`. Not in this diff: item 4 (quadraui's `MinimapSizing::FixedPitch`, quadraui#667) is already present at the currently pinned rev, so no pin bump is needed — but switching GTK off `display_rows * MINIMAP_LINES_PER_ROW` and onto the fixed-pitch row model is a separate behavior change to the sampling target that deserves its own pass and test suite; leaving it for a follow-up rather than bundling it here. Tests: render.rs formula/predicate unit tests (including a RED-verified regression for the status-row divergence), a timed regression guard for the O(buffer) fix, gtk::testing GtkDriver black-box tests (strip width under a wide pane, strip/status-row non-overlap), and a private-fn geometry test for h_scrollbar_geometry's status offset (same pattern as the existing native_scrollbar_placement_tests). Co-Authored-By: Claude Sonnet 5 --- src/gtk/mod.rs | 121 ++++++++++++- src/gtk/testing.rs | 83 +++++++++ src/render.rs | 435 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 601 insertions(+), 38 deletions(-) diff --git a/src/gtk/mod.rs b/src/gtk/mod.rs index b6eb017b..1414c4a9 100644 --- a/src/gtk/mod.rs +++ b/src/gtk/mod.rs @@ -10439,8 +10439,15 @@ fn h_scrollbar_geometry( // line_height` and paints after the scrollbars, so anchor the // h-scrollbar above it when the status line is on. Otherwise the // status bar overdraws the entire scrollbar (it's `line_height` - // tall vs the scrollbar's ~5px). - let status_offset = if engine.settings.window_status_line && !engine.terminal_maximized { + // tall vs the scrollbar's ~5px). `render::window_status_row_reserved` + // is the single source of truth for whether that row is actually + // painted (#728) — this used to check `window_status_line && + // !terminal_maximized` directly, which (unlike the shared helper) + // never accounted for `status_line_above_terminal`/bottom-panel state + // pulling the status line out into a separated bar instead, and so + // could disagree with `build_screen_layout` about whether this row is + // free. + let status_offset = if render::window_status_row_reserved(engine) { line_height } else { 0.0 @@ -10771,6 +10778,116 @@ mod native_scrollbar_placement_tests { } } +#[cfg(test)] +mod h_scrollbar_status_offset_tests { + //! #728: `h_scrollbar_geometry`'s status-row offset used to check + //! `window_status_line && !terminal_maximized` directly, while + //! `render::build_screen_layout`'s reservation of that same row used + //! `per_window_status && !separate_status` — two independent answers to + //! "is a per-window status row painted here", each covering an axis the + //! other didn't (`terminal_maximized` vs. `separate_status`). Both now + //! go through `render::window_status_row_reserved`; these pin that the + //! scrollbar's track actually moves in lockstep with it rather than + //! re-diverging. + use super::h_scrollbar_geometry; + use crate::core::{Engine, WindowRect}; + + /// A window whose longest line overflows a narrow viewport, so + /// `h_scrollbar_geometry` returns `Some` rather than `None` ("content + /// fits" — nothing to offset). + fn engine_needing_h_scrollbar() -> Engine { + let mut e = Engine::new_for_test(); + e.buffer_mut().insert(0, &"x".repeat(500)); + // `max_col` (what `h_scrollbar_geometry` reads) is a cache + // refreshed by `update_syntax`, not by a raw `Buffer::insert` — + // force it so the 500-char line above is actually reflected. + let wid = e.active_window_id(); + let buffer_id = e.windows.get(&wid).unwrap().buffer_id; + e.buffer_manager.get_mut(buffer_id).unwrap().update_syntax(); + e + } + + #[test] + fn track_moves_up_by_exactly_one_row_when_the_status_row_is_reserved() { + let mut e = engine_needing_h_scrollbar(); + e.settings.window_status_line = true; + let wid = e.active_window_id(); + let rect = WindowRect::new(0.0, 0.0, 100.0, 40.0); + let line_height = 20.0; + + let (_, track_y_with, ..) = h_scrollbar_geometry(&e, wid, &rect, 8.0, line_height) + .expect("an overflowing line needs an h-scrollbar"); + + e.settings.window_status_line = false; + let (_, track_y_without, ..) = h_scrollbar_geometry(&e, wid, &rect, 8.0, line_height) + .expect("still overflowing with the status line off"); + + assert_eq!( + track_y_without - track_y_with, + line_height, + "the status row must shift the h-scrollbar up by exactly one line_height" + ); + } + + /// #728 regression: with `status_line_above_terminal` OFF and the + /// bottom panel open, the active window's status is pulled into a + /// *separated* bar above the terminal instead of painting inside this + /// window — `render::window_status_row_reserved` reports the row as + /// free, and the h-scrollbar must agree. The old + /// `window_status_line && !terminal_maximized` predicate never checked + /// this axis and would have offset for a row nothing paints here. + /// RED against that predicate (verified while writing this fix): 13.0 + /// vs. 33.0 — the old code offset the track by a full `line_height` for + /// a status row that was actually painted as a separated bar elsewhere. + #[test] + fn track_does_not_move_when_status_is_separated_above_the_terminal() { + let mut e = engine_needing_h_scrollbar(); + e.settings.window_status_line = true; + e.settings.status_line_above_terminal = false; + e.terminal_open = true; + let wid = e.active_window_id(); + let rect = WindowRect::new(0.0, 0.0, 100.0, 40.0); + let line_height = 20.0; + + let (_, track_y_separated, ..) = h_scrollbar_geometry(&e, wid, &rect, 8.0, line_height) + .expect("an overflowing line needs an h-scrollbar"); + + e.settings.window_status_line = false; + let (_, track_y_no_status, ..) = h_scrollbar_geometry(&e, wid, &rect, 8.0, line_height) + .expect("still overflowing with the status line off"); + + assert_eq!( + track_y_separated, track_y_no_status, + "a separated status bar must not offset the h-scrollbar — this \ + window's own bottom row is free" + ); + } + + /// #728 regression: while the terminal panel is maximized, editor + /// windows are not the visible surface, so nothing paints a per-window + /// status row even with the setting on — the h-scrollbar must not + /// offset for one. This is the axis `build_screen_layout`'s old + /// predicate never checked (only GTK's did). + #[test] + fn track_does_not_move_when_the_terminal_is_maximized() { + let mut e = engine_needing_h_scrollbar(); + e.settings.window_status_line = true; + e.terminal_maximized = true; + let wid = e.active_window_id(); + let rect = WindowRect::new(0.0, 0.0, 100.0, 40.0); + let line_height = 20.0; + + let (_, track_y_maximized, ..) = h_scrollbar_geometry(&e, wid, &rect, 8.0, line_height) + .expect("an overflowing line needs an h-scrollbar"); + + e.settings.window_status_line = false; + let (_, track_y_no_status, ..) = h_scrollbar_geometry(&e, wid, &rect, 8.0, line_height) + .expect("still overflowing with the status line off"); + + assert_eq!(track_y_maximized, track_y_no_status); + } +} + #[cfg(test)] mod shell_config_identity_tests { //! #719: quadraui#656/#657 landed `ShellConfig::with_app_id()` / diff --git a/src/gtk/testing.rs b/src/gtk/testing.rs index cf03f185..343ddc69 100644 --- a/src/gtk/testing.rs +++ b/src/gtk/testing.rs @@ -4050,6 +4050,89 @@ mod minimap { ); } + /// #728 acceptance: on an ordinary wide pane the minimap strip settles + /// at VS Code's own ~120px width instead of scaling up with the pane — + /// the pre-fix `rect_width * MINIMAP_WIDTH_FRACTION` formula reached + /// ~240px on a pane this wide, roughly twice VS Code's. Driven through + /// the real paint path (`ScreenLayout` from an actual `window_center` + /// call), not just `minimap_reserved_width` in isolation. + #[test] + fn minimap_strip_settles_at_vs_code_parity_width_on_a_wide_pane() { + let h = harness(engine_with_shaped_buffer(), 1600, 900); + let win = h.engine.borrow().active_window_id(); + h.window_center(win) + .expect("editor pane must paint with the default settings"); + + let (strip_width, pane_width) = { + let layout = h.screen_layout.borrow(); + let l = layout.as_ref().unwrap(); + let mm = l + .minimap + .iter() + .find(|m| m.window_id == win) + .expect("the layout must carry a minimap for the pane"); + let rw = l.windows.iter().find(|w| w.window_id == win).unwrap(); + (mm.rect.width, rw.rect.width + mm.rect.width) + }; + let char_width = h.painted_char_width(); + let expected = + crate::render::minimap_reserved_width(&h.engine.borrow(), pane_width, char_width); + + assert_eq!( + strip_width, expected, + "the real paint path must reserve exactly what \ + minimap_reserved_width computes" + ); + assert!( + strip_width < 150.0, + "a 1600px pane must not blow past VS Code's ~120px minimap \ + width (got {strip_width}px — the pre-#728 formula would have \ + hit ~240px here)" + ); + } + + /// #728 acceptance: the minimap strip must never extend into the + /// per-window status row painted at the bottom of the same window. + /// `build_screen_layout` reserves `status_h` off the bottom of the + /// strip's own rect via `render::window_status_row_reserved` — the same + /// predicate GTK's h-scrollbar geometry now shares (previously it used + /// a diverging predicate; see `gtk::h_scrollbar_status_offset_tests`). + #[test] + fn minimap_strip_never_overlaps_the_per_window_status_row() { + let mut engine = engine_with_shaped_buffer(); + engine.settings.window_status_line = true; + let h = harness(engine, 1400, 900); + let win = h.engine.borrow().active_window_id(); + h.window_center(win) + .expect("editor pane must paint with the status line on"); + + let lh = h + .painted_line_height() + .expect("frame must publish the line height it painted with"); + let layout = h.screen_layout.borrow(); + let l = layout.as_ref().unwrap(); + let mm = l + .minimap + .iter() + .find(|m| m.window_id == win) + .expect("the layout must carry a minimap for the pane"); + let rw = l.windows.iter().find(|w| w.window_id == win).unwrap(); + assert!( + rw.status_line.is_some(), + "test setup sanity: the per-window status line must actually \ + be painted, or this test isn't exercising the overlap risk \ + at all" + ); + + let status_row_top = rw.rect.y + rw.rect.height - lh; + let strip_bottom = mm.rect.y + mm.rect.height; + assert!( + strip_bottom <= status_row_top + 0.01, + "the minimap strip (bottom={strip_bottom}) must not extend \ + into the per-window status row (top={status_row_top})" + ); + } + /// Acceptance (#35): a click at the vertical middle of the strip scrolls /// the pane to ~50% of the file — the GTK half of the cross-backend /// claim, driven through the real `pixel_to_click_target` path. diff --git a/src/render.rs b/src/render.rs index a13c0131..f44a977e 100644 --- a/src/render.rs +++ b/src/render.rs @@ -4350,17 +4350,32 @@ pub struct ScreenLayout { // ─── Minimap (#35) ──────────────────────────────────────────────────────────── -/// Fraction of a pane's own width the minimap strip reserves (#722). +/// Fraction of a pane's own width the minimap strip may reserve, as an +/// *upper bound* only (#728). /// -/// VS Code keeps its minimap at a roughly constant fraction of the editor -/// regardless of window width or editor font size — e.g. its ~120px strip -/// over a ~800px editor pane at default settings is ~15%. This is the -/// primary driver of `minimap_reserved_width`'s `want`; `MINIMAP_MIN_COLS`/ -/// `MINIMAP_MAX_COLS` below only bound the extremes, so widening a pane -/// grows the strip and resizing the editor font does not (both pinned by -/// `render.rs`'s minimap test module). +/// #722 made this fraction the primary driver of `minimap_reserved_width`'s +/// `want`, which is what let the strip grow to ~240px (`MINIMAP_MAX_PX`) in +/// an ordinary wide pane — roughly twice VS Code's own strip. VS Code does +/// not scale its minimap with window width at all: it derives a *fixed* +/// width from `minimap.maxColumn` (120, one pixel per assumed column — see +/// `MINIMAP_TARGET_COLS`). #728 makes that fixed target the primary driver +/// instead, and demotes this fraction to a cap that only matters for a pane +/// too narrow to afford the full 120px/cols (so the strip still shrinks +/// smoothly with the pane rather than snapping straight from 120 to +/// suppressed at `MINIMAP_MIN_TEXT_COLS`). pub const MINIMAP_WIDTH_FRACTION: f64 = 0.15; +/// Target minimap width, in the caller's own unit (px for GTK, columns for +/// TUI) — VS Code's `minimap.maxColumn` default (#728). VS Code renders its +/// minimap at one pixel per assumed source column, so at 120px this is +/// "precisely 1px per column", matching quadraui's own colour-bar heuristic +/// (`aggregate_spans`'s `MINIMAP_SPAN_COLS` window), and TUI's column-native +/// units make the same constant trivially the column count directly. +/// `minimap_reserved_width` takes `min(MINIMAP_TARGET_COLS, pane-fraction +/// cap)`, so this is a ceiling a wide pane settles at, not something a wider +/// pane keeps growing past (unlike the old fraction-only formula). +const MINIMAP_TARGET_COLS: f64 = 120.0; + /// Floor on the reserved width for TUI, in cell columns directly. TUI is /// cell-native and always passes `char_width == 1.0` into /// [`minimap_reserved_width`], so multiplying by it is a no-op and this @@ -4394,6 +4409,13 @@ const MINIMAP_MIN_PX: f64 = 48.0; /// Ceiling on the reserved width for GTK, same units/rationale as /// `MINIMAP_MIN_PX` (30 cols * 8px at the same representative font). +/// +/// #728: since `MINIMAP_TARGET_COLS` (120) is now `want`'s primary driver +/// and is itself below this ceiling, `want` no longer reaches 240px in +/// practice — this remains only as a defensive bound (e.g. if +/// `MINIMAP_TARGET_COLS` is ever raised above it) rather than the everyday +/// cap it used to be. The strip's everyday ceiling is `MINIMAP_TARGET_COLS` +/// itself. const MINIMAP_MAX_PX: f64 = 240.0; /// Text columns that must survive after reserving the strip. Below this the @@ -4420,6 +4442,40 @@ const MINIMAP_COLS_PER_CELL: usize = 2; /// influence any painted cell, so aggregating it would be wasted work. const MINIMAP_SPAN_COLS: usize = 200; +/// Character-count ceiling for `build_minimap_data`'s `to_col` closure +/// (#728). `aggregate_spans` never looks past `MINIMAP_SPAN_COLS` cells of +/// `MINIMAP_COLS_PER_CELL` raw columns each, so a byte offset past that many +/// characters always maps to a column `aggregate_spans` discards anyway — +/// scanning further just to report an exact (and irrelevant) larger number +/// is wasted, unbounded work on a long line. The `+ 1` keeps the boundary +/// value itself exact rather than off-by-one short. +const MINIMAP_COL_SCAN_LIMIT: usize = MINIMAP_SPAN_COLS * MINIMAP_COLS_PER_CELL + 1; + +/// Buffer line numbers `build_minimap_data` will actually sample, computed +/// **before** any line text is fetched (#728). +/// +/// Mirrors `quadraui::sample_lines`'s own stride formula exactly (never +/// upscales — keep every line when `total_lines <= target_lines` — otherwise +/// stride every `total_lines / target_lines` lines), so handing exactly +/// these `total_lines.min(target_lines)`-many candidates back into +/// `sample_lines` with `target_rows` set to that same count always takes its +/// cheap "keep every candidate, in order" path. That lets the caller fetch +/// only these lines' text from the rope instead of materialising a `String` +/// for every line in the buffer, while still going through `sample_lines` +/// for the actual `MinimapLine` construction. +fn minimap_sample_indices(total_lines: usize, target_lines: usize) -> Vec { + if total_lines == 0 || target_lines == 0 { + return Vec::new(); + } + if total_lines <= target_lines { + return (0..total_lines).collect(); + } + let stride = total_lines as f64 / target_lines as f64; + (0..target_lines) + .map(|r| ((r as f64 * stride) as usize).min(total_lines - 1)) + .collect() +} + /// The active window's minimap: a quadraui `Minimap` primitive plus the strip /// it occupies. Both backends consume this verbatim — `rect` goes straight to /// `draw_minimap`, and the returned `MinimapLayout` answers clicks. @@ -4435,11 +4491,16 @@ pub struct RenderedMinimap { /// Width the minimap reserves alongside the editor, in the caller's units. /// -/// `want` is `rect_width * MINIMAP_WIDTH_FRACTION`, clamped to a floor/ -/// ceiling — a proportion of *this pane's* width, not a fixed column count -/// multiplied by the editor's font metrics (#722). That makes the strip -/// grow with the pane and hold steady across a font-size change, matching -/// VS Code. +/// #728: `want` is `min(MINIMAP_TARGET_COLS, rect_width * +/// MINIMAP_WIDTH_FRACTION)`, clamped to a floor/ceiling. The fixed target is +/// VS Code parity — its minimap does not grow with the window — and the +/// fraction only caps `want` down for a pane too narrow to afford the full +/// target, still as a proportion of *this pane's* width rather than a fixed +/// column count multiplied by the editor's font metrics (#722). That keeps +/// the strip narrowing smoothly in a narrow/split pane while holding steady +/// at the VS Code-sized target in an ordinary or wide one, and — since +/// neither term depends on `char_width` — still holds steady across a +/// font-size change. /// /// The clamp bounds themselves must be font-invariant too, or the *clamped* /// result reintroduces #722's defect B in exactly the narrow-pane regime a @@ -4462,7 +4523,9 @@ pub fn minimap_reserved_width(engine: &Engine, rect_width: f64, char_width: f64) } else { (MINIMAP_MIN_COLS * cw, MINIMAP_MAX_COLS * cw) }; - let want = (rect_width * MINIMAP_WIDTH_FRACTION).clamp(min_w, max_w); + let want = MINIMAP_TARGET_COLS + .min(rect_width * MINIMAP_WIDTH_FRACTION) + .clamp(min_w, max_w); let has = engine.settings.minimap && rect_width >= want + MINIMAP_MIN_TEXT_COLS * cw; quadraui::reserved_width(want as f32, has) as f64 } @@ -4500,10 +4563,22 @@ pub fn build_minimap_data( return None; } - // Whole-file text, trimmed of line endings — `sample_lines` picks the - // stride, we only supply the candidates. - let owned: Vec = (0..total_buffer_lines) - .map(|i| { + // #728: pick *which* buffer lines to sample before fetching any line + // text — mirrors `quadraui::sample_lines`'s own stride formula (never + // upscales; otherwise strides every `total / target` lines) so only the + // ~`target_lines` candidates that will actually survive get fetched + // from the rope, instead of materialising a `String` for every line in + // the buffer on every frame (the fix this pins in + // `minimap_scroll_does_not_scale_with_buffer_size`). `sample_lines` is + // still the function that turns text into `MinimapLine`s below — this + // only decides which lines are worth reading in the first place. + let sample_indices = minimap_sample_indices(total_buffer_lines, target_lines); + if sample_indices.is_empty() { + return None; + } + let owned: Vec = sample_indices + .iter() + .map(|&i| { rope.line(i) .as_str() .map(|s| s.trim_end_matches(['\n', '\r']).to_string()) @@ -4516,7 +4591,17 @@ pub fn build_minimap_data( }) .collect(); let borrowed: Vec<&str> = owned.iter().map(String::as_str).collect(); - let lines = quadraui::sample_lines(&borrowed, target_lines); + // `sample_indices.len()` candidates against a `target_rows` of exactly + // that count always takes `sample_lines`'s "never upscales, keep every + // candidate" branch, so `line_idx` below is just each candidate's + // position in `borrowed`/`owned` — remapped to the real buffer line + // number via `sample_indices` right after, since `sample_lines` only + // knows positions within the slice it was given, not buffer line + // numbers. + let mut lines = quadraui::sample_lines(&borrowed, borrowed.len()); + for (line, &real_idx) in lines.iter_mut().zip(sample_indices.iter()) { + line.line_idx = real_idx; + } if lines.is_empty() { return None; } @@ -4539,13 +4624,30 @@ pub fn build_minimap_data( continue; }; let line_start = rope.line_to_byte(buf_line); - let line_str = &owned[buf_line]; + // `owned`/`borrowed`/`lines` are all indexed by *sampled* position + // (`idx`), not by real buffer line number (`buf_line`) — `owned` no + // longer has one entry per buffer line since #728 stopped + // materialising the whole buffer, so `sampled_at`'s value (the + // sampled index) is what indexes it now. + let line_str = &owned[idx]; // quadraui's rasterisers treat span columns as *character* columns // (GTK converts them back to byte offsets for Pango attributes), so - // convert here rather than handing over raw byte deltas. + // convert here rather than handing over raw byte deltas. Capped at + // `MINIMAP_COL_SCAN_LIMIT` chars: columns past + // `MINIMAP_SPAN_COLS` * `MINIMAP_COLS_PER_CELL` never affect + // `aggregate_spans`'s output (it drops any cell at/past + // `grid.cols`), so counting further into a long — e.g. minified — + // line is wasted, and unbounded: a span's byte offset can land + // arbitrarily far into it. Without the cap this was an O(line + // length) rescan run up to twice per highlight span on that line + // (#728). let to_col = |b: usize| -> usize { let b = b.min(line_str.len()); - line_str.get(..b).map(|p| p.chars().count()).unwrap_or(b) + line_str + .char_indices() + .take(MINIMAP_COL_SCAN_LIMIT) + .take_while(|&(byte_idx, _)| byte_idx < b) + .count() }; let start_col = to_col(start.saturating_sub(line_start)); let end_col = to_col(end.saturating_sub(line_start)); @@ -6804,6 +6906,41 @@ impl Theme { } } +/// Whether a per-window status line is reserved (and painted) at the bottom +/// of each editor window's own rect, given the engine's current state. +/// +/// Single source of truth for "does this window reserve its bottom row for +/// its own status line" (#728). Before this, the question was answered +/// independently — and inconsistently — in two places: +/// - `build_screen_layout_with_breadcrumb_row` used `per_window_status && +/// !separate_status`, correctly handling `status_line_above_terminal` +/// being OFF with the bottom panel open (which pulls the active +/// window's status into a *separated* bar above the terminal instead, +/// freeing that window's own bottom row) but never checking +/// `terminal_maximized`. +/// - GTK's `h_scrollbar_geometry` used `window_status_line && +/// !terminal_maximized` (to avoid offsetting the horizontal scrollbar +/// for a status row that isn't painted while the terminal panel covers +/// the editor windows entirely), but never checked `separate_status`. +/// +/// Each covered an axis the other didn't, so either one alone could +/// disagree with what actually gets painted. Both call sites now go through +/// this one function instead. +pub fn window_status_row_reserved(engine: &Engine) -> bool { + // While the terminal panel is maximized, editor windows are not the + // visible surface at all (`breadcrumb_draw_targets` suppresses every + // breadcrumb the same way), so nothing paints a per-window status row + // regardless of the setting. + if engine.terminal_maximized { + return false; + } + let per_window_status = engine.settings.window_status_line; + let bottom_panel_open = engine.terminal_open || engine.bottom_panel_open; + let separate_status = + per_window_status && !engine.settings.status_line_above_terminal && bottom_panel_open; + per_window_status && !separate_status +} + // ─── build_screen_layout ────────────────────────────────────────────────────── /// Build a complete `ScreenLayout` from current engine state. @@ -6875,6 +7012,9 @@ pub fn build_screen_layout_with_breadcrumb_row( // window — they're naturally above the terminal by being part of the editor area. let separate_status = per_window_status && !engine.settings.status_line_above_terminal && bottom_panel_open; + // Single source of truth for "does this window paint its own bottom-row + // status line" (#728) — also consulted by GTK's `h_scrollbar_geometry`. + let own_status_row = window_status_row_reserved(engine); // Window-split dividers (#582) — independent of the `n >= 2` editor-group // check below, since `:split`/`:vsplit` panes exist within a single group. @@ -6896,7 +7036,7 @@ pub fn build_screen_layout_with_breadcrumb_row( .iter() .map(|(window_id, rect)| { let mut visible_lines = (rect.height / line_height).floor() as usize; - if per_window_status && !separate_status && visible_lines > 1 { + if own_status_row && visible_lines > 1 { visible_lines -= 1; // reserve bottom row for per-window status bar } let is_active = *window_id == active_window_id; @@ -6919,7 +7059,7 @@ pub fn build_screen_layout_with_breadcrumb_row( multi_window, color_headings, ); - if per_window_status && !separate_status { + if own_status_row { rw.status_line = Some(build_window_status_line( engine, theme, *window_id, is_active, )); @@ -6944,7 +7084,7 @@ pub fn build_screen_layout_with_breadcrumb_row( if minimap_w <= 0.0 { return None; } - let status_h = if per_window_status && !separate_status && r.height > line_height { + let status_h = if own_status_row && r.height > line_height { line_height } else { 0.0 @@ -16065,6 +16205,134 @@ mod tests { assert!(layout.global_status_bar.is_some()); } + // ─── #728: single-predicate status-row reservation ────────────────── + + /// `window_status_row_reserved` is now the only place either + /// `build_screen_layout` or GTK's `h_scrollbar_geometry` decide whether + /// a window paints its own bottom-row status line. Pin every axis it + /// depends on: the base setting, `terminal_maximized` (GTK's old + /// predicate accounted for this; `build_screen_layout`'s old one + /// didn't), and the `status_line_above_terminal`/bottom-panel + /// combination that produces a *separated* status bar instead + /// (`build_screen_layout`'s old predicate accounted for this; GTK's old + /// one didn't). + #[test] + fn window_status_row_reserved_covers_every_axis() { + use crate::core::engine::Engine; + + let base = || { + let mut e = Engine::new_for_test(); + e.settings.window_status_line = true; + e + }; + + // Setting off → never reserved, regardless of anything else. + let mut e = base(); + e.settings.window_status_line = false; + assert!(!window_status_row_reserved(&e), "setting off"); + + // Setting on, nothing else in play → reserved. + let e = base(); + assert!( + window_status_row_reserved(&e), + "plain per-window status must reserve its own row" + ); + + // Terminal maximized → editor windows aren't the visible surface; + // GTK's old predicate caught this, `build_screen_layout`'s didn't. + let mut e = base(); + e.terminal_maximized = true; + assert!( + !window_status_row_reserved(&e), + "a maximized terminal panel must suppress the per-window row" + ); + + // status_line_above_terminal ON (default) with the bottom panel + // open: per-window status bars stay inside each window (still + // "naturally above" the terminal), so the row is still reserved. + let mut e = base(); + e.settings.status_line_above_terminal = true; + e.terminal_open = true; + assert!( + window_status_row_reserved(&e), + "status_line_above_terminal keeps the status row inside the window" + ); + + // status_line_above_terminal OFF with the bottom panel open: the + // active window's status is pulled into a *separated* bar above the + // terminal instead, freeing this row. GTK's old predicate + // (`window_status_line && !terminal_maximized`) missed this axis + // entirely and would have reported "reserved" here. + let mut e = base(); + e.settings.status_line_above_terminal = false; + e.terminal_open = true; + assert!( + !window_status_row_reserved(&e), + "separated status must free the window's own bottom row" + ); + + // Same, but via the non-terminal bottom panel (debug output etc.) + // rather than the terminal specifically. + let mut e = base(); + e.settings.status_line_above_terminal = false; + e.bottom_panel_open = true; + assert!( + !window_status_row_reserved(&e), + "any open bottom panel — not just the terminal — must separate the status" + ); + + // status_line_above_terminal OFF but nothing open at the bottom: + // `separate_status` requires `bottom_panel_open`, so the row stays + // reserved in-window. + let e = { + let mut e = base(); + e.settings.status_line_above_terminal = false; + e + }; + assert!( + window_status_row_reserved(&e), + "no bottom panel open → nothing to separate the status from" + ); + } + + /// #728 acceptance: across every combination of the four settings + /// `window_status_row_reserved` depends on, GTK's h-scrollbar geometry + /// (via the same shared predicate) must never disagree with + /// `build_screen_layout` about whether a window's bottom row is free. + /// Exercised here through the shared predicate directly (both call + /// sites now route through it), rather than duplicating GTK's own + /// geometry math into a render.rs test. + #[test] + fn window_status_row_reserved_is_deterministic_across_all_combinations() { + use crate::core::engine::Engine; + + for window_status_line in [false, true] { + for status_line_above_terminal in [false, true] { + for bottom_panel_open in [false, true] { + for terminal_maximized in [false, true] { + let mut e = Engine::new_for_test(); + e.settings.window_status_line = window_status_line; + e.settings.status_line_above_terminal = status_line_above_terminal; + e.bottom_panel_open = bottom_panel_open; + e.terminal_maximized = terminal_maximized; + + let expected = window_status_line + && !terminal_maximized + && !(!status_line_above_terminal && bottom_panel_open); + assert_eq!( + window_status_row_reserved(&e), + expected, + "window_status_line={window_status_line} \ + status_line_above_terminal={status_line_above_terminal} \ + bottom_panel_open={bottom_panel_open} \ + terminal_maximized={terminal_maximized}" + ); + } + } + } + } + } + #[test] fn test_status_segments_have_actions() { use crate::core::engine::Engine; @@ -16537,6 +16805,81 @@ mod tests { test_engine(&text) } + /// A synthetic file large enough to make an O(buffer) per-frame cost + /// visible: 10,000 lines, none of them trivially short (so a full + /// buffer-wide `String` materialisation actually does real allocation + /// work, not just touch 10,000 empty strings). + fn large_minimap_engine(n_lines: usize) -> Engine { + let mut text = String::with_capacity(n_lines * 24); + for i in 0..n_lines { + text.push_str(&format!("fn line_{i}() {{ do_something({i}); }}\n")); + } + test_engine(&text) + } + + /// #728 performance acceptance: wheel-scrolling a 10,000-line file must + /// not cost O(buffer) per frame. `build_minimap_data` used to allocate a + /// `String` for *every* line in the buffer on every call regardless of + /// how many it actually samples (`quadraui::sample_lines` only keeps + /// ~`target_lines`, but the old code built the whole buffer as + /// candidates first) — this simulates sustained wheel scroll (one call + /// per frame, `scroll_top` advancing each time so the sampled window + /// keeps moving, the same as a real scroll) and pins a cost ceiling a + /// buffer-wide allocation blows through. + /// + /// Measured on this machine (debug `cargo test --no-default-features + /// --bin vcd`), 300 simulated scroll frames over a 10,000-line file: + /// - before (whole-buffer `owned: Vec` every frame): **24.23s + /// total, ~80.8ms/frame** — unusable under sustained wheel scroll, + /// matching the issue's report. + /// - after (index-first sampling, only ~`target_lines` lines fetched + /// from the rope per frame): **0.40s total, ~1.34ms/frame** — a + /// ~60x improvement, and no longer scales with buffer size at all + /// (cost tracks `target_lines`, i.e. the strip's own display rows). + /// + /// The ceiling below (500ms) is intentionally generous — an order of + /// magnitude above the fixed measurement — so the test is a regression + /// guard against reintroducing O(buffer) behavior, not a tight perf pin + /// that flakes on a loaded CI box. + #[test] + fn minimap_scroll_does_not_scale_with_buffer_size() { + let n_lines = 10_000; + let mut e = large_minimap_engine(n_lines); + e.settings.minimap = true; + let wid = e.active_window_id(); + let theme = Theme::onedark(); + let rect = WindowRect::new(0.0, 0.0, 100.0, 40.0); + + let frames = 300; + let start = std::time::Instant::now(); + for i in 0..frames { + if let Some(w) = e.windows.get_mut(&wid) { + w.view.scroll_top = i % n_lines; + } + let mm = build_minimap_data(&e, &theme, wid, rect, 1.0); + assert!(mm.is_some(), "minimap must build for every simulated frame"); + } + let elapsed = start.elapsed(); + eprintln!( + "minimap_scroll_does_not_scale_with_buffer_size: {frames} frames / \ + {n_lines} lines in {elapsed:?} ({:.4}ms/frame)", + elapsed.as_secs_f64() * 1000.0 / frames as f64 + ); + + // A per-frame whole-buffer materialisation (10,000 lines * 300 + // frames = 3,000,000 allocated/trimmed Strings) is the regression + // this guards against; a per-frame ~display-rows sampling is not. + // 500ms for 300 frames (1.6ms/frame) is generous headroom above the + // fixed cost on this machine, chosen to catch "still O(buffer)" + // without flaking on a slower/loaded box. + assert!( + elapsed.as_millis() < 500, + "300 simulated scroll frames over a {n_lines}-line buffer took \ + {elapsed:?} — expected well under 500ms; this smells like an \ + O(buffer)-per-frame regression" + ); + } + /// Acceptance: `:set nominimap` must widen the editor text area by /// *exactly* the reserved width, and `:set minimap` must give it back. /// Asserted on the rendered `text_viewport_cols`, not on the setting. @@ -16725,10 +17068,10 @@ mod tests { /// change the reserved width at a fixed pane width — the old formula /// multiplied `MINIMAP_COLS` by `char_width` directly, so a larger font /// made the strip wider, which is backwards (VS Code's minimap width is - /// independent of the editor font). Pane width chosen so the - /// fraction-derived want sits inside the clamp band for every - /// `char_width` tested, so the clamp itself can't be the reason the - /// widths happen to match. + /// independent of the editor font). Pane wide enough that `want` is + /// driven by `MINIMAP_TARGET_COLS` (#728) rather than the fraction, for + /// every `char_width` tested, so the clamp/fraction can't be the reason + /// the widths happen to match. #[test] fn minimap_reserved_width_is_unchanged_by_font_size() { let e = minimap_engine(); @@ -16740,7 +17083,7 @@ mod tests { "reserved width must not depend on char_width: \ 8px/char={small_font}, 16px/char={large_font}" ); - assert_eq!(small_font, pane_width * MINIMAP_WIDTH_FRACTION); + assert_eq!(small_font, MINIMAP_TARGET_COLS); } /// #722 review regression: `minimap_reserved_width_is_unchanged_by_font_size` @@ -16791,12 +17134,14 @@ mod tests { /// font's `MINIMAP_MAX_COLS * char_width` ceiling can exceed a /// merely-wide pane's `want`, so the clamp never engages) — precisely /// backwards, since VS Code's minimap cap does not grow with the - /// editor font. Pane wide enough that `want` exceeds the fixed 240px - /// ceiling at every font size tested. + /// editor font. Pane wide enough that the pane-fraction cap alone would + /// exceed `MINIMAP_TARGET_COLS` at every font size tested, so `want` + /// settles at the fixed target rather than either the fraction or + /// `MINIMAP_MAX_PX`. #[test] fn minimap_reserved_width_clamp_ceiling_is_font_invariant() { let e = minimap_engine(); - let pane_width = 2000.0; // want = 2000 * 0.15 = 300px, above the 240px ceiling + let pane_width = 2000.0; // fraction = 2000 * 0.15 = 300px, above the 120px target let small_font = minimap_reserved_width(&e, pane_width, 8.0); let large_font = minimap_reserved_width(&e, pane_width, 16.0); assert_eq!( @@ -16805,9 +17150,27 @@ mod tests { on char_width: 8px/char={small_font}, 16px/char={large_font}" ); assert_eq!( - small_font, MINIMAP_MAX_PX, - "a pane this wide must clamp to the fixed pixel ceiling, not a \ - char_width-scaled one" + small_font, MINIMAP_TARGET_COLS, + "a pane this wide must settle at the fixed VS Code-parity \ + target, not a char_width-scaled one" + ); + } + + /// #728 acceptance: the old formula (`want = rect_width * + /// MINIMAP_WIDTH_FRACTION`, clamped only at 240px) let an ordinary wide + /// GTK pane reach ~240px — roughly twice VS Code's own ~120px minimap, + /// which does not grow with window width at all. A representative + /// 1600px pane at an 8px font is exactly the case #728 reported: RED + /// against the pre-fix formula (`1600 * 0.15 = 240`, hitting + /// `MINIMAP_MAX_PX` — double the VS Code-parity width asserted here). + #[test] + fn minimap_reserved_width_matches_vs_code_parity_on_a_wide_pane() { + let e = minimap_engine(); + let want = minimap_reserved_width(&e, 1600.0, 8.0); + assert_eq!( + want, MINIMAP_TARGET_COLS, + "an ordinary wide GTK pane must settle at VS Code's ~120px \ + minimap width, not scale up with the pane: got {want}" ); } From 5adc8dc2d99fa6ce1f52fa001cb1e14be7cc8dec Mon Sep 17 00:00:00 2001 From: JDonaghy Date: Tue, 1 Sep 2026 18:46:52 -0500 Subject: [PATCH 2/2] test(#728): make the minimap scroll perf guard a scaling ratio, not a wall clock `minimap_scroll_does_not_scale_with_buffer_size` pinned an absolute 500ms budget for 300 frames. The fixed cost on an idle box is ~405ms, so the margin was only ~20% and the test flaked at 517-586ms when the full suite ran it alongside ~2,300 other tests, while passing in isolation. An absolute wall-clock budget cannot be made contention-proof. Assert on the property the test name already claims instead: run the same scroll workload over a 1,000-line and a 10,000-line buffer (10x the buffer, identical strip geometry so identical `target_lines`) and compare per-frame cost. The two series are interleaved frame-by-frame so a burst of CPU contention inflates both equally and cancels out of the ratio. Only the `build_minimap_data` call itself is inside the timed region. RED/GREEN verified: with the whole-buffer materialisation reinstated in `build_minimap_data`, the test fails at ratio 10.27x (1k: 7.93ms/frame, 10k: 81.48ms/frame - matching the issue's ~80.8ms/frame report); with the fix in place it passes at ratio ~0.96x. Threshold is 4.0x, sitting between the two with generous room on both sides. Contention-proofing checked directly: under 24 busy-loops on a 20-core box the per-frame cost roughly doubles (1.0ms -> 1.9ms, which is what blew the old 500ms budget) while the ratio holds at 0.94-0.97x across three runs. Test-only change; `build_minimap_data` is untouched. Co-Authored-By: Claude Opus 5 --- src/render.rs | 132 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 85 insertions(+), 47 deletions(-) diff --git a/src/render.rs b/src/render.rs index f44a977e..b6acbe35 100644 --- a/src/render.rs +++ b/src/render.rs @@ -16817,66 +16817,104 @@ mod tests { test_engine(&text) } - /// #728 performance acceptance: wheel-scrolling a 10,000-line file must - /// not cost O(buffer) per frame. `build_minimap_data` used to allocate a - /// `String` for *every* line in the buffer on every call regardless of - /// how many it actually samples (`quadraui::sample_lines` only keeps - /// ~`target_lines`, but the old code built the whole buffer as - /// candidates first) — this simulates sustained wheel scroll (one call - /// per frame, `scroll_top` advancing each time so the sampled window - /// keeps moving, the same as a real scroll) and pins a cost ceiling a - /// buffer-wide allocation blows through. - /// - /// Measured on this machine (debug `cargo test --no-default-features - /// --bin vcd`), 300 simulated scroll frames over a 10,000-line file: - /// - before (whole-buffer `owned: Vec` every frame): **24.23s - /// total, ~80.8ms/frame** — unusable under sustained wheel scroll, - /// matching the issue's report. - /// - after (index-first sampling, only ~`target_lines` lines fetched - /// from the rope per frame): **0.40s total, ~1.34ms/frame** — a - /// ~60x improvement, and no longer scales with buffer size at all - /// (cost tracks `target_lines`, i.e. the strip's own display rows). - /// - /// The ceiling below (500ms) is intentionally generous — an order of - /// magnitude above the fixed measurement — so the test is a regression - /// guard against reintroducing O(buffer) behavior, not a tight perf pin - /// that flakes on a loaded CI box. - #[test] - fn minimap_scroll_does_not_scale_with_buffer_size() { - let n_lines = 10_000; - let mut e = large_minimap_engine(n_lines); - e.settings.minimap = true; + /// Time `frames` simulated wheel-scroll frames of `build_minimap_data` + /// over `e`, advancing `scroll_top` each frame so the sampled window + /// keeps moving — exactly what sustained wheel scroll does. Returns the + /// accumulated time spent *inside* `build_minimap_data` only (engine + /// setup and the `scroll_top` write are outside the timed region). + fn time_minimap_frames(e: &mut Engine, n_lines: usize, frames: usize) -> std::time::Duration { let wid = e.active_window_id(); let theme = Theme::onedark(); let rect = WindowRect::new(0.0, 0.0, 100.0, 40.0); - - let frames = 300; - let start = std::time::Instant::now(); + let mut total = std::time::Duration::ZERO; for i in 0..frames { if let Some(w) = e.windows.get_mut(&wid) { w.view.scroll_top = i % n_lines; } - let mm = build_minimap_data(&e, &theme, wid, rect, 1.0); + let t0 = std::time::Instant::now(); + let mm = build_minimap_data(e, &theme, wid, rect, 1.0); + total += t0.elapsed(); assert!(mm.is_some(), "minimap must build for every simulated frame"); } - let elapsed = start.elapsed(); + total + } + + /// #728 performance acceptance: wheel-scrolling a big file must not cost + /// O(buffer) per frame. `build_minimap_data` used to allocate a `String` + /// for *every* line in the buffer on every call regardless of how many + /// it actually samples (`quadraui::sample_lines` only keeps + /// ~`target_lines`, but the old code built the whole buffer as + /// candidates first). + /// + /// **This asserts on a *ratio*, not a wall-clock ceiling.** The same + /// scroll workload is run over a 1,000-line buffer and a 10,000-line + /// one — 10x the buffer, identical strip geometry, so identical + /// `target_lines` — and the per-frame costs are compared. The two + /// measurements are *interleaved* frame-by-frame so a burst of CPU + /// contention (the full suite runs these tests in parallel with ~2,300 + /// others) inflates both sides equally and cancels out of the ratio. + /// + /// The earlier version of this test pinned a 500ms absolute budget for + /// 300 frames; the fixed cost on an idle box is ~405ms, so the margin + /// was ~20% and it flaked at 517–586ms under full-suite contention + /// while passing in isolation. Absolute timings cannot be made + /// contention-proof; the ratio can, and it is what the test name + /// actually claims. + /// + /// Measured on this machine (debug `cargo test --no-default-features + /// --bin vcd`), per-frame cost over a 10,000-line file: + /// - before (whole-buffer `owned: Vec` every frame): + /// **~80.8ms/frame** — unusable under sustained wheel scroll, + /// matching the issue's report, and ~10x the 1,000-line cost + /// because the work is linear in buffer size. + /// - after (index-first sampling, only ~`target_lines` lines fetched + /// from the rope per frame): **~1.34ms/frame**, i.e. ~1x the + /// 1,000-line cost — the cost tracks `target_lines` (the strip's + /// own display rows), not the buffer. + /// + /// So the discriminator is ~1x (fixed) vs ~10x (linear); the 4x + /// threshold below sits between them with generous room on both sides. + #[test] + fn minimap_scroll_does_not_scale_with_buffer_size() { + const SMALL_LINES: usize = 1_000; + const LARGE_LINES: usize = 10_000; + const FRAMES: usize = 150; + + let mut small = large_minimap_engine(SMALL_LINES); + small.settings.minimap = true; + let mut large = large_minimap_engine(LARGE_LINES); + large.settings.minimap = true; + + // Warm both sides (first-touch page faults, allocator growth) so the + // ratio measures steady-state work rather than one-time setup. + time_minimap_frames(&mut small, SMALL_LINES, 5); + time_minimap_frames(&mut large, LARGE_LINES, 5); + + // Interleaved: alternate one small frame and one large frame so both + // series see the same scheduling weather. + let mut small_total = std::time::Duration::ZERO; + let mut large_total = std::time::Duration::ZERO; + for _ in 0..FRAMES { + small_total += time_minimap_frames(&mut small, SMALL_LINES, 1); + large_total += time_minimap_frames(&mut large, LARGE_LINES, 1); + } + + let small_ms = small_total.as_secs_f64() * 1000.0 / FRAMES as f64; + let large_ms = large_total.as_secs_f64() * 1000.0 / FRAMES as f64; + let ratio = large_ms / small_ms.max(f64::MIN_POSITIVE); eprintln!( - "minimap_scroll_does_not_scale_with_buffer_size: {frames} frames / \ - {n_lines} lines in {elapsed:?} ({:.4}ms/frame)", - elapsed.as_secs_f64() * 1000.0 / frames as f64 + "minimap_scroll_does_not_scale_with_buffer_size: {FRAMES} frames \ + each — {SMALL_LINES} lines {small_ms:.4}ms/frame, {LARGE_LINES} \ + lines {large_ms:.4}ms/frame, ratio {ratio:.2}x" ); - // A per-frame whole-buffer materialisation (10,000 lines * 300 - // frames = 3,000,000 allocated/trimmed Strings) is the regression - // this guards against; a per-frame ~display-rows sampling is not. - // 500ms for 300 frames (1.6ms/frame) is generous headroom above the - // fixed cost on this machine, chosen to catch "still O(buffer)" - // without flaking on a slower/loaded box. assert!( - elapsed.as_millis() < 500, - "300 simulated scroll frames over a {n_lines}-line buffer took \ - {elapsed:?} — expected well under 500ms; this smells like an \ - O(buffer)-per-frame regression" + ratio < 4.0, + "a 10x bigger buffer cost {ratio:.2}x more per minimap frame \ + ({SMALL_LINES} lines: {small_ms:.4}ms/frame, {LARGE_LINES} \ + lines: {large_ms:.4}ms/frame) — the per-frame cost must track \ + the strip's display rows, not the buffer; this smells like a \ + return of the whole-buffer materialisation" ); }