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

Support completion styles #82

Closed
clemera opened this issue May 1, 2020 · 67 comments
Closed

Support completion styles #82

clemera opened this issue May 1, 2020 · 67 comments

Comments

@clemera
Copy link
Collaborator

clemera commented May 1, 2020

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.

@AmaiKinono
Copy link
Contributor

completion-styles and completion-styles-alist

There's also completion-category-overrides.

@clemera
Copy link
Collaborator Author

clemera commented May 2, 2020

Oh yes, overlooked that one.

@raxod502
Copy link
Member

raxod502 commented May 4, 2020

Is the behavior generated by the default completion-styles equivalent to what we currently have as the default filtering behavior?

@raxod502 raxod502 added the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label May 4, 2020
@clemera
Copy link
Collaborator Author

clemera commented May 4, 2020

No but some default completion styles also don't make complete sense for selectrum because they were not designed for the interactive narrowing selectrum does(maybe we would need to exclude those). The available completion styles are described in the description strings of the elements in completion-styles-alist).

For completion-styles support with selectrum-refine-candidates-function it would probably need to accept a regular completion table as input (but this would be a breaking change for people who use filter functions which only accepts list of strings). Or we could decide to only make use of completion styles when selectrum-refine-candidates-function is set to nil (which could be the default for selectrum, most people probably use prescient or other string filter functions anyway).

@clemera clemera changed the title Support completion-styles for refinement Support completion-styles and completion metadata May 4, 2020
@clemera clemera changed the title Support completion-styles and completion metadata Support completion settings and completion metadata May 4, 2020
@clemera clemera changed the title Support completion settings and completion metadata Support completion settings and completion metadata for completing-read May 4, 2020
@clemera clemera changed the title Support completion settings and completion metadata for completing-read Support completion settings and completion metadata for completing-read collections May 4, 2020
@clemera clemera changed the title Support completion settings and completion metadata for completing-read collections Support completion styles and completion metadata for completing-read collections May 4, 2020
@raxod502
Copy link
Member

raxod502 commented May 9, 2020

If the default behavior of completion-styles is not as good as what we have by default in Selectrum, then what is the benefit from adding support for it, as opposed to just making the default behavior of Selectrum dependent on some of the completion user options?

@clemera
Copy link
Collaborator Author

clemera commented May 9, 2020

One benefit would be that the user sets completion-styles according to his preferences in his configuration and in the ideal case it's automatically picked up by frameworks. Helm has recently added support for it as well and there is an open issue for ivy. The idea is that when users configure completion-styles they get the same filtering behaviour everywhere.

Another one is that via completion-category-defaults and completion-category-overrides users can configure custom filtering for different categories. The category can be configured by the metadata of a completion table.

It's true that the current built-in completion-styles are not exhaustive in current Emacs versions (in Emacs 27 flex was added at least) but other packages can provide them, too (there already exits orderless for example which we talked about earlier).

@clemera
Copy link
Collaborator Author

clemera commented May 9, 2020

But I agree that support of completion-styles are not essential and I'm not sure custom filtering per category as described above makes much sense for selectrums model where you likely want the same filtering behaviour everywhere. In addition to that I believe one downside of using completion styles is that we would have to do filtering and highlighting in one step opposed to the more efficient way selectrum currently does it (only highlight the displayed candidates).

I think more important (but also easier) would be adding support for display-sort-function metadata of completing read collections which are functions. Currently selectrum ignores any metadata of those collections and uses (funcall collection "" predicate t) to retrieve the candidates. Letting completion tables enforce a specific sorting makes sense because there is no global variable for default completion to control sorting. The default completion behaviour also sorts the candidates but with this metadata completion tables have control over it (see tmm--completion-table for a built-in example which uses the metadata to disable sorting).

@raxod502
Copy link
Member

That seems reasonable.

@mathrick
Copy link

This seems like exactly what I'm missing from Selectrum currently. I'm trying to migrate from icomplete with partial-completion style, which allows me to type C-xC-f~/D/r-g/s/TAB and get ~/Dev/renpy-games/scratchpad/ back. I prefer Selectrum's UI to icomplete's, and I'm scared away from Ivy by its tangled mess of interdependencies with Consul and Swiper, but the fact I can't get the same completion style with either plain Selectrum or Selectrum + prescient is a huge regression for my workflow.

