#728: Minimap: VS Code-consistent width, stop rebuilding the whole buffer every frame, and stop overdrawing the status line - #739
Merged
JDonaghy merged 2 commits intoSep 2, 2026
Conversation
…er frame, reconcile status-row predicates 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 <noreply@anthropic.com>
… 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #728
Automated PR opened by coordinator for review of issue #728.