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

feat(render): merge keybindings in UI #1332

Closed
wants to merge 1 commit into from

Conversation

har7an
Copy link

@har7an har7an commented Feb 20, 2024

This PR updates the keybindings view in the "hints" section in lazy home.
Activation keys for plugins are now iterated in sorted order (by keys) and
common prefixes, if present, are shown instead of all the individual
keybindings.

Given a config like this:

return {
    {
        'rcarriga/nvim-notify',
        lazy = true,
        keys = {
            { "<leader>N",  desc = "+notifications" },
            { "<leader>Ny", "<Cmd>Notifications<CR>", desc = "history (plain)" },
            { "<leader>Nd", function() require("notify").dismiss() end, desc = "dismiss" },
            { "<leader>Np", function() require("notify").pending() end, desc = "show pending" },
        },
    },
}

The UI entry in the Not Loaded plugins now looks like this:

   ○ nvim-notify  <leader>N

instead of previously:

   ○ nvim-notify  <leader>Nd  <leader>Np  <leader>N  <leader>Ny

As a side-effect, this ensures that, even if no "common prefix" exists, keys
are always shown in sorted order.

Additional remarks

Compound keybindings

I assume that the current implementation fails when "Ctrl"-based keybindings
are mixed with anything that has "C" as a prefix, for example. I'm not sure
about the exact notation of Ctrl-/Alt-keybindings in nvim because I never use
them, but a quick test with this setup:

        keys = {
            { "<leader>C",  desc = "+notifications" },
            { "<leader>Cy", "<Cmd>Notifications<CR>", desc = "history (plain)" },
            { "<leader>C-d", function() require("notify").dismiss() end, desc = "dismiss" },
            { "<leader>Cp", function() require("notify").pending() end, desc = "show pending" },
        },

shortens the whole group to <leader>C in the UI, where it should probably
show <leader>C <leader>C-d instead. In a similar fashion, I assume that a
scenario like this fails as well:

        keys = {
            -- Plain "C" with plain "-"
            { "<leader>C-",  desc = "+notifications" },
            { "<leader>C-y", "<Cmd>Notifications<CR>", desc = "history (plain)" },
            { "<leader>C-d", function() require("notify").dismiss() end, desc = "dismiss" },
            { "<leader>C-p", function() require("notify").pending() end, desc = "show pending" },
        },

although I'm unsure whether this is a realistic scenario to consider here. Is
there something like an iterator over individual/compound keys in the raw data?
Or do I have to check whether C-? is part of a compound binding myself? If
so, do you know of any other compound bindings I should be aware of?

which-key integration

Users of which-key will probably not benefit from this change. That is
because keybinding groups must be registered in which-key directly (so the
"group"-prefix is picked up correctly), so lazy doesn't know about them. Since
keybindings can only be performed once, we cannot just "copy" the which-key
group assignment into lazy just for the UI.

I've opened a PR for which-key that solves this inconvenience, in case
you're interested.

Copy link
Contributor

github-actions bot commented Jul 6, 2024

This PR is stale because it has been open 60 days with no activity.

@github-actions github-actions bot added the stale label Jul 6, 2024
@har7an har7an force-pushed the feature/merge-keybindings branch from 0e6ce41 to 9f5fe9c Compare July 7, 2024 05:40
@har7an
Copy link
Author

har7an commented Jul 7, 2024

Rebased onto latest main.

@har7an har7an changed the title view/render: Merge keybindings in UI feat(render): Merge keybindings in UI Jul 7, 2024
@har7an har7an force-pushed the feature/merge-keybindings branch from 9f5fe9c to 4903dcb Compare July 7, 2024 05:42
in lazy overview when a common prefix exists.
@har7an har7an changed the title feat(render): Merge keybindings in UI feat(render): merge keybindings in UI Jul 7, 2024
@har7an har7an force-pushed the feature/merge-keybindings branch from 4903dcb to 39b4369 Compare July 7, 2024 05:44
@folke
Copy link
Owner

folke commented Jul 7, 2024

I'm not a fan of this. Thank you anyway though!

@folke folke closed this Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants