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

Initialize selectrum after all other functions in minibuffer-setup-hook #242

Conversation

clemera
Copy link
Collaborator

@clemera clemera commented Nov 29, 2020

When using completing-read sorting can be configured/disabled by providing display-sort-function via the metadata. To correctly disable sorting for selectrum-read one needs to set it in minibuffer-setup-hook like this:

(minibuffer-with-setup-hook
    (lambda ()
      (setq-local selectrum-should-sort-p nil))
  (selectrum-read "Switch to: " 
                  '("longest" "longer" "short")))

Before this PR this doesn't actually work because selectrum--minibuffer-setup-hook runs before the temporary hook added above. Alternatively we could use (:append (lambda ...)) for selectrum-read but this might cause unexpected side effects for existing code which assumes selectrum-read runs early, we also have such code in selectrum itself (see selectrum-read-file-name). Because of that I moved the sorting code into selectrum--minibuffer-post-command-hook which seems like the simplest fix without breaking anything due to changed behaviour.

@clemera clemera mentioned this pull request Nov 29, 2020
@minad
Copy link
Contributor

minad commented Nov 29, 2020

@clemera I wonder if there is a simpler alternative to disable sorting. I think the nicety of using let with selectrum-should-sort-p is somehow lost using this hook idiom. Rather add a :sort option to the selectrum api? I always have recursive minibuffers enabled and use them often. Therefore I would always prefer to disable things locally. Always writing this minibuffer hook idiom is very ugly.

@clemera
Copy link
Collaborator Author

clemera commented Nov 29, 2020

Yeah, it's not too beautiful ;) Generally we don't want to encourage using selectrum-read and adding more options and convenience features would probably result in more users use it instead of completing-read.

@minad
Copy link
Contributor

minad commented Nov 29, 2020

@clemera Please look at my proposal in #243. I don't have a really good solution yet, but I need something like configuration "from outside"...

@clemera clemera force-pushed the ensure-sort-runs-after-minibuffer-setup-hook branch from 842416f to 8ba7fd9 Compare November 30, 2020 10:27
@minad
Copy link
Contributor

minad commented Nov 30, 2020

Anything preventing from merging this? Looks simple enough.

@clemera
Copy link
Collaborator Author

clemera commented Nov 30, 2020

I'm still thinking if we shouldn't move to use (:append ...) within selectrum-read instead because it would be cleaner and does not need us to move additional code into post command hook if we add more variables which are set initially. We can also refactor that later though if there is need to hurry, I guess you want to get it merged before consult gets included in melpa?

@minad
Copy link
Contributor

minad commented Nov 30, 2020

@clemera No hurry from me and the melpa pipeline is kind of clogged - just asking.

@clemera clemera force-pushed the ensure-sort-runs-after-minibuffer-setup-hook branch from 8ba7fd9 to d98d3a0 Compare November 30, 2020 13:24
@clemera
Copy link
Collaborator Author

clemera commented Nov 30, 2020

I assume there isn't any code out there which relies on this minor detail. I adjusted the case where we used (:append...) around selectrum-read accordingly.

@clemera clemera force-pushed the ensure-sort-runs-after-minibuffer-setup-hook branch 2 times, most recently from 9154183 to c674791 Compare November 30, 2020 14:01
@clemera clemera force-pushed the ensure-sort-runs-after-minibuffer-setup-hook branch from c674791 to 26604ec Compare November 30, 2020 14:02
@clemera clemera changed the title Ensure sort runs after minibuffer-setup-hook Apply minibuffer setup last in minibuffer-setup-hook Nov 30, 2020
@clemera clemera changed the title Apply minibuffer setup last in minibuffer-setup-hook Run selectrum setup last in minibuffer-setup-hook Nov 30, 2020
@clemera clemera changed the title Run selectrum setup last in minibuffer-setup-hook Setup selectrum last in minibuffer-setup-hook Nov 30, 2020
@clemera clemera changed the title Setup selectrum last in minibuffer-setup-hook Setup selectrum at last in minibuffer-setup-hook Nov 30, 2020
@clemera clemera changed the title Setup selectrum at last in minibuffer-setup-hook Setup selectrum last in minibuffer-setup-hook Nov 30, 2020
@clemera clemera changed the title Setup selectrum last in minibuffer-setup-hook Setup selectrum after all other functions in minibuffer-setup-hook Nov 30, 2020
@clemera clemera changed the title Setup selectrum after all other functions in minibuffer-setup-hook Initialize selectrum after all other functions in minibuffer-setup-hook Nov 30, 2020
@clemera clemera merged commit 945f8fe into radian-software:master Nov 30, 2020
@clemera clemera mentioned this pull request Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants