Introduce lsp-completion-passthrough-try-completion #4544
Conversation
This makes is less "passthrough", but it checks that the string proposed by completion-basic-try-completion is actually a proper prefix of all the available completions. Otherwise no expansion. Which makes it more conservative.
| @@ -755,6 +755,20 @@ The CLEANUP-FN will be called to cleanup." | |||
| "Disable LSP completion support." | |||
| (lsp-completion-mode -1)) | |||
|
|
|||
| (defun lsp-completion-passthrough-try-completion (string table pred point) | |||
| (let* ((completion-ignore-case t) | |||
There was a problem hiding this comment.
Do we need to assign this to t? I think we should just use the user's preference value here?
There was a problem hiding this comment.
AFAICS lsp-mode doesn't use this variable when matching: the list of completions is constructed case-insensitively, at least with the language server that I'm currently testing with.
There was a problem hiding this comment.
Ah, that's right. lsp-mode is using string-match, so this should be bound to case-fold-search instead
There was a problem hiding this comment.
Okay. Well, when you do string matching in completion, I think you're expected to bind case-fold-search to completion-ignore-case around that logic.
Or just go with the value t, like I think happens now most of the time.
There was a problem hiding this comment.
Great, I can bind the case-fold-search to completion-ignore-case in another PR, or if possible, can you help to bind it in this same PR if possible :).
I think there's only two places in this file.
There was a problem hiding this comment.
Note that case-fold-search is t by default, whereas completion-ignore-case is nil by default.
Obeying the user-set completion-ignore-case would make lsp-mode's completion case-sensitive. This is not bad (Eglot currently does that), but it's a significant change in behavior, I think, and it'll make it less "passthrough".
So, are you sure you want that?
There was a problem hiding this comment.
I mean we should change that in other places to make it conforms with completion-ignore-case. In this passthrough function, just returns (cons string point) should be okay. It will be similar to VsCode.
@yyoncho, let me know if you have any concern
There was a problem hiding this comment.
In this passthrough function, just returns (cons string point) should be okay.
Like mentioned, I'd like to keep the change in behavior in this PR to a minimum. After the merge, this code would be equivalent to the latest Eglot in Emacs master where I had the same goal.
Except Eglot does case matching for prefix (by default) - and lsp-mode ignores case. Not sure which is better but both of the corresponding behaviors seem fit the corresponding package' goals.
There was a problem hiding this comment.
You might very well do that change in the next edit, though (if there is agreement among the maintainers).
There was a problem hiding this comment.
Got it. I signed off for this change. Let's get another signoff and we can merge
| (string-prefix-p | ||
| beforepoint | ||
| (try-completion "" table pred) | ||
| t)) |
There was a problem hiding this comment.
instead of t, can we use completion-ignore-case here?
| (try-completion "" table pred) | ||
| t)) | ||
| try | ||
| (cons string point)))) |
There was a problem hiding this comment.
Since this is a passthrough try completion, can we just return the (cons string point) instead of using try-completion here?
There was a problem hiding this comment.
It's possible - then "expand common" will always just keep the current input.
That seems like a bit of a loss from my POV, but VS Code never supported this anyway, so 🤷
There was a problem hiding this comment.
Agree, the basic-try-completion is also matching prefix anyway, and in case of advanced language server that do the fuzzy match by themselves, I don't think we want to be smart and step in to decide where is the common prefix.
There was a problem hiding this comment.
This is my main feedback, for other comments, feel free to won't fix them.
|
@dgutov can you help me to understand more how |
It will only be used by |
Hi!
The latest version of Company features a tighter integration for
try-completionbehavior of CAPF, and a side-effect is inheriting its problems too. 😦 See this video:Screencast.from.2024-08-19.04-57-37.webm
From this comment, also see debbugs report linked there.
The same or similar bug is also present when using the default completion UI and Corfu, and this should fix that too.
Note this does not address #1653, but could be the place for the subsequent improvement/workaround.