Discussion: why removing default mappings? How can we improve mapping config?

[wookayin]

Hi, first of all, I wanted deeply thank and appreciate your hard work and efforts to maintain this popular plugin keep improving.

I wanted to bring some dumb question regarding the recent breaking change which removes all the default keymapping, including up, down, c-n, c-p, etc. You must have some reasonable reason behind that, perhaps something about configurability with insert-mode and cmdline-mode settings, but I am very confused why you have chosen that.

We could go with the workaround as suggested, but doesn't it make sense to have a reasonable, default behavior up/down/c-n/c-p out-of-box?

I don't like breaking changes much, especially when the plugin lacks some sort of versioning and a mechanism for checking compatibility, though I totally understand there are often times such breaking changes are of course inevitable. However, it would be even better if users could be allowed to be notified through deprecation warnings and then smoothly migrate to new configs whenever possible.

Could you please provide more insight for us? Thanks!

[axieax]

I'm guessing this could be related to #888 where there were conflicting mappings? I just wanted to add that another thing I found quite frustrating was splitting the mappings for normal setup and cmdline setup functions. I've created a helper function to filter mappings based on mode to avoid repeating the same mapping code in multiple setup functions (1 normal setup and 2 cmdline setup functions), but I can see how this isn't a very practical change for many.

[hrsh7th]

See this.
#739

[wookayin]

Thanks @hrsh7th for the pointer. It was just the commit message was not associated with the existing issue. Sorry for having missed the duplicate issue.

With all due respect, in my view I don't think it's worth a breaking change. Or at least we could have some deprecation period (with some warnings) instead or chosen less destructive changes.

[hrsh7th]

I think it's worth a breaking change.

For example, if the user does not use cmp-cmdline, the new implementation doesn't map anything in cmdline mode.
The old implementation always mapping the cmap <Tab> ....

[hrsh7th]

Did you notice that you can inherit the past settings with the argument of cmp.mapping.preset.insert ({})?

So I can answer How can we improve mapping config?.

You should mapping the following.

cmp.setup {
  mapping = cmp.mapping.preset.insert({
    ['<Tab>'] = {
      i = cmp.mapping.select_next_item()
    }
  })
}

[wookayin]

For example, if the user does not use cmp-cmdline, the new implementation doesn't map anything in cmdline mode.
The old implementation always mapping the cmap ....

This makes sense. Thanks. I can agree with removing the default keymappings.

Of course I checked the workaround you announced, but it was frustrating for me to manage a config that is compatible with two versions of nvim-cmp (or I would want to not just do this).

Let me close this issue in favor of #739. My only frustration is that there was no deprecation period and I respect the plugin authors' decision on the default keymapping behavior which doesn't have the right answer --- it would be much appreciated if you could have some deprecation period next time if possible.

Thank you!

[wookayin]

@hrsh7th, one more thing:

Another thought I had on improving keymap was that, because the above snippet remains still implicit (as mentioned in #739). So now I would prefer listing all the keymaps, but then I have to list the mapping config for those sensible default keymaps like:

local cmp_types = require"cmp.types.cmp"
...
['<Down>'] = cmp.mapping.select_next_item({ behavior = cmp_types.SelectBehavior.Select }),
['<Up>'] = cmp.mapping.select_prev_item({ behavior = cmp_types.SelectBehavior.Select }),  
['<C-n>'] = cmp.mapping.select_next_item({ behavior = cmp_types.SelectBehavior.Insert }), 
['<C-p>'] = cmp.mapping.select_prev_item({ behavior = cmp_types.SelectBehavior.Insert }), 

I am not sure you wanted to users to do this. Is this what you really intended, because using cmp.mapping.preset.insert is not recommended per #739? I somehow felt this was verbose and it requires dependency on some internal modules like cmp.types.cmp. I am not sure what is the recommended config you were considering.

[Kjir]

I just lost half my morning trying to figure out why completions didn't work anymore. A silent breaking change is really the worst way to tackle this, especially since many other plugins choose to notify users with a message in their vim before proceeding with such a change.

Could you please consider doing a more gradual change in the future, with proper advance notice?
Thanks!

[ngalaiko]

I also spent too much time looking for what's wrong with my completion

Breaking changes are bad (and this is breaking). It shoud've been announced at least some time ago in a very clear way: readme, pinned issues, message in the console, etc.

[hrsh7th]

I'm good at programming with LSP related areas, but I'm not good at caring for plugin users.

But let me be clear. This is my hobby project and I don't think "destructive changes are bad".

I welcome advices!

[Kjir]

The problem is not with destructive changes, but about the lack of upfront communication. As @ngalaiko pointed out, finding out which plugin broke and why is time consuming and frustrating as a user, which is why a notification would have been extremely helpful "Ah I remember seeing this message about something being deprecated in nvim-cmp, maybe that is what happened?"

I will think about a possible easy-to-use solution, for simple needs having a vim.notify("[DEPRECATION] Default mappings will be removed in the near future. Check here for how to upgrade your configuration: https://example.com") would already be better than nothing. Using the conventional commits format as mentioned in #896 would also help at least for users of Packer (and possibly other plugin managers too)

[hrsh7th]

I don't use packer.nvim so it's not a solution for someone.

I'm considering the following API (but I don't think it's good... this plugin is just nvim plugin. I didn't see any plugin that have own nortification mechanism. I tjink this is overkill).

require('cmp').dismiss_announcement(100)

I say again this is just nvim-plugin abd that created by personal auther...

[gegoune]

@hrsh7th It is an overkill and something that would distract you from what really matters here. I think versioning is the answer here. With following semantic version principles you shelter yourself from any complaints and nagging from users. Simply bump major version, include note about breaking change and be done with it all.

If you want to go this route I can work on PR automating it all for you. Will open an issue to discuss it in single place.

[hrsh7th]

It's interesting but I don't know the plugin manager that supports version concept.

[gegoune]

@hrsh7th Most of them do. Release are git tags. Tags are git references, same way branches are. Whenever plugin manager allows you to use specific branch, tag, etc, it can be used to pin to specific version.

[smjonas]

@hrsh7th Btw, packer supports wildcard tags like 1.0.* since wbthomason/packer.nvim#809, also see wbthomason/packer.nvim#797 (comment) where it is explained how to use semantic versioning using GitHub actions - I believe this is an alternative to release-please-action which @gegoune suggested in #906