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

Allow using completion-styles in Helm and implementation of own helm style #2165

Closed
joaotavora opened this issue Jun 27, 2019 · 135 comments
Closed

Comments

@joaotavora
Copy link

Expected behavior

Helm's completion engine can use SLY's completion functions as a backend. The original issue is here: joaotavora/sly#214

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

The value for completion-at-point-functions is sly-complete-symbol which does the completion itself. This does not trigger the problem. It only happens in other situations such as when SLY's sly-read-symbol-name calls completing-read which Helm overrides via completing-read-function. Actually it used to happen because I've turned off that overriding in that particular context context.

But the root problem remains and I guess some users would like to use Helm's UI for completing SLY symbols. However, when trying

(helm-comp-read "foo " (sly--completion-function-wrapper sly-complete-symbol-function))

Nothing interesting happens. sly--completion-function-wrapper is supposed to produce a reasonably well-behaved completion-function/table. It works well with SLY's own sly-complete-symbol

But it probably isn't well behaved enough, or not to Helm's expectations. I want you to help me understand what's missing.

In particular SLY's lower-level sly-flex-completions and sly-simple-completions functions, which request completions over the network, will return an empty list of completions when the pattern is the empty string, i.e. they only start operating once you feed them at least a character.

Perhaps Helm expects the full completion list to be handed to it up front. But that is impractical because it's frequent that a running SLY session has many thousands of symbols available. Is there a way for Helm to behave more incrementally, a little bit like company does?

Also sly-flex-completions will perform "scatter-match" completion instead of "prefix" completion, which is what sly-simple-completions does.

Steps to reproduce (recipe)

./emacs-helm.sh
(add-to-list 'load-path "~/Source/to/sly")
(require 'sly-autoloads)
M-x sly ; this requires a `lisp` program somewhere in your path, possibly sbcl

Backtraces if any (M-x toggle-debug-on-error)

N/A

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

MELPA helm-20190627.758
A reasonably fresh Emacs 27
Windows 10

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

yes

@thierryvolpiatto
Copy link
Member

Thanks to report, I will install sly and try to understand what's going on ASAP.

@thierryvolpiatto
Copy link
Member

Ok, I think I found what's wrong.
What you expect is after starting Sly TAB trigger helm completion when helm-mode is enabled, right?
It is not working because you override locally the value of completion-in-region-function, if you don't set it, helm completion is working fine.
You have to comment the line number 340 in sly-completion.el and eval the function BEFORE starting the REPL. See screenshot below.
Capture d’écran_2019-06-28_17-52-48

@thierryvolpiatto
Copy link
Member

OTH.

@thierryvolpiatto
Copy link
Member

Also I noticed that when I hit "," from the REPL I have an ido completion, you need completing-read as value for sly-completing-read-function to have helm completion. A helm user wanting ido completion somewhere have just to customize helm-completing-read-handlers-alist for the needed commands.

@joaotavora
Copy link
Author

OTH

"Off the hook" Lol :-) OK mostly. I think there will be some complications somehow somewhere, I'll test with your advice.

Also I noticed that when I hit "," from the REPL I have an ido completion, you need completing-read as value for sly-completing-read-function to have helm completion. A helm user wanting ido completion somewhere have just to customize helm-completing-read-handlers-alist for the needed commands.

That's a different problem, but thanks for reminding me.

@joaotavora
Copy link
Author

think there will be some complications somehow somewhere, I'll test with your advice.

As I feared, a deeper problem persists. First of all, thanks for promptly installing SLY and reproducing this, it is an effort not every package maintainer will make.

Now, the root problem is that Helm is caching whatever completions it gets from the completion-at-point function (capf). In the scenario where capf is and non-prefix is used, the completion UI cannot do that. If you have access to it, I would refer you to how company-mode deals with this.

But even before that, you can try this out: notice that once you complete "sly", you get a completion list of, say, 100 symbols. If you add a letter to "sly", such as "slyt" you now get 73 symbols. Fine so far. Delete the "t", you get the 100 again, now delete the "y" and notice that you stay at the very same 100, while you should now have other symbols at your disposal. (I understand the 100 is somehow a default cap in Helm, but it doesn't change the problem: the symbol list isn't refreshed).

In other words, the Lisp process needs to be contacted every time a character is added or removed from the pattern string. It is only in the reasonably simple "prefix"-matching situation that caching and client-side matching can be used. But even so, Helm must either get the full completion list upfront, or contact the process every time the pattern is widened.

Being such a popular package, surely there is some similar use-case in say, https://github.com/clojure-emacs/cider.

I hope I have managed to explain myself.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Jun 29, 2019 via email

@thierryvolpiatto
Copy link
Member

What may help is quitting helm as soon as minibuffer-contents length is < to initial input which would allow user modify its input and hit TAB again when done.

@joaotavora
Copy link
Author

The question is what is completion? Is it completing what is under cursor in current-buffer or is it completing what we have in minibuffer?

So Helm assumes that whatever text it starts out with in the minibuffer is correct and immutable until the next time you invoke Helm? And likewise the the list of completions corresponding to that text. Is that set of (apparently 100 max by default) the largest set of completions that will ever be attained in that particular Helm invocation/session?

If what I'm asking is against Helm's philosophy, it's perfectly OK. Let me just ask, to clarify. Helm is an "incremental completion and narrowing framework", right? Do you think there there is any way that this framework can be used to present the user the choice of one among, say, 1 million symbols, especially given that those 1 million symbols do not reside in the same memory as the Emacs process?

Both SLY's native interface (very close to Emacs's and the company-mode interface allow this, without forcing the user to exit that interface. Does Helm, too? (notice that this is not the same as saying that 1 million symbols are visible or present in the interface at any given time).

@joaotavora
Copy link
Author

What may help is quitting helm as soon as minibuffer-contents length is < to initial input which would allow user modify its input and hit TAB again when done.

But don't you think it's a reasonable expectation by the user that the set of possible completions is enlarged as he backspaces in the Helm interface, just as it does when the user does so after more characters have been entered beyond "initial input"?

@thierryvolpiatto
Copy link
Member

At first yes, but after thinking about it no.
Emacs completion-in-region expect completing something against initial input, adding the missing characters to what is at point.
If you have at point "sly" completion-in-region will complete "sly" with anything else starting with "sly", if you change your mind and change the minibuffer contents to what is at point e.g. "def" and Helm behave as you are asking, completion in region mechanism would be confused and insert e.g. "slydefun".
Remember that what we are talking about is a generic completion i.e. same function should handle sly, elisp completion etc...
So for a generic completion, I am not sure we should do more than what we are doing actually (apart maybe quitting when minibuffer differs from input).
But a specialized package may be created to complete in sly as you are expecting.
What do you think?

@thierryvolpiatto
Copy link
Member

Apart this SLY looks really cool!

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Jun 29, 2019 via email

@joaotavora
Copy link
Author

Completing against million symbols ? Yes.

But to do so without exiting the Helm interface, you need to fetch the million symbols upfront, yes?

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Jun 29, 2019 via email

@joaotavora
Copy link
Author

joaotavora commented Jun 29, 2019

Not if you complete against the prefix, which is completion, I have the impression you want to compute all candidates like if input were empty string.

No, it's very simple: I want to be able to explore the whole domain, the whole set/space of completions, in the following way:

  1. Without exiting the completion interface, or whatever you want to call it.
  2. Without ever having to compute the whole set upfront.

Prefix or non-prefix does not enter into it. I just found it very surprising that once you invoke helm on abc and then backspace back to ab you stay in the Helm interface but don't see the completions that start with abd. My simple question is: is this by design or am I doing something wrong? If it's by design just say so :) I don't use Helm but perhaps that is what your users expect/or how you think it should behave.

thierryvolpiatto pushed a commit that referenced this issue Jun 30, 2019
helm is completing against.

* helm-mode.el (helm-mode-prefix): New face.
(helm-comp-read-map): Bind new delete backward command.
(helm-mode--completion-in-region-initial-input): Add props to initial input.
(helm-mode-delete-char-backward-1): New.
(helm-mode-delete-char-backward-2): New
(helm-mode-delete-char-backward-maybe): New, bind it.
(helm--completion-in-region): Propertize prefix completion.
@thierryvolpiatto
Copy link
Member

What you are asking (1.) is not completion, so I can't make this happen in Helm as it will no more fit with the requirements of a generic command.
(2.) can be done by dynamically computing candidates with probably a function instead of a plain list of candidates precomputed.

@thierryvolpiatto
Copy link
Member

With last Helm, now completion behave like this:

  • Prefix is highlighted and read-only
  • When trying to delete backward char prefix helm is completing against, a message is sent on first hit on DEL and second hit delete the last char of what you have at point in current-buffer quitting helm at the same time, letting user correct its initial input and hitting TAB again to start a new completion.

@joaotavora
Copy link
Author

What you are asking (1.) is not completion

In your personal view of what completion is. company behaves exactly like this. Is company a completion engine or is it something else? Maybe it's a question that doesn't need an answer: let's call it a different UI.

with probably a function instead of a plain list of candidates precomputed

Great idea. But what do you mean "probably"? In fact that idea has already been had, and, what a coincidence ;-) company uses it!

With last Helm, now completion behave like this:

I think these are improvements on the current situation. I don't understand the use case of letting the user delete the initial pattern and suggesting he press TAB. In my opinion it should either be totally forbidden, or the re-fetch should be automatic. Don't you think this is a simpler interface?

Anyway remember that you shouldn't be doing these improvements motivated by Sly, which, for the time being now, uses either its own UI or company. Think about what your current users would expect (i'm not one of them ;-) )

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Jul 1, 2019 via email

@joaotavora
Copy link
Author

No, as explained before company behave the same and emacs vanilla completion too, the only difference is they have NO minibuffer, i.e. the input is fetched directly from what the user write in current buffer whereas helm is taking this same input and put it in minibuffer to provide completion.

Helm is taking this same input and put it in minibuffer to provide completion.

I don't understand why it makes a difference where typing happens. The end goal for the user is to complete the thing he started, presumably with as few keystrokes as possible, with as much visual feedback as possible, and as little visual intrusion as possible. Different UI's have different approaches. Helm happens uses the minibuffer, fine by me.

I also don't understand what semiauto/auto/manual mean in your case. Certainly none of these UI engines complete something "automatically", i.e. with some kind of IA engine. They all show behave slightly differently on certain keystrokes, and show information in different areas of the screen.

Helm could also do this but for a specialized command to complete dynamically, here we are speaking of generic completion for helm-mode.

A helm-sly (or company-sly, or x-sly) would be a waste of effort. I have another LSP extension, eglot that uses the same concepts, I'm not going to repeat that effort, and I don't think anyone should have to. You (and I) should be coding to a common interface, not to a specific backend/frontend.

The good news is that interface already exists, see info node 20.6.7 Programmed Completion. The bad news is it not the best (for historical/backward compatibility reasons), and not wonderfully documented either. I'm frequently in touch with Stefan Monnier about this. Let's bother him here again, @monnier. See also the variable completion-styles, it's another part of the interface that frontends should respect.

Now SLY doesn't yet respect these interfaces fully (and neither does company-capf by the way). But it respects parts of it and I am improving support for it. If Helm were to respect it a bit more, it would work with SLY and other backends that respect it.

To have dynamic completion with Helm you have to write a specialized package providing a new command for TAB replacing completion-at-point.

No, this is precisely what I want to avoid. No speciazlied nothings, life is too short.

joaotavora added a commit to joaotavora/sly that referenced this issue Jul 1, 2019
This introduces a new "backenbd" completion style (per
completion-styles-alist) that allows SLY's function backend to be
written more concisely and comply more closely with other UI's (such
as icomplete or others), which can be used if the user turns off
sly-symbol-completion-mode.

Also motivated by emacs-helm/helm#2165.

* lib/sly-completion.el (completion-styles-alist): Add backend
completion-style.
(completion--backend-call, completion-backend-try-completion)
(completion-backend-all-completions): New helpers.
(sly--completion-function-wrapper): Simplify, and rewrite.
(sly--completion-request-completions): Remove "no completion message"
(sly-simple-completions): Add a bit of doc.
(sly--completion-function-wrapper): Rewrite.
(sly--completion-setup-target-buffer): Remove.
(sly--setup-completion): New helper.
(sly-symbol-completion-mode): New minor mode
(sly-mode-hook): Use sly--setup-completion.
(sly--completion-in-region-function): Cleanup slightly.
(sly--with-sly-minibuffer): Simplify.
(sly-minibuffer-setup-hook): Add a bit of doc.
@joaotavora
Copy link
Author

Now SLY doesn't yet respect these interfaces fully

I've done some work in a side branch (https://github.com/joaotavora/sly/tree/scratch/fix-214-rework-completion) and it should be more compliant now. To illustrate (and to prove that it really does work) here are 4 scenarios:

(1) basic completion (what you call "manual" completion).

sly-basic

(2) company (what you call "fully auto")
sly-company

(3) icomplete (an ido.el replacement in emacs core)
sly-icomplete

(4) SLY's own UI
sly-own-ui

(5) Helm, not quite working

sly-helm

@thierryvolpiatto
Copy link
Member

You can try with this branch https://github.com/emacs-helm/helm/tree/completion_in_region_dynamic.
Not sure it works everywhere.

@joaotavora
Copy link
Author

You can try with this branch https://github.com/emacs-helm/helm/tree/completion_in_region_dynamic.
Not sure it works everywhere.

Thanks very much for attempting this. Working properly with Helm is a big plus for any package.

Now, I tried it, but it didn't work. Perhaps I'm not testing correctly. I checked out your branch, git clean -fdx, make EMACS_COMMAND=..., then ./emacs-helm and did basically what I do in the last .gif. I get the same behaviour except, in the completion-at-point case, it doesn't even get to the Helm interface.

How are you testing?

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Jul 2, 2019 via email

@joaotavora
Copy link
Author

Tested with completion-at-point in emacs-lisp buffers and completing-read-multiple, these are working.

That's a good start. I will debug SLY a bit today.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Jul 2, 2019 via email

@thierryvolpiatto
Copy link
Member

As I expected, trying to pass pattern (minibuffer-contents) to a dynamic function is a regression in Helm because it defeat all helm matching functions. In the example you show above e.g. icomplete, it is simple because the completion is basic, and helm is working fine like this, but as soon as you enter a more complex pattern, the function doesn't understand it, we would need a super function handling alone all the helm features, which is not possible. So with the actual behavior of the Master branch, I think we have a good compromise, loosing only the ability of changing on the fly the initial input, but well when you start to complete on e.g. "def" I hardly see why you would change your mind as soon you hit TAB to complete on something else.
Below a screenshot illustrating pattern than can be used with helm:
Capture d’écran_2019-07-03_09-37-29

@monnier
Copy link
Contributor

monnier commented Nov 21, 2019 via email

@joaotavora
Copy link
Author

I think flex-no-space is the way to go here. Or just advising users that mixing and matching styles doesn't always work as expected.

On a tangent, I'm quite suprised helm doesn't provide a flex style itself. I assumed it provides many modern and sophisticated things.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 21, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 21, 2019 via email

@joaotavora
Copy link
Author

joaotavora commented Nov 21, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 21, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 21, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 21, 2019 via email

@thierryvolpiatto
Copy link
Member

So now with the use of the new variable completion-flex-nospace in helm (it is let bounded) helm and flex work beautifully with e.g. switch-to-buffer :-).

@joaotavora
Copy link
Author

@thierryvolpiatto thanks. It's a decent approach, but it still doesn't solve the use case where I don't the helm interface but still want to use some helm matching styles, right?

This would require an actual flex-nospace style. However, and since you already pushed this, I think a good compromise is to keep completion-flex-nospace but default it to t. I think most people don't expect flex to match spaces anyway.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 22, 2019 via email

@joaotavora
Copy link
Author

joaotavora commented Nov 22, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 22, 2019 via email

@joaotavora
Copy link
Author

The shame is that there is no one UI in emacs vanilla able to handle
this, what can I do on helm side? nothing.

Makes sense. But you can explain exactly what the minimal requirements of such an UI is. To be clear, an UI where the styles provided by Helm make sense (and why it is a good thing).

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 22, 2019 via email

@monnier
Copy link
Contributor

monnier commented Nov 22, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Nov 23, 2019 via email

@thierryvolpiatto
Copy link
Member

@joaotavora @monnier , does it make a difference for you if using add-face-text-property instead of put-text-property for setting completions-first-difference in completion-pcm--hilit-commonality?
The problem is this face (completely usefulness in helm) is overriding the possible face actually in use in helm display like actually in helm-org:
Capture d’écran_2019-12-05_11-02-57

but after applying patch below using add-face-text-property leading star appear fontified properly:

Capture d’écran_2019-12-05_11-03-37

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 779c3c88ae8..2cef08fd9f0 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3180,9 +3180,10 @@ one-letter-long matches).")
                               'face 'completions-common-part
                               str)
            (if (> (length str) pos)
-               (put-text-property pos (1+ pos)
-                                  'face 'completions-first-difference
-                                  str))
+               (add-face-text-property
+                pos (1+ pos)
+                'face 'completions-first-difference
+                str))
            (unless (zerop (length str))
              (put-text-property
               0 1 'completion-score

More generally I wonder why you are bothering showing this in completions...

@joaotavora
Copy link
Author

I think the patch makes sense: using put-text-property there is a bug, I think. (did I add it?)

More generally I wonder why you are bothering showing this in completions...

I agree, and there's a long long bikeshedding thread about that that didn't end on particularly happy note.

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 5, 2019 via email

@joaotavora
Copy link
Author

It is also used for completions-common-part may be we should use
add-face-text-property as well here?

Probably. yes.

Not sure, but it seems you only merge the scoring code inside this function.

That's my perception as well, which means you should also quickly ping Stefan about this change.

most of the time suffix is irrelevant,

I think Stefan and some other users of completion will modify "most of the times" to "sometimes". IOW that code is there for reasons.

@monnier
Copy link
Contributor

monnier commented Dec 5, 2019 via email

@thierryvolpiatto
Copy link
Member

thierryvolpiatto commented Dec 6, 2019 via email

@thierryvolpiatto
Copy link
Member

Pushed to master.
With emacs -Q:
Capture d’écran_2019-12-06_07-46-39

@thierryvolpiatto
Copy link
Member

Closing now this long thread, many thanks to all for their help, specially to @joaotavora and @monnier .
Feel free to open a new issue if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants