Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update diff color changes #155344

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Update diff color changes #155344

merged 1 commit into from
Jul 18, 2022

Conversation

miguelsolorio
Copy link
Contributor

Refs #153563

In #154969 I simply updated the color token diffEditor.insertedTextBackground which is used in both the line and word diff. There was some feedback that this was too strong of a change and maybe we should update the word diff color instead. This PR attempts to update the word diff color token instead.

There is one downsides to this approach: by default diffEditor.insertedLineBackground is set to null but it uses the same color as diffEditor.insertedTextBackground. In order to achieve what we want, I had to swap the values and add in the new color for diffEditor.insertedTextBackground. Below is a comparison of original (left) and proposed (right):

CleanShot 2022-07-15 at 13 23 26@2x

CleanShot 2022-07-15 at 13 23 45@2x

All seems fine but when looking at themes, not every theme defines these two color tokens, so this change does mean some authors need to define this token:

CleanShot 2022-07-15 at 13 25 34@2x

CleanShot 2022-07-15 at 13 26 21@2x

CleanShot 2022-07-15 at 13 27 46@2x

If this looks good then we can make the change and mention this in the release notes for theme authors.

cc @bpasero @joaomoreno

@hediet hediet merged commit 58402a0 into main Jul 18, 2022
@hediet hediet deleted the misolori/brilliant-unicorn branch July 18, 2022 09:45
export const diffInserted = registerColor('diffEditor.insertedTextBackground', { dark: defaultInsertColor, light: defaultInsertColor, hcDark: null, hcLight: null }, nls.localize('diffEditorInserted', 'Background color for text that got inserted. The color must not be opaque so as not to hide underlying decorations.'), true);
export const diffRemoved = registerColor('diffEditor.removedTextBackground', { dark: defaultRemoveColor, light: defaultRemoveColor, hcDark: null, hcLight: null }, nls.localize('diffEditorRemoved', 'Background color for text that got removed. The color must not be opaque so as not to hide underlying decorations.'), true);
export const diffInserted = registerColor('diffEditor.insertedTextBackground', { dark: '#9ccc2c33', light: '#9ccc2c66', hcDark: null, hcLight: null }, nls.localize('diffEditorInserted', 'Background color for text that got inserted. The color must not be opaque so as not to hide underlying decorations.'), true);
export const diffRemoved = registerColor('diffEditor.removedTextBackground', { dark: '#ff000066', light: '#ff00004d', hcDark: null, hcLight: null }, nls.localize('diffEditorRemoved', 'Background color for text that got removed. The color must not be opaque so as not to hide underlying decorations.'), true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hediet Was this change tested in the cases where the charDeleteWholeLine decoration is used?
The problem there is that you end up with a rectangular region of the viewport where a (quasi-infinite width) .char-delete is layered on top of .line-delete.

You can easily get this in this situation when comparing a non-empty file with an empty one, e.g.:

seq 50 > a; echo -n > b; code -d a b

(Though a more realistic usecase, where I actually hit this in practice, is large refactors where sometimes more than a screenful of code got removed/replaced)

Having charDeleteWholeLine be forced to look exactly like charDelete is probably wrong (I get more into this later on, just wanted to mention it), for example GitHub diffs don't use the "inline removed" highlight style (like charDelete) for "bulk line removal" (i.e. as if charDeleteWholeLine wasn't applied or was simply transparent, though I doubt getting rid of charDeleteWholeLine makes sense for VSCode).


If my alpha blending math isn't off, this is what happens (for the Dark+ theme):

Before this PR (≤1.69) After this PR (≥1.70)
.char-delete #ff000033 ( 0.2 alpha) #ff000066 ( 0.4 alpha) ↗️
.line-delete #ff000033 ( 0.2 alpha) #ff000033 ( 0.2 alpha)
.char-delete
on top of
.line-delete
#ff00005c (0.36 alpha) #ff000085 (0.52 alpha) ↗️

(The formula I used for e.g. "0.4 alpha red on top of 0.2 alpha red" was: (bg * (1 - 0.2) + red * 0.2) * (1 - 0.4) + red * 0.4, which simplifies to bg * 0.48 + red * 0.52, i.e. "0.52 alpha red")

So that's a ~44% increase in the brightness of pure red (though that's before it gets blended with the background under it, gamma curves, etc. etc.)

More visually, I was able to get these comparisons (both red and green are from "bulk" changes, i.e. char{Delete,Insert}WholeLine that span the whole viewport width):

≤1.69 ≥1.70 greener

To me, it looks like only the first and third columns have "equivalent red/green brightness".
The "greener" one was made by bumping up the green alpha in this PR from 0.2 to 0.4:

"workbench.colorCustomizations": {
    "diffEditor.insertedTextBackground": "#9ccc2c66"
}

My personal experience is that I happened to completely change monitor setups around the same time 1.70 released (the first release to have this PR), and kept trying to adjust their settings to no avail: I couldn't get such intense red diffs (covering a large part of 27" 4K monitor) to not physically hurt without dimming the monitor so much that looking at (non-diff) syntax-highlighted code felt straining.

Eventually I gave up and started searching for VSCode changes, and that's how I found this. I was also able to run a VSCode 1.69 instance (with --user-data-dir= pointing to a temp dir) to confirm it's indeed a recent change. While testing, even looking at a different monitor, the eye in the direction of the monitor with the VSCode 1.70 window on it, was hurting.

(This experience has also made me consider switching themes, but I still really like Dark+ aesthetically)


The ≤1.69 behavior can be perfectly replicated with this setting, AFAICT:

    "workbench.colorCustomizations": {
        "diffEditor.removedTextBackground": "#ff000033"
    }

And with that I have been able to adjust my monitors to get things looking the way I remember them.

However I will have to say that I really like the new more intense highlights for small ranges, and my humble suggestion would be that charDeleteWholeLine and charDelete should be independently themable (similarly with insertions, it's just that "pure red" had a more immediate impact, especially given that the softer green still has a smaller alpha, even after this PR - and I may even prefer a brighter green under the same "only for small inline highlights" condition).

Right now, the "small inline highlight" and "bulk coloring of a large area" situations are indistinguishable by CSS so it's impossible to assign different background colors to them.


Also, I am not opening this as a separate issue just yet because I couldn't find any reports of people complaining about this, so I'm not sure if there's something else going on that I'm missing - it's possible on average almost nobody sees this to an extent that it's actually noticeable?

It does happen to be the case that I've been combing through a lot of diffs recently (checking for various inconsistencies after a large refactor), so I got exposed to a lot more of the color changes all at once - and even then, it took me a whole week before I finally was sure of what had happened.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddyb thanks for you feedback! I don't have the time right now to look into your issue, but I want to soon. Can you please create an issue so I/we can keep track of it in the GH inbox? Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants