search

emacs-helm / helm

Emacs incremental completion and selection narrowing framework
https://emacs-helm.github.io/helm/
GNU General Public License v3.0
3.37k stars 390 forks source link

Helm capf does not call exit-function #2265

Closed yyoncho closed 4 years ago

yyoncho commented 4 years ago

Expected behavior

helm calls :exit-function

Actual behavior (from emacs-helm.sh if possible, see note at the bottom)

helm does not call exit function

Steps to reproduce (recipe)

(defun bug-completion-at-point ()
  (list
   (point)
   (point)
   (completion-table-dynamic (lambda (&rest _)
                               (list "foobar"
                                     "foobar2")))
   :exit-function (lambda (candidate _status)
                    (debug))))
(setq-local completion-at-point-functions (list 'bug-completion-at-point))

Describe versions of Helm, Emacs, operating system, etc.

Latest from melpa, emacs 27/26 . Linux.

Are you using emacs-helm.sh to reproduce this bug? (yes/no):

No, but reproduced in clean env with

Are you using Spacemacs? (yes/no):

No.

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

Expected behavior

helm calls :exit-function

No.

helm does not call exit function

:exit-function is only needed for the emacs vanilla completion UI, i.e. "completions" buffer, so yes helm doesn't handle it, though it handle :annotation-function.

What do you want do do exactly?

-- Thierry

Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

yyoncho commented 4 years ago

:exit-function is used by the backends (capf functions) to perform post-completion processing(e. g. expanding snippets). ATM the only frontend that does not call :exit-function is helm. It works fine with company-capf, ivy and vanilla. The result is that helm capf frontend does not work with lsp-mode(and probably all other backends that perform post-completion).

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

:exit-function is used by the backends (capf functions) to perform post-completion processing(e. g. expanding snippets). ATM the only frontend that does not call :exit-function is helm. It works fine with company-capf, ivy and vanilla. The result is that helm capf frontend does not work with lsp-mode(and probably all other backends that perform post-completion).

Thanks for explanation, is this working for you?

diff --git a/helm-mode.el b/helm-mode.el index cb75b989..0dd9e32c 100644 --- a/helm-mode.el +++ b/helm-mode.el @@ -1674,7 +1674,8 @@ Can be used for `completion-in-region-function' by advicing it with an (advice-add 'lisp--local-variables :around #'helm-mode--advice-lisp--local-variables)

-- Thierry

Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

thierryvolpiatto commented 4 years ago

If you can provide a recipe on how to reproduce with lsp-mode it is perfect ;-) At least with the recipe you sent and the patch above applied, the exit fn is called.

Note: I know nothing about lsp-mode and how to make it working (dependencies etc..). Thanks.

yyoncho commented 4 years ago

Thank you for the quick reply, I will test it on my side and report back.

yyoncho commented 4 years ago

Using the patch from above I get an error in lsp-mode since it is called with candidate = "" (in your case the string param in completion--done). I would expect completion--done to be called with the selected candidate as it was generated from the capf function since we encode properties in it.

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

Using the patch from above I get an error in lsp-mode since it is called with candidate = "" (in your case the string param in completion--done). I would expect completion--done to be called with the selected candidate as it was generated from the capf function since we encode properties in it.

Ok, thanks, here the modified patch.

diff --git a/helm-mode.el b/helm-mode.el index cb75b989..3cdb81af 100644 --- a/helm-mode.el +++ b/helm-mode.el @@ -1674,7 +1674,8 @@ Can be used for `completion-in-region-function' by advicing it with an (advice-add 'lisp--local-variables :around #'helm-mode--advice-lisp--local-variables)

-- Thierry

Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

yyoncho commented 4 years ago

I tested the last patch - the candidate text is the correct one but the text properties are stipped.

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

I tested the last patch - the candidate text is the correct one but the text properties are stipped.

Yes text properties are always stripped, at least in helm-mode. So what you are expecting is much harder to fix (I mean to fix without breaking everything else).

-- Thierry

Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

yyoncho commented 4 years ago

Isn't it possible to fix only this source to provide a lookup and allow finding the original item without changing the helm core stuff? Another option is to do that on our side via hacky buffer-local variable solution to do the same. And the third option is to declare/document both helm capf and lsp-mode incompatible.

thierryvolpiatto commented 4 years ago

Ivan Yonchovski notifications@github.com writes:

Isn't it possible to fix only this source to provide a lookup and allow finding the original item without changing the helm core stuff?

Kind of, this is what I am working on.

-- Thierry

Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

thierryvolpiatto commented 4 years ago

Can you try last changes?

thierryvolpiatto commented 4 years ago

Merged to master.

yyoncho commented 4 years ago

I can confirm that it works fine. Thank you.