Fixed line number painting (#247)

[danwilliams]

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:

New behaviour example 1:

Previous behaviour example 2:

New behaviour example 2:

Previous behaviour side-by-side:

New behaviour side-by-side:

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]

@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]

@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:

• If a line has been removed it should be red on both sides.
• If a line has been added it should be green on both sides.
• If a line has changed it should be red on the left and green on the right.

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:

• 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.
• The suggestion you've made is interesting, about suppressing the background colours for that context, and it may well be worth adding some configuration to control this. If that happens, it may even be that the point above becomes configurable "for free", even though it is not worth doing on its own merits. I guess that depends on how the config would be implemented.

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]

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]

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:
bew/dotfiles@fb7ebb7?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'.

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:
bew/dotfiles@083bc66?diff=split
(I tried to find simple example commits in delta but couldn't find short ones)

[danwilliams]

@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]

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]

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

[RuRo]

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

[dandavison]

@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:

cc @th1000s @clnoll

[pablospe]

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

[dandavison]

+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]

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)