[vim/vim] fix: do not close pum on <BS> using `complete()` (PR #15846)

Sebastian Lyng Johansen

unread,

Oct 10, 2024, 5:28:47 PM10/10/24

to vim/vim, Subscribed

Closes #15833

You can view, comment on, or merge this pull request online at:

https://github.com/vim/vim/pull/15846

Commit Summary

c030596 fix: do not close pum on <BS> using `complete()`

File Changes

(1 file)

M src/insexpand.c (1)

Patch Links:

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 10, 2024, 5:30:45 PM10/10/24

to vim/vim, Subscribed

I should probably update the test? @chrisbra

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 11, 2024, 1:31:41 AM10/11/24

to vim/vim, Subscribed

update the test case in test_popup which i already mentioned in issue and add some test case.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 11, 2024, 1:33:57 AM10/11/24

to vim/vim, Subscribed

Will do later today

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 11, 2024, 11:06:05 AM10/11/24

to vim/vim, Push

@seblj pushed 1 commit.

1680534 Add test for bs with complete()

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 11, 2024, 11:08:34 AM10/11/24

to vim/vim, Subscribed

Let's see what CI says.

Let me know if I should it needs any more tests. I added a test that does a bunch of backspaces, and then do <C-n> to select the next item in the menu that should now be open.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 12, 2024, 2:55:46 AM10/12/24

to vim/vim, Subscribed

@glepnir commented on this pull request.

In src/testdir/test_popup.vim:

>    call feedkeys("aJ\<f5>\<bs>\<c-l>\<esc>", 'tx')
-  call assert_equal(["Januar"], getline(1,2))
+  call assert_equal(["January"], getline(1,2))
+  %d
+
+  " C-L and a <bs> back to `J`, and then select next item from completion
+  " menu which should still be open
+  call feedkeys("aJ\<f5>\<bs>\<bs>\<bs>\<bs>\<bs>\<bs>\<c-n>\<c-y>\<esc>", 'tx')

Is it possible to add some more test cases to check for edge cases?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 12, 2024, 2:57:10 AM10/12/24

to vim/vim, Subscribed

@seblj commented on this pull request.

In src/testdir/test_popup.vim:

>    call feedkeys("aJ\<f5>\<bs>\<c-l>\<esc>", 'tx')
-  call assert_equal(["Januar"], getline(1,2))
+  call assert_equal(["January"], getline(1,2))
+  %d
+
+  " C-L and a <bs> back to `J`, and then select next item from completion
+  " menu which should still be open
+  call feedkeys("aJ\<f5>\<bs>\<bs>\<bs>\<bs>\<bs>\<bs>\<c-n>\<c-y>\<esc>", 'tx')

Sure, but do you have any edge cases in mind?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 12, 2024, 7:27:16 AM10/12/24

to vim/vim, Subscribed

Failed CI seems unrelated to my changes?

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 12, 2024, 7:32:06 AM10/12/24

to vim/vim, Subscribed

Nope. random build error

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 12, 2024, 7:36:27 AM10/12/24

to vim/vim, Subscribed

@glepnir commented on this pull request.

In src/testdir/test_popup.vim:

>    call feedkeys("aJ\<f5>\<bs>\<c-l>\<esc>", 'tx')
-  call assert_equal(["Januar"], getline(1,2))
+  call assert_equal(["January"], getline(1,2))
+  %d
+
+  " C-L and a <bs> back to `J`, and then select next item from completion
+  " menu which should still be open
+  call feedkeys("aJ\<f5>\<bs>\<bs>\<bs>\<bs>\<bs>\<bs>\<c-n>\<c-y>\<esc>", 'tx')

Single and multiple bs behaviors. otherwise LGTM

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Sebastian Lyng Johansen

unread,

Oct 12, 2024, 7:39:28 AM10/12/24

to vim/vim, Subscribed

@seblj commented on this pull request.

In src/testdir/test_popup.vim:

>    call feedkeys("aJ\<f5>\<bs>\<c-l>\<esc>", 'tx')
-  call assert_equal(["Januar"], getline(1,2))
+  call assert_equal(["January"], getline(1,2))
+  %d
+
+  " C-L and a <bs> back to `J`, and then select next item from completion
+  " menu which should still be open
+  call feedkeys("aJ\<f5>\<bs>\<bs>\<bs>\<bs>\<bs>\<bs>\<c-n>\<c-y>\<esc>", 'tx')

Isn't that what I already have? I changed the failing test to a single bs, and then <c-l> to accept it, and then this with multiple bs

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 12, 2024, 7:47:33 AM10/12/24

to vim/vim, Subscribed

@glepnir commented on this pull request.

In src/testdir/test_popup.vim:

>    call feedkeys("aJ\<f5>\<bs>\<c-l>\<esc>", 'tx')
-  call assert_equal(["Januar"], getline(1,2))
+  call assert_equal(["January"], getline(1,2))
+  %d
+
+  " C-L and a <bs> back to `J`, and then select next item from completion
+  " menu which should still be open
+  call feedkeys("aJ\<f5>\<bs>\<bs>\<bs>\<bs>\<bs>\<bs>\<c-n>\<c-y>\<esc>", 'tx')

Yes, it's just that the few case always feels a little loose. but it's okay. It's reasonable.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Boris Staletic

unread,

Oct 12, 2024, 1:00:46 PM10/12/24

to vim/vim, Subscribed

After ~~receiving the bat signal~~ ping from puremourning, I did test this pull request with YouCompleteMe.
It does subtly break things.

Having vim hold onto the menu while the user hits backspace means a more appropriate menu now has to be explicitly requested by the user.

Here's how it used to work:

https://asciinema.org/a/xuCNFKE0LuJbqSrBpJOYIPLF3

And here's how it works after this patch:

https://asciinema.org/a/IkQ7OLa0uVYY6rjOJLPQ4PB1h

Notice that, at the end, where the line is std::common_reference_, in the "after" recording, the only candidate remained common_reference_with. To get all the candidates I, as a user, would have had to hit <C-Space> and explicitly make YouCompleteMe give me all the completions.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Boris Staletic

unread,

Oct 12, 2024, 1:22:28 PM10/12/24

to vim/vim, Subscribed

A more problematic breaking change that this introduces is holding onto <C-x><C-l> menu when we shouldn't have.
Unfortunately, asciinema does not let vim receive <C-l>, so I'll have to describe the steps:

Open a new python file.
Write aaaaaaaaaa = 5555555555 on the first line.
Open a new line.
Use <C-x><C-l> to complete the whole line.
Kill = and 555 with <C-w><C-w>
Start backspacing into aaaaa.

Expectation: YouCompleteMe takes over word completion automatically.
Actual: YouCompleteMe calls complete() once, the vim recalls... the full line completion and starts doing that instead.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Oct 13, 2024, 4:42:22 AM10/13/24

to vim/vim, Subscribed

oh wow, that's a bummer. May be we can change the behaviour only for CTRL-H and not BS instead?

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Boris Staletic

unread,

Oct 13, 2024, 4:53:02 AM10/13/24

to vim/vim, Subscribed

@chrisbra YouCompleteMe used to have s:OnCharacterDelete, to implement the current behaviour back before complete() was a thing and we had to feed <C-x><C-u>.
We could resurrect that part of code again and make it work with the newly fixed complete(). It might be ugly, because of possible vim version checks, but it's doable.

The second problem that I've posted, the one where vim seems to be holding onto <C-x><C-l> for far too long - that one is a concern.
Can that part be fixed on the vim side? I have no clue right now how could that one be solved by plugins.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 13, 2024, 5:06:00 AM10/13/24

to vim/vim, Subscribed

does relate #7567 ? The failure you mentioned is not caught by our test case. strange. I tried the minimal reproducible steps. Unfortunately I can't find it. It seems that ycm is doing something special here

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Boris Staletic

unread,

Oct 13, 2024, 5:24:03 AM10/13/24

to vim/vim, Subscribed

does relate #7567 ?

I don't think so... That's talking about :h i_CTRL-l, while I mentioned :h i_CTRL-x_CTRL-l.

I tried the minimal reproducible steps. Unfortunately I can't find it. It seems that ycm is doing something special here

Hmm... YCM is, effectively, calling complete() on every TextChangedI, but through a timer, so that calculation of new menu candidates happens without blocking things. I guess that does count as "something special".

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 13, 2024, 5:39:47 AM10/13/24

to vim/vim, Subscribed

yes it is the usual approach for completion plugins with a debounce timer. I might try this again later. I hope to find a minimal test case and update our tests.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

glepnir

unread,

Oct 14, 2024, 3:44:49 AM10/14/24

to vim/vim, Subscribed

@bstaletic Hi I want to know how the matches list is generated when ycm calls complete here? When the timer is triggered, I see that ycm in the gif provides the entire line of text after receiving the patch with few times bs. So how is the list of the complete function generated? This will help me complete a test case :)

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Boris Staletic

unread,

Oct 14, 2024, 4:03:57 AM10/14/24

to vim/vim, Subscribed

When the timer is triggered, I see that ycm in the gif provides the entire line

That's exactly the problem. YCM never completes the full line. YCM is only trying to complete identifiers in this case - the aaaaaaaa identifier. You can recognize YCM completion by the [ID] which ends up in the menu property of the completion candidates.

This is where YCM takes the generated identifier candidates and converts them to vim data:
https://github.com/ycm-core/YouCompleteMe/blob/master/python/ycm/client/completion_request.py#L186

And this class is responsible for producing identifier candidates:
https://github.com/ycm-core/ycmd/blob/master/ycmd/completers/all/identifier_completer.py

I hope that helps! I'll gladly answer any further questions.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Dec 28, 2024, 10:01:29 AM12/28/24

to vim/vim, Subscribed

converting to draft, until we have found out how to best handle this breaking change.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Jun 13, 2026, 4:14:19 PM (10 hours ago) Jun 13

to vim/vim, Subscribed

chrisbra left a comment (vim/vim#15846)

this has stalled, so closing

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward