sindrets / diffview.nvim

Single tabpage interface for easily cycling through diffs for all modified files for any git rev.
Other
3.74k stars 105 forks source link

`DiffDelete` highlight used in expected places #340

Closed egargan closed 1 year ago

egargan commented 1 year ago

There are a few situations where Diffview uses the DiffDelete highlight group, where it seems it'd make more sense to use DiffviewDiffDelete or DiffviewDiffAddAsDelete.

I have my coloursheme set up as follows.

highlights["DiffDelete"] = { bg = "#5A424D", fg = "none" } -- Red
highlights["DiffviewDiffAddAsDelete"] = { bg = "none", fg = "#4C566A" } -- Dark grey
highlights["DiffviewDiffDelete"] = { bg = "none", fg = "#4C566A" } -- Dark grey
highlights["DiffAdd"] = { bg = "#45504F" } -- Green
highlights["DiffChange"] = { bg = "#54524F" } -- Yellow
highlights["DiffText"] = { bg = "#70695A" } -- Lighter yellow

This looks great for the most part, for files with single or few-line diffs, e.g.

But when diffs span entire files, e.g. when a file is added, it appears DiffDelete is applied, which seems inappropriate.

The same goes for files that have been deleted...
As well as merge conflict views...

Do you think it'd make sense to change the highlight groups used in these scenarios? Or can you suggest another change that might fix these odd-looking colours? Happy to have a go myself with a bit of guidance.

sindrets commented 1 year ago

I pushed a fix that addresses the enhanced_diff_hl not being applied to null buffers (used as the other side of the diff when a file has been added / deleted).

enhanced_diff_hl only applies to diff2* layouts, so this won't change anything for merge tool layouts. I realize this wasn't very clear in the documentation, so I have amended the docs to clarify this.

DiffDelete is the default highlight used for the filler lines in diff-mode. If you - like me - prefer more subtle highlighting for these filler lines, I suggest you link this group to something else. It's what I do:

:hi DiffDelete
DiffDelete     xxx guibg=#3e303d
                   links to Comment
egargan commented 1 year ago

Thank you very much for those swift fixes!

If you - like me - prefer more subtle highlighting for these filler lines, I suggest you link this group to something else. It's what I do:

This does look much better in Diffview views absolutely, but DiffDelete being the de facto highlight group for all Git-delete-related highlight groups, doing this makes other Git plugins (e.g. lewis6991/gitsigns.nvim) look a bit odd.

image

I'm tempted to suggest an optional set of highlight groups for Diffview, e.g. DiffviewDiffAdd, DiffviewDiffDelete, to avoid conflicts between Git plugins, and let us make both Diffview and other plugins as beautiful as possible.

This could just be a plugin option like custom_diff_hl_groups, which tells Diffview to use e.g. DiffviewDiffAdd, DiffviewDiffDelete vs DiffAdd and DiffDelete.

But this is just a suggestion, consider this issue fixed, thanks again!

dasupradyumna commented 1 year ago

enhanced_diff_hl only applies to diff2* layouts, so this won't change anything for merge tool layouts. I realize this wasn't very clear in the documentation, so I have amended the docs to clarify this.

@sindrets Is extending enhanced_diff_hl to apply to merge tool layout in the future of this plugin? I think it will improve the uniformity of colors across the Diffviews.

sindrets commented 1 year ago

@dasupradyumna What would you expect enhanced_diff_hl to do in a 3-way diff? 2-way diffs in VCS are in a unique position. It's a comparison between old state on one side, and new state on the other. As such, a diff "insertion" in the old state actually represents a deletion from the perspective of the new state.

This same logic does not apply to 3-way diffs, where you have one version for each merge parent, as well as the result from the merge. Highlighting insertions as deletions does not make sense in any of these states.

dasupradyumna commented 1 year ago

I am not aware of the specifics of the implementation, so I apologize if I asked something unreasonable.

status_merge

I was simply wondering whether it's possible to color the red "deletion" regions in 3-way (or 4-way) merge layout to be grey like a 2-way diff between 2 states. If it's not, I understand; I am a big fan of this plugin either way. :)

sindrets commented 1 year ago

I'm tempted to suggest an optional set of highlight groups for Diffview, e.g. DiffviewDiffAdd, DiffviewDiffDelete, to avoid conflicts between Git plugins, and let us make both Diffview and other plugins as beautiful as possible.

@egargan granted!


I was simply wondering whether it's possible to color the red "deletion" regions in 3-way (or 4-way) merge layout to be grey like a 2-way diff between 2 states.

@dasupradyumna after the latest commit enhanced_diff_hl will highlight the diff delete fill-chars more subtly in all layouts. Not only the diff2* layouts. And if you wish to change this highlight without messing with how DiffDelete is used in other contexts, you can simply adjust DiffviewDiffDelete.


Including the full commit message below for additional clarity:

Adds new highlight groups shadowing all the default diff highlights:

- `DiffviewDiffAdd` -> `DiffAdd`
- `DiffviewDiffDelete` -> `DiffDelete`
- `DiffviewDiffChange` -> `DiffChange`
- `DiffviewDiffText` -> `DiffText`

These are set as 'winhl' in all the diff buffers created by the plugin.
This way users will be able to change the diff highlighting for Diffview
only, without affecting the use of these groups outside of the plugin.

OTHER CHANGES:

Having `enhanced_diff_hl` enabled will now highlight delete fill-chars
more subtly in *all* layouts. Not only `diff2*` layouts.
dasupradyumna commented 1 year ago

Thanks a lot for the lightning fast response! ❤️

status_merge

It works as expected now, I needed this change to include in my new colorscheme plugin. Cheers mate ~