Removing the ability for plugins to modify settings.json and bindings.json. Adding an option to reject plugins to bind keys.

[Neko-Box-Coder]

If you have comments in either settings.json or bindings.json, it is possible for plugins to overwrite the json, and erases all the formatting done by the user.

Adding an option to lock these files to avoid losing formatting.

Related to #3385 (Which we should close), #3302 , #2194

[dmaluka]

Now I'm inclined to think that adding such options would be a good improvement (although I still think it would good to implement what I suggested in #3385 (comment); but that can be implemented independently, and that is about bindings only, not settings).

But, why limit these options to plugins only? settings.json or bindings.json are also overwritten by set and bind commands, so why not let locksettings and lockbindings also prevent those commands from overwriting them? So that users can be sure that nothing will ever mess with their cherished formatting and comments in their config files.

I.e. set and bind would modify settings or bindings globally (not just for the current buffer like setlocal) but for the current micro session only, i.e. not save them in settings.json or bindings.json?

[dmaluka]

And another thought: maybe we should go ahead and make it the default behavior?

Or maybe not just yet, but at least eventually?

So it seems that instead of locksettings and lockbuffers that prevent overwriting when set to true, it would be nicer to add savesettings and savebindings that prevent overwriting when set to false? And set them by default to true for now, but maybe eventually to false?

[Neko-Box-Coder]

Now I'm inclined to think that adding such options would be a good improvement (although I still think it would good to implement what I suggested in #3385 (comment); but that can be implemented independently, and that is about bindings only, not settings).

Yeah that can be a separate logic.

But, why limit these options to plugins only? settings.json or bindings.json are also overwritten by set and bind commands, so why not let locksettings and lockbindings also prevent those commands from overwriting them? So that users can be sure that nothing will ever mess with their cherished formatting and comments in their config files.

I.e. set and bind would modify settings or bindings globally (not just for the current buffer like setlocal) but for the current micro session only, i.e. not save them in settings.json or bindings.json?

The main use case of this PR (for me at least) is to remove the concern that any newly installed plugin (although this also kinda applies to the builtin comment plugin) will remove all the formatting + comments.

I don't mind also applying this for set and bind as well to be consistent. I would assume if the user is calling set or bind, the settings.json and bindings.json are expected to be overwritten, but I guess it wouldn't hurt to apply to these commands as well.

And another thought: maybe we should go ahead and make it the default behavior?

Or maybe not just yet, but at least eventually?

So it seems that instead of locksettings and lockbuffers that prevent overwriting when set to true, it would be nicer to add savesettings and savebindings that prevent overwriting when set to false? And set them by default to true for now, but maybe eventually to false?

I am leaning towards not making it a default behavior since I think only a minority of the users would care about the settings and bindings to be formatted/commented, and I reckon the user would normally expect the set and bind to "just work" instead of having to turn this off manually. As for the names (locksettings vs savesettings), I guess it depends on what the defaults we decided to be?

[dmaluka]

I think only a minority of the users would care about the settings and bindings to be formatted/commented, and I reckon the user would normally expect the set and bind to "just work" instead of having to turn this off manually.

I'm not sure about that. We've seen quite a few complaints about the existing behavior, although it doesn't prove that they represent the majority of users, but we have no proof of the opposite either.

Anyway, yep, we can keep the default as is for now.

I don't mind also applying this for set and bind as well to be consistent. I would assume if the user is calling set or bind, the settings.json and bindings.json are expected to be overwritten, but I guess it wouldn't hurt to apply to these commands as well.

Actually, now arguing with myself: perhaps we should not overthink it and should leave set and bind as they are for now. I don't recall any complaints about this particular aspect of the existing behavior.

As for the names (locksettings vs savesettings), I guess it depends on what the defaults we decided to be?

It seems it rather depends on what kind of solution we want: what your PR already implements (i.e. mere protection against insidious plugins, i.e. essentially an ad-hoc workaround for the unfortunate fact that micro allows this sneaky plugins behavior in the first place), or a "generic" solution for disabling saving settings to *.json when setting them, that covers set and bind commands as well. For the former, the lock* names seem more appropriate. For the latter, the save* names seem more appropriate, regardless of their default values.

Now, assuming we want the former kind of solution... Perhaps instead of working around with adding options for blocking the annoying plugin behavior, we should just prevent this behavior unconditionally, once and for all? In the case of TryBindKey() with overwrite=false, #3385 (comment) would prevent it. As for TryBindKey() with overwrite=true, SetGlobalOption() and SetGlobalOptionNative(), are there any legitimate use cases for plugins to use them? Are there any plugins that actually use them?

[Neko-Box-Coder]

Yeah, then we can change the behavior of TryBindKey() with overwrite to false which doesn't write to bindings.json but instead just set as default like you said. Most of the plugins don't set overwrite to true so I think it is safe to ignore overwrite and treat it as always false. (And also add & migrate to RegisterKeybinding() for consistent naming)

SetGlobalOption() / SetGlobalOptionNative() on the other hand is a bit tricky since there are plugins that do use it unfortunately. I personally think we should just treat it as RegisterGlobalOption() / RegisterCommonOption() and override the default value (from any previous RegisterGlobalOption() calls), there shouldn't be any observable difference. In fact, it should be better since it won't override what the user has set in settings.json (and deprecate SetGlobalOption())

I think my take is that if we are changing the behavior, we should change both keybindings and options and not 1 of them. If we are planning to only change 1 of them, we might as well just merge this PR.

[Neko-Box-Coder]

Also, I think it would be nice to add a (1 time?) warning to set and bind that it will overwrite and reformat the file to let the user know all the formatting and comments will be gone after executing the command.

[dmaluka]

SetGlobalOption() / SetGlobalOptionNative() on the other hand is a bit tricky since there are plugins that do use it unfortunately. I personally think we should just treat it as RegisterGlobalOption() / RegisterCommonOption() and override the default value (from any previous RegisterGlobalOption() calls), there shouldn't be any observable difference.

Ok, I don't know which plugins are those, but I can imagine that. Yeah, it seems it makes sense to handle it as you suggest.

[Neko-Box-Coder]

I have just updated the plugin functions to not write to any json.
I removed locksettings but kept lockbindings even if it doesn't write to json now.

The reason is if the plugin action is irreversible (i.e. delete a file, edit a buffer, etc...), I want to explicitly bind it myself instead of having the plugin bind it behind my back. Having this settings guarantees that even if I pressed a keybind by mistake, I know for sure nothing nasty would happen.

The same logic however can't apply to locksettings. The fact that a decent amount of plugins expects a setting to be there after calling RegisterGlobalOption() would mean that if I do not allow the plugin to do that (i.e. locksettings = true), many plugins will break since they expect the option is already there.

I haven't tested it yet but will do in a bit.

[dmytrovoytko]

Hi @dmaluka @Neko-Box-Coder ! any chance this would be merged?

[Neko-Box-Coder]

Just rebased it for the merge conflict. Ready to be mergied if good.

[dmaluka]

Hi @dmaluka @Neko-Box-Coder ! any chance this would be merged?

I guess not. If we merge it, we are stuck with these lock* options forever, whereas as we discussed above, that would be just a band-aid that wouldn't really solve the problem by default. So, as discussed, it seems more reasonable to fix the plugin-exposed functions (TryBindKey, SetGlobalOption etc) so that plugins never write anything settings.json or bindings.json in the first place (i.e. so that those functions still set bindings or options as plugins expect them to do, but only for the current session, not permanently).

Any volunteering to make that happen is welcome.

[dmaluka]

Ok, I see @Neko-Box-Coder just pushed an updated version. But it doesn't make a lot of sense to me.

[Neko-Box-Coder]

@dmaluka
I just resolved the conflicts but I don't really remember what was the exact outcome of the conversation, I thought we decided the current implementation was good. Let me re-read the whole thing and get back to you in a minute.

[Neko-Box-Coder]

Okay, I remember now.

The PR changed the implementations such that

• TryBindKey() doesn't write to bindings.json no matter what
• SetGlobalOption() now just uses RegisterGlobalOption() which also doesn't write to settings.json.

And I kept lockbindings (happy for a different name if you want) since I want the option to disable the plugin from binding keys entirely in case the newly binded action can cause irreversible effects (Like deleting a file for example) (See comment)

The descriptions for documentation are not accurate, so I need to update them. But before I update them, did I miss anything else? @dmaluka

Judging from my faint memory and by quickly reading back the conversation, I think the implementation here is what we want unless I missed/misinterpreted your replies.

[dmaluka]

If we are "deprecating" SetGlobalOption and SetGlobalOptionNative, what are we suggesting to use instead?

I haven't analyzed if implementing them via RegisterGlobalOption() is a correct approach, but assuming that it is, it means now they implement the new desired behavior, instead of the old undesired one, so we should just update their documentation accordingly, not deprecate them?

[Neko-Box-Coder]

I will update the docs but if a function simply gets redirected to another function that the plugin can call, the "proxy" function should get deprecated no? Since there's no point to have it anymore.

As said before, I forgot to update the docs. I will update it and mention the user/plugin should call RegisterGlobalOption instead.

[dmaluka]

but if a function simply gets redirected to another function that the plugin can call

It doesn't. RegisterGlobalOption that the plugin can call is a different function, it is implemented via RegisterGlobalOptionPlug() which does a different thing than RegisterGlobalOption().

[Neko-Box-Coder]

Updated the documentation.

[dmaluka]

How about actually registering the keybinding (as I suggested in #3385 (comment))?

[Neko-Box-Coder]

But that's no longer the "default" keybinding right?
I think we should treat plugin registered keybinds and user keybinds the same no?

So for example if an unbind is called, it will use the micro's actual default keybindings. I don't think a mutable default keybinding is a good idea.

As for reload in your comment, I am not sure what will happen atm, I assume the bindings will be reconstructed from scratch (default + user keybindings + plugin keybindings) without the unloaded plugin I guess.

[Neko-Box-Coder]

@dmaluka
I will do the changes in one go after we get to a conclusion for this thread, since the implementation can change quite a bit depending on what conclusion we arrive to.

[dmaluka]

Ok, but then why call it "register", if it doesn't really register anything? Let's call it TryBindKey as we used to, since that it what it does - it just tries to bind a key?

[Neko-Box-Coder]

I don't mind either way, I am just trying to be consistent with RegisterGlobalOption, since that one also doesn't write to the .json originally. Which one do we want?

[dmaluka]

RegisterGlobalOption does register a global option. It adds it to the list of known global options, right?

Actually, after a closer look, what you are doing with RegisterGlobalOption in this PR (i.e. using it to implement SetGlobalOption* for plugins) seems not just dubious but utterly wrong. SetGlobalOption* is supposed to just set a (possibly existing) option to a given value, not make this value its default value, right? Moreover, it is definitely not supposed to make it a global-only option (which is what RegisterGlobalOption does)? i.e. what happens if a plugin calls SetGlobalOption("scrollmargin", 0)? It will have not one but two nasty side effects: 0 will become the default value of scrollmargin, and the user will be no longer able to set scrollmargin per buffer via setlocal?

It seems what we actually want for plugins is just a version of SetGlobalOption*() that doesn't call config.WriteSettings(), not a hack using RegisterGlobalOption()?

[Neko-Box-Coder]

Yeah you are right, I see what you mean now. Then I will just add a flag to prevent the json write for SetGlobalOption() and SetGlobalOptionNative()

I got confused when trying to read and differentiate RegisterGlobalOption(), RegisterCommonOption() and SetGlobalOption().

Then in this case, I will just have TryBindKey() but without writing to the json for the plugin

[Neko-Box-Coder]

Yep, updated the behavior of TryBindKey() now.

[dmaluka]

What kind of "unexpected actions"?

AFAICS, lockbindings is completely pointless now, what it prevents is not overwriting bindings.json (that is already prevented anyway) but doing anything at all?

[dmaluka]

The reason is if the plugin action is irreversible (i.e. delete a file, edit a buffer, etc...), I want to explicitly bind it myself instead of having the plugin bind it behind my back. Having this settings guarantees that even if I pressed a keybind by mistake, I know for sure nothing nasty would happen.

Ok, haven't noticed this. Well, a plugin can do nasty things behind your back anyway, by virtue of being installed.

But if you really find this lockbindings useful, we can add it.

But it is a separate change, why not do it in a separate commit? This commit does 3 different things, it can be naturally split into three?

[Neko-Box-Coder]

Ok, haven't noticed this. Well, a plugin can do nasty things behind your back anyway, by virtue of being installed.

Well yes, but the point of it is to stop accidental effect instead of stopping any malicious intent.

For example if a shortcut for deleting a file in a file manager plugin is ctrl-d, it is easy for the user to press it by accident (say when copying with ctrl-c) without knowing its existence. (An extreme example, of course, but the point still stands)

lockbindings (or whatever we want to call it) makes sure the user is ALWAYS in control of the bindings and mis-pressing any keys combinations will not cause any undesired effects.

But it is a separate change, why not do it in a separate commit? This commit does 3 different things, it can be naturally split into three?

Yeah, that's fine, I can separate them.

[Neko-Box-Coder]

Yep, separated them into 3 commits now.

[dmaluka]

LGTM now.

[JoeKar]

The rest looks good so far!

[dmytrovoytko]

Thanks to all of you! 🙏