Skip to content

First pass at a very dumb two file diff viewer#15631

Open
jcmuller wants to merge 18 commits intohelix-editor:masterfrom
jcmuller:jcmuller/push-xzklsptukmnn
Open

First pass at a very dumb two file diff viewer#15631
jcmuller wants to merge 18 commits intohelix-editor:masterfrom
jcmuller:jcmuller/push-xzklsptukmnn

Conversation

@jcmuller
Copy link
Copy Markdown
Contributor

@jcmuller jcmuller commented Apr 13, 2026

A vimdiff-style side-by-side diff viewer.

image

Usage

  • :diff-open file1 file2 opens both files in a vertical split and starts a diff session. Alias: :diffs.
  • hx --diff file1 file2 (or -d) does the same from the command line.
  • :diff-this marks a view; call it in a second view to pair them.

Inside a session

  • :diffget / reset-diff-change pulls the hunk at the cursor from the partner file.
  • :diffput pushes the current selection's hunk the other way.
  • :diff-off tears the session down, leaving both views open.
  • ]g and [g jump between hunks, using session hunks and falling back to VCS hunks when outside a session.

How it works

The renderer recomputes hunks when Document::version() changes. A LineAnnotation inserts virtual filler lines so both sides line up by logical row. Line backgrounds come from diff.plus, diff.minus, and diff.delta. Intra-line highlights use an OverlayHighlights layer keyed on diff.delta.text, falling back to diff.delta when unset. Changed regions are diffed at the word level (alphanumeric runs), so foo_bar vs baz_bar highlights foo rather than a run of character edits.

Theme

diff.delta.text is a new scope for intra-line changed characters; it falls back to diff.delta when unset. The default theme.toml defines it with an explicit bg so whitespace changes are visible against the diff.delta line background.

Tests

23 unit tests in helix-view/src/diff_session.rs cover alignment, hunk intersection, and the diffget/diffput transaction builders. Integration tests in helix-term/tests/test/diff_mode.rs exercise :diff-open, :diff-this, :diff-off, :diffget, and :diffput against tempfile-backed documents.

Known rough edges

  • Clicking on a filler line places the cursor above the intended row; click coordinates need remapping for virtual lines.
  • Intra-line highlights won't show up well if the active theme has low contrast between diff.delta's fg and bg.
  • A pure insertion at line 0 renders filler after the first line rather than before it.
  • Horizontal scroll, tabs, and wide characters aren't accounted for in filler column math.

@jcmuller jcmuller force-pushed the jcmuller/push-xzklsptukmnn branch from 9ece1d0 to 7dbb344 Compare April 13, 2026 22:23
@jcmuller jcmuller marked this pull request as ready for review April 14, 2026 08:42
@archseer archseer requested a review from pascalkuthe April 15, 2026 02:46
@jcmuller jcmuller force-pushed the jcmuller/push-xzklsptukmnn branch 3 times, most recently from a1e2218 to 38b6910 Compare April 20, 2026 13:17
@archseer
Copy link
Copy Markdown
Member

Great work, this is awesome to see! I'll test drive the branch over the week

@archseer
Copy link
Copy Markdown
Member

Here's the default theme config I'd like to see:

"diff.plus" = { bg = "#1f6f4e" } # shifted down by 20% on hsl
"diff.minus" = { bg = "#ad0b55" } # shifted down by 20% on hsl
"diff.delta" = { bg = "#41288c" }
"diff.delta.text" = { bg = "#5e3bc6" }
"diff.delta.conflict" = { fg = "#f22c86", bg = "#6a2038" }

"diff.plus.gutter" = { fg = "#35bf86" }
"diff.minus.gutter" = { fg = "#f22c86" }
"diff.delta.gutter" = { fg = "#6f44f0" }

I haven't adjusted diff.delta.conflict since I haven't seen it in practice yet.

Screenshot 2026-04-27 at 14 45 21

Bugs so far:

  • ]g seems to get stuck if your cursor is already on a changed area. Scrolling past it will allow you to correctly jump to the next change.
  • There seems to be some issues with diff alignment after removed lines. My test case is hx -d hosts/cerulean/kernel.config hosts/trantor/kernel.config under https://github.com/archseer/snowflake, it seems to align the first two deletions in the file correctly, but then the further changed blocks are slightly offset:
