[vim/vim] Cursor screenline public (#4933)
Christian Brabandt
As requested in #4693 (comment) amend the cursorlineopt setting and allow a 'screenline' value. The value both has been aliased to the option number,line which is now the new default. To set the value do :set culopt=screenline or :set culopt=screenline,number. Setting both,screenline or line,screenline will be an error.
I have been conscious in adjusting the win_line() function. It needed quite a bit additional code to handle all different special cases, like when 'list' is set or when a multibyte character does not fit on the screen and gets wrapped and handling are different drawing states. Also, it might cause a small performance penalty, because the caching using w_last_cursorline does not work, since we are only highlighting parts of the file.
BTW: I noticed that the default cursorline highlighting does also underline the sbr setting (which is a bit strange, since it does not actually contain to the line content). I almost fixed it, but a test started to fail, so I left it as is (and is only a minor annoyance).
To that end, I added quite a few screen tests, that should make sure that most of the special cases are correctly handled.
I think most of the special cases are explained in the corresponding commits. I left those commits there and did not squash them to allow for easier review, but I can squash if needed.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/4933
Commit Summary
- Allow screenline as value for cursorlineopt
- Initial implementation for screenline highlighting
- Force cursorline updating in main_loop and change.c
- Correctly reset highlighting after wrapping
- Only calculate margin columns once
- Apply screenline cursorline to special chars
- Re-apply screenline cursorline highlight after wrapped multibyte char
- refactor parsing of culopt option
- Make use of cursorlineopt flags in redraw logic
- cursorline highlighting: add tests
- Document changed values for the cursorlineopt setting
File Changes
- M runtime/doc/options.txt (14)
- M src/change.c (5)
- M src/main.c (15)
- M src/option.c (65)
- M src/option.h (5)
- M src/optiondefs.h (3)
- M src/screen.c (226)
- M src/structs.h (2)
- A src/testdir/dumps/Test_Xcursorline_1.dump (20)
- A src/testdir/dumps/Test_Xcursorline_10.dump (20)
- A src/testdir/dumps/Test_Xcursorline_11.dump (20)
- A src/testdir/dumps/Test_Xcursorline_12.dump (20)
- A src/testdir/dumps/Test_Xcursorline_13.dump (20)
- A src/testdir/dumps/Test_Xcursorline_14.dump (20)
- A src/testdir/dumps/Test_Xcursorline_15.dump (20)
- A src/testdir/dumps/Test_Xcursorline_16.dump (20)
- A src/testdir/dumps/Test_Xcursorline_17.dump (20)
- A src/testdir/dumps/Test_Xcursorline_18.dump (20)
- A src/testdir/dumps/Test_Xcursorline_2.dump (20)
- A src/testdir/dumps/Test_Xcursorline_3.dump (20)
- A src/testdir/dumps/Test_Xcursorline_4.dump (20)
- A src/testdir/dumps/Test_Xcursorline_5.dump (20)
- A src/testdir/dumps/Test_Xcursorline_6.dump (20)
- A src/testdir/dumps/Test_Xcursorline_7.dump (20)
- A src/testdir/dumps/Test_Xcursorline_8.dump (20)
- A src/testdir/dumps/Test_Xcursorline_9.dump (20)
- M src/testdir/gen_opt_test.vim (2)
- M src/testdir/test_cursorline.vim (94)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Dominique Pellé
@dpelle commented on this pull request.
In src/option.c:
> @@ -9515,3 +9522,55 @@ get_winbuf_options(int bufopt)
return d;
}
#endif
+
+#ifdef FEAT_SYN_HL
+/*
+ * This is called when 'culopt' is change
is change → is changed
> CursorLineNr |hl-CursorLineNr|. - "both" Highlight as both "line" and "number" are set. + Special value: + "both" Alias for the values "line,number".
"both" is a strange name, now that 'culopt' has more than 2 options. But I understand it's needed for backward compatibility.
In src/screen.c:
> @@ -158,6 +158,9 @@ static void win_redr_custom(win_T *wp, int draw_ruler); #ifdef FEAT_CMDL_INFO static void win_redr_ruler(win_T *wp, int always, int ignore_pum); #endif +#ifdef FEAT_SYN_HL +static void margin_columns_win(win_T *wp, int *lmargin, int *rmargin); + #endif
Indentation is wrong here.
In src/screen.c:
> @@ -5127,6 +5233,11 @@ win_line(
{
n_attr = n_extra + 1;
extra_attr = HL_ATTR(HLF_8);
+ saved_extra_attr = extra_attr;
+#ifdef FEAT_SYN_HL
+ if (need_cul_screenline)
+ extra_attr = hl_combine_attr(extra_attr, HL_ATTR(HLF_CUL));
+ #endif
Indentation not aligned with #ifndef
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Codecov
Codecov Report
Merging #4933 into master will decrease coverage by
0.05%.
The diff coverage is82.05%.
@@ Coverage Diff @@ ## master #4933 +/- ## ========================================== - Coverage 81.44% 81.39% -0.06% ========================================== Files 126 126 Lines 146272 146381 +109 ========================================== + Hits 119134 119143 +9 - Misses 27138 27238 +100
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/main.c | 72.37% <100%> (-0.08%) |
⬇️ |
| src/change.c | 79.44% <100%> (+0.02%) |
⬆️ |
| src/screen.c | 82.91% <77.77%> (+0.16%) |
⬆️ |
| src/option.c | 82.68% <93.1%> (+0.09%) |
⬆️ |
| src/libvterm/src/vterm_internal.h | 50% <0%> (-50%) |
⬇️ |
| src/libvterm/src/rect.h | 79.31% <0%> (-17.25%) |
⬇️ |
| src/libvterm/include/vterm.h | 25% <0%> (-12.5%) |
⬇️ |
| src/libvterm/src/mouse.c | 91.07% <0%> (-5.36%) |
⬇️ |
| src/libvterm/src/unicode.c | 84.09% <0%> (-5.04%) |
⬇️ |
| src/libvterm/src/encoding.c | 72.91% <0%> (-3.13%) |
⬇️ |
| ... and 29 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 57da698...2674dde. Read the comment docs.
—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
Christian wrote:
> As requested in
> https://github.com/vim/vim/pull/4693#issuecomment-529642042 amend the
> cursorlineopt setting and allow a 'screenline' value. The
> value `both` has been aliased to the option `number,line` which is now
> the new default. To set the value do `:set culopt=screenline` or `:set
> `line,screenline` will be an error.
>
> I have been conscious in adjusting the `win_line()` function. It
> needed quite a bit additional code to handle all different special
> does not fit on the screen and gets wrapped and handling are different
> drawing states. Also, it might cause a small performance penalty,
> because the caching using `w_last_cursorline` does not work, since we
> are only highlighting parts of the file.
very wrong. Try editing src/main.c with:
hi CursorLine cterm=underline
With 'cursorlineopt' set to "both" the number column isn't highlighted.
With set to "screenline" lots of highlighting is missing.
--
If VIM were a woman, I'd marry her. Slim, organized, helpful
and beautiful; what's not to like? --David A. Rogers
/// 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 ///
Bram Moolenaar
> Christian wrote:
>
> > As requested in
> > https://github.com/vim/vim/pull/4693#issuecomment-529642042 amend the
> > cursorlineopt setting and allow a 'screenline' value. The
> > value `both` has been aliased to the option `number,line` which is now
> > the new default. To set the value do `:set culopt=screenline` or `:set
> > culopt=screenline,number`. Setting `both,screenline` or
> > `line,screenline` will be an error.
> >
> > I have been conscious in adjusting the `win_line()` function. It
> > needed quite a bit additional code to handle all different special
> > cases, like when 'list' is set or when a multibyte character
> > does not fit on the screen and gets wrapped and handling are different
> > drawing states. Also, it might cause a small performance penalty,
> > because the caching using `w_last_cursorline` does not work, since we
> > are only highlighting parts of the file.
>
> I was going to include this, but with a simple test the highlighting is
> very wrong. Try editing src/main.c with:
> hi CursorLine cterm=underline
>
> With 'cursorlineopt' set to "both" the number column isn't highlighted.
> With set to "screenline" lots of highlighting is missing.
underline attribute. So th problem is only when using "screenline".
--
Did you hear about the new 3 million dollar West Virginia State Lottery?
The winner gets 3 dollars a year for a million years.
Christian Brabandt
On Sa, 14 Sep 2019, Bram Moolenaar wrote:
> Oh wait, the default CursorLineNr highlight is wrong, it is missing
> the underline attribute. So th problem is only when using "screenline".
attribute. What exactly is wrong?
Best,
Christian
--
Heirate oder heirate nicht, du wirst beides bereuen.
-- Søren Kierkegaard
Bram Moolenaar
Christian wrote:
> On Sa, 14 Sep 2019, Bram Moolenaar wrote:
> > Oh wait, the default CursorLineNr highlight is wrong, it is missing
> > the underline attribute. So th problem is only when using "screenline".
>
> Hm? Not sure I understand. I did not change any highlighting specific
> attribute. What exactly is wrong?
the code by using line_attr. Now checking if everything still works...
--
hundred-and-one symptoms of being an internet addict:
255. You work for a newspaper and your editor asks you to write an
article about Internet addiction...in the "first person."