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

Add vertico-candidate-action-hook. #248

Closed
wants to merge 1 commit into from

Conversation

okamsn
Copy link
Contributor

@okamsn okamsn commented Jul 1, 2022

Add a hook to run in commands associated with candidates, such as
vertico-insert and vertico-exit. This is for #237, but could be used other things.

From a small amount of testing, I think this change and the below is enough to get candidates from Vertico to Prescient.

  (add-hook 'vertico-candidate-inserted-hook
          (defun my-vertico-prescient-remeber ()
            (prescient-remember (vertico--candidate))))

Selectrum has the hook variables selectrum-candidate-inserted-hook and selectrum-candidate-selected-hook, but I saw that the command vertico-exit uses vertico-insert, so I've left it at one hook variable.

Would you like any changes? I thought about passing the candidate to the functions in the hook variable, but thought that might complicate other possible uses.

Add a hook to run in commands associated with candidates, such as
`vertico-insert` and `vertico-exit`.
@minad
Copy link
Owner

minad commented Jul 2, 2022

Is there an advantage of adding a hook? An advice should work too, which would require changes of Vertico and doesn't require more code on the side of Prescient.

(advice-add #'vertico-insert :after
            (lambda () (prescient-remember (vertico--candidate))))

If there are many uses cases for a hook or if it is likely that the hook is configured in various ways, the addition of such a hook variable seems justified.

@okamsn
Copy link
Contributor Author

okamsn commented Jul 2, 2022

Selectrum-Prescient records the candidate when submitted with RET or inserted with TAB, including for the arbitrary input not in the candidate list. If I'm reading the code of vertico-exit correctly, it only calls vertico-insert when exiting with a candidate from the candidate list.

I suppose vertico-exit and vertico-insert could both be advised, but then the advice would be run twice in some cases, yes? That would affect the frequency count of Prescient. Do you see a way to avoid this for advice? I chose a hook to try to avoid depending on how functions are defined internally.

@minad
Copy link
Owner

minad commented Jul 2, 2022

I think it should be sufficient to just advise vertico-insert. Then the candidate will be recorded in all cases when a candidate is inserted (either via TAB or via RET). The only case which will be missed is when you submit the input directly (M-RET), but this is a rare case and usually not relevant for Prescient sorting, since you only submit the input if it differs from all available candidates.

If you have a working Prescient setup, could you please add it to the Vertico wiki, such that we can point users there, who are interested in Prescient? We can also document the setup in the README later on. Is there progress on making Prescient a completion style or do you rather prefer to strip down Prescient such that it is only responsible for sorting, relying on Orderless otherwise? I mentioned this as a possible way forward in #237.

@okamsn
Copy link
Contributor Author

okamsn commented Jul 9, 2022

I think it should be sufficient to just advise vertico-insert.

Understood.

If you have a working Prescient setup, could you please add it to the Vertico wiki, such that we can point users there, who are interested in Prescient?

Done. Please edit as you see fit.

Is there progress on making Prescient a completion style or do you rather prefer to strip down Prescient such that it is only responsible for sorting, relying on Orderless otherwise?

I haven't tried working on Prescient in a few months. I have no time limit for it, since I'm not affected by the limitations from it not being a completion style. I would like to make it a completion style at some point to support prescient-sort-full-matches-first, but I think that I'm in a minority in using that setting. I would think that Orderless is more than enough for most users.

Sorting is implemented separately from filtering in Prescient, so the sorting part should be fine to use in Vertico already. Whether prescient.el gets pared down, split up, or something else is @raxod502's decision. I have no opinion on it myself.

@okamsn okamsn closed this Jul 9, 2022
@minad
Copy link
Owner

minad commented Jul 9, 2022

Great, thanks!

@okamsn okamsn deleted the selection-hooks branch July 11, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants