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

Use separator character for boundary quit logic and prefix sort #120

Merged
merged 17 commits into from
Feb 7, 2022

Conversation

jdtsmith
Copy link
Collaborator

@jdtsmith jdtsmith commented Feb 6, 2022

Implement a component separator character and associated insertion command (corfu-insert-separator-char), used to inhibit quitting at completion boundaries (see #119). This character is also removed from prefix sorting, superseding #118.

This is useful for multi-component completion styles such as orderless. A convenient key binding for corfu-insert-separator-char such as M-SPC can be used to insert the initial completion component separator.

Obsoletes corfu-quit-at-boundary (for v0.19). Setup documentation is provided.

@minad
Copy link
Owner

minad commented Feb 6, 2022

Very good, I will try this in the next few days. So if we bind SPC to corfu-insert-separator we get the current quit-at-boundary behavior? If we bind M-SPC instead we get the enhanced behavior? I guess we could bind M-SPC by default.

README.org Outdated Show resolved Hide resolved
README.org Outdated Show resolved Hide resolved
corfu.el Outdated Show resolved Hide resolved
corfu.el Outdated Show resolved Hide resolved
corfu.el Outdated Show resolved Hide resolved
corfu.el Outdated Show resolved Hide resolved
corfu.el Show resolved Hide resolved
@minad
Copy link
Owner

minad commented Feb 6, 2022

I gave it a quick review. This looks very good, nice and simple. There are only a few minor details. Then there is the question if we want to support disabling the separator character completely - by setting it to ?\0 or nil.

@jdtsmith
Copy link
Collaborator Author

jdtsmith commented Feb 6, 2022

So if we bind SPC to corfu-insert-separator we get the current quit-at-boundary behavior?

I suppose that's right. Or, if you (or the user) binds nothing at all to corfu-insert-separator-char, you will simply fall through to the predicate boundary check. So the default could become the less intrusive option.

if we want to support disabling the separator character completely

I tried to think of a real usage case for disabling the behavior of the separator. Unless you invoke M-SPC, the separator is benign. The only thing I could come up with:

  1. You do not use orderless or a multi-component completion style.
  2. Your mode's CAPF happily includes the separator char within the completion boundary (I suppose more likely if it's not space).
  3. You want other non-word chars to quit completion (e.q. punctuation like , or =).

I don't know if 2 is occurring anywhere in the wild for the default space separator. But allowing this to be nil is perhaps easy insurance for any cases we are missing. I'd still recommend keeping space as the default though.

Also, regarding the binding, should we bind it ourselves or let the user pick? I've also tried S-SPACE which works well.

@minad
Copy link
Owner

minad commented Feb 6, 2022

I just want the ability to disable this out of principle, but I agree that having a separator like space (also having it enabled by default) is benign

corfu.el Outdated Show resolved Hide resolved
@minad
Copy link
Owner

minad commented Feb 7, 2022

Also, regarding the binding, should we bind it ourselves or let the user pick? I've also tried S-SPACE which works well.

We should add the binding M-SPC by default. It is easy to unbind this in a user configuration. We also preconfigure separator=SPC.

@minad
Copy link
Owner

minad commented Feb 7, 2022

This looks good, thanks!

@minad minad force-pushed the boundary-separator branch from fe491a5 to 2c5967d Compare February 7, 2022 15:57
@minad minad merged commit 7b71f58 into minad:main Feb 7, 2022
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