plugins: Add capability to dis-/enable them per buffer#2836
plugins: Add capability to dis-/enable them per buffer#2836zyedidia merged 1 commit intomicro-editor:masterfrom
Conversation
|
Tested with patch. Works as expected on my end. 👍 |
| if !pl.Loaded { | ||
| pl.Load() | ||
| } | ||
| _, err := pl.Call("init") |
There was a problem hiding this comment.
init and deinit are a global thing, right? Calling them has effect on all buffers.
There was a problem hiding this comment.
On more thought, this concerns the above pl.Load() call as well, since it enables onBufferOpen, onBufPaneOne etc, which are global.
This seems be a problem with the very concept of enabling/disabling plugins per buffer, not just with its implementation in this PR. If a plugin is disabled globally (and thus not loaded yet), and we enable it locally for a given buffer, we need to load this plugin, to make it work for this buffer. But by loading it we actually enable it globally for other buffers as well (although only partially, but that's even worse).
There was a problem hiding this comment.
init and deinit are a global thing, right? Calling them has effect on all buffers.
On more thought, this concerns the above pl.Load() call as well, [...]
Unfortunately, yes. 😟
The focus was on preventing the usual callbacks.
There was a problem hiding this comment.
And the result seems buggy. The user may run setlocal foo on and expect it not to take any effect for other buffers, but it does take effect for them.
Maybe, to avoid that, we shouldn't load the plugin when enabling it locally, i.e. assume that it is already initialized globally, i.e. that it is enabled globally. Which would probably also mean that we should not allow enabling the plugin locally if it is not enabled globally (and should print an error to the user in this case).
I don't quite like this suggestion myself, but at least it would avoid unexpected unwanted side effects in the most common cases.
Looking at the autoclose plugin as an example, it so happens that it can be safely enabled/disabled without limitations, since it only includes "local" callbacks and no global state (it has global variables but they are actually constant). But we cannot assume that about an arbitrary plugin, and most plugins are not like that.
| eh.UndoStack.Push(t) | ||
|
|
||
| b, err := config.RunPluginFnBool("onBeforeTextEvent", luar.New(ulua.L, eh.buf), luar.New(ulua.L, t)) | ||
| b, err := config.RunPluginFnBool(nil, "onBeforeTextEvent", luar.New(ulua.L, eh.buf), luar.New(ulua.L, t)) |
There was a problem hiding this comment.
Why nil? Shouldn't this be per-buffer too?
| @@ -42,11 +42,11 @@ func RunPluginFn(fn string, args ...lua.LValue) error { | |||
| // RunPluginFnBool runs a function in all plugins and returns | |||
There was a problem hiding this comment.
The comment needs an update?
There was a problem hiding this comment.
You keep punishing me for my carelessness. 😄
But you're right, it's not for every plugin any longer.
|
Stumbled across this (merged) PR while figuring out how to address the problem described in #2973 (comment), and found a couple of dubious things in this PR. Please see my comments. |
| eh.UndoStack.Push(t) | ||
|
|
||
| b, err := config.RunPluginFnBool("onBeforeTextEvent", luar.New(ulua.L, eh.buf), luar.New(ulua.L, t)) | ||
| b, err := config.RunPluginFnBool(nil, "onBeforeTextEvent", luar.New(ulua.L, eh.buf), luar.New(ulua.L, t)) |
There was a problem hiding this comment.
Here (with the introduced nil) I'm currently unsure if this will work in Go, since the targeting type is passed by reference by default.
There was a problem hiding this comment.
@dmaluka:
I created this review entry directly after creating the PR, but unfortunately it was still pending. 😓
Yep, it should be per buffer, but at the time of creation I didn't found an easy way to do so.
As you already figured out the whole stuff seems to need a larger rework.
There was a problem hiding this comment.
eh.Buf.Settings wouldn't work?
There was a problem hiding this comment.
Yes, eh.buf.Settings seems to be the correct choice. Don't ask me why I didn't realized that this interface is already present within the EventHandler.
From current end user perspective it's not really clear why plugins can't be disabled for local buffers and receiving an invalid option doesn't tell that much. So I tried to block at least the callbacks triggering actions in the plugins.
Close #2835