search

dandavison / delta

A syntax-highlighting pager for git, diff, grep, and blame output
https://dandavison.github.io/delta/
MIT License
22.76k stars 381 forks source link

Fixed line number painting (#247) #1558

Open danwilliams opened 11 months ago

danwilliams commented 11 months ago

Description of changes

Fixed the painting of line numbers so that instead of each column or side always keeping the same colour, which looks odd, it instead uses the colour appropriate to the line's status.

In other words, if there has been a line removed, instead of showing the left (minus) column/side as minus style and the right (plus) column/side as plus style, both will be shown as minus style.

Previous behaviour example 1: image

New behaviour example 1: image

Previous behaviour example 2: image

New behaviour example 2: image

Previous behaviour side-by-side: image

New behaviour side-by-side: image

Notes for review

Although this change was incredibly simple to make, and appears to work just fine in all the places I've tried, I do not have wide codebase knowledge on this project and so if there are any additional things I should check or consider, please let me know 🙂

RuRo commented 11 months ago

@danwilliams the "New behaviour side-by-side" looks a bit weird (especially around line 15). I kind of understand why it is like that, but I wonder if it would be better to not apply any background color to the "empty"/"padding" lines in side-by-side mode?

Also, how hard do you think it would be to make this more configurable instead of hard coding one style or the other? Something like minus_left_style, minus_right_style and plus_left_style, plus_right_style that default to minus_style and plus_style correspondingly.

Other than that, LGTM.

danwilliams commented 11 months ago

@RuRo thanks for your consideration and feedback 😄 Those are good questions.

the "New behaviour side-by-side" looks a bit weird (especially around line 15).

When I was testing the change I considered how it should look, and whether this behaviour is correct. The logic as I see it is:

With that in mind I think the changes fix the issue and make the output match logical expectation, but I did purposefully choose a diff to screenshot that would illustrate an interesting example. If the example on line 15, the line itself has been changed, hence red on the left and green on the right, but it has also got longer and wrapped, and the continuation line on the right is "new". This makes it green on the left. It's exactly the same outcome if you modify one line and then add one right below it - but I did have a hard think about whether this is right or not.

I kind of understand why it is like that, but I wonder if it would be better to not apply any background color to the "empty"/"padding" lines in side-by-side mode?

That's an interesting possibility that I had briefly considered. I think it is better to show the colours on both sides, but I am not 100% convinced on that. I think for "standard" mode the behaviour is definitely correct, but in side-by-side mode it might work just as well - or better - to suppress the background colour in this scenario.

Also, how hard do you think it would be to make this more configurable instead of hard coding one style or the other?

Well, I think there are two scopes there to answer:

I think I would take the view that the addition of new config would be a feature, and I'd be happy to look at doing that separately. Meanwhile this change is small and fixes the immediate issue. So I guess I'd vote to merge this, and then create another ticket for a new feature to implement the configuration change, which I can pick up and deal with as well 🙂

What do you think?

RuRo commented 11 months ago

Well, I think there are two scopes there to answer:

  • The change I've made is to fix a bug in the existing behaviour, because the current rendering is clearly wrong. So I don't personally see that there should be an option to toggle between the old and new behaviour, as I don't think the old behaviour makes sense, and just looks confusing.

I agree, that in the "standard" view, this is an unambiguous improvement (fix for a clearly buggy behaviour).

However, my primary concern here is that in "side-by-side mode" the old painting rules arguably make more sense than the new style. Since the proposed change is non-configurable, I am a bit hesitant to trade off the broken "standard mode" number painting for broken "side-by-side mode" number painting.

bew commented 11 months ago

I agree with @RuRo, that line 15 on the side-by-side layout is weird. To me the left side is for lines that have been removed, the right side is for lines that have been added.

With the OLD behavior it was always misleading me into thinking there was a blank line below L15 that was replaced with actual content. With the NEW behavior it feels out of place (like a bug) to have green background in the 'removed lines side'. And having green blank line on the left side is even weirder because the newlines are definitely not empty.

I'd also prefer to have no background when there was no useful line info on a side.

If we compare to github's side-by-side layout, for example this commit from my dotfiles: https://github.com/bew/dotfiles/commit/fb7ebb71cba2777671daafd433fe6e9bcfe60070?diff=split You can see that added lines are completely gray on the 'removed side', and removed lines are completely gray on the 'added side'.

Screenshot_20231109-062731

I'd like to have that kind of behavior with delta as well, with a separate color (might be gray) when there is nothing on a side, to clearly see what is old/new code.

I'm not sure what to do with the line number separator for those lines, I'm on the fence of simply removing it so the whole side line is 'gray'..

(in your screenshots I think you should also show an example of deleted code with/without replacement, not only added code)

Here is another example commit with ~all cases I think, with changed lines (with different number of lines before/after), only removed and only added lines: https://github.com/bew/dotfiles/commit/083bc6614e26aea6ef5eef31c3bd913c754bbf7b?diff=split (I tried to find simple example commits in delta but couldn't find short ones)

danwilliams commented 11 months ago

@bew yes that's a good point, and a valid one. I like the idea of there being a grey colour - that makes it much clearer indeed.

What do we think the right answer is for the current fix? Just restrict it to "normal" mode and not alter side-by-side for now? Or suppress it for side-by-side? It feels like adding the grey would be a new feature (which I will happily look at later).

bew commented 11 months ago

My opinion here would be to keep your fix for unified layout, and for side-by-side layout: remove the bg color of lines that should be 'grey' to be 'correct'. Ideally we should fix / get a good behavior for both layouts in one go though, but I think we can do in 2 steps as long as we don't make it worse in between.

@dandavison as the author of the project, what do you think?

dandavison commented 10 months ago

Sorry I'm being slow to contribute here. What do you think about laying out the options in a series of schematic diagrams, where we can also add annotations regarding options etc? I've made a quick start here (doc editable by anyone):

https://docs.google.com/document/d/1XF8Rcg5rNuE1976ttf6yCzlfBHDRxGJJCIpfSVQjV_Y/edit?usp=sharing

image
RuRo commented 10 months ago

I've updated the visualizations in the doc. Here are the screenshots:

old

new

dandavison commented 10 months ago

@RuRo that is a work of art, thank you very much for doing that. I have set permissions on the doc to comment-only to avoid vandalism, but anyone -- please let me know if you'd like edit permissions.

@danwilliams (and @bew) would you be able to look at the Google Doc and say (a) whether it accurately represents your understanding and (b) where you think we should go from here? My initial reaction is that @RuRo's proposed modification to side-by-side looks good:

image

cc @th1000s @clnoll

pablospe commented 9 months ago

+1 to the @RuRo's proposed modification to side-by-side.

dandavison commented 9 months ago

+1 to the @RuRo's proposed modification to side-by-side.

Agreed. I think this branch doesn't currently implement quite that. Is @danwilliams or anyone else able to modify this branch to do it?

dandavison commented 7 months ago

Just a status note: I think the work here is awesome and we should try to get it merged, but there is one visual aspect that I think the majority of us discussing still want to improve. (It's possible that that remaining bit is going against the current implementation and that's why no-one has done it; I haven't looked properly)