Repository navigation
scrollbar: Let the vertical scrollbar sit on the left side - #3416
Conversation
eb4366c to
afe8e70
Compare
afe8e70 to
0338bdc
Compare
huacnlee
left a comment
There was a problem hiding this comment.
This PR adds left-side vertical scrollbars for mirrored diff panes, while preserving the existing right-side default. The requirement is reasonable, and the geometry changes appear consistent with that purpose.
Before merging, please reconsider the public placement API so it accounts for horizontal scrollbars as well. Scrollbar::side(Side) and the editor's scrollbar_side currently expose a general scrollbar setting that only describes the vertical axis. We should establish a coherent placement contract for both axes before publishing this API.
One option to consider is a single placement value representing the complete arrangement:
Scrollbar::new(&handle)
.placement(ScrollbarPlacement::BottomLeft);
EditorState::new(window, cx)
.scrollbar_placement(ScrollbarPlacement::BottomLeft);For example:
BottomRight: vertical on the right, horizontal at the bottom (the existing default).BottomLeft: vertical on the left, horizontal at the bottom.TopRight: vertical on the right, horizontal at the top.TopLeft: vertical on the left, horizontal at the top.
The editor's runtime setter would replace the complete placement with one call as well. For a single-axis scrollbar, only the corresponding part of the placement would apply.
This is a suggested design, not a requirement to use these exact names or this exact enum. Please use it as a reference, or propose a clearer alternative that covers both axes with one configuration method. Avoid requiring two axis-specific methods or making repeated calls to the same setter implicitly accumulate different axis settings.
The resulting implementation should consistently handle track placement, corner avoidance, thumb and hitbox geometry, and entrance direction for both axes. Please also keep the default behavior unchanged and update both documentation locales.
Validation: reviewed the diff at 0338bdc73 and the repository guides; git diff --check passed. Tests and real-window validation were not run. This request concerns the public API design, not a demonstrated regression in the current left-side geometry.
Replace `Scrollbar::side(Side)` with `Scrollbar::placement(ScrollbarPlacement)`, which places both axes at once: `BottomRight` (the default), `BottomLeft`, `TopRight` and `TopLeft`. A horizontal scrollbar at the top gets its track, thumb and hitbox on the top edge and slides in from the top; when both scrollbars show, the horizontal track gives way at the vertical scrollbar's end. The editor's `scrollbar_side` and `set_scrollbar_side` become `scrollbar_placement` and `set_scrollbar_placement`.
|
I added the Story changes locally: a single test area with an Options menu for switching Dataset and Placement, instead of displaying all four placements at once. Please cherry-pick commit d8057f92 from the git fetch https://github.com/longbridge/gpui-kit.git pr-3416-story-options
git cherry-pick d8057f92For future PRs, please use a fork under your personal GitHub account and enable “Allow edits from maintainers.” This PR uses an organization-owned fork, and both SSH and HTTPS pushes to your branch were denied with permission errors despite Being able to make small adjustments directly would reduce back-and-forth and help us move the PR forward faster. |
|
Thanks — took |
## Description Builds on #3416 (`ScrollbarPlacement`, merged into `next`) and #3359's gutter markers. In a side-by-side diff, both gutters can face the center, so the line numbers of corresponding lines sit next to each other across the divider and can be compared directly. The editor's gutter is always on the left of the text today, so the left pane cannot do that. This adds `gutter_side(Side)` / `set_gutter_side` to the editor state for the left pane of such a diff, default `Side::Left` (unchanged). On the right the gutter is mirrored: the same columns, in the same order counted from the text, with the line numbers aligned toward it. The text and gutter x offsets are computed once per layout and used for painting, hit testing, IME bounds and scroll-into-view. A scrollbar on the gutter's side stays outermost. With the gutter on the right and the scrollbar on the left — the left pane's outer edge — the text keeps clear of the scrollbar's whole track: its effective width, the active thumb included, less the editor's left padding, at least the usual 10 px margin; that one reservation sets the text origin, width, wrap width and scroll size. Line numbers are placed by their shaped width rather than padded with spaces: in a proportional font a space is about a third of a digit, so the default left gutter left right-aligned numbers ragged and 6–11 px short of the text; they now line up against it on either side. `Editor` puts its narrow padding on the gutter's side. `gutter_order([GutterColumn])` orders the gutter's columns from the text outward, on either side: the default is `[FoldIcons, LineNumbers, Markers]` (unchanged); `[FoldIcons, Markers, LineNumbers]` puts a diff's change markers between the text and the line numbers, as IntelliJ's diff does. A column left out follows the listed ones in its default order. The **Editor Diff** story (`70b271ad`, @huacnlee) shows original and modified source with change markers; its Options menu switches between both gutters on the left and center-facing gutters, and optionally puts the markers before the line numbers. The two panes scroll together, by wheel and by dragging either scrollbar. The ordinary Editor story is unchanged. ## Screenshot | Before | After | | ------ | ----- | | <img width="800" alt="scrollbar-left" src="https://raw.githubusercontent.com/GigLaboCom/gpui-component/pr-assets/screenshots/scrollbar-left.png" /> | <img width="800" alt="mirrored" src="https://raw.githubusercontent.com/GigLaboCom/gpui-component/pr-assets/screenshots/mirrored.png" /> | A side-by-side diff with changed lines marked; the left pane uses `.scrollbar_placement(ScrollbarPlacement::BottomLeft).gutter_side(Side::Right)`, mirrored: | Both gutters on the left | Mirrored | | ------------------------ | -------- | | <img width="800" alt="both gutters on the left" src="https://raw.githubusercontent.com/GigLaboCom/gpui-component/pr-assets/screenshots/gutter-before.png" /> | <img width="800" alt="mirrored" src="https://raw.githubusercontent.com/GigLaboCom/gpui-component/pr-assets/screenshots/gutter-mirrored.png" /> | The columns in IntelliJ's order, `.gutter_order([GutterColumn::FoldIcons, GutterColumn::Markers, GutterColumn::LineNumbers])` on both panes — without and with folding (an empty fold column here): | IntelliJ's order | With folding | | ---------------- | ------------ | | <img width="800" alt="IntelliJ's order" src="https://raw.githubusercontent.com/GigLaboCom/gpui-component/pr-assets/screenshots/gutter-intellij.png" /> | <img width="800" alt="IntelliJ's order with folding" src="https://raw.githubusercontent.com/GigLaboCom/gpui-component/pr-assets/screenshots/gutter-intellij-fold.png" /> | ## Public API `gpui_base` (re-exported by `gpui_component`): - `InputBaseState::gutter_side(mut self, side: Side) -> Self` — builder: which side of the text the gutter is drawn on; default `Side::Left`, `Side::Right` for the left pane of a side-by-side diff. - `InputBaseState::set_gutter_side(&mut self, side: Side, cx: &mut Context<Self>)` — the same, at runtime. - `InputPresentation::gutter_side(&self) -> Side` — the side the editor's gutter sits on. - `GutterColumn { FoldIcons, LineNumbers, Markers }` — a column of the gutter. - `InputBaseState::gutter_order(mut self, columns: impl IntoIterator<Item = GutterColumn>) -> Self` / `set_gutter_order(&mut self, columns, cx: &mut Context<Self>)` — the columns from the text outward; default `[FoldIcons, LineNumbers, Markers]`; a column left out follows in its default order, a repeat is ignored. ## How to Test - `cargo test -p gpui-base --lib`: new tests for the text and gutter origins with an unchanged wrap width, the mirrored fold icons and columns, the scrollbar staying outermost (left/left, right/left, right/right), clicks in the text and in the gutter on both sides, caret / IME / `range_to_bounds` / selection-path x including a long line scrolled clear of the gutter, a right-side case in the existing gutter-bounds test, every gutter column — markers included — mirrored to 0.01 px for all six orders with folding on and off, the order normalised and counted from the text on both sides, clicks on the fold icon and the line number in every side/order combination, `Editor` keeping its gutter-side padding, and the text clear of the whole track of a left scrollbar with zero padding and with a wider themed track. Each fails with its code reverted. - `cargo test -p gpui-component-story --lib editor_diff`: layout switching keeps both sources, and the panes stay aligned on wheel input and while either scrollbar is dragged. - `cargo run -p gpui-component-story -- 'Editor Diff'`, then Options. ## Checklist - [x] I have read the [CONTRIBUTING](../CONTRIBUTING.md) document and followed the guidelines. - [x] Reviewed the changes in this PR and confirmed AI generated code (If any) is accurate. - [x] Passed `cargo run` for story tests related to the changes. - [x] Tested macOS, Windows and Linux platforms performance (if the change is platform-specific) --------- Co-authored-by: Jason Lee <[email protected]>
Description
The vertical scrollbar is hard-coded to the right edge and the horizontal one to the bottom. In a side-by-side diff the left pane's scrollbar belongs on its outer (left) edge, so the two panes mirror each other.
This adds
ScrollbarPlacement, one value for both axes, set with one call —Scrollbar::placement, andscrollbar_placement/set_scrollbar_placementon the editor state:BottomRight(default, unchanged)BottomLeftTopRightTopLeftEach axis follows its half of the placement: the track sits on that edge, the thumb and its hit area are anchored to it, and the bar slides in from that edge. When both bars show, the vertical one keeps its full height and the horizontal one stops short of it — at its start when the vertical bar is on the left, at its end when it is on the right. A single-axis scrollbar uses only its own half. The editor reserves no room for its horizontal bar at the bottom today, so at the top it overlays the first line the same way. Nothing changes by default.
Targets
next: it is the first half of a mirrored side-by-side diff whose second half, #3417, builds on #3359's gutter markers, which are onnext.Screenshot
The four placements (
ScrollbarMode::Always); the story switches Dataset and Placement from one Options menu (d8057f92):Public API
gpui_base(re-exported bygpui_component, also fromgpui_component::scroll):ScrollbarPlacement { BottomRight, BottomLeft, TopRight, TopLeft }— defaultBottomRight;is_left()(vertical bar on the left),is_top()(horizontal bar at the top).Scrollbar::placement(mut self, placement: ScrollbarPlacement) -> Self.InputBaseState::scrollbar_placement(mut self, placement: ScrollbarPlacement) -> Self— builder for the editor's scrollbars.InputBaseState::set_scrollbar_placement(&mut self, placement: ScrollbarPlacement, cx: &mut Context<Self>)— replaces the whole placement at runtime.How to Test
cargo test -p gpui-base --lib scrollbar: theScrollbarharness clicks and drags on each placement — the vertical track and thumb on the left, the horizontal track and thumb at the top, the horizontal track starting after a left bar and ending before a right one with both bars at the top, a thumb drag and a track click moving the offset the right way — andvisibility_translation_moves_toward_the_nearest_edgecovers all four placements on both axes; an editor layout test forTopLeft. Each fails with its code reverted.cargo run -p gpui-component-story -- scrollbar, then Options → Placement.Checklist
cargo runfor story tests related to the changes.