Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 111 additions & 7 deletions src/gtk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,39 @@ enum Msg {
},
}

/// Left edge (drawing-area px, as a GTK `margin-start`) of a window's native
/// vertical `gtk4::Scrollbar`.
///
/// #723: `minimap_reserved_width` carves the minimap strip out of the pane's
/// **right** edge, but native `gtk4::Scrollbar` widgets live in the `Overlay`
/// *above* the `DrawingArea` — so a scrollbar pinned to `rect`'s outer edge
/// lands on top of the strip, where it is both indistinguishable from the
/// minimap and (being an opaque widget) hides the strip content underneath.
/// That is the "no scrollbar when the minimap is on" the issue reports.
///
/// Insetting by `minimap_width` puts the scrollbar immediately to the *left*
/// of the strip — exactly where TUI's editor-internal scrollbar column
/// already sits (quadraui's `tui::draw_editor` paints it at the editor
/// viewport's right edge, and that viewport is already narrowed by the same
/// reserved width). Both backends therefore land on one scroll affordance in
/// the same relative place, which is why `draw_minimap_strip` deliberately
/// does not paint a second one over the strip.
///
/// The trailing 2px keeps the bar off the group divider / adjacent group,
/// preserved from the pre-#723 positioning. `minimap_width` is `0.0` when the
/// strip is off (`:set nominimap`, or a pane too narrow to afford one), which
/// reduces this to exactly that original expression. Clamped to `rect_x` so a
/// pathologically narrow pane can never push the widget left of its own pane.
fn native_scrollbar_margin_start(
rect_x: f64,
rect_width: f64,
scrollbar_width: f64,
minimap_width: f64,
) -> i32 {
let x = rect_x + rect_width - minimap_width - scrollbar_width - 2.0;
x.max(rect_x).round() as i32
}

