Change TryBindKey() behavior to prevent plugins from needlessly rewriting bindings.json

[dmaluka]

Change the (documented) behavior of TryBindKey(): unless the overwrite argument is true, never write to bindings.json, even if the new binding has been successfully applied.

TryBindKey() with overwrite=false is mainly used by plugins for adding their own default keybindings. It is not really necessary for them to write those default bindings to bindings.json: even if the binding is not saved in bindings.json, it will be still applied by the plugin calling TryBindKey() every time micro starts.

This fixes undesirable behavior mentioned in #2194 (comment): unless there is already a binding for the given key in bindings.json, a plugin forcibly rewrites bindings.json, in particular removing any user's comments from it. With this change, bindings.json will be only rewritten by micro in the case when the user runs bind or unbind command, not automatically by plugins (unless there is a plugin calling TryBindKey() with overwrite=true, which is unlikely).

[Neko-Box-Coder]

Nice one.
I normally put comments in my bindings.json since I rebind a lot of the default keys.

It has always been a chore to find out which plugin is "overwriting" my bindings.json. Especially when installing multiple plugins at once.

Tested the changes on my machine with both overwrite as true and false. Works as expected.

[injust]

Currently, bindings.json is the source of truth for any changes away from the default keybindings. Some things become less clear after this change:

1. How can I tell what a keybinding does?
2. What happens when two plugins try to set conflicting keybindings?

[Neko-Box-Coder]

Currently, bindings.json is the source of truth for any changes away from the default keybindings. Some things become less clear after this change:

1. How can I tell what a keybinding does?

2. What happens when two plugins try to set conflicting keybindings?

1. The showkey command shows that.

2. If the key is not binded, whoever calls the TryBindKey() first set the action at runtime. Before this change would be the same but only happen once since the change is written to disk (Assuming overwrite is false).

In the case of multiple plugins trying to bind the same key, this change would be a breaking change.
Maybe we can output an error message when multiple plugins try to bind the same key?

[injust]

The showkey command shows that.

I'm glad this exists. Opened #3391 because it took me a few tries to figure out how to use it.

Finding out what a keybinding does (or which plugin is setting it) will require figuring out how to pass it to showkey. For some keybindings, that is non-trivial and probably requires documentation-reading + the raw command. That sounds an order of magnitude more difficult than scanning through a config file.

Could we write the plugin keybindings to somewhere like bindings-plugins.json? Put a comment at the top to mention that it's auto-generated and that user keybindings should go to plugins.json. That gives you your source of truth back, at the cost of an additional config file.

Alternatively (or do both!), make showkey easier to use.

[injust]

In the case of multiple plugins trying to bind the same key, this change would be a breaking change. Maybe we can output an error message when multiple plugins try to bind the same key?

Not a breaking change I think? Right now, bindings.json will already contain the keybinding for the plugin that "wins".

[Neko-Box-Coder]

Alternatively (or do both!), make showkey easier to use.

Yeah, I do agree showkey can be more intuitive, similar to the raw command.

Alternatively, maybe we can add a flag that shows the action for all binded keys, like micro -showkeys which we can just serialize the mapped keys back to json and output it to console.

Could we write the plugin keybindings to somewhere like bindings-plugins.json? Put a comment at the top to mention that it's auto-generated and that user keybindings should go to plugins.json. That gives you your source of truth back, at the cost of an additional config file.

Yeah, this is worth considering I think.

In the case of multiple plugins trying to bind the same unbinded key, the change we have right now can cause said key to behave differently each time, since whoever calls TryBindKey() first owns that key. I think we should avoid this or at least notify the user if such scenario happens.

While having a dedicated json just for this is not elegant, it would avoid this problem I think.

[dmaluka]

Having a dedicated json is just crazy, I think. We have enough crap already.

I think the request #3391 is reasonable, we can try to implement an interactive version of showkey (when no argument is given to showkey). Feel free to submit a PR.

Regarding non-deterministic behavior when multiple plugins set conflicting keybindings - yeah, good point, thanks for pointing it out. Thinking how to fix it, the only more or less clean solution I can think of is:

