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: add "default" field to maparg() return value #28527

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gpanders
Copy link
Member

With so many default mappings, plugins are not able to detect if an existing mapping is one created by the user or one created by Nvim.

Add a "default" field to the dict return value of maparg() that plugins can use to determine if a mapping is a default mapping provided by Nvim (which they can safely override).

With so many default mappings, plugins are not able to detect if an
existing mapping is one created by the user or one created by Nvim.

Add a "default" field to the dict return value of maparg() that plugins
can use to determine if a mapping is a default mapping provided by Nvim
(which they can safely override).
/// Whether new mappings should be marked as "default". This is modified only in map_set_default().
static bool create_default_maps;

/// Enable or disable creationg of default mappings.
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment accurate?

@justinmk
Copy link
Member

plugins are not able to detect if an existing mapping is one created by the user or one created by Nvim.

The reason for choosing to set default mappings instead of creating builtins was because this shouldn't matter. Builtin default mappings are just like any other plugin default mappings.

plugins are not able to detect if an existing mapping is one created by the user or one created by Nvim.

And what would the plugin do with that information? Users can :mapclear if they don't want default mappings. Then plugins won't see any non-user-defined mappings.

@gpanders
Copy link
Member Author

plugins are not able to detect if an existing mapping is one created by the user or one created by Nvim.

The reason for choosing to set default mappings instead of creating builtins was because this shouldn't matter.

How could it not matter? A mapping created by the user and a mapping that comes bundled with Neovim are different. Well-behaved plugins use maparg() to ensure they don't clobber a user's mapping, but they're free to override over builtins/defaults.

plugins are not able to detect if an existing mapping is one created by the user or one created by Nvim.

And what would the plugin do with that information? Users can :mapclear if they don't want default mappings. Then plugins won't see any non-user-defined mappings.

Plugins use this information to determine if a mapping was set by the user or not. A plugin can override a default Nvim mapping harmlessly, but a plugin should not override a user's own mapping.

Copy link
Member

@zeertzjq zeertzjq left a comment

Choose a reason for hiding this comment

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

This may not be a good idea. For default autocommands their augroups have a common nvim_ prefix. For mappings there is a proposal for namespaced mappings. If namespaced mappings are implemented, default mappings can be in namespaces with specific names, and this field will become redundant again.

@@ -2118,6 +2130,7 @@ static Dictionary mapblock_fill_dict(const mapblock_T *const mp, const char *lhs
PUT_C(dict, "mode", CSTR_AS_OBJ(mapmode));
PUT_C(dict, "abbr", INTEGER_OBJ(abbr ? 1 : 0));
PUT_C(dict, "mode_bits", INTEGER_OBJ(mp->m_mode));
PUT_C(dict, "default", BOOLEAN_OBJ(mp->m_default));
Copy link
Member

Choose a reason for hiding this comment

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

It may be a bit inconsistent for this to be a boolean as there are no other boolean fields. For consistency this should be an integer, but that's inconvenient for Lua plugins. Since this is mostly likely to be used by Lua plugins, maybe it should be like this:

Suggested change
PUT_C(dict, "default", BOOLEAN_OBJ(mp->m_default));
if (mp->m_default) {
PUT_C(dict, "default", INTEGER_OBJ(1));
}

In future it may also be possible for the default field to have a different type (e.g. string) to provide more information, in which case a missing default field indicating a non-default mapping makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed as suggested d72a4be.

@zeertzjq zeertzjq added the mappings key bindings label Apr 26, 2024
@gpanders
Copy link
Member Author

This may not be a good idea. For default autocommands their augroups have a common nvim_ prefix. For mappings there is a proposal for namespaced mappings. If namespaced mappings are implemented, default mappings can be in namespaces with specific names, and this field will become redundant again.

Even with namespaces, we would need a way to designate that a mapping is "bundled" with Nvim, since anybody (including a plugin) could just add a new mapping to the "defaults" namespace.

@justinmk
Copy link
Member

what about other default mappings, e.g. % ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mappings key bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants