[Bug] Issues with keymaps persist

asmodeus812 commented 1 month ago

Description

This has been a problem forever since i have been using neogit, every time i pull from master the change in keymaps breaks the plugin and makes it unusable. I have to track down, from the source which keymaps are added or removed and duplicate them in my custom config, just so i can use the plugin again. This is not sustainable i believe. The various issues are described below:

  -- Set to false if you want to be responsible for creating _ALL_ keymappings
  use_default_keymaps = false,

This is a bad idea in every way, we would like to customize a few of the available keymaps, noone is going to manually merge his own custom keymaps manually each time they update to keep just a handful of custom ones, this needs to be revisited. The plugin has been extremely agressive with its keymapping enforcement.

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 = false 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 = true 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.
If a keymap is defined as a user defined callback, instead of a key/action string neogit errors out.

Neovim version

Nvim 0.10

Operating system and version

Ubuntu 22

Steps to reproduce

run nvim with minimal
try to commit a change - commit editor is not opened
try to rebase commits - no rebase editor is opened

Expected behavior

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 be able to provide user defined callbacks instead of action names for certain keymaps, e.g. the close keymap at the moment does not work well, since when a new neogit status tab is opened it is opened to the right of the current tab, when you close the status it is only natural to go back to the previous tab, as of now the close action simply puts you to the next tab.
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

Neogit breaks on most pulls from master. Forcing the user to track down keymap issues and merge them manually, so he can use his own custom mppings.

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",
    },
  },

}) -- For instance, setup Neogit

CKolkey commented 1 month ago

So here's the thing - I'm entirely sympathetic to your issue. I don't love the current way the config works, but.... I also just use the defaults 🤷🏼 So this isn't an issue I really have, or a pain I feel.

Add to this, I don't have a ton of spare time right now to work on stuff. My wife's pregnant, and I've got a 2 year old, so I'm a bit tired in the evenings. I'm just saying that to explain that I'm not going to do anything about this for the foreseeable future.

But.

It's an open source project. I got involved with it because, honestly, it kinda sucked compared to Magit. The performance with a lot of changes in the index was terrible, diff's weren't lazy loaded, the UI had no titles over sections in popups, there was no integrated fuzzy finder, etc. So... I started making it the tool I wanted it to be.

So thats my proposal to you: Help me make this the tool you want it to be. I'm incredibly receptive to fixes to the mapping system.. really. Otherwise, it's going to stay like this for... no clue. A few years, probably. Couldn't say.

rain9441 commented 1 month ago

I didn't notice this until Neovim 0.10. It seems like there wasn't a lot of new key bindings added prior to 0.10 but now it's hitting a lot. For anyone who hits this who is looking for a solution, consider locking your Neogit plugin version to tag v0.0.1 or your favorite commit as it seems to be quite stable.

CKolkey commented 1 month ago

True, but that tag doesn't work with nvim 0.10. The reason it seems like a lot of changes at once is that I've been working on features that used nvim 0.10 API's since like... january. So, the change you noticed was five months of work.

Anyways, you can also pin to tag v1.0.0 if you want nvim 0.10 support 🤷🏼

asmodeus812 commented 1 month ago

@CKolkey as i am sure everyone here, we appreciate this plugin a lot, using it daily, that is why i am confused, considering how complex the rest of it is, something as trivial as overriding user mappings seems like a weird oversight. I myself use a fork that is like 500 commits behind since i am too afraid to even update it due to these issues. I gave it a shot, pulled master, and did spent an afternoon chasing, which new keymaps are missing. But gave up, then after seing that we can no longer set user callbacks as keys was something i do use a lot at the moment.

CKolkey commented 1 month ago

Hmmm. What do you mean "user callbacks as keys"?

asmodeus812 commented 1 month ago

I have something like that in my config atm, to make sure i go to the actual tab i came from when i close the git status view.

 local function close_git_view()
        local curr = vim.fn.tabpagenr()
        local last = vim.fn.tabpagenr("$")

        if curr ~= last then
            vim.cmd.tabnext({ args = { curr - 1 } })
        end
        pcall(neogit.close)
    end

["q"] = close_git_view,

mangelozzi commented 6 days ago

A 2year old will chow time, I have a 1 year old and 3 year old, I can sympathize. I have the same issue, as the OP. I am also confused at how complex the project is, yet a simple hotkey system is no present. I just wish to use like 1% of the plugin power, but I am failing due to the hotkeys. I will see how complex it is, maybe I can port a hotkey system from a plugin I made into it.

CKolkey commented 6 days ago

I'd be more than happy to help you do that, if you're up for it 😅

CKolkey commented 3 days ago

I have something like that in my config atm, to make sure i go to the actual tab i came from when i close the git status view.
 local function close_git_view()
        local curr = vim.fn.tabpagenr()
        local last = vim.fn.tabpagenr("$")

        if curr ~= last then
            vim.cmd.tabnext({ args = { curr - 1 } })
        end
        pcall(neogit.close)
    end

["q"] = close_git_view,

This not working was an oversight on my part - that's now fixed on master

NeogitOrg / neogit