Closed okamsn closed 1 year ago
This is great, thank you. I will migrate Radian to use it after merge.
@raxod502 and @minad, do you have any thoughts on these changes? Are there other things that should be configured? For example, completion categories that should be overridden so that they use prescient, or so that they don't fail when trying to use prescient.
@okamsn
Should the toggling command be bound in the Corfu map? Do they make sense there?
I think you should bind them there. Changing the filtering on the fly should work with Corfu.
Regarding the completion-styles - I would make sure that the Vertico prescient mode only applies its overrides in the minibuffer. Likewise the Corfu overrides should be applied only locally in the buffer where corfu-mode is enabled. Then the settings won't interfere.
@okamsn Any update here?
@okamsn Any update here?
@minad: Not yet. I've been busy.
How does one detect that they are in Vertico in the minibuffer, and not, for example in M-:
?
How does one select the buffer of Corfu candidates? Is that still the minibuffer?
Corfu can be enabled in any buffer, in contrast to Vertico which is always in the mini buffer. The candidates are stored in a local variable in the current buffer.
Corfu can be enabled in any buffer, in contrast to Vertico which is always in the mini buffer. The candidates are stored in a local variable in the current buffer.
Better that I ask, what do you think is the best way to have a toggling command's changes to the prescient.el variables only affect Corfu completion for the current completion session?
For Selectrum and Vertico, we just make the variables local to the minibuffer. With how the toggling commands are currently written, the variables are made local to buffer in which Corfu is used and the change persists after the Corfu pop-up is closed.
Take a look at the Corfu source :)
The local variables holding the Corfu state are destroyed during teardown, see:
https://github.com/minad/corfu/blob/99d3a0c8a626db1ff5832b013b2e50352613dd2d/corfu.el#L1096
I haven't looked through in detail, since I think you know what you are doing, but I would be more than happy to offer advice if there is any point that I could help with! I think the questions on Corfu and Vertico I am not knowledgeable to answer.
I think that's everything. Are there any final comments/requests before I merge this?
@okamsn This looks great! Thanks for the cleanup, such that the code is shared between the different modes.
@okamsn I went over the code and added a few comments.
@okamsn Is this ready?
@okamsn Is this ready?
I think so, unless there are any further requests/comments.
Okay. Looks good to me!
@okamsn Thanks! Can you please also update the Vertico migration guide accordingly?
@okamsn Thanks! Can you please also update the Vertico migration guide accordingly?
I will do this once the package becomes available on MELPA.
vertico-prescient.el
andcorfu-prescient.el
.selectrum-prescient-toggle-*
commands more generic and move them toprescient.el
.prescient--toggle-refresh-functions
and functionprescient--toggle-refresh
. The toggle commands run this hook to refresh the UI. Integration packages add functions to this hook.prescient-create-toggling-command
from the existingprescient-create-and-bind-toggling-command
.prescient-completion-mode
. This minor mode will handle the completion settings shared byvertico-prescient-mode
andcorfu-prescient-mode
.TODO