@okamsn
Copy link
Contributor

okamsn commented Jun 13, 2020

This seems like exactly what I'm missing from Selectrum currently. I'm trying to migrate from icomplete with partial-completion style, which allows me to type C-xC-f~/D/r-g/s/TAB and get ~/Dev/renpy-games/scratchpad/ back. I prefer Selectrum's UI to icomplete's, and I'm scared away from Ivy by its tangled mess of interdependencies with Consul and Swiper, but the fact I can't get the same completion style with either plain Selectrum or Selectrum + prescient is a huge regression for my workflow.

@mathrick, in the mean time, have you tried the package icomplete-vertical? Maybe its UI is close enough for now.

@mathrick
Copy link

@okamsn: that is working great so far, thanks for the suggestion!

@clemera clemera changed the title Support completion styles and completion metadata for completing-read collections Support completion styles Jul 17, 2020
@raxod502
Copy link
Member

Stefan Monnier sent me the following patch by personal email:

diff --git a/selectrum.el b/selectrum.el
index 6d0956f..59dfb14 100644
--- a/selectrum.el
+++ b/selectrum.el
@@ -772,6 +763,9 @@ PRED defaults to `minibuffer-completion-predicate'."
                                     selectrum--refined-candidates)))
                   -1)
                  ((and selectrum--init-p
+                       ;; FIXME: Check the `category' returned by
+                       ;; `completion-metadata' instead of
+                       ;; `minibuffer-completing-file-name'!
                        minibuffer-completing-file-name
                        (eq minibuffer-completion-predicate
                            'file-directory-p)

I think this is the issue thread that tracks support for completion-metadata features, right? If so, here's another one.

@minad
Copy link
Contributor

minad commented Dec 27, 2020

@clemera @raxod502 Would it make sense to revisit this issue - is it still necessary to have separate refinement and highlighting functions instead of using the standard completion style facilities? In particular if the idea is to move closer to the standard API with regards to configuration.

EDIT: The following seems to work with orderless for example. But the selectrum API is clearly better since in that case the highlights are only applied to the visible candidates and not to all filtered candidates.

(setq selectrum-refine-candidates-function
      (lambda (string candidates)
        (let ((result (completion-all-completions string candidates nil (length string))))
          (let ((last result))
            (while last
              (unless (listp (cdr last))
                (setcdr last nil))
              (setq last (cdr last))))
          result)))

(setq selectrum-highlight-candidates-function (lambda (_ candidates) candidates))

EDIT2: @oantolin told be that the function can be improved.

(setq selectrum-refine-candidates-function
      (lambda (string candidates)
        (let* ((result (completion-all-completions string candidates nil (length string)))
               (last (last result)))
          (setcdr last nil)
          result)))

@raxod502
Copy link
Member

raxod502 commented Jan 7, 2021

I think it's a fine idea to use the default facilities, as long as it does not make it a lot more difficult for a new user to configure the things that are currently supported by the refinement and highlighting functions, which seem fairly straightforward in usage to me. That could be either by making it clear how to make simple adjustments using the completing-read-style API, or by adding a wrapper function that can be used for these kinds of adjustments.

@oantolin
Copy link

oantolin commented Jan 7, 2021

One possibility would be to keep the current selectrum-refine-candidates-function and selectrum-highlight-candidates-function variables and simply set their default value to the values @minad suggested. That way someone who uses Selectrum's default values gets the benefit of Emacs' completion-styles. The out of the box experience with Selectrum is pretty limited, since it only matches substrings, using the default value of completion-styles is already a big improvement.

@raxod502
Copy link
Member

raxod502 commented Jan 7, 2021

That sounds perfect to me.

@clemera
Copy link
Collaborator Author

clemera commented Jan 19, 2021

@mathrick

This seems like exactly what I'm missing from Selectrum currently. I'm trying to migrate from icomplete with partial-completion style, which allows me to type C-xC-f~/D/r-g/s/TAB and get ~/Dev/renpy-games/scratchpad/ back

With #390 you can now do that, when you type the pattern you automatically get offered the possible candidates of partial matches.

@mathrick
Copy link

@clemera: Awesome, I'll give it a spin!

@mathrick
Copy link

@clemera: This is looking very nice!

I found one bug and one case in which selectrum's behaviour is less useful than icomplete's. This assumes @minad's snippet to use default completion-styles. I'm using the Emacs 26 default value for it, ie. '(basic partial-completion emacs22)

  • Bug: paths aren't actually expanded until the last character is /. That is, if I type ~/D/fo/ba.t, I won't get ~/Dev/foo/bar.txt, in fact no candidates at all will be available. I need to back up to ~/D/fo/ (note the trailing slash, ~/D/fo will again result in no candidates), at which point ~/Dev/foo/bar.txt becomes the only candidate. I need to delete the slash, enter it again, press TAB to get ~/Dev/foo/ expanded, then I can see bar.txt as well as any other file in ~/Dev/foo/
  • Deficiency / bug: TRAMP methods and hosts cannot be completed. Ie. with icomplete, /ss<TAB> gives me /ssh: and /sshx: as candidates, and /ssh:mat<TAB> results in /ssh:mathrick-thinkpad: (which is a host I've previously connected to, I'm actually not sure how and where that is stored). None of these work with selectrum-read-file-name.

@mathrick
Copy link

@minad: There's a small bug in your completion-styles adapter. Should be:

(setq! selectrum-refine-candidates-function
       (lambda (string candidates)
         (let ((result (completion-all-completions string candidates nil (length string))))
           (when result
             (setcdr (last result) nil))
           result)))

Without (when result ...), it will cause an error if no results were returned.

@clemera
Copy link
Collaborator Author

clemera commented Jan 19, 2021

@mathrick
Thanks for your feedback!

paths aren't actually expanded until the last character is /.

The idea is that once you typed a / you get all the partial completions for that path level and then can filter them down using configured filter function/ completion-styles. Except that for the filtering itself partial-completion for file names still does not work because of reasons how we handle the whole thing internally. But you could add the flex style which exists in Emacs 27 which would filter the results in a similar way for this use case. I will think about ways we could work around this automatically so you wouldn't notice the difference UI wise when are used to getting matches in you example when using partial-completion style for filtering.

TRAMP methods and hosts cannot be completed

That is a good observation, I don't know where these completions are coming from with icomplete, I will open an issue for it.

Without (when result ...), it will cause an error if no results were returned.

Did you experienced one? Because (nconc nil nil) works fine.

@minad
Copy link
Contributor

minad commented Jan 19, 2021

@clemera I think @mathrick looked at the outdated version proposed above. The actual version we are using is without issues.

@clemera
Copy link
Collaborator Author

clemera commented Jan 19, 2021

@minad
EDIT: I realized you probably referred only to the completion-styles adapter comment. The other points seem to apply, when trying to use partial completion on the results coming from partial input pattern it won't work. For example /u/s/e/s/d.el should have /usr/share/emacs/site-lisp/debian-startup.el in the results (when that path exists). Using (setq completion-styles '(partial-completion orderless)). I have proposed a fix in #393

@minad
Copy link
Contributor

minad commented Jan 19, 2021

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

@clemera
Copy link
Collaborator Author

clemera commented Jan 19, 2021

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

@clemera
Copy link
Collaborator Author

clemera commented Jan 19, 2021

@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
Copy link

@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
Copy link

mathrick commented Jan 19, 2021

@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
Copy link
Collaborator Author

clemera commented Jan 19, 2021

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

@mathrick
Copy link

@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):

  • /ssh:somehost:| → TRAMP connects to somehost with default username
  • I realise I need to add mathrick@: /ssh:m|somehost: → selectrum triggers a reconnect, TRAMP fails because it can't connect to msomehost
  • Selectrum is now wedged, and I need to C-g and repeat the whole C-x C-f process to get it working again

@clemera
Copy link
Collaborator Author

clemera commented Jan 19, 2021

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
Copy link

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
Copy link
Contributor

minad commented Feb 14, 2021

@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
Copy link
Collaborator Author

clemera commented Feb 14, 2021

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
Copy link
Contributor

minad commented Feb 14, 2021

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
Copy link
Collaborator Author

clemera commented Feb 14, 2021

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
Copy link
Contributor

minad commented Feb 14, 2021

Okay, good to know! I am considering to use completion-in-region now instead of company, see also minad/consult#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
Copy link
Collaborator Author

clemera commented Feb 14, 2021

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

@oantolin
Copy link

@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
Copy link
Contributor

minad commented Feb 14, 2021

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.

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

No branches or pull requests

7 participants