[vim/vim] runtime(diff): Update default links (PR #13776)
Evgeni Chasnovski
Problem: Current default links for diffAdded, diffChanged, and
diffRemoved do not address the diff nature of the filetype.
Solution: Use DiffAdd, DiffChange, and DiffDelete.
Resolves #13759. I also updated default link for diffChanged, as there is also a good DiffChange alternative.
You can view, comment on, or merge this pull request online at:
Ā Ā https://github.com/vim/vim/pull/13776
Commit Summary
- e65c756 runtime(diff): Update default links
File Changes
(1Ā file)
- M runtime/syntax/diff.vim (6)
Patch Links:
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
thanks!
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Merged #13776 into master.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
ä¾äŗ
Diff* groups are documented as diff mode things. Perhaps the documentation should be updated?
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Evgeni Chasnovski
Diff* groups are still used as diff mode things. Now they are also used as default links for diff* groups. There is no reason to document it; same as with other default links. For example, it would be strange to also document that Constant is used to display diffCommon by default.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
ä¾äŗ
DiffAdd Diff mode: Added line. |diff.txt|
This sounds like it's only used in diff mode (and it was). I have a colorscheme which made this assumption and broke diff filetype highlighting.
On the other hand, Constant isn't tagged. It's below "group-name" rather than "hl-XXX". If you compare the two parts of the documentation, it's clear that "hl-XXX" part intends for Vim's interface, while the "group-name" one intends for filetype syntax definition.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
DiffAdd Diff mode: Added line. |diff.txt|
So DiffAdd is now used for Diff mode as well as for diff filetypes to mark lines that will be added. Similar for DiffDelete and Diffchanged. It seems similar enough, that I did not think documentation should be necessary. So how about the following then:
diff --git a/runtime/doc/syntax.txt b/runtime/doc/syntax.txt index 040780b35..b96978a3c 100644 --- a/runtime/doc/syntax.txt +++ b/runtime/doc/syntax.txt @@ -5383,11 +5383,11 @@ CursorLine Screen line that the cursor is in when 'cursorline' is set. *hl-Directory* Directory Directory names (and other special names in listings). *hl-DiffAdd* -DiffAdd Diff mode: Added line. |diff.txt| +DiffAdd Diff mode and diff filetype: Added line. |diff.txt| *hl-DiffChange* -DiffChange Diff mode: Changed line. |diff.txt| +DiffChange Diff mode and diff filetype: Changed line. |diff.txt| *hl-DiffDelete* -DiffDelete Diff mode: Deleted line. |diff.txt| +DiffDelete Diff mode and diff filetype: Deleted line. |diff.txt| *hl-DiffText* DiffText Diff mode: Changed text within a changed line. |diff.txt| *hl-EndOfBuffer*
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Evgeni Chasnovski
@chrisbra, adding information about default links is a bit misleading. The groups in 'diff' filetype are diffAdded, diffRemoved, and diffChanged; these are actually used and not DiffAdd, DiffDelete, DiffChange.
It might be better to document diff* groups directly. For example, by adding *ft-diff-syntax* in 'doc/syntax.txt', similar to *ft-tex-syntax*.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
I see. I'll add something later
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
This sounds like it's only used in diff mode (and it was). I have a colorscheme which made this assumption and broke diff filetype highlighting.
Just out of curiosity why would this change break diff filetype highlighting? Just the colors chosen doesn't match?
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
ä¾äŗ
Just out of curiosity why would this change break diff filetype highlighting? Just the colors chosen don't match?
In diff mode, DiffRemoved never has useful text (they are all ----) so I made a dimmed color that was very similar to the background.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Romain Lafourcade
This is a mistake.
DiffAdd and friends are use in diff "mode".
DiffAdded and friends are used in buffers with the diff filetype (patches, etc) and git commits when you set up Git to show diffs in that context.
Those are two different usages that require different stylings.
Here is a file with the diff filetype before this change:
where everything is smooth and readable.
Here is the same file, after this change:
Capture.d.ecran.2024-01-04.a.16.42.27.png (view on web)where everything is noisy.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
Possible problems were mentioned: #13759 (comment)
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Clason
This is just about the default link; colorschemes can (and should) still style these however they wish.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Evgeni Chasnovski
Those are two different usages that require different stylings.
Nobody argues against that. The question is about default links for diff* highlight groups which would with higher probability result into what is usually considered "diff colors": green for diffAdded, red for diffRemoved, not green and not red for DiffChanged.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
It's true, the new default looks noisy. Not sure if we should make all color scheme authors additional work to fix this issue on their side.
Can we find a better default? that also fixes #13759 ? I am open to suggestions.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Clason
Note that not having these links added more work for other colorscheme authors. No matter what you pick, it will be bad for some and better for others. If you want to be objective, you would need to count how many colorschemes (that do not set these directly -- which professional colorschemes should) profit from each default.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Yeah also true. Whatever you do, you will choice wrong š
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
--
shane.xb.qian
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
Hmm, maybe it's better to just set these to green and red directly instead of linking them to some other groups? That may achieve the intended purpose for this PR better.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
Hmm, maybe it's better to just set these to green and red directly instead of linking them to some other groups? That may achieve the intended purpose for this PR better than relying on assumptions about colorschemes.
FWIW, we can update bundled colorschemes and set greenish/reddish colors to diffAdded/diffRemoved.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Romain Lafourcade
OK, let's rewind a little to the initial issue.
This is how the diff syntax looks in the following environment, which has been more or less the baseline for decades: 16 colors $TERM, default xterm palette, default colorscheme.
That is rather obviously bad, if only because diffRemoved and diffChanged have the same color. Can we do better?
Yes we can, and it is easy to do it without breaking anything: specify diffAdded, diffChanged, and diffRemoved explicitly in $VIMRUNTIME/syntax/syncolor.vim:
- The highlight groups in
difffiles have a nice semantically correct default indefault, - colorscheme authors and users can override them how they want,
- no one's setup is broken.
Now that we have seen the screenshot above, does the crap below still looks like a solution to anyone?
Capture.d.ecran.2024-01-04.a.19.25.14.png (view on web)ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
really? if you are willing to listen, the solution had emphasis many times: Not
fixing something while break something, bram chose compatible more than feature, and was Not so easy to break the useddefault, but you chose the work style like neovim.
Wow, thank you for this constructive feedback
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Yes we can, and it is easy to do it without breaking anything: specify diffAdded, diffChanged, and diffRemoved explicitly in $VIMRUNTIME/syntax/syncolor.vim
So can you prepare a PR for this then?
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Romain Lafourcade
Yes, I can do it tonight (Europe/Paris time).
red for diffRemoved and darkgreen for diffAdded are a no-brainer, but feel free to suggest alternatives to blue for diffChanged in the mean time.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
--
shane.xb.qian
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gary Johnson
> Yeah also true. Whatever you do, you will choice wrong š
commit and looked at the diff section. It is butt ugly and
unreadable.
I use Vim to look at diffs daily, mostly in diff mode but often to
look at patches. I have never run into any problems with the
previous defaults.
If some new application needs some new diff color thingy then add
something, but retain the existing defaults and don't make drastic
changes to defaults that had been working fine.
Regards,
Gary
vim-dev ML
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Yeah, I'll try to be more careful next time. Sorry for this. Hopefully we can improve this again.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Evgeni Chasnovski
I have never run into any problems with the
previous defaults.
... but retain the existing defaults and don't make drastic
changes to defaults that had been working fine.
Having working fine for you doesn't necessarily mean that it is a proper choice of default links. Having default highlighting with quite low probability of being green-ish and red-ish is a bit odd based on establied UI colors.
If some new application needs some new diff color thingy then add
something...
Same goes for users which care about their diff colors.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
I have never run into any problems with the
previous defaults.
... but retain the existing defaults and don't make drastic
changes to defaults that had been working fine.Having working fine for you doesn't necessarily mean that it is a proper choice of default links.
crap? it had been proper 20 years (#13700 (comment)), now you are saying it's not proper? where were those tens of thousands of Vimer in that 20 years hiding, no one complain? // since IF the work looked like was basing on 'complain'.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Clason
You are welcome to disagree, but please stay civil.
where were those tens of thousands of Vimer in that 20 years hiding
Probably just setting these groups explicitly in their (personal and plugin) colorschemes.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
what is 'misleading'? that is the misleading.
if you thought it's not proper, should complain it at the beginning (like me "complain" vim9 script #13769) or when it still had chance, though i know it is still difficult, but later 20 years and after 3 major (v7/v8/v9) release, you want to break whole/most Vimer used way?
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Clason
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
please help official vim keep consistent, thanks.
// as for this ticket itself, #13776 (comment) had answer it.
// please revert this change, or @romainl #13776 (comment)
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
@Shane-XB-Qian can you please calm down a bit?
Please let's all stay civil, it doesn't help anybody to heat up this dicussion further. Let's see if we can fix this problem. I see @romainl already working on a fix. So let's be nice, test the suggested changes (once they are ready) give feedback and if it's fine, I'll merge it.
Hope this is plan.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Romain Lafourcade
@chrisbra I just submitted my idea of a fix for a) the original issue, and b) its original fix.
ā
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
@chrisbra i am cool (tempeture 7ā) , just worried you were mislead-ed.
@romainl thanks.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.