Skip to content

Make the word highlighting colors follow the hunk rather than the line#2400

Merged
Urgau merged 4 commits into
rust-lang:masterfrom
Urgau:gh-range-diff-word-hl-colors
May 11, 2026
Merged

Make the word highlighting colors follow the hunk rather than the line#2400
Urgau merged 4 commits into
rust-lang:masterfrom
Urgau:gh-range-diff-word-hl-colors

Conversation

@Urgau
Copy link
Copy Markdown
Member

@Urgau Urgau commented May 9, 2026

While looking at one of the example from #2394, I found my-self a bit perplex at the word highlighting we do.

In particular regarding the color we use which currently follows the color of the line, but that doesn't really make sense, as we want to show what was removed/added between the two part of the hunk, and for that red/green are the standard colors.

This PR therefor changes the word highlighting to always show removed words in red, and added words in green (EDIT: except in a removed line where we use orange, see below). An attempt is made to dim the color for a removed line, where knowing exactly what changed is less relevant.

Light theme:

Currently This PR
image image
V1 & V2imageimage

Dark theme:

Currently This PR
image image
V1 & V2imageimage

First commit is the actual change, second commit is a refactor/simplification and the other are the orange change.

Testing: http://localhost:8000/gh-range-diff/rust-lang/miri/98cb1d3d48fa7c8e97994050bfd69e8f3da2a1a7..3f8007e19b8cac3d08fb07579bde3217c58807e4/e0b3afbdd5362aa348abc6661ed6417d1a21f58c..d811aaf958594156e2ce38365b835c361aa88de9

@RalfJung does the new colors make sense to you?

@Urgau Urgau requested a review from Kobzol May 9, 2026 21:41
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented May 10, 2026

For - +/+ + lines, I agree the new colors are strictly better, That's just "changes to the code being added" and it should be red/green.

For + -/- - lines, I am less sure. The green makes it look like there's text being added, but it's really "more text being removed" or "text was added on master". I have no idea how to best indicate that with colors...

Maybe hovering that text could show a tooltip explaining this? Like, "this word was recently added on main; this diff indicates that it is being added to the code removed by this PR".

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented May 10, 2026

A tooltip could work, but requires the user to hover which I think is less than ideal.

Maybe we shouldn't be using green for word added to the removal, but using an intermediate color like orange? It's an intermediate color that doesn't have the meaning of green but is still distinctive.

Slightly modified example:
image

@RalfJung
Copy link
Copy Markdown
Member

requires the user to hover which I think is less than ideal.

Yeah that was just meant as a way for the user to discover the meaning of the color. Adding it to the legend at the top could also work.

I like the orange! I wonder if - - lines should also use a different color then, though it already is a darker shade of red than - + which is good.

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented May 10, 2026

I wonder if - - lines should also use a different color then, though it already is a darker shade of red than - + which is good.

Yeah, it's already a dimmed red variant, which doesn't like an issue for - -.

I like the orange!

Then that's settled, let's use a orange for + -.

Juts noting that I had to use a slightly darker orange to respect the minimum contrast requirements, still works well IMO.

image

@RalfJung
Copy link
Copy Markdown
Member

That darker orange does get quite close to the red though. I guess we'll see in practice whether that is a problem.

@fmease
Copy link
Copy Markdown
Member

fmease commented May 10, 2026

What if you used the original lighter orange but had the text be black (in the first image it's white)? Wouldn't that uphold a good contrast ratio? Or would that look awful in some way?

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented May 10, 2026

Here the lighter orange with a black text. It's quite good in terms of contrast, but I can tell if it's better than the darker orange with white text.

image

Maybe it should be even lighter?

image

Or even more orange?
image

@RalfJung
Copy link
Copy Markdown
Member

How does it look next to a - - line? Is it odd that one of them uses black text and the other white text?

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented May 10, 2026

It would look like these:

image image image

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented May 10, 2026

And the one currently implemented in the PR (with the white text and darker orange), looks like this:
image

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented May 10, 2026

It would look like these:

I strongly prefer all of those over the currently implemented one.

Among the alternatives with black text, I have a mild preference for the 3rd variant (strong orange).

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented May 10, 2026

Sure, works for me. Changed it.

@Urgau Urgau added this pull request to the merge queue May 11, 2026
Merged via the queue into rust-lang:master with commit 1a96029 May 11, 2026
3 checks passed
@Urgau Urgau deleted the gh-range-diff-word-hl-colors branch May 11, 2026 10:30
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.

4 participants