Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(defaults): add LSP default mappings (again) #28650

Merged
merged 5 commits into from
May 24, 2024

Conversation

gpanders
Copy link
Member

@gpanders gpanders commented May 6, 2024

This is the PR to re-introduce default LSP mappings after being reverted in #28649. The goal is to merge this early in the 0.11 release cycle.

I've changed the defaults to use the gl prefix rather than cr. This avoids all of the messiness with operator-pending mode that was discussed in #28634. We had originally wanted to "reserve" gl for a future "align" feature, but using it as the general purpose "LSP prefix" might be more high leverage (and we can always use gla or ga for "align", falling back to :ascii for the default ga behavior).

We can still revisit these mappings (I think in particular the <C-S> mapping in Insert mode is a bit controversial), but at some point an executive decision will need to be made. We can't please everyone with defaults, so some people are going to be unhappy, unfortunately, but the hope is that providing reasonable defaults makes the onboarding experience easier for more users.

@gpanders gpanders added revisit-at-release DO NOT MERGE Nothing to see here; move along labels May 6, 2024
@github-actions github-actions bot added the defaults issues or PRs involving changing the defaults label May 6, 2024
@zeertzjq zeertzjq added this to the 0.11 milestone May 6, 2024
runtime/doc/lsp.txt Outdated Show resolved Hide resolved
@gpanders
Copy link
Member Author

gpanders commented May 6, 2024

Argument against gr prefix is that if a user does want to restore the builtin gr behavior, they would have to delete all of the mappings under the gr prefix (although I suppose the same argument applies to anyone using a plugin that currently maps gl, so maybe that's actually not a very strong argument).

@zeertzjq
Copy link
Member

zeertzjq commented May 6, 2024

I don't think the same argument applies to plugins that map gl. The builtin gr can be following by any character, but that's not likely to apply to plugins that map gl.

Meanwhile, another argument for using gl instead of gr is that gl is easier to type.

@gpanders
Copy link
Member Author

gpanders commented May 6, 2024

I don't think the same argument applies to plugins that map gl. The builtin gr can be following by any character, but that's not likely to apply to plugins that map gl.

Ah I see what you mean. I am referring to the fact that if we create new gl prefixed keys, then pressing gl will require a delay to use the gl behavior provided by the plugin. If a user wants the mapping to work as before, they have to delete all other keymaps that are prefixed with gl (or change the mapping to gla or something, which is probably what I will do if this goes through).

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

gr

Argument against gr prefix is that if a user does want to restore the builtin gr behavior, they would have to delete all of the mappings under the gr prefix

That needs to be solved some other way. It is a temporary problem.

  • "r" ("refactor") is a more appropriate mnemonic than "l" ("language").
  • "gl" will be useful for other purposes. Whereas there is no alternative, planned use-case for "gr".

gl is easier to type

"grr" and "gra" can be typed with one hand on querty, which is relevant for refactoring-related mappings which may involve the mouse.

@zeertzjq
Copy link
Member

zeertzjq commented May 6, 2024

Actually, that can be solved by nnoremap <nowait> gr gr.

@famiu
Copy link
Member

famiu commented May 6, 2024

Whereas there is no alternative, planned use-case for "gr".

"gr" could be used for LSP references

hieulw added a commit to hieulw/nvimrc that referenced this pull request May 13, 2024
the upstream pr is reverted: neovim/neovim#28649
alternative pr for 0.11 neovim/neovim#28650
@gpanders gpanders removed revisit-at-release DO NOT MERGE Nothing to see here; move along labels May 22, 2024
@gpanders
Copy link
Member Author

gpanders commented May 22, 2024

Whereas there is no alternative, planned use-case for "gr".

"gr" could be used for LSP references

grr is LSP references. gr becomes a prefix for a group of related mappings.

Current proposal is:

grr -> vim.lsp.buf.references()
gra -> vim.lsp.buf.code_action() (works in Visual mode too)
grn -> vim.lsp.buf.rename()

In Insert mode:

<C-S> -> vim.lsp.buf.signature_help()

@gpanders gpanders requested a review from justinmk May 22, 2024 13:52
@gpanders gpanders force-pushed the reintroduce-lsp-defaults branch 3 times, most recently from 6c02095 to ba00ff3 Compare May 22, 2024 13:55
stefanvanburen added a commit to stefanvanburen/dotfiles that referenced this pull request May 22, 2024
@justinmk
Copy link
Member

Whereas there is no alternative, planned use-case for "gr".

"gr" could be used for LSP references

That is completely missing the point.

antoineco added a commit to antoineco/dotfiles that referenced this pull request May 23, 2024
@deathbeam
Copy link
Contributor

gr is very commonly used for LSP references (see for example https://www.lazyvim.org/keymaps#lsp, https://github.com/nvim-lua/kickstart.nvim/blob/master/init.lua#L475 its also recommended mapping by bunch of other stuff). and g as prefix is often used as goto (see native gg, gd, gD) so stuff like code actions, rename (and possibly) format imo do not rly makes sense behind it.

Imo the lsp refactor stuff and lsp goto stuff should have separate prefixes as its 2 very different operations but thats just my 2 cents.

@gpanders
Copy link
Member Author

gpanders commented May 23, 2024

and g as prefix is often used as goto (see native gg, gd, gD)

The g prefix does not mean goto. gs, gp, ga, g8, g?, gJ, gu, gU, gv, gq, gw, and g~ are all counter examples.

Imo the lsp refactor stuff and lsp goto stuff

"LSP goto stuff" is just 'tagfunc', which uses the builtin mappings from :h tag.

gr is very commonly used for LSP references

The proposal here is to use grr for references. Is moving from gr to grr that much of a problem?

@deathbeam
Copy link
Contributor

Imo the lsp refactor stuff and lsp goto stuff

"LSP goto stuff" is just 'tagfunc', which uses the builtin mappings from :h tag.

Well LSP goto stuff can also include implementation, type definiton, defition, declaration, which could possibly also have mappings.

gr is very commonly used for LSP references

The proposal here is to use grr for references. Is moving from gr to grr that much of a problem?

Well is there any reason to not use the mappings that ppl are already used to? The mappings are unused and LSP being integral part of neovim I think justifies not putting the whole thing behind single prefix (especially when gr doesnt even mean anything here that could be used as mnemonic)

@gpanders
Copy link
Member Author

Well is there any reason to not use the mappings that ppl are already used to?

Yes, because default mappings have much greater constraints than arbitrary user mappings. If we make gr map to references, we cannot use gr as a prefix for anything else. So instead of grr, gra, and grn (3 default mappings), we have gr (1 mapping) and have to find separate mappings for the other two. Finding unused default mappings is extremely difficult (if you've been following the "new LSP defaults" saga then you will have observed this).

I will be 100% clear about this (and I will probably end up repeating myself): it is not possible, nor is there any expectation, that we will find default mappings that make everyone happy. The goal is to provide a better out of the box experience for new and less experienced users by providing access to builtin Neovim capabilities without needing much (or any) additional configuration. Experienced and power users will likely have separate preferences, but they already know how to create and delete mappings, so they are free to ignore the defaults.

- CTRL-S is mapped in Insert mode to |vim.lsp.buf.signature_help()|

If not wanted, these keymaps can be removed at any time using
|vim.keymap.del()| or |:unmap|.
Copy link
Member

@justinmk justinmk May 23, 2024

Choose a reason for hiding this comment

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

for gr we may want to mention nnoremap <nowait> gr gr (mentioned by zeer), which avoids needing to unmap every gr* mapping

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't seem to work. If I use that mapping then I'm not able to use any of the new defaults. It reverts to the builtin gr behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I use vim.keymap.set('n', 'gr', 'gr', { nowait = true }) before creating the other gr mappings, then I can still use the new mappings, but it doesn't seem to behave any differently than it does without the <nowait> mapping. So I'm not sure what it's meant to be doing.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't seem to work. If I use that mapping then I'm not able to use any of the new defaults. It reverts to the builtin gr behavior.

That's exactly what is supposed to do: conveniently disable all these mappings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that makes sense, I misunderstood the original suggestion. I’ll include that

Copy link
Member Author

Choose a reason for hiding this comment

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

@gpanders gpanders merged commit 2c6b635 into neovim:master May 24, 2024
29 checks passed
@gpanders gpanders deleted the reintroduce-lsp-defaults branch May 24, 2024 16:33
@bluebrown
Copy link

This is not setting definition Is this already covered by something else? Otherwise, imo, definition and references are the most important ones.

What determines the selection of these mappings?

@arcxio
Copy link
Sponsor

arcxio commented May 25, 2024

Is this already covered by something else

it's C-] by default. there is a proposal to map it to gd in #28476

@neovim neovim locked as resolved and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
defaults issues or PRs involving changing the defaults
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants