search

martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
7.24k stars 240 forks source link

diff: remove highlight from line-level hunks #3958

Open yuja opened 1 week ago

yuja commented 1 week ago

Checklist

If applicable:

yuja commented 1 week ago

@jonathantanmy, @martinvonz any thoughts on this? good? bad?

martinvonz commented 1 week ago

@jonathantanmy, @martinvonz any thoughts on this? good? bad?

Can you share a screenshot of what it looks like?

yuja commented 1 week ago

image

jonathantanmy commented 6 days ago

The highlight (that is, the brighter color and bold) looks distracting to me, and I think that both new lines and new words should look the same (as it is, before this patch), but I'll leave it to you all to decide.

yuja commented 6 days ago

The highlight (that is, the brighter color and bold) looks distracting to me,

I'm happy to remove bold = true. I just copied it from Mercurial as baseline.

I think that both new lines and new words should look the same (as it is, before this patch),

I see. The intent of this patch is to remove underline from large block, but I understand that consistent looking is also nice.

the current main: image

this patch + bold = false: image

jonathantanmy commented 5 days ago

Ah, good point. I personally think both are fine, but I agree that there are users that might want to style removed/added and changed lines differently (something more subtle and less precise for removed/added lines and something more precise and less subtle for changed lines) so this change looks good to me. Let's see what others think.

ilyagr commented 5 days ago

I think I'd find this quite useful!

Apart from less busyness, it's nice to be able to tell at a glance whether only a part of a line is changed.

This will also highlight things like removing all of a line except for the newline. At first, I thought this is very good, though I now wonder whether this might also be annoying if the diff algorithm interprets what the user thinks of as deleting and inserting a line this way. Perhaps we should add a test like this? I might also just test it out on my own machine, or somebody else could.

I'm not sure whether everyone will like this, so think it should be easy to disable for people who want more uniformity. I am not 100% sure whether this should be the default, but well -- it'd work for me.

joyously commented 4 days ago

I do not like the underlines. I prefer the background color change. I don't quite understand the coloring of the line numbers, though. What does it do when there is both an add and a delete on the same line?

ilyagr commented 3 days ago

I do not like the underlines. I prefer the background color change.

Do you like the delta example here: https://github.com/martinvonz/jj/pull/3914#discussion_r1649825847 ?

I am not entirely sure, but I think something like that should be possible with this PR (and in just 16 colors). If it is in fact possible, it'd likely be just a matter of changing some config after this PR. We could experiment with making that either as an alternative theme or by default.

(The one part we wouldn't yet be able to support with a config change is the special trailing whitespace highlight, but if we decide it's useful, we could implement that too)

What does it do when there is both an add and a delete on the same line?

See the screenshot in https://github.com/martinvonz/jj/pull/3958#issuecomment-2192839153, the line in the middle.

joyously commented 3 days ago

Do you like the delta example here: #3914 (comment) ?

Yes, it looks good to me, but I think it would be difficult for the 7% that are colorblind, so if it's the default, there needs to be a colorblind theme.

emilazy commented 3 days ago

Because of the colour‐blindness considerations, MediaWiki has used #FFCC33 for deletions and #AFB6E9 for additions by default for several years. Blue is usually rather dark in terminals, so it may work as a background for additions; the “yellow/brown” colour might work for removals too. I don’t know if they would be intuitive enough to use as a default; the 16‐colour palette is depressingly limited.