/// Reposition existing scrollbar widgets for the given drawing-area size.
///
/// This is a free function so it can be called both from `sync_scrollbar` (via
Expand All @@ -1427,7 +1460,7 @@ fn sync_scrollbar_positions(
da_width: f64,
da_height: f64,
line_height: f64,
_char_width: f64,
char_width: f64,
engine: &core::Engine,
scrollbars: &HashMap<core::WindowId, WindowScrollbars>,
) {
Expand Down Expand Up @@ -1474,13 +1507,16 @@ fn sync_scrollbar_positions(
// — Vertical scrollbar —
// Query the actual allocated width so we position correctly even if
// GTK's theme enforces a minimum wider than our CSS min-width.
// Inset 2px from the right edge so the scrollbar doesn't visually
// overlap the group divider or the adjacent group's space.
let sb_actual_w = ws.vertical.width().max(4) as f64;
let minimap_w = render::minimap_reserved_width(engine, rect.width, char_width);
ws.vertical.set_halign(gtk4::Align::Start);
ws.vertical.set_valign(gtk4::Align::Start);
ws.vertical
.set_margin_start(rect.x as i32 + (rect.width - sb_actual_w) as i32 - 2);
ws.vertical.set_margin_start(native_scrollbar_margin_start(
rect.x,
rect.width,
sb_actual_w,
minimap_w,
));
ws.vertical.set_margin_top(rect.y as i32);
ws.vertical
.set_height_request((rect.height as i32 - 4).max(0));
Expand Down Expand Up @@ -2681,6 +2717,13 @@ impl App {
}
}

// #723: the minimap strip is reserved out of each pane's right edge,
// so the native scrollbar has to be inset past it — see
// `native_scrollbar_margin_start`. Same per-window call
// `build_screen_layout_with_breadcrumb_row` reserves/paints the strip
// with, so the inset can't drift from what actually painted.
let char_width = self.cached_char_width;

// Hide scrollbars for windows that exist but aren't visible
// (e.g. windows in non-active tabs), or when a modal popup is
// open. Native gtk4::Scrollbar widgets render above the
Expand Down Expand Up @@ -2729,7 +2772,10 @@ impl App {
ws.vertical.set_halign(gtk4::Align::Start);
ws.vertical.set_valign(gtk4::Align::Start);

let scrollbar_x = rect.x as i32 + (rect.width - 10.0) as i32;
// Inset past the minimap strip (#723) — see
// `native_scrollbar_margin_start`.
let minimap_w = render::minimap_reserved_width(&engine, rect.width, char_width);
let scrollbar_x = native_scrollbar_margin_start(rect.x, rect.width, 10.0, minimap_w);
ws.vertical.set_margin_start(scrollbar_x);
ws.vertical.set_margin_top(rect.y as i32);
ws.vertical
Expand Down Expand Up @@ -2757,7 +2803,10 @@ impl App {
let indicator_y = rect.y + (ratio * scrollbar_height);

let sb_w = ws.vertical.width().max(4) as f64;
let indicator_x = rect.x as i32 + (rect.width - sb_w) as i32;
// Track the scrollbar's own inset so the cursor tick stays
// on the bar rather than under the minimap strip (#723).
let indicator_x =
native_scrollbar_margin_start(rect.x, rect.width, sb_w, minimap_w);
ws.cursor_indicator.set_margin_start(indicator_x);
ws.cursor_indicator.set_margin_top(indicator_y as i32);

Expand Down Expand Up @@ -10538,6 +10587,61 @@ fn build_shell_config(app: &App) -> quadraui::ShellConfig {
.with_activity_bar_width_px(48.0)
}

#[cfg(test)]
mod native_scrollbar_placement_tests {
//! #723: GTK's native `gtk4::Scrollbar` widgets live in the `Overlay`
//! *above* the `DrawingArea`, so a scrollbar pinned to the pane's outer
//! edge lands on top of the minimap strip that
//! `render::minimap_reserved_width` reserves out of that same edge —
//! the "no scrollbar when the minimap is on" symptom.
//!
//! Widget visibility/geometry itself is out of reach headlessly
//! (`App::new_headless` never constructs a real `gtk4::Scrollbar`, and
//! `GtkDriver` only sees Cairo paint, not overlay widgets), so the
//! geometry decision is factored into the pure
//! `native_scrollbar_margin_start` and pinned here. The on-screen
//! result is a SMOKE_TESTS item.
use super::native_scrollbar_margin_start;

/// Strip off (`:set nominimap`, or a pane too narrow to afford one):
/// unchanged from the pre-#723 expression — right edge, less the
/// scrollbar's own width, less the 2px divider gap.
#[test]
fn no_minimap_keeps_the_scrollbar_on_the_panes_right_edge() {
assert_eq!(native_scrollbar_margin_start(0.0, 800.0, 12.0, 0.0), 786);
assert_eq!(native_scrollbar_margin_start(400.0, 400.0, 12.0, 0.0), 786);
}

/// RED against `develop`: the pre-fix expression ignored the strip and
/// returned 786 here too — 48px *inside* the strip, i.e. the scrollbar
/// painted over the minimap instead of beside it.
#[test]
fn minimap_pushes_the_scrollbar_left_of_the_strip() {
let x = native_scrollbar_margin_start(0.0, 800.0, 12.0, 48.0);
assert_eq!(x, 738, "must be inset by the strip's full 48px width");
assert!(
x + 12 <= 800 - 48,
"the scrollbar's right edge ({}) must not reach into the strip, \
which starts at 752",
x + 12
);
}

/// Second pane of a `:vsplit` — the inset is relative to that pane's own
/// origin, not the drawing area's.
#[test]
fn split_pane_inset_is_relative_to_the_panes_own_origin() {
assert_eq!(native_scrollbar_margin_start(400.0, 400.0, 12.0, 48.0), 738);
}

/// A pane narrower than strip + scrollbar can't push the widget out of
/// its own pane.
#[test]
fn degenerate_pane_clamps_to_the_pane_origin() {
assert_eq!(native_scrollbar_margin_start(100.0, 30.0, 12.0, 48.0), 100);
}
}

#[cfg(test)]
mod shell_config_identity_tests {
//! #719: quadraui#656/#657 landed `ShellConfig::with_app_id()` /
Expand Down
14 changes: 14 additions & 0 deletions src/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4604,6 +4604,20 @@ pub fn build_minimap_data(
/// `Backend::draw_minimap`, so each backend's wiring is a single call to this
/// function. Nothing about sampling, scaling, dot packing or colour
/// aggregation exists on either side of it in vimcode.
///
/// #723: the strip carries its own scroll affordance — `Minimap::layout`
/// resolves a `viewport_highlight` band that *both* quadraui rasterisers
/// already paint (a background accent across the visible rows on TUI, a
/// translucent slider on GTK). `MinimapLayout.scrollbar` is deliberately
/// **not** painted on top of it: TUI's `draw_editor` already paints a solid
/// one-column vertical scrollbar in the column immediately left of the
/// strip, so an extra solid bar in the strip's own first column reads as
/// two bars jammed together — the exact regression
/// `test_tui_two_groups_single_boundary_scrollbar_481` guards. The pane's
/// scrollbar instead stays *beside* the strip on both backends; GTK's
/// native widget is inset past the strip by `native_scrollbar_margin_start`
/// in `src/gtk/mod.rs`, which reads the strip width from
/// [`minimap_reserved_width`] — the same call that reserved it here.
pub fn draw_minimap_strip(
backend: &mut dyn quadraui::Backend,
screen: &ScreenLayout,
Expand Down
116 changes: 116 additions & 0 deletions src/tui_main/shell_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8296,4 +8296,120 @@ mod tests {
240-line file, landed on line {top} ({frac:.3}); screen:\n{after}"
);
}

/// `app_with_shaped_buffer` with the sidebar forced off (mirrors
/// `app_with_split_shaped_buffer`'s own doc comment on why: sidebar
/// visibility is otherwise ambient, read off the developer's real
/// `~/.config/vimcode`). The two tests below key off the *column* a
/// scrollbar glyph paints at, and the explorer tree view paints its
/// own `'█'`/`'░'` scrollbar too — indistinguishable by character from
/// the editor's/minimap's — so it must be off, not just "usually
/// absent", for those tests to reliably find the right column.
fn app_with_shaped_buffer_no_sidebar() -> 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
}

/// Buffer short enough to fit entirely within the viewport — the other
/// half of #723's acceptance: nothing overflows, so no scrollbar (and no
/// minimap thumb) should paint anywhere. Sidebar forced off for the same
/// reason as `app_with_shaped_buffer_no_sidebar`.
fn app_with_short_buffer() -> TuiShellApp {
let mut app = TuiShellApp::new(None);
app.engine.settings.autohide_panels = false;
app.engine.app_shell.hide_sidebar();
app.engine.session.explorer_visible = false;
let text: String = (0..5).map(|i| format!("line {i}\n")).collect();
app.engine.buffer_mut().insert(0, &text);
app
}

/// #723 acceptance (TUI half): with the minimap on and a file longer
/// than the viewport, the pane shows **exactly one** vertical scroll
/// affordance, and it sits *beside* the strip rather than on top of it —
/// one column of `'█'`/`'░'` (quadraui's `tui::draw_editor` scrollbar),
/// with the minimap's braille starting in the very next column.
///
/// This is the invariant the first attempt at #723 broke: painting
/// `MinimapLayout.scrollbar` over the strip via `Backend::draw_scrollbar`
/// put a second solid bar in the strip's leftmost column, directly
/// against the editor's own — two bars jammed together, which is exactly
/// the operator-visible defect
/// `render_impl::tests::test_tui_two_groups_single_boundary_scrollbar_481`
/// exists to prevent (it went from 2 scrollbar columns to 4). The strip's
/// own scroll feedback is quadraui's `viewport_highlight` band — a
/// *background* accent across the visible rows, painted by both
/// rasterisers — not a second foreground bar.
///
/// RED against the reverted state: with `draw_minimap_strip` calling
/// `draw_scrollbar`, the column right of the editor's scrollbar is a
/// second `'░'`/`'█'` instead of braille, and the "exactly one" count is
/// 2. Verified by hand by restoring that call.
#[test]
fn minimap_strip_does_not_double_the_scrollbar_via_shell_app() {
let mut driver = driver_with_shell(app_with_shaped_buffer_no_sidebar(), config(), 100, 24);
// Warm-up dispatch: the runner's own `AppShell` only picks up the
// engine's pinned sidebar/autohide state at the tail of a
// `handle()` call, never on the construction-time first frame
// (see `app_with_shaped_buffer_no_sidebar`'s doc comment).
driver.press_named(quadraui::NamedKey::Escape);
let screen = driver.screen();

fn is_scrollbar_glyph(c: char) -> bool {
c == '█' || c == '░'
}
// Braille block: what `tui::draw_minimap` packs its dot cells from.
fn is_braille(c: char) -> bool {
('\u{2800}'..='\u{28FF}').contains(&c)
}

let row = 15usize;
let line: Vec<char> = screen
.lines()
.nth(row)
.unwrap_or_else(|| panic!("row {row} must exist; screen:\n{screen}"))
.chars()
.collect();

let sb_cols: Vec<usize> = line
.iter()
.enumerate()
.filter(|(_, c)| is_scrollbar_glyph(**c))
.map(|(i, _)| i)
.collect();
assert_eq!(
sb_cols.len(),
1,
"a pane with the minimap on must show exactly one vertical \
scrollbar column on row {row}, got {sb_cols:?}; screen:\n{screen}"
);

let next = line.get(sb_cols[0] + 1).copied();
assert!(
next.is_some_and(is_braille),
"the minimap strip must begin in the column immediately right of \
the scrollbar ({}), painting braille rather than a second bar; \
got {next:?}; screen:\n{screen}",
sb_cols[0]
);
}

/// #723 acceptance, the other half: a file that fits entirely within the
/// viewport must paint no scroll affordance anywhere — neither the
/// editor's own scrollbar nor a thumb over the minimap strip.
#[test]
fn minimap_paints_no_scroll_thumb_when_the_file_fits_via_shell_app() {
let mut driver = driver_with_shell(app_with_short_buffer(), config(), 100, 24);
// Warm-up dispatch — see `minimap_paints_a_scroll_thumb_when_the_file_overflows_via_shell_app`.
driver.press_named(quadraui::NamedKey::Escape);
let screen = driver.screen();
assert!(
!screen.contains('█') && !screen.contains('░'),
"a file that fits entirely within the viewport must paint no \
scroll thumb/track glyphs anywhere; screen:\n{screen}"
);
}
}
Loading