[vim/vim] feat: make it possible to restore default highlighting (#6956)
Maxim Kim
problem: users are unable to restore default highlighting
solution: treat cleared highlight group as "has no hl-settings"
Fixes #4405
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/6956
Commit Summary
- feat: make it possible to restore default highlighting
File Changes
- M src/highlight.c (3)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
codecov[bot]
Codecov Report
Merging #6956 into master will increase coverage by
0.03%.
The diff coverage is100.00%.
@@ Coverage Diff @@ ## master #6956 +/- ## ========================================== + Coverage 88.55% 88.58% +0.03% ========================================== Files 148 148 Lines 161412 161413 +1 ========================================== + Hits 142936 142993 +57 + Misses 18476 18420 -56
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/highlight.c | 90.31% <100.00%> (+<0.01%) |
⬆️ |
| src/ex_getln.c | 91.34% <0.00%> (-0.35%) |
⬇️ |
| src/getchar.c | 86.20% <0.00%> (-0.30%) |
⬇️ |
| src/gui_gtk_x11.c | 58.73% <0.00%> (+0.09%) |
⬆️ |
| src/sign.c | 94.94% <0.00%> (+0.17%) |
⬆️ |
| src/if_xcmdsrv.c | 88.73% <0.00%> (+0.17%) |
⬆️ |
| src/drawline.c | 84.91% <0.00%> (+0.20%) |
⬆️ |
| src/gui_gtk.c | 31.67% <0.00%> (+0.27%) |
⬆️ |
| src/term.c | 82.95% <0.00%> (+0.43%) |
⬆️ |
| src/profiler.c | 93.03% <0.00%> (+0.55%) |
⬆️ |
| ... and 4 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 9b123d8...a673b34. Read the comment docs.
Bram Moolenaar
I'm wondering: would it also work to remove the SG_LINK flag from sg_set in highlight_clear() ?
Since you know what you are fixing, can you also add a test for that?
Antony Scriven
In case I wasn't clear before, I'm wondering if could you save sg_link
in do_highlight() when doing ":hi def link", and then restore it in
highlight_clear(). Or would there be unintended consequences?
I've not had any highlighting issues since trying it out (altering links
and then changing colorschemes), but I'm not a heavy user of syntax
highlighting and I'm not sure what the implications would be.
--Antony
Maxim Kim
Since you know what you are fixing, can you also add a test for that?
Test has been added.
I'm wondering: would it also work to remove the SG_LINK flag from sg_set in highlight_clear() ?
Looks like it also solves the issue.
Removed check for cleared in hl_has_settings
Added HL_TABLE()[idx].sg_set = 0; to highlight_clear
Should I make this change instead?
Maxim Kim
Should I make this change instead?
It looks like this flag is reset outside of the highlight_clear:
https://github.com/vim/vim/blob/8b51b7f0f17af149a8ce76e805050977857f9e50/src/highlight.c#L882-L888
But I am not sure if there is a combination of forceit==1 and init==1 and doclear == 0 we can call from vimscript, as was revealed by @andymass: #4405 (comment)
Maxim Kim
Antony Scriven
> среда, 16 сентября 2020 г. в 03:38:37 UTC+3, Antony Scriven:
>
> > In case I wasn't clear before, I'm wondering if could you save sg_link
> > > in do_highlight() when doing ":hi def link", and then restore it in
> > > highlight_clear(). Or would there be unintended consequences?
> > >
> > > I've not had any highlighting issues since trying it out (altering links
> > > and then changing colorschemes), but I'm not a heavy user of syntax
> > > highlighting and I'm not sure what the implications would be.
> > >
> > > --Antony
> > >
again. Sorry.
> Yes, result is the same and it also passes the test I have added.
> I think that proper check for cleared in 'hl_has_settings' is simpler
> though.
require that syntax is reloaded. This seems to be welcome behaviour but
I can't decide whether this is an intrusive change or not. --Antony
Maxim Kim
> I think that proper check for cleared in 'hl_has_settings' is simpler
> though.
Yeah I know. I'm only mentioning this alternative because it doesn't
require that syntax is reloaded. This seems to be welcome behaviour but
I can't decide whether this is an intrusive change or not. --Antony
Maxim Kim
Maxim Kim
Bram, I have actually just pushed a new variant, like a seconds before you closed the PR :)
It might be better in the end, but it is up to you to decide.
This approach is from Antony Scriven @adscriven:
- default hi-links are saved on
hi def- and restored in
hi clearHaving this user doesn't have to do neither
syntax onnorsyntax enableto be able to apply default links on a cleared highlight groups.Thus there will be no need to update old colorschemes to make it work.
Andy Massimino
@habamax does Bram's commit resolve the issue I pointed out in that comment? (If so can you briefly explain how?)
Maxim Kim
@andymass there is a code path checking if default link is being set for a highlight group and if it has any settings.
// Return if "default" was used and the group already has settings.
if (dodefault && hl_has_settings(idx, TRUE))
return;
That function was always false for a cleared highlight group thus preventing :hi def GroupName
So with the patch cleared hi group is treated as the group without hl-settings.
Bram Moolenaar
> Bram, I have actually just pushed a new variant, like a seconds before you closed the PR :)
>
> It might be better in the end, but it is up to you to decide.
% vim --clean
:hi
" Shows Character xxx links to Constant
:hi link Character Number
:hi
" Shows Character xxx links to Number
:hi clear
:hi
" Shows Character xxx links to Constant
--
Did Adam and Eve have navels?
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Antony Scriven
> [...]
>
> This would restore default links? Is that actually needed?
>
E.g. syntax/html.vim links htmlTagName to Statement, but a fancy
colorscheme may choose to give htmlTagName specific colours. This sort
of thing happens frequently.
If you then switch from one of these fancy colorschemes to one that uses
only the standard groups, htmlTagName won't be highlighted with
Statement colours as you might expect, because the link has gone. It
looks like the second colorscheme is broken.
The idea was that if default links as well as default colours are
restored on ":hi clear", then colorschemes will be working with a more
consistent initial state, and produce the colours that the user expects
to see.
--Antony
Bram Moolenaar
> On Sep 16, Bram Moolenaar wrote:
>
> > [...]
> >
> > This would restore default links? Is that actually needed?
> >
> > [...]
>
> E.g. syntax/html.vim links htmlTagName to Statement, but a fancy
> colorscheme may choose to give htmlTagName specific colours. This sort
> of thing happens frequently.
>
> If you then switch from one of these fancy colorschemes to one that uses
> only the standard groups, htmlTagName won't be highlighted with
> Statement colours as you might expect, because the link has gone. It
> looks like the second colorscheme is broken.
>
> The idea was that if default links as well as default colours are
> restored on ":hi clear", then colorschemes will be working with a more
> consistent initial state, and produce the colours that the user expects
> to see.
already included solution? The test would also need updating.
--
The Characters and incidents portrayed and the names used are fictitious and
any similarity to the names, characters, or history of any person is entirely
accidental and unintentional.
Signed RICHARD M. NIXON
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Maxim Kim
ср, 16 сент. 2020 г., 22:50 Bram Moolenaar <notifi...@github.com>:
> Reply to this email directly, view it on GitHub
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABZKFXGDL23NNLGFWYKTG3SGEJI3ANCNFSM4RMVTWQA>
> .
Maxim Kim
It is in an old PR as a new commit, I can make it as a separate PR.
ср, 16 сент. 2020 г., 22:50 Bram Moolenaar <notifi...@github.com>:
> > E.g. syntax/html.vim links htmlTagName to Statement, but a fancy
> > colorscheme may choose to give htmlTagName specific colours. This sort
> > of thing happens frequently.
> >
> > If you then switch from one of these fancy colorschemes to one that uses
> > only the standard groups, htmlTagName won't be highlighted with
> > Statement colours as you might expect, because the link has gone. It
> > looks like the second colorscheme is broken.
> >
> > The idea was that if default links as well as default colours are
> > restored on ":hi clear", then colorschemes will be working with a more
> > consistent initial state, and produce the colours that the user expects
> > to see.
>
> Can you make a new pull request, so we can see the difference with the
> already included solution? The test would also need updating.
From: Maxim Kim <hab...@gmail.com>
Date: Wed, 16 Sep 2020 16:35:11 +0300
Subject: [PATCH] feat: save and restore default links
This approach is from Antony Scriven @adscriven:
* and restored in `hi clear`
Having this user doesn't have to do neither `syntax on` nor `syntax
enable` to be able to apply default links on a cleared highlight groups.
Thus there will be no need to update old colorschemes to make it work.
runtime/doc/syntax.txt | 3 ++-
src/highlight.c | 9 +++++++--
src/testdir/test_highlight.vim | 19 +++++--------------
3 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/runtime/doc/syntax.txt b/runtime/doc/syntax.txt
index 9c5656719..f7de54a55 100644
--- a/runtime/doc/syntax.txt
+++ b/runtime/doc/syntax.txt
@@ -4808,7 +4808,8 @@ in their own color.
:hi[ghlight] clear Reset all highlighting to the defaults. Removes all
highlighting for groups added by the user!
Uses the current value of 'background' to decide which
- default colors to use.
+ default colors to use. Default highlight links are
+ restored. |:hi-link|
:hi[ghlight] clear {group-name}
:hi[ghlight] {group-name} NONE
diff --git a/src/highlight.c b/src/highlight.c
index fe22da0f8..7e325afde 100644
--- a/src/highlight.c
+++ b/src/highlight.c
@@ -73,6 +73,7 @@ typedef struct
char_u *sg_gui_sp_name;// GUI special color name
#endif
int sg_link; // link to this highlight group ID
+ int sg_deflink; // default link; restored in highlight_clear()
int sg_set; // combination of SG_* flags
#ifdef FEAT_EVAL
sctx_T sg_script_ctx; // script in which the group was last set
@@ -739,6 +740,9 @@ do_highlight(
else
to_id = syn_check_group(to_start, (int)(to_end - to_start));
+ if (dodefault && (forceit || HL_TABLE()[from_id - 1].sg_deflink == 0))
+ HL_TABLE()[from_id - 1].sg_deflink = to_id;
+
if (from_id > 0 && (!init || HL_TABLE()[from_id - 1].sg_set == 0))
{
/*
@@ -1683,6 +1686,8 @@ highlight_clear(int idx)
HL_TABLE()[idx].sg_gui_attr = 0;
#endif
#ifdef FEAT_EVAL
+ // Restore default link
+ HL_TABLE()[idx].sg_link = HL_TABLE()[idx].sg_deflink;
// Clear the script ID only when there is no link, since that is not
// cleared.
if (HL_TABLE()[idx].sg_link == 0)
diff --git a/src/testdir/test_highlight.vim b/src/testdir/test_highlight.vim
index b8fdf17a8..be96efe79 100644
--- a/src/testdir/test_highlight.vim
+++ b/src/testdir/test_highlight.vim
@@ -796,14 +796,12 @@ func Test_highlight_term_attr()
hi clear
endfunc
-" Test default highlighting is restored
-func Test_highlight_restore_defaults()
+" Test default highlighting links are restored
+func Test_highlight_restore_default_links()
- hi! link TestLink Identifier
- hi! TestHi ctermbg=red
+ hi def link TestLink Identifier
let hlTestLinkPre = HighlightArgs('TestLink')
- let hlTestHiPre = HighlightArgs('TestHi')
" Test colorscheme
hi clear
@@ -811,21 +809,14 @@ func Test_highlight_restore_defaults()
syntax reset
endif
let g:colors_name = 'test'
- hi! link TestLink ErrorMsg
- hi! TestHi ctermbg=green
+ hi link TestLink ErrorMsg
- " Restore default highlighting
+ " Default links should be restored
colorscheme default
- syntax on
- " 'default' should work no matter if highlight group was cleared
- hi def link TestLink Identifier
- hi def TestHi ctermbg=red
let hlTestLinkPost = HighlightArgs('TestLink')
- let hlTestHiPost = HighlightArgs('TestHi')
call assert_equal(hlTestLinkPre, hlTestLinkPost)
- call assert_equal(hlTestHiPre, hlTestHiPost)
hi clear
endfunc
--
2.20.1
Maxim Kim
Shane-XB-Qian
after look over #6970, maybe a silly question but one concern:
if one colorscheme was on, and it set or override one 'hi def', then now issued 'colo default' (had a 'hi clear'), what would happened on that 'hi def', or just supposed that/all 'hi def' would not impact (not existed in) 'colo default' ?
specially 'hi clear', perhaps i would run 'hi clear' and/or specially some vim scripts had 'hi clear' in it, but user did not expect those such kind 'hi def' impact its previous logic/effect.
was it possible to happen, or i missunderstood sth ?
means just wish really only the default 'hi def' which not 3rd-party made ones to be restored after 'hi clear'.
Maxim Kim
after look over #6970, maybe a silly question but one concern:
if one colorscheme was on, and it set or override one 'hi def', then now issued 'colo default' (had a 'hi clear'), what would happened on that 'hi def', or just supposed that/all 'hi def' would not impact (not existed in) 'colo default' ?
if there was no syntax loaded, meaning there was no :hi def GroupName defined, then colorscheme will define it and :hi clear will make it cleared:
Colorscheme redefines
:hi vimOption ctermbg=red
Then you do:
:hi clear
:hi vimOption --> cleared
Before the patch, it would stay cleared and it was impossible to make it default again as :hi def vimOption just didn't work for cleared. You could only override it with :hi vimOption in some other colorscheme.
specially 'hi clear', perhaps i would run 'hi clear' and/or specially some vim scripts had 'hi clear' in it, but user did not expect those such kind 'hi def' impact its previous logic/effect.
was it possible to happen, or i missunderstood sth ?
means just wish really only the default 'hi def' which not 3rd-party made ones to be restored after 'hi clear'.
With the #6970 patch vim also restores default links defined in syntax files, thus if some colorscheme redefines vimOption it would be restored on :hi clear as if it was initially defined in syntax file.
If you check issue description it has all the details: #4405
Shane-XB-Qian
Before the patch, it would stay cleared and it was impossible to make it default again as
:hi def vimOptionjust didn't work for cleared. You could only override it with:hi vimOptionin some other colorscheme.
yes, that is what i am doing for 'restore' default.
With the #6970 patch vim also restores default links defined in syntax files, thus if some colorscheme redefines vimOption it would be restored on
:hi clearas if it was initially defined in syntax file.
actually my concern was - for example:
one 3rd part colorschema had a 'hi! def link difftext foo' in it, then now, after your patch, whatever 'colo default' or 'hi clear', after one of them, what would happened, would it back to original 'difftext' ? just wish you gave me a confirm answer. :)
Maxim Kim
actually my concern was - for example:
one 3rd part colorschema had a 'hi! def link difftext foo' in it, then now, after your patch, whatever 'colo default' or 'hi clear', after one of them, what would happened, would it back to original 'difftext' ? or still is 'foo' ? since now that 'def link' is 'foo' now.
just wish you gave me a confirm answer. :)
with patches:
hi difftext
output:
DiffText xxx term=reverse cterm=bold ctermbg=9 gui=bold guibg=Red
hi! def link difftext foo
hi difftext
output:
DiffText xxx term=reverse cterm=bold ctermbg=9 gui=bold guibg=Red
links to foo
colo default
hi difftext
output:
DiffText xxx term=reverse cterm=bold ctermbg=9 gui=bold guibg=Red
Shane-XB-Qian
good, looks nice. would try again after/if that patch merged. thx.
Shane-XB-Qian
tried (that pr merged today), so far have not found side-effect, but one maybe a tiny issue: the bg after 'colo default' did not recover to that ft syntax default, then hi/color actually (by eye) was not really same like it was (that ft default).
and #6975 looks found sth, btw.
// anyway, i actually did not count on this to restore default, just wish it would not impact user previous code.