Skip to content

(re)Add gitsigns recommended keymaps#740

Closed
dam9000 wants to merge 1 commit into
nvim-lua:masterfrom
dam9000:pr-gitsigns-keys
Closed

(re)Add gitsigns recommended keymaps#740
dam9000 wants to merge 1 commit into
nvim-lua:masterfrom
dam9000:pr-gitsigns-keys

Conversation

@dam9000

@dam9000 dam9000 commented Mar 11, 2024

Copy link
Copy Markdown
Contributor

This used to be in kickstart, but was removed in the last rewrite, probably to keep the file short.
I would ask you to reconsider and include these keymaps.
I use them regularly and suspect others might find them useful too.

The keymaps are copied from:
https://github.com/lewis6991/gitsigns.nvim?tab=readme-ov-file#keymaps
with added descriptions.

@feoh

feoh commented Mar 12, 2024

Copy link
Copy Markdown
Collaborator

Hi! I personally have zero problem with this change. These seem like useful features.

However I know @tjdevries explicitly had removing the gitsign stuff in mind with this rewrite, so you may need to take it up directly with him.

One question: Could this be maybe moved into an optional enhancement folks could uncomment to include? I'd do that!

Just trying to figure out if there's middle ground to get everyone what they want.

@dam9000

dam9000 commented Mar 12, 2024

Copy link
Copy Markdown
Contributor Author

Right, it was removed at this point:
https://www.youtube.com/watch?v=-joJuscbM5w&t=526s
With the explanation of reducing the number of lines of code:

gitsigns is a great plugin, but probably all you need is [the signs character opts]
I don't think we need any of [the keymaps] by default, I don't think anyone is using this...
So if I delete this ... this is 60 lines ...

If we add an "optional" way to enable this it would still increase the amount of code, even more so than if it's not an optional feature. Unless, you're suggesting to comment out the whole block in which case it would count towards the number of comments and not the number of code :)

Anyway it seems we need @tjdevries to make a decision here.

@feoh

feoh commented Mar 12, 2024

Copy link
Copy Markdown
Collaborator

for what it's worth I spoke to him about this on discord, and he says he'll be looking at PR on this project again in a week or so.

@tjdevries

Copy link
Copy Markdown
Member

Let me think about it a little bit more, but I'm still leaning towards it adds a lot of noise for newcomers for something that is pretty auxillary. They should probably be focusing on other movements / actions / keymaps /etc instead of thinking about some of these git items.

I wouldn't mind some way to allow them to see these keymaps more easily, or add them but I don't know that it's a good idea to have in the base init.lua

@ro0gr

ro0gr commented Mar 15, 2024

Copy link
Copy Markdown

I think the "next/prev hunk" keymaps are the only critical ones for the default experience, cause they provide with a way to navigate throgh the signs rendered explicitly by the plugin.

While the rest of features are also cool, I find them being used way less frequently then navigating through the chunks. But maybe it's just me.

@feoh

feoh commented Mar 18, 2024

Copy link
Copy Markdown
Collaborator

@dam9000 Would you consider making this another optional commented out add in like the linting one we just added?

I can totally see where these features are useful but I also understand and agree with @tjdevries desire to keep the base init.lua lean and mean so it's an easier jumping off point for new folks to actually read and understand.

@dam9000

dam9000 commented Mar 19, 2024

Copy link
Copy Markdown
Contributor Author

@feoh you mean moving the keymaps to a separate lua/kickstart/plugins/gitsigns.lua which would be commented out? That would kinda work but is not a clean solution because then the lazy spec would be divided into two files (init.lua for the opt.signs and gitsigns.lua for the opt.on_attach). While this works because lazy will merge the tables it will lead to confusion. Also at least the navigation keymaps ([c ]c) should be part of the default config and if that is done then the other keymaps can no longer be split because lazy will not merge the function contents (of course) - only one opt.on_attach hook can be active. Including these keymaps is part of the "good defaults" that attracts people because efficiently navigating git is just as important as navigating the code. (All of the above is of course just my view)

@feoh

feoh commented Mar 19, 2024

Copy link
Copy Markdown
Collaborator

@dam9000 You're right, that's icky.

If @tjdevries ultimately decides to keep it out, I'll be merging your PR into my fork :) Because if nothing else this functionality would make resolving merge conflicts easier to maintain said fork :)

I'm sorry this has been so choppy and back and forth. I guess this is the way the software donuts get made :)

Comment thread init.lua
-- Toggles
map('n', '<leader>tb', gs.toggle_current_line_blame, { desc = '[T]oggle git show [b]lame line' })
map('n', '<leader>td', gs.toggle_deleted, { desc = '[T]oggle git show [d]eleted' })
end,

@lewis6991 lewis6991 Mar 20, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to keep this minimal I would remove:

  • toggle_deleted
  • toggle_current_line_blame
  • <leaderhD> diff against last commit
  • reset_buffer
  • undo_stage_hunk
  • stage_buffer

All these can be done with the :Gitsigns command.

The rest are pretty critical to my workflow: navigation, individual hunk stage/reset, diff, preview, blame.

@dam9000

dam9000 commented Mar 20, 2024

Copy link
Copy Markdown
Contributor Author

force pushed: rebased to latest master and made a minor simplification of vim.schedule that reduces 4 lines.

-          vim.schedule(function()
-            gs.prev_hunk()
-          end)
+          vim.schedule(gs.prev_hunk)

(same for next_hunk)

Verified that it works correctly.

@dam9000

dam9000 commented Mar 20, 2024

Copy link
Copy Markdown
Contributor Author

@feoh , @tjdevries here is a PR with reduced list of keymaps and simplified code: #779

@feoh

feoh commented Apr 17, 2024

Copy link
Copy Markdown
Collaborator

I can appreciate where you're coming from around elegance and cleanliness, but I don't think we're going to see these merged.

I'm running with gitsigns installed as a regular plugin in my fork and it works great.

Would it make sense to not let the perfect be the enemy of the good? I could draft a PR where we make it another optional and by default commented out addition?

@dam9000

dam9000 commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

@feoh yeah I see this is not getting anywhere, if making it an optional plugin would help getting it merged I can prepare that too.

@feoh

feoh commented Apr 17, 2024

Copy link
Copy Markdown
Collaborator

If you can get to that before I can that would be grand. Thank you for all your efforts - I really appreciate it!

@feoh

feoh commented Apr 18, 2024

Copy link
Copy Markdown
Collaborator

Closed in favor of #858. Thanks for the hard work on this!

@feoh feoh closed this Apr 18, 2024
@dam9000 dam9000 deleted the pr-gitsigns-keys branch April 18, 2024 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants