jj icon indicating copy to clipboard operation
jj copied to clipboard

diff: remove highlight from line-level hunks

Open yuja opened this issue 1 year ago • 11 comments

Checklist

If applicable:

  • [x] I have updated CHANGELOG.md
  • [x] I have updated the documentation (README.md, docs/, demos/)
  • [ ] I have updated the config schema (cli/src/config-schema.json)
  • [x] I have added tests to cover my changes

yuja avatar Jun 25 '24 11:06 yuja

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

yuja avatar Jun 26 '24 01:06 yuja

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

Can you share a screenshot of what it looks like?

martinvonz avatar Jun 26 '24 01:06 martinvonz

image

yuja avatar Jun 26 '24 01:06 yuja

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.

jonathantanmy avatar Jun 26 '24 19:06 jonathantanmy

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

yuja avatar Jun 27 '24 00:06 yuja

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.

jonathantanmy avatar Jun 27 '24 16:06 jonathantanmy

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.

ilyagr avatar Jun 27 '24 20:06 ilyagr

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?

joyously avatar Jun 28 '24 13:06 joyously

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.

ilyagr avatar Jun 29 '24 03:06 ilyagr

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.

joyously avatar Jun 29 '24 13:06 joyously

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.

emilazy avatar Jun 29 '24 14:06 emilazy

I'm closing this. The current diff output might look a bit verbose, but is consistent.

yuja avatar Aug 15 '24 12:08 yuja