Set default reviewer keymaps

[jakubbortlik]

Feature Description

I'd like to share a piece of my gitlab.nvim/DiffView configuration. It creates operator and visual mode mappings in the reviewer windows. Rather than having this in my configuration, I think it would be more suitable to be part of gitlab.nvim itself for the following reasons:

1. Comment/suggestion creation and move_to_discussion_tree_from_diagnostic only make sense in the reviewer. No need for the mappings to be global.
2. My current configuration creates the mappings for any DiffView reviewer buffers, even those not created by gitlab.nvim, where they make no sense.
3. Currently, the "plugin does not set up any keybindings outside of the special buffers it creates", but most users probably do create mappings for commenting. However, some of the suggested mappings are quite cumbersome, e.g., to create a suggestion for a paragraph, one would type vipgls. With the operator mappings, I can just type sip, or vips if I want to first see the visual selection -- since the reviewer windows are non-modifiable, s and c are practically free to remap and the cip, sip mappings are much easier to type.
4. Using the same keymap for move_to_discussion_tree_from_diagnostic as for jump_to_reviewer it's easier to quickly move between the tree and reviewer without having to think if its m or glm that one has to use.

    ---@param cb string Name of the API function to call
    local function execute_operatorfunc(cb)
      local old_opfunc = vim.opt.operatorfunc
      local cur_win = vim.api.nvim_get_current_win()
      local cur_pos = vim.api.nvim_win_get_cursor(cur_win)

      _G.CreateOperatorfunc = function(callback)
        return function()
          vim.cmd.execute([["normal! '[V']"]])  -- Make sure we are in linewise visual mode
          vim.api.nvim_command(('lockmarks lua require("gitlab").%s()'):format(callback))
          vim.api.nvim_win_set_cursor(cur_win, cur_pos)
          vim.opt.operatorfunc = old_opfunc
        end
      end

      vim.opt.operatorfunc = ("v:lua.CreateOperatorfunc'%s'"):format(cb)
      vim.api.nvim_feedkeys("g@", "n", false)
    end

    require("diffview").setup({
      keymaps = {
        view = {
          { "n", "c", function() execute_operatorfunc("create_multiline_comment") end, { desc = "Create comment in range of motion"} },
          { "n", "s", function() execute_operatorfunc("create_comment_suggestion") end, { desc = "Create suggestion for range of motion"} },
          { "n", "a", function() require("gitlab").move_to_discussion_tree_from_diagnostic() end, { desc = "Move to discussion"} },
          { "v", "s", function () require("gitlab").create_comment_suggestion() end, {desc = "Create suggestion for selected text"}},
          { "v", "c", function () require("gitlab").create_multiline_comment() end, {desc = "Create comment for selected text"}},
        }
      },
    })

I'd prefer to use a rather than m for jumping between the reviewer and the tree since a is "free" in the non-modifiable buffers, and m could be left available for setting marks. x would also be a fine alternative. But of course, I can do this in my personal config, and I can use the existing default m for jumping. The default operator mappings could be glc and gls instead of c and s, to be more in line with the current mappings, but I find that unnecessarily conservative.

@harrisoncramer, would you be willing to include something like this into the plugin? I imagine this could be called in the initialize_discussions function, something like:

diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua
index 3894bf5..739ef08 100644
--- a/lua/gitlab/actions/discussions/init.lua
+++ b/lua/gitlab/actions/discussions/init.lua
@@ -55,6 +55,7 @@ M.initialize_discussions = function()
   end)
   reviewer.set_callback_for_reviewer_enter(function()
     M.modifiable(false)
+    M.set_reviewer_keymaps()
   end)
   reviewer.set_callback_for_reviewer_leave(function()
     signs.clear_signs()

Where M.set_reviewer_keymaps would work similar to M.modifiable (lua/gitlab/actions/discussions/init.lua:68).

I'd only use create_multiline_comment and create_comment_suggestion, since create_comment is just a special case of create_multiline_comment anyway, and it can be achieved by simply using a motion that moves on the same line, e.g., cl.

[harrisoncramer]

Hey, this is possibly a good idea, but I'd really like to close out your other issues before adding others. There are two other issue requests with your name on them, do you think you'd be able to tackle either of these?

https://github.com/harrisoncramer/gitlab.nvim/issues/282 https://github.com/harrisoncramer/gitlab.nvim/issues/158

You also suggested that you may be able to address this one, I don't know if it's been resolved yet:

https://github.com/harrisoncramer/gitlab.nvim/issues/247

[jakubbortlik]

Hi, #158 is tricky, but I'll see what I can do about it.

With #247, I think Aravind's point about the "origin/" .. base_branch being the correct merge target makes sense, so I can prepare a PR with this change.

[harrisoncramer]

I'm game to review an MR implementing default keymaps if you wanted to take a stab at it, specifically for the reviewer pane. I think you're right that we shouldn't make people set them up themselves.

It'd have to be configurable, e.g. a "keymaps" object of some sort in settings, where someone can opt into the defaults or not, and then the ability to override each one. We'd also probably make this a breaking change and set them on by default to reduce friction for new users.

[harrisoncramer]

@jakubbortlik I'd like to try and get this implemented next to reduce friction for new users, if you want to do it go for it, or I can try, let me know!

[jakubbortlik]

Hi @harrisoncramer, I'll try and get a PR ready.