action: Stop processing chained actions/commands in the moment the current Pane is not a BufPane (fix crash)

[JoeKar]

Actually the TermPane isn't really considered as a Pane, even it really is a Pane.
-> In the first version of this PR I added the Name() function to the TermPane to be able to consider the TermPane as a Pane in the future.

The main change to fix the crash is the check the return of MainTab().CurPane() against nil in the BufMapEvent() respective bufAction() function, because CurPane() will return it in the moment the current pane changes to a pane other than a BufPane.

Fixes #2288
Fixes #2307

[dmaluka]

Why not keep it returning *BufPane, and in the callers just check if the returned value is not nil?

The lua-exported function returns Pane, yes, but according to the documentation it should return *BufPane. Isn't it better to fix it to be in accordance with the documentation?

[JoeKar]

Why not keep it returning *BufPane, and in the callers just check if the returned value is not nil?

This would be the easiest approach, but the function is called CurPane() and as such it should return the current pane, which might be a BufPane, RawPane (derived from BufPane), InfoPane (derived from BufPane) or a TermPane.
I just imagine the moment we really like to address the current active TermPane received from CurPane(), without changing the interface again.

The lua-exported function returns Pane, yes, but according to the documentation it should return *BufPane. Isn't it better to fix it to be in accordance with the documentation?

But we could also correct the documentation, that the function returns a Pane. Anyway, isn't there a type conversion in Lua to address the BufPane based on the received value (I'm no Lua expert)?

[dmaluka]

Anyway, isn't there a type conversion in Lua to address the BufPane based on the received value (I'm no Lua expert)?

I'm afraid there isn't. I don't see https://pkg.go.dev/layeh.com/gopher-luar#New saying anything about Go's type assertion.

So, unless you know a way to do that, keeping the existing (implemented but not documented) polymorphic behavior of lua's CurState() doesn't seem like a good idea. With the (documented but not implemented) non-polymorphic behavior, a plugin at least would be able to check if the current pane is a bufpane or not.

We may consider generalizing CurPane() to non-buf panes in the future, but I think that when it comes to non-buf panes, we have more important stuff to generalize anyway. At least the following:

• Lack of support for non-buf panes in chained actions. While this PR will fix the crashes in #2307 and #2288, it will still not make either of the keybindings in #2307 and #2288 actually work, since it just interrupts handling the action once it encounters to a non-buf pane.
• Lack of support for onAction and preAction callbacks for non-buf panes. See e.g. 2677#issuecomment-1409211651

...In the meantime, when it comes to CurPane():

• InfoPane: plugins can use InfoBar().HasPrompt to check if the infopane is currently active or not (like micro itself does).
• TermPane: yes, plugins probably cannot check if the current pane is a term pane (and which of them, if there are multiple term panes), but possibly there will never be a use case for that?
• RawPane: it is quite a special type of pane, plugins would hardly want to do anything with it.

[JoeKar]

I more or less reverted the most of the stuff. I hope I didn't overlook or brake something in regards of plugin compatibility.
Currently it feels like not moving forward with micro, since so many functions are exported and can't simply changed or evolved. :/

[dmaluka]

It always return true otherwise. It returning innerSuccess here useful? Looks like the return value of this function is never really used.

In other words, can we just break here?

[JoeKar]

But innerSuccess is modified in the condition...
innerSuccess = innerSuccess && h.execAction(a, names[i], j, te)
...depending on the result of execAction() or do I oversee something?
In case we break we return the forced true.

[dmaluka]

So, as I said, it doesn't look like this return value is used anywhere anyway. So it doesn't look like it makes sense to return anything else than the forced true here. (Or, if we still want to return innerSuccess here, then at least for consistency we should also at line 177 return success instead of the forced true, right?)

AFAICS success and innerSuccess are only for determining if we should execute the next action, but for determining which value should bufAction return.

I actually have no idea what is the semantics of this return value. To be precise, I actually see just one place where this return value is used: https://github.com/zyedidia/micro/blob/master/internal/action/infopane.go#L143

and, this particular code path looks VERY confusing to me. Can you explain to me what does this code do and why does it use this return value the way it does? Why doesn't InfoPane's DoKeyEvent() just do the same thing as BufPane's DoKeyEvent()?

[dmaluka]

Reading your question again... Yes, innerSuccess is modified depending on the value returned by execAction(), but what does it have to do with the value returned by bufAction?

[JoeKar]

but what does it have to do with the value returned by bufAction?

Unfortunately nothing.

[...] return success instead of the forced true, right?

Yes and so I did.

[dmaluka]

Answering myself:

To be precise, I actually see just one place where this return value is used: https://github.com/zyedidia/micro/blob/master/internal/action/infopane.go#L143

and, this particular code path looks VERY confusing to me. Can you explain to me what does this code do and why does it use this return value the way it does? Why doesn't InfoPane's DoKeyEvent() just do the same thing as BufPane's DoKeyEvent()?

Ok, I've figured out myself what's up with that logic in InfoPane's DoKeyEvent(). That logic makes sense, although is quite non-obvious and also somewhat buggy... PR #3267 is the result of my investigations.

[...] return success instead of the forced true, right?

Yes and so I did.

And I'm still not sure it's the best idea. I still believe this return value is not really used, and should not really be used. (Please see 8c7f63a and let me know if you disagree with the commit message.)

Well... if we merge #3267, including 8c7f63a, then there will be no more code that (improperly) tries to use this return value. So then I'm ok with either keeping this return true here or changing it to return success.

[dmaluka]

Just a nit though: it's better to do this (change to return success) in a separate commit, since it is a separate change, not related to the fix for the crash.

[JoeKar]

Please see 8c7f63a and let me know if you disagree with the commit message.

Hm, then we could drop the return at all. Anyway, I'm fine with that.