[RDY] hl-Syntax should override cursorline and cursorcolumn #6380

zhou13 · 2017-03-28T01:59:18Z

This patch makes syntax highlight correctly overrides 'cursorline' and 'cursorcolumn'

Before this patch (default json syntax highlights every comment as error):

After this patch:

While nvim -d or "folding" is not affected (This may be good but I don't know exactly why):

tweekmonster · 2017-03-28T02:06:09Z

Does this also preserve the text color for concealed characters when set list is enabled? If so, this gets a big 👍 from me. I have a feeling that some tests are going to fail because of this, though.

zhou13 · 2017-03-28T02:08:48Z

I don't quite understand your concern since it involves "conceal", "list", "cursorline". Could you give me an example?

tweekmonster · 2017-03-28T02:18:28Z

Oh I feel dumb now. You fixed it in #4744. I disabled CursorLine highlighting a long time ago because of how annoying it was and forgot about it. This PR reminded me of the old issue. It never crossed my mind that it was already fixed. 😭 🎉

justinmk · 2017-03-29T23:33:06Z

Just tried this PR (9bc7715). It regresses e.g. the DiffChange highlight. I have this:

hi DiffChange      guifg=white   guibg=#4C4745 ctermfg=255 ctermbg=239 gui=NONE cterm=NONE

But the changed diff lines don't show white foreground anymore.

zhou13 · 2017-03-29T23:59:38Z

This is intended. Do you think DiffChange should override syntax? To fix it, we just need to change HLF_CHD from line_attr to line_attr_prior.

justinmk · 2017-03-30T00:47:34Z

Do you think DiffChange should override syntax?

There wouldn't be any purpose to DiffChange if it gets overridden. If colorscheme or user wants DiffChange foreground color to be "overridden", they can do that by not specifying ctermfg/guifg:

:hi clear DiffChange
:hi DiffChange ctermbg=... guibg=...

If we take that away we've lost flexibility. We don't want to do that for any case where it's already controllable. (So if there are any other cases of this, same principle applies)

zhou13 · 2017-03-30T00:49:29Z

There wouldn't be any purpose to DiffChange if it gets overridden. If colorscheme or user wants DiffChange foreground color to be "overridden", they can do that by not specifying ctermfg/guifg:

Yes. I have already restored the old behavior of Diff*.

zhou13 · 2017-03-31T06:33:50Z

Just did a refactor so that it maintains old nvim -d behavior better.

zhou13 · 2017-05-23T21:08:37Z

Is this patch ready to go?

zhou13 · 2017-09-28T00:14:13Z

@justinmk I fixed some merge conflicts. Is there anything prevent this from being merged?

justinmk · 2017-09-29T06:12:30Z

src/nvim/screen.c

-      && !(wp == curwin && VIsual_active)) {
-    if (line_attr != 0 && !(State & INSERT) && bt_quickfix(wp->w_buffer)
-        && qf_current_entry(wp) == lnum) {
-      line_attr = hl_combine_attr(win_hl_attr(wp, HLF_CUL), line_attr);


Why isn't this branch needed anymore?

The old one basically says if we are in quickfix window, then we use HLF_CUL to complement current line_attr, otherwise it will directly set line_attr to HLF_CUL. Setting line_attr to HLF_CUL is not what we want, since it incorrectly override some highlights under some condition (the logic is complex here)....

In this patch, I split line_attr into line_attr and line_attr_low_priority. Now line_attr_low_priority only takes care of cursorline, and it will be merged into char_attr with lowest priority. For other line_attr, it will still use the old logic. Since line_attr_low_priority already has lowest priority, I believe these lines are not necessary.

justinmk · 2017-09-29T06:22:01Z

src/nvim/screen.c

      } else if (draw_color_col && VCOL_HLC == *color_cols) {
        vcol_save_attr = char_attr;
-        char_attr = hl_combine_attr(char_attr, win_hl_attr(wp, HLF_MC));
+        char_attr = hl_combine_attr(win_hl_attr(wp, HLF_MC), char_attr);


This could be a separate change, right? And it's much lower-risk than the line_attr_low_priority change.

Yes. They can fix the problem for cursorcolumn and colorcolumn.

justinmk · 2017-09-29T06:22:55Z

Looking at this again, this adds numerous decisions that appear fragile and difficult to follow.

A new line_attr_low_priority variable is introduced which is "like line_attr but different".
Several checks are added in various places, spread over 1500 lines of code.

Is there any way to avoid this? Can we make a smaller improvement with a much smaller code change?

zhou13 · 2017-09-29T08:07:47Z

@justinmk I will say this is unlikely. The old vim uses line_attr for a bunch of things that should be treated differently. For example, you mentioned that DiffChange should override syntax highlight, while cursorline should not. I think they need to be split.

Also most changes to checks are minor: just replace line_attr with line_attr | line_attr_low_priority. This pull request only adds one line of code in total...

justinmk · 2017-10-07T14:04:14Z

Merged in #7364

@zhou13 I have to admit the current cursorline behavior for diff-mode is very annoying. I wonder if it makes sense to have cursorline override only the NonText part?

zhou13 · 2017-10-07T16:17:12Z

Could you upload a screenshot to describe your problem?

justinmk · 2017-10-07T16:21:03Z

@zhou13 sorry to confuse, I was referring to the existing behavior, i.e., the behavior I requested we preserve. Nothing changed by this PR.

I think ignoring 'cursorline' for diff highlights completely is probably too much, but if we could enable the 'cursorline' highlight only for non-text, that might be interesting. Not sure. Just a thought for the future.

justinmk · 2017-10-08T15:26:06Z

@zhou13 If you get a chance, it would be valuable to have some test coverage for these changes. I think there's no coverage of 'cursorcolumn' nor 'colorcolumn', and only indirect coverage of 'cursorline'.

zhou13 · 2017-10-08T18:45:26Z

Oh yes. I will work on that.

…

On Sun, Oct 8, 2017, 8:26 AM Justin M. Keyes ***@***.***> wrote: @zhou13 <https://github.com/zhou13> If you get a chance, it would be valuable to have some test coverage for these changes. I think there's no coverage of 'cursorcolumn' nor 'colorcolumn', and only indirect coverage of 'cursorline'. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6380 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjLfoXka7cdEK_8Yzcf9MMdxomiMC0wks5sqOmggaJpZM4MrCy9> .

justinmk · 2017-10-15T18:40:16Z

For reference: this PR was partially reverted, tentative future plan is described in #7383 (comment)

ref neovim#7383 reverts d1874ab ref neovim#6380

ref #7383 reverts d1874ab ref #6380

ref neovim#6380

zhou13 force-pushed the cursorline-priority branch from f794e72 to ea23b04 Compare March 28, 2017 01:59

zhou13 force-pushed the cursorline-priority branch from ea23b04 to e5aafd8 Compare March 28, 2017 03:54

marvim added the RFC label Mar 28, 2017

justinmk added this to the 0.2.1 milestone Mar 30, 2017

zhou13 force-pushed the cursorline-priority branch 5 times, most recently from e5aafd8 to ecd74d4 Compare March 31, 2017 06:31

zhou13 changed the title ~~[RFC] everything should override cursorline and cursorcolumn~~ [RFC] hl-Syntax should override cursorline and cursorcolumn Mar 31, 2017

zhou13 force-pushed the cursorline-priority branch 3 times, most recently from 7ece468 to 5ae213c Compare March 31, 2017 08:34

zhou13 force-pushed the cursorline-priority branch from 5ae213c to 53ada62 Compare May 23, 2017 20:13

dylnmc mentioned this pull request Jul 17, 2017

hightlight whole line on hightlighted Neovim health check text makes it disappear nordtheme/vim#31

Closed

arcticicestudio mentioned this pull request Jul 17, 2017

Neovim health check highlight status groups support nordtheme/vim#42

Merged

zhou13 force-pushed the cursorline-priority branch from 53ada62 to ef6dda1 Compare September 28, 2017 00:11

zhou13 changed the title ~~[RFC] hl-Syntax should override cursorline and cursorcolumn~~ [RDY] hl-Syntax should override cursorline and cursorcolumn Sep 28, 2017

zhou13 force-pushed the cursorline-priority branch from ef6dda1 to 95134d5 Compare September 28, 2017 00:16

marvim removed the RFC label Sep 28, 2017

justinmk reviewed Sep 29, 2017

View reviewed changes

justinmk mentioned this pull request Oct 7, 2017

hl-Syntax can override 'cursorline', 'cursorcolumn' #7364

Merged

justinmk closed this Oct 7, 2017

justinmk added syntax regex syntax or non-regex parsing, lpeg, grammars and removed RDY labels Oct 7, 2017

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 7, 2017

syntax: 'cursorcolumn', 'colorcolumn': low priority neovim#6380

7a832c3

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 7, 2017

syntax: 'cursorline': low priority neovim#6380

ba118c9

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 7, 2017

syntax: 'cursorline': low priority neovim#6380

d1874ab

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 15, 2017

syntax: 'cursorline': revert priority change

406bc62

ref neovim#7383 reverts d1874ab ref neovim#6380

justinmk mentioned this pull request Oct 15, 2017

syntax: 'cursorline': revert priority change #7400

Merged

justinmk added a commit that referenced this pull request Oct 16, 2017

syntax: 'cursorline': revert priority change (#7400)

56eda2a

ref #7383 reverts d1874ab ref #6380

justinmk mentioned this pull request Dec 10, 2017

cursorline CTERM highlight blocks set spell highlight #7715

Closed

zhou13 added a commit to zhou13/neovim that referenced this pull request Jun 18, 2018

lower the priority of cursorline, close neovim#7383 and neovim#6380

22c1a1f

justinmk mentioned this pull request Sep 9, 2018

highlight: high-priority CursorLine if fg is set #8578

Closed

justinmk added a commit to justinmk/neovim that referenced this pull request Sep 21, 2018

diff/highlight: do not overlay low-priority CursorLine

a9a320f

ref neovim#6380

justinmk mentioned this pull request Sep 21, 2018

diff/highlight: diminish low-priority CursorLine #9028

Merged

1 task

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 31, 2018

diff/highlight: do not overlay low-priority CursorLine

be9de88

ref neovim#6380

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 26, 2018

diff/highlight: do not overlay low-priority CursorLine

1626807

ref neovim#6380

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 26, 2018

diff/highlight: do not overlay low-priority CursorLine

9c07645

ref neovim#6380

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 27, 2018

diff/highlight: do not overlay low-priority CursorLine

3283db4

ref neovim#6380

zeertzjq mentioned this pull request Apr 15, 2024

fix(highlight): combine line attributes before column attributes #28345

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] hl-Syntax should override cursorline and cursorcolumn #6380

[RDY] hl-Syntax should override cursorline and cursorcolumn #6380

zhou13 commented Mar 28, 2017

tweekmonster commented Mar 28, 2017

zhou13 commented Mar 28, 2017

tweekmonster commented Mar 28, 2017

justinmk commented Mar 29, 2017

zhou13 commented Mar 29, 2017 •

edited

justinmk commented Mar 30, 2017 •

edited

zhou13 commented Mar 30, 2017

zhou13 commented Mar 31, 2017

zhou13 commented May 23, 2017

zhou13 commented Sep 28, 2017 •

edited

justinmk Sep 29, 2017

zhou13 Sep 29, 2017

justinmk Sep 29, 2017

zhou13 Sep 29, 2017

justinmk commented Sep 29, 2017 •

edited

zhou13 commented Sep 29, 2017 •

edited

justinmk commented Oct 7, 2017 •

edited

zhou13 commented Oct 7, 2017

justinmk commented Oct 7, 2017 •

edited

justinmk commented Oct 8, 2017

zhou13 commented Oct 8, 2017 via email

justinmk commented Oct 15, 2017

[RDY] hl-Syntax should override cursorline and cursorcolumn #6380

[RDY] hl-Syntax should override cursorline and cursorcolumn #6380

Conversation

zhou13 commented Mar 28, 2017

tweekmonster commented Mar 28, 2017

zhou13 commented Mar 28, 2017

tweekmonster commented Mar 28, 2017

justinmk commented Mar 29, 2017

zhou13 commented Mar 29, 2017 • edited

justinmk commented Mar 30, 2017 • edited

zhou13 commented Mar 30, 2017

zhou13 commented Mar 31, 2017

zhou13 commented May 23, 2017

zhou13 commented Sep 28, 2017 • edited

justinmk Sep 29, 2017

Choose a reason for hiding this comment

zhou13 Sep 29, 2017

Choose a reason for hiding this comment

justinmk Sep 29, 2017

Choose a reason for hiding this comment

zhou13 Sep 29, 2017

Choose a reason for hiding this comment

justinmk commented Sep 29, 2017 • edited

zhou13 commented Sep 29, 2017 • edited

justinmk commented Oct 7, 2017 • edited

zhou13 commented Oct 7, 2017

justinmk commented Oct 7, 2017 • edited

justinmk commented Oct 8, 2017

zhou13 commented Oct 8, 2017 via email

justinmk commented Oct 15, 2017

zhou13 commented Mar 29, 2017 •

edited

justinmk commented Mar 30, 2017 •

edited

zhou13 commented Sep 28, 2017 •

edited

justinmk commented Sep 29, 2017 •

edited

zhou13 commented Sep 29, 2017 •

edited

justinmk commented Oct 7, 2017 •

edited

justinmk commented Oct 7, 2017 •

edited