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

Create vertico-prescient-mode and corfu-prescient-mode. #131

Merged
merged 1 commit into from
Nov 12, 2022

Conversation

okamsn
Copy link
Contributor

@okamsn okamsn commented Oct 2, 2022

  • Create files vertico-prescient.el and corfu-prescient.el.
  • Add stub definitions for Vertico and Corfu.
  • Make the selectrum-prescient-toggle-* commands more generic and move them to prescient.el.
    • Add variable prescient--toggle-refresh-functions and function prescient--toggle-refresh. The toggle commands run this hook to refresh the UI. Integration packages add functions to this hook.
    • Extract out new macro prescient-create-toggling-command from the existing prescient-create-and-bind-toggling-command.
  • Create a prescient-completion-mode. This minor mode will handle the completion settings shared by vertico-prescient-mode and corfu-prescient-mode.

TODO

  • Should the toggling command be bound in the Corfu map? Do they make sense there?
    • Thinking no. They would need to work differently for Corfu, and adjusting the filtering on the fly suggests a more involved (for lack of a better term) completion session than how Corfu is probably used. To switch to the minibuffer, there is already a command given on the Corfu GitHub page.

@raxod502
Copy link
Member

raxod502 commented Oct 5, 2022

This is great, thank you. I will migrate Radian to use it after merge.

@okamsn okamsn marked this pull request as ready for review October 6, 2022 00:33
@okamsn okamsn force-pushed the vert-pres branch 2 times, most recently from 80f5aa6 to 448b086 Compare October 6, 2022 01:38
@okamsn
Copy link
Contributor Author

okamsn commented Oct 6, 2022

@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.

@minad
Copy link

minad commented Oct 6, 2022

@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.

@minad
Copy link

minad commented Oct 10, 2022

@okamsn Any update here?

@okamsn
Copy link
Contributor Author

okamsn commented Oct 11, 2022

@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?

@minad
Copy link

minad commented Oct 11, 2022

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.

@okamsn
Copy link
Contributor Author

okamsn commented Oct 12, 2022

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.

@minad
Copy link

minad commented Oct 12, 2022

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

@raxod502
Copy link
Member

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.

@okamsn
Copy link
Contributor Author

okamsn commented Oct 29, 2022

I think that's everything. Are there any final comments/requests before I merge this?

@minad
Copy link

minad commented Oct 30, 2022

@okamsn This looks great! Thanks for the cleanup, such that the code is shared between the different modes.

README.md Outdated Show resolved Hide resolved
corfu-prescient.el Outdated Show resolved Hide resolved
prescient.el Outdated Show resolved Hide resolved
@minad
Copy link

minad commented Oct 30, 2022

@okamsn I went over the code and added a few comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
prescient.el Show resolved Hide resolved
@minad
Copy link

minad commented Nov 12, 2022

@okamsn Is this ready?

@okamsn
Copy link
Contributor Author

okamsn commented Nov 12, 2022

@okamsn Is this ready?

I think so, unless there are any further requests/comments.

@minad
Copy link

minad commented Nov 12, 2022

Okay. Looks good to me!

- Create files `vertico-prescient.el` and `corfu-prescient.el`.
- Add stub definitions for Vertico and Corfu.
- Make the `selectrum-prescient-toggle-*` commands more generic
  and move them to `prescient.el`.
  - Add variable `prescient--toggle-refresh-functions` and function
    `prescient--toggle-refresh`. The toggle commands run this hook to
    refresh the UI. Integration packages add functions to this hook.
- Add variables `prescient--completion-recommended-styles`
  and `prescient--completion-recommended-overrides`, which are the
  default values for the completion settings used by the integration
  modes.
- Candidates are remembered using `substring-no-properties`.
@okamsn okamsn merged commit fbb4dc6 into radian-software:main Nov 12, 2022
@minad
Copy link

minad commented Nov 12, 2022

@okamsn Thanks! Can you please also update the Vertico migration guide accordingly?

@okamsn
Copy link
Contributor Author

okamsn commented Nov 13, 2022

@okamsn Thanks! Can you please also update the Vertico migration guide accordingly?

I will do this once the package becomes available on MELPA.

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.

3 participants