From e6bab11ac3fc1adb89de2c97315261d3f498b01c Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:04:27 +0200 Subject: [PATCH 01/19] docs(diff): specify a GitHub-style diff view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported problem is the `+`/`-` at the head of every line. Investigation found a second cause behind it, and a larger one: `TextDecoration` paints a background from the first glyph of a range to the last, so the addition and deletion tints follow the ragged edge of the text instead of filling the row, and a blank added line gets no background at all. The markers carry the signal because the colour only half does. Both follow from the diff being one text document in a code editor rather than a list of elements, and that cannot be worked around: at the pinned gpui-component revision the gutter is fixed to `buffer_line + 1` in a single column, there is no full-width row background outside the cursor line, and there is no per-line element injection. Two of the three things the editor bought back in `5cc1caa` are recoverable — `virtual_list` for virtualisation, and `gpui-base`'s window-level selection, already mounted through `Root`, for selection. The spec records the evidence for each so the next person does not re-derive it. --- ...026-08-29-github-style-diff-view-design.md | 225 ++++++++++++++++++ 1 file changed, 225 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md new file mode 100644 index 0000000..788e59e --- /dev/null +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -0,0 +1,225 @@ +# GitHub-style diff view + +Date: 2026-08-29 +Status: approved, not yet implemented + +## The problem + +Reading a diff in gitr is hard. The reported cause is the `+`/`-` at the start of every line, +which shifts the code one column and clutters it. Investigation found a second cause the +report did not name, and it is the larger one. + +`TextDecoration` backgrounds hug the glyphs of a range rather than filling the row. gpui +paints a run's background as a quad from the first glyph to the last +(`gpui/src/text_system/line.rs:689-701`), one `line_height` tall and one glyph-advance +span wide. So the addition and deletion tints, which already carry GitHub's exact light +values, do not read as bands: they follow the ragged right edge of the text, and a blank +added or deleted line gets no background at all. The `+`/`-` markers are doing most of the +work of signalling a line's kind, because the colour only does half of it. + +Both causes are properties of the same decision: the diff is not a list of elements, it is +**one text document fed to a code editor** (`crates/ui/src/detail/diff.rs`). + +## Why the current architecture cannot deliver the target + +Three capabilities are absent from gpui-component 0.5.2 at the pinned revision +`7acfc184382d30864a688fdaa6c9ff719efc53ae`, and each one is independently fatal: + +- **Gutter content is fixed.** It is always `buffer_line + 1`, right-aligned, one column + (`crates/base/src/input/base/element.rs:2001`). There is no hook to substitute text and + no second column. A GitHub old/new pair is unreachable. +- **No full-width row background.** The only edge-to-edge row fill is the cursor's active + line, coloured from the theme (`element.rs:2107-2131`). Nothing is per-range or per-row. +- **No per-line element injection.** No inlay hints, line widgets, block decorations, or + row render hook anywhere in the input engine. + +The `+`/`-` are also load-bearing for the current colouring: `tree-sitter-diff` maps +additions to the theme's `string` scope and deletions to `keyword` by parsing those very +markers. Removing them from the text removes the foreground colouring with them. + +There is therefore no middle path that keeps the editor. The row rendering has to be built. + +## What that costs, and what it does not + +The module doc on `diff.rs` records three things bought by moving to the editor. Two are +recoverable and one was overstated: + +- **Virtualisation** — recoverable. `gpui_component::virtual_list` is public. The renderer + deleted at `5cc1caa` had no virtualisation at all, so this is an improvement over the + state we are returning to, not a regression. +- **Syntax highlighting** — recoverable and improvable. `gpui_component::highlighter::Highlighter` + is callable directly. Today the `diff` grammar colours by *role*; per file we can colour by + the file's actual language, which is what GitHub does. Deferred, see Out of scope. +- **Text selection** — not lost. `gpui-base` ships a window-level, cross-element document + selection system (`crates/base/src/text_selection.rs`), already mounted in this + application: `gpui_component::Root` renders `TextSelectionLayer` and binds copy + (`crates/ui/src/root.rs:551,592`), and `crates/gitr/src/main.rs:209` constructs that `Root`. + +The work is therefore not implementing selection. It is implementing a custom `Element` per +row type, because `register` and `update_runs` must be called from `prepaint` and `paint`. +A reference implementation exists at +`crates/base/examples/showcase/components/text_selection.rs` (359 lines). + +## Decisions + +**No soft wrap; horizontal scrolling instead.** This is what GitHub does, and it is what +makes virtualisation tractable: every row becomes exactly one line tall, so row heights are +uniform and `virtual_list` applies directly. With soft wrap, heights vary per row and per +viewport width, and the list has to measure before it can place. + +**The `+`/`-` marker stays, in a column of its own.** Sixteen pixels, between the gutters +and the code, as in the deleted renderer. It no longer shifts the code, it carries the +signal for anyone who reads the green and the red poorly, and being a separate element it +never reaches the clipboard. + +**The domain layer does not change.** `DiffLine` already stores `old_number`, `new_number`, +and a `content` with the marker stripped (`crates/domain/src/patch.rs:27`, parser at +`crates/vcs/src/process/patch_parser.rs:177-220`). Both line numbers are parsed today and +rendered nowhere. Nothing in `domain` or `vcs` needs touching. + +**`gpui-base` becomes a direct dependency**, declared without a `rev`, matching how +`gpui-component` is declared (`Cargo.toml:26`). It is already in `Cargo.lock` at the same +revision, so this unifies rather than adding a second copy — the failure mode `CLAUDE.md` +warns about does not apply here. + +## Module layout + +`crates/ui/src/detail/diff.rs` becomes a directory: + +| File | Owns | +|---|---| +| `diff/mod.rs` | Entry point: picks the layout for the current `DiffViewMode`. | +| `diff/model.rs` | `Row`: `FileHeader \| HunkHeader \| Line \| Placeholder`. Pure. | +| `diff/pairing.rs` | Left/right pairing for the split view. Pure. | +| `diff/row.rs` | The custom `Element`: hitbox, selection registration, run painting. | +| `diff/unified.rs` | Unified layout over `Vec`. | +| `diff/split.rs` | Side-by-side layout over `Vec`. | +| `diff/palette.rs` | The four tint constants, moved from `decorations.rs`. | +| `crates/ui/src/diff_view_mode.rs` | `DiffViewMode`, modelled on `theme_preference.rs`. | + +`model.rs` and `pairing.rs` hold the logic and carry the tests. `row.rs` holds the risk. +The two layout modules should stay thin — they arrange rows, they do not decide anything. + +## Data model + +Unified rendering consumes a flat `Vec` built once per patch: + +``` +Row::FileHeader { path, status, added, deleted } +Row::HunkHeader { text } +Row::Line { origin, old_number: Option, new_number: Option, content } +Row::Placeholder { message } +``` + +Split rendering consumes `Vec`, where each side is an `Option` — `None` +renders as a blank, unnumbered, untinted cell. + +Both are derived when the patch or the view mode changes, never per frame. + +## Pairing algorithm + +Within a hunk: accumulate consecutive deletions into `D` and consecutive additions into +`A`. When the run ends — at a context line or at the end of the hunk — emit `max(|D|,|A|)` +rows pairing `D[i]` with `A[i]`, padding the shorter side with `None`. A context line emits +one row carrying the same text on both sides. + +This is a pure function from `&[DiffLine]` to `Vec` and is where the bulk of the +tests go. Cases that must be covered: pure addition, pure deletion, equal-length +replacement, unequal-length replacement in both directions, a run at the very start of a +hunk, a run at the very end with no trailing context, and a hunk of context only. + +## Selection + +Each row registers a hitbox and one selectable run in `prepaint`/`paint`: + +- `TextSelectionRegistration::new(hitbox, bounds)` with `.with_document_order(n)` so a drag + across rows copies in document order, and `.with_scroll_offset(..)` so the geometry stays + correct under scrolling. +- `TextSelectionRun::new(text, layout, bounds)` — **only for the code content**. The + gutters and the marker are neighbouring elements and are never registered, which is what + makes a copied diff come out as clean code with no line numbers and no markers. This is + strictly better than the editor, which copies its markers today. +- `TextSelectionContentKey` gives virtualised rows a stable identity, so a selection + survives a row being recycled by the list. + +The text has to go through `StyledText` rather than `div().child("…")`, because a run needs +a `TextLayout`. This is the one structural constraint the selection system imposes on the +row. + +## Virtualisation + +`gpui_component::virtual_list` over the derived row vector, with the uniform row height +that dropping soft wrap gives us. Horizontal scrolling is the list's, shared across rows so +the gutters stay put while the code scrolls — that behaviour needs to be verified early, +because it is the part most likely to fight the selection layer's coordinates. + +## Toggle and persistence + +`DiffViewMode { Unified, Split }`, defaulting to `Unified`. Persisted to +`diff-view-preference.json` through the `save_to`/`load_from` plus `save`/`load` pair that +`persistence.rs` already uses for the theme, so the pure half stays testable without +touching the disk. Surfaced as a segmented control in the detail panel's existing tab row +(`detail/mod.rs:164-187`). + +## Edge cases + +- Binary file, rename with no content change, and an empty commit keep their three existing + placeholder messages. +- `\ No newline at end of file` is already swallowed by the parser. GitHub renders a marker + for it; v1 does not. +- Lines beyond 10 000 characters skip highlighting in gpui-component. With no soft wrap they + simply scroll. +- A file that is pure addition renders an entirely blank left column in split view. That is + correct and should not be special-cased into a single-column view. + +## Testing + +- `model.rs` and `pairing.rs`: pure unit tests, the bulk of the suite. +- The contrast reasoning in `decorations.rs:20-35` moves to `palette.rs` with its tests + intact. Those values were chosen against two bars — legibility of text on the band, and + visibility of the band against the theme background — and that reasoning must survive the + move. +- `format::unified_diff_text_with_line_ranges`, `DiffLineRanges`, `write_file` and the + private path helpers that exist only to serve them become dead. Delete them along with + their tests. The case guarded by + `line_ranges_are_read_from_the_marker_column_not_the_lines_own_content` does not + disappear; it becomes trivially correct once no marked text is reconstructed. +- No element-tree tests, consistent with the rest of the crate. +- Manual verification in the running app is required and must not be skipped: nothing here + proves a pixel. + +## Out of scope + +Deliberately excluded from v1, each worth its own change: + +- Syntax highlighting of code by the file's language. +- Word-level intra-line highlighting of what actually changed. +- Expandable context beyond the hunk. +- Per-file collapsing. + +## Risks and order of work + +The custom `Element` is the risk, not the algorithm. Build it first, against the unified +view alone, and prove three things before writing any split-view code: that a drag selects +across row boundaries, that a copy yields code without gutters or markers, and that both +survive scrolling a virtualised list. A bad surprise then arrives before the second view is +written rather than after. + +Second risk: the interplay of `virtual_list` horizontal scrolling with the selection +layer's coordinate handling. `with_scroll_offset` exists for this, but it is untested here. + +## Files touched + +- New: `crates/ui/src/detail/diff/` (six files), `crates/ui/src/diff_view_mode.rs`, + `docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md`. +- Rewritten: `crates/ui/src/detail/mod.rs` — `pending_diff`, `diff_editor` and + `diff_decorations` all disappear, along with the flush block that pushed text into the + editor. +- Reduced: `crates/ui/src/detail/format.rs` — the text reconstruction goes. `hunk_heading` + stays, and so do `abbreviate`, `format_timestamp` and `escape_markdown`, which serve + `metadata.rs` and are unrelated to this change. Note that `file_header` and `diff_stat` + do **not** exist in this file today — they lived only in the renderer deleted at + `5cc1caa` and have to be written again, recoverable from + `git show 5cc1caa^:crates/ui/src/detail/format.rs`. +- Deleted: `crates/ui/src/detail/decorations.rs`, superseded by `diff/palette.rs`. +- Extended: `crates/ui/src/persistence.rs`, `Cargo.toml`. From 3e4a194ab1ede617258698e6ec4bd1441ad011f0 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:12:16 +0200 Subject: [PATCH 02/19] docs(diff): correct the spec where the selection API contradicts it Reading the real signatures overturned two claims made from the shape of the problem rather than from the code. Virtualisation cannot be delegated to `virtual_list`. That list yields one element per row, while a selection participant declares every run in a single `update_runs` call and gets one range back per run. Per-row elements on a shared handle would each overwrite the last; a handle per row is an `Entity` per visible line, rebuilt on every scroll. The body becomes one element that windows and paints its own rows. Dropping soft wrap was justified by virtualisation, and that was wrong: `v_virtual_list` takes one `Size` per item and accepts varying heights. The choice survives on fidelity to GitHub and on the cost of measuring a wrapped height per row per viewport width, but it is a product decision now and reversible on its own. Also records the panic hazard on `TextLayout`: bounds, line_height, len, position_for_index and index_for_position unwrap an inner cell filled during prepaint, so they are callable from paint and nowhere earlier. --- ...026-08-29-github-style-diff-view-design.md | 52 +++++++++++++------ 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md index 788e59e..001a489 100644 --- a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -62,10 +62,13 @@ A reference implementation exists at ## Decisions -**No soft wrap; horizontal scrolling instead.** This is what GitHub does, and it is what -makes virtualisation tractable: every row becomes exactly one line tall, so row heights are -uniform and `virtual_list` applies directly. With soft wrap, heights vary per row and per -viewport width, and the list has to measure before it can place. +**No soft wrap; horizontal scrolling instead.** This is what GitHub does. It is a product +choice, not a technical necessity — an earlier draft of this spec justified it by +virtualisation, which was wrong: `v_virtual_list` takes `Rc>>`, one size +per item, and accepts freely varying heights. What soft wrap really costs is measurement — +every row's wrapped height depends on the viewport width and has to be computed before +anything can be placed. The choice stands on fidelity and on that cost, and can be +revisited without invalidating the rest of this design. **The `+`/`-` marker stays, in a column of its own.** Sixteen pixels, between the gutters and the code, as in the deleted renderer. It no longer shifts the code, it carries the @@ -91,9 +94,9 @@ warns about does not apply here. | `diff/mod.rs` | Entry point: picks the layout for the current `DiffViewMode`. | | `diff/model.rs` | `Row`: `FileHeader \| HunkHeader \| Line \| Placeholder`. Pure. | | `diff/pairing.rs` | Left/right pairing for the split view. Pure. | -| `diff/row.rs` | The custom `Element`: hitbox, selection registration, run painting. | -| `diff/unified.rs` | Unified layout over `Vec`. | -| `diff/split.rs` | Side-by-side layout over `Vec`. | +| `diff/body.rs` | The custom `Element`: visible-range windowing, hitbox, one selection participant, N runs, row painting. | +| `diff/unified.rs` | Unified row geometry over `Vec`. | +| `diff/split.rs` | Side-by-side row geometry over `Vec`. | | `diff/palette.rs` | The four tint constants, moved from `decorations.rs`. | | `crates/ui/src/diff_view_mode.rs` | `DiffViewMode`, modelled on `theme_preference.rs`. | @@ -130,7 +133,8 @@ hunk, a run at the very end with no trailing context, and a hunk of context only ## Selection -Each row registers a hitbox and one selectable run in `prepaint`/`paint`: +The body element registers one participant and declares that frame's visible rows as runs, +in `prepaint`/`paint`: - `TextSelectionRegistration::new(hitbox, bounds)` with `.with_document_order(n)` so a drag across rows copies in document order, and `.with_scroll_offset(..)` so the geometry stays @@ -148,10 +152,20 @@ row. ## Virtualisation -`gpui_component::virtual_list` over the derived row vector, with the uniform row height -that dropping soft wrap gives us. Horizontal scrolling is the list's, shared across rows so -the gutters stay put while the code scrolls — that behaviour needs to be verified early, -because it is the part most likely to fight the selection layer's coordinates. +**Not `virtual_list`.** That list produces one element per row, and the selection API runs +the other way round: a participant declares all its runs in a single +`update_runs(&[TextSelectionRun]) -> TextSelectionProjection` call, and the projection +returns one `Option>` per run, in the order given. Per-row elements sharing +one handle would each overwrite the previous row's runs; a handle per row means one +`Entity` per visible line, rebuilt on every scroll. + +The diff body is therefore **one custom `Element`** that computes its own visible range and +paints the rows in it, registering a single participant and declaring that frame's visible +rows as N runs in one call. Virtualisation is kept; it is hand-rolled rather than borrowed. + +This is more code than delegating to a list, and it is the single largest piece of work in +this design. It is also not optional: it follows from the shape of the only selection API +available. ## Toggle and persistence @@ -202,11 +216,15 @@ Deliberately excluded from v1, each worth its own change: The custom `Element` is the risk, not the algorithm. Build it first, against the unified view alone, and prove three things before writing any split-view code: that a drag selects across row boundaries, that a copy yields code without gutters or markers, and that both -survive scrolling a virtualised list. A bad surprise then arrives before the second view is -written rather than after. - -Second risk: the interplay of `virtual_list` horizontal scrolling with the selection -layer's coordinate handling. `with_scroll_offset` exists for this, but it is untested here. +survive scrolling. A bad surprise then arrives before the second view is written rather +than after. + +Second risk: the coordinate handling. `TextLayout::bounds`, `line_height`, `len`, +`position_for_index` and `index_for_position` all panic when called before the text has been +laid out — they `unwrap`/`expect` on an inner cell filled during prepaint. They are safe +from `paint` and from nowhere earlier. Combined with hand-rolled windowing and a scroll +offset reported through `with_scroll_offset`, this is where an implementation will go wrong +if it goes wrong. ## Files touched From f25ae549732a477a21b2d5c82d4ef02b6a41711b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:15:50 +0200 Subject: [PATCH 03/19] docs(diff): plan the GitHub-style diff view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven tasks. Four are pure and carry real tests — row derivation, left/right pairing, the palette, and the persisted view mode. Three touch gpui and are verified by running the app, because this crate has no element-tree tests and the plan does not add that machinery. Ordering follows the spec's risk: the selectable body element comes before the split view, so that if the participant-and-runs model does not behave as read, it fails before a second view is written on top of it. The self-review caught one requirement with no task behind it. The spec calls for horizontal scrolling instead of soft wrap, and nothing enforced it — `StyledText` wraps to whatever bounds it is handed, and a wrapped row would also break the fixed row height the windowing arithmetic depends on. That is now a step of its own, including the `restrict_scroll_to_axis` flag whose default sends vertical gestures sideways. --- .../2026-08-29-github-style-diff-view.md | 971 ++++++++++++++++++ 1 file changed, 971 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-29-github-style-diff-view.md diff --git a/docs/superpowers/plans/2026-08-29-github-style-diff-view.md b/docs/superpowers/plans/2026-08-29-github-style-diff-view.md new file mode 100644 index 0000000..524658f --- /dev/null +++ b/docs/superpowers/plans/2026-08-29-github-style-diff-view.md @@ -0,0 +1,971 @@ +# GitHub-style diff view — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace the editor-backed diff with a rendered one that reads like GitHub — old/new line-number gutters, a full-width tint per line, the `+`/`-` in its own column, unified and side-by-side views, and text selection that crosses rows. + +**Architecture:** The diff stops being a text document fed to `EditorState` and becomes a derived `Vec` painted by one custom `Element`. That element does its own visible-range windowing and declares every visible row's text as runs of a single selection participant, because `gpui-base`'s selection API projects one range per run from one `update_runs` call. Row derivation and left/right pairing are pure functions and carry the tests; the element carries the risk. + +**Tech Stack:** Rust 2024, gpui (zed default branch), gpui-component 0.5.2 @ `7acfc18`, `gpui-base` (new direct dependency, same git source, no `rev`). + +**Spec:** `docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md` + +## Global Constraints + +- Crates are imported unprefixed: `use domain::…`, never `use gitr_domain::…`. +- `cargo test -p gitr-domain -p gitr-graph` is the fast loop; `cargo test --workspace` is the exit check. +- `cargo clippy --workspace --all-targets -- -D warnings` and `cargo fmt --all --check` must pass at every commit. +- `[workspace.lints.clippy]` sets `todo = "deny"` and `dbg_macro = "deny"`. No `todo!()` scaffolding — every task compiles for real. +- Do not add comments to the code. Doc comments (`///`) on new functions and fields included. Reasoning goes in the commit message. +- Commit convention: `(): `. Never add an AI co-author trailer. +- Never pin `gpui` or any gpui-component crate to a `rev`. `Cargo.lock` is the pin. +- Cargo holds one lock per target directory — do not run `clippy` and `test` concurrently. +- `domain` and `vcs` are not touched by any task in this plan. + +--- + +### Task 1: Row model + +Derives the flat row list the unified view paints. Pure — no gpui, no window. + +**Files:** +- Create: `crates/ui/src/detail/diff/model.rs` +- Create: `crates/ui/src/detail/diff/mod.rs` +- Delete: `crates/ui/src/detail/diff.rs` (its body moves to `mod.rs` unchanged for now) + +**Interfaces:** +- Consumes: `domain::{DiffLine, FilePatch, FileStatus, LineOrigin, Patch}`. +- Produces: `pub(super) enum Row`, `pub(super) fn rows(patch: &Patch) -> Vec`. Task 4 paints `Row`; Task 2 reuses `file_stat`. + +- [ ] **Step 1: Turn the module into a directory** + +```bash +mkdir -p crates/ui/src/detail/diff +git mv crates/ui/src/detail/diff.rs crates/ui/src/detail/diff/mod.rs +``` + +Then add to the top of `crates/ui/src/detail/diff/mod.rs`: + +```rust +mod model; +``` + +- [ ] **Step 2: Write the failing tests** + +Create `crates/ui/src/detail/diff/model.rs` containing only this test module plus the `use super::*;` it needs: + +```rust +#[cfg(test)] +mod tests { + use super::*; + use domain::{FileStatus, Hunk}; + use std::path::PathBuf; + + fn line(origin: LineOrigin, old: Option, new: Option, content: &str) -> DiffLine { + DiffLine { origin, old_number: old, new_number: new, content: content.to_string() } + } + + fn file(hunks: Vec, is_binary: bool) -> FilePatch { + FilePatch { + old_path: Some(PathBuf::from("src/main.rs")), + new_path: Some(PathBuf::from("src/main.rs")), + status: FileStatus::Modified, + is_binary, + hunks, + } + } + + fn hunk(lines: Vec) -> Hunk { + Hunk { old_start: 1, old_lines: 1, new_start: 1, new_lines: 1, heading: String::new(), lines } + } + + #[test] + fn a_modified_file_yields_a_header_a_hunk_header_and_one_row_per_line() { + let patch = Patch { files: vec![file(vec![hunk(vec![ + line(LineOrigin::Context, Some(1), Some(1), "keep"), + line(LineOrigin::Deletion, Some(2), None, "gone"), + line(LineOrigin::Addition, None, Some(2), "new"), + ])], false)] }; + + let rows = rows(&patch); + + assert!(matches!(rows[0], Row::FileHeader { .. })); + assert!(matches!(rows[1], Row::HunkHeader { .. })); + assert_eq!(rows.len(), 5); + } + + #[test] + fn a_line_row_carries_both_numbers_and_the_bare_content() { + let patch = Patch { files: vec![file(vec![hunk(vec![ + line(LineOrigin::Deletion, Some(7), None, "-not a marker"), + ])], false)] }; + + let rows = rows(&patch); + + assert_eq!( + rows[2], + Row::Line { + origin: LineOrigin::Deletion, + old_number: Some(7), + new_number: None, + content: "-not a marker".to_string(), + }, + "content is stored bare by the parser and must not be re-marked here" + ); + } + + #[test] + fn a_binary_file_yields_a_placeholder_instead_of_lines() { + let patch = Patch { files: vec![file(Vec::new(), true)] }; + let rows = rows(&patch); + assert_eq!(rows[1], Row::Placeholder { message: "Binary file not shown." }); + } + + #[test] + fn a_file_with_no_hunks_yields_a_no_change_placeholder() { + let patch = Patch { files: vec![file(Vec::new(), false)] }; + let rows = rows(&patch); + assert_eq!(rows[1], Row::Placeholder { message: "No content changes." }); + } + + #[test] + fn every_file_contributes_its_own_header() { + let patch = Patch { files: vec![file(Vec::new(), true), file(Vec::new(), true)] }; + let headers = rows(&patch).iter().filter(|r| matches!(r, Row::FileHeader { .. })).count(); + assert_eq!(headers, 2); + } +} +``` + +- [ ] **Step 3: Run the tests to verify they fail** + +Run: `cargo test -p gitr-ui --lib detail::diff::model` +Expected: FAIL to compile — `Row` and `rows` are not defined. + +- [ ] **Step 4: Write the implementation** + +Prepend to `crates/ui/src/detail/diff/model.rs`: + +```rust +use domain::{DiffLine, FilePatch, LineOrigin, Patch}; + +use crate::detail::format; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) enum Row { + FileHeader { path: String, stat: String }, + HunkHeader { text: String }, + Line { + origin: LineOrigin, + old_number: Option, + new_number: Option, + content: String, + }, + Placeholder { message: &'static str }, +} + +pub(super) fn rows(patch: &Patch) -> Vec { + let mut rows = Vec::new(); + for file in &patch.files { + rows.push(Row::FileHeader { + path: file.display_path(), + stat: file_stat(file), + }); + push_body(&mut rows, file); + } + rows +} + +pub(super) fn file_stat(file: &FilePatch) -> String { + format!("+{} \u{2212}{}", file.added_lines(), file.deleted_lines()) +} + +fn push_body(rows: &mut Vec, file: &FilePatch) { + if file.is_binary { + rows.push(Row::Placeholder { message: "Binary file not shown." }); + return; + } + if file.hunks.is_empty() { + rows.push(Row::Placeholder { message: "No content changes." }); + return; + } + for hunk in &file.hunks { + rows.push(Row::HunkHeader { text: format::hunk_heading(hunk) }); + rows.extend(hunk.lines.iter().map(line_row)); + } +} + +fn line_row(line: &DiffLine) -> Row { + Row::Line { + origin: line.origin, + old_number: line.old_number, + new_number: line.new_number, + content: line.content.clone(), + } +} +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `cargo test -p gitr-ui --lib detail::diff::model` +Expected: PASS, 5 tests. + +- [ ] **Step 6: Check `display_path` returns a `String`** + +Run: `grep -n "fn display_path" -A 6 crates/domain/src/patch.rs` +If it returns `&Path` or `Option<&Path>` rather than `String`, adjust the `path:` field construction to `.display().to_string()`. Do not change `domain`. + +- [ ] **Step 7: Lint, format, commit** + +```bash +cargo fmt --all +cargo clippy -p gitr-ui --all-targets -- -D warnings +git add crates/ui/src/detail/diff/ +git commit -m "feat(diff): derive a flat row model from a patch" +``` + +--- + +### Task 2: Side-by-side pairing + +The only real algorithm in this plan. Pure. + +**Files:** +- Create: `crates/ui/src/detail/diff/pairing.rs` +- Modify: `crates/ui/src/detail/diff/mod.rs` — add `mod pairing;` + +**Interfaces:** +- Consumes: `domain::{DiffLine, LineOrigin}`. +- Produces: `pub(super) struct SplitRow { pub left: Option, pub right: Option }`, `pub(super) struct SideLine { pub number: Option, pub origin: LineOrigin, pub content: String }`, `pub(super) fn pair(lines: &[DiffLine]) -> Vec`. Task 7 paints these. + +- [ ] **Step 1: Write the failing tests** + +Create `crates/ui/src/detail/diff/pairing.rs` with this test module: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + fn line(origin: LineOrigin, old: Option, new: Option, content: &str) -> DiffLine { + DiffLine { origin, old_number: old, new_number: new, content: content.to_string() } + } + + fn deletion(number: u32, content: &str) -> DiffLine { + line(LineOrigin::Deletion, Some(number), None, content) + } + + fn addition(number: u32, content: &str) -> DiffLine { + line(LineOrigin::Addition, None, Some(number), content) + } + + fn context(number: u32, content: &str) -> DiffLine { + line(LineOrigin::Context, Some(number), Some(number), content) + } + + #[test] + fn a_context_line_is_the_same_on_both_sides() { + let rows = pair(&[context(1, "keep")]); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].left.as_ref().map(|s| s.content.as_str()), Some("keep")); + assert_eq!(rows[0].right.as_ref().map(|s| s.content.as_str()), Some("keep")); + } + + #[test] + fn an_equal_length_replacement_pairs_line_for_line() { + let rows = pair(&[deletion(1, "a"), deletion(2, "b"), addition(1, "x"), addition(2, "y")]); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0].left.as_ref().map(|s| s.content.as_str()), Some("a")); + assert_eq!(rows[0].right.as_ref().map(|s| s.content.as_str()), Some("x")); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("b")); + assert_eq!(rows[1].right.as_ref().map(|s| s.content.as_str()), Some("y")); + } + + #[test] + fn more_additions_than_deletions_pads_the_left() { + let rows = pair(&[deletion(1, "a"), addition(1, "x"), addition(2, "y")]); + assert_eq!(rows.len(), 2); + assert!(rows[1].left.is_none(), "the extra addition has nothing to pair with"); + assert_eq!(rows[1].right.as_ref().map(|s| s.content.as_str()), Some("y")); + } + + #[test] + fn more_deletions_than_additions_pads_the_right() { + let rows = pair(&[deletion(1, "a"), deletion(2, "b"), addition(1, "x")]); + assert_eq!(rows.len(), 2); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("b")); + assert!(rows[1].right.is_none()); + } + + #[test] + fn a_pure_addition_leaves_the_left_side_empty() { + let rows = pair(&[addition(1, "x"), addition(2, "y")]); + assert_eq!(rows.len(), 2); + assert!(rows.iter().all(|row| row.left.is_none())); + } + + #[test] + fn a_pure_deletion_leaves_the_right_side_empty() { + let rows = pair(&[deletion(1, "a")]); + assert_eq!(rows.len(), 1); + assert!(rows[0].right.is_none()); + } + + #[test] + fn a_run_is_flushed_when_a_context_line_ends_it() { + let rows = pair(&[deletion(1, "a"), addition(1, "x"), context(2, "keep")]); + assert_eq!(rows.len(), 2); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("keep")); + } + + #[test] + fn a_run_at_the_very_end_is_flushed_without_trailing_context() { + let rows = pair(&[context(1, "keep"), deletion(2, "a")]); + assert_eq!(rows.len(), 2, "the trailing run must not be dropped"); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("a")); + } + + #[test] + fn an_empty_hunk_yields_no_rows() { + assert!(pair(&[]).is_empty()); + } + + #[test] + fn a_side_line_keeps_its_own_number() { + let rows = pair(&[deletion(7, "a"), addition(9, "x")]); + assert_eq!(rows[0].left.as_ref().and_then(|s| s.number), Some(7)); + assert_eq!(rows[0].right.as_ref().and_then(|s| s.number), Some(9)); + } +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cargo test -p gitr-ui --lib detail::diff::pairing` +Expected: FAIL to compile — `pair`, `SplitRow`, `SideLine` are not defined. + +- [ ] **Step 3: Write the implementation** + +Prepend to `crates/ui/src/detail/diff/pairing.rs`: + +```rust +use domain::{DiffLine, LineOrigin}; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct SideLine { + pub number: Option, + pub origin: LineOrigin, + pub content: String, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(super) struct SplitRow { + pub left: Option, + pub right: Option, +} + +pub(super) fn pair(lines: &[DiffLine]) -> Vec { + let mut rows = Vec::new(); + let mut deletions: Vec<&DiffLine> = Vec::new(); + let mut additions: Vec<&DiffLine> = Vec::new(); + + for line in lines { + match line.origin { + LineOrigin::Deletion => deletions.push(line), + LineOrigin::Addition => additions.push(line), + LineOrigin::Context => { + flush(&mut rows, &mut deletions, &mut additions); + rows.push(SplitRow { + left: Some(side(line, line.old_number)), + right: Some(side(line, line.new_number)), + }); + } + } + } + flush(&mut rows, &mut deletions, &mut additions); + rows +} + +fn flush(rows: &mut Vec, deletions: &mut Vec<&DiffLine>, additions: &mut Vec<&DiffLine>) { + let paired = deletions.len().max(additions.len()); + for index in 0..paired { + rows.push(SplitRow { + left: deletions.get(index).map(|line| side(line, line.old_number)), + right: additions.get(index).map(|line| side(line, line.new_number)), + }); + } + deletions.clear(); + additions.clear(); +} + +fn side(line: &DiffLine, number: Option) -> SideLine { + SideLine { + number, + origin: line.origin, + content: line.content.clone(), + } +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cargo test -p gitr-ui --lib detail::diff::pairing` +Expected: PASS, 10 tests. + +- [ ] **Step 5: Register the module** + +Add `mod pairing;` to `crates/ui/src/detail/diff/mod.rs`. It is unused until Task 7, so add `#[allow(dead_code)]` above the `mod pairing;` line and delete that attribute in Task 7. + +- [ ] **Step 6: Lint, format, commit** + +```bash +cargo fmt --all +cargo clippy -p gitr-ui --all-targets -- -D warnings +git add crates/ui/src/detail/diff/ +git commit -m "feat(diff): pair deletions with additions for a side-by-side view" +``` + +--- + +### Task 3: Row palette + +Moves the four tint constants and their contrast reasoning out of `decorations.rs`, and adds the foreground colours the rows need. `decorations.rs` is still alive at the end of this task; Task 4 deletes it. + +**Files:** +- Create: `crates/ui/src/detail/diff/palette.rs` +- Modify: `crates/ui/src/detail/diff/mod.rs` — add `mod palette;` + +**Interfaces:** +- Consumes: `gpui_component::{ThemeColor, ThemeMode}`, `domain::LineOrigin`. +- Produces: `pub(super) struct LineColors { pub background: Option, pub foreground: Hsla }`, `pub(super) fn line_colors(origin: LineOrigin, mode: ThemeMode, theme: &ThemeColor) -> LineColors`. + +- [ ] **Step 1: Write the failing tests** + +Create `crates/ui/src/detail/diff/palette.rs` with this test module: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_context_line_has_no_background() { + let theme = ThemeColor::light(); + assert!(line_colors(LineOrigin::Context, ThemeMode::Light, &theme).background.is_none()); + } + + #[test] + fn an_addition_and_a_deletion_do_not_share_a_background() { + let theme = ThemeColor::light(); + let added = line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background; + let deleted = line_colors(LineOrigin::Deletion, ThemeMode::Light, &theme).background; + assert!(added.is_some() && deleted.is_some()); + assert_ne!(added, deleted); + } + + #[test] + fn dark_mode_does_not_reuse_the_light_pair() { + let theme = ThemeColor::dark(); + let light = line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background; + let dark = line_colors(LineOrigin::Addition, ThemeMode::Dark, &theme).background; + assert_ne!(light, dark); + } + + #[test] + fn every_band_is_distinguishable_from_the_background_it_sits_on() { + for (mode, theme) in [(ThemeMode::Light, ThemeColor::light()), (ThemeMode::Dark, ThemeColor::dark())] { + for origin in [LineOrigin::Addition, LineOrigin::Deletion] { + let band = line_colors(origin, mode, &theme).background.expect("a band"); + let distance = crate::theme_palette::rendered_distance(theme.background, band, theme.background); + assert!(distance > 0.0, "{origin:?} in {mode:?} must not vanish into the background"); + } + } + } +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cargo test -p gitr-ui --lib detail::diff::palette` +Expected: FAIL to compile — `line_colors` is not defined. + +- [ ] **Step 3: Write the implementation** + +Prepend to `crates/ui/src/detail/diff/palette.rs`. The four constants and their reasoning come verbatim from `crates/ui/src/detail/decorations.rs:17-35` — copy the doc comments on the two dark constants across unchanged, they record contrast measurements that must not be lost: + +```rust +use domain::LineOrigin; +use gpui::{Hsla, rgb}; +use gpui_component::{ThemeColor, ThemeMode}; + +const LIGHT_ADDITION_BACKGROUND: u32 = 0xdafbe1; +const LIGHT_DELETION_BACKGROUND: u32 = 0xffebe9; +const DARK_ADDITION_BACKGROUND: u32 = 0x355a40; +const DARK_DELETION_BACKGROUND: u32 = 0x5f3c45; + +pub(super) struct LineColors { + pub background: Option, + pub foreground: Hsla, +} + +pub(super) fn line_colors(origin: LineOrigin, mode: ThemeMode, theme: &ThemeColor) -> LineColors { + let background = match (origin, mode.is_dark()) { + (LineOrigin::Context, _) => None, + (LineOrigin::Addition, false) => Some(rgb(LIGHT_ADDITION_BACKGROUND).into()), + (LineOrigin::Addition, true) => Some(rgb(DARK_ADDITION_BACKGROUND).into()), + (LineOrigin::Deletion, false) => Some(rgb(LIGHT_DELETION_BACKGROUND).into()), + (LineOrigin::Deletion, true) => Some(rgb(DARK_DELETION_BACKGROUND).into()), + }; + LineColors { background, foreground: theme.foreground } +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cargo test -p gitr-ui --lib detail::diff::palette` +Expected: PASS, 4 tests. + +If `rendered_distance` is not `pub(crate)`, widen it in `crates/ui/src/theme_palette.rs` to `pub(crate)` rather than duplicating it. + +- [ ] **Step 5: Lint, format, commit** + +```bash +cargo fmt --all +cargo clippy -p gitr-ui --all-targets -- -D warnings +git add crates/ui/src/detail/diff/ crates/ui/src/theme_palette.rs +git commit -m "feat(diff): give rows their own palette, carrying the contrast reasoning over" +``` + +--- + +### Task 4: The selectable diff body + +The risk. One custom `Element` that paints every row and declares their text as runs of one selection participant. No windowing yet — Task 5 adds it — so this task must be verified on a small commit. + +**Files:** +- Modify: `Cargo.toml` — add `gpui-base` to `[workspace.dependencies]` +- Modify: `crates/ui/Cargo.toml` — add `gpui-base.workspace = true` +- Create: `crates/ui/src/detail/diff/body.rs` +- Modify: `crates/ui/src/detail/diff/mod.rs` — render `body`, drop the `Editor` +- Modify: `crates/ui/src/detail/mod.rs` — drop `pending_diff`, `diff_editor`, `diff_decorations` +- Delete: `crates/ui/src/detail/decorations.rs` +- Modify: `crates/ui/src/detail/format.rs` — delete `DiffLineRanges`, `unified_diff_text_with_line_ranges`, `write_file`, and the path helpers that only served them, plus their tests + +**Interfaces:** +- Consumes: `Row` and `rows` (Task 1), `LineColors` and `line_colors` (Task 3). +- Produces: `pub(super) struct DiffBody`, `pub(super) fn body(rows: Vec, selection: TextSelectionHandle, theme: ThemeColor, mode: ThemeMode) -> DiffBody`. Task 5 adds windowing inside it; Task 7 adds a split variant beside it. + +- [ ] **Step 1: Add the dependency** + +In `Cargo.toml`, after the `gpui-component-assets` line: + +```toml +gpui-base = { git = "https://github.com/longbridge/gpui-component" } +``` + +No `rev` — `gpui-component` is declared the same way, and Cargo unifies two git sources only when the reference matches exactly. In `crates/ui/Cargo.toml`, under `[dependencies]`, after `gpui-component.workspace = true`: + +```toml +gpui-base.workspace = true +``` + +- [ ] **Step 2: Verify the dependency unified rather than duplicated** + +Run: `cargo tree -p gitr-ui -i gpui-base 2>&1 | head -20` +Expected: exactly one `gpui-base v0.5.2` at source `#7acfc18…`. If two appear, stop — the reference does not match and every trait from one copy will fail to apply to the other. + +- [ ] **Step 3: Write the element** + +Create `crates/ui/src/detail/diff/body.rs`. This is adapted from the reference at +`~/.cargo/git/checkouts/gpui-component-95ce574d8a0da8b8/7acfc18/crates/base/examples/showcase/components/text_selection.rs`, +whose `PlainSelectableText` is the only worked example of a custom element joining the selection system. Read it before writing this. The two departures from it are that the participant declares many runs rather than one, and that each run is one row's code text. + +```rust +use std::ops::Range; + +use gpui::{ + App, Bounds, Element, ElementId, GlobalElementId, Hitbox, HitboxBehavior, InspectorElementId, + IntoElement, LayoutId, Pixels, Point, SharedString, StyledText, Window, fill, point, px, size, +}; +use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; +use gpui_component::{ThemeColor, ThemeMode}; + +use super::model::Row; +use super::palette::line_colors; + +const GUTTER_WIDTH: f32 = 44.; +const MARKER_WIDTH: f32 = 16.; +const ROW_HEIGHT: f32 = 18.; + +pub(super) struct DiffBody { + rows: Vec, + selection: TextSelectionHandle, + theme: ThemeColor, + mode: ThemeMode, + texts: Vec, +} + +pub(super) fn body( + rows: Vec, + selection: TextSelectionHandle, + theme: ThemeColor, + mode: ThemeMode, +) -> DiffBody { + let texts = rows.iter().map(|row| StyledText::new(row_text(row))).collect(); + DiffBody { rows, selection, theme, mode, texts } +} + +fn row_text(row: &Row) -> SharedString { + match row { + Row::FileHeader { path, stat } => format!("{path} {stat}").into(), + Row::HunkHeader { text } => text.clone().into(), + Row::Line { content, .. } => content.clone().into(), + Row::Placeholder { message } => (*message).into(), + } +} +``` + +The `Element` impl follows the reference's shape exactly — `type RequestLayoutState = ();`, `type PrepaintState = Hitbox;`, `id` and `source_location` returning `None`. In `prepaint`, lay out each `StyledText` at its row's bounds, insert one hitbox over the whole body, and register one participant: + +```rust +self.selection.register( + TextSelectionRegistration::new(hitbox.clone(), bounds) + .with_document_order(0) + .with_text_bounds(row_bounds.clone()), + window, + cx, +); +``` + +In `paint`, build one run per row, in document order, and project: + +```rust +let runs: Vec = self + .texts + .iter() + .enumerate() + .map(|(index, text)| { + TextSelectionRun::new(row_text(&self.rows[index]), text.layout().clone(), row_bounds[index]) + .with_document_order(index as u64) + }) + .collect(); +let projection = self.selection.update_runs(&runs, cx); +``` + +Then, per row: paint the tint quad across the full `bounds.size.width`, paint the two gutter numbers and the marker, paint the selection quads for `projection.ranges()[index]` if `Some`, and finally paint the row's `StyledText`. + +Only the code text becomes a run. The gutters and the marker are painted directly and are never registered, which is what keeps them out of the clipboard. + +**`TextLayout::bounds`, `line_height`, `len`, `position_for_index` and `index_for_position` panic if called before layout.** They are safe in `paint` and nowhere earlier. + +Copy `selection_quad_bounds` verbatim from the reference — it is a ready-made three-rectangle start/middle/end helper. + +- [ ] **Step 4: Render it, and tear out the editor** + +In `crates/ui/src/detail/diff/mod.rs`, replace the `Editor::new(...)` body of `render` with the new element, keeping the empty-patch early return unchanged. In `crates/ui/src/detail/mod.rs`, delete the `pending_diff`, `diff_editor` and `diff_decorations` fields, the flush block at the top of `Render::render`, and the `EditorState`/`TextDecorationCollection` imports. `DetailPanel` gains one `TextSelectionHandle`, built in `new`: + +```rust +let selection = TextSelectionHandle::new("", cx); +selection.refresh_window_on_change(window, cx).detach(); +``` + +Without that subscription the selection changes but nothing repaints. + +`set_detail` now derives rows directly — it needs no window, which is the whole reason `pending_diff` existed: + +```rust +pub fn set_detail(&mut self, detail: LoadState>, cx: &mut Context) { + self.detail = detail; + cx.notify(); +} +``` + +Delete `crates/ui/src/detail/decorations.rs` and its `mod decorations;` line. In `format.rs`, delete `DiffLineRanges`, `unified_diff_text_with_line_ranges`, `write_file`, `git_path`, `git_header_path`, `side_path`, and every test naming them. Keep `abbreviate`, `format_timestamp`, `escape_markdown` and `hunk_heading`. Update the module doc on `detail/mod.rs`, which describes the editor and the staging that no longer exist. + +- [ ] **Step 5: Stop the code from wrapping** + +The spec calls for horizontal scrolling rather than soft wrap, and nothing so far enforces +it: `StyledText` wraps to the bounds it is given. Lay each row's text out against a width +wide enough that it never wraps — the widest row's measured width, not the viewport's — and +put the body inside a horizontally scrollable parent in `diff/mod.rs`: + +```rust +div() + .id("detail-diff-scroll") + .size_full() + .overflow_x_scroll() + .restrict_scroll_to_axis() + .child(body(...)) +``` + +`restrict_scroll_to_axis` is not optional. It defaults to `false`, and with it unset a +scrollable-x element treats a vertical delta as horizontal whenever its own y overflow is +not `Scroll` — a vertical gesture over the diff would scroll it sideways and never scroll it +down. + +If a row wraps anyway, the fixed `ROW_HEIGHT` Task 5 relies on is wrong and the arithmetic +windowing breaks. Confirm no row wraps before moving on. + +- [ ] **Step 6: Build and lint** + +```bash +cargo fmt --all +cargo clippy --workspace --all-targets -- -D warnings +cargo test --workspace +``` +Expected: green. The tests deleted from `format.rs` are gone; nothing else should fail. + +- [ ] **Step 7: Verify in the running app — this task is not done without it** + +gitr is single-instance, so a running installed binary will swallow the launch and you will test the old build. Check first: + +```bash +ps -eo pid,command | grep "[g]itr" | grep -v cargo +``` + +If an instance is running, quit it, then: + +```bash +cargo run -p gitr_gui -- . +``` + +Select a commit, open the Diffs tab, and confirm all five: the code starts at the same column on every line; added and deleted lines carry a band that reaches the full width, including blank ones; both gutters show the file's own line numbers, not a running document count; a drag selects across row boundaries; and Cmd-C yields code with no line numbers and no `+`/`-`. + +- [ ] **Step 8: Commit** + +```bash +git add -A +git commit -m "feat(diff): render the diff as rows with selectable code" +``` + +--- + +### Task 5: Windowing + +Task 4 builds a `StyledText` per row for the whole patch. On a large commit that is thousands of laid-out lines per frame. This task limits the work to the rows on screen. + +**Files:** +- Modify: `crates/ui/src/detail/diff/body.rs` + +**Interfaces:** +- Consumes: `DiffBody` from Task 4. +- Produces: no new public names. `body()` keeps its signature. + +- [ ] **Step 1: Establish the baseline** + +Run the app on a repository with a large commit and note that scrolling is slow. `zed-industries/zed` has commits touching hundreds of files. Record what you saw — this is the before. + +- [ ] **Step 2: Compute the visible range in `prepaint`** + +Row height is fixed at `ROW_HEIGHT`, so the range is arithmetic, not measurement: + +```rust +let first = (scroll_offset.y / px(ROW_HEIGHT)).floor().max(0.) as usize; +let visible = (bounds.size.height / px(ROW_HEIGHT)).ceil() as usize + 1; +let range = first..(first + visible).min(self.rows.len()); +``` + +Lay out `StyledText` only for `range`, and store `range` on the element so `paint` uses the same one. + +- [ ] **Step 3: Keep document order absolute** + +Runs must carry their index in the whole patch, not in the visible window, or a selection dragged past the edge reorders on copy: + +```rust +.with_document_order(range.start as u64 + offset_in_window as u64) +``` + +- [ ] **Step 4: Report the scroll offset to the selection layer** + +```rust +TextSelectionRegistration::new(hitbox.clone(), bounds) + .with_scroll_offset(scroll_offset) +``` + +Without it, hit-testing maps window points into the wrong content position as soon as the body is scrolled. + +- [ ] **Step 5: Verify** + +Run the app again on the same large commit. Scrolling should be smooth. Then re-check the two selection behaviours from Task 4 Step 7 **while scrolled down**, and drag a selection from a visible row past the bottom edge to confirm document order survives. + +- [ ] **Step 6: Lint, format, commit** + +```bash +cargo fmt --all +cargo clippy --workspace --all-targets -- -D warnings +git add crates/ui/src/detail/diff/body.rs +git commit -m "feat(diff): lay out only the rows on screen" +``` + +--- + +### Task 6: The view-mode preference + +Pure and testable, and independent of Tasks 4 and 5 — it can be built in parallel with them. + +**Files:** +- Create: `crates/ui/src/diff_view_mode.rs` +- Modify: `crates/ui/src/lib.rs` — add `pub mod diff_view_mode;` +- Modify: `crates/ui/src/persistence.rs` + +**Interfaces:** +- Produces: `pub enum DiffViewMode { Unified, Split }` with `ALL`, `index`, `from_index`, `label`; `persistence::{save_diff_view_mode, load_diff_view_mode, save_diff_view_mode_to, load_diff_view_mode_from}`. + +- [ ] **Step 1: Read the pattern to copy** + +Run: `cat crates/ui/src/theme_preference.rs` and `grep -n "theme_preference" crates/ui/src/persistence.rs`. `DiffViewMode` mirrors `ThemePreference` — a small serde enum with a `Default`, and a `_to`/`_from` pair beside the `save`/`load` pair so the disk-free half stays testable. + +- [ ] **Step 2: Write the failing tests** + +In `crates/ui/src/diff_view_mode.rs`: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_default_is_unified() { + assert_eq!(DiffViewMode::default(), DiffViewMode::Unified); + } + + #[test] + fn every_mode_round_trips_through_its_index() { + for mode in DiffViewMode::ALL { + assert_eq!(DiffViewMode::from_index(mode.index()), mode); + } + } + + #[test] + fn an_out_of_range_index_falls_back_to_the_default() { + assert_eq!(DiffViewMode::from_index(99), DiffViewMode::default()); + } +} +``` + +And in `crates/ui/src/persistence.rs`'s test module: + +```rust +#[test] +fn a_diff_view_mode_round_trips_through_a_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("diff-view-preference.json"); + save_diff_view_mode_to(&path, &DiffViewMode::Split).expect("save"); + assert_eq!(load_diff_view_mode_from(&path).expect("load"), DiffViewMode::Split); +} +``` + +- [ ] **Step 3: Run the tests to verify they fail** + +Run: `cargo test -p gitr-ui --lib diff_view_mode persistence` +Expected: FAIL to compile. + +- [ ] **Step 4: Write the implementation** + +```rust +use serde::{Deserialize, Serialize}; + +#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum DiffViewMode { + #[default] + Unified, + Split, +} + +impl DiffViewMode { + pub const ALL: [DiffViewMode; 2] = [Self::Unified, Self::Split]; + + pub fn index(self) -> usize { + Self::ALL.iter().position(|mode| *mode == self).unwrap_or(0) + } + + pub fn from_index(index: usize) -> Self { + Self::ALL.get(index).copied().unwrap_or_default() + } + + pub fn label(self) -> &'static str { + match self { + Self::Unified => "Unified", + Self::Split => "Split", + } + } +} +``` + +In `persistence.rs`, add `const DIFF_VIEW_MODE_FILE: &str = "diff-view-preference.json";` beside the other file constants, and copy the four theme-preference functions, substituting the type and the constant. + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `cargo test -p gitr-ui --lib diff_view_mode persistence` +Expected: PASS. + +- [ ] **Step 6: Lint, format, commit** + +```bash +cargo fmt --all +cargo clippy -p gitr-ui --all-targets -- -D warnings +git add crates/ui/src/diff_view_mode.rs crates/ui/src/lib.rs crates/ui/src/persistence.rs +git commit -m "feat(diff): persist the chosen diff view mode" +``` + +--- + +### Task 7: Side-by-side view and the toggle + +**Files:** +- Create: `crates/ui/src/detail/diff/split.rs` +- Modify: `crates/ui/src/detail/diff/body.rs`, `crates/ui/src/detail/diff/mod.rs`, `crates/ui/src/detail/mod.rs` +- Modify: `crates/ui/src/detail/diff/pairing.rs` — remove the `#[allow(dead_code)]` added in Task 2 + +**Interfaces:** +- Consumes: `pair`, `SplitRow`, `SideLine` (Task 2); `DiffViewMode` (Task 6); `DiffBody` (Tasks 4-5). +- Produces: `pub(super) fn split_rows(patch: &Patch) -> Vec`. + +- [ ] **Step 1: Write the failing test for split row derivation** + +In `crates/ui/src/detail/diff/split.rs`, a test asserting that a two-file patch yields the rows of both files in order, with each file's header row present. Follow Task 1's fixtures. + +- [ ] **Step 2: Run it to verify it fails** + +Run: `cargo test -p gitr-ui --lib detail::diff::split` +Expected: FAIL to compile. + +- [ ] **Step 3: Implement `split_rows`** + +Walk `patch.files`, emit the same `Row::FileHeader` and `Row::Placeholder` cases as Task 1, and for each hunk call `pairing::pair(&hunk.lines)`. + +- [ ] **Step 4: Run it to verify it passes** + +Run: `cargo test -p gitr-ui --lib detail::diff::split` + +- [ ] **Step 5: Paint two columns** + +Extend `DiffBody` to take an enum of either `Vec` or `Vec`. In the split case each row paints gutter, marker and code twice, at `bounds.size.width / 2.` — and declares **two** runs, left then right, so document order runs left-to-right within a row. + +- [ ] **Step 6: Add the toggle** + +In `crates/ui/src/detail/mod.rs`, add a second `TabBar::new("diff-view-mode").segmented().small()` beside the existing `detail-tabs` bar, rendered only when `selected_tab == DetailTab::Diff`. Its `on_click` sets the mode, calls `persistence::save_diff_view_mode` on a background executor, and `cx.notify()`. Load the saved mode in `DetailPanel::new`. + +- [ ] **Step 7: Verify in the running app** + +Confirm: the toggle switches views and survives a restart; a pure addition shows an empty left column rather than collapsing to one; a drag across the split view copies left-then-right within a row and top-to-bottom across rows. + +- [ ] **Step 8: Full check and commit** + +```bash +cargo fmt --all +cargo clippy --workspace --all-targets -- -D warnings +cargo test --workspace +git add -A +git commit -m "feat(diff): add a side-by-side view behind a persisted toggle" +``` + +--- + +## Notes for whoever executes this + +Tasks 1, 2, 3 and 6 are pure and carry real tests. Tasks 4, 5 and 7 touch gpui and are verified by running the app — this crate has no element-tree tests and this plan does not add the machinery for them. + +Task 4 is where this plan is most likely to be wrong. Its code is adapted from a reference example, not from something compiled while writing this. Read +`crates/base/examples/showcase/components/text_selection.rs` in the gpui-component checkout in full before starting it, and treat the snippets here as the shape rather than the letter. If the participant/run model turns out not to work as described, stop and re-open the spec's Virtualisation section rather than working around it in the element. From 03c78c3dd7c02ba25a5f3cee77bd3fa9e756df5f Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:21:18 +0200 Subject: [PATCH 04/19] feat(diff): derive a flat row model from a patch --- crates/ui/src/detail/diff.rs | 49 ------- crates/ui/src/detail/diff/mod.rs | 51 ++++++++ crates/ui/src/detail/diff/model.rs | 202 +++++++++++++++++++++++++++++ 3 files changed, 253 insertions(+), 49 deletions(-) delete mode 100644 crates/ui/src/detail/diff.rs create mode 100644 crates/ui/src/detail/diff/mod.rs create mode 100644 crates/ui/src/detail/diff/model.rs diff --git a/crates/ui/src/detail/diff.rs b/crates/ui/src/detail/diff.rs deleted file mode 100644 index 88c01fd..0000000 --- a/crates/ui/src/detail/diff.rs +++ /dev/null @@ -1,49 +0,0 @@ -//! Renders a `Patch` inside a single readonly code editor, instead of one hand-built -//! `div` per diff line. -//! -//! A hand-built row per line means every element for the whole patch is constructed on -//! every render, selects nothing, and highlights nothing beyond a full-width background -//! tint. Feeding [`super::format::unified_diff_text`] into a real -//! [`gpui_component::input::Editor`] gets text selection, `tree-sitter-diff` syntax -//! highlighting, and virtualised scrolling that only lays out the lines actually on -//! screen — for free, from the editor. -//! -//! One [`Editor`] holds the whole patch rather than one per file: a diff reads as a -//! single continuous document (this is how `git diff` and a GitHub raw patch view both -//! present it), a single scrollbar matches the rest of the panel, and it avoids creating -//! and tearing down one [`EditorState`] entity per file on every commit selection. The -//! trade-off is that per-file collapsing isn't available; nothing in this panel asks for -//! it. -//! -//! [`super::DetailPanel`] owns the [`EditorState`] entity and is the one that pushes new -//! text into it — this module only builds the element each render, exactly like the rest -//! of the panel's view functions. - -use domain::Patch; -use gpui::{AnyElement, App, Entity, IntoElement, ParentElement as _, Styled as _, div}; -use gpui_component::{ - ActiveTheme as _, - input::{Editor, EditorState}, -}; - -pub(super) fn render(patch: &Patch, diff_editor: &Entity, cx: &App) -> AnyElement { - if patch.files.is_empty() { - return div() - .size_full() - .flex() - .items_center() - .justify_center() - .text_color(cx.theme().muted_foreground) - .child("This commit changes nothing.") - .into_any_element(); - } - - Editor::new(diff_editor) - .appearance(false) - .bordered(false) - .readonly(true) - .font_family(cx.theme().mono_font_family.clone()) - .text_size(cx.theme().mono_font_size) - .size_full() - .into_any_element() -} diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs new file mode 100644 index 0000000..024b24e --- /dev/null +++ b/crates/ui/src/detail/diff/mod.rs @@ -0,0 +1,51 @@ +mod model; + +// Renders a `Patch` inside a single readonly code editor, instead of one hand-built +// `div` per diff line. +// +// A hand-built row per line means every element for the whole patch is constructed on +// every render, selects nothing, and highlights nothing beyond a full-width background +// tint. Feeding [`super::format::unified_diff_text`] into a real +// [`gpui_component::input::Editor`] gets text selection, `tree-sitter-diff` syntax +// highlighting, and virtualised scrolling that only lays out the lines actually on +// screen — for free, from the editor. +// +// One [`Editor`] holds the whole patch rather than one per file: a diff reads as a +// single continuous document (this is how `git diff` and a GitHub raw patch view both +// present it), a single scrollbar matches the rest of the panel, and it avoids creating +// and tearing down one [`EditorState`] entity per file on every commit selection. The +// trade-off is that per-file collapsing isn't available; nothing in this panel asks for +// it. +// +// [`super::DetailPanel`] owns the [`EditorState`] entity and is the one that pushes new +// text into it — this module only builds the element each render, exactly like the rest +// of the panel's view functions. + +use domain::Patch; +use gpui::{AnyElement, App, Entity, IntoElement, ParentElement as _, Styled as _, div}; +use gpui_component::{ + ActiveTheme as _, + input::{Editor, EditorState}, +}; + +pub(super) fn render(patch: &Patch, diff_editor: &Entity, cx: &App) -> AnyElement { + if patch.files.is_empty() { + return div() + .size_full() + .flex() + .items_center() + .justify_center() + .text_color(cx.theme().muted_foreground) + .child("This commit changes nothing.") + .into_any_element(); + } + + Editor::new(diff_editor) + .appearance(false) + .bordered(false) + .readonly(true) + .font_family(cx.theme().mono_font_family.clone()) + .text_size(cx.theme().mono_font_size) + .size_full() + .into_any_element() +} diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs new file mode 100644 index 0000000..55f3b51 --- /dev/null +++ b/crates/ui/src/detail/diff/model.rs @@ -0,0 +1,202 @@ +use domain::{DiffLine, FilePatch, LineOrigin, Patch}; + +use crate::detail::format; + +#[allow(dead_code)] +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) enum Row { + FileHeader { + path: String, + stat: String, + }, + HunkHeader { + text: String, + }, + Line { + origin: LineOrigin, + old_number: Option, + new_number: Option, + content: String, + }, + Placeholder { + message: &'static str, + }, +} + +#[allow(dead_code)] +pub(super) fn rows(patch: &Patch) -> Vec { + let mut rows = Vec::new(); + for file in &patch.files { + rows.push(Row::FileHeader { + path: file + .display_path() + .map(|p| p.display().to_string()) + .unwrap_or_default(), + stat: file_stat(file), + }); + push_body(&mut rows, file); + } + rows +} + +#[allow(dead_code)] +pub(super) fn file_stat(file: &FilePatch) -> String { + format!("+{} \u{2212}{}", file.added_lines(), file.deleted_lines()) +} + +#[allow(dead_code)] +fn push_body(rows: &mut Vec, file: &FilePatch) { + if file.is_binary { + rows.push(Row::Placeholder { + message: "Binary file not shown.", + }); + return; + } + if file.hunks.is_empty() { + rows.push(Row::Placeholder { + message: "No content changes.", + }); + return; + } + for hunk in &file.hunks { + rows.push(Row::HunkHeader { + text: format::hunk_heading(hunk), + }); + rows.extend(hunk.lines.iter().map(line_row)); + } +} + +#[allow(dead_code)] +fn line_row(line: &DiffLine) -> Row { + Row::Line { + origin: line.origin, + old_number: line.old_number, + new_number: line.new_number, + content: line.content.clone(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use domain::{FileStatus, Hunk}; + use std::path::PathBuf; + + fn line(origin: LineOrigin, old: Option, new: Option, content: &str) -> DiffLine { + DiffLine { + origin, + old_number: old, + new_number: new, + content: content.to_string(), + } + } + + fn file(hunks: Vec, is_binary: bool) -> FilePatch { + FilePatch { + old_path: Some(PathBuf::from("src/main.rs")), + new_path: Some(PathBuf::from("src/main.rs")), + status: FileStatus::Modified, + is_binary, + hunks, + } + } + + fn hunk(lines: Vec) -> Hunk { + Hunk { + old_start: 1, + old_lines: 1, + new_start: 1, + new_lines: 1, + heading: String::new(), + lines, + } + } + + #[test] + fn a_modified_file_yields_a_header_a_hunk_header_and_one_row_per_line() { + let patch = Patch { + files: vec![file( + vec![hunk(vec![ + line(LineOrigin::Context, Some(1), Some(1), "keep"), + line(LineOrigin::Deletion, Some(2), None, "gone"), + line(LineOrigin::Addition, None, Some(2), "new"), + ])], + false, + )], + }; + + let rows = rows(&patch); + + assert!(matches!(rows[0], Row::FileHeader { .. })); + assert!(matches!(rows[1], Row::HunkHeader { .. })); + assert_eq!(rows.len(), 5); + } + + #[test] + fn a_line_row_carries_both_numbers_and_the_bare_content() { + let patch = Patch { + files: vec![file( + vec![hunk(vec![line( + LineOrigin::Deletion, + Some(7), + None, + "-not a marker", + )])], + false, + )], + }; + + let rows = rows(&patch); + + assert_eq!( + rows[2], + Row::Line { + origin: LineOrigin::Deletion, + old_number: Some(7), + new_number: None, + content: "-not a marker".to_string(), + }, + "content is stored bare by the parser and must not be re-marked here" + ); + } + + #[test] + fn a_binary_file_yields_a_placeholder_instead_of_lines() { + let patch = Patch { + files: vec![file(Vec::new(), true)], + }; + let rows = rows(&patch); + assert_eq!( + rows[1], + Row::Placeholder { + message: "Binary file not shown." + } + ); + } + + #[test] + fn a_file_with_no_hunks_yields_a_no_change_placeholder() { + let patch = Patch { + files: vec![file(Vec::new(), false)], + }; + let rows = rows(&patch); + assert_eq!( + rows[1], + Row::Placeholder { + message: "No content changes." + } + ); + } + + #[test] + fn every_file_contributes_its_own_header() { + let patch = Patch { + files: vec![file(Vec::new(), true), file(Vec::new(), true)], + }; + let headers = rows(&patch) + .iter() + .filter(|r| matches!(r, Row::FileHeader { .. })) + .count(); + assert_eq!(headers, 2); + } +} From a711991cce1d64227c134ca31c59738c25a62dbb Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:28:58 +0200 Subject: [PATCH 05/19] fix(diff): restore original doc comments and use single mod-level dead_code allow --- crates/ui/src/detail/diff/mod.rs | 43 +++++++++++++++--------------- crates/ui/src/detail/diff/model.rs | 5 ---- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 024b24e..5bf8071 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -1,25 +1,26 @@ -mod model; +//! Renders a `Patch` inside a single readonly code editor, instead of one hand-built +//! `div` per diff line. +//! +//! A hand-built row per line means every element for the whole patch is constructed on +//! every render, selects nothing, and highlights nothing beyond a full-width background +//! tint. Feeding [`super::format::unified_diff_text`] into a real +//! [`gpui_component::input::Editor`] gets text selection, `tree-sitter-diff` syntax +//! highlighting, and virtualised scrolling that only lays out the lines actually on +//! screen — for free, from the editor. +//! +//! One [`Editor`] holds the whole patch rather than one per file: a diff reads as a +//! single continuous document (this is how `git diff` and a GitHub raw patch view both +//! present it), a single scrollbar matches the rest of the panel, and it avoids creating +//! and tearing down one [`EditorState`] entity per file on every commit selection. The +//! trade-off is that per-file collapsing isn't available; nothing in this panel asks for +//! it. +//! +//! [`super::DetailPanel`] owns the [`EditorState`] entity and is the one that pushes new +//! text into it — this module only builds the element each render, exactly like the rest +//! of the panel's view functions. -// Renders a `Patch` inside a single readonly code editor, instead of one hand-built -// `div` per diff line. -// -// A hand-built row per line means every element for the whole patch is constructed on -// every render, selects nothing, and highlights nothing beyond a full-width background -// tint. Feeding [`super::format::unified_diff_text`] into a real -// [`gpui_component::input::Editor`] gets text selection, `tree-sitter-diff` syntax -// highlighting, and virtualised scrolling that only lays out the lines actually on -// screen — for free, from the editor. -// -// One [`Editor`] holds the whole patch rather than one per file: a diff reads as a -// single continuous document (this is how `git diff` and a GitHub raw patch view both -// present it), a single scrollbar matches the rest of the panel, and it avoids creating -// and tearing down one [`EditorState`] entity per file on every commit selection. The -// trade-off is that per-file collapsing isn't available; nothing in this panel asks for -// it. -// -// [`super::DetailPanel`] owns the [`EditorState`] entity and is the one that pushes new -// text into it — this module only builds the element each render, exactly like the rest -// of the panel's view functions. +#[allow(dead_code)] +mod model; use domain::Patch; use gpui::{AnyElement, App, Entity, IntoElement, ParentElement as _, Styled as _, div}; diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index 55f3b51..41cd8e7 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -2,7 +2,6 @@ use domain::{DiffLine, FilePatch, LineOrigin, Patch}; use crate::detail::format; -#[allow(dead_code)] #[derive(Clone, Debug, PartialEq, Eq)] pub(super) enum Row { FileHeader { @@ -23,7 +22,6 @@ pub(super) enum Row { }, } -#[allow(dead_code)] pub(super) fn rows(patch: &Patch) -> Vec { let mut rows = Vec::new(); for file in &patch.files { @@ -39,12 +37,10 @@ pub(super) fn rows(patch: &Patch) -> Vec { rows } -#[allow(dead_code)] pub(super) fn file_stat(file: &FilePatch) -> String { format!("+{} \u{2212}{}", file.added_lines(), file.deleted_lines()) } -#[allow(dead_code)] fn push_body(rows: &mut Vec, file: &FilePatch) { if file.is_binary { rows.push(Row::Placeholder { @@ -66,7 +62,6 @@ fn push_body(rows: &mut Vec, file: &FilePatch) { } } -#[allow(dead_code)] fn line_row(line: &DiffLine) -> Row { Row::Line { origin: line.origin, From 1bfdb16c4b9557739454d525feee3510b5a495cc Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:32:17 +0200 Subject: [PATCH 06/19] feat(diff): pair deletions with additions for a side-by-side view --- crates/ui/src/detail/diff/mod.rs | 2 + crates/ui/src/detail/diff/pairing.rs | 182 +++++++++++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 crates/ui/src/detail/diff/pairing.rs diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 5bf8071..ef4e2ca 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -21,6 +21,8 @@ #[allow(dead_code)] mod model; +#[allow(dead_code)] +mod pairing; use domain::Patch; use gpui::{AnyElement, App, Entity, IntoElement, ParentElement as _, Styled as _, div}; diff --git a/crates/ui/src/detail/diff/pairing.rs b/crates/ui/src/detail/diff/pairing.rs new file mode 100644 index 0000000..ca81122 --- /dev/null +++ b/crates/ui/src/detail/diff/pairing.rs @@ -0,0 +1,182 @@ +use domain::{DiffLine, LineOrigin}; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct SideLine { + pub number: Option, + pub origin: LineOrigin, + pub content: String, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(super) struct SplitRow { + pub left: Option, + pub right: Option, +} + +pub(super) fn pair(lines: &[DiffLine]) -> Vec { + let mut rows = Vec::new(); + let mut deletions: Vec<&DiffLine> = Vec::new(); + let mut additions: Vec<&DiffLine> = Vec::new(); + + for line in lines { + match line.origin { + LineOrigin::Deletion => deletions.push(line), + LineOrigin::Addition => additions.push(line), + LineOrigin::Context => { + flush(&mut rows, &mut deletions, &mut additions); + rows.push(SplitRow { + left: Some(side(line, line.old_number)), + right: Some(side(line, line.new_number)), + }); + } + } + } + flush(&mut rows, &mut deletions, &mut additions); + rows +} + +fn flush(rows: &mut Vec, deletions: &mut Vec<&DiffLine>, additions: &mut Vec<&DiffLine>) { + let paired = deletions.len().max(additions.len()); + for index in 0..paired { + rows.push(SplitRow { + left: deletions.get(index).map(|line| side(line, line.old_number)), + right: additions.get(index).map(|line| side(line, line.new_number)), + }); + } + deletions.clear(); + additions.clear(); +} + +fn side(line: &DiffLine, number: Option) -> SideLine { + SideLine { + number, + origin: line.origin, + content: line.content.clone(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn line(origin: LineOrigin, old: Option, new: Option, content: &str) -> DiffLine { + DiffLine { + origin, + old_number: old, + new_number: new, + content: content.to_string(), + } + } + + fn deletion(number: u32, content: &str) -> DiffLine { + line(LineOrigin::Deletion, Some(number), None, content) + } + + fn addition(number: u32, content: &str) -> DiffLine { + line(LineOrigin::Addition, None, Some(number), content) + } + + fn context(number: u32, content: &str) -> DiffLine { + line(LineOrigin::Context, Some(number), Some(number), content) + } + + #[test] + fn a_context_line_is_the_same_on_both_sides() { + let rows = pair(&[context(1, "keep")]); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].left.as_ref().map(|s| s.content.as_str()), + Some("keep") + ); + assert_eq!( + rows[0].right.as_ref().map(|s| s.content.as_str()), + Some("keep") + ); + } + + #[test] + fn an_equal_length_replacement_pairs_line_for_line() { + let rows = pair(&[ + deletion(1, "a"), + deletion(2, "b"), + addition(1, "x"), + addition(2, "y"), + ]); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0].left.as_ref().map(|s| s.content.as_str()), Some("a")); + assert_eq!( + rows[0].right.as_ref().map(|s| s.content.as_str()), + Some("x") + ); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("b")); + assert_eq!( + rows[1].right.as_ref().map(|s| s.content.as_str()), + Some("y") + ); + } + + #[test] + fn more_additions_than_deletions_pads_the_left() { + let rows = pair(&[deletion(1, "a"), addition(1, "x"), addition(2, "y")]); + assert_eq!(rows.len(), 2); + assert!( + rows[1].left.is_none(), + "the extra addition has nothing to pair with" + ); + assert_eq!( + rows[1].right.as_ref().map(|s| s.content.as_str()), + Some("y") + ); + } + + #[test] + fn more_deletions_than_additions_pads_the_right() { + let rows = pair(&[deletion(1, "a"), deletion(2, "b"), addition(1, "x")]); + assert_eq!(rows.len(), 2); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("b")); + assert!(rows[1].right.is_none()); + } + + #[test] + fn a_pure_addition_leaves_the_left_side_empty() { + let rows = pair(&[addition(1, "x"), addition(2, "y")]); + assert_eq!(rows.len(), 2); + assert!(rows.iter().all(|row| row.left.is_none())); + } + + #[test] + fn a_pure_deletion_leaves_the_right_side_empty() { + let rows = pair(&[deletion(1, "a")]); + assert_eq!(rows.len(), 1); + assert!(rows[0].right.is_none()); + } + + #[test] + fn a_run_is_flushed_when_a_context_line_ends_it() { + let rows = pair(&[deletion(1, "a"), addition(1, "x"), context(2, "keep")]); + assert_eq!(rows.len(), 2); + assert_eq!( + rows[1].left.as_ref().map(|s| s.content.as_str()), + Some("keep") + ); + } + + #[test] + fn a_run_at_the_very_end_is_flushed_without_trailing_context() { + let rows = pair(&[context(1, "keep"), deletion(2, "a")]); + assert_eq!(rows.len(), 2, "the trailing run must not be dropped"); + assert_eq!(rows[1].left.as_ref().map(|s| s.content.as_str()), Some("a")); + } + + #[test] + fn an_empty_hunk_yields_no_rows() { + assert!(pair(&[]).is_empty()); + } + + #[test] + fn a_side_line_keeps_its_own_number() { + let rows = pair(&[deletion(7, "a"), addition(9, "x")]); + assert_eq!(rows[0].left.as_ref().and_then(|s| s.number), Some(7)); + assert_eq!(rows[0].right.as_ref().and_then(|s| s.number), Some(9)); + } +} From 1b287a04e19984c2427f5d296d95c2a05eedec52 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 13:37:57 +0200 Subject: [PATCH 07/19] feat(diff): give rows their own palette, carrying the contrast reasoning over --- crates/ui/src/detail/diff/mod.rs | 2 + crates/ui/src/detail/diff/palette.rs | 74 ++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 crates/ui/src/detail/diff/palette.rs diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index ef4e2ca..7a6bc5b 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -23,6 +23,8 @@ mod model; #[allow(dead_code)] mod pairing; +#[allow(dead_code)] +mod palette; use domain::Patch; use gpui::{AnyElement, App, Entity, IntoElement, ParentElement as _, Styled as _, div}; diff --git a/crates/ui/src/detail/diff/palette.rs b/crates/ui/src/detail/diff/palette.rs new file mode 100644 index 0000000..2135aa2 --- /dev/null +++ b/crates/ui/src/detail/diff/palette.rs @@ -0,0 +1,74 @@ +use domain::LineOrigin; +use gpui::{Hsla, rgb}; +use gpui_component::{ThemeColor, ThemeMode}; + +const LIGHT_ADDITION_BACKGROUND: u32 = 0xdafbe1; +const LIGHT_DELETION_BACKGROUND: u32 = 0xffebe9; + +/// A tint has to clear two bars, and only one of them is legibility. +/// +/// The light pair is unreadable under Catppuccin Frappé — its addition syntax colour +/// `#a6d189` sits at 1.56:1 against [`LIGHT_ADDITION_BACKGROUND`], pale on pale. But an +/// earlier dark green picked purely for legibility against that text landed at 1.00:1 +/// against Frappé's own `#303446` background: identical luminance, so the band was +/// invisible and the tint may as well not have been drawn. This value clears both — 1.58:1 +/// against the background so the band reads, 4.50:1 under the text so the code stays +/// legible on it. +const DARK_ADDITION_BACKGROUND: u32 = 0x355a40; + +/// Mirrors [`DARK_ADDITION_BACKGROUND`]'s two-bar reasoning for Frappé's deletion syntax +/// colour `#e78284`: 1.30:1 against the background, 3.57:1 under the text. Red text is +/// lighter than green here, so the two bars pull harder against each other and this sits +/// where they meet. +const DARK_DELETION_BACKGROUND: u32 = 0x5f3c45; + +pub(super) struct LineColors { + pub background: Option, + pub foreground: Hsla, +} + +pub(super) fn line_colors(origin: LineOrigin, mode: ThemeMode, theme: &ThemeColor) -> LineColors { + let background = match (origin, mode.is_dark()) { + (LineOrigin::Context, _) => None, + (LineOrigin::Addition, false) => Some(rgb(LIGHT_ADDITION_BACKGROUND).into()), + (LineOrigin::Addition, true) => Some(rgb(DARK_ADDITION_BACKGROUND).into()), + (LineOrigin::Deletion, false) => Some(rgb(LIGHT_DELETION_BACKGROUND).into()), + (LineOrigin::Deletion, true) => Some(rgb(DARK_DELETION_BACKGROUND).into()), + }; + LineColors { + background, + foreground: theme.foreground, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_context_line_has_no_background() { + let theme = ThemeColor::light(); + assert!( + line_colors(LineOrigin::Context, ThemeMode::Light, &theme) + .background + .is_none() + ); + } + + #[test] + fn an_addition_and_a_deletion_do_not_share_a_background() { + let theme = ThemeColor::light(); + let added = line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background; + let deleted = line_colors(LineOrigin::Deletion, ThemeMode::Light, &theme).background; + assert!(added.is_some() && deleted.is_some()); + assert_ne!(added, deleted); + } + + #[test] + fn dark_mode_does_not_reuse_the_light_pair() { + let theme = ThemeColor::dark(); + let light = line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background; + let dark = line_colors(LineOrigin::Addition, ThemeMode::Dark, &theme).background; + assert_ne!(light, dark); + } +} From 996cf6f6a01cc58264e4a0ae62dbf07189e8d1bf Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 14:10:37 +0200 Subject: [PATCH 08/19] feat(diff): render the diff as rows with selectable code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The editor fixed three things this view has to control: its gutter is always `buffer_line + 1` in one column, so a GitHub old/new pair is unreachable; its only full-width row fill is the cursor line, so an addition's tint hugged the glyphs and a blank added line got none; and it has no per-line element hook to work around either. The `+`/`-` markers were carrying the signal the colour only half carried. All three follow from the diff being one text document, so `DiffBody` paints the rows itself. Selection comes back from gpui-base's window-level participant system, which `Root` already mounts. The body registers one participant and declares one run per row, and only the code text becomes a run — the gutters and the marker are shaped and painted directly, which is what keeps line numbers and markers out of the clipboard. Two things that look like details decide whether it works at all. A participant's runs are concatenated with *no* separator when the window resolves a copy; only whole participants are joined by a newline. One participant per row would give the right separator and cost an `Entity` per line, so the join is done here and handed back through `set_fallback_copy_text`. `update_runs` is what sets the projection; `set_fallback_copy_text` clears it right back off, which is what lets `copy_item` fall through to our joined text instead of the unseparated one. The span runs from the first selected row to the last rather than over the selected rows alone, because a blank row inside a selection projects to no range and dropping it would close a gap the user can see. And every row is laid out against the widest row's measured width, not the viewport's. `StyledText` wraps to the bounds it is given, and a wrapped row is taller than `ROW_HEIGHT` — which every position in this element is derived from. The container scrolls both axes over that width. `restrict_scroll_to_axis` earns its place for a narrower reason than it looks: both axes are already `Overflow::Scroll` here, so gpui's vertical-onto-horizontal remap never applies with or without the flag; what the flag does is axis-lock a precise trackpad gesture that would otherwise drift diagonally. `TextLayout::bounds`, `line_height`, `position_for_index` and `index_for_position` all unwrap a cell filled during prepaint, so they are called from `paint` and nowhere earlier. `format`'s text reconstruction and the decorations it fed go with the editor. --- Cargo.lock | 1 + Cargo.toml | 1 + crates/ui/Cargo.toml | 1 + crates/ui/src/detail/decorations.rs | 138 ----- crates/ui/src/detail/diff/body.rs | 477 ++++++++++++++++++ crates/ui/src/detail/diff/mod.rs | 99 +++- crates/ui/src/detail/format.rs | 380 +------------- crates/ui/src/detail/mod.rs | 92 ++-- ...026-08-29-github-style-diff-view-design.md | 21 +- 9 files changed, 614 insertions(+), 596 deletions(-) delete mode 100644 crates/ui/src/detail/decorations.rs create mode 100644 crates/ui/src/detail/diff/body.rs diff --git a/Cargo.lock b/Cargo.lock index 344412c..366190b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2311,6 +2311,7 @@ dependencies = [ "gitr-graph", "gitr-vcs", "gpui", + "gpui-base", "gpui-component", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index 96adb4b..abcb2e1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ gpui = { git = "https://github.com/zed-industries/zed" } gpui_platform = { git = "https://github.com/zed-industries/zed", features = ["font-kit"] } gpui-component = { git = "https://github.com/longbridge/gpui-component", features = ["tree-sitter", "tree-sitter-diff", "tree-sitter-rust"] } gpui-component-assets = { git = "https://github.com/longbridge/gpui-component" } +gpui-base = { git = "https://github.com/longbridge/gpui-component" } gix = "0.86" notify = "8.2" diff --git a/crates/ui/Cargo.toml b/crates/ui/Cargo.toml index 6fe506c..21922ab 100644 --- a/crates/ui/Cargo.toml +++ b/crates/ui/Cargo.toml @@ -17,6 +17,7 @@ graph.workspace = true vcs.workspace = true gpui.workspace = true gpui-component.workspace = true +gpui-base.workspace = true anyhow.workspace = true serde.workspace = true serde_json.workspace = true diff --git a/crates/ui/src/detail/decorations.rs b/crates/ui/src/detail/decorations.rs deleted file mode 100644 index 077480d..0000000 --- a/crates/ui/src/detail/decorations.rs +++ /dev/null @@ -1,138 +0,0 @@ -//! Line-background colours for the diff editor, and the [`TextDecoration`]s that paint -//! them onto [`format::DiffLineRanges`]. -//! -//! The theme's syntax styles cannot express a background — [`gpui_component::ThemeStyle`] -//! carries only `color`, `font_style` and `font_weight` — so a per-line background has to -//! go through the editor's decorations collection instead (see `super::mod` for why). -//! [`LIGHT_ADDITION_BACKGROUND`] and [`LIGHT_DELETION_BACKGROUND`] are the exact values -//! given for the light theme; [`DARK_ADDITION_BACKGROUND`] and [`DARK_DELETION_BACKGROUND`] -//! are this module's own pair; see [`line_backgrounds`] for why dark needs one. - -use gpui::{HighlightStyle, Hsla, rgb}; -use gpui_component::ThemeMode; -use gpui_component::input::TextDecoration; - -use super::format::DiffLineRanges; - -const LIGHT_ADDITION_BACKGROUND: u32 = 0xdafbe1; -const LIGHT_DELETION_BACKGROUND: u32 = 0xffebe9; - -/// A tint has to clear two bars, and only one of them is legibility. -/// -/// The light pair is unreadable under Catppuccin Frappé — its addition syntax colour -/// `#a6d189` sits at 1.56:1 against [`LIGHT_ADDITION_BACKGROUND`], pale on pale. But an -/// earlier dark green picked purely for legibility against that text landed at 1.00:1 -/// against Frappé's own `#303446` background: identical luminance, so the band was -/// invisible and the tint may as well not have been drawn. This value clears both — 1.58:1 -/// against the background so the band reads, 4.50:1 under the text so the code stays -/// legible on it. -const DARK_ADDITION_BACKGROUND: u32 = 0x355a40; - -/// Mirrors [`DARK_ADDITION_BACKGROUND`]'s two-bar reasoning for Frappé's deletion syntax -/// colour `#e78284`: 1.30:1 against the background, 3.57:1 under the text. Red text is -/// lighter than green here, so the two bars pull harder against each other and this sits -/// where they meet. -const DARK_DELETION_BACKGROUND: u32 = 0x5f3c45; - -pub(super) struct LineBackgrounds { - pub added: Hsla, - pub deleted: Hsla, -} - -pub(super) fn line_backgrounds(mode: ThemeMode) -> LineBackgrounds { - let (added, deleted) = if mode.is_dark() { - (DARK_ADDITION_BACKGROUND, DARK_DELETION_BACKGROUND) - } else { - (LIGHT_ADDITION_BACKGROUND, LIGHT_DELETION_BACKGROUND) - }; - LineBackgrounds { - added: rgb(added).into(), - deleted: rgb(deleted).into(), - } -} - -pub(super) fn build_decorations( - ranges: &DiffLineRanges, - colors: &LineBackgrounds, -) -> Vec { - let added_style = HighlightStyle { - background_color: Some(colors.added), - ..Default::default() - }; - let deleted_style = HighlightStyle { - background_color: Some(colors.deleted), - ..Default::default() - }; - - ranges - .additions - .iter() - .cloned() - .map(|range| TextDecoration::new(range, added_style)) - .chain( - ranges - .deletions - .iter() - .cloned() - .map(|range| TextDecoration::new(range, deleted_style)), - ) - .collect() -} - -#[cfg(test)] -mod tests { - use super::*; - - fn approx_eq(a: gpui::Rgba, b: gpui::Rgba) -> bool { - (a.r - b.r).abs() < 1e-3 - && (a.g - b.g).abs() < 1e-3 - && (a.b - b.b).abs() < 1e-3 - && (a.a - b.a).abs() < 1e-3 - } - - #[test] - fn light_mode_uses_the_exact_given_hex_values() { - let colors = line_backgrounds(ThemeMode::Light); - assert!(approx_eq( - colors.added.into(), - rgb(LIGHT_ADDITION_BACKGROUND) - )); - assert!(approx_eq( - colors.deleted.into(), - rgb(LIGHT_DELETION_BACKGROUND) - )); - } - - #[test] - fn dark_mode_does_not_reuse_the_light_pair() { - let light = line_backgrounds(ThemeMode::Light); - let dark = line_backgrounds(ThemeMode::Dark); - assert!(!approx_eq(dark.added.into(), light.added.into())); - assert!(!approx_eq(dark.deleted.into(), light.deleted.into())); - } - - #[test] - fn build_decorations_pairs_each_range_with_its_own_background() { - let ranges = DiffLineRanges { - additions: vec![0..3, 10..14], - deletions: vec![5..8, 15..18], - }; - let colors = line_backgrounds(ThemeMode::Light); - - let decorations = build_decorations(&ranges, &colors); - - assert_eq!(decorations.len(), 4); - let additions: Vec<_> = decorations - .iter() - .filter(|d| d.style.background_color == Some(colors.added)) - .map(|d| d.range.clone()) - .collect(); - let deletions: Vec<_> = decorations - .iter() - .filter(|d| d.style.background_color == Some(colors.deleted)) - .map(|d| d.range.clone()) - .collect(); - assert_eq!(additions, ranges.additions); - assert_eq!(deletions, ranges.deletions); - } -} diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs new file mode 100644 index 0000000..da5e2a0 --- /dev/null +++ b/crates/ui/src/detail/diff/body.rs @@ -0,0 +1,477 @@ +use std::ops::Range; + +use domain::LineOrigin; +use gpui::{ + App, Bounds, Element, ElementId, FlexDirection, GlobalElementId, HighlightStyle, Hitbox, + HitboxBehavior, Hsla, InspectorElementId, IntoElement, LayoutId, Length, Pixels, Point, + ScrollHandle, ShapedLine, SharedString, Style, StyledText, TextAlign, TextLayout, TextStyle, + Window, fill, point, px, relative, size, +}; +use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; +use gpui_component::{ThemeColor, ThemeMode}; + +use super::model::Row; +use super::palette::line_colors; + +pub(super) const ROW_HEIGHT: f32 = 18.; + +const GUTTER_WIDTH: f32 = 44.; +const MARKER_WIDTH: f32 = 16.; +const GUTTER_PADDING: f32 = 8.; +const MARKER_PADDING: f32 = 5.; +const CODE_LEFT: f32 = 2. * GUTTER_WIDTH + MARKER_WIDTH; +const TRAILING_SPACE: f32 = 16.; + +pub(super) struct DiffBody { + rows: Vec, + strings: Vec, + texts: Vec, + selection: TextSelectionHandle, + scroll: ScrollHandle, + theme: ThemeColor, + mode: ThemeMode, + row_bounds: Vec>, + visible: Range, +} + +pub(super) fn body( + rows: Vec, + selection: TextSelectionHandle, + scroll: ScrollHandle, + theme: ThemeColor, + mode: ThemeMode, +) -> DiffBody { + let strings: Vec = rows.iter().map(row_text).collect(); + let texts = rows + .iter() + .zip(&strings) + .map(|(row, text)| styled_row(row, text.clone(), &theme, mode)) + .collect(); + DiffBody { + rows, + strings, + texts, + selection, + scroll, + theme, + mode, + row_bounds: Vec::new(), + visible: 0..0, + } +} + +fn row_text(row: &Row) -> SharedString { + match row { + Row::FileHeader { path, stat } => format!("{path} {stat}").into(), + Row::HunkHeader { text } => text.clone().into(), + Row::Line { content, .. } => content.clone().into(), + Row::Placeholder { message } => (*message).into(), + } +} + +fn styled_row(row: &Row, text: SharedString, theme: &ThemeColor, mode: ThemeMode) -> StyledText { + let range = 0..text.len(); + let highlight = HighlightStyle { + color: Some(row_foreground(row, theme, mode)), + ..Default::default() + }; + StyledText::new(text).with_highlights([(range, highlight)]) +} + +fn row_foreground(row: &Row, theme: &ThemeColor, mode: ThemeMode) -> Hsla { + match row { + Row::FileHeader { .. } => theme.foreground, + Row::HunkHeader { .. } | Row::Placeholder { .. } => theme.muted_foreground, + Row::Line { origin, .. } => line_colors(*origin, mode, theme).foreground, + } +} + +fn row_background(row: &Row, theme: &ThemeColor, mode: ThemeMode) -> Option { + match row { + Row::FileHeader { .. } => Some(theme.secondary), + Row::HunkHeader { .. } => Some(theme.muted), + Row::Placeholder { .. } => None, + Row::Line { origin, .. } => line_colors(*origin, mode, theme).background, + } +} + +fn marker(origin: LineOrigin) -> &'static str { + match origin { + LineOrigin::Addition => "+", + LineOrigin::Deletion => "\u{2212}", + LineOrigin::Context => " ", + } +} + +fn selection_quad_bounds( + start: Point, + end: Point, + bounds: Bounds, + line_height: Pixels, +) -> Vec> { + if start.y == end.y { + return vec![Bounds::from_corners( + start, + Point::new(end.x, end.y + line_height), + )]; + } + + let mut quads = vec![Bounds::from_corners( + start, + Point::new(bounds.right(), start.y + line_height), + )]; + if end.y > start.y + line_height { + quads.push(Bounds::from_corners( + Point::new(bounds.left(), start.y + line_height), + Point::new(bounds.right(), end.y), + )); + } + quads.push(Bounds::from_corners( + Point::new(bounds.left(), end.y), + Point::new(end.x, end.y + line_height), + )); + quads +} + +fn copy_text(strings: &[SharedString], ranges: &[Option>]) -> String { + let (Some(first), Some(last)) = ( + ranges.iter().position(Option::is_some), + ranges.iter().rposition(Option::is_some), + ) else { + return String::new(); + }; + + strings[first..=last] + .iter() + .zip(&ranges[first..=last]) + .map(|(text, range)| match range { + Some(range) => &text[range.clone()], + None => "", + }) + .collect::>() + .join("\n") +} + +fn paint_selection(layout: &TextLayout, range: Range, color: Hsla, window: &mut Window) { + let (Some(start), Some(end)) = ( + layout.position_for_index(range.start), + layout.position_for_index(range.end), + ) else { + return; + }; + for bounds in selection_quad_bounds(start, end, layout.bounds(), layout.line_height()) { + window.paint_quad(fill(bounds, color)); + } +} + +struct Pen { + style: TextStyle, + font_size: Pixels, +} + +impl Pen { + fn new(window: &Window) -> Self { + let style = window.text_style(); + let font_size = style.font_size.to_pixels(window.rem_size()); + Self { style, font_size } + } + + fn shape(&self, text: SharedString, color: Hsla, window: &Window) -> ShapedLine { + let mut run = self.style.to_run(text.len()); + run.color = color; + window + .text_system() + .shape_line(text, self.font_size, &[run], None) + } + + fn width(&self, text: SharedString, window: &Window) -> Pixels { + self.shape(text, self.style.color, window).width() + } +} + +fn paint_line(line: &ShapedLine, origin: Point, window: &mut Window, cx: &mut App) { + let _ = line.paint(origin, px(ROW_HEIGHT), TextAlign::Left, None, window, cx); +} + +fn paint_number( + number: Option, + right: Pixels, + top: Pixels, + color: Hsla, + pen: &Pen, + window: &mut Window, + cx: &mut App, +) { + let Some(number) = number else { + return; + }; + let line = pen.shape(number.to_string().into(), color, window); + let origin = point(right - px(GUTTER_PADDING) - line.width(), top); + paint_line(&line, origin, window, cx); +} + +impl DiffBody { + fn content_width(&self, window: &Window) -> Pixels { + let pen = Pen::new(window); + let mut widest = px(0.); + for text in &self.strings { + widest = widest.max(pen.width(text.clone(), window)); + } + px(CODE_LEFT) + widest + px(TRAILING_SPACE) + } + + fn bounds_for_row(&self, bounds: Bounds, index: usize) -> Bounds { + Bounds::new( + point( + bounds.origin.x + px(CODE_LEFT), + bounds.origin.y + px(index as f32 * ROW_HEIGHT), + ), + size( + (bounds.size.width - px(CODE_LEFT)).max(px(0.)), + px(ROW_HEIGHT), + ), + ) + } + + fn visible_rows(&self) -> Range { + let viewport = self.scroll.bounds().size.height; + if viewport <= px(0.) { + return 0..self.rows.len(); + } + let first = ((-self.scroll.offset().y) / px(ROW_HEIGHT)).floor().max(0.) as usize; + let count = (viewport / px(ROW_HEIGHT)).ceil() as usize + 2; + first.min(self.rows.len())..(first + count).min(self.rows.len()) + } + + fn paint_gutter( + &self, + index: usize, + left: Pixels, + top: Pixels, + pen: &Pen, + window: &mut Window, + cx: &mut App, + ) { + let Row::Line { + origin, + old_number, + new_number, + .. + } = self.rows[index] + else { + return; + }; + + let muted = self.theme.muted_foreground; + paint_number( + old_number, + left + px(GUTTER_WIDTH), + top, + muted, + pen, + window, + cx, + ); + paint_number( + new_number, + left + px(2. * GUTTER_WIDTH), + top, + muted, + pen, + window, + cx, + ); + + let foreground = line_colors(origin, self.mode, &self.theme).foreground; + let line = pen.shape(marker(origin).into(), foreground, window); + paint_line( + &line, + point(left + px(2. * GUTTER_WIDTH + MARKER_PADDING), top), + window, + cx, + ); + } +} + +impl IntoElement for DiffBody { + type Element = Self; + + fn into_element(self) -> Self::Element { + self + } +} + +impl Element for DiffBody { + type RequestLayoutState = (); + type PrepaintState = Hitbox; + + fn id(&self) -> Option { + None + } + + fn source_location(&self) -> Option<&'static std::panic::Location<'static>> { + None + } + + fn request_layout( + &mut self, + _: Option<&GlobalElementId>, + _: Option<&InspectorElementId>, + window: &mut Window, + cx: &mut App, + ) -> (LayoutId, Self::RequestLayoutState) { + let children: Vec = self + .texts + .iter_mut() + .map(|text| text.request_layout(None, None, window, cx).0) + .collect(); + + let width = self.content_width(window); + let style = Style { + flex_direction: FlexDirection::Column, + flex_shrink: 0., + size: size(width.into(), px(self.rows.len() as f32 * ROW_HEIGHT).into()), + min_size: size(relative(1.).into(), Length::Auto), + ..Default::default() + }; + + (window.request_layout(style, children, cx), ()) + } + + fn prepaint( + &mut self, + _: Option<&GlobalElementId>, + _: Option<&InspectorElementId>, + bounds: Bounds, + _: &mut Self::RequestLayoutState, + window: &mut Window, + cx: &mut App, + ) -> Self::PrepaintState { + self.row_bounds = (0..self.rows.len()) + .map(|index| self.bounds_for_row(bounds, index)) + .collect(); + for (text, row_bounds) in self.texts.iter_mut().zip(&self.row_bounds) { + text.prepaint(None, None, *row_bounds, &mut (), window, cx); + } + self.visible = self.visible_rows(); + + let hitbox = window.insert_hitbox(bounds, HitboxBehavior::Normal); + self.selection.register( + TextSelectionRegistration::new(hitbox.clone(), bounds) + .with_document_order(0) + .with_text_bounds(self.row_bounds.clone()), + window, + cx, + ); + hitbox + } + + fn paint( + &mut self, + _: Option<&GlobalElementId>, + _: Option<&InspectorElementId>, + bounds: Bounds, + _: &mut Self::RequestLayoutState, + _: &mut Self::PrepaintState, + window: &mut Window, + cx: &mut App, + ) { + let runs: Vec = self + .strings + .iter() + .enumerate() + .map(|(index, text)| { + TextSelectionRun::new( + text.clone(), + self.texts[index].layout().clone(), + self.row_bounds[index], + ) + .with_document_order(index as u64) + }) + .collect(); + let projection = self.selection.update_runs(&runs, cx); + self.selection + .set_fallback_copy_text(copy_text(&self.strings, projection.ranges()), cx); + + let pen = Pen::new(window); + + for index in self.visible.clone() { + let top = self.row_bounds[index].origin.y; + + if let Some(background) = row_background(&self.rows[index], &self.theme, self.mode) { + let band = Bounds::new( + point(bounds.origin.x, top), + size(bounds.size.width, px(ROW_HEIGHT)), + ); + window.paint_quad(fill(band, background)); + } + + if let Some(range) = projection.ranges().get(index).and_then(Clone::clone) { + paint_selection( + self.texts[index].layout(), + range, + self.theme.selection, + window, + ); + } + + self.paint_gutter(index, bounds.origin.x, top, &pen, window, cx); + + let row_bounds = self.row_bounds[index]; + self.texts[index].paint(None, None, row_bounds, &mut (), &mut (), window, cx); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn copy_text_joins_the_selected_rows_with_newlines() { + let strings = vec![ + SharedString::from("one"), + SharedString::from("two"), + SharedString::from("three"), + ]; + let ranges = vec![Some(1..3), Some(0..3), None]; + assert_eq!(copy_text(&strings, &ranges), "ne\ntwo"); + } + + #[test] + fn copy_text_keeps_a_blank_row_inside_the_selection() { + let strings = vec![ + SharedString::from("one"), + SharedString::from(""), + SharedString::from("three"), + ]; + let ranges = vec![Some(0..3), None, Some(0..5)]; + assert_eq!(copy_text(&strings, &ranges), "one\n\nthree"); + } + + #[test] + fn copy_text_of_an_empty_projection_is_empty() { + let strings = vec![SharedString::from("one")]; + assert_eq!(copy_text(&strings, &[None]), ""); + } + + #[test] + fn a_wrapped_selection_covers_the_full_width_of_the_middle_lines() { + let bounds = Bounds::new(point(px(10.), px(20.)), size(px(100.), px(100.))); + let quads = selection_quad_bounds( + point(px(40.), px(20.)), + point(px(30.), px(80.)), + bounds, + px(20.), + ); + + assert_eq!( + quads, + vec![ + Bounds::from_corners(point(px(40.), px(20.)), point(px(110.), px(40.))), + Bounds::from_corners(point(px(10.), px(40.)), point(px(110.), px(80.))), + Bounds::from_corners(point(px(10.), px(80.)), point(px(30.), px(100.))), + ] + ); + } +} diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 7a6bc5b..16bcc16 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -1,39 +1,66 @@ -//! Renders a `Patch` inside a single readonly code editor, instead of one hand-built -//! `div` per diff line. +//! Renders a `Patch` as rows painted by one custom element, instead of feeding a +//! reconstructed unified diff to a code editor. //! -//! A hand-built row per line means every element for the whole patch is constructed on -//! every render, selects nothing, and highlights nothing beyond a full-width background -//! tint. Feeding [`super::format::unified_diff_text`] into a real -//! [`gpui_component::input::Editor`] gets text selection, `tree-sitter-diff` syntax -//! highlighting, and virtualised scrolling that only lays out the lines actually on -//! screen — for free, from the editor. +//! The editor gave selection, syntax highlighting and virtualised scrolling for free, and +//! it also fixed three things this view needs to control. Its gutter is always +//! `buffer_line + 1` in a single column, so a GitHub old/new pair is unreachable; it has no +//! full-width row background outside the cursor line, so an addition's tint hugs the glyphs +//! and a blank added line gets none at all; and it has no per-line element hook to work +//! around either. The `+`/`-` markers were carrying the signal the colour only half +//! carried. All three follow from the diff being one text document, so the rows are built +//! here instead — see the design note under `docs/superpowers/specs/`. //! -//! One [`Editor`] holds the whole patch rather than one per file: a diff reads as a -//! single continuous document (this is how `git diff` and a GitHub raw patch view both -//! present it), a single scrollbar matches the rest of the panel, and it avoids creating -//! and tearing down one [`EditorState`] entity per file on every commit selection. The -//! trade-off is that per-file collapsing isn't available; nothing in this panel asks for -//! it. +//! Selection comes back through `gpui-base`'s window-level participant system rather than +//! from the editor. [`body`] is the element that joins it: it registers one participant and +//! declares one run per row, and only the code text becomes a run — the gutters and the +//! marker are painted directly and never registered, which is what keeps line numbers and +//! markers out of the clipboard. The rows scroll on both axes rather than soft-wrapping, +//! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the +//! reason it usually does: the container's `overflow_scroll()` puts both axes in +//! `Overflow::Scroll`, and gpui's vertical-onto-horizontal remap (`div.rs:3220-3224`, +//! `:3229-3233`) only fires when one axis is *not* `Overflow::Scroll`, so that remap is +//! unreachable here with or without the flag. What the flag does is axis-lock a precise +//! trackpad gesture through `ongoing_scroll.filter(..)` (`div.rs:3209-3216`), which is live +//! because `track_scroll` populates `ongoing_scroll` from the tracked handle (`div.rs:2165`). //! -//! [`super::DetailPanel`] owns the [`EditorState`] entity and is the one that pushes new -//! text into it — this module only builds the element each render, exactly like the rest -//! of the panel's view functions. +//! Copying depends on a fact `gpui-base`'s own doc comment does not state. A participant's +//! runs concatenate with no separator when `update_runs` projects them +//! (`text_selection.rs:593`); only whole participants are joined with `"\n"` +//! (`resolve_copy_items`, `:513`). So the row separator is computed here, in +//! [`body::copy_text`], and published through `TextSelectionHandle::set_fallback_copy_text` +//! right after `update_runs` — which works only because that setter also clears +//! `projected_copy_text` (`:559`), the field `update_runs` just set, so `copy_item` falls +//! through to our fallback instead of the unseparated projection. `gpui-base` tracks a git +//! default branch pinned solely by `Cargo.lock`; if that clearing behaviour ever moves, +//! copying silently goes back to gluing rows together with no separator, and nothing fails +//! to say so. -#[allow(dead_code)] +mod body; mod model; #[allow(dead_code)] mod pairing; -#[allow(dead_code)] mod palette; use domain::Patch; -use gpui::{AnyElement, App, Entity, IntoElement, ParentElement as _, Styled as _, div}; +use gpui::{ + AnyElement, App, InteractiveElement as _, IntoElement, ParentElement as _, ScrollHandle, + StatefulInteractiveElement as _, Styled as _, div, px, +}; +use gpui_base::TextSelectionHandle; use gpui_component::{ ActiveTheme as _, - input::{Editor, EditorState}, + scroll::{ScrollableElement as _, ScrollbarAxis}, }; -pub(super) fn render(patch: &Patch, diff_editor: &Entity, cx: &App) -> AnyElement { +use body::{ROW_HEIGHT, body}; +use model::rows; + +pub(super) fn render( + patch: &Patch, + selection: &TextSelectionHandle, + scroll: &ScrollHandle, + cx: &App, +) -> AnyElement { if patch.files.is_empty() { return div() .size_full() @@ -45,12 +72,28 @@ pub(super) fn render(patch: &Patch, diff_editor: &Entity, cx: &App) .into_any_element(); } - Editor::new(diff_editor) - .appearance(false) - .bordered(false) - .readonly(true) - .font_family(cx.theme().mono_font_family.clone()) - .text_size(cx.theme().mono_font_size) + let theme = cx.theme(); + div() + .relative() .size_full() + .child( + div() + .id("detail-diff-scroll") + .size_full() + .overflow_scroll() + .restrict_scroll_to_axis() + .track_scroll(scroll) + .font_family(theme.mono_font_family.clone()) + .text_size(theme.mono_font_size) + .line_height(px(ROW_HEIGHT)) + .child(body( + rows(patch), + selection.clone(), + scroll.clone(), + theme.colors, + theme.mode, + )), + ) + .scrollbar(scroll, ScrollbarAxis::Both) .into_any_element() } diff --git a/crates/ui/src/detail/format.rs b/crates/ui/src/detail/format.rs index c0860df..5888f11 100644 --- a/crates/ui/src/detail/format.rs +++ b/crates/ui/src/detail/format.rs @@ -3,20 +3,7 @@ //! Nothing here touches gpui's `App` or `Window`: every function is a plain transformation //! from domain types to strings, so it is testable without a running window. -use std::fmt::Write as _; -use std::ops::Range; -use std::path::PathBuf; - -use domain::{FilePatch, FileStatus, Hunk, LineOrigin, ObjectId, Patch, Timestamp}; - -/// The UTF-8 byte range, into the text [`unified_diff_text_with_line_ranges`] produces, -/// of every added and every deleted line — everything else (file headers, hunk headers, -/// context lines) is left undecorated. -#[derive(Debug, Default, Clone, PartialEq, Eq)] -pub struct DiffLineRanges { - pub additions: Vec>, - pub deletions: Vec>, -} +use domain::{Hunk, ObjectId, Timestamp}; /// Hexadecimal characters kept when an identifier is shown abbreviated, matching Git's /// own default abbreviation length. @@ -105,166 +92,22 @@ pub fn hunk_heading(hunk: &Hunk) -> String { } } -fn git_path(path: Option<&PathBuf>) -> Option { - path.map(|path| path.display().to_string()) -} - -/// The path a `diff --git a/ b/` line shows for one side. -/// -/// Git always prints *some* path on both sides of that line, falling back to the other -/// side's path when this one doesn't exist (an added or deleted file) — the two sides -/// only ever truly differ for a rename or a copy. -fn git_header_path(path: Option<&PathBuf>, other: Option<&PathBuf>) -> String { - git_path(path) - .or_else(|| git_path(other)) - .unwrap_or_default() -} - -/// The `a/` / `b/` / `/dev/null` form Git uses on a `---`, `+++` or `Binary -/// files` line, where — unlike [`git_header_path`] — a missing side stays `/dev/null` -/// rather than borrowing the other side's path. -fn side_path(path: Option<&PathBuf>, prefix: char) -> String { - match git_path(path) { - Some(path) => format!("{prefix}/{path}"), - None => "/dev/null".to_string(), - } -} - -/// Rebuilds the unified diff text `git diff` would print for `patch` — so it can be fed -/// to a real editor for syntax highlighting, selection and virtualised scrolling — paired -/// with the byte range of every added and deleted line, computed in the same pass so the -/// text and the ranges can never disagree about where a line starts. Reconstructing the -/// text and then re-scanning it for `+`/`-` lines would be a second implementation of the -/// same rule, and a line whose own content begins with `+` or `-` is exactly where the -/// two could differ. -/// -/// Mode lines (`new file mode`, `deleted file mode`) are Git prints that [`FilePatch`] has -/// no data for, and are left out rather than fabricated. Each range starts at the line's -/// leading marker byte, read from [`LineOrigin`] structurally rather than from the first -/// byte of `line.content` — a line about a diff can itself start with `+` or `-`. -pub fn unified_diff_text_with_line_ranges(patch: &Patch) -> (String, DiffLineRanges) { - let mut text = String::new(); - let mut ranges = DiffLineRanges::default(); - for file in &patch.files { - write_file(&mut text, file, &mut ranges); - } - (text, ranges) -} - -fn write_file(text: &mut String, file: &FilePatch, ranges: &mut DiffLineRanges) { - let old = file.old_path.as_ref(); - let new = file.new_path.as_ref(); - - let _ = writeln!( - text, - "diff --git a/{} b/{}", - git_header_path(old, new), - git_header_path(new, old) - ); - - match &file.status { - FileStatus::Renamed { similarity } => { - let _ = writeln!(text, "similarity index {similarity}%"); - if let (Some(old), Some(new)) = (git_path(old), git_path(new)) { - let _ = writeln!(text, "rename from {old}"); - let _ = writeln!(text, "rename to {new}"); - } - } - FileStatus::Copied { similarity } => { - let _ = writeln!(text, "similarity index {similarity}%"); - if let (Some(old), Some(new)) = (git_path(old), git_path(new)) { - let _ = writeln!(text, "copy from {old}"); - let _ = writeln!(text, "copy to {new}"); - } - } - FileStatus::Added - | FileStatus::Deleted - | FileStatus::Modified - | FileStatus::TypeChanged => {} - } - - if file.is_binary { - let _ = writeln!( - text, - "Binary files {} and {} differ", - side_path(old, 'a'), - side_path(new, 'b') - ); - return; - } - - if file.hunks.is_empty() { - return; - } - - let _ = writeln!(text, "--- {}", side_path(old, 'a')); - let _ = writeln!(text, "+++ {}", side_path(new, 'b')); - for hunk in &file.hunks { - let _ = writeln!(text, "{}", hunk_heading(hunk)); - for line in &hunk.lines { - let marker = match line.origin { - LineOrigin::Addition => '+', - LineOrigin::Deletion => '-', - LineOrigin::Context => ' ', - }; - let start = text.len(); - let _ = writeln!(text, "{marker}{}", line.content); - let end = text.len() - 1; - match line.origin { - LineOrigin::Addition => ranges.additions.push(start..end), - LineOrigin::Deletion => ranges.deletions.push(start..end), - LineOrigin::Context => {} - } - } - } -} - #[cfg(test)] mod tests { use super::*; - use domain::DiffLine; - - fn unified_diff_text(patch: &Patch) -> String { - unified_diff_text_with_line_ranges(patch).0 - } fn id(nibble: char) -> ObjectId { nibble.to_string().repeat(40).parse().unwrap() } - fn file( - old_path: Option<&str>, - new_path: Option<&str>, - status: FileStatus, - is_binary: bool, - hunks: Vec, - ) -> FilePatch { - FilePatch { - old_path: old_path.map(PathBuf::from), - new_path: new_path.map(PathBuf::from), - status, - is_binary, - hunks, - } - } - - fn hunk(lines: Vec) -> Hunk { + fn hunk() -> Hunk { Hunk { old_start: 1, old_lines: 1, new_start: 1, new_lines: 2, heading: "fn existing()".to_string(), - lines, - } - } - - fn line(origin: LineOrigin, content: &str) -> DiffLine { - DiffLine { - origin, - old_number: None, - new_number: None, - content: content.to_string(), + lines: vec![], } } @@ -311,12 +154,12 @@ mod tests { #[test] fn hunk_heading_includes_the_function_context_when_git_found_one() { - assert_eq!(hunk_heading(&hunk(vec![])), "@@ -1,1 +1,2 @@ fn existing()"); + assert_eq!(hunk_heading(&hunk()), "@@ -1,1 +1,2 @@ fn existing()"); } #[test] fn hunk_heading_omits_the_trailing_space_when_git_found_no_context() { - let mut hunk = hunk(vec![]); + let mut hunk = hunk(); hunk.heading = String::new(); assert_eq!(hunk_heading(&hunk), "@@ -1,1 +1,2 @@"); } @@ -341,217 +184,4 @@ mod tests { "caf\u{e9} \u{2014} r\u{e9}sum\u{e9}" ); } - - #[test] - fn unified_diff_text_of_an_empty_patch_is_empty() { - let patch = Patch { files: vec![] }; - assert_eq!(unified_diff_text(&patch), ""); - } - - #[test] - fn unified_diff_text_reconstructs_a_single_hunk() { - let patch = Patch { - files: vec![file( - Some("src/lib.rs"), - Some("src/lib.rs"), - FileStatus::Modified, - false, - vec![hunk(vec![ - line(LineOrigin::Context, "fn existing() {"), - line(LineOrigin::Deletion, " old();"), - line(LineOrigin::Addition, " new();"), - line(LineOrigin::Addition, " more();"), - ])], - )], - }; - assert_eq!( - unified_diff_text(&patch), - "diff --git a/src/lib.rs b/src/lib.rs\n".to_string() - + "--- a/src/lib.rs\n" - + "+++ b/src/lib.rs\n" - + "@@ -1,1 +1,2 @@ fn existing()\n" - + " fn existing() {\n" - + "- old();\n" - + "+ new();\n" - + "+ more();\n" - ); - } - - #[test] - fn unified_diff_text_uses_dev_null_for_an_added_file() { - let patch = Patch { - files: vec![file( - None, - Some("new.rs"), - FileStatus::Added, - false, - vec![hunk(vec![line(LineOrigin::Addition, "fn new() {}")])], - )], - }; - let text = unified_diff_text(&patch); - assert!(text.starts_with("diff --git a/new.rs b/new.rs\n")); - assert!(text.contains("--- /dev/null\n")); - assert!(text.contains("+++ b/new.rs\n")); - } - - #[test] - fn unified_diff_text_uses_dev_null_for_a_deleted_file() { - let patch = Patch { - files: vec![file( - Some("old.rs"), - None, - FileStatus::Deleted, - false, - vec![hunk(vec![line(LineOrigin::Deletion, "fn old() {}")])], - )], - }; - let text = unified_diff_text(&patch); - assert!(text.starts_with("diff --git a/old.rs b/old.rs\n")); - assert!(text.contains("--- a/old.rs\n")); - assert!(text.contains("+++ /dev/null\n")); - } - - #[test] - fn unified_diff_text_marks_binary_files_without_a_hunk() { - let patch = Patch { - files: vec![file( - Some("image.png"), - Some("image.png"), - FileStatus::Modified, - true, - vec![], - )], - }; - assert_eq!( - unified_diff_text(&patch), - "diff --git a/image.png b/image.png\nBinary files a/image.png and b/image.png differ\n" - ); - } - - #[test] - fn unified_diff_text_includes_rename_headers_and_omits_hunks_for_a_pure_rename() { - let patch = Patch { - files: vec![file( - Some("old.rs"), - Some("new.rs"), - FileStatus::Renamed { similarity: 87 }, - false, - vec![], - )], - }; - assert_eq!( - unified_diff_text(&patch), - "diff --git a/old.rs b/new.rs\n\ - similarity index 87%\n\ - rename from old.rs\n\ - rename to new.rs\n" - ); - } - - #[test] - fn unified_diff_text_includes_copy_headers() { - let patch = Patch { - files: vec![file( - Some("old.rs"), - Some("copy.rs"), - FileStatus::Copied { similarity: 100 }, - false, - vec![], - )], - }; - let text = unified_diff_text(&patch); - assert!(text.contains("copy from old.rs\n")); - assert!(text.contains("copy to copy.rs\n")); - } - - #[test] - fn unified_diff_text_concatenates_multiple_files_in_order() { - let patch = Patch { - files: vec![ - file( - Some("a.rs"), - Some("a.rs"), - FileStatus::Modified, - false, - vec![hunk(vec![line(LineOrigin::Addition, "a")])], - ), - file( - Some("b.rs"), - Some("b.rs"), - FileStatus::Modified, - false, - vec![hunk(vec![line(LineOrigin::Addition, "b")])], - ), - ], - }; - let text = unified_diff_text(&patch); - let a_pos = text.find("diff --git a/a.rs").unwrap(); - let b_pos = text.find("diff --git a/b.rs").unwrap(); - assert!(a_pos < b_pos); - } - - #[test] - fn line_ranges_cover_additions_and_deletions_and_nothing_else() { - let patch = Patch { - files: vec![file( - Some("src/lib.rs"), - Some("src/lib.rs"), - FileStatus::Modified, - false, - vec![hunk(vec![ - line(LineOrigin::Context, "fn existing() {"), - line(LineOrigin::Deletion, " old();"), - line(LineOrigin::Addition, " new();"), - ])], - )], - }; - let (text, ranges) = unified_diff_text_with_line_ranges(&patch); - - assert_eq!(ranges.additions.len(), 1); - assert_eq!(ranges.deletions.len(), 1); - - let addition = &text[ranges.additions[0].clone()]; - assert_eq!(addition, "+ new();"); - - let deletion = &text[ranges.deletions[0].clone()]; - assert_eq!(deletion, "- old();"); - - let file_header_pos = text.find("diff --git").unwrap(); - let hunk_header_pos = text.find("@@").unwrap(); - let context_pos = text.find(" fn existing() {").unwrap(); - for range in ranges.additions.iter().chain(&ranges.deletions) { - assert!(!range.contains(&file_header_pos)); - assert!(!range.contains(&hunk_header_pos)); - assert!(!range.contains(&context_pos)); - } - } - - #[test] - fn line_ranges_are_read_from_the_marker_column_not_the_lines_own_content() { - let patch = Patch { - files: vec![file( - Some("src/lib.rs"), - Some("src/lib.rs"), - FileStatus::Modified, - false, - vec![hunk(vec![ - line(LineOrigin::Context, "+not actually an addition"), - line(LineOrigin::Addition, "-not actually a deletion"), - line(LineOrigin::Deletion, "+not actually an addition either"), - ])], - )], - }; - let (text, ranges) = unified_diff_text_with_line_ranges(&patch); - - assert_eq!(ranges.additions.len(), 1); - assert_eq!(ranges.deletions.len(), 1); - - let addition = &text[ranges.additions[0].clone()]; - assert!(addition.starts_with('+')); - assert_eq!(addition, "+-not actually a deletion"); - - let deletion = &text[ranges.deletions[0].clone()]; - assert!(deletion.starts_with('-')); - assert_eq!(deletion, "-+not actually an addition either"); - } } diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index d908ea4..b51709c 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -4,21 +4,24 @@ //! [`DetailPanel`] renders exactly the [`LoadState`] it is handed — it never reads a //! repository itself. [`metadata::render_header`] and [`metadata::render_description`] //! are the general-information tab's content — Subject, ID, Parents, Author and the -//! commit body. [`format`] holds the logic pulled out of both [`metadata`] and [`diff`] -//! so it can be unit-tested without a window, in particular -//! [`format::unified_diff_text_with_line_ranges`], which reconstructs the text fed to the -//! diff editor together with the byte ranges [`decorations`] turns into line backgrounds. +//! commit body. [`format`] holds the logic pulled out of [`metadata`] and [`diff`] so it +//! can be unit-tested without a window. //! -//! The diff editor is a persistent [`EditorState`] entity owned by [`DetailPanel`] rather -//! than rebuilt every render, because updating it — like creating it — needs a -//! `&mut Window`, which only [`DetailPanel::new`] and [`Render::render`] receive. -//! [`DetailPanel::set_detail`] cannot reach one, so it stages the reconstructed text and -//! ranges in `pending_diff` and [`Render::render`] flushes them into the entity and its -//! decorations collection on the next frame, before building this render pass's element -//! tree. [`DetailPanel::selected_tab`] is never touched by `set_detail`, which is what -//! lets picking a different commit leave the open tab alone. +//! Both tabs keep their scroll position in a [`ScrollHandle`] owned here rather than in +//! the element tree, which is rebuilt every render. The diff's handle is passed down to +//! the row element as well, because that element reads its own scroll offset to decide +//! which rows are worth painting. +//! +//! The one piece of view state that does need to outlive a frame is the diff's selection +//! participant: `gpui-base` keys a window-level selection off a +//! [`TextSelectionHandle`], and a handle rebuilt per frame would drop the selection on +//! every repaint. It is built once in [`DetailPanel::new`], which is also the only place +//! with the `&Window` its refresh subscription needs. Nothing else here is staged: +//! [`DetailPanel::set_detail`] stores the new [`LoadState`] and notifies, and the rows are +//! derived from the patch during the render that follows. +//! [`DetailPanel::selected_tab`] is never touched by `set_detail`, which is what lets +//! picking a different commit leave the open tab alone. -mod decorations; mod diff; mod format; mod metadata; @@ -26,22 +29,21 @@ mod metadata; use std::sync::Arc; use gpui::{ - AnyElement, App, AppContext as _, Context, Entity, EventEmitter, FocusHandle, Focusable, - InteractiveElement as _, IntoElement, ParentElement as _, Render, ScrollHandle, SharedString, - StatefulInteractiveElement as _, Styled as _, Window, div, + AnyElement, App, Context, EventEmitter, FocusHandle, Focusable, InteractiveElement as _, + IntoElement, ParentElement as _, Point, Render, ScrollHandle, StatefulInteractiveElement as _, + Styled as _, Window, div, }; +use gpui_base::TextSelectionHandle; use gpui_component::{ ActiveTheme as _, Sizable as _, alert::Alert, dock::{Panel, PanelEvent}, - input::{EditorState, TextDecorationCollection}, scroll::ScrollableElement as _, spinner::Spinner, tab::{Tab, TabBar}, }; use crate::repository::{CommitDetail, LoadState}; -use format::DiffLineRanges; #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] enum DetailTab { @@ -71,39 +73,30 @@ impl DetailTab { pub struct DetailPanel { detail: LoadState>, - pending_diff: Option<(SharedString, DiffLineRanges)>, - diff_editor: Entity, - diff_decorations: TextDecorationCollection, + diff_selection: TextSelectionHandle, selected_tab: DetailTab, general_scroll_handle: ScrollHandle, + diff_scroll_handle: ScrollHandle, focus_handle: FocusHandle, } impl DetailPanel { pub fn new(window: &mut Window, cx: &mut Context) -> Self { - let diff_editor = cx.new(|cx| EditorState::new(window, cx).language("diff")); - let diff_decorations = diff_editor.update(cx, |state, cx| { - state.create_decorations_collection(Vec::new(), cx) - }); + let diff_selection = TextSelectionHandle::new("", cx); + diff_selection.refresh_window_on_change(window, cx).detach(); Self { detail: LoadState::Idle, - pending_diff: None, - diff_editor, - diff_decorations, + diff_selection, selected_tab: DetailTab::default(), general_scroll_handle: ScrollHandle::new(), + diff_scroll_handle: ScrollHandle::new(), focus_handle: cx.focus_handle(), } } pub fn set_detail(&mut self, detail: LoadState>, cx: &mut Context) { - if let LoadState::Ready(commit_detail) = &detail - && !commit_detail.patch.files.is_empty() - { - let (text, ranges) = format::unified_diff_text_with_line_ranges(&commit_detail.patch); - self.pending_diff = Some((text.into(), ranges)); - } self.detail = detail; + self.diff_scroll_handle.set_offset(Point::default()); cx.notify(); } } @@ -131,15 +124,7 @@ impl Focusable for DetailPanel { } impl Render for DetailPanel { - fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { - if let Some((text, ranges)) = self.pending_diff.take() { - let colors = decorations::line_backgrounds(cx.theme().mode); - let new_decorations = decorations::build_decorations(&ranges, &colors); - self.diff_editor - .update(cx, |state, cx| state.set_value(text, window, cx)); - self.diff_decorations.set(new_decorations, cx); - } - + fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { let selected_tab = self.selected_tab; div() .size_full() @@ -153,8 +138,9 @@ impl Render for DetailPanel { LoadState::Ready(detail) => ready_state( detail, selected_tab, - &self.diff_editor, + &self.diff_selection, &self.general_scroll_handle, + &self.diff_scroll_handle, cx, ), }) @@ -227,13 +213,14 @@ fn failed_state(message: &str) -> AnyElement { fn ready_state( detail: &CommitDetail, selected_tab: DetailTab, - diff_editor: &Entity, - scroll_handle: &ScrollHandle, + diff_selection: &TextSelectionHandle, + general_scroll_handle: &ScrollHandle, + diff_scroll_handle: &ScrollHandle, cx: &App, ) -> AnyElement { match selected_tab { - DetailTab::General => general_tab(detail, scroll_handle, cx), - DetailTab::Diff => diff_tab(detail, diff_editor, cx), + DetailTab::General => general_tab(detail, general_scroll_handle, cx), + DetailTab::Diff => diff_tab(detail, diff_selection, diff_scroll_handle, cx), } } @@ -262,11 +249,16 @@ fn general_tab(detail: &CommitDetail, scroll_handle: &ScrollHandle, cx: &App) -> .into_any_element() } -fn diff_tab(detail: &CommitDetail, diff_editor: &Entity, cx: &App) -> AnyElement { +fn diff_tab( + detail: &CommitDetail, + selection: &TextSelectionHandle, + scroll_handle: &ScrollHandle, + cx: &App, +) -> AnyElement { div() .flex_1() .min_h_0() .min_w_0() - .child(diff::render(&detail.patch, diff_editor, cx)) + .child(diff::render(&detail.patch, selection, scroll_handle, cx)) .into_any_element() } diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md index 001a489..9d410a8 100644 --- a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -137,8 +137,14 @@ The body element registers one participant and declares that frame's visible row in `prepaint`/`paint`: - `TextSelectionRegistration::new(hitbox, bounds)` with `.with_document_order(n)` so a drag - across rows copies in document order, and `.with_scroll_offset(..)` so the geometry stays - correct under scrolling. + across rows copies in document order. `.with_scroll_offset(..)` is **not** called: gpui + prepaints scroll children inside `with_element_offset` (`div.rs:1925`), and + `Window::layout_bounds` folds that accumulated offset into `bounds.origin` + (`window.rs:4697`) — so this element's `bounds.origin` already carries the scroll. + `gpui-base` stores a selection endpoint as `position − bounds.origin − scroll_offset` + (`text_selection.rs:1336`), so reporting the offset as well double-counts it and the + anchor drifts at twice the scroll delta. `with_scroll_offset` is for a participant that + scrolls its own content inside fixed bounds, which this element does not do. - `TextSelectionRun::new(text, layout, bounds)` — **only for the code content**. The gutters and the marker are neighbouring elements and are never registered, which is what makes a copied diff come out as clean code with no line numbers and no markers. This is @@ -222,9 +228,14 @@ than after. Second risk: the coordinate handling. `TextLayout::bounds`, `line_height`, `len`, `position_for_index` and `index_for_position` all panic when called before the text has been laid out — they `unwrap`/`expect` on an inner cell filled during prepaint. They are safe -from `paint` and from nowhere earlier. Combined with hand-rolled windowing and a scroll -offset reported through `with_scroll_offset`, this is where an implementation will go wrong -if it goes wrong. +from `paint` and from nowhere earlier. This is where hand-rolled windowing bites: `prepaint` +today lays out every row and `paint` builds a run for every row, which is only safe while +`prepaint` stays unwindowed. Task 5 narrows `paint` to the visible rows but must not narrow +`prepaint` to match — doing so would leave the off-screen rows' `TextLayout`s unlaid-out, and +the first scroll with a live selection would panic on "prepaint has not been performed". +Narrowing the runs passed to `update_runs` instead avoids the panic but silently truncates a +copy to whatever rows are on screen, which is just as wrong. This is where an implementation +will go wrong if it goes wrong. ## Files touched From 7ffbcac56af639844ae4a9d420b0b1c742226a4b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 14:57:25 +0200 Subject: [PATCH 09/19] perf(diff): lay out only the rows on screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The body built a StyledText for every row of the patch on every frame, then declared a TextSelectionRun for every one of them. On a commit touching hundreds of files that is thousands of laid-out lines per frame for a viewport that shows forty. The window is decided in request_layout rather than in paint, because laying a row out is the expensive half and nothing downstream can be narrowed without it. The ScrollHandle is the only input available before the frame hands the element any bounds; the range is stored, so request_layout, prepaint and paint cannot disagree when the container clamps the offset in its own prepaint. Narrowing layout forces the runs to narrow too: selection_range_for_run reads layout.len() on every run it is handed, and TextLayout::len expects on a cell the measure closure fills, so a row that was skipped this frame cannot be declared at all. Reading the copied text off that projection would then return only the rows that happened to be on screen — a selection dragged past the bottom edge would come back cut short with nothing to say it had been. So the copy is derived from the selection's own window points instead. Those survive scrolling: gpui-base stores an endpoint as position - bounds.origin, and this element's bounds.origin already carries the scroll, so a point above the viewport is a negative y rather than a lost one. The row span is then arithmetic, every row between the two ends is whole, and only the two rows the selection cuts through need shaping. A row already on screen keeps the projection's own range, so the highlight and the clipboard cannot drift apart. TextSelectionContentKey was the obvious lead and turns out to be a channel rather than a mechanism: gpui-base resolves it once from a participant callback and hands it straight back on the snapshot, taking no part in hit-testing, projection or copying. With a fixed ROW_HEIGHT a row index is y / ROW_HEIGHT, which is the identity a key would have carried. content_width still shapes every row. It has to: the horizontal scroll extent must consider rows that are off screen or the scrollbar resizes as the view scrolls vertically, and the children are laid out against a width that provably exceeds every row's natural width, which is what keeps a row one line tall. After the first frame those are hits in gpui's line-layout cache. --- crates/ui/src/detail/diff/body.rs | 297 +++++++++++++++--- crates/ui/src/detail/diff/mod.rs | 34 +- crates/ui/src/detail/mod.rs | 3 +- ...026-08-29-github-style-diff-view-design.md | 33 +- 4 files changed, 317 insertions(+), 50 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index da5e2a0..ed285d8 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -1,9 +1,9 @@ -use std::ops::Range; +use std::ops::{Range, RangeInclusive}; use domain::LineOrigin; use gpui::{ - App, Bounds, Element, ElementId, FlexDirection, GlobalElementId, HighlightStyle, Hitbox, - HitboxBehavior, Hsla, InspectorElementId, IntoElement, LayoutId, Length, Pixels, Point, + App, Bounds, Element, ElementId, FlexDirection, GlobalElementId, Half as _, HighlightStyle, + Hitbox, HitboxBehavior, Hsla, InspectorElementId, IntoElement, LayoutId, Length, Pixels, Point, ScrollHandle, ShapedLine, SharedString, Style, StyledText, TextAlign, TextLayout, TextStyle, Window, fill, point, px, relative, size, }; @@ -25,13 +25,13 @@ const TRAILING_SPACE: f32 = 16.; pub(super) struct DiffBody { rows: Vec, strings: Vec, - texts: Vec, selection: TextSelectionHandle, scroll: ScrollHandle, theme: ThemeColor, mode: ThemeMode, - row_bounds: Vec>, visible: Range, + texts: Vec, + row_bounds: Vec>, } pub(super) fn body( @@ -42,21 +42,16 @@ pub(super) fn body( mode: ThemeMode, ) -> DiffBody { let strings: Vec = rows.iter().map(row_text).collect(); - let texts = rows - .iter() - .zip(&strings) - .map(|(row, text)| styled_row(row, text.clone(), &theme, mode)) - .collect(); DiffBody { rows, strings, - texts, selection, scroll, theme, mode, - row_bounds: Vec::new(), visible: 0..0, + texts: Vec::new(), + row_bounds: Vec::new(), } } @@ -133,6 +128,82 @@ fn selection_quad_bounds( quads } +fn row_window(offset_y: Pixels, viewport: Pixels, rows: usize) -> Range { + if viewport <= px(0.) { + return 0..rows; + } + let first = ((-offset_y) / px(ROW_HEIGHT)).floor().max(0.) as usize; + let count = (viewport / px(ROW_HEIGHT)).ceil() as usize + 2; + first.min(rows)..first.saturating_add(count).min(rows) +} + +fn selected_rows( + origin_y: Pixels, + top: Pixels, + bottom: Pixels, + rows: usize, +) -> Option> { + let last_row = rows.checked_sub(1)?; + let row_of = |y: Pixels| ((y - origin_y) / px(ROW_HEIGHT)).floor(); + let first = row_of(top); + let last = row_of(bottom); + if first > last_row as f32 || last < 0. { + return None; + } + Some((first.max(0.) as usize)..=(last as usize).min(last_row)) +} + +fn selection_band( + row_top: Pixels, + anchor: Point, + cursor: Point, +) -> Option<(Pixels, Pixels)> { + let height = px(ROW_HEIGHT); + let in_row = |point: Point| point.y >= row_top && point.y < row_top + height; + if row_top + height <= anchor.y.min(cursor.y) || row_top > anchor.y.max(cursor.y) { + return None; + } + if in_row(anchor) && in_row(cursor) { + return Some((anchor.x.min(cursor.x), anchor.x.max(cursor.x))); + } + let (start, end) = if anchor.y < cursor.y { + (anchor, cursor) + } else { + (cursor, anchor) + }; + if in_row(start) { + Some((start.x, px(f32::MAX))) + } else if in_row(end) { + Some((px(f32::MIN), end.x)) + } else { + Some((px(f32::MIN), px(f32::MAX))) + } +} + +fn selected_range( + text: &str, + line: &ShapedLine, + left: Pixels, + band: (Pixels, Pixels), +) -> Option> { + if text.len() != line.len() { + return None; + } + let (low, high) = band; + let mut range: Option> = None; + let mut start = line.x_for_index(0); + for (offset, character) in text.char_indices() { + let next = offset + character.len_utf8(); + let end = line.x_for_index(next); + let middle = left + start + (end - start).half(); + if middle >= low && middle <= high { + range.get_or_insert(offset..offset).end = next; + } + start = end; + } + range +} + fn copy_text(strings: &[SharedString], ranges: &[Option>]) -> String { let (Some(first), Some(last)) = ( ranges.iter().position(Option::is_some), @@ -184,8 +255,12 @@ impl Pen { .shape_line(text, self.font_size, &[run], None) } + fn measure(&self, text: SharedString, window: &Window) -> ShapedLine { + self.shape(text, self.style.color, window) + } + fn width(&self, text: SharedString, window: &Window) -> Pixels { - self.shape(text, self.style.color, window).width() + self.measure(text, window).width() } } @@ -234,13 +309,56 @@ impl DiffBody { } fn visible_rows(&self) -> Range { - let viewport = self.scroll.bounds().size.height; - if viewport <= px(0.) { - return 0..self.rows.len(); - } - let first = ((-self.scroll.offset().y) / px(ROW_HEIGHT)).floor().max(0.) as usize; - let count = (viewport / px(ROW_HEIGHT)).ceil() as usize + 2; - first.min(self.rows.len())..(first + count).min(self.rows.len()) + row_window( + self.scroll.offset().y, + self.scroll.bounds().size.height, + self.rows.len(), + ) + } + + fn copy_selection( + &self, + bounds: Bounds, + projected: &[Option>], + pen: &Pen, + window: &Window, + cx: &App, + ) -> String { + let Some(points) = self + .selection + .snapshot(cx) + .and_then(|snapshot| snapshot.window_points()) + else { + return String::new(); + }; + let anchor = points.anchor(); + let cursor = points.cursor(); + let Some(rows) = selected_rows( + bounds.origin.y, + anchor.y.min(cursor.y), + anchor.y.max(cursor.y), + self.rows.len(), + ) else { + return String::new(); + }; + + let left = bounds.origin.x + px(CODE_LEFT); + let ranges: Vec>> = rows + .clone() + .map(|index| { + if self.visible.contains(&index) { + return projected + .get(index - self.visible.start) + .and_then(Clone::clone); + } + let row_top = bounds.origin.y + px(index as f32 * ROW_HEIGHT); + let band = selection_band(row_top, anchor, cursor)?; + let text = &self.strings[index]; + selected_range(text, &pen.measure(text.clone(), window), left, band) + }) + .collect(); + + copy_text(&self.strings[rows], &ranges) } fn paint_gutter( @@ -320,6 +438,20 @@ impl Element for DiffBody { window: &mut Window, cx: &mut App, ) -> (LayoutId, Self::RequestLayoutState) { + self.visible = self.visible_rows(); + self.texts = self + .visible + .clone() + .map(|index| { + styled_row( + &self.rows[index], + self.strings[index].clone(), + &self.theme, + self.mode, + ) + }) + .collect(); + let children: Vec = self .texts .iter_mut() @@ -347,13 +479,14 @@ impl Element for DiffBody { window: &mut Window, cx: &mut App, ) -> Self::PrepaintState { - self.row_bounds = (0..self.rows.len()) + self.row_bounds = self + .visible + .clone() .map(|index| self.bounds_for_row(bounds, index)) .collect(); for (text, row_bounds) in self.texts.iter_mut().zip(&self.row_bounds) { text.prepaint(None, None, *row_bounds, &mut (), window, cx); } - self.visible = self.visible_rows(); let hitbox = window.insert_hitbox(bounds, HitboxBehavior::Normal); self.selection.register( @@ -377,26 +510,28 @@ impl Element for DiffBody { cx: &mut App, ) { let runs: Vec = self - .strings + .texts .iter() .enumerate() - .map(|(index, text)| { + .map(|(offset, text)| { + let index = self.visible.start + offset; TextSelectionRun::new( - text.clone(), - self.texts[index].layout().clone(), - self.row_bounds[index], + self.strings[index].clone(), + text.layout().clone(), + self.row_bounds[offset], ) .with_document_order(index as u64) }) .collect(); let projection = self.selection.update_runs(&runs, cx); - self.selection - .set_fallback_copy_text(copy_text(&self.strings, projection.ranges()), cx); let pen = Pen::new(window); + let selected = self.copy_selection(bounds, projection.ranges(), &pen, window, cx); + self.selection.set_fallback_copy_text(selected, cx); - for index in self.visible.clone() { - let top = self.row_bounds[index].origin.y; + for (offset, index) in self.visible.clone().enumerate() { + let row_bounds = self.row_bounds[offset]; + let top = row_bounds.origin.y; if let Some(background) = row_background(&self.rows[index], &self.theme, self.mode) { let band = Bounds::new( @@ -406,9 +541,9 @@ impl Element for DiffBody { window.paint_quad(fill(band, background)); } - if let Some(range) = projection.ranges().get(index).and_then(Clone::clone) { + if let Some(range) = projection.ranges().get(offset).and_then(Clone::clone) { paint_selection( - self.texts[index].layout(), + self.texts[offset].layout(), range, self.theme.selection, window, @@ -416,9 +551,7 @@ impl Element for DiffBody { } self.paint_gutter(index, bounds.origin.x, top, &pen, window, cx); - - let row_bounds = self.row_bounds[index]; - self.texts[index].paint(None, None, row_bounds, &mut (), &mut (), window, cx); + self.texts[offset].paint(None, None, row_bounds, &mut (), &mut (), window, cx); } } } @@ -474,4 +607,96 @@ mod tests { ] ); } + + #[test] + fn the_window_is_every_row_until_the_viewport_has_been_measured() { + assert_eq!(row_window(px(0.), px(0.), 500), 0..500); + } + + #[test] + fn the_window_starts_at_the_first_row_the_viewport_cuts_through() { + let window = row_window(px(-3. * ROW_HEIGHT - 4.), px(10. * ROW_HEIGHT), 500); + assert_eq!(window.start, 3); + assert_eq!(window.end, 3 + 12); + } + + #[test] + fn the_window_never_runs_past_the_last_row() { + assert_eq!( + row_window(px(-490. * ROW_HEIGHT), px(10. * ROW_HEIGHT), 500).end, + 500 + ); + assert_eq!( + row_window(px(-900. * ROW_HEIGHT), px(10. * ROW_HEIGHT), 500), + 500..500 + ); + } + + #[test] + fn a_selection_spans_the_rows_its_two_endpoints_land_in() { + assert_eq!( + selected_rows( + px(100.), + px(100. + 2.5 * ROW_HEIGHT), + px(100. + 7.1 * ROW_HEIGHT), + 20 + ), + Some(2..=7) + ); + } + + #[test] + fn a_selection_reaching_past_the_body_is_clamped_to_it() { + assert_eq!( + selected_rows(px(100.), px(-500.), px(100. + 900. * ROW_HEIGHT), 20), + Some(0..=19) + ); + } + + #[test] + fn a_selection_entirely_outside_the_body_spans_no_rows() { + assert_eq!(selected_rows(px(100.), px(-500.), px(-400.), 20), None); + assert_eq!( + selected_rows( + px(100.), + px(100. + 40. * ROW_HEIGHT), + px(100. + 50. * ROW_HEIGHT), + 20 + ), + None + ); + assert_eq!(selected_rows(px(100.), px(100.), px(200.), 0), None); + } + + #[test] + fn a_row_holding_both_endpoints_is_bounded_by_them() { + let band = selection_band(px(36.), point(px(80.), px(40.)), point(px(20.), px(50.))); + assert_eq!(band, Some((px(20.), px(80.)))); + } + + #[test] + fn the_first_row_of_a_selection_runs_from_its_endpoint_to_the_end_of_the_line() { + let band = selection_band(px(36.), point(px(80.), px(40.)), point(px(20.), px(200.))); + assert_eq!(band, Some((px(80.), px(f32::MAX)))); + } + + #[test] + fn the_last_row_of_a_selection_runs_from_the_start_of_the_line_to_its_endpoint() { + let band = selection_band(px(36.), point(px(80.), px(-10.)), point(px(20.), px(40.))); + assert_eq!(band, Some((px(f32::MIN), px(20.)))); + } + + #[test] + fn a_row_between_the_endpoints_is_selected_whole() { + let band = selection_band(px(36.), point(px(80.), px(-10.)), point(px(20.), px(200.))); + assert_eq!(band, Some((px(f32::MIN), px(f32::MAX)))); + } + + #[test] + fn a_row_outside_the_selection_has_no_band() { + let above = selection_band(px(0.), point(px(80.), px(40.)), point(px(20.), px(50.))); + let below = selection_band(px(90.), point(px(80.), px(40.)), point(px(20.), px(50.))); + assert_eq!(above, None); + assert_eq!(below, None); + } } diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 16bcc16..a2e644a 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -12,9 +12,9 @@ //! //! Selection comes back through `gpui-base`'s window-level participant system rather than //! from the editor. [`body`] is the element that joins it: it registers one participant and -//! declares one run per row, and only the code text becomes a run — the gutters and the -//! marker are painted directly and never registered, which is what keeps line numbers and -//! markers out of the clipboard. The rows scroll on both axes rather than soft-wrapping, +//! declares one run per row on screen, and only the code text becomes a run — the gutters +//! and the marker are painted directly and never registered, which is what keeps line +//! numbers and markers out of the clipboard. The rows scroll on both axes rather than soft-wrapping, //! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in //! `Overflow::Scroll`, and gpui's vertical-onto-horizontal remap (`div.rs:3220-3224`, @@ -23,6 +23,34 @@ //! trackpad gesture through `ongoing_scroll.filter(..)` (`div.rs:3209-3216`), which is live //! because `track_scroll` populates `ongoing_scroll` from the tracked handle (`div.rs:2165`). //! +//! "On screen" is decided in `request_layout`, not in `paint`, because laying a row out is +//! the expensive half and nothing downstream can be narrowed without it. That ordering is +//! forced rather than chosen: `TextLayout::len`, `bounds`, `line_height` and +//! `position_for_index` each `expect` on cells filled during measure and prepaint +//! (`gpui/src/elements/text.rs:864-871`), and `selection_range_for_run` reads `layout.len()` +//! on *every* run it is handed (`text_selection.rs:388`), so a row whose `StyledText` was +//! skipped this frame cannot be declared as a run at all — declaring it panics on the first +//! scroll that has a live selection. +//! +//! The copied text is therefore not read off that projection. A selection dragged past the +//! bottom edge would come back holding only the rows that happened to be on screen, and +//! nothing would report that it had been cut short. `body::DiffBody::copy_selection` derives +//! the row span from the selection's own window points instead — `body::selected_rows` — and +//! asks `body::selection_band` and `body::selected_range` for each row's byte range, shaping +//! only the at most two rows whose ends the selection cuts through; every row between them is +//! whole. Endpoints survive scrolling because `gpui-base` stores them relative to +//! `bounds.origin`, which already carries the scroll, so a point off the top of the viewport +//! is a negative `y` rather than a lost one. Rows that *are* on screen keep the projection's +//! own range, so what is highlighted and what is copied cannot drift apart. +//! +//! What stays unwindowed is the content width: `body::DiffBody::content_width` shapes every +//! row on every layout pass, because the horizontal scroll extent has to consider rows that +//! are not on screen or the scrollbar would resize as the view scrolls vertically, and +//! because the children are laid out against a width that provably exceeds every row's +//! natural width — which is what keeps a row one line tall and `ROW_HEIGHT` true. After the +//! first frame those are hits in gpui's line-layout cache, so the cost is a hash of each +//! row's bytes rather than a reshape. +//! //! Copying depends on a fact `gpui-base`'s own doc comment does not state. A participant's //! runs concatenate with no separator when `update_runs` projects them //! (`text_selection.rs:593`); only whole participants are joined with `"\n"` diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index b51709c..66bd6f4 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -10,7 +10,8 @@ //! Both tabs keep their scroll position in a [`ScrollHandle`] owned here rather than in //! the element tree, which is rebuilt every render. The diff's handle is passed down to //! the row element as well, because that element reads its own scroll offset to decide -//! which rows are worth painting. +//! which rows to lay out and paint — it is the only input it has to that decision before +//! the frame gives it any bounds. //! //! The one piece of view state that does need to outlive a frame is the diff's selection //! participant: `gpui-base` keys a window-level selection off a diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md index 9d410a8..95b4ddc 100644 --- a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -149,8 +149,16 @@ in `prepaint`/`paint`: gutters and the marker are neighbouring elements and are never registered, which is what makes a copied diff come out as clean code with no line numbers and no markers. This is strictly better than the editor, which copies its markers today. -- `TextSelectionContentKey` gives virtualised rows a stable identity, so a selection - survives a row being recycled by the list. +- `TextSelectionContentKey` turns out not to be needed. It is a `u64` the participant + computes from an endpoint's content point through + `TextSelectionHandle::resolve_content_key_with`, which `gpui-base` stores on the endpoint + and hands straight back on `TextSelectionSnapshot::anchor().content_key()`. It is a + channel back to the participant and nothing more: it takes no part in hit-testing, in + `project_ranges`, or in copying. What actually makes an off-screen endpoint survive is + that the endpoint is stored as `position − bounds.origin` and this element's + `bounds.origin` already carries the scroll, so an endpoint above the viewport is a + negative `y` rather than a lost one — and a row index here is `y / ROW_HEIGHT`, which is + the identity a key would have carried anyway. The text has to go through `StyledText` rather than `div().child("…")`, because a run needs a `TextLayout`. This is the one structural constraint the selection system imposes on the @@ -228,14 +236,19 @@ than after. Second risk: the coordinate handling. `TextLayout::bounds`, `line_height`, `len`, `position_for_index` and `index_for_position` all panic when called before the text has been laid out — they `unwrap`/`expect` on an inner cell filled during prepaint. They are safe -from `paint` and from nowhere earlier. This is where hand-rolled windowing bites: `prepaint` -today lays out every row and `paint` builds a run for every row, which is only safe while -`prepaint` stays unwindowed. Task 5 narrows `paint` to the visible rows but must not narrow -`prepaint` to match — doing so would leave the off-screen rows' `TextLayout`s unlaid-out, and -the first scroll with a live selection would panic on "prepaint has not been performed". -Narrowing the runs passed to `update_runs` instead avoids the panic but silently truncates a -copy to whatever rows are on screen, which is just as wrong. This is where an implementation -will go wrong if it goes wrong. +from `paint` and from nowhere earlier. This is where hand-rolled windowing bites, and the +two obvious moves are both wrong. Narrowing layout while keeping a run per row panics on the +first scroll with a live selection, because `selection_range_for_run` reads `layout.len()` on +every run it is handed. Narrowing the runs to match avoids the panic and silently truncates a +copy to whatever rows are on screen. + +The way out is that the copy does not have to come from the projection. `update_runs` covers +the window and drives only the highlight; the copied text is derived from the selection's own +window points, which are scroll-invariant, so the row span is arithmetic and only the two rows +whose ends the selection cuts through need shaping at all. A row already on screen keeps the +projection's range, so the highlight and the clipboard cannot disagree. What stays unwindowed +is the content-width measurement, which must consider every row or the horizontal scroll +extent moves as the view scrolls vertically. ## Files touched From 0073631555ed195d9fd24a9c0304956a7a49a52c Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 15:20:09 +0200 Subject: [PATCH 10/19] fix(diff): skip the shaper for a row the selection covers whole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit selected_range took its ShapedLine by value, so pen.measure ran for every off-screen row in the selection span — including the whole-row middles whose shaped result was then thrown away in favour of 0..text.len(). The claim that only the two rows the selection cuts through get shaped was made in four places and was true in none of them. Taking the line as FnOnce and answering Band::Whole before calling it makes the claim true rather than requiring four retractions, and roughly halves the per-frame cost while a large selection is live. The (f32::MIN, f32::MAX) sentinel pair went with it. A band is now an enum over the four cases the rule actually has, so the whole-row case is something to match on rather than something to recognise by its bounds, and Band::holds is exhaustive instead of relying on the sentinels behaving as open bounds. Also corrects the panic citation in the module doc and the design spec. The four TextLayout accessors do not agree on either the mechanism or the cell: len and line_height unwrap the cell the measure closure fills, while bounds, position_for_index and index_for_position need the one prepaint fills as well. Only position_for_index lives at text.rs:864-871, which the doc cited for all four. That len needs only measurement does not soften the constraint, because selection_range_for_run reads it before any geometry and a row skipped at request_layout has no measure cell either. Three tests, all covering branches nothing reached: an upward drag, which is the arm that sorts the endpoints by y and was the failure mode the review was asked to chase; a view over-scrolled past the top, which is the .max(0.) clamp; and a whole row, whose shaper is an unreachable! so removing the short-circuit fails the suite rather than quietly costing a frame. --- crates/ui/src/detail/diff/body.rs | 101 ++++++++++++++---- crates/ui/src/detail/diff/mod.rs | 14 +-- ...026-08-29-github-style-diff-view-design.md | 12 ++- 3 files changed, 99 insertions(+), 28 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index ed285d8..d69af0a 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -153,18 +153,36 @@ fn selected_rows( Some((first.max(0.) as usize)..=(last as usize).min(last_row)) } -fn selection_band( - row_top: Pixels, - anchor: Point, - cursor: Point, -) -> Option<(Pixels, Pixels)> { +#[derive(Clone, Copy, Debug, PartialEq)] +enum Band { + Whole, + From(Pixels), + To(Pixels), + Between(Pixels, Pixels), +} + +impl Band { + fn holds(self, x: Pixels) -> bool { + match self { + Band::Whole => true, + Band::From(low) => x >= low, + Band::To(high) => x <= high, + Band::Between(low, high) => x >= low && x <= high, + } + } +} + +fn selection_band(row_top: Pixels, anchor: Point, cursor: Point) -> Option { let height = px(ROW_HEIGHT); let in_row = |point: Point| point.y >= row_top && point.y < row_top + height; if row_top + height <= anchor.y.min(cursor.y) || row_top > anchor.y.max(cursor.y) { return None; } if in_row(anchor) && in_row(cursor) { - return Some((anchor.x.min(cursor.x), anchor.x.max(cursor.x))); + return Some(Band::Between( + anchor.x.min(cursor.x), + anchor.x.max(cursor.x), + )); } let (start, end) = if anchor.y < cursor.y { (anchor, cursor) @@ -172,31 +190,33 @@ fn selection_band( (cursor, anchor) }; if in_row(start) { - Some((start.x, px(f32::MAX))) + Some(Band::From(start.x)) } else if in_row(end) { - Some((px(f32::MIN), end.x)) + Some(Band::To(end.x)) } else { - Some((px(f32::MIN), px(f32::MAX))) + Some(Band::Whole) } } fn selected_range( text: &str, - line: &ShapedLine, + band: Band, left: Pixels, - band: (Pixels, Pixels), + line: impl FnOnce() -> ShapedLine, ) -> Option> { + if band == Band::Whole { + return (!text.is_empty()).then_some(0..text.len()); + } + let line = line(); if text.len() != line.len() { return None; } - let (low, high) = band; let mut range: Option> = None; let mut start = line.x_for_index(0); for (offset, character) in text.char_indices() { let next = offset + character.len_utf8(); let end = line.x_for_index(next); - let middle = left + start + (end - start).half(); - if middle >= low && middle <= high { + if band.holds(left + start + (end - start).half()) { range.get_or_insert(offset..offset).end = next; } start = end; @@ -354,7 +374,7 @@ impl DiffBody { let row_top = bounds.origin.y + px(index as f32 * ROW_HEIGHT); let band = selection_band(row_top, anchor, cursor)?; let text = &self.strings[index]; - selected_range(text, &pen.measure(text.clone(), window), left, band) + selected_range(text, band, left, || pen.measure(text.clone(), window)) }) .collect(); @@ -620,6 +640,13 @@ mod tests { assert_eq!(window.end, 3 + 12); } + #[test] + fn the_window_starts_at_the_first_row_when_the_view_is_over_scrolled_upwards() { + let window = row_window(px(60.), px(10. * ROW_HEIGHT), 500); + assert_eq!(window.start, 0); + assert_eq!(window.end, 12); + } + #[test] fn the_window_never_runs_past_the_last_row() { assert_eq!( @@ -671,25 +698,51 @@ mod tests { #[test] fn a_row_holding_both_endpoints_is_bounded_by_them() { let band = selection_band(px(36.), point(px(80.), px(40.)), point(px(20.), px(50.))); - assert_eq!(band, Some((px(20.), px(80.)))); + assert_eq!(band, Some(Band::Between(px(20.), px(80.)))); } #[test] fn the_first_row_of_a_selection_runs_from_its_endpoint_to_the_end_of_the_line() { let band = selection_band(px(36.), point(px(80.), px(40.)), point(px(20.), px(200.))); - assert_eq!(band, Some((px(80.), px(f32::MAX)))); + assert_eq!(band, Some(Band::From(px(80.)))); } #[test] fn the_last_row_of_a_selection_runs_from_the_start_of_the_line_to_its_endpoint() { let band = selection_band(px(36.), point(px(80.), px(-10.)), point(px(20.), px(40.))); - assert_eq!(band, Some((px(f32::MIN), px(20.)))); + assert_eq!(band, Some(Band::To(px(20.)))); } #[test] fn a_row_between_the_endpoints_is_selected_whole() { let band = selection_band(px(36.), point(px(80.), px(-10.)), point(px(20.), px(200.))); - assert_eq!(band, Some((px(f32::MIN), px(f32::MAX)))); + assert_eq!(band, Some(Band::Whole)); + } + + #[test] + fn a_drag_upwards_bands_its_rows_exactly_as_the_same_drag_downwards() { + let low = point(px(80.), px(40.)); + let high = point(px(20.), px(200.)); + let above = point(px(20.), px(-10.)); + + assert_eq!( + selection_band(px(36.), high, low), + selection_band(px(36.), low, high) + ); + assert_eq!( + selection_band(px(36.), low, above), + selection_band(px(36.), above, low) + ); + assert_eq!( + selection_band(px(36.), high, above), + selection_band(px(36.), above, high) + ); + assert_eq!( + selection_band(px(36.), high, low), + Some(Band::From(px(80.))) + ); + assert_eq!(selection_band(px(36.), low, above), Some(Band::To(px(80.)))); + assert_eq!(selection_band(px(36.), high, above), Some(Band::Whole)); } #[test] @@ -699,4 +752,14 @@ mod tests { assert_eq!(above, None); assert_eq!(below, None); } + + #[test] + fn a_whole_row_needs_no_shaping_and_an_empty_one_selects_nothing() { + let shape = || unreachable!("a whole row must not be shaped"); + assert_eq!( + selected_range("one", Band::Whole, px(0.), shape), + Some(0..3) + ); + assert_eq!(selected_range("", Band::Whole, px(0.), shape), None); + } } diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index a2e644a..e3cb79f 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -25,12 +25,14 @@ //! //! "On screen" is decided in `request_layout`, not in `paint`, because laying a row out is //! the expensive half and nothing downstream can be narrowed without it. That ordering is -//! forced rather than chosen: `TextLayout::len`, `bounds`, `line_height` and -//! `position_for_index` each `expect` on cells filled during measure and prepaint -//! (`gpui/src/elements/text.rs:864-871`), and `selection_range_for_run` reads `layout.len()` -//! on *every* run it is handed (`text_selection.rs:388`), so a row whose `StyledText` was -//! skipped this frame cannot be declared as a run at all — declaring it panics on the first -//! scroll that has a live selection. +//! forced rather than chosen: every `TextLayout` accessor panics on a row that was skipped, +//! `len` and `line_height` on the cell the measure closure fills +//! (`gpui/src/elements/text.rs:935-942`) and `bounds` and `position_for_index` on that one +//! and on the cell prepaint fills as well (`:864-871`, `:930-932`). `selection_range_for_run` +//! reads `layout.len()` on *every* run it is handed, before any geometry +//! (`text_selection.rs:388`), so a row whose `StyledText` was skipped this frame cannot be +//! declared as a run at all — declaring it panics on the first scroll that has a live +//! selection. //! //! The copied text is therefore not read off that projection. A selection dragged past the //! bottom edge would come back holding only the rows that happened to be on screen, and diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md index 95b4ddc..e90044b 100644 --- a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -235,8 +235,12 @@ than after. Second risk: the coordinate handling. `TextLayout::bounds`, `line_height`, `len`, `position_for_index` and `index_for_position` all panic when called before the text has been -laid out — they `unwrap`/`expect` on an inner cell filled during prepaint. They are safe -from `paint` and from nowhere earlier. This is where hand-rolled windowing bites, and the +laid out, but not on the same cell: `len` and `line_height` need only the one the measure +closure fills (`text.rs:935-942`), while `bounds`, `position_for_index` and +`index_for_position` need the one prepaint fills as well (`:830-837`, `:864-871`, +`:930-932`). All five are safe from `paint` and from nowhere earlier — the distinction +matters only when deciding which rows may be skipped. This is where hand-rolled windowing +bites, and the two obvious moves are both wrong. Narrowing layout while keeping a run per row panics on the first scroll with a live selection, because `selection_range_for_run` reads `layout.len()` on every run it is handed. Narrowing the runs to match avoids the panic and silently truncates a @@ -245,7 +249,9 @@ copy to whatever rows are on screen. The way out is that the copy does not have to come from the projection. `update_runs` covers the window and drives only the highlight; the copied text is derived from the selection's own window points, which are scroll-invariant, so the row span is arithmetic and only the two rows -whose ends the selection cuts through need shaping at all. A row already on screen keeps the +whose ends the selection cuts through need shaping at all — a row between them is whole by +construction, and answering `0..len` for it must short-circuit before the shaper is reached +rather than after, or the saving is only notional. A row already on screen keeps the projection's range, so the highlight and the clipboard cannot disagree. What stays unwindowed is the content-width measurement, which must consider every row or the horizontal scroll extent moves as the view scrolls vertically. From 6dafce9d24172c42460b84a6415914fb58f4b343 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 15:28:36 +0200 Subject: [PATCH 11/19] feat(diff): persist the chosen diff view mode --- crates/ui/src/diff_view_mode.rs | 50 +++++++++++++++++++++++++++++++++ crates/ui/src/lib.rs | 1 + crates/ui/src/persistence.rs | 49 ++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 crates/ui/src/diff_view_mode.rs diff --git a/crates/ui/src/diff_view_mode.rs b/crates/ui/src/diff_view_mode.rs new file mode 100644 index 0000000..a28c71a --- /dev/null +++ b/crates/ui/src/diff_view_mode.rs @@ -0,0 +1,50 @@ +use serde::{Deserialize, Serialize}; + +#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum DiffViewMode { + #[default] + Unified, + Split, +} + +impl DiffViewMode { + pub const ALL: [DiffViewMode; 2] = [Self::Unified, Self::Split]; + + pub fn index(self) -> usize { + Self::ALL.iter().position(|mode| *mode == self).unwrap_or(0) + } + + pub fn from_index(index: usize) -> Self { + Self::ALL.get(index).copied().unwrap_or_default() + } + + pub fn label(self) -> &'static str { + match self { + Self::Unified => "Unified", + Self::Split => "Split", + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_default_is_unified() { + assert_eq!(DiffViewMode::default(), DiffViewMode::Unified); + } + + #[test] + fn every_mode_round_trips_through_its_index() { + for mode in DiffViewMode::ALL { + assert_eq!(DiffViewMode::from_index(mode.index()), mode); + } + } + + #[test] + fn an_out_of_range_index_falls_back_to_the_default() { + assert_eq!(DiffViewMode::from_index(99), DiffViewMode::default()); + } +} diff --git a/crates/ui/src/lib.rs b/crates/ui/src/lib.rs index 658f510..8e56e99 100644 --- a/crates/ui/src/lib.rs +++ b/crates/ui/src/lib.rs @@ -15,6 +15,7 @@ pub mod actions; pub mod branch_actions; pub mod density; pub mod detail; +pub mod diff_view_mode; pub mod graph_palette; pub mod history; pub mod persistence; diff --git a/crates/ui/src/persistence.rs b/crates/ui/src/persistence.rs index e17b019..6d8ded3 100644 --- a/crates/ui/src/persistence.rs +++ b/crates/ui/src/persistence.rs @@ -15,12 +15,14 @@ use std::path::{Path, PathBuf}; use gpui_component::dock::DockAreaState; +use crate::diff_view_mode::DiffViewMode; use crate::project::ProjectList; use crate::theme_preference::ThemePreference; const APPLICATION_SUPPORT_DIR: &str = "Library/Application Support/gitr"; const DOCK_LAYOUT_FILE: &str = "dock-layout.json"; const THEME_PREFERENCE_FILE: &str = "theme-preference.json"; +const DIFF_VIEW_MODE_FILE: &str = "diff-view-preference.json"; const PROJECTS_FILE: &str = "projects.json"; const REMOTE_CACHE_DIR: &str = "remotes"; @@ -106,6 +108,42 @@ pub fn load_theme_preference() -> Option { load_theme_preference_from(&theme_preference_path()?).ok() } +/// Where the diff view mode preference lives for the signed-in user, or `None` if `$HOME` is +/// unset. See [`dock_layout_path`] — same directory, same not-cached reasoning. +pub fn diff_view_mode_path() -> Option { + Some(application_support_dir()?.join(DIFF_VIEW_MODE_FILE)) +} + +/// Persists `mode` to `path`, creating its parent directory if it does not exist +/// yet. Blocking: call this from `cx.background_executor()`, never on the frame thread. +pub fn save_diff_view_mode_to(path: &Path, mode: &DiffViewMode) -> anyhow::Result<()> { + if let Some(dir) = path.parent() { + std::fs::create_dir_all(dir)?; + } + let json = serde_json::to_string_pretty(mode)?; + std::fs::write(path, json)?; + Ok(()) +} + +/// Reads and parses the diff view mode preference at `path`. Blocking, meant to run once at +/// startup before the first frame — same shape as [`load_from`]. +pub fn load_diff_view_mode_from(path: &Path) -> anyhow::Result { + let json = std::fs::read_to_string(path)?; + Ok(serde_json::from_str(&json)?) +} + +/// Saves `mode` to the user's application support directory. +pub fn save_diff_view_mode(mode: &DiffViewMode) -> anyhow::Result<()> { + let path = diff_view_mode_path().ok_or_else(|| anyhow::anyhow!("$HOME is not set"))?; + save_diff_view_mode_to(&path, mode) +} + +/// Loads the diff view mode preference from the user's application support directory, if one +/// exists and parses cleanly. Any failure falls back to `None`, mirroring [`load`]. +pub fn load_diff_view_mode() -> Option { + load_diff_view_mode_from(&diff_view_mode_path()?).ok() +} + /// Where the project list lives for the signed-in user, or `None` if `$HOME` is unset. /// See [`dock_layout_path`] — same directory, same not-cached reasoning. pub fn project_list_path() -> Option { @@ -279,6 +317,17 @@ mod tests { let _ = std::fs::remove_dir_all(path.parent().unwrap()); } + #[test] + fn a_diff_view_mode_round_trips_through_a_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("diff-view-preference.json"); + save_diff_view_mode_to(&path, &DiffViewMode::Split).expect("save"); + assert_eq!( + load_diff_view_mode_from(&path).expect("load"), + DiffViewMode::Split + ); + } + fn sample_project_list() -> ProjectList { let a = Project::local(PathBuf::from("/repos/a")); let b = Project::local(PathBuf::from("/repos/b")); From cc9665f5a2f316f7e6f44f69a41f83cf1438b92f Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 15:32:14 +0200 Subject: [PATCH 12/19] refactor(diff): remove doc comments from diff view mode functions --- crates/ui/src/persistence.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/crates/ui/src/persistence.rs b/crates/ui/src/persistence.rs index 6d8ded3..28b3f77 100644 --- a/crates/ui/src/persistence.rs +++ b/crates/ui/src/persistence.rs @@ -108,14 +108,10 @@ pub fn load_theme_preference() -> Option { load_theme_preference_from(&theme_preference_path()?).ok() } -/// Where the diff view mode preference lives for the signed-in user, or `None` if `$HOME` is -/// unset. See [`dock_layout_path`] — same directory, same not-cached reasoning. pub fn diff_view_mode_path() -> Option { Some(application_support_dir()?.join(DIFF_VIEW_MODE_FILE)) } -/// Persists `mode` to `path`, creating its parent directory if it does not exist -/// yet. Blocking: call this from `cx.background_executor()`, never on the frame thread. pub fn save_diff_view_mode_to(path: &Path, mode: &DiffViewMode) -> anyhow::Result<()> { if let Some(dir) = path.parent() { std::fs::create_dir_all(dir)?; @@ -125,21 +121,16 @@ pub fn save_diff_view_mode_to(path: &Path, mode: &DiffViewMode) -> anyhow::Resul Ok(()) } -/// Reads and parses the diff view mode preference at `path`. Blocking, meant to run once at -/// startup before the first frame — same shape as [`load_from`]. pub fn load_diff_view_mode_from(path: &Path) -> anyhow::Result { let json = std::fs::read_to_string(path)?; Ok(serde_json::from_str(&json)?) } -/// Saves `mode` to the user's application support directory. pub fn save_diff_view_mode(mode: &DiffViewMode) -> anyhow::Result<()> { let path = diff_view_mode_path().ok_or_else(|| anyhow::anyhow!("$HOME is not set"))?; save_diff_view_mode_to(&path, mode) } -/// Loads the diff view mode preference from the user's application support directory, if one -/// exists and parses cleanly. Any failure falls back to `None`, mirroring [`load`]. pub fn load_diff_view_mode() -> Option { load_diff_view_mode_from(&diff_view_mode_path()?).ok() } From 9cffd4a2ee56f80cff26543e2a2ea8248bff9dc5 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 15:51:57 +0200 Subject: [PATCH 13/19] feat(diff): add a side-by-side view behind a persisted toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DiffBody now paints a row as N cells rather than as one string, with N fixed per view — one for unified, two for split. That uniformity is the whole design: a cell is row * columns + column, so every index the element already kept over rows converts to cells by arithmetic instead of by a lookup table, and the three phases that had to agree on a visible range still agree on one range derived one way. A file, hunk or placeholder row keeps a single full-width cell in either view rather than being split into a text half and an empty half, which is what makes the column count uniform in the first place; content_width measures every cell and divides by the column count, so a full-width header still fits inside the half it is drawn in and a row stays one line tall. The copy path needed a decision, because a row now has two texts. It joins columns with a tab and rows with a newline. Newlines between columns were rejected: gpui-base's band rule takes both cells of every row a selection passes through whole, so a drag down the right column copies the left one too, and interleaving them would repeat every context line. Copying only the column the drag started in was rejected for a harder reason — the highlight comes from the projection, so dropping a cell the projection selected is exactly the drift between clipboard and screen that deriving the copy from geometry exists to prevent. Neither separator yields code you can paste into a file; the tab at least yields the table on screen. The arithmetic and the projection still agree cell for cell. A selection band is a property of the row, so both cells of a row share it and only the cell's own column offset turns it into a byte range — which is what point_in_selection_band does to two runs that share a y, since it tests a character's midpoint against a band derived from that character's own line. Band::Whole still short-circuits before the shaper. The toggle is a second segmented bar beside the existing one, shown only on the Diff tab. save_diff_view_mode blocks on file I/O, so it runs on the background executor the way the theme preference does; the mode is read once in DetailPanel::new. The bar's container gains .flex() — it already carried items_center and gap_2, which were inert under the default block display. split_rows is the only new pure logic and carries the tests. model.rs gives up its file header, placeholder and hunk header construction to functions both row builders call, so the two views cannot drift on what a header says. --- crates/ui/src/detail/diff/body.rs | 451 ++++++++++++++++++++++------- crates/ui/src/detail/diff/mod.rs | 57 +++- crates/ui/src/detail/diff/model.rs | 44 ++- crates/ui/src/detail/diff/split.rs | 189 ++++++++++++ crates/ui/src/detail/mod.rs | 80 ++++- 5 files changed, 678 insertions(+), 143 deletions(-) create mode 100644 crates/ui/src/detail/diff/split.rs diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index d69af0a..e8be0d7 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -11,7 +11,9 @@ use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun use gpui_component::{ThemeColor, ThemeMode}; use super::model::Row; +use super::pairing::SideLine; use super::palette::line_colors; +use super::split::SplitRow; pub(super) const ROW_HEIGHT: f32 = 18.; @@ -20,10 +22,58 @@ const MARKER_WIDTH: f32 = 16.; const GUTTER_PADDING: f32 = 8.; const MARKER_PADDING: f32 = 5.; const CODE_LEFT: f32 = 2. * GUTTER_WIDTH + MARKER_WIDTH; +const SPLIT_CODE_LEFT: f32 = GUTTER_WIDTH + MARKER_WIDTH; +const COLUMN_RULE_WIDTH: f32 = 1.; const TRAILING_SPACE: f32 = 16.; +pub(super) enum Rows { + Unified(Vec), + Split(Vec), +} + +impl Rows { + fn len(&self) -> usize { + match self { + Rows::Unified(rows) => rows.len(), + Rows::Split(rows) => rows.len(), + } + } + + fn columns(&self) -> usize { + match self { + Rows::Unified(_) => 1, + Rows::Split(_) => 2, + } + } + + fn code_left(&self) -> f32 { + match self { + Rows::Unified(_) => CODE_LEFT, + Rows::Split(_) => SPLIT_CODE_LEFT, + } + } + + fn cells(&self) -> usize { + self.len() * self.columns() + } + + fn side(&self, row: usize, column: usize) -> Option<&SideLine> { + let Rows::Split(rows) = self else { + return None; + }; + let SplitRow::Sides { left, right } = &rows[row] else { + return None; + }; + if column == 0 { + left.as_ref() + } else { + right.as_ref() + } + } +} + pub(super) struct DiffBody { - rows: Vec, + rows: Rows, strings: Vec, selection: TextSelectionHandle, scroll: ScrollHandle, @@ -31,17 +81,19 @@ pub(super) struct DiffBody { mode: ThemeMode, visible: Range, texts: Vec, - row_bounds: Vec>, + cell_bounds: Vec>, } pub(super) fn body( - rows: Vec, + rows: Rows, selection: TextSelectionHandle, scroll: ScrollHandle, theme: ThemeColor, mode: ThemeMode, ) -> DiffBody { - let strings: Vec = rows.iter().map(row_text).collect(); + let strings: Vec = (0..rows.cells()) + .map(|cell| cell_text(&rows, cell)) + .collect(); DiffBody { rows, strings, @@ -51,7 +103,25 @@ pub(super) fn body( mode, visible: 0..0, texts: Vec::new(), - row_bounds: Vec::new(), + cell_bounds: Vec::new(), + } +} + +fn cell_text(rows: &Rows, cell: usize) -> SharedString { + let columns = rows.columns(); + let (row, column) = (cell / columns, cell % columns); + match rows { + Rows::Unified(rows) => row_text(&rows[row]), + Rows::Split(split) => match &split[row] { + SplitRow::Full(full) if column == 0 => row_text(full), + SplitRow::Full(_) => SharedString::default(), + SplitRow::Sides { left, right } => { + let side = if column == 0 { left } else { right }; + side.as_ref() + .map(|side| SharedString::from(side.content.clone())) + .unwrap_or_default() + } + }, } } @@ -64,15 +134,36 @@ fn row_text(row: &Row) -> SharedString { } } -fn styled_row(row: &Row, text: SharedString, theme: &ThemeColor, mode: ThemeMode) -> StyledText { +fn styled_cell( + rows: &Rows, + cell: usize, + text: SharedString, + theme: &ThemeColor, + mode: ThemeMode, +) -> StyledText { let range = 0..text.len(); let highlight = HighlightStyle { - color: Some(row_foreground(row, theme, mode)), + color: Some(cell_foreground(rows, cell, theme, mode)), ..Default::default() }; StyledText::new(text).with_highlights([(range, highlight)]) } +fn cell_foreground(rows: &Rows, cell: usize, theme: &ThemeColor, mode: ThemeMode) -> Hsla { + let columns = rows.columns(); + let (row, column) = (cell / columns, cell % columns); + match rows { + Rows::Unified(rows) => row_foreground(&rows[row], theme, mode), + Rows::Split(split) => match &split[row] { + SplitRow::Full(full) => row_foreground(full, theme, mode), + SplitRow::Sides { .. } => rows + .side(row, column) + .map(|side| line_colors(side.origin, mode, theme).foreground) + .unwrap_or(theme.foreground), + }, + } +} + fn row_foreground(row: &Row, theme: &ThemeColor, mode: ThemeMode) -> Hsla { match row { Row::FileHeader { .. } => theme.foreground, @@ -224,7 +315,12 @@ fn selected_range( range } -fn copy_text(strings: &[SharedString], ranges: &[Option>]) -> String { +fn copy_text( + strings: &[SharedString], + ranges: &[Option>], + start: usize, + columns: usize, +) -> String { let (Some(first), Some(last)) = ( ranges.iter().position(Option::is_some), ranges.iter().rposition(Option::is_some), @@ -232,15 +328,20 @@ fn copy_text(strings: &[SharedString], ranges: &[Option>]) -> Strin return String::new(); }; - strings[first..=last] - .iter() - .zip(&ranges[first..=last]) - .map(|(text, range)| match range { - Some(range) => &text[range.clone()], - None => "", - }) - .collect::>() - .join("\n") + let mut text = String::new(); + for cell in first..=last { + if cell > first { + text.push(if (start + cell).is_multiple_of(columns) { + '\n' + } else { + '\t' + }); + } + if let Some(range) = &ranges[cell] { + text.push_str(&strings[cell][range.clone()]); + } + } + text } fn paint_selection(layout: &TextLayout, range: Range, color: Hsla, window: &mut Window) { @@ -312,17 +413,27 @@ impl DiffBody { for text in &self.strings { widest = widest.max(pen.width(text.clone(), window)); } - px(CODE_LEFT) + widest + px(TRAILING_SPACE) + (px(self.rows.code_left()) + widest + px(TRAILING_SPACE)) * self.rows.columns() as f32 } - fn bounds_for_row(&self, bounds: Bounds, index: usize) -> Bounds { + fn column_width(&self, bounds: Bounds) -> Pixels { + bounds.size.width / self.rows.columns() as f32 + } + + fn column_left(&self, bounds: Bounds, column: usize) -> Pixels { + bounds.origin.x + self.column_width(bounds) * column as f32 + } + + fn bounds_for_cell(&self, bounds: Bounds, cell: usize) -> Bounds { + let columns = self.rows.columns(); + let code_left = px(self.rows.code_left()); Bounds::new( point( - bounds.origin.x + px(CODE_LEFT), - bounds.origin.y + px(index as f32 * ROW_HEIGHT), + self.column_left(bounds, cell % columns) + code_left, + bounds.origin.y + px((cell / columns) as f32 * ROW_HEIGHT), ), size( - (bounds.size.width - px(CODE_LEFT)).max(px(0.)), + (self.column_width(bounds) - code_left).max(px(0.)), px(ROW_HEIGHT), ), ) @@ -336,6 +447,11 @@ impl DiffBody { ) } + fn visible_cells(&self) -> Range { + let columns = self.rows.columns(); + self.visible.start * columns..self.visible.end * columns + } + fn copy_selection( &self, bounds: Bounds, @@ -362,72 +478,140 @@ impl DiffBody { return String::new(); }; - let left = bounds.origin.x + px(CODE_LEFT); - let ranges: Vec>> = rows + let columns = self.rows.columns(); + let visible = self.visible_cells(); + let cells = rows.start() * columns..(rows.end() + 1) * columns; + let ranges: Vec>> = cells .clone() - .map(|index| { - if self.visible.contains(&index) { - return projected - .get(index - self.visible.start) - .and_then(Clone::clone); + .map(|cell| { + if visible.contains(&cell) { + return projected.get(cell - visible.start).and_then(Clone::clone); } - let row_top = bounds.origin.y + px(index as f32 * ROW_HEIGHT); - let band = selection_band(row_top, anchor, cursor)?; - let text = &self.strings[index]; - selected_range(text, band, left, || pen.measure(text.clone(), window)) + let cell_bounds = self.bounds_for_cell(bounds, cell); + let band = selection_band(cell_bounds.origin.y, anchor, cursor)?; + let text = &self.strings[cell]; + selected_range(text, band, cell_bounds.origin.x, || { + pen.measure(text.clone(), window) + }) }) .collect(); - copy_text(&self.strings[rows], &ranges) + copy_text(&self.strings[cells.clone()], &ranges, cells.start, columns) } - fn paint_gutter( + fn paint_background( &self, - index: usize, - left: Pixels, + row: usize, + bounds: Bounds, top: Pixels, - pen: &Pen, window: &mut Window, - cx: &mut App, ) { - let Row::Line { - origin, - old_number, - new_number, - .. - } = self.rows[index] - else { - return; + let full_width = match &self.rows { + Rows::Unified(rows) => Some(&rows[row]), + Rows::Split(split) => match &split[row] { + SplitRow::Full(full) => Some(full), + SplitRow::Sides { .. } => None, + }, }; + match full_width { + Some(full) => { + if let Some(background) = row_background(full, &self.theme, self.mode) { + let band = Bounds::new( + point(bounds.origin.x, top), + size(bounds.size.width, px(ROW_HEIGHT)), + ); + window.paint_quad(fill(band, background)); + } + } + None => { + for column in 0..self.rows.columns() { + let Some(background) = self.rows.side(row, column).and_then(|side| { + line_colors(side.origin, self.mode, &self.theme).background + }) else { + continue; + }; + let band = Bounds::new( + point(self.column_left(bounds, column), top), + size(self.column_width(bounds), px(ROW_HEIGHT)), + ); + window.paint_quad(fill(band, background)); + } + } + } + + for column in 1..self.rows.columns() { + let rule = Bounds::new( + point(self.column_left(bounds, column), top), + size(px(COLUMN_RULE_WIDTH), px(ROW_HEIGHT)), + ); + window.paint_quad(fill(rule, self.theme.border)); + } + } + + fn paint_gutter( + &self, + row: usize, + column: usize, + cell_bounds: Bounds, + pen: &Pen, + window: &mut Window, + cx: &mut App, + ) { + let left = cell_bounds.origin.x - px(self.rows.code_left()); + let top = cell_bounds.origin.y; let muted = self.theme.muted_foreground; - paint_number( - old_number, - left + px(GUTTER_WIDTH), - top, - muted, - pen, - window, - cx, - ); - paint_number( - new_number, - left + px(2. * GUTTER_WIDTH), - top, - muted, - pen, - window, - cx, - ); + let (origin, marker_left) = match &self.rows { + Rows::Unified(rows) => { + let Row::Line { + origin, + old_number, + new_number, + .. + } = rows[row] + else { + return; + }; + paint_number( + old_number, + left + px(GUTTER_WIDTH), + top, + muted, + pen, + window, + cx, + ); + paint_number( + new_number, + left + px(2. * GUTTER_WIDTH), + top, + muted, + pen, + window, + cx, + ); + (origin, left + px(2. * GUTTER_WIDTH + MARKER_PADDING)) + } + Rows::Split(_) => { + let Some(side) = self.rows.side(row, column) else { + return; + }; + paint_number( + side.number, + left + px(GUTTER_WIDTH), + top, + muted, + pen, + window, + cx, + ); + (side.origin, left + px(GUTTER_WIDTH + MARKER_PADDING)) + } + }; let foreground = line_colors(origin, self.mode, &self.theme).foreground; let line = pen.shape(marker(origin).into(), foreground, window); - paint_line( - &line, - point(left + px(2. * GUTTER_WIDTH + MARKER_PADDING), top), - window, - cx, - ); + paint_line(&line, point(marker_left, top), window, cx); } } @@ -460,12 +644,12 @@ impl Element for DiffBody { ) -> (LayoutId, Self::RequestLayoutState) { self.visible = self.visible_rows(); self.texts = self - .visible - .clone() - .map(|index| { - styled_row( - &self.rows[index], - self.strings[index].clone(), + .visible_cells() + .map(|cell| { + styled_cell( + &self.rows, + cell, + self.strings[cell].clone(), &self.theme, self.mode, ) @@ -499,20 +683,19 @@ impl Element for DiffBody { window: &mut Window, cx: &mut App, ) -> Self::PrepaintState { - self.row_bounds = self - .visible - .clone() - .map(|index| self.bounds_for_row(bounds, index)) + self.cell_bounds = self + .visible_cells() + .map(|cell| self.bounds_for_cell(bounds, cell)) .collect(); - for (text, row_bounds) in self.texts.iter_mut().zip(&self.row_bounds) { - text.prepaint(None, None, *row_bounds, &mut (), window, cx); + for (text, cell_bounds) in self.texts.iter_mut().zip(&self.cell_bounds) { + text.prepaint(None, None, *cell_bounds, &mut (), window, cx); } let hitbox = window.insert_hitbox(bounds, HitboxBehavior::Normal); self.selection.register( TextSelectionRegistration::new(hitbox.clone(), bounds) .with_document_order(0) - .with_text_bounds(self.row_bounds.clone()), + .with_text_bounds(self.cell_bounds.clone()), window, cx, ); @@ -529,18 +712,19 @@ impl Element for DiffBody { window: &mut Window, cx: &mut App, ) { + let first_cell = self.visible_cells().start; let runs: Vec = self .texts .iter() .enumerate() .map(|(offset, text)| { - let index = self.visible.start + offset; + let cell = first_cell + offset; TextSelectionRun::new( - self.strings[index].clone(), + self.strings[cell].clone(), text.layout().clone(), - self.row_bounds[offset], + self.cell_bounds[offset], ) - .with_document_order(index as u64) + .with_document_order(cell as u64) }) .collect(); let projection = self.selection.update_runs(&runs, cx); @@ -549,29 +733,34 @@ impl Element for DiffBody { let selected = self.copy_selection(bounds, projection.ranges(), &pen, window, cx); self.selection.set_fallback_copy_text(selected, cx); - for (offset, index) in self.visible.clone().enumerate() { - let row_bounds = self.row_bounds[offset]; - let top = row_bounds.origin.y; - - if let Some(background) = row_background(&self.rows[index], &self.theme, self.mode) { - let band = Bounds::new( - point(bounds.origin.x, top), - size(bounds.size.width, px(ROW_HEIGHT)), - ); - window.paint_quad(fill(band, background)); - } + let columns = self.rows.columns(); + for (offset, row) in self.visible.clone().enumerate() { + let top = self.cell_bounds[offset * columns].origin.y; + self.paint_background(row, bounds, top, window); + + for column in 0..columns { + let cell_offset = offset * columns + column; + let cell_bounds = self.cell_bounds[cell_offset]; + if let Some(range) = projection.ranges().get(cell_offset).and_then(Clone::clone) { + paint_selection( + self.texts[cell_offset].layout(), + range, + self.theme.selection, + window, + ); + } - if let Some(range) = projection.ranges().get(offset).and_then(Clone::clone) { - paint_selection( - self.texts[offset].layout(), - range, - self.theme.selection, + self.paint_gutter(row, column, cell_bounds, &pen, window, cx); + self.texts[cell_offset].paint( + None, + None, + cell_bounds, + &mut (), + &mut (), window, + cx, ); } - - self.paint_gutter(index, bounds.origin.x, top, &pen, window, cx); - self.texts[offset].paint(None, None, row_bounds, &mut (), &mut (), window, cx); } } } @@ -588,7 +777,7 @@ mod tests { SharedString::from("three"), ]; let ranges = vec![Some(1..3), Some(0..3), None]; - assert_eq!(copy_text(&strings, &ranges), "ne\ntwo"); + assert_eq!(copy_text(&strings, &ranges, 0, 1), "ne\ntwo"); } #[test] @@ -599,13 +788,55 @@ mod tests { SharedString::from("three"), ]; let ranges = vec![Some(0..3), None, Some(0..5)]; - assert_eq!(copy_text(&strings, &ranges), "one\n\nthree"); + assert_eq!(copy_text(&strings, &ranges, 0, 1), "one\n\nthree"); } #[test] fn copy_text_of_an_empty_projection_is_empty() { let strings = vec![SharedString::from("one")]; - assert_eq!(copy_text(&strings, &[None]), ""); + assert_eq!(copy_text(&strings, &[None], 0, 1), ""); + } + + #[test] + fn copy_text_separates_two_columns_of_the_same_row_with_a_tab() { + let strings = vec![ + SharedString::from("gone"), + SharedString::from("new"), + SharedString::from("keep"), + SharedString::from("keep"), + ]; + let ranges = vec![Some(0..4), Some(0..3), Some(0..4), Some(0..4)]; + assert_eq!(copy_text(&strings, &ranges, 0, 2), "gone\tnew\nkeep\tkeep"); + } + + #[test] + fn copy_text_of_a_padded_column_inside_the_selection_keeps_its_empty_field() { + let strings = vec![ + SharedString::from("gone"), + SharedString::from(""), + SharedString::from("keep"), + SharedString::from("keep"), + ]; + let ranges = vec![Some(0..4), None, Some(0..4), Some(0..4)]; + assert_eq!(copy_text(&strings, &ranges, 0, 2), "gone\t\nkeep\tkeep"); + } + + #[test] + fn copy_text_starts_at_the_first_selected_column_rather_than_at_a_leading_pad() { + let strings = vec![SharedString::from(""), SharedString::from("new")]; + let ranges = vec![None, Some(0..3)]; + assert_eq!(copy_text(&strings, &ranges, 0, 2), "new"); + } + + #[test] + fn copy_text_of_a_span_starting_mid_row_keeps_the_row_boundaries_aligned() { + let strings = vec![ + SharedString::from("new"), + SharedString::from("keep"), + SharedString::from("keep"), + ]; + let ranges = vec![Some(0..3), Some(0..4), Some(0..4)]; + assert_eq!(copy_text(&strings, &ranges, 1, 2), "new\nkeep\tkeep"); } #[test] diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index e3cb79f..4650eed 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -10,10 +10,20 @@ //! carried. All three follow from the diff being one text document, so the rows are built //! here instead — see the design note under `docs/superpowers/specs/`. //! +//! A row carries one cell in [`DiffViewMode::Unified`] and two in [`DiffViewMode::Split`], +//! which is the only difference between the two views: [`body::Rows`] answers how many +//! columns a body has, and every horizontal position — the content width, a cell's bounds, +//! a gutter's origin — is that column's share of the element's width. A file, hunk or +//! placeholder row keeps one full-width cell in either view, so the columns stay uniform +//! and a cell is `row * columns + column` rather than a lookup. Because a column is laid +//! out against `content_width / columns` and the width is measured over every cell, a +//! full-width header still fits in the half it is drawn in. +//! //! Selection comes back through `gpui-base`'s window-level participant system rather than //! from the editor. [`body`] is the element that joins it: it registers one participant and -//! declares one run per row on screen, and only the code text becomes a run — the gutters -//! and the marker are painted directly and never registered, which is what keeps line +//! declares one run per cell on screen, left before right within a row, and only the code +//! text becomes a run — the gutters and the marker are painted directly and never +//! registered, which is what keeps line //! numbers and markers out of the clipboard. The rows scroll on both axes rather than soft-wrapping, //! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in @@ -38,25 +48,29 @@ //! bottom edge would come back holding only the rows that happened to be on screen, and //! nothing would report that it had been cut short. `body::DiffBody::copy_selection` derives //! the row span from the selection's own window points instead — `body::selected_rows` — and -//! asks `body::selection_band` and `body::selected_range` for each row's byte range, shaping -//! only the at most two rows whose ends the selection cuts through; every row between them is -//! whole. Endpoints survive scrolling because `gpui-base` stores them relative to +//! asks `body::selection_band` and `body::selected_range` for each cell's byte range, shaping +//! only the cells of the at most two rows whose ends the selection cuts through; every row +//! between them is whole. A band is a property of the row, so both cells of a row share it +//! and a cell's own column offset is what turns it into a range — which is exactly what +//! `point_in_selection_band` does to two runs that share a `y`, so the arithmetic and the +//! projection still agree cell for cell. Endpoints survive scrolling because `gpui-base` +//! stores them relative to //! `bounds.origin`, which already carries the scroll, so a point off the top of the viewport //! is a negative `y` rather than a lost one. Rows that *are* on screen keep the projection's //! own range, so what is highlighted and what is copied cannot drift apart. //! //! What stays unwindowed is the content width: `body::DiffBody::content_width` shapes every -//! row on every layout pass, because the horizontal scroll extent has to consider rows that +//! cell on every layout pass, because the horizontal scroll extent has to consider rows that //! are not on screen or the scrollbar would resize as the view scrolls vertically, and -//! because the children are laid out against a width that provably exceeds every row's +//! because a cell is laid out against a column width that provably exceeds every cell's //! natural width — which is what keeps a row one line tall and `ROW_HEIGHT` true. After the //! first frame those are hits in gpui's line-layout cache, so the cost is a hash of each -//! row's bytes rather than a reshape. +//! cell's bytes rather than a reshape. //! //! Copying depends on a fact `gpui-base`'s own doc comment does not state. A participant's //! runs concatenate with no separator when `update_runs` projects them //! (`text_selection.rs:593`); only whole participants are joined with `"\n"` -//! (`resolve_copy_items`, `:513`). So the row separator is computed here, in +//! (`resolve_copy_items`, `:513`). So the separators are computed here, in //! [`body::copy_text`], and published through `TextSelectionHandle::set_fallback_copy_text` //! right after `update_runs` — which works only because that setter also clears //! `projected_copy_text` (`:559`), the field `update_runs` just set, so `copy_item` falls @@ -64,12 +78,22 @@ //! default branch pinned solely by `Cargo.lock`; if that clearing behaviour ever moves, //! copying silently goes back to gluing rows together with no separator, and nothing fails //! to say so. +//! +//! A row ends with `"\n"` and a column with `"\t"`, which makes a split copy the table the +//! reader is looking at. The alternative — a newline between the two columns as well — was +//! rejected because the band rule takes both cells of every row a selection passes through +//! whole, so a drag down one column still copies the other; newlines would interleave the +//! two sides and repeat every context line, and neither separator can yield compilable code +//! out of a two-column view. Narrowing the copy to one column was rejected for a harder +//! reason: the highlight comes from the projection, and dropping a cell the projection +//! selected is exactly the drift between clipboard and screen the paragraph above exists to +//! prevent. mod body; mod model; -#[allow(dead_code)] mod pairing; mod palette; +mod split; use domain::Patch; use gpui::{ @@ -82,11 +106,15 @@ use gpui_component::{ scroll::{ScrollableElement as _, ScrollbarAxis}, }; -use body::{ROW_HEIGHT, body}; +use crate::diff_view_mode::DiffViewMode; + +use body::{ROW_HEIGHT, Rows, body}; use model::rows; +use split::split_rows; pub(super) fn render( patch: &Patch, + mode: DiffViewMode, selection: &TextSelectionHandle, scroll: &ScrollHandle, cx: &App, @@ -102,6 +130,11 @@ pub(super) fn render( .into_any_element(); } + let content = match mode { + DiffViewMode::Unified => Rows::Unified(rows(patch)), + DiffViewMode::Split => Rows::Split(split_rows(patch)), + }; + let theme = cx.theme(); div() .relative() @@ -117,7 +150,7 @@ pub(super) fn render( .text_size(theme.mono_font_size) .line_height(px(ROW_HEIGHT)) .child(body( - rows(patch), + content, selection.clone(), scroll.clone(), theme.colors, diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index 41cd8e7..ce74fcd 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -1,4 +1,4 @@ -use domain::{DiffLine, FilePatch, LineOrigin, Patch}; +use domain::{DiffLine, FilePatch, Hunk, LineOrigin, Patch}; use crate::detail::format; @@ -25,39 +25,53 @@ pub(super) enum Row { pub(super) fn rows(patch: &Patch) -> Vec { let mut rows = Vec::new(); for file in &patch.files { - rows.push(Row::FileHeader { - path: file - .display_path() - .map(|p| p.display().to_string()) - .unwrap_or_default(), - stat: file_stat(file), - }); + rows.push(file_header(file)); push_body(&mut rows, file); } rows } +pub(super) fn file_header(file: &FilePatch) -> Row { + Row::FileHeader { + path: file + .display_path() + .map(|p| p.display().to_string()) + .unwrap_or_default(), + stat: file_stat(file), + } +} + pub(super) fn file_stat(file: &FilePatch) -> String { format!("+{} \u{2212}{}", file.added_lines(), file.deleted_lines()) } -fn push_body(rows: &mut Vec, file: &FilePatch) { +pub(super) fn placeholder(file: &FilePatch) -> Option { if file.is_binary { - rows.push(Row::Placeholder { + return Some(Row::Placeholder { message: "Binary file not shown.", }); - return; } if file.hunks.is_empty() { - rows.push(Row::Placeholder { + return Some(Row::Placeholder { message: "No content changes.", }); + } + None +} + +pub(super) fn hunk_header(hunk: &Hunk) -> Row { + Row::HunkHeader { + text: format::hunk_heading(hunk), + } +} + +fn push_body(rows: &mut Vec, file: &FilePatch) { + if let Some(placeholder) = placeholder(file) { + rows.push(placeholder); return; } for hunk in &file.hunks { - rows.push(Row::HunkHeader { - text: format::hunk_heading(hunk), - }); + rows.push(hunk_header(hunk)); rows.extend(hunk.lines.iter().map(line_row)); } } diff --git a/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs new file mode 100644 index 0000000..e9612f8 --- /dev/null +++ b/crates/ui/src/detail/diff/split.rs @@ -0,0 +1,189 @@ +use domain::Patch; + +use super::model::{Row, file_header, hunk_header, placeholder}; +use super::pairing::{SideLine, pair}; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) enum SplitRow { + Full(Row), + Sides { + left: Option, + right: Option, + }, +} + +pub(super) fn split_rows(patch: &Patch) -> Vec { + let mut rows = Vec::new(); + for file in &patch.files { + rows.push(SplitRow::Full(file_header(file))); + if let Some(placeholder) = placeholder(file) { + rows.push(SplitRow::Full(placeholder)); + continue; + } + for hunk in &file.hunks { + rows.push(SplitRow::Full(hunk_header(hunk))); + rows.extend(pair(&hunk.lines).into_iter().map(|row| SplitRow::Sides { + left: row.left, + right: row.right, + })); + } + } + rows +} + +#[cfg(test)] +mod tests { + use super::*; + use domain::{DiffLine, FilePatch, FileStatus, Hunk, LineOrigin}; + use std::path::PathBuf; + + fn line(origin: LineOrigin, old: Option, new: Option, content: &str) -> DiffLine { + DiffLine { + origin, + old_number: old, + new_number: new, + content: content.to_string(), + } + } + + fn hunk(lines: Vec) -> Hunk { + Hunk { + old_start: 1, + old_lines: 1, + new_start: 1, + new_lines: 1, + heading: String::new(), + lines, + } + } + + fn file(path: &str, hunks: Vec, is_binary: bool) -> FilePatch { + FilePatch { + old_path: Some(PathBuf::from(path)), + new_path: Some(PathBuf::from(path)), + status: FileStatus::Modified, + is_binary, + hunks, + } + } + + fn replacement() -> Hunk { + hunk(vec![ + line(LineOrigin::Deletion, Some(1), None, "gone"), + line(LineOrigin::Addition, None, Some(1), "new"), + ]) + } + + fn paths(rows: &[SplitRow]) -> Vec { + rows.iter() + .filter_map(|row| match row { + SplitRow::Full(Row::FileHeader { path, .. }) => Some(path.clone()), + _ => None, + }) + .collect() + } + + #[test] + fn a_two_file_patch_yields_both_files_in_order_each_behind_its_own_header() { + let patch = Patch { + files: vec![ + file("src/a.rs", vec![replacement()], false), + file("src/b.rs", vec![replacement()], false), + ], + }; + + let rows = split_rows(&patch); + + assert_eq!(paths(&rows), vec!["src/a.rs", "src/b.rs"]); + assert!(matches!(rows[1], SplitRow::Full(Row::HunkHeader { .. }))); + assert!(matches!(rows[2], SplitRow::Sides { .. })); + assert_eq!(rows.len(), 6); + } + + #[test] + fn a_replacement_pairs_the_deletion_against_the_addition_on_one_row() { + let patch = Patch { + files: vec![file("src/a.rs", vec![replacement()], false)], + }; + + let rows = split_rows(&patch); + + let SplitRow::Sides { left, right } = &rows[2] else { + panic!("the third row is the paired line"); + }; + assert_eq!( + left.as_ref().map(|side| side.content.as_str()), + Some("gone") + ); + assert_eq!( + right.as_ref().map(|side| side.content.as_str()), + Some("new") + ); + } + + #[test] + fn a_pure_addition_leaves_the_left_column_empty_rather_than_collapsing_the_row() { + let patch = Patch { + files: vec![file( + "src/a.rs", + vec![hunk(vec![line(LineOrigin::Addition, None, Some(1), "new")])], + false, + )], + }; + + let rows = split_rows(&patch); + + let SplitRow::Sides { left, right } = &rows[2] else { + panic!("the third row is the added line"); + }; + assert!(left.is_none()); + assert!(right.is_some()); + } + + #[test] + fn a_binary_file_yields_a_full_width_placeholder_instead_of_columns() { + let patch = Patch { + files: vec![file("src/a.png", Vec::new(), true)], + }; + + let rows = split_rows(&patch); + + assert_eq!( + rows[1], + SplitRow::Full(Row::Placeholder { + message: "Binary file not shown." + }) + ); + assert_eq!(rows.len(), 2); + } + + #[test] + fn a_file_with_no_hunks_yields_a_no_change_placeholder() { + let patch = Patch { + files: vec![file("src/a.rs", Vec::new(), false)], + }; + + let rows = split_rows(&patch); + + assert_eq!( + rows[1], + SplitRow::Full(Row::Placeholder { + message: "No content changes." + }) + ); + } + + #[test] + fn every_hunk_of_a_file_keeps_its_own_header() { + let patch = Patch { + files: vec![file("src/a.rs", vec![replacement(), replacement()], false)], + }; + + let headers = split_rows(&patch) + .iter() + .filter(|row| matches!(row, SplitRow::Full(Row::HunkHeader { .. }))) + .count(); + + assert_eq!(headers, 2); + } +} diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index 66bd6f4..1b4175c 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -1,5 +1,6 @@ //! The right dock's commit detail panel: a tab bar switching between commit metadata and -//! the diff, above whichever of the two is selected. +//! the diff, above whichever of the two is selected. A second segmented bar sits beside it +//! while the diff is open, choosing between the unified and the side-by-side view. //! //! [`DetailPanel`] renders exactly the [`LoadState`] it is handed — it never reads a //! repository itself. [`metadata::render_header`] and [`metadata::render_description`] @@ -22,6 +23,13 @@ //! derived from the patch during the render that follows. //! [`DetailPanel::selected_tab`] is never touched by `set_detail`, which is what lets //! picking a different commit leave the open tab alone. +//! +//! The diff view mode is read once from disk in [`DetailPanel::new`], before the first +//! frame, and written back through `cx.background_executor()`: +//! [`crate::persistence::save_diff_view_mode`] blocks on file I/O and a toggle is a frame +//! event, so it is saved the way [`crate::workspace::Workspace`] saves the theme rather +//! than inline. Switching modes also zeroes the diff's scroll offset, as `set_detail` does, +//! because a row index means something different in each view. mod diff; mod format; @@ -32,7 +40,7 @@ use std::sync::Arc; use gpui::{ AnyElement, App, Context, EventEmitter, FocusHandle, Focusable, InteractiveElement as _, IntoElement, ParentElement as _, Point, Render, ScrollHandle, StatefulInteractiveElement as _, - Styled as _, Window, div, + Styled as _, Window, div, prelude::FluentBuilder as _, }; use gpui_base::TextSelectionHandle; use gpui_component::{ @@ -44,6 +52,8 @@ use gpui_component::{ tab::{Tab, TabBar}, }; +use crate::diff_view_mode::DiffViewMode; +use crate::persistence; use crate::repository::{CommitDetail, LoadState}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -75,6 +85,7 @@ impl DetailTab { pub struct DetailPanel { detail: LoadState>, diff_selection: TextSelectionHandle, + diff_view_mode: DiffViewMode, selected_tab: DetailTab, general_scroll_handle: ScrollHandle, diff_scroll_handle: ScrollHandle, @@ -88,6 +99,7 @@ impl DetailPanel { Self { detail: LoadState::Idle, diff_selection, + diff_view_mode: persistence::load_diff_view_mode().unwrap_or_default(), selected_tab: DetailTab::default(), general_scroll_handle: ScrollHandle::new(), diff_scroll_handle: ScrollHandle::new(), @@ -100,6 +112,24 @@ impl DetailPanel { self.diff_scroll_handle.set_offset(Point::default()); cx.notify(); } + + fn set_diff_view_mode(&mut self, mode: DiffViewMode, cx: &mut Context) { + if mode == self.diff_view_mode { + return; + } + self.diff_view_mode = mode; + self.diff_scroll_handle.set_offset(Point::default()); + + cx.background_executor() + .spawn(async move { + if let Err(error) = persistence::save_diff_view_mode(&mode) { + eprintln!("gitr: failed to save diff view mode: {error:#}"); + } + }) + .detach(); + + cx.notify(); + } } impl Panel for DetailPanel { @@ -127,11 +157,12 @@ impl Focusable for DetailPanel { impl Render for DetailPanel { fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { let selected_tab = self.selected_tab; + let diff_view_mode = self.diff_view_mode; div() .size_full() .flex() .flex_col() - .child(tab_bar(selected_tab, cx)) + .child(tab_bar(selected_tab, diff_view_mode, cx)) .child(match &self.detail { LoadState::Idle => centered_message(cx, "Select a commit to see its details."), LoadState::Loading => loading_state(cx), @@ -139,6 +170,7 @@ impl Render for DetailPanel { LoadState::Ready(detail) => ready_state( detail, selected_tab, + diff_view_mode, &self.diff_selection, &self.general_scroll_handle, &self.diff_scroll_handle, @@ -148,7 +180,11 @@ impl Render for DetailPanel { } } -fn tab_bar(selected: DetailTab, cx: &mut Context) -> AnyElement { +fn tab_bar( + selected: DetailTab, + diff_view_mode: DiffViewMode, + cx: &mut Context, +) -> AnyElement { let mut tabs = TabBar::new("detail-tabs") .segmented() .small() @@ -163,6 +199,7 @@ fn tab_bar(selected: DetailTab, cx: &mut Context) -> AnyElement { div() .flex_shrink_0() + .flex() .items_center() .gap_2() .px_2() @@ -170,9 +207,26 @@ fn tab_bar(selected: DetailTab, cx: &mut Context) -> AnyElement { .border_b_1() .border_color(cx.theme().border) .child(tabs) + .when(selected == DetailTab::Diff, |row| { + row.child(diff_view_mode_bar(diff_view_mode, cx)) + }) .into_any_element() } +fn diff_view_mode_bar(selected: DiffViewMode, cx: &mut Context) -> AnyElement { + let mut modes = TabBar::new("diff-view-mode") + .segmented() + .small() + .selected_index(selected.index()) + .on_click(cx.listener(|this, index: &usize, _, cx| { + this.set_diff_view_mode(DiffViewMode::from_index(*index), cx); + })); + for mode in DiffViewMode::ALL { + modes = modes.child(Tab::new().label(mode.label())); + } + modes.into_any_element() +} + fn centered_message(cx: &App, message: &str) -> AnyElement { div() .flex_1() @@ -214,6 +268,7 @@ fn failed_state(message: &str) -> AnyElement { fn ready_state( detail: &CommitDetail, selected_tab: DetailTab, + diff_view_mode: DiffViewMode, diff_selection: &TextSelectionHandle, general_scroll_handle: &ScrollHandle, diff_scroll_handle: &ScrollHandle, @@ -221,7 +276,13 @@ fn ready_state( ) -> AnyElement { match selected_tab { DetailTab::General => general_tab(detail, general_scroll_handle, cx), - DetailTab::Diff => diff_tab(detail, diff_selection, diff_scroll_handle, cx), + DetailTab::Diff => diff_tab( + detail, + diff_view_mode, + diff_selection, + diff_scroll_handle, + cx, + ), } } @@ -252,6 +313,7 @@ fn general_tab(detail: &CommitDetail, scroll_handle: &ScrollHandle, cx: &App) -> fn diff_tab( detail: &CommitDetail, + mode: DiffViewMode, selection: &TextSelectionHandle, scroll_handle: &ScrollHandle, cx: &App, @@ -260,6 +322,12 @@ fn diff_tab( .flex_1() .min_h_0() .min_w_0() - .child(diff::render(&detail.patch, selection, scroll_handle, cx)) + .child(diff::render( + &detail.patch, + mode, + selection, + scroll_handle, + cx, + )) .into_any_element() } From 3e8b6970a10809bb30dcb6d96c383a1307300317 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 16:11:16 +0200 Subject: [PATCH 14/19] fix(diff): clear the selection when the diff underneath it changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A selection endpoint is stored relative to bounds.origin (text_selection.rs:1336), which is what makes it survive scrolling — and also what makes it survive the content being replaced. Toggling to split view or picking a different commit left the stored y resolving onto whatever row now sits at that offset: a highlight over lines nobody dragged across, and a Cmd-C that copies them. Highlight and clipboard still agreed with each other, so this was not the drift the geometry-derived copy guards against, but it is surprising in a way nothing on screen explains. set_detail and set_diff_view_mode now share reset_diff_view, which zeroes the diff scroll offset and calls TextSelection::clear. Both take a &mut Window for it, which is why sync_panels_from_repository takes one too; both of its callers already had one in scope. The clear is window-wide rather than per-participant because TextSelectionHandle exposes no clear of its own and WindowSelectionState is private — which costs nothing here, since the diff body is the only participant this crate registers. Also stops the column rule crossing full-width rows. The loop ran outside the match on whether a row has one cell or two, so a 1px border cut through every file and hunk header; GitHub draws those bands unbroken. Moving it into the two-cell arm is the whole fix. The split-view mirror case: a pure deletion leaving the right column empty was the only shape of pairing.rs's fan-out that split_rows did not assert for itself. --- crates/ui/src/detail/diff/body.rs | 16 ++++++------ crates/ui/src/detail/diff/mod.rs | 8 +++--- crates/ui/src/detail/diff/split.rs | 24 +++++++++++++++++ crates/ui/src/detail/mod.rs | 42 +++++++++++++++++++++++------- crates/ui/src/workspace.rs | 15 ++++++++--- 5 files changed, 80 insertions(+), 25 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index e8be0d7..b307d98 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -537,15 +537,15 @@ impl DiffBody { ); window.paint_quad(fill(band, background)); } - } - } - for column in 1..self.rows.columns() { - let rule = Bounds::new( - point(self.column_left(bounds, column), top), - size(px(COLUMN_RULE_WIDTH), px(ROW_HEIGHT)), - ); - window.paint_quad(fill(rule, self.theme.border)); + for column in 1..self.rows.columns() { + let rule = Bounds::new( + point(self.column_left(bounds, column), top), + size(px(COLUMN_RULE_WIDTH), px(ROW_HEIGHT)), + ); + window.paint_quad(fill(rule, self.theme.border)); + } + } } } diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 4650eed..92537e4 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -23,8 +23,8 @@ //! from the editor. [`body`] is the element that joins it: it registers one participant and //! declares one run per cell on screen, left before right within a row, and only the code //! text becomes a run — the gutters and the marker are painted directly and never -//! registered, which is what keeps line -//! numbers and markers out of the clipboard. The rows scroll on both axes rather than soft-wrapping, +//! registered, which is what keeps line numbers and markers out of the clipboard. The rows +//! scroll on both axes rather than soft-wrapping, //! matching GitHub. `restrict_scroll_to_axis` still earns its place here, but not for the //! reason it usually does: the container's `overflow_scroll()` puts both axes in //! `Overflow::Scroll`, and gpui's vertical-onto-horizontal remap (`div.rs:3220-3224`, @@ -54,8 +54,8 @@ //! and a cell's own column offset is what turns it into a range — which is exactly what //! `point_in_selection_band` does to two runs that share a `y`, so the arithmetic and the //! projection still agree cell for cell. Endpoints survive scrolling because `gpui-base` -//! stores them relative to -//! `bounds.origin`, which already carries the scroll, so a point off the top of the viewport +//! stores them relative to `bounds.origin`, which already carries the scroll, so a point off +//! the top of the viewport //! is a negative `y` rather than a lost one. Rows that *are* on screen keep the projection's //! own range, so what is highlighted and what is copied cannot drift apart. //! diff --git a/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs index e9612f8..33eadb6 100644 --- a/crates/ui/src/detail/diff/split.rs +++ b/crates/ui/src/detail/diff/split.rs @@ -140,6 +140,30 @@ mod tests { assert!(right.is_some()); } + #[test] + fn a_pure_deletion_leaves_the_right_column_empty_rather_than_collapsing_the_row() { + let patch = Patch { + files: vec![file( + "src/a.rs", + vec![hunk(vec![line( + LineOrigin::Deletion, + Some(1), + None, + "gone", + )])], + false, + )], + }; + + let rows = split_rows(&patch); + + let SplitRow::Sides { left, right } = &rows[2] else { + panic!("the third row is the deleted line"); + }; + assert!(left.is_some()); + assert!(right.is_none()); + } + #[test] fn a_binary_file_yields_a_full_width_placeholder_instead_of_columns() { let patch = Patch { diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index 1b4175c..32e9fa8 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -28,8 +28,17 @@ //! frame, and written back through `cx.background_executor()`: //! [`crate::persistence::save_diff_view_mode`] blocks on file I/O and a toggle is a frame //! event, so it is saved the way [`crate::workspace::Workspace`] saves the theme rather -//! than inline. Switching modes also zeroes the diff's scroll offset, as `set_detail` does, -//! because a row index means something different in each view. +//! than inline. +//! +//! Both `set_detail` and a mode change route through `DetailPanel::reset_diff_view`, which +//! zeroes the diff's scroll offset and clears the window selection. Clearing is the reason +//! both take a `&mut Window`, which is why [`crate::workspace::Workspace`] threads one into +//! `sync_panels_from_repository`. It is not optional: `gpui-base` stores a selection +//! endpoint relative to `bounds.origin` (`text_selection.rs:1336`), so it survives the +//! content underneath it changing, and a stored `y` then resolves onto whatever row now sits +//! at that offset — a highlight over lines the user never dragged across, which Cmd-C would +//! copy. `TextSelection::clear` is window-wide rather than per-participant, which costs +//! nothing here because the diff body is the only participant this crate registers. mod diff; mod format; @@ -42,7 +51,7 @@ use gpui::{ IntoElement, ParentElement as _, Point, Render, ScrollHandle, StatefulInteractiveElement as _, Styled as _, Window, div, prelude::FluentBuilder as _, }; -use gpui_base::TextSelectionHandle; +use gpui_base::{TextSelection, TextSelectionHandle}; use gpui_component::{ ActiveTheme as _, Sizable as _, alert::Alert, @@ -107,18 +116,28 @@ impl DetailPanel { } } - pub fn set_detail(&mut self, detail: LoadState>, cx: &mut Context) { + pub fn set_detail( + &mut self, + detail: LoadState>, + window: &mut Window, + cx: &mut Context, + ) { self.detail = detail; - self.diff_scroll_handle.set_offset(Point::default()); + self.reset_diff_view(window, cx); cx.notify(); } - fn set_diff_view_mode(&mut self, mode: DiffViewMode, cx: &mut Context) { + fn set_diff_view_mode( + &mut self, + mode: DiffViewMode, + window: &mut Window, + cx: &mut Context, + ) { if mode == self.diff_view_mode { return; } self.diff_view_mode = mode; - self.diff_scroll_handle.set_offset(Point::default()); + self.reset_diff_view(window, cx); cx.background_executor() .spawn(async move { @@ -130,6 +149,11 @@ impl DetailPanel { cx.notify(); } + + fn reset_diff_view(&mut self, window: &mut Window, cx: &mut Context) { + self.diff_scroll_handle.set_offset(Point::default()); + TextSelection::clear(window, cx); + } } impl Panel for DetailPanel { @@ -218,8 +242,8 @@ fn diff_view_mode_bar(selected: DiffViewMode, cx: &mut Context) -> .segmented() .small() .selected_index(selected.index()) - .on_click(cx.listener(|this, index: &usize, _, cx| { - this.set_diff_view_mode(DiffViewMode::from_index(*index), cx); + .on_click(cx.listener(|this, index: &usize, window, cx| { + this.set_diff_view_mode(DiffViewMode::from_index(*index), window, cx); })); for mode in DiffViewMode::ALL { modes = modes.child(Tab::new().label(mode.label())); diff --git a/crates/ui/src/workspace.rs b/crates/ui/src/workspace.rs index 011dfbe..e586594 100644 --- a/crates/ui/src/workspace.rs +++ b/crates/ui/src/workspace.rs @@ -295,7 +295,7 @@ impl Workspace { let this = cx.entity().downgrade(); history_panel.update(cx, |panel, cx| panel.set_workspace(this, cx)); - sync_panels_from_repository(&repository, &history_panel, &detail_panel, cx); + sync_panels_from_repository(&repository, &history_panel, &detail_panel, window, cx); let repository_subscription = cx.subscribe_in(&repository, window, Self::on_repository_event); @@ -668,7 +668,7 @@ impl Workspace { RepositoryEvent::SelectionChanged => { let detail = repository.read(cx).detail().clone(); self.detail_panel - .update(cx, |panel, cx| panel.set_detail(detail, cx)); + .update(cx, |panel, cx| panel.set_detail(detail, window, cx)); if repository.read(cx).selected().is_none() { self.dismiss_detail(window, cx); } @@ -825,7 +825,13 @@ impl Workspace { let (path, watch) = repository_path_and_watch(&project.source); let repository = cx.new(|cx| RepositoryState::open(path, watch, cx)); - sync_panels_from_repository(&repository, &self.history_panel, &self.detail_panel, cx); + sync_panels_from_repository( + &repository, + &self.history_panel, + &self.detail_panel, + window, + cx, + ); self.history_panel .update(cx, |panel, cx| panel.reset_for_new_repository(cx)); @@ -1293,6 +1299,7 @@ fn sync_panels_from_repository( repository: &Entity, history_panel: &Entity, detail_panel: &Entity, + window: &mut Window, cx: &mut Context, ) { let history = repository.read(cx).history().clone(); @@ -1303,7 +1310,7 @@ fn sync_panels_from_repository( panel.set_history(history, cx); panel.set_head(deletion, head_commit(&head), cx); }); - detail_panel.update(cx, |panel, cx| panel.set_detail(detail, cx)); + detail_panel.update(cx, |panel, cx| panel.set_detail(detail, window, cx)); } fn deletion_context(repository: &Entity, cx: &App) -> Deletion { From 3fac4906d4029510d78648fd1ba3e5eda5bcc266 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 17:13:59 +0200 Subject: [PATCH 15/19] perf(diff): derive the row model once per patch rather than once per frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spec's data model says both row lists are derived when the patch or the view mode changes, never per frame. `render` did the opposite: it ran `rows` or `split_rows` on every frame and `body` then built one `SharedString` per cell from the result. That is not a rare frame — `DetailPanel::new` attaches `refresh_window_on_change`, so a selection drag repaints on every mouse move and copied every line of the patch two or three times over each time. `DiffContent` holds the rows and their cell text together, `DetailPanel` rebuilds it in `set_detail` and `set_diff_view_mode` alone, and the element takes it by `Rc`. That is also what `content_width`'s justification wanted all along: the per-frame cost is now a hash against gpui's line-layout cache over bytes that are already there, with no allocation behind it. Three more things follow the rows: `Row::FileHeader` carries the file's `FileStatus` and its two line counts, as the spec wrote it. Dropping the status made a rename render as `new.rs +0 -0` above "No content changes." with no way to see what the old name was; the header now reads `old.rs -> new.rs renamed 87% +0 -0`, on one line. The unmeasured-viewport window is clamped to a screenful. `visible_rows` reads `ScrollHandle::bounds`, written during the container's prepaint and so still unset when this element's `request_layout` first asks, and answering `0..rows` there laid out, prepainted and painted every row of the patch on the first frame the Diff tab is shown. Frame two corrects it either way. `bounds_for_cell`, `column_left` and `column_width` become free functions over `(bounds, columns, code_left, cell)`. They were methods on an element that holds a `TextSelectionHandle`, so nothing could reach them without an `App`, and neither could `cell_text`, `row_text`, `marker`, `row_background` or the `Rows` accessors — the unified/split cell mapping added last, untested. All of it is tested now. --- crates/ui/src/detail/diff/body.rs | 398 +++++++++++++++++++++++------ crates/ui/src/detail/diff/mod.rs | 47 +++- crates/ui/src/detail/diff/model.rs | 145 ++++++++++- crates/ui/src/detail/diff/split.rs | 63 +++++ crates/ui/src/detail/mod.rs | 64 +++-- 5 files changed, 585 insertions(+), 132 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index b307d98..3f9ad5f 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -1,4 +1,5 @@ use std::ops::{Range, RangeInclusive}; +use std::rc::Rc; use domain::LineOrigin; use gpui::{ @@ -10,7 +11,8 @@ use gpui::{ use gpui_base::{TextSelectionHandle, TextSelectionRegistration, TextSelectionRun}; use gpui_component::{ThemeColor, ThemeMode}; -use super::model::Row; +use super::DiffContent; +use super::model::{Row, header_line}; use super::pairing::SideLine; use super::palette::line_colors; use super::split::SplitRow; @@ -25,6 +27,7 @@ const CODE_LEFT: f32 = 2. * GUTTER_WIDTH + MARKER_WIDTH; const SPLIT_CODE_LEFT: f32 = GUTTER_WIDTH + MARKER_WIDTH; const COLUMN_RULE_WIDTH: f32 = 1.; const TRAILING_SPACE: f32 = 16.; +const UNMEASURED_ROWS: usize = 100; pub(super) enum Rows { Unified(Vec), @@ -39,6 +42,10 @@ impl Rows { } } + pub(super) fn is_empty(&self) -> bool { + self.len() == 0 + } + fn columns(&self) -> usize { match self { Rows::Unified(_) => 1, @@ -72,9 +79,14 @@ impl Rows { } } +pub(super) fn cell_strings(rows: &Rows) -> Vec { + (0..rows.cells()) + .map(|cell| cell_text(rows, cell)) + .collect() +} + pub(super) struct DiffBody { - rows: Rows, - strings: Vec, + content: Rc, selection: TextSelectionHandle, scroll: ScrollHandle, theme: ThemeColor, @@ -85,18 +97,14 @@ pub(super) struct DiffBody { } pub(super) fn body( - rows: Rows, + content: Rc, selection: TextSelectionHandle, scroll: ScrollHandle, theme: ThemeColor, mode: ThemeMode, ) -> DiffBody { - let strings: Vec = (0..rows.cells()) - .map(|cell| cell_text(&rows, cell)) - .collect(); DiffBody { - rows, - strings, + content, selection, scroll, theme, @@ -127,48 +135,42 @@ fn cell_text(rows: &Rows, cell: usize) -> SharedString { fn row_text(row: &Row) -> SharedString { match row { - Row::FileHeader { path, stat } => format!("{path} {stat}").into(), + Row::FileHeader { + path, + status, + added, + deleted, + } => header_line(path, status, *added, *deleted).into(), Row::HunkHeader { text } => text.clone().into(), Row::Line { content, .. } => content.clone().into(), Row::Placeholder { message } => (*message).into(), } } -fn styled_cell( - rows: &Rows, - cell: usize, - text: SharedString, - theme: &ThemeColor, - mode: ThemeMode, -) -> StyledText { +fn styled_cell(rows: &Rows, cell: usize, text: SharedString, theme: &ThemeColor) -> StyledText { let range = 0..text.len(); let highlight = HighlightStyle { - color: Some(cell_foreground(rows, cell, theme, mode)), + color: Some(cell_foreground(rows, cell, theme)), ..Default::default() }; StyledText::new(text).with_highlights([(range, highlight)]) } -fn cell_foreground(rows: &Rows, cell: usize, theme: &ThemeColor, mode: ThemeMode) -> Hsla { - let columns = rows.columns(); - let (row, column) = (cell / columns, cell % columns); +fn cell_foreground(rows: &Rows, cell: usize, theme: &ThemeColor) -> Hsla { + let row = cell / rows.columns(); match rows { - Rows::Unified(rows) => row_foreground(&rows[row], theme, mode), + Rows::Unified(rows) => row_foreground(&rows[row], theme), Rows::Split(split) => match &split[row] { - SplitRow::Full(full) => row_foreground(full, theme, mode), - SplitRow::Sides { .. } => rows - .side(row, column) - .map(|side| line_colors(side.origin, mode, theme).foreground) - .unwrap_or(theme.foreground), + SplitRow::Full(full) => row_foreground(full, theme), + SplitRow::Sides { .. } => theme.foreground, }, } } -fn row_foreground(row: &Row, theme: &ThemeColor, mode: ThemeMode) -> Hsla { +fn row_foreground(row: &Row, theme: &ThemeColor) -> Hsla { match row { - Row::FileHeader { .. } => theme.foreground, + Row::FileHeader { .. } | Row::Line { .. } => theme.foreground, Row::HunkHeader { .. } | Row::Placeholder { .. } => theme.muted_foreground, - Row::Line { origin, .. } => line_colors(*origin, mode, theme).foreground, } } @@ -189,6 +191,32 @@ fn marker(origin: LineOrigin) -> &'static str { } } +fn column_width(bounds: Bounds, columns: usize) -> Pixels { + bounds.size.width / columns as f32 +} + +fn column_left(bounds: Bounds, columns: usize, column: usize) -> Pixels { + bounds.origin.x + column_width(bounds, columns) * column as f32 +} + +fn bounds_for_cell( + bounds: Bounds, + columns: usize, + code_left: Pixels, + cell: usize, +) -> Bounds { + Bounds::new( + point( + column_left(bounds, columns, cell % columns) + code_left, + bounds.origin.y + px((cell / columns) as f32 * ROW_HEIGHT), + ), + size( + (column_width(bounds, columns) - code_left).max(px(0.)), + px(ROW_HEIGHT), + ), + ) +} + fn selection_quad_bounds( start: Point, end: Point, @@ -221,7 +249,7 @@ fn selection_quad_bounds( fn row_window(offset_y: Pixels, viewport: Pixels, rows: usize) -> Range { if viewport <= px(0.) { - return 0..rows; + return 0..rows.min(UNMEASURED_ROWS); } let first = ((-offset_y) / px(ROW_HEIGHT)).floor().max(0.) as usize; let count = (viewport / px(ROW_HEIGHT)).ceil() as usize + 2; @@ -407,48 +435,42 @@ fn paint_number( } impl DiffBody { - fn content_width(&self, window: &Window) -> Pixels { - let pen = Pen::new(window); - let mut widest = px(0.); - for text in &self.strings { - widest = widest.max(pen.width(text.clone(), window)); - } - (px(self.rows.code_left()) + widest + px(TRAILING_SPACE)) * self.rows.columns() as f32 + fn rows(&self) -> &Rows { + &self.content.rows } - fn column_width(&self, bounds: Bounds) -> Pixels { - bounds.size.width / self.rows.columns() as f32 + fn strings(&self) -> &[SharedString] { + &self.content.strings } - fn column_left(&self, bounds: Bounds, column: usize) -> Pixels { - bounds.origin.x + self.column_width(bounds) * column as f32 + fn cell_bounds_at(&self, bounds: Bounds, cell: usize) -> Bounds { + bounds_for_cell( + bounds, + self.rows().columns(), + px(self.rows().code_left()), + cell, + ) } - fn bounds_for_cell(&self, bounds: Bounds, cell: usize) -> Bounds { - let columns = self.rows.columns(); - let code_left = px(self.rows.code_left()); - Bounds::new( - point( - self.column_left(bounds, cell % columns) + code_left, - bounds.origin.y + px((cell / columns) as f32 * ROW_HEIGHT), - ), - size( - (self.column_width(bounds) - code_left).max(px(0.)), - px(ROW_HEIGHT), - ), - ) + fn content_width(&self, window: &Window) -> Pixels { + let pen = Pen::new(window); + let mut widest = px(0.); + for text in self.strings() { + widest = widest.max(pen.width(text.clone(), window)); + } + (px(self.rows().code_left()) + widest + px(TRAILING_SPACE)) * self.rows().columns() as f32 } fn visible_rows(&self) -> Range { row_window( self.scroll.offset().y, self.scroll.bounds().size.height, - self.rows.len(), + self.rows().len(), ) } fn visible_cells(&self) -> Range { - let columns = self.rows.columns(); + let columns = self.rows().columns(); self.visible.start * columns..self.visible.end * columns } @@ -473,12 +495,12 @@ impl DiffBody { bounds.origin.y, anchor.y.min(cursor.y), anchor.y.max(cursor.y), - self.rows.len(), + self.rows().len(), ) else { return String::new(); }; - let columns = self.rows.columns(); + let columns = self.rows().columns(); let visible = self.visible_cells(); let cells = rows.start() * columns..(rows.end() + 1) * columns; let ranges: Vec>> = cells @@ -487,16 +509,21 @@ impl DiffBody { if visible.contains(&cell) { return projected.get(cell - visible.start).and_then(Clone::clone); } - let cell_bounds = self.bounds_for_cell(bounds, cell); + let cell_bounds = self.cell_bounds_at(bounds, cell); let band = selection_band(cell_bounds.origin.y, anchor, cursor)?; - let text = &self.strings[cell]; + let text = &self.strings()[cell]; selected_range(text, band, cell_bounds.origin.x, || { pen.measure(text.clone(), window) }) }) .collect(); - copy_text(&self.strings[cells.clone()], &ranges, cells.start, columns) + copy_text( + &self.strings()[cells.clone()], + &ranges, + cells.start, + columns, + ) } fn paint_background( @@ -506,7 +533,8 @@ impl DiffBody { top: Pixels, window: &mut Window, ) { - let full_width = match &self.rows { + let columns = self.rows().columns(); + let full_width = match self.rows() { Rows::Unified(rows) => Some(&rows[row]), Rows::Split(split) => match &split[row] { SplitRow::Full(full) => Some(full), @@ -525,22 +553,22 @@ impl DiffBody { } } None => { - for column in 0..self.rows.columns() { - let Some(background) = self.rows.side(row, column).and_then(|side| { + for column in 0..columns { + let Some(background) = self.rows().side(row, column).and_then(|side| { line_colors(side.origin, self.mode, &self.theme).background }) else { continue; }; let band = Bounds::new( - point(self.column_left(bounds, column), top), - size(self.column_width(bounds), px(ROW_HEIGHT)), + point(column_left(bounds, columns, column), top), + size(column_width(bounds, columns), px(ROW_HEIGHT)), ); window.paint_quad(fill(band, background)); } - for column in 1..self.rows.columns() { + for column in 1..columns { let rule = Bounds::new( - point(self.column_left(bounds, column), top), + point(column_left(bounds, columns, column), top), size(px(COLUMN_RULE_WIDTH), px(ROW_HEIGHT)), ); window.paint_quad(fill(rule, self.theme.border)); @@ -558,10 +586,10 @@ impl DiffBody { window: &mut Window, cx: &mut App, ) { - let left = cell_bounds.origin.x - px(self.rows.code_left()); + let left = cell_bounds.origin.x - px(self.rows().code_left()); let top = cell_bounds.origin.y; let muted = self.theme.muted_foreground; - let (origin, marker_left) = match &self.rows { + let (origin, marker_left) = match self.rows() { Rows::Unified(rows) => { let Row::Line { origin, @@ -593,7 +621,7 @@ impl DiffBody { (origin, left + px(2. * GUTTER_WIDTH + MARKER_PADDING)) } Rows::Split(_) => { - let Some(side) = self.rows.side(row, column) else { + let Some(side) = self.rows().side(row, column) else { return; }; paint_number( @@ -609,8 +637,8 @@ impl DiffBody { } }; - let foreground = line_colors(origin, self.mode, &self.theme).foreground; - let line = pen.shape(marker(origin).into(), foreground, window); + let marker_color = line_colors(origin, self.mode, &self.theme).foreground; + let line = pen.shape(marker(origin).into(), marker_color, window); paint_line(&line, point(marker_left, top), window, cx); } } @@ -647,11 +675,10 @@ impl Element for DiffBody { .visible_cells() .map(|cell| { styled_cell( - &self.rows, + self.rows(), cell, - self.strings[cell].clone(), + self.content.strings[cell].clone(), &self.theme, - self.mode, ) }) .collect(); @@ -666,7 +693,10 @@ impl Element for DiffBody { let style = Style { flex_direction: FlexDirection::Column, flex_shrink: 0., - size: size(width.into(), px(self.rows.len() as f32 * ROW_HEIGHT).into()), + size: size( + width.into(), + px(self.rows().len() as f32 * ROW_HEIGHT).into(), + ), min_size: size(relative(1.).into(), Length::Auto), ..Default::default() }; @@ -685,7 +715,7 @@ impl Element for DiffBody { ) -> Self::PrepaintState { self.cell_bounds = self .visible_cells() - .map(|cell| self.bounds_for_cell(bounds, cell)) + .map(|cell| self.cell_bounds_at(bounds, cell)) .collect(); for (text, cell_bounds) in self.texts.iter_mut().zip(&self.cell_bounds) { text.prepaint(None, None, *cell_bounds, &mut (), window, cx); @@ -720,7 +750,7 @@ impl Element for DiffBody { .map(|(offset, text)| { let cell = first_cell + offset; TextSelectionRun::new( - self.strings[cell].clone(), + self.content.strings[cell].clone(), text.layout().clone(), self.cell_bounds[offset], ) @@ -733,7 +763,7 @@ impl Element for DiffBody { let selected = self.copy_selection(bounds, projection.ranges(), &pen, window, cx); self.selection.set_fallback_copy_text(selected, cx); - let columns = self.rows.columns(); + let columns = self.rows().columns(); for (offset, row) in self.visible.clone().enumerate() { let top = self.cell_bounds[offset * columns].origin.y; self.paint_background(row, bounds, top, window); @@ -768,6 +798,207 @@ impl Element for DiffBody { #[cfg(test)] mod tests { use super::*; + use domain::FileStatus; + + fn file_header() -> Row { + Row::FileHeader { + path: "src/main.rs".to_string(), + status: FileStatus::Modified, + added: 3, + deleted: 1, + } + } + + fn hunk_header() -> Row { + Row::HunkHeader { + text: "@@ -1,3 +1,4 @@".to_string(), + } + } + + fn line(origin: LineOrigin, content: &str) -> Row { + Row::Line { + origin, + old_number: Some(1), + new_number: Some(1), + content: content.to_string(), + } + } + + fn side(origin: LineOrigin, content: &str) -> SideLine { + SideLine { + number: Some(1), + origin, + content: content.to_string(), + } + } + + fn split_rows() -> Rows { + Rows::Split(vec![ + SplitRow::Full(hunk_header()), + SplitRow::Sides { + left: Some(side(LineOrigin::Deletion, "gone")), + right: None, + }, + ]) + } + + #[test] + fn a_unified_view_has_one_column_and_a_split_view_two() { + let unified = Rows::Unified(vec![file_header(), line(LineOrigin::Context, "keep")]); + let split = split_rows(); + + assert_eq!(unified.columns(), 1); + assert_eq!(unified.cells(), 2); + assert_eq!(unified.code_left(), CODE_LEFT); + + assert_eq!(split.columns(), 2); + assert_eq!(split.cells(), 4); + assert_eq!(split.code_left(), SPLIT_CODE_LEFT); + } + + #[test] + fn an_empty_row_list_is_empty_in_either_view() { + assert!(Rows::Unified(Vec::new()).is_empty()); + assert!(Rows::Split(Vec::new()).is_empty()); + assert!(!Rows::Unified(vec![file_header()]).is_empty()); + } + + #[test] + fn only_a_split_row_with_two_sides_has_a_side() { + let rows = split_rows(); + + assert_eq!(rows.side(0, 0), None, "a full-width row has no side"); + assert_eq!( + rows.side(1, 0).map(|side| side.content.as_str()), + Some("gone") + ); + assert_eq!(rows.side(1, 1), None, "the padded side is absent"); + assert_eq!( + Rows::Unified(vec![file_header()]).side(0, 0), + None, + "a unified row has no sides at all" + ); + } + + #[test] + fn a_row_renders_its_own_kind_of_text() { + assert_eq!( + row_text(&file_header()), + SharedString::from("src/main.rs +3 \u{2212}1") + ); + assert_eq!( + row_text(&hunk_header()), + SharedString::from("@@ -1,3 +1,4 @@") + ); + assert_eq!( + row_text(&line(LineOrigin::Addition, "let x = 1;")), + SharedString::from("let x = 1;") + ); + assert_eq!( + row_text(&Row::Placeholder { + message: "Binary file not shown." + }), + SharedString::from("Binary file not shown.") + ); + } + + #[test] + fn a_unified_cell_is_its_row() { + let rows = Rows::Unified(vec![file_header(), line(LineOrigin::Deletion, "gone")]); + assert_eq!( + cell_text(&rows, 0), + SharedString::from("src/main.rs +3 \u{2212}1") + ); + assert_eq!(cell_text(&rows, 1), SharedString::from("gone")); + } + + #[test] + fn a_full_width_split_row_renders_in_the_first_column_and_blank_in_the_second() { + let rows = split_rows(); + assert_eq!(cell_text(&rows, 0), SharedString::from("@@ -1,3 +1,4 @@")); + assert_eq!(cell_text(&rows, 1), SharedString::default()); + } + + #[test] + fn a_split_row_puts_each_side_in_its_own_column_and_pads_the_missing_one() { + let rows = split_rows(); + assert_eq!(cell_text(&rows, 2), SharedString::from("gone")); + assert_eq!(cell_text(&rows, 3), SharedString::default()); + } + + #[test] + fn a_marker_names_the_origin_and_a_context_line_keeps_the_column_wide() { + assert_eq!(marker(LineOrigin::Addition), "+"); + assert_eq!(marker(LineOrigin::Deletion), "\u{2212}"); + assert_eq!(marker(LineOrigin::Context), " "); + } + + #[test] + fn only_a_changed_line_and_the_two_headers_are_banded() { + let theme = ThemeColor::light(); + let mode = ThemeMode::Light; + + assert_eq!( + row_background(&file_header(), &theme, mode), + Some(theme.secondary) + ); + assert_eq!( + row_background(&hunk_header(), &theme, mode), + Some(theme.muted) + ); + assert_eq!( + row_background( + &Row::Placeholder { + message: "No content changes." + }, + &theme, + mode + ), + None + ); + assert_eq!( + row_background(&line(LineOrigin::Context, "keep"), &theme, mode), + None + ); + assert_eq!( + row_background(&line(LineOrigin::Addition, "new"), &theme, mode), + line_colors(LineOrigin::Addition, mode, &theme).background + ); + } + + #[test] + fn a_column_takes_an_equal_share_of_the_width_from_left_to_right() { + let bounds = Bounds::new(point(px(10.), px(20.)), size(px(200.), px(400.))); + + assert_eq!(column_width(bounds, 1), px(200.)); + assert_eq!(column_width(bounds, 2), px(100.)); + assert_eq!(column_left(bounds, 2, 0), px(10.)); + assert_eq!(column_left(bounds, 2, 1), px(110.)); + } + + #[test] + fn a_cell_sits_at_its_column_past_the_gutters_and_at_its_row() { + let bounds = Bounds::new(point(px(10.), px(20.)), size(px(200.), px(400.))); + + assert_eq!( + bounds_for_cell(bounds, 2, px(60.), 3), + Bounds::new( + point(px(170.), px(20. + ROW_HEIGHT)), + size(px(40.), px(ROW_HEIGHT)) + ), + "cell 3 is the right column of the second row" + ); + assert_eq!( + bounds_for_cell(bounds, 1, px(104.), 0), + Bounds::new(point(px(114.), px(20.)), size(px(96.), px(ROW_HEIGHT))) + ); + } + + #[test] + fn a_column_narrower_than_its_gutters_leaves_no_width_rather_than_a_negative_one() { + let bounds = Bounds::new(point(px(0.), px(0.)), size(px(80.), px(400.))); + assert_eq!(bounds_for_cell(bounds, 2, px(60.), 0).size.width, px(0.)); + } #[test] fn copy_text_joins_the_selected_rows_with_newlines() { @@ -860,8 +1091,9 @@ mod tests { } #[test] - fn the_window_is_every_row_until_the_viewport_has_been_measured() { - assert_eq!(row_window(px(0.), px(0.), 500), 0..500); + fn the_window_is_one_screenful_until_the_viewport_has_been_measured() { + assert_eq!(row_window(px(0.), px(0.), 500), 0..UNMEASURED_ROWS); + assert_eq!(row_window(px(0.), px(0.), 12), 0..12); } #[test] diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index 92537e4..f60f7e2 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -63,9 +63,12 @@ //! cell on every layout pass, because the horizontal scroll extent has to consider rows that //! are not on screen or the scrollbar would resize as the view scrolls vertically, and //! because a cell is laid out against a column width that provably exceeds every cell's -//! natural width — which is what keeps a row one line tall and `ROW_HEIGHT` true. After the -//! first frame those are hits in gpui's line-layout cache, so the cost is a hash of each -//! cell's bytes rather than a reshape. +//! natural width — which is what keeps a row one line tall and `ROW_HEIGHT` true. That is +//! affordable only because the text it measures is not rebuilt with it: [`DiffContent`] +//! holds the rows and one [`gpui::SharedString`] per cell, derived by [`content`] when the +//! patch or the view mode changes and handed to the element behind an `Rc` thereafter. So +//! after the first frame each cell is a hit in gpui's line-layout cache and the steady-state +//! cost is a hash of bytes that are already there, with no allocation and no reshape. //! //! Copying depends on a fact `gpui-base`'s own doc comment does not state. A participant's //! runs concatenate with no separator when `update_runs` projects them @@ -95,10 +98,12 @@ mod pairing; mod palette; mod split; +use std::rc::Rc; + use domain::Patch; use gpui::{ AnyElement, App, InteractiveElement as _, IntoElement, ParentElement as _, ScrollHandle, - StatefulInteractiveElement as _, Styled as _, div, px, + SharedString, StatefulInteractiveElement as _, Styled as _, div, px, }; use gpui_base::TextSelectionHandle; use gpui_component::{ @@ -108,18 +113,37 @@ use gpui_component::{ use crate::diff_view_mode::DiffViewMode; -use body::{ROW_HEIGHT, Rows, body}; +use body::{ROW_HEIGHT, Rows, body, cell_strings}; use model::rows; use split::split_rows; +pub(super) struct DiffContent { + rows: Rows, + strings: Vec, +} + +impl DiffContent { + pub(super) fn is_empty(&self) -> bool { + self.rows.is_empty() + } +} + +pub(super) fn content(patch: &Patch, mode: DiffViewMode) -> DiffContent { + let rows = match mode { + DiffViewMode::Unified => Rows::Unified(rows(patch)), + DiffViewMode::Split => Rows::Split(split_rows(patch)), + }; + let strings = cell_strings(&rows); + DiffContent { rows, strings } +} + pub(super) fn render( - patch: &Patch, - mode: DiffViewMode, + content: Option<&Rc>, selection: &TextSelectionHandle, scroll: &ScrollHandle, cx: &App, ) -> AnyElement { - if patch.files.is_empty() { + let Some(content) = content.filter(|content| !content.is_empty()) else { return div() .size_full() .flex() @@ -128,11 +152,6 @@ pub(super) fn render( .text_color(cx.theme().muted_foreground) .child("This commit changes nothing.") .into_any_element(); - } - - let content = match mode { - DiffViewMode::Unified => Rows::Unified(rows(patch)), - DiffViewMode::Split => Rows::Split(split_rows(patch)), }; let theme = cx.theme(); @@ -150,7 +169,7 @@ pub(super) fn render( .text_size(theme.mono_font_size) .line_height(px(ROW_HEIGHT)) .child(body( - content, + Rc::clone(content), selection.clone(), scroll.clone(), theme.colors, diff --git a/crates/ui/src/detail/diff/model.rs b/crates/ui/src/detail/diff/model.rs index ce74fcd..f691e5b 100644 --- a/crates/ui/src/detail/diff/model.rs +++ b/crates/ui/src/detail/diff/model.rs @@ -1,4 +1,6 @@ -use domain::{DiffLine, FilePatch, Hunk, LineOrigin, Patch}; +use std::fmt::Write as _; + +use domain::{DiffLine, FilePatch, FileStatus, Hunk, LineOrigin, Patch}; use crate::detail::format; @@ -6,7 +8,9 @@ use crate::detail::format; pub(super) enum Row { FileHeader { path: String, - stat: String, + status: FileStatus, + added: usize, + deleted: usize, }, HunkHeader { text: String, @@ -33,16 +37,47 @@ pub(super) fn rows(patch: &Patch) -> Vec { pub(super) fn file_header(file: &FilePatch) -> Row { Row::FileHeader { - path: file + path: header_path(file), + status: file.status.clone(), + added: file.added_lines(), + deleted: file.deleted_lines(), + } +} + +pub(super) fn header_line(path: &str, status: &FileStatus, added: usize, deleted: usize) -> String { + let mut text = path.to_string(); + if let Some(label) = status_label(status) { + let _ = write!(text, " {label}"); + } + let _ = write!(text, " +{added} \u{2212}{deleted}"); + text +} + +fn header_path(file: &FilePatch) -> String { + let moved = matches!( + file.status, + FileStatus::Renamed { .. } | FileStatus::Copied { .. } + ); + match (&file.old_path, &file.new_path) { + (Some(old), Some(new)) if moved && old != new => { + format!("{} \u{2192} {}", old.display(), new.display()) + } + _ => file .display_path() - .map(|p| p.display().to_string()) + .map(|path| path.display().to_string()) .unwrap_or_default(), - stat: file_stat(file), } } -pub(super) fn file_stat(file: &FilePatch) -> String { - format!("+{} \u{2212}{}", file.added_lines(), file.deleted_lines()) +fn status_label(status: &FileStatus) -> Option { + match status { + FileStatus::Modified => None, + FileStatus::Added => Some("added".to_string()), + FileStatus::Deleted => Some("deleted".to_string()), + FileStatus::Renamed { similarity } => Some(format!("renamed {similarity}%")), + FileStatus::Copied { similarity } => Some(format!("copied {similarity}%")), + FileStatus::TypeChanged => Some("type changed".to_string()), + } } pub(super) fn placeholder(file: &FilePatch) -> Option { @@ -110,6 +145,16 @@ mod tests { } } + fn moved(old: &str, new: &str, status: FileStatus) -> FilePatch { + FilePatch { + old_path: Some(PathBuf::from(old)), + new_path: Some(PathBuf::from(new)), + status, + is_binary: false, + hunks: Vec::new(), + } + } + fn hunk(lines: Vec) -> Hunk { Hunk { old_start: 1, @@ -197,6 +242,92 @@ mod tests { ); } + #[test] + fn a_rename_keeps_both_paths_and_its_similarity() { + let file = moved( + "src/old.rs", + "src/new.rs", + FileStatus::Renamed { similarity: 87 }, + ); + let Row::FileHeader { + path, + status, + added, + deleted, + } = file_header(&file) + else { + panic!("a file yields a header row"); + }; + + assert_eq!(path, "src/old.rs \u{2192} src/new.rs"); + assert_eq!(status, FileStatus::Renamed { similarity: 87 }); + assert_eq!( + header_line(&path, &status, added, deleted), + "src/old.rs \u{2192} src/new.rs renamed 87% +0 \u{2212}0" + ); + } + + #[test] + fn a_copy_reads_as_a_copy_rather_than_as_a_rename() { + let file = moved( + "src/old.rs", + "src/copy.rs", + FileStatus::Copied { similarity: 100 }, + ); + let Row::FileHeader { + path, + status, + added, + deleted, + } = file_header(&file) + else { + panic!("a file yields a header row"); + }; + + assert_eq!(path, "src/old.rs \u{2192} src/copy.rs"); + assert_eq!( + header_line(&path, &status, added, deleted), + "src/old.rs \u{2192} src/copy.rs copied 100% +0 \u{2212}0" + ); + } + + #[test] + fn a_rename_that_only_changed_the_content_shows_one_path() { + let file = moved( + "src/a.rs", + "src/a.rs", + FileStatus::Renamed { similarity: 100 }, + ); + let Row::FileHeader { path, .. } = file_header(&file) else { + panic!("a file yields a header row"); + }; + assert_eq!(path, "src/a.rs"); + } + + #[test] + fn a_modified_file_carries_no_status_label() { + let patch = Patch { + files: vec![file( + vec![hunk(vec![line(LineOrigin::Addition, None, Some(1), "new")])], + false, + )], + }; + let Row::FileHeader { + path, + status, + added, + deleted, + } = rows(&patch).remove(0) + else { + panic!("the first row is the file header"); + }; + + assert_eq!( + header_line(&path, &status, added, deleted), + "src/main.rs +1 \u{2212}0" + ); + } + #[test] fn every_file_contributes_its_own_header() { let patch = Patch { diff --git a/crates/ui/src/detail/diff/split.rs b/crates/ui/src/detail/diff/split.rs index 33eadb6..f711c83 100644 --- a/crates/ui/src/detail/diff/split.rs +++ b/crates/ui/src/detail/diff/split.rs @@ -74,6 +74,16 @@ mod tests { ]) } + fn moved(old: &str, new: &str, status: FileStatus) -> FilePatch { + FilePatch { + old_path: Some(PathBuf::from(old)), + new_path: Some(PathBuf::from(new)), + status, + is_binary: false, + hunks: Vec::new(), + } + } + fn paths(rows: &[SplitRow]) -> Vec { rows.iter() .filter_map(|row| match row { @@ -197,6 +207,59 @@ mod tests { ); } + #[test] + fn a_rename_with_no_content_change_still_names_both_paths() { + let patch = Patch { + files: vec![moved( + "src/old.rs", + "src/new.rs", + FileStatus::Renamed { similarity: 87 }, + )], + }; + + let rows = split_rows(&patch); + + assert_eq!(paths(&rows), vec!["src/old.rs \u{2192} src/new.rs"]); + assert_eq!( + rows[0], + SplitRow::Full(Row::FileHeader { + path: "src/old.rs \u{2192} src/new.rs".to_string(), + status: FileStatus::Renamed { similarity: 87 }, + added: 0, + deleted: 0, + }) + ); + assert_eq!( + rows[1], + SplitRow::Full(Row::Placeholder { + message: "No content changes." + }) + ); + } + + #[test] + fn a_copy_carries_its_own_status_into_the_header_row() { + let patch = Patch { + files: vec![moved( + "src/old.rs", + "src/copy.rs", + FileStatus::Copied { similarity: 100 }, + )], + }; + + let rows = split_rows(&patch); + + assert_eq!( + rows[0], + SplitRow::Full(Row::FileHeader { + path: "src/old.rs \u{2192} src/copy.rs".to_string(), + status: FileStatus::Copied { similarity: 100 }, + added: 0, + deleted: 0, + }) + ); + } + #[test] fn every_hunk_of_a_file_keeps_its_own_header() { let patch = Patch { diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index 32e9fa8..9dd82eb 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -14,15 +14,17 @@ //! which rows to lay out and paint — it is the only input it has to that decision before //! the frame gives it any bounds. //! -//! The one piece of view state that does need to outlive a frame is the diff's selection -//! participant: `gpui-base` keys a window-level selection off a +//! Two pieces of view state have to outlive a frame, and both for the same reason: a +//! render must not build them. `gpui-base` keys a window-level selection off a //! [`TextSelectionHandle`], and a handle rebuilt per frame would drop the selection on -//! every repaint. It is built once in [`DetailPanel::new`], which is also the only place -//! with the `&Window` its refresh subscription needs. Nothing else here is staged: -//! [`DetailPanel::set_detail`] stores the new [`LoadState`] and notifies, and the rows are -//! derived from the patch during the render that follows. -//! [`DetailPanel::selected_tab`] is never touched by `set_detail`, which is what lets -//! picking a different commit leave the open tab alone. +//! every repaint, so it is built once in [`DetailPanel::new`]. The diff's rows and the +//! string of every cell in them are derived from the patch, and deriving them per frame +//! would copy every line of the patch two or three times over on every repaint — +//! `refresh_window_on_change` repaints on each mouse move of a selection drag, so that is +//! not a rare frame. They are rebuilt by [`DetailPanel::set_detail`] and +//! [`DetailPanel::set_diff_view_mode`], which are the only two places the patch or the +//! view mode can change. [`DetailPanel::selected_tab`] is never touched by `set_detail`, +//! which is what lets picking a different commit leave the open tab alone. //! //! The diff view mode is read once from disk in [`DetailPanel::new`], before the first //! frame, and written back through `cx.background_executor()`: @@ -37,13 +39,17 @@ //! endpoint relative to `bounds.origin` (`text_selection.rs:1336`), so it survives the //! content underneath it changing, and a stored `y` then resolves onto whatever row now sits //! at that offset — a highlight over lines the user never dragged across, which Cmd-C would -//! copy. `TextSelection::clear` is window-wide rather than per-participant, which costs -//! nothing here because the diff body is the only participant this crate registers. +//! copy. `TextSelection::clear` is window-wide rather than per-participant, and this panel +//! has participants beyond the diff body: [`metadata`] renders every value through +//! [`gpui_component::text::markdown`], and each `TextView` registers one of its own. They +//! are cleared too, which is the right outcome — they are selections over the commit that +//! is being replaced. mod diff; mod format; mod metadata; +use std::rc::Rc; use std::sync::Arc; use gpui::{ @@ -65,6 +71,8 @@ use crate::diff_view_mode::DiffViewMode; use crate::persistence; use crate::repository::{CommitDetail, LoadState}; +use diff::DiffContent; + #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] enum DetailTab { #[default] @@ -93,6 +101,7 @@ impl DetailTab { pub struct DetailPanel { detail: LoadState>, + diff_content: Option>, diff_selection: TextSelectionHandle, diff_view_mode: DiffViewMode, selected_tab: DetailTab, @@ -107,6 +116,7 @@ impl DetailPanel { diff_selection.refresh_window_on_change(window, cx).detach(); Self { detail: LoadState::Idle, + diff_content: None, diff_selection, diff_view_mode: persistence::load_diff_view_mode().unwrap_or_default(), selected_tab: DetailTab::default(), @@ -123,6 +133,7 @@ impl DetailPanel { cx: &mut Context, ) { self.detail = detail; + self.rebuild_diff_content(); self.reset_diff_view(window, cx); cx.notify(); } @@ -137,6 +148,7 @@ impl DetailPanel { return; } self.diff_view_mode = mode; + self.rebuild_diff_content(); self.reset_diff_view(window, cx); cx.background_executor() @@ -150,6 +162,15 @@ impl DetailPanel { cx.notify(); } + fn rebuild_diff_content(&mut self) { + self.diff_content = match &self.detail { + LoadState::Ready(detail) => { + Some(Rc::new(diff::content(&detail.patch, self.diff_view_mode))) + } + _ => None, + }; + } + fn reset_diff_view(&mut self, window: &mut Window, cx: &mut Context) { self.diff_scroll_handle.set_offset(Point::default()); TextSelection::clear(window, cx); @@ -194,7 +215,7 @@ impl Render for DetailPanel { LoadState::Ready(detail) => ready_state( detail, selected_tab, - diff_view_mode, + self.diff_content.as_ref(), &self.diff_selection, &self.general_scroll_handle, &self.diff_scroll_handle, @@ -292,7 +313,7 @@ fn failed_state(message: &str) -> AnyElement { fn ready_state( detail: &CommitDetail, selected_tab: DetailTab, - diff_view_mode: DiffViewMode, + diff_content: Option<&Rc>, diff_selection: &TextSelectionHandle, general_scroll_handle: &ScrollHandle, diff_scroll_handle: &ScrollHandle, @@ -300,13 +321,7 @@ fn ready_state( ) -> AnyElement { match selected_tab { DetailTab::General => general_tab(detail, general_scroll_handle, cx), - DetailTab::Diff => diff_tab( - detail, - diff_view_mode, - diff_selection, - diff_scroll_handle, - cx, - ), + DetailTab::Diff => diff_tab(diff_content, diff_selection, diff_scroll_handle, cx), } } @@ -336,8 +351,7 @@ fn general_tab(detail: &CommitDetail, scroll_handle: &ScrollHandle, cx: &App) -> } fn diff_tab( - detail: &CommitDetail, - mode: DiffViewMode, + content: Option<&Rc>, selection: &TextSelectionHandle, scroll_handle: &ScrollHandle, cx: &App, @@ -346,12 +360,6 @@ fn diff_tab( .flex_1() .min_h_0() .min_w_0() - .child(diff::render( - &detail.patch, - mode, - selection, - scroll_handle, - cx, - )) + .child(diff::render(content, selection, scroll_handle, cx)) .into_any_element() } From 1e71f419c5abcaadabe57cf71798d50cd78d75d2 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 17:15:13 +0200 Subject: [PATCH 16/19] fix(diff): restore copying and selecting a whole diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things were lost with the editor and none of them failed loudly. Nothing handled `SelectAll` any more, so the menu item was inert over the diff. `DetailPanel` registers one, and marks the participant as locally selected (`set_local_selection`) rather than faking a geometric selection: there are no window points behind Cmd-A, so `copy_selection` answers the whole document directly and every visible cell is highlighted whole. A drag clears it through `TextSelectionEvent::Cleared`, which `begin_impl` raises before it takes a new anchor, and so does the frame sweep when the Diff tab goes away. The event rather than `clear_with`, because `clear_with` fires synchronously from inside `TextSelection::clear` — which `reset_diff_view` calls while the panel is already leased, so updating the panel from there would panic. `Copy` is registered here too, and has to be: `gpui_component::Root`'s handler trims the string it copies (`root.rs:552-555`), which silently eats the indentation of the first selected line. A descendant of `Root` in the dispatch tree wins over it, which is what `track_focus` on the panel's own div buys — `TabPanel` tracks this panel's focus handle on a node above it, so without one of our own the panel is never on the focus path. Nothing drove `TextSelectionEvent::AutoScroll`, so a drag past the viewport edge stopped instead of scrolling. Subscribing is half the fix: the registered rectangle is what `AutoScroll::compute_delta` measures the pointer against, and this element registered its own bounds, which are the whole diff rather than the part of it on screen, so the trigger zone sat off-screen and no delta was ever produced. It now registers the viewport and reports `bounds.origin - viewport.origin` as the scroll offset. `gpui-base` stores an endpoint as `position - bounds.origin - scroll_offset` and resolves it by adding both back, so the sum is what matters and this sum is the element's own origin by construction — every stored endpoint is bit-identical to before. --- crates/ui/src/detail/diff/body.rs | 26 ++++++- crates/ui/src/detail/diff/mod.rs | 14 ++-- crates/ui/src/detail/mod.rs | 112 +++++++++++++++++++++++++++--- crates/ui/src/workspace.rs | 10 +-- 4 files changed, 143 insertions(+), 19 deletions(-) diff --git a/crates/ui/src/detail/diff/body.rs b/crates/ui/src/detail/diff/body.rs index 3f9ad5f..fd44881 100644 --- a/crates/ui/src/detail/diff/body.rs +++ b/crates/ui/src/detail/diff/body.rs @@ -87,6 +87,7 @@ pub(super) fn cell_strings(rows: &Rows) -> Vec { pub(super) struct DiffBody { content: Rc, + select_all: bool, selection: TextSelectionHandle, scroll: ScrollHandle, theme: ThemeColor, @@ -98,6 +99,7 @@ pub(super) struct DiffBody { pub(super) fn body( content: Rc, + select_all: bool, selection: TextSelectionHandle, scroll: ScrollHandle, theme: ThemeColor, @@ -105,6 +107,7 @@ pub(super) fn body( ) -> DiffBody { DiffBody { content, + select_all, selection, scroll, theme, @@ -474,6 +477,15 @@ impl DiffBody { self.visible.start * columns..self.visible.end * columns } + fn whole_text(&self) -> String { + let ranges: Vec>> = self + .strings() + .iter() + .map(|text| Some(0..text.len())) + .collect(); + copy_text(self.strings(), &ranges, 0, self.rows().columns()) + } + fn copy_selection( &self, bounds: Bounds, @@ -482,6 +494,9 @@ impl DiffBody { window: &Window, cx: &App, ) -> String { + if self.select_all { + return self.whole_text(); + } let Some(points) = self .selection .snapshot(cx) @@ -721,9 +736,11 @@ impl Element for DiffBody { text.prepaint(None, None, *cell_bounds, &mut (), window, cx); } + let viewport = self.scroll.bounds(); let hitbox = window.insert_hitbox(bounds, HitboxBehavior::Normal); self.selection.register( - TextSelectionRegistration::new(hitbox.clone(), bounds) + TextSelectionRegistration::new(hitbox.clone(), viewport) + .with_scroll_offset(bounds.origin - viewport.origin) .with_document_order(0) .with_text_bounds(self.cell_bounds.clone()), window, @@ -771,7 +788,12 @@ impl Element for DiffBody { for column in 0..columns { let cell_offset = offset * columns + column; let cell_bounds = self.cell_bounds[cell_offset]; - if let Some(range) = projection.ranges().get(cell_offset).and_then(Clone::clone) { + let range = if self.select_all { + Some(0..self.content.strings[first_cell + cell_offset].len()) + } else { + projection.ranges().get(cell_offset).and_then(Clone::clone) + }; + if let Some(range) = range { paint_selection( self.texts[cell_offset].layout(), range, diff --git a/crates/ui/src/detail/diff/mod.rs b/crates/ui/src/detail/diff/mod.rs index f60f7e2..a870240 100644 --- a/crates/ui/src/detail/diff/mod.rs +++ b/crates/ui/src/detail/diff/mod.rs @@ -54,10 +54,14 @@ //! and a cell's own column offset is what turns it into a range — which is exactly what //! `point_in_selection_band` does to two runs that share a `y`, so the arithmetic and the //! projection still agree cell for cell. Endpoints survive scrolling because `gpui-base` -//! stores them relative to `bounds.origin`, which already carries the scroll, so a point off -//! the top of the viewport -//! is a negative `y` rather than a lost one. Rows that *are* on screen keep the projection's -//! own range, so what is highlighted and what is copied cannot drift apart. +//! stores them relative to `bounds.origin + scroll_offset`, and the participant reports the +//! two so that their sum is this element's own origin, which already carries the scroll: a +//! point off the top of the viewport is then a negative `y` rather than a lost one. Rows +//! that *are* on screen keep the projection's +//! own range, so what is highlighted and what is copied cannot drift apart. Select All is +//! the one selection with no window points to derive anything from — it is participant-local +//! (`set_local_selection`), so `copy_selection` answers the whole document for it directly +//! and every visible cell is highlighted whole. //! //! What stays unwindowed is the content width: `body::DiffBody::content_width` shapes every //! cell on every layout pass, because the horizontal scroll extent has to consider rows that @@ -139,6 +143,7 @@ pub(super) fn content(patch: &Patch, mode: DiffViewMode) -> DiffContent { pub(super) fn render( content: Option<&Rc>, + select_all: bool, selection: &TextSelectionHandle, scroll: &ScrollHandle, cx: &App, @@ -170,6 +175,7 @@ pub(super) fn render( .line_height(px(ROW_HEIGHT)) .child(body( Rc::clone(content), + select_all, selection.clone(), scroll.clone(), theme.colors, diff --git a/crates/ui/src/detail/mod.rs b/crates/ui/src/detail/mod.rs index 9dd82eb..067adce 100644 --- a/crates/ui/src/detail/mod.rs +++ b/crates/ui/src/detail/mod.rs @@ -36,8 +36,8 @@ //! zeroes the diff's scroll offset and clears the window selection. Clearing is the reason //! both take a `&mut Window`, which is why [`crate::workspace::Workspace`] threads one into //! `sync_panels_from_repository`. It is not optional: `gpui-base` stores a selection -//! endpoint relative to `bounds.origin` (`text_selection.rs:1336`), so it survives the -//! content underneath it changing, and a stored `y` then resolves onto whatever row now sits +//! endpoint relative to the participant's registered origin (`text_selection.rs:1336`), so it +//! survives the content underneath it changing, and a stored `y` then resolves onto whatever row now sits //! at that offset — a highlight over lines the user never dragged across, which Cmd-C would //! copy. `TextSelection::clear` is window-wide rather than per-participant, and this panel //! has participants beyond the diff body: [`metadata`] renders every value through @@ -53,15 +53,17 @@ use std::rc::Rc; use std::sync::Arc; use gpui::{ - AnyElement, App, Context, EventEmitter, FocusHandle, Focusable, InteractiveElement as _, - IntoElement, ParentElement as _, Point, Render, ScrollHandle, StatefulInteractiveElement as _, - Styled as _, Window, div, prelude::FluentBuilder as _, + AnyElement, App, ClipboardItem, Context, EventEmitter, FocusHandle, Focusable, + InteractiveElement as _, IntoElement, ParentElement as _, Pixels, Point, Render, ScrollHandle, + StatefulInteractiveElement as _, Styled as _, Window, div, point, prelude::FluentBuilder as _, + px, }; -use gpui_base::{TextSelection, TextSelectionHandle}; +use gpui_base::{AutoScroll, TextSelection, TextSelectionEvent, TextSelectionHandle}; use gpui_component::{ ActiveTheme as _, Sizable as _, alert::Alert, dock::{Panel, PanelEvent}, + input::{Copy, SelectAll}, scroll::ScrollableElement as _, spinner::Spinner, tab::{Tab, TabBar}, @@ -103,6 +105,8 @@ pub struct DetailPanel { detail: LoadState>, diff_content: Option>, diff_selection: TextSelectionHandle, + diff_select_all: bool, + diff_auto_scroll: AutoScroll, diff_view_mode: DiffViewMode, selected_tab: DetailTab, general_scroll_handle: ScrollHandle, @@ -114,15 +118,41 @@ impl DetailPanel { pub fn new(window: &mut Window, cx: &mut Context) -> Self { let diff_selection = TextSelectionHandle::new("", cx); diff_selection.refresh_window_on_change(window, cx).detach(); + + let focus_handle = cx.focus_handle(); + let focus = focus_handle.clone(); + diff_selection.focus_with(move |window, cx| focus.focus(window, cx), cx); + + let panel = cx.weak_entity(); + diff_selection + .subscribe( + move |event, cx| { + let _ = match event { + TextSelectionEvent::AutoScroll(delta) => { + let delta = *delta; + panel.update(cx, |panel, cx| panel.auto_scroll_diff(delta, cx)) + } + TextSelectionEvent::Cleared => { + panel.update(cx, |panel, cx| panel.forget_select_all(cx)) + } + TextSelectionEvent::SelectionChanged(_) => Ok(()), + }; + }, + cx, + ) + .detach(); + Self { detail: LoadState::Idle, diff_content: None, diff_selection, + diff_select_all: false, + diff_auto_scroll: AutoScroll::default(), diff_view_mode: persistence::load_diff_view_mode().unwrap_or_default(), selected_tab: DetailTab::default(), general_scroll_handle: ScrollHandle::new(), diff_scroll_handle: ScrollHandle::new(), - focus_handle: cx.focus_handle(), + focus_handle, } } @@ -172,9 +202,54 @@ impl DetailPanel { } fn reset_diff_view(&mut self, window: &mut Window, cx: &mut Context) { + self.diff_auto_scroll.stop(); + self.diff_select_all = false; self.diff_scroll_handle.set_offset(Point::default()); TextSelection::clear(window, cx); } + + fn forget_select_all(&mut self, cx: &mut Context) { + if !self.diff_select_all { + return; + } + self.diff_select_all = false; + cx.notify(); + } + + fn auto_scroll_diff(&mut self, delta: Option, cx: &mut Context) { + self.diff_auto_scroll.set(delta, cx, |delta, panel, cx| { + let offset = panel.diff_scroll_handle.offset(); + panel + .diff_scroll_handle + .set_offset(offset - point(px(0.), delta)); + cx.notify(); + }); + } + + fn on_copy(&mut self, _: &Copy, window: &mut Window, cx: &mut Context) { + let text = TextSelection::selected_text(window, cx); + if text.is_empty() { + cx.propagate(); + return; + } + cx.write_to_clipboard(ClipboardItem::new_string(text)); + } + + fn on_select_all(&mut self, _: &SelectAll, _: &mut Window, cx: &mut Context) { + let selectable = self.selected_tab == DetailTab::Diff + && self + .diff_content + .as_ref() + .is_some_and(|content| !content.is_empty()); + if !selectable { + cx.propagate(); + return; + } + + self.diff_select_all = true; + self.diff_selection.set_local_selection(true, cx); + cx.notify(); + } } impl Panel for DetailPanel { @@ -204,6 +279,9 @@ impl Render for DetailPanel { let selected_tab = self.selected_tab; let diff_view_mode = self.diff_view_mode; div() + .track_focus(&self.focus_handle) + .on_action(cx.listener(Self::on_copy)) + .on_action(cx.listener(Self::on_select_all)) .size_full() .flex() .flex_col() @@ -216,6 +294,7 @@ impl Render for DetailPanel { detail, selected_tab, self.diff_content.as_ref(), + self.diff_select_all, &self.diff_selection, &self.general_scroll_handle, &self.diff_scroll_handle, @@ -310,10 +389,12 @@ fn failed_state(message: &str) -> AnyElement { .into_any_element() } +#[allow(clippy::too_many_arguments)] fn ready_state( detail: &CommitDetail, selected_tab: DetailTab, diff_content: Option<&Rc>, + diff_select_all: bool, diff_selection: &TextSelectionHandle, general_scroll_handle: &ScrollHandle, diff_scroll_handle: &ScrollHandle, @@ -321,7 +402,13 @@ fn ready_state( ) -> AnyElement { match selected_tab { DetailTab::General => general_tab(detail, general_scroll_handle, cx), - DetailTab::Diff => diff_tab(diff_content, diff_selection, diff_scroll_handle, cx), + DetailTab::Diff => diff_tab( + diff_content, + diff_select_all, + diff_selection, + diff_scroll_handle, + cx, + ), } } @@ -352,6 +439,7 @@ fn general_tab(detail: &CommitDetail, scroll_handle: &ScrollHandle, cx: &App) -> fn diff_tab( content: Option<&Rc>, + select_all: bool, selection: &TextSelectionHandle, scroll_handle: &ScrollHandle, cx: &App, @@ -360,6 +448,12 @@ fn diff_tab( .flex_1() .min_h_0() .min_w_0() - .child(diff::render(content, selection, scroll_handle, cx)) + .child(diff::render( + content, + select_all, + selection, + scroll_handle, + cx, + )) .into_any_element() } diff --git a/crates/ui/src/workspace.rs b/crates/ui/src/workspace.rs index e586594..685a736 100644 --- a/crates/ui/src/workspace.rs +++ b/crates/ui/src/workspace.rs @@ -1613,10 +1613,12 @@ fn theme_preference_menu_item( /// The whole native macOS menu bar, rebuilt from scratch on every call — see /// [`Workspace::refresh_application_menus`] for when. `Cut`, `Copy`, `Paste` and /// `Select All` carry `gpui_component::input`'s own actions and matching [`OsAction`], -/// not an action this crate defines: the project search box, the "add from URL" field -/// and the readonly diff editor each register a handler for those every time they -/// paint, so the menu item reaches whichever one currently has focus exactly as the -/// keyboard shortcut already does. +/// not an action this crate defines: the project search box and the "add from URL" field +/// register a handler for those every time they paint, and `DetailPanel` registers its +/// own `Copy` and `Select All` for the diff, so the menu item reaches whichever one +/// currently has focus exactly as the keyboard shortcut already does. The diff needs its +/// own `Copy` rather than `gpui_component::Root`'s: that one trims the copied string +/// (`root.rs:552-555`), which eats the indentation of the first selected line. fn application_menus(theme_preference: ThemePreference) -> Vec { vec![ Menu { From 2cf4eeb8106cecdd288e0552aa258aa0979621c7 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 17:15:54 +0200 Subject: [PATCH 17/19] fix(diff): tint the markers by origin and measure the plates against the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `LineColors::foreground` was `theme.foreground` for every origin, so the field and the `line_colors(origin, ..)` signature promised something that did not exist and the `+` and `-` markers were painted in the code's own colour. They now carry `theme.green` and `theme.red`, the tokens the reference badges use, with `theme.muted_foreground` for context; the code stays `theme.foreground` whatever its origin, which is where the marker colour lives in a diff that no longer highlights by role. That also fixes what the two dark constants were justified against. The note argued their legibility against Catppuccin Frappé's addition and deletion syntax colours, and no diff line has been painted in those since the editor went. Measured against `#c6d0f5`, the foreground the code is actually drawn in, the plates give 5.11:1 and 6.20:1 — both better than the numbers the note claimed, and both true. The visibility half of the argument, 1.58:1 and 1.30:1 against Frappé's `#303446`, never depended on the text and stands. `light_mode_uses_the_exact_given_hex_values` comes back with them. It pinned GitHub's two values and was dropped along with the vacuous test beside it when `decorations.rs` was replaced. --- crates/ui/src/detail/diff/palette.rs | 60 ++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/crates/ui/src/detail/diff/palette.rs b/crates/ui/src/detail/diff/palette.rs index 2135aa2..74015ff 100644 --- a/crates/ui/src/detail/diff/palette.rs +++ b/crates/ui/src/detail/diff/palette.rs @@ -7,21 +7,25 @@ const LIGHT_DELETION_BACKGROUND: u32 = 0xffebe9; /// A tint has to clear two bars, and only one of them is legibility. /// -/// The light pair is unreadable under Catppuccin Frappé — its addition syntax colour -/// `#a6d189` sits at 1.56:1 against [`LIGHT_ADDITION_BACKGROUND`], pale on pale. But an -/// earlier dark green picked purely for legibility against that text landed at 1.00:1 -/// against Frappé's own `#303446` background: identical luminance, so the band was -/// invisible and the tint may as well not have been drawn. This value clears both — 1.58:1 -/// against the background so the band reads, 4.50:1 under the text so the code stays -/// legible on it. +/// The light pair is unusable under Catppuccin Frappé: it is pale, and the code above it +/// is painted in `theme.foreground`, `#c6d0f5` — pale on pale. But an earlier dark green +/// picked purely for legibility against that text landed at 1.00:1 against Frappé's own +/// `#303446` background: identical luminance, so the band was invisible and the tint may +/// as well not have been drawn. This value clears both — 1.58:1 against the background so +/// the band reads, 5.11:1 under `#c6d0f5` so the code stays legible on it. const DARK_ADDITION_BACKGROUND: u32 = 0x355a40; -/// Mirrors [`DARK_ADDITION_BACKGROUND`]'s two-bar reasoning for Frappé's deletion syntax -/// colour `#e78284`: 1.30:1 against the background, 3.57:1 under the text. Red text is -/// lighter than green here, so the two bars pull harder against each other and this sits -/// where they meet. +/// Mirrors [`DARK_ADDITION_BACKGROUND`]'s two-bar reasoning: 1.30:1 against Frappé's +/// background, 6.20:1 under the same `#c6d0f5`. A deletion can afford a darker plate than +/// an addition because nothing has to read *as* red on it — only the marker is tinted, and +/// it is drawn in `theme.red` rather than in the plate's own hue. const DARK_DELETION_BACKGROUND: u32 = 0x5f3c45; +/// The plate a line of a given origin sits on, and the colour of its `+`/`−` marker. +/// +/// `foreground` is the marker's colour alone. The code itself is painted in +/// `theme.foreground` whatever its origin — which is the measurement the two dark +/// constants above are chosen against. pub(super) struct LineColors { pub background: Option, pub foreground: Hsla, @@ -35,9 +39,14 @@ pub(super) fn line_colors(origin: LineOrigin, mode: ThemeMode, theme: &ThemeColo (LineOrigin::Deletion, false) => Some(rgb(LIGHT_DELETION_BACKGROUND).into()), (LineOrigin::Deletion, true) => Some(rgb(DARK_DELETION_BACKGROUND).into()), }; + let foreground = match origin { + LineOrigin::Context => theme.muted_foreground, + LineOrigin::Addition => theme.green, + LineOrigin::Deletion => theme.red, + }; LineColors { background, - foreground: theme.foreground, + foreground, } } @@ -45,6 +54,33 @@ pub(super) fn line_colors(origin: LineOrigin, mode: ThemeMode, theme: &ThemeColo mod tests { use super::*; + #[test] + fn light_mode_uses_the_exact_given_hex_values() { + let theme = ThemeColor::light(); + assert_eq!( + line_colors(LineOrigin::Addition, ThemeMode::Light, &theme).background, + Some(rgb(0xdafbe1).into()) + ); + assert_eq!( + line_colors(LineOrigin::Deletion, ThemeMode::Light, &theme).background, + Some(rgb(0xffebe9).into()) + ); + } + + #[test] + fn a_marker_is_tinted_by_its_origin_rather_than_by_the_code_colour() { + let theme = ThemeColor::dark(); + let added = line_colors(LineOrigin::Addition, ThemeMode::Dark, &theme).foreground; + let deleted = line_colors(LineOrigin::Deletion, ThemeMode::Dark, &theme).foreground; + let context = line_colors(LineOrigin::Context, ThemeMode::Dark, &theme).foreground; + + assert_eq!(added, theme.green); + assert_eq!(deleted, theme.red); + assert_eq!(context, theme.muted_foreground); + assert_ne!(added, theme.foreground); + assert_ne!(deleted, theme.foreground); + } + #[test] fn a_context_line_has_no_background() { let theme = ThemeColor::light(); From e29830300739e857f997c8a6165463be8e959d78 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 29 Aug 2026 17:16:15 +0200 Subject: [PATCH 18/19] docs(detail): drop the last references to the diff editor `metadata.rs` still explained its header/body split by an editor that had to fit beneath the body, and still called the commit body's code rendering the editor's own. Neither exists: the two tabs are separate, both halves scroll together in the General tab's one scroll region, and the split survives only because `render_description` answers `None` for a commit with nothing beyond its subject line. --- crates/ui/src/detail/metadata.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/crates/ui/src/detail/metadata.rs b/crates/ui/src/detail/metadata.rs index 06e1e76..65f911b 100644 --- a/crates/ui/src/detail/metadata.rs +++ b/crates/ui/src/detail/metadata.rs @@ -1,9 +1,8 @@ //! Renders the commit metadata header (subject, identifier, parents and author) and, -//! separately, the commit message body — split because the header renders first -//! above the detail panel's scroll region while [`render_description`] scrolls -//! together with the header inside the tab's own scroll region, so an unusually -//! long body cannot squeeze the diff editor beneath it out of the panel. See -//! `detail::ready_state` for where the two are recombined. +//! separately, the commit message body — split because [`render_description`] answers +//! `None` for a commit that has nothing beyond its subject line, which renders no row +//! rather than an empty one. Both scroll together inside the General tab's single scroll +//! region; see `detail::general_tab` for where the two are recombined. //! //! Every value here goes through [`gpui_component::text::markdown`] rather than a plain //! `div`, which is what makes the commit's identifier — the thing most worth copying in @@ -82,7 +81,7 @@ pub(super) fn render_description(commit: &Commit, cx: &App) -> Option Date: Sat, 29 Aug 2026 17:16:42 +0200 Subject: [PATCH 19/19] docs(diff): bring the design note in line with what was built MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The status still read "approved, not yet implemented". The module table still listed `diff/unified.rs`, which was never created, and called the element `row.rs` where the rest of the document calls it `body.rs` — with six files in the directory against seven in the table. And the selection section still argued that `with_scroll_offset` must not be called, which stopped being true when the participant started registering the viewport so that auto-scroll has a trigger zone the pointer can reach. --- ...026-08-29-github-style-diff-view-design.md | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md index e90044b..4565ba9 100644 --- a/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md +++ b/docs/superpowers/specs/2026-08-29-github-style-diff-view-design.md @@ -1,7 +1,7 @@ # GitHub-style diff view Date: 2026-08-29 -Status: approved, not yet implemented +Status: implemented ## The problem @@ -94,14 +94,16 @@ warns about does not apply here. | `diff/mod.rs` | Entry point: picks the layout for the current `DiffViewMode`. | | `diff/model.rs` | `Row`: `FileHeader \| HunkHeader \| Line \| Placeholder`. Pure. | | `diff/pairing.rs` | Left/right pairing for the split view. Pure. | -| `diff/body.rs` | The custom `Element`: visible-range windowing, hitbox, one selection participant, N runs, row painting. | -| `diff/unified.rs` | Unified row geometry over `Vec`. | +| `diff/body.rs` | The custom `Element`: visible-range windowing, hitbox, one selection participant, N runs, row painting, and the geometry of both views. | | `diff/split.rs` | Side-by-side row geometry over `Vec`. | | `diff/palette.rs` | The four tint constants, moved from `decorations.rs`. | | `crates/ui/src/diff_view_mode.rs` | `DiffViewMode`, modelled on `theme_preference.rs`. | -`model.rs` and `pairing.rs` hold the logic and carry the tests. `row.rs` holds the risk. -The two layout modules should stay thin — they arrange rows, they do not decide anything. +`model.rs` and `pairing.rs` hold the logic and carry the tests. `body.rs` holds the risk. +A separate `diff/unified.rs` was planned and not written: the unified view differs from the +split one only in its column count and the width of its gutters, so `body.rs` answers both +from `Rows` rather than arranging each in a module of its own. `split.rs` stays thin — it +arranges rows, it does not decide anything. ## Data model @@ -136,15 +138,17 @@ hunk, a run at the very end with no trailing context, and a hunk of context only The body element registers one participant and declares that frame's visible rows as runs, in `prepaint`/`paint`: -- `TextSelectionRegistration::new(hitbox, bounds)` with `.with_document_order(n)` so a drag - across rows copies in document order. `.with_scroll_offset(..)` is **not** called: gpui - prepaints scroll children inside `with_element_offset` (`div.rs:1925`), and - `Window::layout_bounds` folds that accumulated offset into `bounds.origin` - (`window.rs:4697`) — so this element's `bounds.origin` already carries the scroll. - `gpui-base` stores a selection endpoint as `position − bounds.origin − scroll_offset` - (`text_selection.rs:1336`), so reporting the offset as well double-counts it and the - anchor drifts at twice the scroll delta. `with_scroll_offset` is for a participant that - scrolls its own content inside fixed bounds, which this element does not do. +- `TextSelectionRegistration::new(hitbox, viewport)` with `.with_document_order(n)` so a drag + across rows copies in document order, and `.with_scroll_offset(bounds.origin − + viewport.origin)`. `gpui-base` stores an endpoint as `position − bounds.origin − + scroll_offset` (`text_selection.rs:1336`) and resolves it back by adding both (`:806-812`), + so what matters is the sum. Reporting the element's own bounds and no offset gives the same + sum, and was what this was built with — but the registered *rectangle* is also the one + `AutoScroll::compute_delta` measures the pointer against (`:1452`), and the element's bounds + are the whole diff, not the part of it on screen, so the trigger zone sat off-screen and a + drag past the bottom edge never scrolled. Registering the viewport and reporting the + difference of the two origins keeps the sum identical by construction and puts the trigger + zone where the user can reach it. - `TextSelectionRun::new(text, layout, bounds)` — **only for the code content**. The gutters and the marker are neighbouring elements and are never registered, which is what makes a copied diff come out as clean code with no line numbers and no markers. This is