folke / which-key.nvim

💥 Create key bindings that stick. WhichKey helps you remember your Neovim keymaps, by showing available keybindings in a popup as you type.
Apache License 2.0
5.12k stars 163 forks source link

`v3` breaks my alias mappings, `proxy` is confusing #814

Closed tmillr closed 1 week ago

tmillr commented 1 month ago

Did you check docs and existing issues?

Neovim version (nvim -v)

NVIM v0.10.1

Operating system/version

macOS 14.2.1

Describe the bug

Some of my mappings are aliases. I just updated my plugin a few days ago (which I only do every 4-6 weeks), but ever since I updated none of my "aliases" work. Everything was more or less working perfect prior to the update 😕.

Here's an example from my nvim config (some of my lsp mappings):

    local pre = '<Leader>l'

    local lsp_keymaps = {
      [pre] = { name = 'LSP mappings' },

      [pre .. 'h'] = {
        function() lsp.buf.hover() end,
        'Show hover info',
      },

      [pre .. 'r'] = {
        function()
          lsp.buf.rename(nil, {
            filter = function(client) return not client.name:find 'co%W?pilot' end,
          })
        end,
        'Rename symbol',
      },

      [pre .. 'a'] = {
        function()
          local ok, res = pcall(require, 'tmillr.init.lsp.' .. vim.bo.ft)
          if ok and type(res) == 'table' and res.code_action then
            res.code_action()
          else
            lsp.buf.code_action()
          end
        end,
        'Code Action',
      },
    }

    -- Aliases
    for _, x in ipairs { 'h', 'r', 'a' } do
      lsp_keymaps['<Leader>' .. x] = lsp_keymaps[pre .. x]
    end

    wk.register(lsp_keymaps, { buffer = info.buf, silent = false })

These all used to work just fine, but now typing <Leader>h in normal mode does absolutely nothing. However, <Leader>lh does work.

I've also tried the new API, both with and without proxy:

wk.add {
  -- NOTE: I have this group and also a group for <Leader>, but
  --       I'm not sure if that's relevant to this issue or not.
  { '<Leader>l', group = 'LSP' },
  mode = { 'n', 'x' },
  silent = false,

  {
    '<Leader>lh',
    function() lsp.buf.hover() end,
    desc = 'Show hover info',
  },

  {
    '<Leader>lr',
    function()
      lsp.buf.rename(nil, {
        filter = function(client) return not client.name:find 'co%W?pilot' end,
      })
    end,
    desc = 'Rename symbol',
  },

  {
    '<Leader>la',
    function()
      local ok, res = pcall(require, 'tmillr.init.lsp.' .. vim.bo.ft)
      if ok and type(res) == 'table' and res.code_action then
        res.code_action()
      else
        lsp.buf.code_action()
      end
    end,
    desc = 'Code Action',
  },

  -- Shorthand Aliases
  -- NOTE: I've tried these without the `proxy = ` as well (and with `desc` set).
  --       Doesn't work!
  {
    '<Leader>a',
    proxy = '<Leader>la',
  },
  {
    '<Leader>h',
    proxy = '<Leader>lh',
    -- desc = 'Hover',
  },
  {
    '<Leader>r',
    proxy = '<Leader>lr',
    -- desc = 'Rename',
  },
}

that doesn't work either? As you can see, I also tried using desc on the aliases, but doing that (when proxy is set) just implicitly turned each respective mapping into a group for some reason (and also didn't work). I'm having a similar issue with other aliases that I have defined which still use register(). In all cases, there seems to be a group involved (or multiple), so maybe it has something to do with that?

Edit: I've now spent 3-4 hours migrating my mappings to use add() and I still can't get these aliases to work.

Edit: I figured out what was causing the issue, see https://github.com/folke/which-key.nvim/issues/814#issuecomment-2281066946.

Lastly, it isn't clear what the purpose of proxy is (or how/when to use it), and it could probably use better documentation. Current it just says proxy to another mapping, but this doesn't seem to be the case for me.

Steps To Reproduce

TODO

Expected Behavior

It should work like it did before the update...

These are basic mappings, and duplicating the definitions would not be ideal at all.

Health

TODO

Log

TODO

Repro

TODO

tmillr commented 1 month ago

Also, FWIW I kinda preferred the old interface of using key/value assignments as opposed to using array indices now. It not only looked nicer and made more sense visually (i.e. keys = action), but also had the benefit of giving immediate LSP warnings/errors for duplicate mappings in a table (as duplicate keys are implicitly forbidden in a table literal). And being able to specify a prefix (which was also lost with the update, if I'm not mistaken?) made keymaps a bit easier to maintain.

Just my 2 cents.

max397574 commented 1 month ago
vim.env.LAZY_STDPATH = ".repro"
load(vim.fn.system("curl -s https://raw.githubusercontent.com/folke/lazy.nvim/main/bootstrap.lua"))()
vim.g.mapleader = " "

require("lazy.minit").repro({
    spec = {
        {
            "folke/which-key.nvim",
            config = function()
                require("which-key").add({
                    { "<leader>l", group = "lsp" },
                    { "<leader>lh", function() vim.lsp.buf.hover() end, desc = "Hover" },
                    { "<leader>lr", function() vim.lsp.buf.rename() end, desc = "Rename" },
                    { "<leader>la", function() vim.lsp.buf.code_action() end, desc = "Code Action" },
                    { "<leader>h", function() vim.lsp.buf.hover() end, desc = "Hover" },
                    { "<leader>r", function() vim.lsp.buf.rename() end, desc = "Rename" },
                    { "<leader>a", function() vim.lsp.buf.code_action() end, desc = "Code Action" },
                })
            end,
        },
    },
})

works for me

also I don't think that's how proxy is intended to be used in the example of the readme it is used for <c-w> which isn't a mapping itself but just a prefix for mappings

max397574 commented 1 month ago

tbh I have no idea what you want to show in that video I also don't understand what is bad about my comment I tried the solution there and it worked with latest neovim stable.

Also since you seem to care about the verbose map output here is what I get with the minimal repro I sent image

max397574 commented 1 month ago

@tmillr I see you've changed the title you're now describing two separate things the register function was simply removed with v3 so this is expected and not an issue

the behavior of proxy being unclear is not related to this issue and you should perhaps open a new issue or a pull request to make it more clear

as far as I understand it proxy isn't intended to create aliases for mappings but rather for groups like (that's also the current behavior afaict) that's also how I understand it when reading the code. it never executes the proxy mapping but just adds it's submappings to the which-key window

tmillr commented 1 month ago

the register function was simply removed with v3 so this is expected and not an issue

This is false. register() has not been removed and is still defined. If it was not defined, I wouldn't be having issues with it. I assume that it is deprecated. It is highly debatable, and I do not agree, that a deprecated function (or a function kept for backwards-compatibility) is "expected" to be broken.

the behavior of proxy being unclear is not related to this issue

It is related.

you're now describing two separate things

Not exactly. They are both related to my issue, related to aliases, and related to the recent updates and breaking changes.

max397574 commented 1 month ago

This is false. register() has not been removed and is still defined. If it was not defined, I wouldn't be having issues with it. I assume that it is deprecated. It is highly debatable, and I do not agree, that a deprecated function (or a function kept for backwards-compatibility) is "expected" to be broken.

yes I can also call it deprecated that doesn't change anything about the fact that you shouldn't use it anymore and it's no longer supported in v3

the behavior of proxy being unclear is not related to this issue It is related.

no it's not because your issue is simply solved by not using the deprecated function with the correct input proxy is simply something that you tried and didn't work but it isn't supposed to work like this. The issue here is that the description is unclear. And that's not an issue that's relevant here

you're now describing two separate things Not exactly. They are both related to my issue, related to aliases, and related to the recent updates and breaking changes.

you are having an issue because old code no longer works that isn't optimal but that can happen there were breaking changes with a new major release and so you have to adapt (with the solution I provided you which you can just copy paste) or just pin the old version

the other thing is just a documentation issue

tmillr commented 1 month ago
vim.env.LAZY_STDPATH = ".repro"
load(vim.fn.system("curl -s https://raw.githubusercontent.com/folke/lazy.nvim/main/bootstrap.lua"))()
vim.g.mapleader = " "

require("lazy.minit").repro({
    spec = {
        {
            "folke/which-key.nvim",
            config = function()
                require("which-key").add({
                    { "<leader>l", group = "lsp" },
                    { "<leader>lh", function() vim.lsp.buf.hover() end, desc = "Hover" },
                    { "<leader>lr", function() vim.lsp.buf.rename() end, desc = "Rename" },
                    { "<leader>la", function() vim.lsp.buf.code_action() end, desc = "Code Action" },
                    { "<leader>h", function() vim.lsp.buf.hover() end, desc = "Hover" },
                    { "<leader>r", function() vim.lsp.buf.rename() end, desc = "Rename" },
                    { "<leader>a", function() vim.lsp.buf.code_action() end, desc = "Code Action" },
                })
            end,
        },
    },
})

works for me

Duplicating the keymap definitions like this is a poor solution, and is not a solution that I am willing to accept. This would be a step backwards compared to builtin keymaps.

also I don't think that's how proxy is intended to be used in the example of the readme it is used for <c-w> which isn't a mapping itself but just a prefix for mappings

If we both have to guess/assume what proxy does, or have to dig in to the code just to try and figure it out, then it's abundantly clear that it is lacking proper documentation.

tmillr commented 1 month ago

Duplicating the keymap definitions like this is a poor solution, and is not a solution that I am willing to accept. This would be a step backwards compared to builtin keymaps.

Even after migrating to add(), I've still been having issues, but I FINALLY just now got it to work with add(). I think that the defaults must have changed? The missing link was setting remap = true (which before the update may have been noremap = false by default instead? idk). Perhaps that is why it wasn't working with register() (i.e. since v3) for me either.

It'd be nice if there was an easier way to setup remaps/aliases (e.g. so I don't have to copy/duplicate the other opts myself, like desc etc.), and that is what I had originally thought proxy was for. For now I guess I'll just copy it or use a homemade solution.

max397574 commented 1 month ago

If we both have to guess/assume what proxy does, or have to dig in to the code just to try and figure it out, then it's abundantly clear that it is lacking proper documentation.

told you multiple times already that I also think it's missing good documentation

Even after migrating to add(), I've still been having issues, but I FINALLY just now got it to work with add(). I think that the defaults must have changed? The missing link was setting remap = true (which before the update may have been noremap = false by default instead? idk). Perhaps that is why it wasn't working with register() either.

remap=true and noremap=false are completely equivalent I can't imagine that having been a default before the rewrite because it can have many bad side effects I guess this just worked because of some internals

but if that works for you then that's the best solution ig

I guess you can close the issue then?

working solution ```lua vim.env.LAZY_STDPATH = ".repro" load(vim.fn.system("curl -s https://raw.githubusercontent.com/folke/lazy.nvim/main/bootstrap.lua"))() vim.g.mapleader = " " require("lazy.minit").repro({ spec = { { "folke/which-key.nvim", config = function() require("which-key").add({ { "l", group = "lsp" }, { "lh", function() vim.lsp.buf.hover() end, desc = "Hover", }, { "lr", function() vim.lsp.buf.rename() end, desc = "Rename", }, { "la", function() vim.lsp.buf.code_action() end, desc = "Code Action", }, { "h", function() vim.lsp.buf.hover() end, desc = "Hover", }, { "r", "lr", remap = true, desc = "Rename", }, { "a", "la", remap = true, desc = "Code Action", }, }) end, }, }, }) ```
tmillr commented 4 weeks ago

remap=true and noremap=false are completely equivalent

That's precisely my point.

I can't imagine that having been a default before the rewrite because it can have many bad side effects I guess this just worked because of some internals

Well, before v3, nvim_set_keymap() was used, which does have a default of noremap = false (similar to :map and :nmap). See :help nvim_set_keymap(). https://github.com/folke/which-key.nvim/blob/0539da005b98b02cf730c1d9da82b8e8edb1c2d2/lua/which-key/keys.lua#L202

I guess you can close the issue then?

I don't think I will at this time. I'll let a maintainer decide what to do. Other users can easily run into this, and if the default did change (whether knowingly or unknowingly), then it appears that this fact was never documented (not in the changelog either).