diff --git a/Cargo.lock b/Cargo.lock index 4f34a08b..bb19e218 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2219,7 +2219,7 @@ checksum = "d55d956fa96f5ec02be2e13af0e20391a5aa83d6a074e3ad368959d0fab299ea" [[package]] name = "quadraui" version = "0.0.1" -source = "git+https://github.com/JDonaghy/quadraui.git?rev=bde2f4481009fa156b20a76debd16c23468a5537#bde2f4481009fa156b20a76debd16c23468a5537" +source = "git+https://github.com/JDonaghy/quadraui.git?rev=69fd9cdd2107be401306bf64d1fd9553127d7226#69fd9cdd2107be401306bf64d1fd9553127d7226" dependencies = [ "arboard", "gtk4", @@ -3382,7 +3382,7 @@ dependencies = [ [[package]] name = "vt100" version = "0.16.2" -source = "git+https://github.com/JDonaghy/quadraui.git?rev=bde2f4481009fa156b20a76debd16c23468a5537#bde2f4481009fa156b20a76debd16c23468a5537" +source = "git+https://github.com/JDonaghy/quadraui.git?rev=69fd9cdd2107be401306bf64d1fd9553127d7226#69fd9cdd2107be401306bf64d1fd9553127d7226" dependencies = [ "itoa", "unicode-width", diff --git a/Cargo.toml b/Cargo.toml index 3782834d..ee81b3ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,7 +59,7 @@ gui = ["gtk4", "pangocairo", "gio", "quadraui/gtk"] # that file and CLAUDE.md's "Wrong quadraui checkout" section) — this # replaces the old `VIMCODE_QUADRAUI_UNPINNED=1` escape hatch, which could # not build backwards drift (a checkout older than a rev vimcode requires). -quadraui = { git = "https://github.com/JDonaghy/quadraui.git", rev = "bde2f4481009fa156b20a76debd16c23468a5537", features = ["tui", "terminal"] } +quadraui = { git = "https://github.com/JDonaghy/quadraui.git", rev = "69fd9cdd2107be401306bf64d1fd9553127d7226", features = ["tui", "terminal"] } gtk4 = { version = "0.7", features = ["v4_10"], optional = true } pangocairo = { version = "0.18", optional = true } ropey = "1.6.1" @@ -121,7 +121,7 @@ cc = "1.0" # do not remove this without reading both first. #691: same rev as the # `quadraui` dep above, resolved via git instead of a sibling path. [patch.crates-io] -vt100 = { git = "https://github.com/JDonaghy/quadraui.git", rev = "bde2f4481009fa156b20a76debd16c23468a5537" } +vt100 = { git = "https://github.com/JDonaghy/quadraui.git", rev = "69fd9cdd2107be401306bf64d1fd9553127d7226" } # Optimize dependencies in dev builds — GTK/Relm4/Cairo/Pango FFI calls and # Tree-sitter parsing are 10-50× slower without optimization, causing visible diff --git a/src/gtk/click.rs b/src/gtk/click.rs index 82f1d98b..decc50ce 100644 --- a/src/gtk/click.rs +++ b/src/gtk/click.rs @@ -91,14 +91,18 @@ pub(super) fn pixel_to_click_target( } } - // ── Minimap click / drag (#35) ────────────────────────────────────────── - // Pure rect plumbing: the shared resolver owns the hit-test and the scroll. - // Checked before the zone walk because the strip was carved out of the - // active window's rect, so a `ScreenZone::Window` hit would otherwise - // swallow it. Gated on `mutate_focus` so a hover query never scrolls. + // ── Minimap click / drag (#35, #722) ──────────────────────────────────── + // Pure rect plumbing: the shared resolver owns the hit-test and the + // scroll. Checked before the zone walk because every window's strip is + // carved out of that window's own rect, so a `ScreenZone::Window` hit + // would otherwise swallow it. Gated on `mutate_focus` so a hover query + // never scrolls. `apply_minimap_click` resolves against *every* pane's + // strip and reports which one it hit — never assumed to be the active + // window, since a split can have a strip on an inactive pane too. if mutate_focus { - if let Some(line) = render_mod::apply_minimap_click(engine, cached_layout, x, y) { - let window_id = engine.active_window_id(); + if let Some((window_id, line)) = + render_mod::apply_minimap_click(engine, cached_layout, x, y) + { return ClickTarget::Minimap(window_id, line); } } diff --git a/src/gtk/mod.rs b/src/gtk/mod.rs index fdf7c584..c2107aa3 100644 --- a/src/gtk/mod.rs +++ b/src/gtk/mod.rs @@ -8498,8 +8498,9 @@ impl quadraui::ShellApp for App { frame.draw(backend); } - // #35: minimap strip on the active window's right edge — one call, the - // font-scaling rasteriser is quadraui's. + // #35/#722: minimap strips on every window's right edge (one entry + // per `WindowId` in `screen.minimap`, not just the active window's) + // — one call, the font-scaling rasteriser is quadraui's. render::draw_minimap_strip(backend, screen); // ── Recover a FrameHitMap for Editor/TabBar zone detection (#449) ────── diff --git a/src/gtk/testing.rs b/src/gtk/testing.rs index d330fd08..661a2788 100644 --- a/src/gtk/testing.rs +++ b/src/gtk/testing.rs @@ -3811,8 +3811,9 @@ mod minimap { let l = layout.as_ref().unwrap(); let mm = l .minimap - .as_ref() - .expect("the layout must carry a minimap when the setting is on"); + .iter() + .find(|m| m.window_id == win_on) + .expect("the layout must carry a minimap for the pane when the setting is on"); let rw = l.windows.iter().find(|w| w.window_id == win_on).unwrap(); (mm.rect, rw.rect.width, rw.text_viewport_cols) }; @@ -3829,21 +3830,23 @@ mod minimap { let layout = h_off.screen_layout.borrow(); let l = layout.as_ref().unwrap(); assert!( - l.minimap.is_none(), + l.minimap.is_empty(), "`minimap: false` must remove the minimap from the layout" ); let rw = l.windows.iter().find(|w| w.window_id == win_off).unwrap(); (rw.rect.width, rw.text_viewport_cols) }; - // `reserved_width` is defined in character cells; assert the strip in - // that unit too, without this test having to re-derive GTK's - // char_width (which is not the same number the harness reports). - assert_eq!( - off_cols - on_cols, - crate::render::MINIMAP_COLS, - "the editor must regain exactly MINIMAP_COLS text columns when \ - the minimap is off (on={on_cols}, off={off_cols})" + // #722: the reserved width is now a proportion of the pane's own + // width rather than a fixed `MINIMAP_COLS` count, so the column + // delta isn't a pinned constant any more. The pixel-exact assertion + // right below is the real acceptance check (`build_screen_layout` + // narrows/widens the rect by exactly `strip.width`); this is just a + // column-domain sanity check that *some* text columns came back. + assert!( + off_cols > on_cols, + "the editor must regain text columns when the minimap is off \ + (on={on_cols}, off={off_cols})" ); assert_eq!( on_w + strip.width, @@ -3912,7 +3915,11 @@ mod minimap { let (strip, total, rect, gutter_px, char_w, lh) = { let layout = h.screen_layout.borrow(); let l = layout.as_ref().unwrap(); - let mm = l.minimap.as_ref().expect("minimap must be present"); + let mm = l + .minimap + .iter() + .find(|m| m.window_id == win) + .expect("minimap must be present for the active pane"); let rw = l .windows .iter() @@ -4032,8 +4039,9 @@ mod minimap { .as_ref() .unwrap() .minimap - .as_ref() - .expect("minimap must be present"); + .iter() + .find(|m| m.window_id == win) + .expect("minimap must be present for the active pane"); (mm.rect, mm.minimap.total_buffer_lines) }; assert_eq!( @@ -4079,6 +4087,155 @@ mod minimap { the drag continuation never reached the minimap" ); } + + /// `engine_with_shaped_buffer` split into two panes (`:vsplit`) — the + /// fixture the split-minimap black-box tests below share. + fn engine_with_split_shaped_buffer() -> Engine { + use crate::core::window::SplitDirection; + let mut engine = engine_with_shaped_buffer(); + engine.split_window(SplitDirection::Vertical, None); + engine + } + + /// #722 acceptance, painted-output tier: a `:vsplit` must paint **two** + /// independent minimap strips, one over each pane's own buffer — not a + /// single strip pinned to whichever pane happens to be active. + /// + /// GTK twin of TUI's + /// `split_paints_two_independent_minimap_strips_via_shell_app` + /// (`tui_main/shell_app.rs`) — same acceptance criterion, driven + /// through the real headless `GtkDriver` paint path. Every #722 GTK + /// test before this one (including + /// `minimap_paints_a_strip_whose_width_matches_reserved_width` above) + /// only ever painted a single, unsplit window; the review that + /// reopened #722 flagged the missing split/pixel coverage by name. + /// + /// RED against the pre-#722 code (single `Option` + /// gated on `active_window_id`): `screen.minimap` would carry no entry + /// for the inactive pane at all, so its strip band would sample as a + /// uniform block (plain editor background) instead of the varied + /// braille/syntax-colour content asserted below — confirmed by hand by + /// reverting `build_screen_layout`'s minimap map to + /// `.find(|(id, _)| *id == active_window_id)` before restoring the fix. + #[test] + fn split_paints_two_independent_minimap_strips() { + let mut h = harness(engine_with_split_shaped_buffer(), 1400, 900); + + let win_ids: Vec<_> = h.engine.borrow().windows.keys().copied().collect(); + assert_eq!(win_ids.len(), 2, "`:vsplit` must produce two windows"); + + // Paint a first frame — also primes `screen_layout`/`painted_line_height`. + for id in &win_ids { + h.window_center(*id) + .unwrap_or_else(|| panic!("pane {id:?} must paint")); + } + let lh = h + .painted_line_height() + .expect("frame must publish the line height it painted with"); + + for id in &win_ids { + let strip = { + let layout = h.screen_layout.borrow(); + layout + .as_ref() + .unwrap() + .minimap + .iter() + .find(|m| m.window_id == *id) + .unwrap_or_else(|| panic!("pane {id:?} must carry its own minimap")) + .rect + }; + let x0 = (strip.x + 2.0) as i32; + let x1 = (strip.x + strip.width - 2.0) as i32; + let y0 = (strip.y + lh) as i32; + let y1 = (strip.y + strip.height - lh) as i32; + let mut seen = std::collections::HashSet::new(); + for y in (y0..y1).step_by(3) { + for x in x0..x1 { + seen.insert(h.driver.pixel(x, y)); + } + } + assert!( + seen.len() > 1, + "pane {id:?}'s minimap strip must paint content, not a \ + uniform block: sampled x in {x0}..{x1}, y in {y0}..{y1}, \ + found only {:?}", + seen + ); + } + } + + /// #722 acceptance, painted-output tier: switching focus between panes + /// of a `:vsplit` must not move either pane's text — GTK twin of TUI's + /// `focus_change_does_not_move_either_panes_text_via_shell_app`. + /// Coverage for the "migrates on focus change, reflowing both panes" + /// symptom the issue called out as *worse* than the missing strip (the + /// width reclaim was gated on the same `is_active` flag as the strip + /// itself, so both panes reflowed on every focus change). + /// + /// Mutates focus directly through `Engine::focus_next_window` on the + /// harness's shared `engine: Rc>` — the same "assert on + /// engine state after an event" escape hatch the harness's own doc + /// comment describes, used here only to *drive* the focus change + /// (GTK's own Ctrl-W accelerator wiring is out of scope for this test) + /// — then forces a real repaint (`h.driver.render()`, the same pattern + /// `menu_bar_visible`'s test elsewhere in this file uses) and diffs the + /// two *painted* window rects, which is the acceptance claim under + /// test. + /// + /// RED against the pre-#722 code: focusing the right pane would widen + /// it (reclaiming the now-inactive left pane's minimap width) and + /// narrow the left pane by the same amount, moving both panes' painted + /// rects — confirmed by hand by reverting the `minimap_w` reclaim to + /// the old `is_active`-gated single value before restoring this fix. + #[test] + fn focus_change_does_not_move_either_panes_text() { + let mut h = harness(engine_with_split_shaped_buffer(), 1400, 900); + + let win_ids: Vec<_> = h.engine.borrow().windows.keys().copied().collect(); + assert_eq!(win_ids.len(), 2, "`:vsplit` must produce two windows"); + for id in &win_ids { + h.window_center(*id) + .unwrap_or_else(|| panic!("pane {id:?} must paint")); + } + + fn painted_rects( + h: &Harness, + win_ids: &[crate::core::WindowId], + ) -> Vec<(f64, f64, f64, f64)> { + let layout = h.screen_layout.borrow(); + let l = layout.as_ref().unwrap(); + win_ids + .iter() + .map(|id| { + let r = l.windows.iter().find(|w| w.window_id == *id).unwrap().rect; + (r.x, r.y, r.width, r.height) + }) + .collect() + } + + let before = painted_rects(&h, &win_ids); + + let active_before = h.engine.borrow().active_window_id(); + h.engine.borrow_mut().focus_next_window(); + let active_after = h.engine.borrow().active_window_id(); + assert_ne!( + active_before, active_after, + "test setup sanity: focus_next_window must actually move focus \ + to the other pane, or this test isn't exercising a focus \ + change at all" + ); + h.driver.render(); + + let after = painted_rects(&h, &win_ids); + + assert_eq!( + before, after, + "cycling focus between panes of a `:vsplit` must not move \ + either pane's painted rect (i.e. must not reflow either \ + pane's text width); before={before:?}, after={after:?}" + ); + } } /// Black-box coverage for the VimCode app icon painted left of the `File` diff --git a/src/render.rs b/src/render.rs index 20da8f7e..921e874d 100644 --- a/src/render.rs +++ b/src/render.rs @@ -4336,20 +4336,65 @@ pub struct ScreenLayout { /// active tabs. Independent of `editor_group_split` — window splits exist /// regardless of how many editor groups are open (#582). pub window_dividers: Vec, - /// Code-overview minimap for the active editor window, or `None` when the - /// `minimap` setting is off / the window is too narrow to spare the strip - /// (#35). Backends paint it with a single `draw_minimap` call and route - /// clicks through `MinimapLayout::hit_test`; every piece of sampling, - /// scale arithmetic, colour aggregation and dot packing lives in quadraui. - pub minimap: Option, + /// Code-overview minimap for *every* editor window (#35, #722) — one + /// entry per `WindowId` that has the `minimap` setting on and enough + /// width to spare the strip, empty when none do. A `:vsplit` therefore + /// carries two entries, one per pane, instead of a single strip pinned + /// to whichever pane happens to be active. Backends paint each with a + /// `draw_minimap` call (see `draw_minimap_strip`, which loops over this + /// vec) and route clicks through `MinimapLayout::hit_test`; every piece + /// of sampling, scale arithmetic, colour aggregation and dot packing + /// lives in quadraui. + pub minimap: Vec, } // ─── Minimap (#35) ──────────────────────────────────────────────────────────── -/// Character-cell columns the minimap strip reserves on the right edge of the -/// active editor window. In GTK this is multiplied by `char_width` to get -/// pixels; in TUI `char_width` is `1.0`, so it *is* the cell count. -pub const MINIMAP_COLS: usize = 12; +/// Fraction of a pane's own width the minimap strip reserves (#722). +/// +/// 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). +pub const MINIMAP_WIDTH_FRACTION: f64 = 0.15; + +/// 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 +/// constant is trivially font-invariant (TUI has no font size to vary). +/// Stops the strip shrinking to an unreadable sliver in a merely-narrow +/// pane, before `MINIMAP_MIN_TEXT_COLS` suppresses it outright in a very +/// narrow one. +const MINIMAP_MIN_COLS: f64 = 6.0; + +/// Ceiling on the reserved width for TUI, same units/caveats as +/// `MINIMAP_MIN_COLS`. Stops the strip from eating an ever-growing share of +/// a very wide pane. +const MINIMAP_MAX_COLS: f64 = 30.0; + +/// Floor on the reserved width for GTK, in **raw pixels** — deliberately +/// *not* `MINIMAP_MIN_COLS * char_width`. #722 made the un-clamped `want` +/// term font-invariant (a fraction of the pane's own pixel width), but an +/// earlier version of this fix left the clamp bounds multiplied by +/// `char_width`, which reintroduces font-dependence exactly where a +/// `:vsplit` pushes panes into: once a pane is narrow enough that `want` +/// falls below the floor, the *clamped* result is `MINIMAP_MIN_COLS * +/// char_width` again — doubling the editor font doubles the minimap width +/// in that regime (a ~300px pane went 48px→96px), which is precisely the +/// backwards behaviour #722 exists to prevent. Chosen to match the old +/// per-column floor at a representative 8px/char default font (6 cols * +/// 8px), but as a fixed pixel amount rather than something derived from +/// `char_width` at call time — see +/// `minimap_reserved_width_clamp_floor_is_font_invariant` below, which pins +/// this in the regime the un-clamped scaling test deliberately avoids. +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). +const MINIMAP_MAX_PX: f64 = 240.0; /// Text columns that must survive after reserving the strip. Below this the /// minimap suppresses itself rather than squeezing the editor into a sliver. @@ -4390,13 +4435,34 @@ 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. +/// +/// 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 +/// split pushes panes into (see `MINIMAP_MIN_PX`'s doc comment). TUI is +/// cell-native (`char_width == 1.0` always) so `MINIMAP_MIN_COLS` / +/// `MINIMAP_MAX_COLS` are already font-invariant there; GTK gets separate +/// raw-pixel bounds (`MINIMAP_MIN_PX` / `MINIMAP_MAX_PX`) that do not scale +/// with `char_width` at all. `char_width > 1.0` is this file's existing +/// convention for "this call is GTK, not TUI" (see the `#515` comment on +/// the `bar_width` hit-region conversion above). +/// /// Delegates the on/off decision to `quadraui::reserved_width` so both -/// backends (and `build_screen_layout`, which shrinks the window rect by +/// backends (and `build_screen_layout`, which shrinks each window rect by /// exactly this much) reclaim identical geometry when `:set nominimap` /// turns the strip off. pub fn minimap_reserved_width(engine: &Engine, rect_width: f64, char_width: f64) -> f64 { let cw = if char_width > 0.0 { char_width } else { 1.0 }; - let want = MINIMAP_COLS as f64 * cw; + let (min_w, max_w) = if char_width > 1.0 { + (MINIMAP_MIN_PX, MINIMAP_MAX_PX) + } else { + (MINIMAP_MIN_COLS * cw, MINIMAP_MAX_COLS * cw) + }; + let want = (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 } @@ -4529,8 +4595,9 @@ pub fn build_minimap_data( }) } -/// Paint `screen.minimap` (if any) through the backend's own rasteriser and -/// return the resolved layout for click routing. +/// Paint every entry in `screen.minimap` (#722 — one per editor pane, not +/// just the active one) through the backend's own rasteriser and return the +/// resolved layouts for click routing, in the same order. /// /// This is the *entire* backend-side contract for the minimap: GTK's font /// scaling and TUI's braille packing are quadraui's implementations of @@ -4540,13 +4607,16 @@ pub fn build_minimap_data( pub fn draw_minimap_strip( backend: &mut dyn quadraui::Backend, screen: &ScreenLayout, -) -> Option { - let mm = screen.minimap.as_ref()?; - Some( - backend - .draw_minimap(minimap_strip_rect(mm), &mm.minimap) - .layout, - ) +) -> Vec { + screen + .minimap + .iter() + .map(|mm| { + backend + .draw_minimap(minimap_strip_rect(mm), &mm.minimap) + .layout + }) + .collect() } /// The strip a `RenderedMinimap` occupies, in quadraui coordinates. @@ -4561,49 +4631,62 @@ pub fn minimap_strip_rect(mm: &RenderedMinimap) -> quadraui::Rect { } /// Resolve a click/drag at `(x, y)` (backend units, same space as -/// [`minimap_strip_rect`]) against the minimap track, returning the buffer -/// line to scroll to. `None` when there is no minimap or the point misses it. +/// [`minimap_strip_rect`]) against every pane's minimap track (#722), +/// returning the window it hit plus the buffer line to scroll that window +/// to. `None` when no pane has a minimap or the point misses all of them. +/// +/// Panes never overlap, so at most one strip can claim a given point — the +/// loop stops at the first hit rather than needing to pick a "closest" one. /// /// Needs no backend instance: `MinimapLayout::hit_test` resolves purely from /// `bounds`, which is the same strip rect both rasterisers were handed — /// `lines_per_row` only shapes the *painted* rows, never the hit fraction. So /// "click the vertical middle → ~50% of the file" is one behaviour computed /// once, not two implementations that can drift. -pub fn minimap_click_line(screen: &ScreenLayout, x: f64, y: f64) -> Option { - let mm = screen.minimap.as_ref()?; - let layout = mm - .minimap - .layout(minimap_strip_rect(mm), MINIMAP_LINES_PER_ROW); - match layout.hit_test(x as f32, y as f32) { - quadraui::MinimapHit::Seek { fraction } => Some(minimap_fraction_to_line( - fraction, - mm.minimap.total_buffer_lines, - )), - quadraui::MinimapHit::None => None, +pub fn minimap_click_line(screen: &ScreenLayout, x: f64, y: f64) -> Option<(WindowId, usize)> { + for mm in &screen.minimap { + let layout = mm + .minimap + .layout(minimap_strip_rect(mm), MINIMAP_LINES_PER_ROW); + if let quadraui::MinimapHit::Seek { fraction } = layout.hit_test(x as f32, y as f32) { + return Some(( + mm.window_id, + minimap_fraction_to_line(fraction, mm.minimap.total_buffer_lines), + )); + } } + None } -/// Apply a minimap click/drag: scroll the strip's window to the clicked -/// fraction of the file and carry the cursor with it, so the next -/// `ensure_cursor_visible` doesn't snap the view straight back. +/// Apply a minimap click/drag: focus the *hit* pane, scroll it to the +/// clicked fraction of the file, and carry the cursor with it, so the next +/// `ensure_cursor_visible` doesn't snap the view straight back. Works +/// against whichever pane's strip the point landed on (#722), not just the +/// active window. +/// +/// The focus switch (`focus_group_for_window` + `set_cursor_for_window`, +/// the same pair `Engine::mouse_click` uses for a plain buffer click) is a +/// #722 follow-up: before every pane had its own minimap, only the active +/// pane's strip was ever clickable, so "click a strip without focusing its +/// pane" could never arise. Now that a background pane's strip is +/// reachable too, scrolling it without also bringing it to the front would +/// read as broken — a click that visibly moves a pane's view but leaves +/// focus (and keyboard input) somewhere else. /// -/// Returns the buffer line scrolled to, or `None` when the point missed the -/// strip (in which case the caller must fall through to its normal editor -/// click handling). +/// Returns the window scrolled and the buffer line scrolled to, or `None` +/// when the point missed every strip (in which case the caller must fall +/// through to its normal editor click handling). pub fn apply_minimap_click( engine: &mut Engine, screen: &ScreenLayout, x: f64, y: f64, -) -> Option { - let line = minimap_click_line(screen, x, y)?; - let window_id = screen.minimap.as_ref()?.window_id; +) -> Option<(WindowId, usize)> { + let (window_id, line) = minimap_click_line(screen, x, y)?; + engine.focus_group_for_window(window_id); engine.set_scroll_top_for_window(window_id, line); - if let Some(w) = engine.windows.get_mut(&window_id) { - w.view.cursor.line = line; - w.view.cursor.col = 0; - } - Some(line) + engine.set_cursor_for_window(window_id, line, 0); + Some((window_id, line)) } /// Buffer line a minimap click at `fraction` of the track should scroll to. @@ -6762,14 +6845,17 @@ pub fn build_screen_layout_with_breadcrumb_row( // check below, since `:split`/`:vsplit` panes exist within a single group. let window_dividers = engine.calculate_window_dividers(window_rects); - // Minimap strip (#35). Reserved off the *active* window's right edge, and - // subtracted from that window's rect before the text is laid out so the - // editor reclaims the exact same width when `:set nominimap` turns it off. - let minimap_w = window_rects + // Minimap strip (#35, #722). Reserved off *every* window's right edge — + // not just the active one — and subtracted from that same window's rect + // before its text is laid out, so each pane reclaims exactly its own + // strip's width when `:set nominimap` turns the strip off. Keyed per + // window (rather than a single scalar) because `minimap_reserved_width` + // is a function of that window's own rect width, so unevenly split + // panes legitimately get differently-sized strips. + let minimap_widths: std::collections::HashMap = window_rects .iter() - .find(|(id, _)| *id == active_window_id) - .map(|(_, r)| minimap_reserved_width(engine, r.width, char_width)) - .unwrap_or(0.0); + .map(|(id, r)| (*id, minimap_reserved_width(engine, r.width, char_width))) + .collect(); let windows = window_rects .iter() @@ -6779,14 +6865,11 @@ pub fn build_screen_layout_with_breadcrumb_row( visible_lines -= 1; // reserve bottom row for per-window status bar } let is_active = *window_id == active_window_id; + let minimap_w = minimap_widths.get(window_id).copied().unwrap_or(0.0); let narrowed = WindowRect::new( rect.x, rect.y, - if is_active { - (rect.width - minimap_w).max(0.0) - } else { - rect.width - }, + (rect.width - minimap_w).max(0.0), rect.height, ); let rect = &narrowed; @@ -6814,34 +6897,37 @@ pub fn build_screen_layout_with_breadcrumb_row( }) .collect(); - // The strip itself: the sliver just reclaimed above, minus the per-window - // status row when one is painted inside the window. - let minimap = if minimap_w > 0.0 { - window_rects - .iter() - .find(|(id, _)| *id == active_window_id) - .and_then(|(id, r)| { - let status_h = if per_window_status && !separate_status && r.height > line_height { - line_height - } else { - 0.0 - }; - build_minimap_data( - engine, - theme, - *id, - WindowRect::new( - r.x + r.width - minimap_w, - r.y, - minimap_w, - (r.height - status_h).max(0.0), - ), - line_height, - ) - }) - } else { - None - }; + // The strips themselves: the sliver just reclaimed off each window above, + // minus the per-window status row when one is painted inside that window. + // One `RenderedMinimap` per window that has a strip, in `window_rects` + // order — a `:vsplit` therefore carries two independent strips, each + // over its own pane's buffer, instead of one that migrates with focus. + let minimap: Vec = window_rects + .iter() + .filter_map(|(id, r)| { + let minimap_w = minimap_widths.get(id).copied().unwrap_or(0.0); + if minimap_w <= 0.0 { + return None; + } + let status_h = if per_window_status && !separate_status && r.height > line_height { + line_height + } else { + 0.0 + }; + build_minimap_data( + engine, + theme, + *id, + WindowRect::new( + r.x + r.width - minimap_w, + r.y, + minimap_w, + (r.height - status_h).max(0.0), + ), + line_height, + ) + }) + .collect(); let separated_status_line = if separate_status { Some(build_window_status_line( @@ -16396,7 +16482,7 @@ mod tests { e } - // ─── Minimap (#35) ─────────────────────────────────────────────────── + // ─── Minimap (#35, #722) ──────────────────────────────────────────── /// A file with a distinctive indentation shape — deep on the inside, /// flush at the edges — so a transposed dot grid would be obvious. @@ -16413,6 +16499,10 @@ mod tests { /// 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. + /// The expected delta is computed through `minimap_reserved_width` + /// itself (the single source of truth `build_screen_layout` also + /// reads) rather than a hardcoded column count, since #722 made that + /// width a function of pane width instead of a fixed constant. #[test] fn nominimap_widens_the_editor_by_exactly_the_reserved_width() { let mut e = minimap_engine(); @@ -16420,24 +16510,124 @@ mod tests { e.settings.minimap = true; let with = render_engine(&e, 120.0, 30.0); let cols_with = with.windows[0].text_viewport_cols; - assert!( - with.minimap.is_some(), + assert_eq!( + with.minimap.len(), + 1, "the minimap must be present when the setting is on" ); + let expected_cols = minimap_reserved_width(&e, 120.0, 1.0) as usize; e.settings.minimap = false; let without = render_engine(&e, 120.0, 30.0); let cols_without = without.windows[0].text_viewport_cols; assert!( - without.minimap.is_none(), + without.minimap.is_empty(), "`:set nominimap` must remove the minimap from the layout" ); assert_eq!( cols_without - cols_with, - MINIMAP_COLS, - "turning the minimap off must hand the editor back exactly \ - MINIMAP_COLS columns (with={cols_with}, without={cols_without})" + expected_cols, + "turning the minimap off must hand the editor back exactly the \ + reserved width (with={cols_with}, without={cols_without}, \ + expected={expected_cols})" + ); + } + + /// #722 acceptance: `:set nominimap` reclaims exactly the reserved width + /// in *every* pane of a split, not just a single active one — extending + /// the test above to `:vsplit`. + #[test] + fn nominimap_widens_every_pane_in_a_split_by_exactly_its_reserved_width() { + let mut e = minimap_engine(); + e.split_window(SplitDirection::Vertical, None); + + e.settings.minimap = true; + let with = render_engine(&e, 160.0, 30.0); + assert_eq!(with.windows.len(), 2, "vsplit must produce two windows"); + assert_eq!( + with.minimap.len(), + 2, + "both panes must carry their own minimap when the setting is on" + ); + + e.settings.minimap = false; + let without = render_engine(&e, 160.0, 30.0); + assert!( + without.minimap.is_empty(), + "`:set nominimap` must remove every pane's minimap" + ); + // `minimap_reserved_width` reads `engine.settings.minimap` itself — + // put it back on before using `e` to recompute what each pane + // *would* reserve, or every expectation below collapses to 0. + e.settings.minimap = true; + + for w_with in &with.windows { + let w_without = without + .windows + .iter() + .find(|w| w.window_id == w_with.window_id) + .expect("window set must be identical with/without the minimap"); + // `w_with.rect.width` is already narrowed by its own strip; + // `w_without.rect.width` is the same pane's un-narrowed width + // (minimap off ⇒ no narrowing), which is what + // `minimap_reserved_width` expects to be fed back in. + let expected_cols = minimap_reserved_width(&e, w_without.rect.width, 1.0) as usize; + assert_eq!( + w_without.text_viewport_cols - w_with.text_viewport_cols, + expected_cols, + "pane {:?} must regain exactly its own reserved width \ + (with={}, without={}, expected={expected_cols})", + w_with.window_id, + w_with.text_viewport_cols, + w_without.text_viewport_cols + ); + } + } + + /// #722 review follow-up (non-blocking finding): clicking a + /// **background** pane's minimap must focus that pane, not just + /// scroll/reposition its cursor while leaving focus (and keyboard + /// input) on whichever pane was already active. Before #722 this could + /// never come up — only the active pane had a minimap at all — so + /// there was no "click a strip that isn't the focused pane's" case to + /// get wrong until every pane got its own strip. + #[test] + fn minimap_click_on_a_background_pane_focuses_that_pane() { + let mut e = minimap_engine(); + e.split_window(SplitDirection::Vertical, None); + let screen = render_engine(&e, 160.0, 30.0); + assert_eq!( + screen.minimap.len(), + 2, + "vsplit must give each pane a minimap" + ); + + let active_before = e.active_window_id(); + let background_mm = screen + .minimap + .iter() + .find(|m| m.window_id != active_before) + .expect("the split must have a non-active pane with its own minimap"); + let mid_x = background_mm.rect.x + background_mm.rect.width / 2.0; + let mid_y = background_mm.rect.y + background_mm.rect.height / 2.0; + + let (hit_win, _line) = + apply_minimap_click(&mut e, &screen, mid_x, mid_y).expect("click must be handled"); + assert_eq!( + hit_win, background_mm.window_id, + "test setup sanity: the click must resolve to the background pane" + ); + assert_ne!( + hit_win, active_before, + "test setup sanity: the clicked pane must actually have been \ + the non-active one" + ); + assert_eq!( + e.active_window_id(), + background_mm.window_id, + "clicking a background pane's minimap must focus that pane, \ + not just scroll it while leaving focus on {active_before:?}" ); } @@ -16447,12 +16637,8 @@ mod tests { fn minimap_reserved_width_is_zero_when_the_setting_is_off() { let mut e = minimap_engine(); e.settings.minimap = true; - assert_eq!(minimap_reserved_width(&e, 120.0, 1.0), MINIMAP_COLS as f64); - // GTK units: the same call in pixels. - assert_eq!( - minimap_reserved_width(&e, 1200.0, 8.0), - MINIMAP_COLS as f64 * 8.0 - ); + assert!(minimap_reserved_width(&e, 120.0, 1.0) > 0.0); + assert!(minimap_reserved_width(&e, 1200.0, 8.0) > 0.0); e.settings.minimap = false; assert_eq!(minimap_reserved_width(&e, 120.0, 1.0), 0.0); assert_eq!(minimap_reserved_width(&e, 1200.0, 8.0), 0.0); @@ -16466,7 +16652,121 @@ mod tests { assert_eq!( minimap_reserved_width(&e, 20.0, 1.0), 0.0, - "a 20-column window cannot spare a 12-column strip" + "a 20-column window cannot spare the minimap's floor width plus \ + MINIMAP_MIN_TEXT_COLS of surviving text" + ); + } + + /// #722 acceptance: the reserved width is a proportion of the *pane's* + /// width, so widening the pane must widen the strip too — unlike the + /// old fixed `MINIMAP_COLS` constant, which stayed put as the window + /// grew. Both widths chosen well inside `[MINIMAP_MIN_COLS, + /// MINIMAP_MAX_COLS]` so the clamp never masks the scaling. + #[test] + fn minimap_reserved_width_scales_with_pane_width() { + let e = minimap_engine(); + // Both widths land `want = width * MINIMAP_WIDTH_FRACTION` strictly + // inside `[MINIMAP_MIN_COLS, MINIMAP_MAX_COLS]` (9 and 18, against a + // 6..30 band) so the clamp can't be masking the scaling either test + // exercises. + let narrow = minimap_reserved_width(&e, 60.0, 1.0); + let wide = minimap_reserved_width(&e, 120.0, 1.0); + assert_eq!(narrow, 60.0 * MINIMAP_WIDTH_FRACTION); + assert_eq!(wide, 120.0 * MINIMAP_WIDTH_FRACTION); + assert!( + wide > narrow * 1.5, + "doubling the pane width must substantially widen the strip: \ + narrow(60)={narrow}, wide(120)={wide}" + ); + } + + /// #722 acceptance: bumping the editor font (`char_width`) must NOT + /// 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. + #[test] + fn minimap_reserved_width_is_unchanged_by_font_size() { + let e = minimap_engine(); + let pane_width = 1200.0; + let small_font = minimap_reserved_width(&e, pane_width, 8.0); + let large_font = minimap_reserved_width(&e, pane_width, 16.0); + assert_eq!( + small_font, large_font, + "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); + } + + /// #722 review regression: `minimap_reserved_width_is_unchanged_by_font_size` + /// above deliberately keeps `want` inside the clamp band, so it cannot + /// catch font-dependence in the clamp *bounds* themselves. A first pass + /// of #722 made the un-clamped `want` term font-invariant but left the + /// floor/ceiling as `MINIMAP_MIN_COLS * char_width` / + /// `MINIMAP_MAX_COLS * char_width` — so once a pane is narrow enough to + /// actually hit the floor (exactly the regime a `:vsplit` produces), + /// the *clamped* result was font-dependent again. This pins the fixed + /// scenario from the review finding: a ~300px pane, at two GTK font + /// sizes close enough together that neither hits + /// `MINIMAP_MIN_TEXT_COLS`'s separate (and correct — #722 keeps it) + /// suppression floor, so any difference in the result can only be the + /// clamp bound moving with the font. + /// + /// RED against the pre-fix formula: at `rect_width = 300.0`, + /// `char_width = 6.0` gives `min_w = 36.0` (want=45 not clamped → 45), + /// while `char_width = 8.0` gives `min_w = 48.0` (want=45 clamped → + /// 48) — two different widths for the same pane at two font sizes. + #[test] + fn minimap_reserved_width_clamp_floor_is_font_invariant() { + let e = minimap_engine(); + let pane_width = 300.0; // want = 300 * 0.15 = 45px, below the 48px floor + let small_font = minimap_reserved_width(&e, pane_width, 6.0); + let large_font = minimap_reserved_width(&e, pane_width, 8.0); + assert!( + small_font > 0.0 && large_font > 0.0, + "test setup sanity: neither font size may trip \ + MINIMAP_MIN_TEXT_COLS's suppression floor, or this isn't \ + exercising the clamp bound at all: small={small_font}, \ + large={large_font}" + ); + assert_eq!( + small_font, large_font, + "the clamped (floor-hitting) reserved width must not depend on \ + char_width either: 6px/char={small_font}, 8px/char={large_font}" + ); + assert_eq!( + small_font, MINIMAP_MIN_PX, + "a pane this narrow must clamp to the fixed pixel floor, not a \ + char_width-scaled one" + ); + } + + /// Same regression, at the ceiling: an old formula whose `max_w` scaled + /// with `char_width` let a large font effectively lift the cap (a giant + /// 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. + #[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 small_font = minimap_reserved_width(&e, pane_width, 8.0); + let large_font = minimap_reserved_width(&e, pane_width, 16.0); + assert_eq!( + small_font, large_font, + "the clamped (ceiling-hitting) reserved width must not depend \ + 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" ); } @@ -16476,12 +16776,18 @@ mod tests { fn minimap_click_at_the_middle_seeks_to_half_the_file() { let mut e = minimap_engine(); let screen = render_engine(&e, 120.0, 30.0); - let mm = screen.minimap.as_ref().expect("minimap present"); + let win_id = screen.windows[0].window_id; + let mm = screen.minimap.first().expect("minimap present"); let total = mm.minimap.total_buffer_lines as f64; let mid_x = mm.rect.x + mm.rect.width / 2.0; let mid_y = mm.rect.y + mm.rect.height / 2.0; - let line = minimap_click_line(&screen, mid_x, mid_y).expect("middle of the strip must hit"); + let (hit_win, line) = + minimap_click_line(&screen, mid_x, mid_y).expect("middle of the strip must hit"); + assert_eq!( + hit_win, win_id, + "the hit must resolve to the pane it was clicked in" + ); let frac = line as f64 / total; assert!( (frac - 0.5).abs() < 0.1, @@ -16490,8 +16796,9 @@ mod tests { ); // …and it actually scrolls the window there. - let scrolled = + let (scrolled_win, scrolled) = apply_minimap_click(&mut e, &screen, mid_x, mid_y).expect("click must be handled"); + assert_eq!(scrolled_win, win_id); assert_eq!(scrolled, line); assert_eq!(e.scroll_top(), line, "the window must be scrolled to it"); } @@ -16502,12 +16809,13 @@ mod tests { fn minimap_click_top_bottom_and_miss() { let e = minimap_engine(); let screen = render_engine(&e, 120.0, 30.0); - let mm = screen.minimap.as_ref().expect("minimap present"); + let win_id = screen.windows[0].window_id; + let mm = screen.minimap.first().expect("minimap present"); let total = mm.minimap.total_buffer_lines; let x = mm.rect.x + 1.0; - assert_eq!(minimap_click_line(&screen, x, mm.rect.y), Some(0)); - let bottom = minimap_click_line(&screen, x, mm.rect.y + mm.rect.height - 0.5) + assert_eq!(minimap_click_line(&screen, x, mm.rect.y), Some((win_id, 0))); + let (_, bottom) = minimap_click_line(&screen, x, mm.rect.y + mm.rect.height - 0.5) .expect("bottom of the track must hit"); assert!( bottom >= total - total / 10, @@ -16541,7 +16849,7 @@ mod tests { fn minimap_samples_the_whole_buffer_in_order() { let e = minimap_engine(); let screen = render_engine(&e, 120.0, 30.0); - let mm = &screen.minimap.as_ref().expect("minimap present").minimap; + let mm = &screen.minimap.first().expect("minimap present").minimap; assert_eq!( mm.total_buffer_lines, 201, "200 lines plus the trailing one" @@ -16562,6 +16870,107 @@ mod tests { ); } + /// #722 acceptance: in a `:vsplit`, both panes show their own minimap + /// over their own buffer — not a single strip pinned to the active + /// pane. Each buffer gets distinct content so a transposed or + /// cross-wired sample (pane A showing pane B's file) would show up as a + /// mismatched `total_buffer_lines`. + #[test] + fn split_gives_every_pane_its_own_minimap_over_its_own_buffer() { + let mut e = test_engine(""); + e.buffer_mut().insert(0, &"left\n".repeat(50)); + e.split_window(SplitDirection::Vertical, None); + // The split's new window starts on the same buffer; give it its own + // so the two minimaps are provably over different files. + let new_buf = e.buffer_manager.create(); + e.buffer_manager + .get_mut(new_buf) + .unwrap() + .buffer + .insert(0, &"right\n".repeat(120)); + e.active_window_mut().buffer_id = new_buf; + + let screen = render_engine(&e, 160.0, 30.0); + assert_eq!(screen.windows.len(), 2, "vsplit must produce two windows"); + assert_eq!( + screen.minimap.len(), + 2, + "both panes must carry a minimap, not just the active one" + ); + + let totals: Vec = screen + .windows + .iter() + .map(|w| { + screen + .minimap + .iter() + .find(|m| m.window_id == w.window_id) + .unwrap_or_else(|| panic!("pane {:?} must have its own minimap", w.window_id)) + .minimap + .total_buffer_lines + }) + .collect(); + assert_ne!( + totals[0], totals[1], + "the two panes' minimaps must reflect their own distinct buffers, \ + got matching totals {totals:?}" + ); + } + + /// #722 acceptance: switching focus between panes must not change + /// either pane's text width. Before the fix, the width reclaim was + /// gated on `is_active`, so *both* panes reflowed every time focus + /// moved — assert the window rects (hence `text_viewport_cols`) are + /// bit-identical across a focus change with the minimap on. + #[test] + fn focus_change_does_not_move_either_panes_text_width() { + let mut e = minimap_engine(); + e.split_window(SplitDirection::Vertical, None); + e.settings.minimap = true; + + let before = render_engine(&e, 160.0, 30.0); + assert_eq!(before.minimap.len(), 2, "both panes must have a minimap"); + let widths_before: std::collections::HashMap = before + .windows + .iter() + .map(|w| (w.window_id, w.rect.width)) + .collect(); + let cols_before: std::collections::HashMap = before + .windows + .iter() + .map(|w| (w.window_id, w.text_viewport_cols)) + .collect(); + + let focus_before = e.active_window_id(); + e.focus_next_window(); + assert_ne!( + e.active_window_id(), + focus_before, + "test setup sanity: focus must actually have moved" + ); + + let after = render_engine(&e, 160.0, 30.0); + assert_eq!( + after.minimap.len(), + 2, + "both panes must still have a minimap" + ); + for w in &after.windows { + assert_eq!( + w.rect.width, widths_before[&w.window_id], + "pane {:?}'s width must not change when focus moves elsewhere", + w.window_id + ); + assert_eq!( + w.text_viewport_cols, cols_before[&w.window_id], + "pane {:?}'s text_viewport_cols must not change when focus \ + moves elsewhere", + w.window_id + ); + } + } + #[test] fn test_settings_to_form_read_only_by_default() { let e = test_engine(""); @@ -16690,6 +17099,11 @@ mod tests { #[test] fn test_screen_layout_split_windows() { let mut e = test_engine("file one\n"); + // This test is about general split geometry, not the minimap — turn + // it off so its per-pane width reservation (#722: every pane reserves + // its own strip now, not just the active one) doesn't confound the + // "windows divide the available width" assertion below. + e.settings.minimap = false; // Open a vertical split e.open_editor_group(SplitDirection::Vertical); diff --git a/src/tui_main/render_impl.rs b/src/tui_main/render_impl.rs index 2a22087e..71d76d9c 100644 --- a/src/tui_main/render_impl.rs +++ b/src/tui_main/render_impl.rs @@ -551,8 +551,9 @@ pub(super) fn draw_frame( ); } render_all_windows(backend, Some(frame), &screen.windows, theme); - // #35: minimap strip on the active window's right edge — one call, the - // braille rasteriser is quadraui's. + // #35/#722: minimap strips on every window's right edge (one entry per + // `WindowId` in `screen.minimap`, not just the active window's) — one + // call, the braille rasteriser is quadraui's. render::draw_minimap_strip(backend, screen); // Draw each group's tab bar. The tab bar sits `tui_tbh` rows above the // group's window content (`bounds.y - tui_tbh`, applied inside diff --git a/src/tui_main/shell_app.rs b/src/tui_main/shell_app.rs index 1d4f0a3a..a8f67e89 100644 --- a/src/tui_main/shell_app.rs +++ b/src/tui_main/shell_app.rs @@ -1469,8 +1469,9 @@ impl ShellApp for TuiShellApp { // longer needs the raw `Frame` that `frame: None` used to skip it // for). render_all_windows(backend, None, &screen.windows, &theme); - // #35: minimap strip on the active window's right edge — one call, the - // braille rasteriser is quadraui's. + // #35/#722: minimap strips on every window's right edge (one entry + // per `WindowId` in `screen.minimap`, not just the active window's) + // — one call, the braille rasteriser is quadraui's. render::draw_minimap_strip(backend, &screen); let tui_tbh: f64 = if self.engine.settings.breadcrumbs && !self.engine.terminal_maximized { @@ -8008,6 +8009,201 @@ mod tests { .position(|c| ('\u{2800}'..='\u{28FF}').contains(&c)) } + /// Whether `s` contains a *non-blank* braille glyph + /// (`\u{2801}'..='\u{28FF}`, excluding the all-empty `\u{2800}` cell) — + /// the minimap's own paint signature. + fn has_braille(s: &str) -> bool { + s.chars().any(|c| ('\u{2801}'..='\u{28FF}').contains(&c)) + } + + /// `app_with_shaped_buffer` split into two panes (`:vsplit`) — the + /// fixture the split-minimap black-box tests below share. + /// + /// Sidebar/autohide state pinned explicitly rather than left ambient: + /// `TuiShellApp::new`'s sidebar visibility reads the developer's real + /// `~/.config/vimcode` off disk (`app_with_sidebar_open`'s doc comment + /// above explains the split-state shape this produces), and per + /// `on_shell_event`'s own doc comment the runner's *painted* `AppShell` + /// only picks up the engine's (the "shadow"'s) sidebar/autohide state + /// at the tail of a dispatch — never on the very first frame + /// `driver_with_shell` paints before any `handle()` call. Pinning both + /// here keeps `focus_change_does_not_move_either_panes_text_via_shell_app` + /// from spuriously seeing the sidebar appear/disappear mid-test, which + /// would otherwise be indistinguishable from a real minimap regression. + fn app_with_split_shaped_buffer() -> TuiShellApp { + let mut app = app_with_shaped_buffer(); + app.engine.settings.autohide_panels = false; + app.engine.app_shell.hide_sidebar(); + app.engine.session.explorer_visible = false; + app.engine.split_window(SplitDirection::Vertical, None); + app + } + + /// #722 acceptance, painted-output tier: a `:vsplit` must paint **two** + /// independent minimap strips, one over each pane's own buffer — not a + /// single strip pinned to whichever pane happens to be active. + /// + /// The review that reopened #722 flagged that every prior test for this + /// (`split_gives_every_pane_its_own_minimap_over_its_own_buffer` et al. + /// in `render.rs`) only asserted on `ScreenLayout.minimap.len()` — a + /// struct field populated by the pure layout function, never proven to + /// reach a real frame. This drives the same fixture through the real + /// `driver_with_shell` → `render_content` → `draw_minimap_strip` path + /// and reads the painted braille back. + /// + /// RED against the pre-#722 code (single `Option` + /// gated on `active_window_id`): only one half of the screen would ever + /// paint braille — confirmed by hand by reverting `build_screen_layout` + /// to `.find(|(id, _)| *id == active_window_id)` before restoring this + /// fix. + #[test] + fn split_paints_two_independent_minimap_strips_via_shell_app() { + let driver = driver_with_shell(app_with_split_shaped_buffer(), config(), 160, 30); + let screen = driver.screen(); + + // Both panes share the same fixture buffer, so a row that paints + // buffer text paints the literal `"line "` prefix twice — once per + // pane. Splitting the row at the second occurrence's start column + // (rather than at a hardcoded fraction of the screen width) locates + // each pane's own column span regardless of sidebar width, gutter + // width or where exactly the window divider/scrollbar glyphs land. + let mut left_hit = false; + let mut right_hit = false; + for row in 0..30usize { + let Some(line) = screen.lines().nth(row) else { + continue; + }; + let starts: Vec = line.match_indices("line ").map(|(i, _)| i).collect(); + let Some(&second) = starts.get(1) else { + continue; // row doesn't paint both panes' buffer text + }; + let (left, right) = line.split_at(second); + left_hit |= has_braille(left); + right_hit |= has_braille(right); + } + assert!( + left_hit && right_hit, + "a `:vsplit` must paint minimap braille in both the left pane \ + and the right pane, not just one; screen:\n{screen}" + ); + } + + /// #722 acceptance, painted-output tier: switching focus between panes + /// of a `:vsplit` must not move either pane's text — coverage for the + /// "migrates on focus change, reflowing both panes" symptom the issue + /// called out as *worse* than the missing strip (the width reclaim was + /// gated on the same `is_active` flag as the strip itself, so both + /// panes reflowed on every focus change). + /// + /// Drives a live `w` — the standard two-keystroke vim window-cycle + /// chord (`Engine::handle_key`'s `pending_key = Some('\x17')` arm, + /// consumed by the plain `w` that follows) — through the real + /// `driver_with_shell` key path, and diffs two real painted frames: the + /// right pane's own `"line "` text column (both panes share the fixture + /// buffer, so this is exactly the column its own gutter/minimap layout + /// puts it at) must land in the same place whichever pane is focused. + /// + /// A test that dispatches `w` but never confirms it actually moved + /// focus would pass vacuously if the chord silently no-ops (identical + /// before/after frames trivially satisfy "column unchanged"), so this + /// first pins that focus really moved by locating the block cursor's + /// own painted cell — quadraui's TUI editor rasteriser paints + /// `CursorShape::Block` (Normal mode's shape, the fixture's default) as + /// a cell background recolour (`theme.cursor`), not ratatui's real + /// terminal cursor, and only in the *active* window + /// (`build_screen_layout`'s `is_active`-gated `cursor` field — see + /// `render.rs`) — so that cell must relocate to the other pane. + /// + /// RED against the pre-#722 code: focusing the right pane would widen + /// it (reclaiming the now-inactive left pane's minimap width) and + /// narrow the left pane by the same amount, moving the right pane's + /// `"line "` column — confirmed by hand by reverting the `minimap_w` + /// reclaim to the old `is_active`-gated single value before restoring + /// this fix. + #[test] + fn focus_change_does_not_move_either_panes_text_via_shell_app() { + const WIDTH: u16 = 160; + const HEIGHT: u16 = 30; + + /// Column of the *second* `"line "` occurrence on the first row + /// that paints both panes' buffer text — i.e. where the right + /// pane's own text starts. + fn right_pane_text_col(screen: &str) -> usize { + screen + .lines() + .find_map(|line| { + let mut hits = line.match_indices("line "); + hits.next()?; + hits.next().map(|(i, _)| i) + }) + .expect( + "a `:vsplit` of the shared fixture must paint a row with \ + both panes' \"line N\" text", + ) + } + + let app = app_with_split_shaped_buffer(); + // Read the fixture's own resolved theme before `app` moves into the + // driver, so the cursor-cell scan below matches whatever colour + // this fixture actually paints with, ambient colorscheme setting + // included, rather than a hardcoded theme that could silently + // drift from it. + let theme = Theme::from_name(&app.engine.settings.colorscheme); + let cursor_bg = quadraui::tui::ratatui_color(super::quadraui_tui::q_theme(&theme).cursor); + + let mut driver = driver_with_shell(app, config(), WIDTH, HEIGHT); + // Warm-up dispatch: the runner's own `AppShell` only picks up the + // engine's pinned sidebar/autohide state (see + // `app_with_split_shaped_buffer`'s doc comment) at the tail of a + // `handle()` call, never on the construction-time first frame. An + // inert `Escape` forces that sync to happen *before* `before` is + // captured, so the real `w` dispatch below isn't the one that + // (spuriously) changes the sidebar's painted state. + driver.press_named(quadraui::NamedKey::Escape); + + fn cursor_cell( + driver: &quadraui::tui::testing::TuiDriver, + cursor_bg: quadraui::tui::testing::Color, + ) -> Option<(u16, u16)> { + for y in 0..HEIGHT { + for x in 0..WIDTH { + if driver.style_at(x, y).map(|s| s.bg) == Some(cursor_bg) { + return Some((x, y)); + } + } + } + None + } + + let cell_before = cursor_cell(&driver, cursor_bg) + .expect("the active pane must paint a block cursor cell"); + let before = driver.screen(); + let col_before = right_pane_text_col(&before); + + driver.ctrl_char('w'); + driver.type_char('w'); + driver.render(); + + let cell_after = cursor_cell(&driver, cursor_bg) + .expect("the newly-active pane must paint a block cursor cell"); + assert_ne!( + cell_before, cell_after, + "test setup sanity: `w` must actually move focus (and so \ + the painted block-cursor cell) to the other pane, or this \ + test isn't exercising a focus change at all" + ); + + let after = driver.screen(); + let col_after = right_pane_text_col(&after); + + assert_eq!( + col_before, col_after, + "cycling focus between panes of a `:vsplit` must not move \ + either pane's text (i.e. must not reflow either pane's text \ + width); before:\n{before}\nafter:\n{after}" + ); + } + /// #35: `render_content` must paint the minimap through the shell path, /// as braille — not just populate `ScreenLayout.minimap`. /// diff --git a/src/tui_main/snapshots/command_line.snap b/src/tui_main/snapshots/command_line.snap index 731e3531..88f1c230 100644 --- a/src/tui_main/snapshots/command_line.snap +++ b/src/tui_main/snapshots/command_line.snap @@ -4,7 +4,7 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × 󰤲  ⋯ ▎ -  buffer content ⠉⠉⠉⠉⠉⠁⠉⠉⠉⠉⠉⠉ +  buffer content ⠉⠉⠉⠁⠉⠉⠉⠉    @@ -12,5 +12,5 @@ expression: "lines.join(\"\\n\")" - COMMAND [No Name] Spaces: 4 Ln 1, Col 1 + COMMAND [No Name] LF Spaces: 4 Ln 1, Col 1  :set diff --git a/src/tui_main/snapshots/insert_mode.snap b/src/tui_main/snapshots/insert_mode.snap index c3555025..47428768 100644 --- a/src/tui_main/snapshots/insert_mode.snap +++ b/src/tui_main/snapshots/insert_mode.snap @@ -4,7 +4,7 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × 󰤲  ⋯ ▎ -  hello world ⠉⠉⠉⠉⠉⠁⠈⠉⠉⠉⠉⠉ +  hello world ⠉⠉⠉⠉⠉⠉⠉⠉    @@ -12,5 +12,5 @@ expression: "lines.join(\"\\n\")" - INSERT [No Name] Spaces: 4 Ln 1, Col 1 + INSERT [No Name] LF Spaces: 4 Ln 1, Col 1  diff --git a/src/tui_main/snapshots/line_numbers.snap b/src/tui_main/snapshots/line_numbers.snap index 15a41244..8d4d2068 100644 --- a/src/tui_main/snapshots/line_numbers.snap +++ b/src/tui_main/snapshots/line_numbers.snap @@ -4,13 +4,13 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × 󰤲  ⋯ ▎ -  1 alpha ⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿ +  1 alpha ⣿⣿⣿⣿⣿⣿⣿⣿  2 beta  3 gamma  4 delta -  5 epsilon ⠉⠉⠉⠉⠉⠉⠉⠉⠉⠉⠉⠉ +  5 epsilon ⠉⠉⠉⠉⠉⠉⠉⠉ - NORMAL [No Name] Spaces: 4 Ln 1, Col 1 + NORMAL [No Name] LF Spaces: 4 Ln 1, Col 1  diff --git a/src/tui_main/snapshots/minimap_braille.snap b/src/tui_main/snapshots/minimap_braille.snap index 8c180f64..b17c0451 100644 --- a/src/tui_main/snapshots/minimap_braille.snap +++ b/src/tui_main/snapshots/minimap_braille.snap @@ -4,17 +4,17 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × 󰤲  ⋯ ▎ -  line 0 █⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⣿⣿ -  line 1 █⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿ -  line 2 ░⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿ -  line 3 ░⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿ -  line 4 ░⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⣿ - line 5 ░⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⣿ - line 6 ░⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⣿ - line 7 ░⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⣿ - line 8 ░⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿ - line 9 ░⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿ - line 10 ░⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿⣿ - line 11 ░⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿⣿ - NORMAL [No Name] 󰆍 󰘖 utf-8 LF Spaces: 4 Ln 1, Col 1 +  line 0 █ ⣿⣿⣿⣿⣿⣿⣿⣿⣿⡇⠀⢸⣿⣿ +  line 1 █ ⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⣿⣿⣿⣿ +  line 2 ░ ⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⣿⣿⣿⣿ +  line 3 ░ ⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⣿⣿⣿⣿ +  line 4 ░ ⠀⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⢸⣿ + line 5 ░ ⠀⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⢸⣿ + line 6 ░ ⠀⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⢸⣿ + line 7 ░ ⠀⠀⠀⠀⠀⠀⠀⠀⢸⣿⣿⣿⢸⣿ + line 8 ░ ⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⣿⣿⣿⣿ + line 9 ░ ⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⣿⣿⣿⣿ + line 10 ░ ⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿⣿⣿ + line 11 ░ ⣿⣿⣿⣿⣿⣿⣿⠀⢸⣿⣿⣿⣿⣿ + NORMAL [No Name] 󰆍 󰘖 utf-8 LF Spaces: 4 Ln 1, Col 1  diff --git a/src/tui_main/snapshots/normal_mode.snap b/src/tui_main/snapshots/normal_mode.snap index 03bc8b9a..071b1ca3 100644 --- a/src/tui_main/snapshots/normal_mode.snap +++ b/src/tui_main/snapshots/normal_mode.snap @@ -4,7 +4,7 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × 󰤲  ⋯ ▎ -  -fn main() { ⠭⠭⠷⠿⠿⠿⠿⠿⠿⠿⠾⠿ +  -fn main() { ⠭⠷⠿⠿⠿⠿⠿⠿  │ println!("hello");  }  @@ -12,5 +12,5 @@ expression: "lines.join(\"\\n\")" - NORMAL [No Name] Spaces: 4 Ln 1, Col 1 + NORMAL [No Name] LF Spaces: 4 Ln 1, Col 1  diff --git a/src/tui_main/snapshots/split_panes.snap b/src/tui_main/snapshots/split_panes.snap index 34fdecb6..db396334 100644 --- a/src/tui_main/snapshots/split_panes.snap +++ b/src/tui_main/snapshots/split_panes.snap @@ -4,17 +4,17 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × ⋯  [No Name] × 󰤲  ⋯ ▎ │ -  left pane content │ left pane content -  │ -  │ -  │ -  │ - │ - │ - │ - │ - │ - │ - │ - [No Name] Ln 1, Col 1│ NORMAL [No Name] Ln 1, Col 1 +  left pane content ⠉⠉⠉⠉⠉⠉│left pane content ⠉⠉⠉⠉⠉⠉ +  │ +  │ +  │ +  │ + │ + │ + │ + │ + │ + │ + │ + [No Name] Ln 1, Col 1 │NORMAL [No Name] Ln 1, Col 1  Editor split diff --git a/src/tui_main/snapshots/visual_selection.snap b/src/tui_main/snapshots/visual_selection.snap index 412135e2..15cf5fd6 100644 --- a/src/tui_main/snapshots/visual_selection.snap +++ b/src/tui_main/snapshots/visual_selection.snap @@ -4,7 +4,7 @@ expression: "lines.join(\"\\n\")" --- 󰍜  [No Name] × 󰤲  ⋯ ▎ -  select this text ⠛⠛⠛⠉⠓⠛⠛⠛⠁⠛⠛⠛ +  select this text ⠛⠛⠙⠚⠛⠃⠛⠛  and this too   @@ -12,5 +12,5 @@ expression: "lines.join(\"\\n\")" - VISUAL [No Name] Spaces: 4 Ln 1, Col 11 + VISUAL [No Name] LF Spaces: 4 Ln 1, Col 11 