Screenshot 2026-04-27 at 14 47 39

@archseer
Copy link
Copy Markdown
Member

Also, only scroll position is updated but it would be good if we kept the cursor line position in sync as well. If I scroll halfway through a file then space-w-w to the other file I'd expect the cursor to stay on the same relative line position (e.g. if I am on line 4 and the file on the left lines up so that the same visual line is line 2 in the other file, I'd expect to go to line 2). That way the view stays fixed as I switch between the files. Currently, cursor will jump to the last known position, which often ends up scrolling both views and can be disorientating (e.g. if the other file was untouched, both views will jump to the start of the file).

jcmuller added 18 commits April 27, 2026 12:52
Add diff-mode.md covering hx --diff/-d, :diff-open, :diff-this,
:diff-put, :reset-diff-change, and :diff-off. Register the page in
SUMMARY.md under Usage.

Regenerate typable-cmd.md via cargo xtask docgen; the four new diff
commands (diff-open, diff-put, diff-off, diff-this) and the updated
reset-diff-change description now appear in the table.

Add diff.delta.text to the theme scope list in themes.md. Also fix
two pre-existing lint errors: MD041 (## to # on first line) and
MD059 ([here] to descriptive link text).

docs(diff-mode): fix theme key table; separate vimdiff vs VCS gutter scopes

The gutter scopes (diff.*.gutter) apply to the VCS diff gutter, not
to the vimdiff session. Split the table to make that clear, and fix
the icon descriptions: the gutter uses `▍` (bar) for added/modified
lines and `▔` (top tick) for deleted lines, not +/-/~.

docs(diff-mode): remove vimdiff reference
Pull out side_for_view(), hunks_arc(), and build_get_transaction() 
as dedicated helpers to cut down on duplication. Use and_then() for 
cleaner Option handling. Remove comments that just restate the code.

54 fewer lines overall.
Editor::close and close_document didn't clean up diff_sessions after a
view or document was closed. The render loop then hit
documents[&session.doc_a()] -- a BTreeMap index that panics on a missing
key -- and crashed.

Fix: Editor::close retains sessions not referencing the closed view, and
clears pending_diff_this if it pointed at that view. close_document
prunes sessions referencing the closed document.

Integration tests cover the three scenarios: closing a session view,
closing a session document, closing the pending diff-this view. Each has
a negative counterpart (unrelated view/doc leaves the session alone).
Unit tests verify DiffSession::contains_doc.
`intra_line_changes()` runs Myers diff over character tokens. The render
loop called it for every modified hunk every frame, even with no document
changes between frames.

`DiffSession` now stores `intra_line_cache: Arc<Vec<...>>` built in
`compute_hunks` alongside `hunks`. The render site snapshots it via
`intra_line_cache_arc()` in `diff_view_info` and reads the cache instead.

`InlineChange` now derives `PartialEq` for the unit tests.
The `line_decoration` closure in `render_view` scanned hunks from index 0
on every rendered line: O(lines * hunks) per frame.

The closure now keeps a `cursor` that advances past hunks whose range has
ended. `decorate_line` is called in ascending `doc_line` order within a
render pass, so no backtracking is needed. Rebuilding the closure each
`render_view` call resets `cursor` to 0 automatically.

Same pattern as `DiffAlignment::filler_lines_at`.
Lines that exist only in file A (pure-deletion hunks) were colored
with diff.plus (green) instead of diff.minus (red). Extract the style
dispatch into diff_line_style and dispatch based on DiffSide so each
category maps to the correct theme scope:

- DiffSide::A, other empty -> diff.minus (deleted from B)
- DiffSide::B, other empty -> diff.plus (added in B)
- either side, other non-empty -> diff.delta (modified)

Covered by a unit test in diff_coloring_tests.
Replace the character-level Myers diff tokenizer in `intra_line_changes`
with a word-level one. Alphanumeric+underscore runs are one token;
whitespace and punctuation stay as single-char tokens.

With character-level diff, substituting one word marks several small
fragments as changed. When the theme defines `diff.delta.text` with an
explicit background, that overlay covers most of the line and hides the
line-level diff color. Word granularity limits the overlay to the changed
word(s) only.

`word_token_char_starts` precomputes char offsets per token so token-index
hunks from imara-diff can be mapped back to char-column ranges.
… scopes

theme.get() returns Style::default() when a scope is absent, which has
no fg and no bg. fg_to_bg then returns Style::default() unchanged, and
renderer.set_style with an all-None style is a no-op on cell backgrounds.

Switch to theme.try_get() so themes that define no diff scopes at all
receive hardcoded fallback colors instead of invisible highlights.
When a theme sets both fg and bg on diff.minus, diff.plus, or diff.delta,
push an OverlayHighlights::Homogeneous for every affected line. Overlays
patch over syntax fg in draw_grapheme, so the theme's fg overrides syntax
color on the diff line (the vimdiff "whole line in one color" look).


Themes that set only fg or only bg don't trigger this. The overlay is
gated on both being present, so the upstream nord convention
("diff.minus" = "nord11", fg-only) continues to behave as a line bg via
the existing fg-as-bg fallback.


Refactors the in-closure fg_to_bg helper into a module-level diff_bg_style
fn with a four-branch match, preserving fg when both fg and bg are set.
Adds diff_line_char_ranges for bucketing hunks into minus/plus/delta char
ranges on the current side. Tests cover all four diff_bg_style branches
and the four hunk-bucketing cases (pure deletion, pure addition, modified
pair on either side, mixed hunks, empty input).
Rebase onto trunk introduced an unconditional `use termina::event::{Event, KeyEvent};` alongside the platform-gated imports below. The cfg-gated block already covers both Windows and non-Windows targets. Remove the duplicate so the integration test target compiles.
Both entry points for the two-file diff view (CLI `--diff` and the
`:diff-open` command) already parsed the optional `file:line` or
`file:line:col` suffix via `parse_file`, but discarded the position.
Opening `hx --diff a.rs:42 b.rs:58` landed both panes at line 1.

Keep the position and apply it after each `editor.open`, using the same
pos_at_coords + set_selection + align_view pattern that the non-diff
file-open loop uses. In application.rs the work goes through a small
`apply_diff_positions` helper so both sides share one code path.

Integration tests cover the four cases that matter:

- `:diff-open a:3 b:5` positions both panes
- `:diff-open a:3 b` positions one pane, leaves the other at the top
- `:diff-open a b` with no suffixes keeps both at line 1 (regression guard)
- `hx --diff a:3 b:5` via `AppBuilder::with_diff` proves the CLI branch
  lands each cursor in its own pane

The CLI test was mutation-checked by commenting out both
`apply_diff_positions` calls and confirming the assertion fails.
@jcmuller jcmuller force-pushed the jcmuller/push-xzklsptukmnn branch from 38b6910 to 9c2608f Compare April 27, 2026 16:52
@m4rch3n1ng
Copy link
Copy Markdown
Contributor

m4rch3n1ng commented Apr 27, 2026

would it be possible to have more dedicated theme keys for this? something like diff.delta.line, diff.plus.line and diff.delta.text or even more explicit diff.delta.view.line, ..., or something like that? 'cause i don't really want to change the diff.delta/diff.plus/etc highlights, as that also changes how the space+g menu and the diff language is rendered.

@m4rch3n1ng
Copy link
Copy Markdown
Contributor

thinking it over, i think my preference would be:

Scope Purpose
diff.delta.view Modified line background
diff.delta.view.text Intra-line changed characters
diff.minus.view Deleted line background
diff.plus.view Added line background

doing it like that and applying the styles directly also makes it, so i can theoretically highlight the diffs in the diff view via text color instead of being forced to use them as background color

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants