[vim/vim] Port the Neovim popup menu support for command-line completion (PR #9707)
Yegappan Lakshmanan
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/9707
Commit Summary
- a373dab Port the Neovim popupmenu support for command-line completion
File Changes
(8 files)
- M runtime/doc/options.txt (13)
- M src/cmdexpand.c (80)
- M src/drawscreen.c (4)
- M src/ex_getln.c (19)
- M src/optionstr.c (2)
- M src/popupmenu.c (10)
- M src/proto/cmdexpand.pro (4)
- M src/vim.h (2)
Patch Links:
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #9707 (a373dab) into master (8cbf249) will decrease coverage by
1.87%.
The diff coverage is30.00%.
@@ Coverage Diff @@ ## master #9707 +/- ## ========================================== - Coverage 82.40% 80.53% -1.88% ========================================== Files 154 152 -2 Lines 171664 173342 +1678 Branches 39322 39334 +12 ========================================== - Hits 141466 139593 -1873 - Misses 17043 20983 +3940 + Partials 13155 12766 -389
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 81.99% <30.00%> (?) |
|
| huge-gcc-none | ? |
|
| huge-gcc-unittests | 2.02% <0.00%> (-0.01%) |
⬇️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/drawscreen.c | 78.72% <0.00%> (+0.25%) |
⬆️ |
| src/optionstr.c | 89.52% <ø> (-0.78%) |
⬇️ |
| src/popupmenu.c | 77.85% <20.00%> (-0.83%) |
⬇️ |
| src/cmdexpand.c | 78.86% <26.47%> (-5.53%) |
⬇️ |
| src/ex_getln.c | 82.44% <55.55%> (-2.29%) |
⬇️ |
| src/libvterm/src/rect.h | 0.00% <0.00%> (-96.78%) |
⬇️ |
| src/libvterm/src/state.c | 34.86% <0.00%> (-54.44%) |
⬇️ |
| src/libvterm/src/keyboard.c | 36.84% <0.00%> (-50.79%) |
⬇️ |
| src/libvterm/include/vterm.h | 0.00% <0.00%> (-44.45%) |
⬇️ |
| src/libvterm/src/parser.c | 55.41% <0.00%> (-40.42%) |
⬇️ |
| ... and 140 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 8cbf249...a373dab. Read the comment docs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
Thanks for working on this. The number of changes is smaller than what I expected.
The navigation with CTRL-N and CTRL-P also works, which is the most natural for me. This should be mentioned somewhere.
The navigation with cursor keys is weird. Up and Down should really select the next/previous entry. Left and Right can be used for what they used to do, thus swap them.
It appears the menu is not positioned properly. E.g. ":e proto/" shows the menu too far left. With ":e proto/p it's off by one character. Can we align it with the text in the command line? It won't always fit, but should work in most cases.
When there are many matches there is a scrollbar, trying to use the mouse scrolls the whole window. Perhaps actual scrolling the menu isn't implemented, but it should probably disable the mouse scrolling.
Would be good to have a couple of screenshot tests, especially for the positioning.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Dominique Pellé
@dpelle commented on this pull request.
In src/cmdexpand.c:
> @@ -553,6 +604,28 @@ showmatches(expand_T *xp, int wildmenu UNUSED)
showtail = cmd_showtail;
}
+#ifdef FEAT_WILDMENU
+ if (wildmenu && STRCMP(p_wop, "pum") == 0)
+ {
+ char_u *endpos;
+
+ compl_match_arraysize = num_files;
+ compl_match_array = ALLOC_MULT(pumitem_T, compl_match_arraysize);
+ for (i = 0; i < num_files; i++)
+ {
+ compl_match_array[i].pum_text = L_SHOWFILE(i);
+ compl_match_array[i].pum_info = NULL;
+ compl_match_array[i].pum_extra = NULL;
+ compl_match_array[i].pum_kind = NULL;
+ }
+ endpos = (showtail ? sm_gettail(xp->xp_pattern) : xp->xp_pattern);
(nitpicky) the parentheses are not useful here.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
bfrg
It doesn't work with set wildoptions=pum,tagfile or set wildoptions=tagfile,pum.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Yegappan Lakshmanan
It doesn't work with
set wildoptions=pum,tagfileorset wildoptions=tagfile,pum.
This is addressed by the latest commit.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/cmdexpand.c:
> @@ -553,6 +604,28 @@ showmatches(expand_T *xp, int wildmenu UNUSED)
showtail = cmd_showtail;
}
+#ifdef FEAT_WILDMENU
+ if (wildmenu && STRCMP(p_wop, "pum") == 0)
+ {
+ char_u *endpos;
+
+ compl_match_arraysize = num_files;
+ compl_match_array = ALLOC_MULT(pumitem_T, compl_match_arraysize);
+ for (i = 0; i < num_files; i++)
+ {
+ compl_match_array[i].pum_text = L_SHOWFILE(i);
+ compl_match_array[i].pum_info = NULL;
+ compl_match_array[i].pum_extra = NULL;
+ compl_match_array[i].pum_kind = NULL;
+ }
+ endpos = (showtail ? sm_gettail(xp->xp_pattern) : xp->xp_pattern);
Thanks. This is addressed.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
The navigation with CTRL-N and CTRL-P also works, which is the most natural for me. This should be mentioned somewhere.
Updated the help text to mention these keys.
The navigation with cursor keys is weird. Up and Down should really select the next/previous entry. Left and Right can
be used for what they used to do, thus swap them.
I have swapped these keys now. Hopefully this doesn't cause new issues.
It appears the menu is not positioned properly. E.g. ":e proto/" shows the menu too far left. With ":e proto/p it's off by
one character. Can we align it with the text in the command line? It won't always fit, but should work in most cases.
Changed the popup menu position to the end of the cmd line. I am not sure whether this works in all the cases.
When there are many matches there is a scrollbar, trying to use the mouse scrolls the whole window. Perhaps actual
scrolling the menu isn't implemented, but it should probably disable the mouse scrolling.
Need to look into this.
Would be good to have a couple of screenshot tests, especially for the positioning.
Yes. I will add some screen shot tests.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
bfrg
When I press :e <tab> in command-line mode, the popup menu opens as expected, but after pressing <c-e> to cancel the pum, followed by <up>, Vim will insert ../ into the command-line, open the popup menu and select the first entry. This is unexpected. <up> and <down> keys should just navigate through the command-line history and not open the popup menu.
Is the pum aligned correctly in this screenshot? It seems to be off by 2 characters.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
When I press
:e <tab>in command-line mode, the popup menu opens as expected, but after pressing<c-e>to cancel the pum, followed by<up>, Vim will insert../into the command-line, open the popup menu and select the first entry. This is unexpected.<up>and<down>keys should just navigate through the command-line history and not open the popup menu.
Is the pum aligned correctly in this screenshot? It seems to be off by 2 characters.
vim-dev ML
On Mon, Feb 7, 2022 at 8:13 PM bfrg ***@***.***> wrote:
> When I press :e <tab> in command-line mode, the popup menu opens as
> expected, but after pressing <c-e> to cancel the pum, followed by <up>,
> Vim will insert ../ into the command-line, open the popup menu and select
> the first entry. This is unexpected. <up> and <down> keys should just
> navigate through the command-line history and not open the popup menu.
>
Thanks for trying out the patch and reporting this issue. I have updated
the PR to fix this issue.
Can you try out the updated PR?
> Is the pum aligned correctly in this screenshot? It seems to be off by 2
> characters.
> [image: screenshot-2022-02-08_050536]
>
>
> I think you typed "cN" and then pressed <Tab> to display the popup menu.
In that case, the popup menu
is correctly aligned. The popup menu uses the last column in the cmd line
where you invoked the completion.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Ben Jackson
What does pumvisible() return when this popup is displayed? I'm thinking that a lot of uses have mappings for <Tab> that look like inoremap <expr> <Tab> pumvisible() ? '<C-n>' : '<Tab>'. I can imagine a use case for that as a command line mapping too (i.e. to make Tab work like Ctrl-n when the menu is visible). WDYT?
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Bram Moolenaar
Let me include this now, so users can try it out. I expect there will be some improvements later.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Bram Moolenaar
I'll fix the horizontal positioning.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Bram Moolenaar
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
bfrg
@puremourning <tab> already works out of the box, pumvisible() too. The only difference I found is that <up> and <down> keys select the previous and next entry, respectively, and insert it similar to <c-p> and <c-n>. This is different from insert-mode completion where <up> and <down> will navigate through the menu without inserting the entry.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Ben Jackson
and will navigate through the menu without inserting the entry.
Only if completeopt contains noinsert right?
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Maxim Kim
Looks nice, although, I was expecting it would work for all cmdline completions, e.g. :b <tab>
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Maxim Kim
Huh, I take my words back, it works for everything, indeed. For buffers I tried the old vim.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Shane-XB-Qian
Only if
completeoptcontainsnoinsertright?
those opt options looks not worked here.
it's more like wildmenu, but just showing at 'popup menu' vs 'stl'.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
bfrg
@puremourning <up> and <down> never insert anything, they just select the previous or next entries in the menu. This is explained under :h popupmenu-keys.
Like @Shane-XB-Qian said, the new pum option behaves exactly like wildmenu but using a popup menu instead of the statusline.
pumvisible() can be used. For example, the following works:
cnoremap <expr> <enter> pumvisible() ? '<c-y>' : '<enter>' cnoremap <expr> <esc> pumvisible() ? '<c-e>' : '<esc>'
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
bfrg
Hmm, It seems the second mappings doesn't work after all:
cnoremap <expr> <esc> pumvisible() ? '<c-e>' : '<esc>'
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Shane-XB-Qian
no, it worked.
// but wildmode=list,full not supported, and wildignorecase not worked for all case ignore, and wildmenumode() seems might be wrong.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Shane-XB-Qian
no, looks it worked.
// but wildmode=list,full not supported, and wildignorecase not worked for all cmd, and wildmenumode() seems might be wrong.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Wang Shidong
Nice work, but How to deal with copyright issues? Neovim contributions since b17d96 are licensed under the Apache 2.0 license.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Yegappan Lakshmanan
Nice work, but How to deal with copyright issues? Neovim contributions since b17d96 are licensed under the Apache 2.0 license.
vim-dev ML
On Tue, Feb 8, 2022 at 7:38 AM Wang Shidong ***@***.***>
wrote:
> Nice work, but How to deal with copyright issues? Neovim contributions
> since b17d96
> are licensed under the Apache 2.0 license.
>
>
>
The changes to support the command-line popup menu are made to the core Vim
command line completion code that Neovim uses from Vim. So I think that the
Vim
license covers these changes. But I am not a lawyer, so I don't know all
the nuances
with the license.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Dominique Pellé
Using vim-8.2.4327, I get the pum completion on Ex line to work with:
vim --clean -c 'set wildoptions=pum'
It's very nice BTW. However, it does not work with:
vim --clean -c 'set wildoptions=pum wildmode=longest,list'
Any reason for pum completion to not work in that case?
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Yegappan Lakshmanan
Using vim-8.2.4327, I get the pum completion on Ex line to work with:
vim --clean -c 'set wildoptions=pum'It's very nice BTW. However, it does not work with:
vim --clean -c 'set wildoptions=pum wildmode=longest,list'Any reason for pum completion to not work in that case?
vim-dev ML
On Tue, Feb 8, 2022 at 8:02 AM Dominique Pellé ***@***.***>
wrote:
> Using vim-8.2.4327, I get the pum completion on Ex line to work with:
>
> vim --clean -c 'set wildoptions=pum'
>
> It's very nice BTW. However, it does not work with:
>
> vim --clean -c 'set wildoptions=pum wildmode=longest,list'
>
> Any reason for pum completion to not work in that case?
>
>
> Yes. Using the 'list' value in 'wildmode' should list all the matches but
not
use the wildmenu. As the help for 'wildmode' describes, if you use
"longest,full" or "longest:full", then it should open the wild menu.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
neovim looks porting vim patches everyday.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
bfredl
@wsdjeg as the main author of the neovim implementation, I have previously stated that inclusion in vim under the vim license is okay.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Shane-XB-Qian
// but i wish it can be working like `noinsert` option.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Dominique Pellé
Using this command, I see a regression:
vim --clean -c 'set wildoptions= wildmode=longest,list'
Then press for example :c<tab>s and notice that the screen is cleared when pressing s.
I don't see this bug prior at 8.2.4324. The bug started to appear at 8.2.4325.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Dominique Pellé
However, it does not work with:
vim --clean -c 'set wildoptions=pum wildmode=longest,list'
Any reason for pum completion to not work in that case?
Yes. Using the 'list' value in 'wildmode' should list all the matches but
not use the wildmenu. As the help for 'wildmode' describes, if you use
"longest,full" or "longest:full", then it should open the wild menu.
I think it could be made to work with the pum though. Maybe I'm missing
something, but when using the pum and I pressing :ar I would like it to
complete to :arg (as all completions start with :arg) on the Ex line and
show the pum with no item selected in the pum as it did not fully complete.
I also wonder, is there a way to always show the pum as I type an Ex
command without pressing ?
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Bram Moolenaar
> Using this command, I see a regression:
> ```
> vim --clean -c 'set wildoptions= wildmode=longest,list'
> ```
> Then press for example `:c<tab>s` and notice that the screen is
> The bug started to appear at 8.2.4325.
--
"Making it up? Why should I want to make anything up? Life's bad enough
as it is without wanting to invent any more of it."
-- Marvin, the Paranoid Android in Douglas Adams'
"The Hitchhiker's Guide to the Galaxy"
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Yegappan Lakshmanan
However, it does not work with:
vim --clean -c 'set wildoptions=pum wildmode=longest,list'
Any reason for pum completion to not work in that case?Yes. Using the 'list' value in 'wildmode' should list all the matches but
not use the wildmenu. As the help for 'wildmode' describes, if you use
"longest,full" or "longest:full", then it should open the wild menu.
I think it could be made to work with the pum though. Maybe I'm missing something, but when using the pum and I pressing
:ar<tab>I would like it to complete to:arg(as all completions start with :arg) on the Ex line and show the pum with no item selected in the pum as it did not fully complete.
We can support this but this will need some work. I will look into this later.
I also wonder, is there a way to always show the pum as I type an Ex command without pressing ?
This will need quite a bit of work (to make it work like the insert-mode completion popup).
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Wang Shidong
@wsdjeg as the main author of the neovim implementation, I have previously stated that inclusion in vim under the vim license is okay.
Ok, thank you. Reference at least should be mentioned in the Commit Message.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
it is just about upstream and fork.
// and BTW: what's related to your biz?
--
shane.xb.qian
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.