Skip to content
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

lsp-completion--exit-fn: works with non props candidate #2146

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

kiennq
Copy link
Member

@kiennq kiennq commented Sep 6, 2020

In case of just using default completion-at-point (in that case completion-in-region-function will be completion--in-region), the :exit-function will be called with selected item as string without any properties that we built up during filtering phase.
We need a way to re-associated the striped string with the full one.

The propose here is:

  • Remember the last completion results in lsp-completion--last-result, use that to trace back to full string in :exit-function.
  • Make try-completion will always return nil (no exact completion item) so that the selection will always shown and prevent early clean up of completion cache.

This will also server as first step for #1894

@yyoncho
Copy link
Member

yyoncho commented Sep 6, 2020

will be called with selected item as string without any properties that we built up during filtering phase.

Isn't that emacs bug?

@kiennq
Copy link
Member Author

kiennq commented Sep 6, 2020

will be called with selected item as string without any properties that we built up during filtering phase.

Isn't that emacs bug?

Is it? I think there's no guarantee that the string's properties will be preserved when passed to :exit-function.

For that's by design since the completion items will be displayed in other buffer in case of default completion-in-region-function.
image

Thus it will mostly cannot retain the property when selecting one of them.

Even if this's a bug (not likely), we still need to work-around in lsp-mode since it will not be fixed in pre Emacs 28.

@yyoncho
Copy link
Member

yyoncho commented Sep 6, 2020

Hm, some of the previous capf implementations used to preserve the properties.

@yyoncho
Copy link
Member

yyoncho commented Sep 6, 2020

The text properties used to work. I even filed a helm issue - emacs-helm/helm#2265 (comment) . Do you know what broke them?

@kiennq
Copy link
Member Author

kiennq commented Sep 6, 2020

AFAIK, ivy, company, emacs-helm are preserving text properties for :exit-function.

However, the default function for completion-in-region-function (which is completion--in-region) doesn't do that.
I've checked Emacs25 and it seems that behavior is there too, so it's pretty much default behavior.
https://github.com/emacs-mirror/emacs/blob/c3ff6712ad24fcf45874dc0665a8606e9b2208a4/lisp/minibuffer.el#L1005

I think you can test this by disable helm-mode and manually trigger completion-at-point (bind to C-M-i), then select item with mouse

@kiennq kiennq merged commit 4bf7c90 into emacs-lsp:master Sep 7, 2020
@kiennq kiennq deleted the bug/capf-noprops branch September 7, 2020 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants