Skip to content

Conversation

zhou13
Copy link
Contributor

@zhou13 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 cursorline-priority branch 2 times, most recently from cebed1e to d9afb2e Compare June 18, 2018 01:23
@janlazo
Copy link
Contributor

janlazo commented Jun 18, 2018

Are there any vim patches that resolve this?

@marvim marvim added the RDY label Jun 18, 2018
@zhou13
Copy link
Contributor Author

zhou13 commented Jun 18, 2018

AFAIK, No.

@zhou13 zhou13 force-pushed the cursorline-priority branch from d9afb2e to 22c1a1f Compare June 18, 2018 02:13
@zhou13 zhou13 changed the title [RDY] lower the priority of cursorline, close #7383 [RDY] lower the priority of cursorline, close #7383 and #6380 Jun 18, 2018
@zhou13 zhou13 force-pushed the cursorline-priority branch from 22c1a1f to 5bd01ee Compare June 18, 2018 02:18
@zhou13 zhou13 changed the title [RDY] lower the priority of cursorline, close #7383 and #6380 [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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@zhou13 zhou13 Jun 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if only foreground is set ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the doc.

@justinmk
Copy link
Member

@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 cursorline-priority branch from 5bd01ee to 338903c Compare June 24, 2018 03:18
@zhou13 zhou13 force-pushed the cursorline-priority branch from 338903c to 887bfae Compare June 24, 2018 03:19
justinmk pushed a commit that referenced this pull request Jun 28, 2018
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 [RDY] lower the priority of cursorline, close #7383 and #7715 highlight: high-priority CursorLine if fg is set. Jun 28, 2018
@justinmk justinmk changed the title highlight: high-priority CursorLine if fg is set. highlight: high-priority CursorLine if fg is set Jun 28, 2018
@justinmk
Copy link
Member

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
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
coditva pushed a commit to coditva/neovim that referenced this pull request Jul 28, 2018
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
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants