-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
feat(completion): add two highlight groups PmenuMatch and PmenuMatchSel #14694
Conversation
Thanks, can you explain what those are for? Also should be documented at |
Will it work with fuzzy matching ? |
yup but not finish . i test it within mulitple bytes characters..something is wrong.. wait for a while |
Hello . currently the |
05c71e0
to
6f8f59e
Compare
@chrisbra PTAL. works fine for me now. also for before comment i think we can support this |
Please vote yes. Suggested here as well. |
* displays text on the popup menu with specific attributes. | ||
*/ | ||
static void | ||
pum_screen_put_with_attr(int row, int col, char_u *text, int textlen, int attr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this interface similar to screen_puts_len()
?
pum_screen_put_with_attr(text, textlen, row, col, attr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently can't
I've thought about this carefully. currently we use strncmp to compare compl_leader which always matches from the beginning of the string. so highlighting this part doesn't make sense. adding completeopt+=fuzzy would make this PR more useful. so I think I'll implement completeopt fuzzy first and then reorganize this PR |
If this is merged, please ping:
Thanks |
0200a55
to
7f767d3
Compare
after a218cc6 ..this should be more useful now and i have do some update to make pr can also handle match highlight when in fuzzy . |
7ac7aec
to
1dc59b3
Compare
e9d7cb8
to
da4d920
Compare
@chrisbra Please merge this |
yeah, those are new failures starting on the Windows builds recently. I have no idea why shortening pathnames does no longer work on Github CI Windows runners. But it only fails with multi-byte paths. No idea why, but I assume it's an issue with the Github CI runners, since there weren't any changes in the related code. |
I think this looks good now. I am just slightly worried this will cause a bit of slowdown, because we are looping over the original text several times, once to determine the match positions and then to set the highlighting attributes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just about to merging this, when I noticed the test does not work as expected and doesn't show any popup menus. Can you please fix those and also make sure the screen-dump test run once in fuzzy mode and one in regular completion mode?
In addition to your changes, I had the following changes on top, please add them while fixing the test:
diff --git a/runtime/doc/options.txt b/runtime/doc/options.txt
index 655fd6c52..1e6f313eb 100644
--- a/runtime/doc/options.txt
+++ b/runtime/doc/options.txt
@@ -1,4 +1,4 @@
-*options.txt* For Vim version 9.1. Last change: 2024 Jun 05
+*options.txt* For Vim version 9.1. Last change: 2024 Jun 10
VIM REFERENCE MANUAL by Bram Moolenaar
@@ -4275,6 +4275,7 @@ A jump table for the options with a short description can be found at |Q_op|.
T:DiffText,>:SignColumn,-:Conceal,
B:SpellBad,P:SpellCap,R:SpellRare,
L:SpellLocal,+:Pmenu,=:PmenuSel,
+ k:PmenuMatch,<:PmenuMatchSel,
[:PmenuKind,]:PmenuKindSel,
{:PmenuExtra,}:PmenuExtraSel,
x:PmenuSbar,X:PmenuThumb,*:TabLine,
@@ -4341,6 +4342,8 @@ A jump table for the options with a short description can be found at |Q_op|.
|hl-PmenuExtraSel| } popup menu "extra" selected line
|hl-PmenuSbar| x popup menu scrollbar
|hl-PmenuThumb| X popup menu scrollbar thumb
+ |hl-PmenuMatch| k popup menu matched text
+ |hl-PmenuMatchSel| < popup menu matched text in selected line
The display modes are:
r reverse (termcap entry "mr" and "me")
diff --git a/runtime/doc/syntax.txt b/runtime/doc/syntax.txt
index 23de60704..3f684412c 100644
--- a/runtime/doc/syntax.txt
+++ b/runtime/doc/syntax.txt
@@ -1,4 +1,4 @@
-*syntax.txt* For Vim version 9.1. Last change: 2024 Jun 09
+*syntax.txt* For Vim version 9.1. Last change: 2024 Jun 10
VIM REFERENCE MANUAL by Bram Moolenaar
diff --git a/runtime/doc/tags b/runtime/doc/tags
index 96be83ab0..3085ccc4c 100644
--- a/runtime/doc/tags
+++ b/runtime/doc/tags
@@ -8090,6 +8090,8 @@ hl-PmenuExtra syntax.txt /*hl-PmenuExtra*
hl-PmenuExtraSel syntax.txt /*hl-PmenuExtraSel*
hl-PmenuKind syntax.txt /*hl-PmenuKind*
hl-PmenuKindSel syntax.txt /*hl-PmenuKindSel*
+hl-PmenuMatch syntax.txt /*hl-PmenuMatch*
+hl-PmenuMatchSel syntax.txt /*hl-PmenuMatchSel*
hl-PmenuSbar syntax.txt /*hl-PmenuSbar*
hl-PmenuSel syntax.txt /*hl-PmenuSel*
hl-PmenuThumb syntax.txt /*hl-PmenuThumb*
diff --git a/runtime/doc/version9.txt b/runtime/doc/version9.txt
index 6edb8891b..71e95554e 100644
--- a/runtime/doc/version9.txt
+++ b/runtime/doc/version9.txt
@@ -1,4 +1,4 @@
-*version9.txt* For Vim version 9.1. Last change: 2024 Jun 05
+*version9.txt* For Vim version 9.1. Last change: 2024 Jun 10
VIM REFERENCE MANUAL by Bram Moolenaar
@@ -41596,6 +41596,9 @@ Autocommands: ~
Highlighting: ~
|hl-MsgArea| highlighting of the Command-line and messages area
+|hl-PmenuMatch| Popup menu: highlighting of matched text
+|hl-PmenuMatchSel| Popup menu: highlighting of matched text in selected
+ line
Commands: ~
diff --git a/src/popupmenu.c b/src/popupmenu.c
index 1c8c91c28..f3726f6ef 100644
--- a/src/popupmenu.c
+++ b/src/popupmenu.c
@@ -434,6 +434,12 @@ pum_screen_put_with_attr(int row, int col, char_u *text, int textlen, int attr)
char_u *leader = ins_compl_leader();
int in_fuzzy = (get_cot_flags() & COT_FUZZY) != 0;
+ if (highlight_attr[HLF_PMSI] == highlight_attr[HLF_PSI] &&
+ highlight_attr[HLF_PMNI] == highlight_attr[HLF_PNI])
+ {
+ screen_puts_len(text, textlen, row, col, attr);
+ return;
+ }
#ifdef FEAT_RIGHTLEFT
if (leader != NULL && curwin->w_p_rl)
rt_leader = reverse_text(leader);
diff --git a/src/testdir/test_popup.vim b/src/testdir/test_popup.vim
index 434d86e2d..db7556d95 100644
--- a/src/testdir/test_popup.vim
+++ b/src/testdir/test_popup.vim
@@ -1357,7 +1357,7 @@ func Test_pum_highlights_match()
hi PmenuMatch ctermfg=4 ctermbg=225
END
call writefile(lines, 'Xscript', 'D')
- let buf = RunVimInTerminal('-S Xscript', {})
+ let buf = RunVimInTerminal('-S Xscript', {})
call TermWait(buf)
call term_sendkeys(buf, "i\<C-X>\<C-O>\<BS>\<BS>\<BS>fi")
call TermWait(buf, 50)
diff --git a/src/vim.h b/src/vim.h
index d32d661d1..e70323937 100644
--- a/src/vim.h
+++ b/src/vim.h
@@ -1501,7 +1501,7 @@ typedef enum
, HLF_PNI // popup menu normal item
, HLF_PSI // popup menu selected item
, HLF_PMNI // popup menu matched text in normal item
- , HLF_PMSI // popup menu mathced text in selected item
+ , HLF_PMSI // popup menu matched text in selected item
, HLF_PNK // popup menu normal item "kind"
, HLF_PSK // popup menu selected item "kind"
, HLF_PNX // popup menu normal item "menu" (extra text)
f5926bb
to
c18f554
Compare
src/popupmenu.c
Outdated
if ((get_cot_flags() & COT_FUZZY) == 0) | ||
{ | ||
screen_puts_len(text, textlen, row, col, attr); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also skip this, if the HLF_PMSI and HLF_PMNI highlightings are not setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup also need compare attr if same as PSI also need run into this if.
fe321e0
to
78445cb
Compare
so PmenuMatch and PmenuMatchSel highlighting only works for when fuzzy completeopt is enabled? I think it should also work when |
emm if you want . I can restore some codes of before commit to make it rework. honestly it not make sense when there is no fuzzy. because we only use prefix match (strncmp(item ,leader, leader_len) that mean it always highlight the first few letters of each word. |
True, but the expectation from those newly introduced highlighting groups is, that they highlight the matching pattern, even if it is only a substring highlight I would think. So yes please enable a simple substring highlighting for the non-fuzzy case then |
I think only the first pattern match should be fine, so a simple subtring match is fine. I don't see the point of highlighting additional matches further down in the matches, because those are not considered when filtering the candidates. Default completion only works with single submatch filtering. |
done @chrisbra PTAL |
I'll try and test drive this during the week. |
Nice, thanks for updating the test! |
@glepnir unfortunately, this has introduced a few issues with Coverity in can you please have a look? 5199 cleanup:
5200 vim_free(tv_str.vval.v_string);
// CID 1603599: (#2 of 2): Dereference after null check (FORWARD_NULL)
// var_deref_model: Passing null pointer match_str_list to list_free, which dereferences it.["show details"]
5201 list_free(match_str_list);
// CID 1603601: (#2 of 2): Dereference after null check (FORWARD_NULL)
// var_deref_model: Passing null pointer match_pos_list to list_free, which dereferences it.["show details"]
// CID 1603602: (#2 of 2): Dereference null return value (NULL_RETURNS)
// dereference: Dereferencing a pointer that might be NULL match_pos_list when calling list_free.
5202 list_free(match_pos_list);
// CID 1603603: (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
// var_deref_model: Passing null pointer match_score_list to list_free, which dereferences it
// CID 1603602: (#2 of 2): Dereference null return value (NULL_RETURNS)
// dereference: Dereferencing a pointer that might be NULL match_pos_list when calling list_free.
5203 list_free(match_score_list);
// CID 1603600: (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
// var_deref_model: Passing null pointer retlist to list_free, which dereferences it.
5204 list_free(retlist);
5205 list_free(l);
5206 ga_clear(match_positions);
5207 return NULL;
5208#else
5209 return NULL;
5210#endif
5211}
5212 |
I am interested in this feature. Is this merged now or not? I am a bit confused that the PR is closed |
It is merged. However it has some issues discovered pointed out by Christian here. |
Alright thanks, will take some time until it lands in the new release anyway, I guess. |
Add two highlights group
PmenuMatch
andPmenuMatchSel
for the leader matched position in candidate words.