New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@zhou13
Contributor

zhou13 commented Jun 18, 2018

This implements the compromise mentioned by @justinmk in #7383

  • low-priority CursorLine if only background is set (foreground is not set)
  • high-priority ("same as Vim" priority) CursorLine if foreground and background are set

@zhou13 zhou13 force-pushed the zhou13:cursorline-priority branch 2 times, most recently from cebed1e to d9afb2e Jun 18, 2018

@janlazo

This comment has been minimized.

Contributor

janlazo commented Jun 18, 2018

Are there any vim patches that resolve this?

@marvim marvim added the RDY label Jun 18, 2018

@zhou13

This comment has been minimized.

Contributor

zhou13 commented Jun 18, 2018

AFAIK, No.

@zhou13 zhou13 force-pushed the zhou13:cursorline-priority branch from d9afb2e to 22c1a1f Jun 18, 2018

@zhou13 zhou13 changed the title from [RDY] lower the priority of cursorline, close #7383 to [RDY] lower the priority of cursorline, close #7383 and #6380 Jun 18, 2018

@zhou13 zhou13 force-pushed the zhou13:cursorline-priority branch from 22c1a1f to 5bd01ee Jun 18, 2018

@zhou13 zhou13 changed the title from [RDY] lower the priority of cursorline, close #7383 and #6380 to [RDY] lower the priority of cursorline, close #7383 and #7715 Jun 18, 2018