1. Add a new RegisterKeybinding() function for plugins, with clean straightforward semantics: register a default keybinding for the given key, enable this binding unless it is overridden by the user in bindings.json (but do not write this default binding to bindings.json), and return an error and inform the user if there is a conflicting default keybinding registered by another plugin or by micro itself (regardless of whether this keybinding is overridden by the user in bindings.json or not unless this keybinding is overridden by the user in bindings.json; in such case, the user shouldn't be bothered about ambiguities of the default keybindings, since they all are overridden by the user's keybinding anyway).
2. Deprecate TryBindKey() for plugins, and recommend them use RegisterKeybinding() instead.
3. Change the behavior of TryBindKey() with overwrite=false to be the same as RegisterKeybinding(), so that we don't need to change existing plugins.

[Neko-Box-Coder]

Having a dedicated json is just crazy, I think. We have enough crap already.

Yeah I can understand how you feel, that's why I said it is not "elegant" 😉

Regarding non-deterministic behavior when multiple plugins set conflicting keybindings - yeah, good point, thanks for pointing it out. Thinking how to fix it, the only more or less clean solution I can think of is:

1. Add a new RegisterKeybinding() function for plugins, with clean straightforward semantics: register a default keybinding for the given key, enable this binding unless it is overridden by the user in bindings.json (but do not write this default binding to bindings.json), and return an error and inform the user if there is a conflicting default keybinding registered by another plugin or by micro itself (regardless of whether this keybinding is overridden by the user in bindings.json or not).

2. Deprecate TryBindKey() for plugins, and recommend them use RegisterKeybinding() instead.

3. Change the behavior of TryBindKey() with overwrite=false to be the same as RegisterKeybinding(), so that we don't need to change existing plugins.

Sorry I am not quite following, what is the difference with the changes in this PR for TryBindKey() with overwrite to false against RegisterKeybinding() you are suggesting here? Are they the same thing or different?

If they are the same, doesn't the non-deterministic behavior behavior still exist? TryBindKey() already returns a boolean result right?

[dmaluka]

TryBindKey() just returns a boolean result, it doesn't inform the user on its own. Also it doesn't distinguish the case of a conflicting keybinding of different plugins from the case of a keybinding overridden by bindings.json, - it returns false in both cases. (So it's no wonder that plugins in practice don't do anything with this return value either. What exactly would they do with it?)

[Neko-Box-Coder]

TryBindKey() just returns a boolean result, it doesn't inform the user on its own. Also it doesn't distinguish the case of a conflicting keybinding of different plugins from the case of a keybinding overridden by bindings.json, - it returns false in both cases. (So it's no wonder that plugins in practice don't do anything with this return value either. What exactly would they do with it?)

It returns a boolean and an error right? It can stay as it is if a keybinding already exists in bindings.json, but in the case of conflicting keybindings of different plugins, we can change it to return an error and potentially also output an error to infobar in micro itself.

1. Add a new RegisterKeybinding() function for plugins, with clean straightforward semantics: register a default keybinding for the given key, enable this binding unless it is overridden by the user in bindings.json (but do not write this default binding to bindings.json), ...

Sorry 😅 , I am still not following. Isn't this what TryBindKey() is already doing? Try to bind a key if not binded already?

I am not too sure about having a separate function (RegisterKeybinding()) just for this rare case unless I am missing anything.

[dmaluka]

It returns a boolean and an error right? It can stay as it is if a keybinding already exists in bindings.json, but in the case of conflicting keybindings of different plugins, we can change it to return an error and potentially also output an error to infobar in micro itself.

So, it would not stay as it is, it would completely change the semantics of its return values, and in a very messy way. Imagine how the documentation for this new behavior would look like?

Isn't this what TryBindKey() is already doing? Try to bind a key if not binded already?

And as I said, it is not enough. I want a function that will register a default keybinding, not necessarily immediately enabling it (since it may be overridden by the user; that's a normal situation, not an error), but remembering this default keybinding. So that when the user, for example, runs the unbind command to disable the user's custom keybinding and restore the default one, micro will remember that there was a default keybinding for this key registered by a plugin, and enable this keybinding (just like it does for micro's own default keybindings).

That would also allow to properly disable a plugin's keybinding when unloading a plugin e.g. in the reload command, and so on.

[Neko-Box-Coder]

Yeah okay, it makes much more sense now. RegisterKeybinding() would be a good replacement of TryBindKey()

Should we still merge this?

[dmaluka]

Should we still merge this?

I'm inclined to think we shouldn't.