radian-software / selectrum

🔔 Better solution for incremental narrowing in Emacs.
MIT License
739 stars 33 forks source link

Support completion styles #82

Closed clemera closed 3 years ago

clemera commented 4 years ago

The default API uses completion-styles and completion-styles-alist to configure completions. Also if the collection passed to completing-read is a function it can return metadata to configure the completion behaviour (like for completion-in-region #84). It would be nice if selectrum would support those settings.

minad commented 3 years ago

@clemera I am not talking about the partial style, but only about the comment by @mathrick regarding using (when result ...).

clemera commented 3 years ago

Yeah, I realized that and just edited my comment above mentioning that :laughing:

clemera commented 3 years ago

@mathrick You first point should be solved by #393, please give it some more testing if you can. For the tramp case I have opened #392

mathrick commented 3 years ago

@clemera: Thanks!

393 seems to fix it completely for local paths, but not for remote ones. I still see that behaviour there, in that <TAB> doesn't trigger an expansion unless the trailing slash is present, and only candidates matching the last segment after the trailing slash are shown even after I delete that segment and press <TAB> to trigger expansion again.

mathrick commented 3 years ago

@minad: indeed, my comment was about the completion-styles adapter code exclusively. I missed the fact there was a fixed version in a PR, which is now upstream, so it's moot I guess :)

clemera commented 3 years ago

@mathrick Is this only for ssh? Because /sudo::/h/cl/co/el/sel/sel.el TAB works here.

mathrick commented 3 years ago

@clemera: nevermind, I think I was accidentally testing on an Emacs instance that had the old version loaded previously. After a restart it seems to work.

However, selectrum also seems to be a bit overzealous with initial expansion of TRAMP paths, as it triggers a connection as soon as the second colon is present in the input, without the user needing to press <TAB>. It seems it will also fail and get permanently wedged (until I cancel the input with C-g anyway) if TRAMP fails. This is a problem if there's a typo in the path, and especially if I realise I need to add a username to the host. Ie. the following flow (| is the cursor):

clemera commented 3 years ago

Okay, thanks for your feedback again! So I guess it would be better to always wait for a TAB first, when detecting a remote path?

mathrick commented 3 years ago

I think so, since there's no way to detect whether it's valid without trying to connect, which is expensive, surprising and might have unforeseen side-effects. It probably makes sense to track "selectrum gets wedged if TRAMP connection fails" as a separate bug too, since it's likely independent of the eager expansion, that just makes it easier to surface that problem.

minad commented 3 years ago

@clemera What is the currently recommended orderless configuration in order to use completion-styles? Something like this? Would it make sense to add this to the wiki?

(setq selectrum-refine-candidates-function
      (lambda (input cands)
        (let ((orderless-skip-highlighting t))
          (selectrum-refine-candidates-using-completions-styles input cands))))
(setq selectrum-highlight-candidates-function #'selectrum-refine-candidates-using-completions-styles)
clemera commented 3 years ago

I use the following (updated the wiki accordingly):

(setq orderless-skip-highlighting (lambda () selectrum-active-p))
(setq selectrum-highlight-candidates-function #'orderless-highlight-matches)
minad commented 3 years ago

Ah okay, makes sense I guess. This should also work nicely if you use (setq completion-styles '(basic substring orderless)), which I tried recently. It seems beneficial to me to use basic and substring since these are subsets of orderless and also allow TAB completion. With basic you get the prefix matches first.

clemera commented 3 years ago

I set selectrum-completion-in-region-styles to nil in my config, this will do the initial filtering using all-completions which also gives you prefix matches first when using selectrum-completion-in-region, though I remember you don't use that command.

minad commented 3 years ago

Okay, good to know! I am considering to use completion-in-region now instead of company, see also https://github.com/minad/consult/pull/218. Maybe I should also add a consult-completion-in-region-styles variable. I dislike a bit the duplication we are having here between Selectrum and Consult, but it is probably unavoidable for the Selectrum users, which do not also use Consult.

clemera commented 3 years ago

Yeah I also don't like the duplication but selectrum-completion-in-region if relative small so it isn't that bad.

oantolin commented 3 years ago

@minad said:

It seems beneficial to me to use basic and substring since these are subsets of orderless and also allow TAB completion.

I have to confess I don't see the point of TAB completion. Usually completion is a game of narrowing down to a single candidate or maybe a handful. TAB should only be allowed to insert some text into the minibuffer for you when the inserted text does not change the candidate list, right? But in that case you have made zero progress towards your goal of having a single candidate!

With basic you get the prefix matches first.

That's a good point.

minad commented 3 years ago

Yes, I agree about completion. But in the shell I am more accustomed to press TAB to complete or cycle between the candidates. I am not consistent here. I only want to have the basic/substring completion style for the shell/capf and not for minibuffer completion. With capf I already pressed TAB to start completion and then I can quickly cycle through candidates with subsequent TAB presses. This feels faster than going directly to the minibuffer completion. Aren't you also doing the same since you are setting completion-cycle-threshold? The cycle threshold feels less useful if you have to many candidates right away which is not the case with basic.