// set)
// * high-priority ("same as Vim" priority) CursorLine if foreground and
// background are set
if (aep->rgb_fg_color == -1 && aep->cterm_fg_color == 0) {

This comment has been minimized.

@justinmk

justinmk Jun 20, 2018

Member

Should this boolean result be stored in a local variable outside of the for loop, to avoid redundant calls to syn_cterm_attr2entry(cul_attr)?

This comment has been minimized.

@zhou13

zhou13 Jun 24, 2018

Contributor

syn_cterm_attr2entry is only called once per win_line and it is a very simple function that returns an address. It is not in a loop, as least in function win_line. I don't think there will be performance issue here.

// * low-priority CursorLine if only background is set (foreground is not
// set)
// * high-priority ("same as Vim" priority) CursorLine if foreground and
// background are set

This comment has been minimized.

@justinmk

justinmk Jun 20, 2018

Member

What if only foreground is set ?

This comment has been minimized.

@zhou13

zhou13 Jun 24, 2018

Contributor

Fixed the doc.

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 20, 2018

@zhou13 thanks for revisiting this , it will be a nice improvement!

@janlazo this is a case where Nvim diverges from Vim.

@zhou13 zhou13 force-pushed the zhou13:cursorline-priority branch from 5bd01ee to 338903c Jun 24, 2018

@zhou13 zhou13 force-pushed the zhou13:cursorline-priority branch from 338903c to 887bfae Jun 24, 2018

justinmk added a commit that referenced this pull request Jun 28, 2018

highlight: high-priority CursorLine if fg is set. #8578
closes #7383
closes #7715

This implements the compromise described in #7383:
* low-priority CursorLine if foreground is not set
* high-priority ("same as Vim" priority) CursorLine if foreground is set

ref d1874ab
ref 56eda2a

@justinmk justinmk changed the title from [RDY] lower the priority of cursorline, close #7383 and #7715 to highlight: high-priority CursorLine if fg is set. Jun 28, 2018

@justinmk justinmk changed the title from highlight: high-priority CursorLine if fg is set. to highlight: high-priority CursorLine if fg is set Jun 28, 2018

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 28, 2018

Merged. Thanks @zhou13 !

@justinmk justinmk closed this Jun 28, 2018

@justinmk justinmk removed the RDY label Jun 28, 2018

justinmk added a commit to justinmk/neovim that referenced this pull request Jul 1, 2018

justinmk added a commit that referenced this pull request Jul 18, 2018

NVIM v0.3.1
FEATURES:
07499a8 #8709 man.vim: C highlighting for EXAMPLES section
07f82ad #8699 TUI: urxvt: also send xterm focus-reporting seqs
40911e4 #8616 API: emit nvim_buf_lines_event from :terminal
c46997a #8546 fillchars: Add "eob" flag

FIXES:
74d19f6 #8576 startup: avoid blank stdin buffer if other files were opened
4874214 #8737 Only waitpid() for processes that we care about
cd6e7e8 #8743 Check all child processes for exit in SIGCHLD handler
c230ef2 #8746 channel.c: Prevent channel_destroy_early() from freeing uninitialized rpc stuff
0ed8b12 #8681 transstr_buf: fix length comparison
d241f27 #8708 TUI: Fix standout mode
9afed40 #8698 man.vim: fix for mandoc
e889640 #8682 provider/node: npm --loglevel silent
1cbc830 #8613 API: nvim_win_set_cursor: set curswant
bf6048e #8628 checkhealth: Python: fix VIRTUAL_ENV check
3cc3506 #8528 checkhealth: node.js: also search yarn

CHANGES:
b751449 #8619 defaults: shortmess+=F
1248178 #8578 highlight: high-priority CursorLine if fg is set.
01570f1 #8726 terminal: handle &confirm and :confirm on unloading
56065bb #8721 screen: truncate showmode messages
bf2460e #7551 buffer: fix copying :setlocal options
c1c14fa #8520 Ex mode: always "improved" (gQ)
050f397 #7992 options: remove 'maxcombine` option (always 6)

INTERNAL:
463da84 #7992 screen: use UTF-8 representation

UtkarshMe added a commit to UtkarshMe/neovim that referenced this pull request Jul 28, 2018

NVIM v0.3.1
FEATURES:
07499a8 neovim#8709 man.vim: C highlighting for EXAMPLES section
07f82ad neovim#8699 TUI: urxvt: also send xterm focus-reporting seqs
40911e4 neovim#8616 API: emit nvim_buf_lines_event from :terminal
c46997a neovim#8546 fillchars: Add "eob" flag

FIXES:
74d19f6 neovim#8576 startup: avoid blank stdin buffer if other files were opened
4874214 neovim#8737 Only waitpid() for processes that we care about
cd6e7e8 neovim#8743 Check all child processes for exit in SIGCHLD handler
c230ef2 neovim#8746 channel.c: Prevent channel_destroy_early() from freeing uninitialized rpc stuff
0ed8b12 neovim#8681 transstr_buf: fix length comparison
d241f27 neovim#8708 TUI: Fix standout mode
9afed40 neovim#8698 man.vim: fix for mandoc
e889640 neovim#8682 provider/node: npm --loglevel silent
1cbc830 neovim#8613 API: nvim_win_set_cursor: set curswant
bf6048e neovim#8628 checkhealth: Python: fix VIRTUAL_ENV check
3cc3506 neovim#8528 checkhealth: node.js: also search yarn

CHANGES:
b751449 neovim#8619 defaults: shortmess+=F
1248178 neovim#8578 highlight: high-priority CursorLine if fg is set.
01570f1 neovim#8726 terminal: handle &confirm and :confirm on unloading
56065bb neovim#8721 screen: truncate showmode messages
bf2460e neovim#7551 buffer: fix copying :setlocal options
c1c14fa neovim#8520 Ex mode: always "improved" (gQ)
050f397 neovim#7992 options: remove 'maxcombine` option (always 6)

INTERNAL:
463da84 neovim#7992 screen: use UTF-8 representation
@justinmk

This comment has been minimized.

Member

justinmk commented Sep 9, 2018

@zhou13 With these changes I now think it makes sense to also override Diff* highlights.

And I think my previous comment was mistaken, usually "diff" highlights background stuff and almost always needs to set the foreground, otherwise it's hard to see the text. Meanwhile with set cursorline does not set foreground, the diff text is hard to see when CursorLine overlays it.

So I think CursorLine should be lower-priority than Diff* text unless CursorLine has background and foreground set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment