feat(diff): fold files away, and let the tint carry the signal - #51
Merged
Conversation
The @@ line went with it. Both gutters already carry the numbers it repeated, so a hunk boundary is now an untexted muted band, between hunks and never before the first, where the file header already parts them. It yields no selection run and no text to a copy. Collapsing is applied where the rows are derived, not where they are painted: a collapsed file does not emit its body, so the element windows, paints and selects over a shorter list and needs to know only how to map a click to a row. The set of collapsed files is keyed by index into the patch rather than by display path, because a file with neither an old nor a new path renders as the empty string and two of them would collapse together; it is cleared whenever the detail changes, so an index never outlives what it indexes. Toggling clears the window selection for the reason set_detail does — gpui-base anchors an endpoint to a pixel offset, which after a collapse resolves onto rows the reader never dragged across, and Cmd-C would copy them. It keeps the scroll offset, pulling it up only as far as the shortened content allows, because the element chooses its rows from that offset one phase before the scrolling container gets to clamp it, and an unclamped one leaves it laying out nothing at all.
In the header's text the marker was part of a selection run, so Cmd-C over a header and Select All both emitted "▾ src/main.rs +3 −1" — against the rule the module doc states, that only code text becomes a run and everything in the gutter is painted directly so line numbers and markers cannot reach the clipboard. paint_gutter already returned early for a header row, which leaves that gutter free: the glyph is shaped and painted there, at the offset a line's +/− uses, and header_line goes back to what it was. A hunk carrying no lines put a separator directly under the file header, which is what "between hunks only" exists to prevent, and two of them stacked two bands. Only git's own output rules that out; the parser accepts @@ -0,0 +0,0 @@, so the rows skip an empty hunk rather than trusting it. Row::FileHeader names its file `index`, leaving `file` free for the &FilePatch it means everywhere else in these two modules, and the collapsed lookup is hashed once per file rather than twice. Rows::full answers the full-width row that file_at, cell_foreground and paint_background each used to match out for themselves, and Rows::marker_left names the offset the three marker sites share.
The two derivations share filled_hunks today, but they are separate functions and one could be inlined or specialised without the other noticing. The split view had only two empty hunks flanking a filled one, which never puts an empty hunk between two filled ones — the arrangement that would produce a doubled separator. It passes as written.
…ge bar The header row read as one line of text — path, status label, `+22 −0` — indented to the code column, with only the disclosure marker painted separately in the gutter. It now spans the row: chevron, status pastille and path flush left at the row's own left edge, and a green/red bar with the total in bold right-aligned at the other end. Only the path is a selection run. `header_line` reduces to the path, so `cell_strings` — the one source both the runs and the clipboard read — carries nothing else, and copying a header yields a bare path. The chevron, pastille, bar and count are painted the way `paint_gutter` already paints line numbers and markers, which is what keeps them out of every run. A bar's total width is its file's share of the most-changed file in the patch, and that maximum is derived in `content` alongside the rows and the strings rather than reached for per frame: sizing a bar from the element would mean walking every file of the patch on every repaint, and a selection drag repaints on each mouse move. Green is `theme.green` and red `theme.red`, the pair `line_colors` already tints a `+` and a `−` with, so a segment and a marker mean the same thing. The pastille is `theme.green` for an addition, `theme.red` for a deletion and `theme.blue` for a modification. Renamed, Copied and TypeChanged share `theme.muted_foreground`: none of the three is a statement about content — they describe the file's identity or its kind moving, and whatever content changed with it is already told by the bar beside them. The similarity percentage those two carried in the old header text is dropped; the `→` in the path still says a rename happened. Four degenerate cases decide the geometry. A patch of pure renames has no scale, so `max_changes` of zero draws no bar rather than dividing by it, and a file of no changes draws none either. A file with only additions gets no red segment and one with only deletions no green, each skipped rather than painted zero-wide. And a bar too short to see is widened twice over: the bar takes a floor of `BAR_MIN_WIDTH`, so one change beside a thousand is 4px rather than 0.064px, and a segment takes a floor of `BAR_MIN_SEGMENT` by moving the boundary rather than growing the bar, so the sum stays exactly proportional. `BAR_MIN_WIDTH` is written as twice `BAR_MIN_SEGMENT` because that is what makes the two clamps impossible to invert. The bar and count are right-aligned to the element's right edge, mirroring the left side being flush to its left edge, rather than pinned to the viewport. Both ends of the header therefore scroll with the content. Only the header's geometry moves: `Rows::cell_left` answers `HEADER_TEXT_LEFT` for a header and `code_left()` for every other row, so lines, separators and placeholders are untouched. Both copy paths go through the same `cell_bounds_at`, so the projection and the off-screen arithmetic still agree cell for cell. The click target is unchanged — `on_mouse_down` reads only `y`.
A header row was laid out in document space: flush to the element's left edge and right-aligned to the element's right edge. With no soft wrap, a Rust diff's content is routinely wider than the dock, so the bar and the count sat off-screen until the reader scrolled right. A statistic you have to go looking for is not a statistic. The mistake was treating the header as document. The code rows scroll horizontally because they are the thing being read; a header describes what is being read and belongs to the frame around it. The chevron, pastille and path now pin to the viewport's left edge and the bar and count to its right, inset by `Scrollbar::width()` — gpui-component's own 16px, taken from the constant rather than copied, so it breaks loudly if it moves instead of drifting into the overlay scrollbar. Only the background band still spans the element, which is what keeps a scrolled header reading as one continuous row rather than as a label floating over the code. The viewport is unmeasured on the first frame. `row_window` already answers that with `UNMEASURED_ROWS`, and this takes the same shape: `header_budget` returns `None`, the header falls back to the element's own edges, nothing is elided, and the second frame corrects it. One `Option` carries that state, so the pin, the elision and the cell bounds cannot disagree about whether the viewport is known. Pinning is what bounds the path, and the bound is one number per frame: the viewport less the furniture at both ends, computed in `request_layout` beside the visible range. A path wider than it is elided from the left — `…detail/diff/split.rs` — because the tail of a path names the file and the head is the part that can be lost. `elide_path` binary-searches the tails, so it shapes about six candidates rather than one per character. The elided string is the one that becomes the run. Every reader of a cell's text now goes through one method, `cell_string`, so the layout, the run, the select-all range and both copy paths cannot disagree — a projected byte range indexing a different string than the one it was measured against would slice a `String` off a character boundary and panic. The price is the other half of that trade: copying a header whose path is too long for the panel yields the elided form rather than the whole path. Highlight and clipboard staying equal is worth more than the full path here, and it is the invariant the module already existed to protect. The click target is unmoved. `on_mouse_down` reads only `event.position.y` against a hitbox that is the whole element, so the entire row width still toggles and still resolves through `file_at` to the same file. Nothing else about a row changed: `content_width` still measures the unelided strings, so a header cell is still laid out against a width that exceeds it and a row is still one line tall.
The row's background tint carries the addition/deletion signal on its own, which is the whole point of moving off the code editor: a full-width band does what a tint hugging the glyphs could not. Seen running, the marker read as leftover furniture between the line numbers and the code. So the column goes rather than being blanked. code_left() is the gutters alone now — 2 * GUTTER_WIDTH unified, GUTTER_WIDTH split — and every code cell gains sixteen pixels. The gutter numbers do not move: paint_gutter recovers the row's left edge as origin.x - code_left(), and both terms shrank by the same sixteen. LineColors::foreground had one caller, the marker's tint, so it is dead the moment the glyph goes and a dead field is a -D warnings failure. line_colors becomes line_background(origin, mode) -> Option<Hsla>; the theme argument went with the field, since only the foreground arm ever read it. The four tint constants and the two-bar contrast reasoning behind them are untouched. The disclosure chevron stays. It positions itself from HEADER_PADDING and DISCLOSURE_WIDTH, not from the marker geometry — verified before deleting anything — so nothing in the header moved. This reverses a Decisions bullet in the design spec, which kept the marker on the grounds that it carried the signal for anyone who reads green and red poorly. That argument was made against a mock-up and is outweighed rather than answered, so the bullet is rewritten rather than dropped, and it records the cheapest mitigation if it is revisited: a 2px coloured edge down the row, which puts shape back to work without putting a character back in the text.
copy_selection's visible branch took its range from the projection, which gpui-base computed against display[cell - visible.start] in request_layout, and then sliced texts[offset], which this function rebuilt in paint with a second cell_string call and its own Pen. The two agree today: path_budget is a field, so it is fixed within a frame, and the text style stack is the same in both phases. But that is two things happening to agree, not an invariant. The cell_string funnel prevents the strings being formatted differently; it cannot prevent them being measured differently, and the failure mode is copy_text slicing on a char boundary that the other string does not have — a panic, not a wrong character. So the visible branch reads its string out of display as well. One closure answers whether a cell is displayed, and both the text map and the range map consult it, which also makes the fallback symmetric: a cell display has no entry for falls through to cell_string in both maps together. Five findings from the same review, none load-bearing: header_budget and paint_header derived the same clearance from the same constants independently. count_right_edge and bar_right_edge now name that arithmetic, and a test pins the widest bar the painter can draw to exactly BAR_GAP right of where the budget stops the path. elide_path is careful about char boundaries and was only ever tested on ASCII. The sweep gains an accented path, and one case pins that the cut is a byte offset landing on a boundary rather than a char count. "That last number" in diff/mod.rs pointed at max_changes until c76c5a6 put the elision aside between them; it is named outright now. Reflowed the two paragraphs my previous commit left ragged. header_line was path.to_string(), called by row_text to build a string it already held. Dropped rather than kept: the seam carried no decision, and three of its four test assertions sat beside an identical assertion on the path itself.
LeadcodeDev
force-pushed
the
feat/collapsible-diff-files
branch
from
August 29, 2026 19:41
c96c3da to
ca9cd9e
Compare
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.
Stacked on #50 — it edits the row model and the custom
Elementthat PR introduces, so it cannot merge before it. Review #50 first.Four decisions were taken while building this, each after looking at the running view rather than at a mock-up.
The
@@line goes@@ -12,7 +12,9 @@ fn reloadsaid nothing the two gutters were not already saying.Row::HunkHeaderbecomesRow::Separator— a muted band with no text, so it produces no selection run and puts nothing on the clipboard. Emitted only between hunks; a file header already parts the first one.Files fold from their header row
Clicking a header toggles that file. The collapse is applied at row derivation, not at render:
rows(patch, collapsed)simply does not emit a collapsed file's body. TheElementtherefore knows nothing about folding — its visible-range windowing, its geometry and its selection all keep working on a shorter list, and none of the seven standing hazards is touched. The only element-side addition is a mouse-down hit test.State is ephemeral, cleared whenever the detail changes, everything expanded initially. Keyed by file index rather than display path, because
display_path()is anOptionand two pathless files would otherwise fold together.Selection is cleared on toggle: gpui-base anchors endpoints to pixel offsets, so a live selection would otherwise resolve onto rows the user never dragged across — and Cmd-C would copy them.
Scroll position is kept and clamped rather than zeroed. The element reads the offset in
request_layout, one phase before the scrolling div clamps it, so an over-scrolled offset after a fold would give an empty visible range and paint nothing. Reusing gpui's ownScrollHandle::max_offset()would have been wrong — it carries the previous frame's pre-collapse height.The header row, restyled and pinned
Chevron, status pastille and path flush left; a green+red change bar and a bold total right-aligned. The bar's total width is proportional to the file's changes measured against the most-changed file in the patch, so files compare at a glance. That scale is computed once at derivation, never per frame.
The row is pinned to the viewport and does not scroll horizontally at all. A file header is chrome, not content — and with no soft wrap a Rust diff is routinely wider than the dock, so the stats would otherwise sit off-screen until you scrolled right. An over-long path elides from the left: the tail identifies a file, the head is what you can lose.
Only the path is a selection run. Chevron, pastille, bar and count are painted directly, so copying a header yields a bare path.
The
+/−markers go, column and allThe background tint carries the signal.
code_left()shrinks by 16px and the code gains the width.This reverses a decision recorded in the design spec, which had kept the marker on the argument that it carries the signal for anyone who reads green and red poorly. That bullet is rewritten rather than deleted — it now records what was decided instead, that accessibility was outweighed rather than answered, and names the cheapest mitigation if it is ever revisited: a 2px coloured edge on the row, which makes shape carry information without putting a character back into the text.
The bug worth knowing about
Review found one that no test would have caught. In
copy_selection's visible branch, the selection range was measured by gpui-base against a string built inrequest_layout, then used to slice a string rebuilt inpaint. The two agreed in practice — the elision budget is a field, fixed within a frame — but&str[range]panics off a char boundary rather than degrading. Both the text and the range now come from the same array by construction, with a symmetric fallback so the rare branch cannot split either.Testing
414 tests,
cargo clippy --workspace --all-targets -- -D warningsandcargo fmt --all --checkclean. The bar geometry, the elision, the collapse derivation and the row/file hit mapping are pure and tested, including every degenerate case: a patch of pure renames (no division by zero), a file with only additions or only deletions, a one-change file beside a thousand-change one (a floor keeps its bar visible), and an elision cutting at a byte offset that is not the char index.Please look before merging
Nothing in these seven commits has been seen on screen. The earlier state was, three commits back; the restyle, the pin and the marker removal have not been.
The one item both the implementer and the reviewer singled out:
HEADER_TEXT_LEFT(36) and the split view'scode_left()(44) are now 8px apart. A near-miss alignment can read worse than either a match or a clear offset, and no test can judge it.Also unmeasured rather than reasoned: whether
DISCLOSURE_WIDTH = 12fits U+25BE in whatever fallback font serves it, and whetherCOUNT_WIDTH = 34fits four digits of the mono font. Both fail as crowding rather than overlap.Parked
path_budgetis one frame stale against the pin during a drag-resize — both copy paths read the same values, so nothing drifts; the effect is one frame of slightly-wrong elision.content_widthmeasures the unelided string. That is what keeps the frame-one layout provably wrap-free.→in the path and the pastille carry the signal.