fix: cmp snippets source selecting issue

[CharlesChiuGit]

1. Announcement: Breaking changes hrsh7th/cmp-nvim-lsp#38
2. some times first candidates can't be selected with our current cmp keymaps, still trying to pin-point what's the cause.

How to reproduce 2.

1. Open ramdon lua file
2. Type local fu (or other keywords that can trigger snippets) and wait for the cmp windows to pop.
3. Press <Tab> to select the first cmp item.

It will keep flashing but won't select any cmp item. However, it can somehow use <Up> and <Down> to select the cmp item.
This error doesn't limited to cmp-nvim-lsp, sometimes luasnip has this issue too.
I think this issue is mostly related to snippets.

recording.mov

[ayamir]

Can repro and can address this issue, but current solution will disable <Tab> to jump to next snippets location.

[CharlesChiuGit]

@ayamir

I changed the keymaps definition a bit, now only <C-n>/<C-p> can select cmp items and <Tab>/<S_tab> can ONLY jump to next snippets location.

I think this is the only way to guarantee functionality of each keymaps.

[Jint-lzxy]

@CharlesChiuGit @ayamir Can u guys still reproduce this after explicitly specifying the argument for the select_next_item() call? After digging thru the source code, it seems that the default select behavior has changed from cmp.SelectBehavior.Select to cmp.SelectBehavior.Insert, likely due to one of folke's async PRs. Interestingly, the actual "selection" is now performed using feedkeys (which was kinda out of the blue for me lol). Anyhow this "default" Insert behavior appears to interfere with other insert-behavior-dependent plugins, causing a plethora of issues afaiu.

diff --git a/lua/modules/configs/completion/cmp.lua b/lua/modules/configs/completion/cmp.lua
index c0c7f57..06d1d95 100644
--- a/lua/modules/configs/completion/cmp.lua
+++ b/lua/modules/configs/completion/cmp.lua
@@ -110,7 +109,7 @@ return function()
 			["<C-w>"] = cmp.mapping.close(),
 			["<Tab>"] = cmp.mapping(function(fallback)
 				if cmp.visible() then
-					cmp.select_next_item()
+					cmp.select_next_item({ behavior = cmp.SelectBehavior.Select })
 				elseif require("luasnip").expand_or_locally_jumpable() then
 					require("luasnip").expand_or_jump()
 				else

EDIT: Another reason I dislike SelectBehavior.Insert is that it renders the ghost text feature useless 😇

[CharlesChiuGit]

Can u guys still reproduce this after explicitly specifying the argument for the select_next_item() call?

ys, i think the reason why

if cmp.visible() then
    cmp.mapping.select_next_item({ behavior = cmp.SelectBehavior.Select, count = 1 }),
....

will still have problem is b/c cmp.visible() is somehow not working as expected.

EDIT: Another reason I dislike SelectBehavior.Insert is that it renders the ghost text feature useless 😇

TBH, i just disable the ghost text in my personal config since it's super annoying loool

[ayamir]

Can u guys still reproduce this after explicitly specifying the argument for the select_next_item() call?

this will fix it.

[CharlesChiuGit]

Can u guys still reproduce this after explicitly specifying the argument for the select_next_item() call?

i tested again and tried few variations, but the only one that can guarantee "cmp item selection" and "in-snippit jumpping" it current pr.

[ayamir]

2024-06-29-00-10-03.mp4

[Jint-lzxy]

... is b/c cmp.visible() is somehow not working as expected.

Yeah this function has had its fair share of bugs lol but it might not be the root cause here...?

i tested again and tried few variations, but the only one that can guarantee "cmp item selection" and "in-snippit jumpping" it current pr.

Here are my observations regarding this:

1. When binding <C-n> to cmp.mapping.select_next_item(), the issue is reproducible when using <C-n>, even with the current PR's changes.
2. The issue disappears if I specify behavior = cmp.SelectBehavior.Select as an argument to cmp.mapping.select_next_item(). Specifically:

-- This PR
L130 | ["<C-n>"] = cmp.mapping.select_next_item({ behavior = cmp.SelectBehavior.Select, count = 1 }),
-- Change #1
L130 | ["<C-n>"] = cmp.mapping.select_next_item(),
-- Change #2
L130 | ["<C-n>"] = cmp.mapping.select_next_item({ behavior = cmp.SelectBehavior.Select }),

TBH, i just disable the ghost text in my personal config since it's super annoying loool

lmao that's fair, and the default select behavior is like adding insult to injury looool

[ayamir]

TBH I don't want to change the behavior of <Tab> and <S-Tab> b/c they indeed turn out into muscle memory. Additionally, this issue is not always shown in each file or it only can be triggered in some specific situation like discovered above. It's influence is limited. A minimal change is appreciated for me.

[Jint-lzxy]

TBH I don't want to change the behavior of <Tab> and <S-Tab> b/c they indeed turn out into muscle memory. Additionally, this issue is not always shown in each file or it only can be triggered in some specific situation like discovered above. It's influence is limited. A minimal change is appreciated for me.

Agreed, I will open a new minimal pull request for this and see how far we could get.

[CharlesChiuGit]

ok, i'll revert <C-n>/<C-p> and <Tab>/<S-Tab> related changes

[ayamir]

ok, i'll revert / and / related changes

Seems we can merge #1315 and close it?

[CharlesChiuGit]

Seems we can merge #1315 and close it?

i had remove the overlapping part of both prs, we should merge both.

[ayamir]

LGTM