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

module: add :completion vertico #4664

Merged
merged 147 commits into from Jul 25, 2021
Merged

Conversation

edmundmiller
Copy link
Member

@edmundmiller edmundmiller commented Feb 16, 2021

This is a work in progress, to quote the great @mjlbach "be the PR you want to see in the repo".

The only thing I take credit for so far is pinning the packages and copying and pasting @Townk's config ontop of @jethrokuan's PR (#4519) so I'll only take it personally if you rip up the way I pinned the repos.

Please add anything that you find isn't working to TODO.org or PR the fix.

This PR used to be named Add Selectrum Round 2

@bdarcus
Copy link
Contributor

bdarcus commented Feb 17, 2021

I'm a newbie on this ecosystem of packages, but would consult-ripgrep be useful somewhere here?

@edmundmiller
Copy link
Member Author

I'm a newbie on this ecosystem of packages, but would consult-ripgrep be useful somewhere here?

I think that's what +selectrum-file-search is aiming to replace.

@bdarcus
Copy link
Contributor

bdarcus commented Feb 17, 2021

I think that's what +selectrum-file-search is aiming to replace.

OIC; thanks.

@bdarcus bdarcus mentioned this pull request Feb 18, 2021
@bdarcus

This comment has been minimized.

@edmundmiller
Copy link
Member Author

Could you drop a screenshot of the difference? Should just be (setq embark-collect-initial-view-alist ...) away, but what values are you using?

@bdarcus

This comment has been minimized.

@bdarcus

This comment has been minimized.

@minad
Copy link
Contributor

minad commented Feb 21, 2021

When grepping the doom-emacs codebase for ivy-read there seem to be a few other places where a completing-read implementation is missing and there are also a few TODO marks.

@edmundmiller
Copy link
Member Author

@minad I appreciate the review! Few questions on either and I'd love any further input you have.

@jethrokuan
Copy link
Contributor

Why is +selectrum-file-search reverted to consult-ripgrep? Wouldn't it then fail to handle +fuzzy, and also searching the correct directory?

@bdarcus
Copy link
Contributor

bdarcus commented Feb 23, 2021

I've run into a keybinding problem, explained here:

tmalsburg/helm-bibtex#355 (comment)

To summarize:

If one runs embark-act via the C-o keybinding defined here from selectrum, it works; it correctly invokes the command.

But if one from there runs embark-collect-snapshot (S) and go to that embark collect buffer, the same keybinding doesn't work in that context. You have to do M-x embark-act to access the command.

The embark author has this suggestion here.

You should bind some key to run embark-act in the Evil normal state in buffers that are in embark-collect-mode.

FWIW, by default, embark has that bound to a in that case, but it's also accessible via the default binding for embark-act.

@bdarcus
Copy link
Contributor

bdarcus commented Feb 23, 2021

Why is +selectrum-file-search reverted to consult-ripgrep? Wouldn't it then fail to handle +fuzzy, and also searching the correct directory?

See the sub-thread above @jethrokuan:

#4664 (comment)

It's just temporary.

@edmundmiller
Copy link
Member Author

Why is +selectrum-file-search reverted to consult-ripgrep? Wouldn't it then fail to handle +fuzzy, and also searching the correct directory?

I couldn't get your version to work locally the first time I tried it, so I was using a combination of pieces that others had written, but kept not getting anywhere, so I took @minad's advice and cut it down. Anyways, I gave yours another go to confirm, and it appears to be working now.

@jethrokuan
Copy link
Contributor

I didn't really know what I was doing wrt to the this-command override either, but it was working for me then 🤷 . Maybe it's safe to remove that clause.

@bdarcus

This comment has been minimized.

- Use `vertico` as default completion engine
- Drop `selectrum` and `selectrum-prescient` support
- Rename module from `:completion selectrum` to `:completion vertico`
- Rename all files involved
- Do *not* yet rename all the functions, as that messes up git's rename
  detection.
- Rename all functions and variables in the module to reflect the
  namechange (and the irc jump function)
- Add basic configuration instructions
- Add Vertico keybindings
- Improve =C-c C-e= documentation
- Prepare TODO for PR review
It didn't do anything for the `orderless` statement, and we can setup
`embark-consult` correctly without it.
motion preview commands to underlying motion commands.
cleans up shadowed paths automatically
While it only gives one candidate per line, it's more important to have
something that uses the completion UI
- unify `map!` statements when possible
- rename `+vertico--embark-target-package!` to
  `+vertico--embark-target-package` and autoload it
- set `completion-in-region-function` to a wrapper function instead of
  changing it with a hook
- use `:override` advice instead of `fset` for `multi-occur`
- document what `vertico-directory-tidy` does
- move `:init` contents to `:config` when possible
d12frosted/flyspell-correct@4042336 -> d12frosted/flyspell-correct@0035795

flyspell-correct got some completing-read improvements relevant to
completion/vertico
- move TODO.org to discourse (https://discourse.doomemacs.org/t/vertico-module-tasklist/1386)
- update README date
- add @iyefrat as maintainer
(:when (featurep! :completion vertico)
(:after vertico
:map vertico-map
"M-RET" #'vertico-exit-input

This comment was marked as outdated.

@hlissner hlissner merged commit 10a42a9 into doomemacs:develop Jul 25, 2021
@hlissner
Copy link
Member

And bam! Thanks for all your help guys, especially @iyefrat. Looking forward to giving this a spin!

@iyefrat
Copy link
Member

iyefrat commented Jul 25, 2021

I would also like to say thanks to all the people that worked on, reviewed, and beta tested the module. Particularly to @minad, but the full list is too long for me to write out without forgetting someone by accident.

@ttys3
Copy link

ttys3 commented Jul 26, 2021

Finally, this get merged! Cheers

ttys3 added a commit to ttys3/.doom.d that referenced this pull request Jul 26, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2021
@hlissner hlissner changed the title Add Vertico module module: add :completion vertico Aug 3, 2022
@hlissner hlissner added the module:completion/vertico Pertains to Doom's :completion vertico module label Aug 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
is:feature Adds or requests new features, or extends existing ones module:completion/vertico Pertains to Doom's :completion vertico module re:modules Pertains to adding, removing and management of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet