Multitude of keymap issues during user customization

[asmodeus812]

Description

I have been trying this for a while, but am unable to customize the keymaps for the commit and rebase editors, in any sensible way without breaking neogit, the moment you try to change the defaults the editor panel would refuse to open, from the popup (commit or rebase ones), no errors, it just does not open. Seems like neogit follows maggit bindings a bit too blindly, not considering that unlike emacs vim is modal. Some keymaps are defined in insertmode some are in both insert and normal, some are defined only in normal mode, and there is no way to customize that, and there is no obvious rhyme or reason behind this.

            commit_editor = {
                ["q"] = "Close", -- this is mapped **only** in normal mode
                ["<c-c><c-c>"] = "Submit", -- is mapped in insert and normal mode
                ["<c-c><c-k>"] = "Abort", -- same as above, because reasons 
                ["<c-p>"] = "PrevMessage", -- only in normal
                ["<c-n>"] = "NextMessage", -- only in normal
                ["<c-r>"] = "ResetMessage", -- only in normal
            },

n  q           *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 3)<CR>
n  <C-C><C-K>  *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 6)<CR>
n  <C-P>       *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 5)<CR>
n  <C-R>       *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 4)<CR>
n  <C-C><C-C>  *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 2)<CR>
n  <C-N>       *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 1)<CR>

i  <C-C><C-K>  *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 8)<CR>
i  <C-C><C-C>  *@<Cmd>lua require('neogit.lib.mappings_manager').invoke(8, 7)<CR>

• If i change the submit and abort to be c, the c would be bound for both normal and insertmode. This is inconsistent with other keymaps actions which are only bound in normal mode. I guess tough luck if your commit message contains the letter "c" in it while you type it out.

• If i remove an entry from the list of keymaps it also fails to open the either the commit or rebase editors, say i remove the reset message keybind because i do not care about it, it only works if ALL possible currently existing actions are in the table, for that particular popup/editor.

• If we have use_default_keymaps = true enabled when new actions are added upstream, this will break the user's configuration constantly, due to the reasons presented above.

• If the use_default_keymaps = false and the user likes to simply override some, then he is bombarded with duplicate key actions messages. The reason, if we define key 'x' in status keymaps but it is by default already defined in the popup keymaps, neogit does not reconcile these changes.

Neovim version

0.9.4

Operating system and version

Ubuntu 22

Steps to reproduce

1. run nvim with minimal
2. try to commit a change - commit editor is opened
3. try to rebase commits - no rebase editor is opened

Expected behavior

• I would like to override the default <c-*> variants to be single keypress, in normal mode., just like any other action.

• I would like to customize / provide only a specific set of actions in the keymaps with use_default_keymaps = false, and have only those be mapped for editors, popups, status etc, the ones which remain unused, should not prevent the user from interacting with neogit

• I would like to have use_default_keymaps = true, and neogit consider correctly the user config and merge the keymaps against the defaults without duplicate keymaps errors. (obviously if the user's config itself contains duplicates between the different keymap sections, that is fine to throw an error)

Actual behavior

Well, the opposite of the expected behaviour.

Minimal config

-- NOTE: See the end of this file if you are reporting an issue, etc. Ignore all the "scary" functions up top, those are
-- used for setup and other operations.
local M = {}

local base_root_path = vim.fn.fnamemodify(debug.getinfo(1, "S").source:sub(2), ":p:h") .. "/.min"
function M.root(path)
  return base_root_path .. "/" .. (path or "")
end

function M.load_plugin(plugin_name, plugin_url)
  local package_root = M.root("plugins/")
  local install_destination = package_root .. plugin_name
  vim.opt.runtimepath:append(install_destination)

  if not vim.loop.fs_stat(package_root) then
    vim.fn.mkdir(package_root, "p")
  end

  if not vim.loop.fs_stat(install_destination) then
    print(string.format("> Downloading plugin '%s' to '%s'", plugin_name, install_destination))
    vim.fn.system({
      "git",
      "clone",
      "--depth=1",
      plugin_url,
      install_destination,
    })
    if vim.v.shell_error > 0 then
      error(string.format("> Failed to clone plugin: '%s' in '%s'!", plugin_name, install_destination),
        vim.log.levels.ERROR)
    end
  end
end

---@alias PluginName string The plugin name, will be used as part of the git clone destination
---@alias PluginUrl string The git url at which a plugin is located, can be a path. See https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols for details
---@alias MinPlugins table<PluginName, PluginUrl>

---Do the initial setup. Downloads plugins, ensures the minimal init does not pollute the filesystem by keeping
---everything self contained to the CWD of the minimal init file. Run prior to running tests, reproducing issues, etc.
---@param plugins? table<PluginName, PluginUrl>
function M.setup(plugins)
  vim.opt.packpath = {}                      -- Empty the package path so we use only the plugins specified
  vim.opt.runtimepath:append(M.root(".min")) -- Ensure the runtime detects the root min dir

  -- Install required plugins
  if plugins ~= nil then
    for plugin_name, plugin_url in pairs(plugins) do
      M.load_plugin(plugin_name, plugin_url)
    end
  end

  vim.env.XDG_CONFIG_HOME = M.root("xdg/config")
  vim.env.XDG_DATA_HOME = M.root("xdg/data")
  vim.env.XDG_STATE_HOME = M.root("xdg/state")
  vim.env.XDG_CACHE_HOME = M.root("xdg/cache")

  -- NOTE: Cleanup the xdg cache on exit so new runs of the minimal init doesn't share any previous state, e.g. shada
  vim.api.nvim_create_autocmd("VimLeave", {
    callback = function()
      vim.fn.system({
        "rm",
        "-r",
        "-f",
        M.root("xdg")
      })
    end
  })
end

-- NOTE: If you have additional plugins you need to install to reproduce your issue, include them in the plugins
-- table within the setup call below.
M.setup({
  plenary = "https://github.com/nvim-lua/plenary.nvim.git",
  telescope = "https://github.com/nvim-telescope/telescope.nvim",
  diffview = "https://github.com/sindrets/diffview.nvim",
  neogit = "https://github.com/NeogitOrg/neogit"
})
-- WARN: Do all plugin setup, test runs, reproductions, etc. AFTER calling setup with a list of plugins!
-- Basically, do all that stuff AFTER this line.
require("neogit").setup({
     use_default_keymaps = false,
        mappings = {
    commit_editor = {
      ["q"] = "Close",
      ["<c-c><c-c>"] = "Submit",
      -- ["<c-c><c-k>"] = "Abort" -- when just one, whichever it is, is commented out, editor fails to open
    },
    rebase_editor = {
      ["p"] = "Pick",
      ["r"] = "Reword",
      ["e"] = "Edit",
      ["s"] = "Squash",
      ["f"] = "Fixup",
      ["x"] = "Execute",
      ["d"] = "Drop",
      ["b"] = "Break",
      ["q"] = "Close",
      ["<cr>"] = "OpenCommit",
      ["gk"] = "MoveUp",
      ["gj"] = "MoveDown",
      ["<c-c><c-c>"] = "Submit",
      -- ["<c-c><c-k>"] = "Abort" -- when just one, whichever it is commented out, editor fails to open
    },
    finder = {
      ["<cr>"] = "Select",
      ["<c-c>"] = "Close",
      ["<esc>"] = "Close",
      ["<c-n>"] = "Next",
      ["<c-p>"] = "Previous",
      ["<down>"] = "Next",
      ["<up>"] = "Previous",
      ["<tab>"] = "MultiselectToggleNext",
      ["<s-tab>"] = "MultiselectTogglePrevious",
      ["<c-j>"] = "NOP",
    },
    -- Setting any of these to `false` will disable the mapping.
    popup = {
      ["?"] = "HelpPopup",
      ["A"] = "CherryPickPopup",
      ["D"] = "DiffPopup",
      ["M"] = "RemotePopup",
      ["P"] = "PushPopup",
      ["X"] = "ResetPopup",
      ["Z"] = "StashPopup",
      ["b"] = "BranchPopup",
      ["c"] = "CommitPopup",
      ["f"] = "FetchPopup",
      ["l"] = "LogPopup",
      ["m"] = "MergePopup",
      ["p"] = "PullPopup",
      ["r"] = "RebasePopup",
      ["v"] = "RevertPopup",
    },
    status = {
      ["q"] = "Close",
      ["I"] = "InitRepo",
      ["1"] = "Depth1",
      ["2"] = "Depth2",
      ["3"] = "Depth3",
      ["4"] = "Depth4",
      ["<tab>"] = "Toggle",
      ["x"] = "Discard",
      ["s"] = "Stage",
      ["S"] = "StageUnstaged",
      ["<c-s>"] = "StageAll",
      ["u"] = "Unstage",
      ["U"] = "UnstageStaged",
      ["$"] = "CommandHistory",
      ["#"] = "Console",
      ["Y"] = "YankSelected",
      ["<c-r>"] = "RefreshBuffer",
      ["<enter>"] = "GoToFile",
      ["<c-v>"] = "VSplitOpen",
      ["<c-x>"] = "SplitOpen",
      ["<c-t>"] = "TabOpen",
      ["{"] = "GoToPreviousHunkHeader",
      ["}"] = "GoToNextHunkHeader",
    },
  },

}) -- For instance, setup Neogit

[CKolkey]

Yeah, fair points. I won't pretend that the existing API is ideal - I think it would be much nicer to pretty much copy what telescope does, and allow users to bind functions directly.

This isn't something very high on my list right now, but I'd welcome any pull request that goes in the direction of telescope - doesn't need to be you, but if you're up for it I can help point you in the right direction :)

[CKolkey]

On the nightly branch you can now set separate insert mode mappings for submit and abort.

[asmodeus812]

@CKolkey this does not however address the initial request. Please revisit the description of this issue, there are more than one issues. And i immediately hit the same issue with master branch. Where if i have even 1 missing keymap, the entire neogit fails to start (with use_default_mappings = false). For example updating to master i was missing "untrack" mapping, and neogit just chokes, and fails to init.

[asmodeus812]

Not addresssed here yet

I would like to customize / provide only a specific set of actions in the keymaps with use_default_keymaps = false, and have only those be mapped for editors, popups, status etc, the ones which remain unused, should not prevent the user from interacting with neogit

I would like to have use_default_keymaps = true, and neogit consider correctly the user config and merge the keymaps against the defaults without duplicate keymaps errors. (obviously if the user's config itself contains duplicates between the different keymap sections, that is fine to throw an error)

[star-szr]

@asmodeus812 you didn’t ask me, I’m not a maintainer of this project (only contributed a few small things), but having maintained other projects my recommendation would be to create a separate issue for each issue.

With separate issues you can provide a clean and clear reproduction for each issue (easier to write tests) and people who want to use that style of config can rally behind it (maybe even help fix/implement) as well. Of course the issues can link to each other (and back to this one) since they are similar and related.

Edit: I see https://github.com/NeogitOrg/neogit/issues/1302 has been created, my recommendation would be to split it, since the “true” and “false” cases are quite different. Of course the actual maintainers may have a different opinion, this is just mine.

[asmodeus812]

I suggest we keep this one closed, since it is also containing some issues that are solved already. But for the rest they are all connected to issues with the keybindings. Mostly revolving around how the use_default_keymaps works.