Support mapping additional modes

[toupeira]

Thanks for the plugin! :heart:

This adds a map_modes option to specify additional modes to map, such as the terminal. And also a small cleanup to use <Cmd> for the mappings, which avoids flickering.

Two things I'm not sure about yet:

• Do you mind if I go ahead and use vim.tbl_deep_extend() to merge config and user_config? That would remove some duplication, but I'm not sure exactly when Neovim added this (I believe it's available in 0.5 so backwards-compatibility should be fine?)
• I'd also like to clean up the configuration section in the README up a bit, since documenting this new option becomes a bit weird otherwise :grinning: Do you mind if I shuffle some things around?

[alexghergh]

Hey, thanks for the pull requests!

At first glance the changes here look good to me. I think what you're aiming at is to have something like:

{ 'alexghergh/nvim-tmux-navigation', config = function()
    require'nvim-tmux-navigation'.setup {
        disable_when_zoomed = true, -- defaults to false
        keybindings = {
            normal = {
                left = "<C-h>",
                down = "<C-j>",
                ...
            },
            terminal = {...}
        }
    }
end
}

right?

I'm happy with those changes, though we need to make sure to properly check modes are correct (i.e. we don't encounter a godzilla mode and don't know how to map that).

A README update is always welcome, of course!

I might need a few more days to go through this and make sure that everything is going smoothly, but otherwise thank you for the work!

[toupeira]

I think what you're aiming at is to have something like:

@alexghergh hmm actually I wasn't thinking of this approach, since I just want to map Ctrl-hjkl additionally in the terminal. So specifying the modes would be easier for me vs. having to repeat them for the normal and terminal tables, but it's not really a big deal :wink:

We could also have this approach where each key can specify modes:

keybindings = {
  left = '<C-h>',                  -- normal only
  left = { 't', '<C-h>' },         -- terminal only
  left = { { 'n', 't' }, '<C-h>' } -- normal and terminal
}

What do you think? I'm happy to go with your suggestion too.

I'll update the README afterwards, and pushed a commit now to switch to vim.tbl_deep_extend().

[alexghergh]

Hey, sorry for the radio silence for a while,

Hmm, your suggestion seems to me a little bit unintuitive for the end-user. You would still have to repeat { { 'n', 't' }, ... for each key mapped, right (i.e. left, right, up, down)? That would make it a bit hard to follow in my opinion.

I'm not saying that my solution is better in any way, though. It still feels a bit repetitive and I don't like it that much either, but at least it's a little bit clearer and easier to mentally parse, I feel.

If we were to go with your solution, I would probably extend it to something like:

keybindings = {
  left = '<C-h>', -- normal only (single string specifed, defaults to keymap in normal mode)
  left = { keymap = '<C-h>' }, -- normal only (full config table specified, but missing 'modes' key, defaults to normal)
  left = {
    modes = 't', -- terminal only
    keymap = '<C-h>'
  },
  left = {
    modes = { 'n', 't' }, -- normal and terminal
    keymap = '<C-h>'
  },
  left = {
    keymap = '<C-h>', -- of course, order shouldn't matter now
    modes = { 'n', 't', 'v', 'x', ... },
  },
  left = { }, -- error!
  left = { '<C-h>' } -- also error! (you have to specify 'keymap', only 'modes' is optional in config table)
  left = { modes = { 't', 'n' } } -- also error! (see above)
}

I am totally down with this solution (which also keeps backwards compatibilty, unless I'm missing something), and also makes it explicit what each argument means inside the table, though this would probably be a bit more work to do. Let me know how you feel about this! Great work and thanks for contributing!

Have a great one!

Edit: more examples

[toupeira]

@alexghergh thanks, I do like your original suggestion and implemented that now, and figured I'll just add a config.keybindings.modes array for the use case where you want to have the same maps in multiple modes (I'm currently trying out Ctrl-hjkl in normal/terminal/command/visual mode).

So you can now do:

keybindings = {
  left = "<C-h>", -- mapped in normal mode, as before
  modes = { "normal", "terminal" }, -- map in terminal mode too

  command = {
    right = "<C-l>", -- only map in command mode
  },
}

Some notes:

• Most configuration keys are now validated and raise errors.
• Invalid values (both the keys of config.keybindings, and the values of modes) are ignored and don't raise errors. I'm not sure how to do that easily with vim.validate, I guess we could pass functions to check all keys.

What do you think?

[toupeira]

@alexghergh also updated the README now, was a bit easier with this syntax :grinning:

[alexghergh]

Hmm, I honestly find this version the most confusing for the user.

keybindings = {
  left = "<C-h>", -- mapped in normal mode, as before
  modes = { "normal", "terminal" }, -- map in terminal mode too

  command = {
    right = "<C-l>", -- only map in command mode
  },
}

This seems a bit odd to me, i.e. you have basically 3 ways of specifying what the mode of a mapping should be. I would, as mentioned above, rather go with your initial version, where you have to specify the mode of a keymap per key, instead of having the keymaps and the modes separate. I.e., I would rather have:

keybindings = {
  left = '<C-h>',                  -- normal only
  left = { 't', '<C-h>' },         -- terminal only
  left = { { 'n', 't' }, '<C-h>' } -- normal and terminal
}

OR, the version suggested by me with the improvements to naming.

I think having the command/visual/normal and modes keys in the top-level table alongside normal key maps is not a good solution, simply because it offers multiple ways of mapping a key and is confusing to the users. I also have thought about this, and I think my initial solution is too verbose. Your solution was much simpler and easier to understand. I think my current stance is to go with the version suggested in my last comment (potentially adjusting key names or other minor things). Otherwise the changes to the README look good!

As always, thanks for taking your time to contribute to the plugin!

[toupeira]

@alexghergh ok fine by me :grinning:

I went with the { 'n', '<C-h>' } syntax now instead of the more verbose { modes = ..., keymap = ... } because it matches Vim's API, so I figure this should feel fairly intuitive.

Also for reference here's how my configuration looks now:

    opts = {
      disable_when_zoomed = true,
      keybindings = {
        left  = { { 'n', 'c', 'v', 't' }, '<C-h>' },
        down  = { { 'n', 'c', 'v', 't' }, '<C-j>' },
        up    = { { 'n', 'c', 'v', 't' }, '<C-k>' },
        right = { { 'n', 'c', 'v', 't' }, '<C-l>' },
      }
    },

I'll drop a few more comments on the PR to point out some things.