Skip to content

Vertico improvements: theming, bugfixes, new bindings, adaptations to upstream changes #5299

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

Merged
merged 11 commits into from
Jul 31, 2021

Conversation

iyefrat
Copy link
Member

@iyefrat iyefrat commented Jul 26, 2021

This PR contains:

  • bumps for :completion vertico and :ui doom
  • teco's recent theming improvements for marginalia and orderless
  • better keys for consult-crm
  • a fix for <leader> c j since it used cmd! instead of cmd!!
  • binding the new consult-line-multi to <leader s B> (the prefix argument makes it search all buffers, without it it just searches buffers of the current project)
  • usage of the recently introduced new embark indicator format
  • add marginalia annotations to functions that don't have them
  • advise the marginalia annotations for the project-file category to use projectile
  • a bugfix for consult-recent-file
  • bugfixes for +vertico/find-file-in
  • replacing an obsolete embark function
    cc: @tecosaur @elken @minad

@iyefrat iyefrat requested a review from hlissner as a code owner July 26, 2021 13:04
@iyefrat iyefrat marked this pull request as draft July 26, 2021 13:06
@hlissner hlissner added is:feature Adds or requests new features, or extends existing ones is:update An effort to catch up with changes made elsewhere module:completion/vertico Pertains to Doom's :completion vertico module labels Jul 26, 2021
@iyefrat iyefrat force-pushed the vertico-improvements branch 4 times, most recently from 29354c5 to f827beb Compare July 27, 2021 12:40
@iyefrat iyefrat changed the title [DRAFT] Vertico theme and crm improvements Vertico improvements: theming, crm, bugfix,<leader> s B Jul 27, 2021
@iyefrat iyefrat changed the title Vertico improvements: theming, crm, bugfix,<leader> s B Vertico improvements: theming, crm, bugfix, <leader> s B Jul 27, 2021
@iyefrat iyefrat marked this pull request as ready for review July 27, 2021 12:43
@iyefrat iyefrat force-pushed the vertico-improvements branch from f827beb to c448928 Compare July 27, 2021 16:19
@iyefrat iyefrat marked this pull request as draft July 27, 2021 18:12
@iyefrat
Copy link
Member Author

iyefrat commented Jul 27, 2021

I'm converting this back to draft because there are some cool upstream embark changes that would be a shame to wait until the next bump to get, but require a bit of manual work on my part to fix the which key intergration. Should be ready soonish.

@iyefrat iyefrat force-pushed the vertico-improvements branch from c448928 to 554e530 Compare July 27, 2021 20:12
@iyefrat iyefrat changed the title Vertico improvements: theming, crm, bugfix, <leader> s B Vertico improvements: theming, crm, bugfix, <leader> s B, new embark indicator Jul 27, 2021
@iyefrat iyefrat marked this pull request as ready for review July 27, 2021 20:13
@iyefrat iyefrat force-pushed the vertico-improvements branch from 554e530 to c15e1cd Compare July 27, 2021 20:17
(which-key--show-keymap "Embark" map nil nil 'no-paging)
#'which-key--hide-popup-ignore-command)
embark-become-indicator embark-action-indicator)
(setq embark-indicator #'+vertico/embark-which-key-indicator)
Copy link
Member Author

@iyefrat iyefrat Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there is an interesting alternative to the which-key indicator, the recently introduced verbose indicator (it's what's used by default). it looks like this:
image

this is more useful than which-key information-wise, but breaks with doom using which key everywhere else, and this information can also be reached by pressing C-h to start a vertico filtering session.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Hey, you should send that screenshot to Omar, who needs a nice screenshot!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh I think that screenshots on the embark readme should probably have things like a nonarbitrary aspect ratio and not cutting off lines of text at the bottom. Also they should probably be taken from a reasonably default config, e.g. not having the doom modeline, or icons.

Although this reminds me, I should probably add a screenshot to all-the-icons-completion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's different about the new which-key one, other than that submenus work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing that I can notice to be honest

Copy link

@oantolin oantolin Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main point of having a new which-key indicator is that the indicator protocol changed so the old one won't work anymore! But also, the new one displays (in the which-key help message) whether you are using embark-act or embark-become, and also shows the current target and an ellipsis when there are further targets you can cycle to.

@iyefrat iyefrat force-pushed the vertico-improvements branch 2 times, most recently from 2f848b2 to 9d2131b Compare July 28, 2021 13:49
@iyefrat
Copy link
Member Author

iyefrat commented Jul 28, 2021

@hlissner also note 9d2131b, the lazy loading of recentf-mode introduced in 97048e2 had the side effect having consult previews on when using SPC f r, due to the fact that consult-customize works on named commands. This fixes that using advice, hopefully that's fine.

@iyefrat iyefrat changed the title Vertico improvements: theming, crm, bugfix, <leader> s B, new embark indicator Vertico improvements: theming, bugfixes, new bindings, adaptations to upstream changes Jul 28, 2021
@numkem
Copy link

numkem commented Jul 28, 2021

Is it possible to ad the icons to projectile-find-file?

Thanks!

@iyefrat
Copy link
Member Author

iyefrat commented Jul 28, 2021

@numkem yeah I'll add this to all-the-icons-completion and bump it, good catch

Edit: fixed

@iyefrat iyefrat force-pushed the vertico-improvements branch 3 times, most recently from cf780b0 to 624bd9d Compare July 28, 2021 18:05
@iyefrat iyefrat force-pushed the vertico-improvements branch from 007266b to ec78473 Compare July 28, 2021 21:13
@iyefrat iyefrat marked this pull request as draft July 28, 2021 21:23
@iyefrat iyefrat force-pushed the vertico-improvements branch from ec78473 to d4396ac Compare July 28, 2021 21:29
@iyefrat iyefrat marked this pull request as ready for review July 28, 2021 21:30
@iyefrat iyefrat force-pushed the vertico-improvements branch 3 times, most recently from c4d9609 to 76bc719 Compare July 29, 2021 17:37
@iyefrat iyefrat force-pushed the vertico-improvements branch from ec833fe to ef277b3 Compare July 29, 2021 23:13
Copy link
Member

@hlissner hlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one correction and I think this is ready to merge.

iyefrat added 9 commits July 31, 2021 10:41
- needs to be `cmd!!` for the prefix argument
- use descriptive quoted symbol instead of `'(4)` for prefix arg
- use `doom-project-root` in the annotation category
  `project-file` (marginalia uses `project.el`)
- annotate more functions, alphabetize list
Turning on `recentf-mode` in a `cmd!` (introduced in 97048e2) means that
the `consult-customize` that turns off the previews doesn't work. This
is fixed by advising the function instead.
- require `consult` (`consult--directory-prompt` isn't autoloaded)
- set category to `'file`
@iyefrat iyefrat force-pushed the vertico-improvements branch from ef277b3 to aac2c2d Compare July 31, 2021 07:49
- replace obsolete `embark-default-action` with `embark-dwim`
- add `defvar` for `embark-quiet-after-action` to fix lexical var bug
@iyefrat iyefrat force-pushed the vertico-improvements branch from aac2c2d to 3af07d2 Compare July 31, 2021 07:51
gagbo/consult-lsp@c882749 -> gagbo/consult-lsp@e8a50f2

compatibility fix with new versions of consult
@hlissner hlissner merged commit e65e2ea into doomemacs:develop Jul 31, 2021
@hlissner
Copy link
Member

Looks good to me. Great work! And thanks again!

@iyefrat iyefrat deleted the vertico-improvements branch August 6, 2021 23:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2021
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 is:update An effort to catch up with changes made elsewhere module:completion/vertico Pertains to Doom's :completion vertico module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants