Skip to content
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

Add CurSearch highlight group #10133

Closed
wants to merge 2 commits into from
Closed

Conversation

LemonBoy
Copy link
Contributor

@LemonBoy LemonBoy commented Apr 9, 2022

The aim of CurSearch is to let the user highlight the current match
without having to resort to overcomplicated plugins to approximate this
simple feature.

Closes #2819

The aim of CurSearch is to let the user highlight the current match
without having to resort to overcomplicated plugins to approximate this
simple feature.

Closes vim#2819
@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #10133 (16ffaa0) into master (cbaff5e) will decrease coverage by 0.00%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master   #10133      +/-   ##
==========================================
- Coverage   81.97%   81.96%   -0.01%     
==========================================
  Files         167      167              
  Lines      187824   187845      +21     
  Branches    42355    42360       +5     
==========================================
+ Hits       153964   153976      +12     
- Misses      21514    21524      +10     
+ Partials    12346    12345       -1     
Flag Coverage Δ
huge-clang-none 82.39% <91.66%> (+<0.01%) ⬆️
huge-gcc-none 82.74% <91.66%> (+0.01%) ⬆️
huge-gcc-testgui 81.20% <91.66%> (+<0.01%) ⬆️
huge-gcc-unittests 2.00% <0.00%> (-0.01%) ⬇️
linux 83.97% <91.66%> (+<0.01%) ⬆️
mingw-x64-HUGE 0.00% <0.00%> (ø)
mingw-x64-HUGE-gui 77.18% <91.66%> (-0.01%) ⬇️
windows 75.97% <91.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/highlight.c 82.08% <ø> (ø)
src/normal.c 91.48% <50.00%> (-0.03%) ⬇️
src/match.c 91.91% <100.00%> (+0.69%) ⬆️
src/if_xcmdsrv.c 76.88% <0.00%> (-0.71%) ⬇️
src/terminal.c 78.06% <0.00%> (-0.35%) ⬇️
src/term.c 73.26% <0.00%> (-0.11%) ⬇️
src/syntax.c 79.72% <0.00%> (-0.07%) ⬇️
src/getchar.c 84.58% <0.00%> (-0.07%) ⬇️
src/window.c 88.38% <0.00%> (-0.07%) ⬇️
src/alloc.c 78.98% <0.00%> (-0.06%) ⬇️
... and 10 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 cbaff5e...16ffaa0. Read the comment docs.

@famiu
Copy link

famiu commented Apr 9, 2022

Personally, I think SearchCurrent may be a better name for the highlight compared to CurSearch

@LemonBoy
Copy link
Contributor Author

LemonBoy commented Apr 9, 2022

Personally, I think SearchCurrent may be a better name for the highlight compared to CurSearch

CurSearch IMO fits better in the existing naming scheme, eg. we already have IncSearch.

@brammool
Copy link
Contributor

brammool commented Apr 9, 2022

Well, I can include it, but I do notice some problems.
"n" does not update. Needs using SOME_VALID instead of VALID. Optimally we would only redraw lines with a matching
search pattern.
"?" does not update the highlight. Perhaps other search commands?

It does not update when the cursor is moved, that is OK, but when then using CTRL-L the highlight disappears.
That is a bit strange.

@brammool brammool closed this in a439938 Apr 9, 2022
@brammool
Copy link
Contributor

brammool commented Apr 9, 2022

Hmm, there are more problems. When putting text multiple matches can be highlighted with CurSearch.
Screen updating should be made more clever.

@Shane-XB-Qian
Copy link
Contributor

It does not update when the cursor is moved, that is OK, but when then using CTRL-L the highlight disappears.
That is a bit strange.

i think it is more useful if the highlight not update when cursor moved, it worked like a temp mark, otherwise cursor shape or color had been helped to locate where is the cur-search place.
however as you said, it disappeared after redraw! or ctrl-l, which probably many plugins would do such, so/then such temp hl stay would be Not Stable.
i suggest better to keep that highlight unless moved to the next by 'n/N' jump.

@romainl
Copy link

romainl commented Apr 10, 2022

What is the rationale for linking CurSearch to Search by default when it is supposed to stand out among matches highlighted with Search?

We already have IncSearch that provides the exact same functionality during a search, showing the current match among a sea of matches, so it seems more appropriate than Search.

@clason
Copy link
Contributor

clason commented Apr 10, 2022

Just for reference, there is a parallel PR for Neovim, which had pretty much the same discussion (with the same conclusion).

@brammool
Copy link
Contributor

brammool commented Apr 10, 2022 via email

@romainl
Copy link

romainl commented Apr 10, 2022

By linking CurSearch to Search it is backwards compatible.

What does "backwards compatible" mean in this context? How is IncSearch, which is not behind any #ifdef in the code and is defined the same for both "light" and "dark" background less "backwards compatible" than Search?

IncSearch has a different purpose.

I disagree, especially now that we have <C-g> and <C-t>, which act like "live" nN. When searching for a word with incsearch and hlsearch on, the match that is highlighted with IncSearch is the "current match": the one where the cursor will be moved after we press <CR>.

@zeertzjq
Copy link
Member

I think it means updating Vim does not change the behavior.

@LemonBoy
Copy link
Contributor Author

Well, I can include it, but I do notice some problems. "n" does not update. Needs using SOME_VALID instead of VALID.

That's weird, I tested this patch for a while and didn't notice this problem. Sorry about that.

Optimally we would only redraw lines with a matching search pattern.

I had implemented the redraw using redrawWinline but found too many edge cases to handle wrt multiline matches.

"?" does not update the highlight. Perhaps other search commands?

Indeed, I missed some redraw calls.

It does not update when the cursor is moved, that is OK, but when then using CTRL-L the highlight disappears. That is a bit strange.

Weird, I don't see the highlighting disappear with Ctrl-L nor :redraw.

What is the rationale for linking CurSearch to Search by default when it is supposed to stand out among matches highlighted with Search?

As Braam already said, this change shall not be noticed by the user unless they (or their theme) explicitly define CurSearch.

@brammool
Copy link
Contributor

brammool commented Apr 10, 2022 via email

@brammool
Copy link
Contributor

brammool commented Apr 10, 2022 via email

@lacygoill
Copy link

lacygoill commented Apr 14, 2022

In this snippet, a part of the match is unexpectedly highlighted with Search:

vim9script
&hlsearch = true
highlight! link CurSearch IncSearch
highlight! link Search ErrorMsg
['abcdefg', 'hijk']->setline(1)
setreg('/', 'defg\nhij')
feedkeys('n')

More specifically, the start of the second line:

abcdefg
hijk
^^^

The issue disappears if we add k to the pattern:

setreg('/', 'defg\nhijk')
                      ^

Test:

vim9script
&hlsearch = true
highlight! link Search ErrorMsg
highlight! link CurSearch IncSearch
['abcdefg', 'hijk']->setline(1)
setreg('/', 'defg\nhijk')
feedkeys('n')

More generally, the issue (if it is one) is triggered on the last line of a multiline match, when the latter ends on a column whose index is lower than the one where the first line starts.

brammool added a commit that referenced this pull request Apr 16, 2022
Problem:    CurSearch highlight does not work for multi-line match.
Solution:   Check cursor position before adjusting columns. (closes #10133)
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Apr 20, 2022
…y spotted

Problem:    Current instance of last search pattern not easily spotted.
Solution:   Add CurSearch highlighting. (closes vim/vim#10133)
vim/vim@a439938

This fixes CurSearch highlight for multiline match.
Omit screen redrawing code because Nvim redraws CurSearch differently.
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Apr 20, 2022
…y spotted

Problem:    Current instance of last search pattern not easily spotted.
Solution:   Add CurSearch highlighting. (closes vim/vim#10133)
vim/vim@a439938

This fixes CurSearch highlight for multiline match.
Omit screen redrawing code because Nvim redraws CurSearch differently.
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Apr 20, 2022
…y spotted

Problem:    Current instance of last search pattern not easily spotted.
Solution:   Add CurSearch highlighting. (closes vim/vim#10133)
vim/vim@a439938

This fixes CurSearch highlight for multiline match.
Omit screen redrawing code because Nvim redraws CurSearch differently.
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Apr 20, 2022
…y spotted

Problem:    Current instance of last search pattern not easily spotted.
Solution:   Add CurSearch highlighting. (closes vim/vim#10133)
vim/vim@a439938

This fixes CurSearch highlight for multiline match.
Omit screen redrawing code because Nvim redraws CurSearch differently.
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Apr 21, 2022
…match

Problem:    CurSearch highlight does not work for multi-line match.
Solution:   Check cursor position before adjusting columns. (closes vim/vim#10133)
vim/vim@693ccd1
dmitmel pushed a commit to dmitmel/neovim that referenced this pull request May 18, 2022
…y spotted

Problem:    Current instance of last search pattern not easily spotted.
Solution:   Add CurSearch highlighting. (closes vim/vim#10133)
vim/vim@a439938

This fixes CurSearch highlight for multiline match.
Omit screen redrawing code because Nvim redraws CurSearch differently.
dmitmel pushed a commit to dmitmel/neovim that referenced this pull request May 18, 2022
…match

Problem:    CurSearch highlight does not work for multi-line match.
Solution:   Check cursor position before adjusting columns. (closes vim/vim#10133)
vim/vim@693ccd1
kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this pull request Jun 1, 2022
…y spotted

Problem:    Current instance of last search pattern not easily spotted.
Solution:   Add CurSearch highlighting. (closes vim/vim#10133)
vim/vim@a439938

This fixes CurSearch highlight for multiline match.
Omit screen redrawing code because Nvim redraws CurSearch differently.
kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this pull request Jun 1, 2022
…match

Problem:    CurSearch highlight does not work for multi-line match.
Solution:   Check cursor position before adjusting columns. (closes vim/vim#10133)
vim/vim@693ccd1
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.

Add highlight group for current match
8 participants