nvim-neorocks / nvim-best-practices

Collection of DOs and DON'Ts for modern Neovim Lua plugin development
Creative Commons Zero v1.0 Universal
237 stars 3 forks source link

idea: document the best practices for plugins users to manage keybindings #12

Closed mikavilpas closed 2 weeks ago

mikavilpas commented 1 month ago

Hi again, hope you don't mind me raising so many issues 😅.

I'm looking for a way to document / redesign how users could manage keybindings in my project. I liked the suggestion of avoiding a fancy DSL you have listed in the best practices.

But what is a good alternative? In my plugin, I would like to support the following cases:

Maybe this is common knowledge, but I wonder if these could be laid out in the documentation.

Thanks, and have an awesome day!

mrcjkb commented 1 month ago

I'm having trouble understanding this (it's late where I am). Do you have some more concrete examples?

the user wants to change an existing keybinding's action

If the plugin doesn't automatically create any keymaps (which I advise against), but instead leaves defining keymaps up to the user (possibly providing <Plug> mappings), there shoudn't be a need to provide a way to change existing keymaps.

the user wants to change an existing keybinding's key (the key that will trigger the action)

This is what <Plug> mappings are for. As a plugin author, you create a <Plug> mapping (or a Lua function if it's something with lots of options), which defines the action, the keymap options and the modes it can be used in, etc. And you leave the actual keys that bind to the mapping up to the user to choose. The user just has to create a keymap with <Plug>MyAction on the rhs.

the user wants to disable an existing keybinding

Should be a no-op if you don't automatically create any keymaps.

mikavilpas commented 1 month ago

Here's a concrete example:

My plugin creates a floating terminal window for a terminal file manager. The terminal window needs custom keybindings to enable neovim specific functionality (such as opening the selected file in a new split with <c-v>)- in these cases the key should not be sent to the underlying terminal application, but should be completely handled in neovim instead.

The current implementation is run after the floating terminal window has been created, and the keymappings are only created for that specific terminal buffer:

function M.default_set_keymappings_function(yazi_buffer, config)
  vim.keymap.set({ 't' }, '<c-v>', function()
    keybinding_helpers.open_file_in_vertical_split(config)
  end, { buffer = yazi_buffer })

  vim.keymap.set({ 't' }, '<c-x>', function()
    keybinding_helpers.open_file_in_horizontal_split(config)
  end, { buffer = yazi_buffer })

  -- ...
end

The current implementation isn't very nice for users that want to partially customize the mappings.

I could solve it by creating a DSL such as this:

---@type table<string, fun(config: YaziConfig): nil>
local normal_mode_keybindings = {
  ['<c-v>'] = function(config)
    keybinding_helpers.open_file_in_vertical_split(config)
  end,
  ['<c-x>'] = function(config)
    keybinding_helpers.open_file_in_horizontal_split(config)
  end,
}

Now the user can customize the keybindings by mutating this table (or a copy of it):

This guide advises against using this kind of DSL, and I think it's much easier for plugin authors not to do it (so I agree). But it's also convenient for users to get a "batteries included" experience.

Sorry for the long post. I'm wondering if it makes any sense, and if you'd have any advice.

mrcjkb commented 1 month ago

I see. In the guide, I mention not to create default keymaps unless they are not controversial. If they're specific to a certain buffer (in this case buffer = yazi_buffer) it's probably not controversial, as you will be unlikely to conflict with user-defined keymaps.

To replace a DSL, here's a way you could potentially allow users to override the default keypaps using <Plug> mappings:

It could look something like this:

-- Search for user keymaps
local user_keymaps = {
  open_vertical_split = false,
  open_horizontal_split = false,
}

for _ , map in pairs(vim.api.nvim_get_keymap("t")) do
  if type(map.rhs) == "string" and map.rhs:find("YaziOpenVertical") ~= nil then
    user_keymaps.open_vertical_split = true
  end
  if type(map.rhs) == "string" and map.rhs:find("YaziOpenHorizontal") ~= nil then
    user_keymaps.open_horizontal_split = true
  end
  -- You may want to add a check for breaking out of the loop early if everything is set
end

if user_keymaps.open_vertical_split then
  -- User has set a keymap. Define a <Plug> mapping.
  -- These <Plug> mappings expose the functionality only for mode "t"
  -- and only for the yazi_buffer.
  -- This means that users cannot create YaziOpen* keymaps for normal mode,
  -- or other buffers
  vim.keymap.set("t", "<Plug>(YaziOpenVertical)", keybinding_helpers.open_file_in_vertical_split, { buffer = yazi_buffer })
else
  -- User has not set a keymap. Define a default one.
  vim.keymap.set("t", "<c-v>", keybinding_helpers.open_file_in_vertical_split, { buffer = yazi_buffer })
end

if user_keymaps.open_horizontal_split then
  vim.keymap.set("t", "<Plug>(YaziOpenHorizontal)", keybinding_helpers.open_file_in_horizontal_split, { buffer = yazi_buffer })
else
  vim.keymap.set("t", "<c-x>", keybinding_helpers.open_file_in_horizontal_split, { buffer = yazi_buffer })
end

In the user's config, to override a default keymap, all they need to do is:

vim.keymap.set("t", "<leader>vv", "<Plug>(YaziOpenVertical)")

(they don't need to worry about things like buffer = yazi_buffer, because your <Plug> mapping handles that.