-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat(lsp): lsp.completion support LSP preselect #36613
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
base: master
Are you sure you want to change the base?
Conversation
3a4bdc9 to
bd3fd7d
Compare
bd3fd7d to
dff7a1d
Compare
| { label = 'zzz', sortText = 'a', preselect = true }, | ||
| { label = 'aaa', sortText = 'z' }, |
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.
Is this actually testing the preselect behavior? sortText = 'a' means this would be the first result even without the preselect change.
Is there a concrete example of a language user making use of preselect?
Given that neovim controls the preselection using the preinsert/noinsert/noselect completeopt I wonder if there's even any point in supporting this at all - given the that the intention from the spec seems to be to change the selection instead of changing the sort ordering.
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.
Is there a concrete example of a language user making use of
preselect?
Rust-Analyzer uses preselect for fields that match the variable name in an assignment, see: helix-editor/helix#4480. The idea seems to be that since the server doesn't control sorting, preselect provides a way to prioritize completions based on stronger signals. When there is no prefix, the server can still mark a completion as more relevant. Another example I can think of is when completing missing type declarations when the server can infer the required type and there is only one correct fix. But if the user is refactoring, returning only that single completion would be poor behavior, and relying on sorting alone may fail to pick it if the client uses custom sorting.
Given that neovim controls the preselection using the preinsert/noinsert/noselect completeopt I wonder if there's even any point in supporting this at all - given the that the intention from the spec seems to be to change the selection instead of changing the sort ordering.
Based on microsoft/vscode#35551 and microsoft/vscode-languageserver-node#371, the client is supposed to keep the original order but move the selection to the preselected item. VS Code appears to behave this way. I'm not sure how different this is from simply placing that item first from a UX perspective, but it does seem incompatible with Neovim’s noselect anyway.
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.
The idea seems to be that since the server doesn't control sorting
The linked issue (microsoft/vscode#35551) even mentions this bit about server being able to control sorting:
Still, you can achieve this by utilising the 'sortText'-property. The VSCode UI always sorts the completion list by "rank" (in contrast to VS which selects the best match). These "ranks" are computed by looking at the text left of the cursor or, when there is no text, by using the 'sortText'-property
Relevant part from the spec:
to achieve consistency across languages and to honor different clients usually the client is responsible for filtering and sorting. This has also the advantage that client can experiment with different filter and sorting models. However servers can enforce different behavior by setting a filterText / sortText
A bit strange that they added another means to control priority of the completion candidates, but the screenshot also shows that the pre-selected entry is not the first - maybe for cases where you want to sort alphanumeric but have one prioritized anyway? I can't always follow MS logic.
In any case, seems to me that in order for neovim to support this properly it would need to extend the complete mechanism to declare preselect as part of the complete-items structure, and then also support controlling the behavior via completeopt.
That said, if adding the preselected item as first item (without duplicating it) is useful behavior in practice I'm also not really opposed. Main concern was with the test.
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.
it would need to extend the complete mechanism to declare preselect as part of the complete-items structure, and then also support controlling the behavior via completeopt.
For now, keeping it always as the first item allows us to respect noselect. if noselect isn’t included in cot, it always select the first item like preselect ? maybe we can set preselect to not vim.o.cot:find("noselect")
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.
@mfussenegger how do you feel now
0255856 to
1239672
Compare
6c196bf to
9ce5fe3
Compare
runtime/lua/vim/lsp/completion.lua
Outdated
| --- @field autotrigger? boolean (default: false) When true, completion triggers automatically based on the server's `triggerCharacters`. | ||
| --- @field convert? fun(item: lsp.CompletionItem): table Transforms an LSP CompletionItem to |complete-items|. | ||
| --- @field cmp? fun(a: table, b: table): boolean Comparator for sorting merged completion items from all servers. | ||
| --- @field preselect? boolean moves the item with `lsp.CompletionItem.preselect` to the first. Default: enabled if "noselect" is not present in `completeopt`. |
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.
| --- @field preselect? boolean moves the item with `lsp.CompletionItem.preselect` to the first. Default: enabled if "noselect" is not present in `completeopt`. | |
| --- @field preselect? boolean moves the item with `lsp.CompletionItem.preselect` to the first. Default: `false` if "noselect" is present in `completeopt`, `true` otherwise. |
Feel free to ignore this though. I just find the double-negative confusing, but I can't English sometimes so maybe this is a me-issue.
@justinmk any thoughts on clarity here?
fdbabf0 to
ddae7ba
Compare
Problem: No default completion item was preselected by LSP. Solution: Added preselect option to always move the preselected item to the first.
ddae7ba to
a3a770d
Compare
Problem: No default completion item was preselected by LSP.
Solution: Added preselect option to always move the preselected item to the first.