doom-neovim / doom-nvim

A Neovim configuration for the advanced martian hacker
GNU General Public License v2.0
1.01k stars 107 forks source link

Feature request: better keybindings management #152

Closed NTBBloodbath closed 2 years ago

NTBBloodbath commented 3 years ago

As discussed here, we could make an improvement to our current keybindings setup, making them more extensible and easier to maintain.

I'm currently making a proof of concept implementation locally that follows this syntax (inspired by nest.nvim)

{
  {
    mode, mode_options
    {
      {
        category,
        {
          { lhs, rhs, options, description },
        },
      },
    },
  },
}

Where

IMPORTANT: Syntax changes proposals are welcome!

We could also extend the keybinds fields to cover the nvim-mapper plugin and maybe use this new way to create keybinds in the doom_config.lua file to cover all the keybinds plugins and let the user declare his keybinds in a more idiomatic and declarative way to rule them all.


Here is an attached demo of how does it looks like

image

CURRENT LIMITATIONS

  1. nested which-key menu entries (e.g. <leader>cdr) aren't being recognized correctly.
connorgmeehan commented 3 years ago

The nest.nvim api seems intuitive and the library itself seems to be quite popular. I think it'd be good to extend something that already exists to provide the which-key naming support than create a new thing. I had a play around with nest.nvim and it lets me do something like this without breaking the keymaps support. Then doom-whichkey could load the mappings and pass the names.

    -- Prefix  every nested keymap with <leader>
    { '<leader>', {
        -- Prefix every nested keymap with f (meaning actually <leader>f here)
        { 'f', name = '+Find', {
            { 'f', '<cmd>Telescope find_files<cr>', 'Files' },
            -- This will actually map <leader>fl
            { 'l', '<cmd>Telescope live_grep<cr>', 'Grep' },
            -- Prefix every nested keymap with g (meaning actually <leader>fg here)
            { 'g', name = '+Git', {
                { 'b', '<cmd>Telescope git_branches<cr>', 'Git Branches' },
                -- This will actually map <leader>fgc
                { 'c', '<cmd>Telescope git_commits<cr>', 'Commits' },
                { 's', '<cmd>Telescope git_status<cr>', 'Status' },
            }},
        }},

Names can be either the third index of the table for the keymap or An optional name = 'string' property in each node of the graph

I don't love the idea of bending a library to how it wasn't meant to bend. Another option could be waiting for nest.nvim to implement its own description API https://github.com/LionC/nest.nvim/issues/4. There's already a PR that adds this functionality https://github.com/LionC/nest.nvim/pull/17.
I do think it should also have a description = 'Blah' api as well because In the case of a keymap group like { 'f', name = '+Find', { in the example above. Putting the description right next to the keymap rather than the end of the block makes more sense to me.

NTBBloodbath commented 3 years ago

Hi, yeah this is still a proof of concept, not our final api tho and can be totally discarded.

The reason of why I'm "reinventing" the wheel (thing that I hate so much) is because making the editor keybinds unusable until the user downloads a plugin makes no sense to me and afaik nest.nvim doesn't provide what we want to rule them all yet.

And yeah we can embed a plugin in doom (I think it's possible), however submodules can be hell and additionally to first two points, what if nest.nvim gets breaking changes? Then the whole keybinds setup will be unusable and this is not a big deal IMO.

An optional name = 'string' property in each node of the graph

I think this could be the best approach here to improve readability and parsing :thinking:

connorgmeehan commented 3 years ago

Yeah agreed, submodules are a pain, making users download a package is also a pain. Also, even if we did pin packer/submodule to a commit to avoid breaking changes, if the Nest API progresses in a way that's incompatible with the API that works for doom-nvim it'd put doom-nvim users in a confusing situation.

These are just personal things that I'd like to see in the API so feel free to disagree:

LionC commented 3 years ago

Hey - author of nest here :-) An own description API will definitely land, the question is more around how that should behave exactly. Any specific input is greatly appreciated - nest is meant to be minimal and very ergonomic to be a good candidate for pre-made neovim configs.

Would you mind elaborating on what you would exactly need to use nest in Doom? That would be very valuable feedback.

NTBBloodbath commented 3 years ago

Hey mate, this is very impressive to me, being honest I never thought that you were going to reply here lol.

First of all nest looks really cool and wow it's a really good work, the way to define keybinds is very clean and understandable, I love it!

An own description API will definitely land, the question is more around how that should behave exactly. Any specific input is greatly appreciated

I've been reading the issues related to this (thanks to @connorgmeehan) and I think a description field in the keybinds declaration would be pretty cool, specially for later getting them and use these descriptions to in a future create an integration with which-key as I've seen in LionC/nest.nvim#17. So by example the user could optionally do something like this

local nest = require('nest')

nest.applyKeymaps {
    -- Prefix  every nested keymap with <leader>
    { '<leader>', {
        -- Lua functions can be right side values instead of key sequences
        { 'l', {
            { 'h', vim.lsp.buf.hover, description = "Open hover documentation" },
        }},
    }},
}

and then enable the which-key integration to get an almost automatized setup for their keybindings, I think this could be a killer feature for a ton of people because at least in my opinion it could be a tedious work to modify two files to properly add your new keybinds.

nest is meant to be minimal and very ergonomic to be a good candidate for pre-made neovim configs.

Yeah agreed, and does exactly what it promises and this is really good!

Would you mind elaborating on what you would exactly need to use nest in Doom? That would be very valuable feedback.

I would basically need just the PR to add descriptions to keybinds, maybe a name or category field like the one that Connor mentioned here and a which-key integration because of the reason that I told before regarding to the modification of keybinds. I want to have a more maintainable and enjoyable keybindings setup for my end users and obviously to me and Doom's contributors and I think nest could be our solution in a hopefully near future :)

The name / category field will be really useful to automatize even more a possible which-key integration because every keybinds group in which-key has a name field, e.g. +buffers.

This is not directly related to nest but still, there will be an internal issue in Doom related to lazytanuki/nvim-mapper plugin if I start using nest because nvim-mapper uses his own API around vim.api.nvim_set_keymap function. Since nest has an objective and this issue is related only to Doom I don't see why should nest change something in the internal API just to cover all doom needs. I think I could make a nvim-mapper fork and make it minimal but also adapt it to be compatible with nest in a future.

Just a last thing, and sorry for this bible haha but as the nest author and maintainer how stable do you think that will be nest? Talking about possible breaking changes in the public API in a future since this is a thing that afraids me as you can see in one of my comments. Without having something else to say, hope you will have a great day mate!

Regards

LionC commented 3 years ago

Noted, thank you for the elaborate post! Working to get an API shipped that allows to name and tag keybinds - sounds like that would be sufficient for your case :-)

NTBBloodbath commented 3 years ago

Thank you, yeah it should be enough for Doom to start a migration :)

connorgmeehan commented 3 years ago

Sweet this is exciting! As for integration into doom-nvim what would the best way to integrate be? Would we add it as a packer dependency it in modules/init.lua or as a submodule (could add git submodules update to :DoomUpdate and keep it pinned to a commit).

NTBBloodbath commented 3 years ago

Maybe we could add it as an obligatory dependency in our packer setup (cannot be disabled), after all Doom first launch automatically installs the required and enabled by default plugins and since I hate submodules maintenance it's a good idea IMO.

But because keybinds are essential we can do just exactly what you've mentioned, pin nest to a certain commit and periodically update it every new Doom release.

And of course I'll add a notice in #113 and wait at least 1 week to release Doom v3.2 so people will know that in the incoming Doom release keybinds will be "broken" after updating. Anyway Doom will automatically install nest on launch after updating Doom so it will be just update, install nest and relaunch neovim :)

LionC commented 3 years ago

I forgot to mention that in my last comment and you asked: I will not make breaking changes to nest's API ever, as there is no established system to manage breaking changes in the nvim plugin ecosystem.

So as long as you use the documented, intended API, you should not need to pin to a commit.

NTBBloodbath commented 3 years ago

That's really great news, thanks for clarifying that doubt :heart:

caligian commented 2 years ago

Ok. I can provide some functions which provide these keybinding features -

  1. Events can be attached as a precondition. This is similar to emacs' language-keymap hook. Multiple events can be defined along with multiple patterns like you would in an autocmd command.
  2. By default, all keybindings are added to GlobalHook augroup. There are LanguageHook augroups present and will be used if users define it. Multiple augroups can be used.
  3. Multiple lua functions, vimscript commands and lua function with names can be provided. If no name is provided then a randomly generated name will be added to the global table _G else the user provided name will be used.
  4. leader and localleader prefixes can be provided a name. For example, leader is pressed 'f' will display 'File operations' in which-key window.
  5. Help string for leader and localleader keybindings can be provided which will displayed by which-key.
  6. Multiple keybindings can be declared in one go.
  7. Recursive or nonrecursive mappings can be made
  8. Commands can be made repeatable
  9. Modes can be defined by a single letter.

Eg. funcs.kbd({{{{'silent', 'buffer'}, '<leader>ff'}, {'*py,*rb,*lua', 'BufEnter'}, {'n', 'v'}, true, telescope.builtin.find_files, {help='Show files in telescope'}}})

This is enough?

Bugs: Lua function or vimscript command output cannot be used to insert text. <expr> is useless :(

connorgmeehan commented 2 years ago

Hey Caligan!

I've been trying to figure out the best way to add keybindings for buffer types. I currently have a fork of nest.nvim that integrates with whichkey and nvim-mapper that we're planning to integrate into doom-nvim for the next release. Afaik it addresses all of your points except for filetype bindings.

Currently it is possible to do per-buffer keybindings with nest.nvim. I'm not sure if we should extend nest.nvim to add filetype keybindings or set autocommands outside of nest.nvim that will trigger keybinds to the current buffer. I'm leaning towards not including it in nest.nvim as I'm pretty sure it's supposed to be a minimal plugin and I hope to get my fork merged sometime in the new year.

caligian commented 2 years ago

Repeatable means they can be executed with '.' in normal mode.

I think it is important to add autocmd support because the possibilities just increase manifold.

In emacs, every package has a 'hook'. For example, the package python.el that provides everything related to python filetypes. You can just hook stuff to python-hook and that will automatically get executed when python.el is loaded.

There are two vim equivalents - autocmd with augroup and filetype plugins. The problem is that filetype plugins can lead to a config getting scattered between several files if there is a same keybinding for several filetypes. With autocmd, the problem is effectively solved. However, the users won't be able to use lua functionality without manually defining the function in the global table and then making an autocmd that executes that function. So, the best approach is to automate everything and make these keybindings only execute when the package is loaded and autocmd condition is fulfilled.

While nest.nvim does everything that I have talked about except autocmd support, I don't see how it compares up to doom-emacs and emacs in general which allows you to set same keybindings with different commands for different filetypes. While you may prefer using ftplugin, I prefer not using ftplugin and putting all my config close to the package they are configuring.

connorgmeehan commented 2 years ago

Ahh right of course and yeah I definitely see the benefit of filetype/autocommand keybindings, it's something we will include but I'm just not sure if the autocommand logic should go in nest.nvim or in doom-nvim.

Ok. I can provide some functions which provide these keybinding features -

Could please you share the code for this for this function? The logic might actually be fine to go into nest.nvim but I'll probably implement this in a seperate PR.

Cheers!

connorgmeehan commented 2 years ago

This is resolved in https://github.com/NTBBloodbath/doom-nvim/tree/next via a custom nest.nvim forked syntax for expressing keymaps, it integrates with nvim-mapper and whichkey.