search

projekt0n / github-nvim-theme

Github's Neovim themes
MIT License
2.02k stars 104 forks source link

Improve diff highlights #335

Open adambarjo opened 1 week ago

adambarjo commented 1 week ago

Hello! I've been trying out this theme in the past few weeks and have been really enjoying using it. It's probably the most well-rounded and polished theme I've come across. One area I found this theme to be slightly weaker than others is inside diff views. Particularly for the DiffText highlight group, it's really quite difficult to see the text within the line that has changed. In addition, the syntax highlighting is not preserved for changed and added code. Having the syntax highlighting preserved makes reading the diff more useful and matches GitHub's own diff style more closely. As well, other popular themes such as Catppuccin and Rose-pine preserve syntax highlighting in their diffs, it seems to be standard.

Below is an example of how the change looks.

Before:

image

After:

image

I have looked closely at all the different theme variations to make sure this change looks good on them. I am fairly new to contributing to open source, but trying to give it a go more regularly. Please feel free to give any feedback. Thanks

tmillr commented 1 week ago

I agree, I think the current diff highlights need some work. I also agree that the fg should not be set. Using a simple/straightforward or the recommended config, they also get overridden by the highlights for/in: https://github.com/projekt0n/github-nvim-theme/blob/4f44a5c930372c85483d02700f332d34417e50b2/lua/github-theme/group/modules/diffchar.lua#L1-L14

although this module could be manually disabled via config. I'm not sure if this override is intentional or not.

What would really be cool is something like this:

Screenshot 2024-06-30 at 3 29 56 AM

which is something that I've been working lately for my own setup. To get something like this though, it requires more than just setting highlights (e.g. custom 'statuscolumn', 'winhl' settings, etc.), and I'm also implementing it on top of diffview.nvim to make things nicer and a bit easier. Maybe we should add a diffview module too.

Also IMO diffAdded and DiffAdd etc. probably shouldn't be linked as they are now. In contrast, the former probably does work better with an fg setting, and typically gets used in situations where there is no highlighting otherwise (e.g. diff filetype, patch files, diff status indicator chars like in Telescope git status/commits). For example:

Screenshot 2024-07-01 at 12 02 11 AM Screenshot 2024-07-01 at 12 01 32 AM

To sum up, here's a potential plan:

  1. Reconcile the conflicts/overlap between diff mode highlights and diffchar module. Personally idk how diffchar works and don't use it, but perhaps we ought to prioritize the regular diff mode experience over it (if they share the same hl groups). It might be too challenging to try to give them both nice/correct defaults at the same time?

  2. Fixup diff mode highlights (i.e. DiffAdd etc.): don't set their fg, unlink them from diff filetype highlights used for patch files. Also improve DiffText in the process.

  3. Maybe add more advanced diff highlighting in order to make it look like GitHub.com, although this is more complicated and may require the user to use a plugin like diffview so that there is a foundation to build upon. Maybe we'd just implement this via a diffview module, separate from the regular diff mode highlights?

Related #334