Skip save on open or term command if buffer is shared

[niten94]

This pull request modifies the open and term command to skip the save prompt if a file is opened in multiple buffers, in the same way with #3559. The buffer or pane is also replaced when all prompts are completed like the Quit action, since the prompt code is generalized into a method.

The names and comments of added methods and parameters could be improved, but I wasn't able to come up with a way to do so.

[dmaluka]

in the same way with #3359

Typo?

[niten94]

in the same way with #3359

Typo?

Sorry, yes, I meant #3559.

[dmaluka]

Passing action="Save" will cause executing onSave callback?

Reminds me of #3689 (comment)

At the moment I'm inclined to think it's ok.

[niten94]

I checked again and this seems fine to me too for now.

I don't think the callbacks are good with cleanly detecting generic events like file close or save regardless of caller or action, but plugins can at least be notified in most cases in ways like this.

[JoeKar]

I also think that it is OK, since we already use it with "SaveAs" too and the boundaries between actions and commands are somehow fluid.
Otherwise there will be complaints like "There was a save, but no plugin callback...".

[dmaluka]

It's actually even messier, since in the save command (#3689 (comment)) we execute either onSave or onSaveAs depending on what kind of save it was, while here we execute onSave in both cases...

[niten94]

I only focused on the point about calling onSave when a command is ran, but I just realized that onSave shouldn't be called when path changes since it's expected with other callbacks (onSaveAs only?) instead.

I took a while coming up with a fix, while checking if there are other issues with Lua callbacks when using closePrompt. I think I should be able to properly submit changes and a simple fix tomorrow.

[dmaluka]

Improving #3689 to pass onSave instead of onSaveAs sounds good to me. (It is trivial to do, it doesn't increase complexity, it would hardly break compatibility for anyone since #3689 was only recently merged, so why not.)

Doing nothing at all also sounds good to me.

it is a burden to have onSave and onSaveAs...

The reason for the existence of onSaveAs is that we have an action SaveAs bindable to a key. We automatically provide callbacks for all such actions.

The problem is that we don't quite clearly define or document when those action callbacks are supposed to be executed: just when the user triggers the bindable action by pressing a key bound to it, or in other cases as well (and if so, in what cases exactly). I recall it was also discussed in #3550.

So besides improving #3689 we might consider improving the documentation, to better match the reality:

--- a/runtime/help/plugins.md
+++ b/runtime/help/plugins.md
@@ -71,11 +71,15 @@ that micro defines:
 
 * `onSetActive(bufpane)`: runs when changing the currently active bufpane.
 
-* `onAction(bufpane)`: runs when `Action` is triggered by the user, where
-   `Action` is a bindable action (see `> help keybindings`). A bufpane
+* `onAction(bufpane)`: runs when `Action` is triggered by the user, when
+   pressing a key bound to this action (see `> help keybindings`). A bufpane
    is passed as input and the function should return a boolean defining
    whether the view should be relocated after this action is performed.
 
+   As a special case, `onSave` runs not only when the `Save` action is
+   triggered by pressing a key bound to it, but in all cases when the
+   buffer is saved, for example in the save dialog when quitting.
+
 * `preAction(bufpane)`: runs immediately before `Action` is triggered
    by the user. Returns a boolean which defines whether the action should
    be canceled.
@@ -88,16 +92,6 @@ that micro defines:
    detecting various changes of micro's state that cannot be detected
    using other callbacks.
 
-For example, a function that is run every time the user saves the buffer
-would be:
-
-```lua
-function onSave(bp)
-    ...
-    return false
-end
-```
-
 The `bp` variable is a reference to the bufpane the action is being executed
 within. This is almost always the current bufpane.
 

But that would be still not quite true: onSave is not called when the file is saved as a result of pressing a key bound to SaveAs (in such case onSaveAs is called instead), or when a plugin directly calls buf:Save(), and so on.

So, what should we do with all this mess? Probably nothing...

Also preSave behavior is still inconsistent with onSave: it is only called when pressing a key bound to Save, not in other cases...

[JoeKar]

So besides improving #3689 we might consider improving the documentation, to better match the reality:

Yes, we should.

But that would be still not quite true: onSave is not called when the file is saved as a result of pressing a key bound to SaveAs (in such case onSaveAs is called instead), or when a plugin directly calls buf:Save(), and so on.

But here the plugin knows, that Save() was called, since it calls it explicit and directly. Furthermore it can evaluate its result on its own.

Also preSave behavior is still inconsistent with onSave: it is only called when pressing a key bound to Save, not in other cases...

What about the implementation and usage of the following wrapper to include the pre-CBs too?

func (h *BufPane) execBufKeyAction(name string) bool {
	if action, ok := BufKeyActions[name]; ok {
		return execAction(action, name, nil)
	}
	return false
}

Or execBufAction() to hide the key relation.

[dmaluka]

But here the plugin knows, that Save() was called, since it calls it explicit and directly. Furthermore it can evaluate its result on its own.

But some other plugin (or the end user, in init.lua) may want to run some logic any time when a buffer is saved, including when it is saved by any other plugin?

[JoeKar]

Hm, then a rework of the plugin callback mechanism would be required to cover such scenarios too.

[niten94]

I may have overlooked something as I read this thread and #3550, but I agree with @JoeKar's comment at #3550 (comment). The only "event callbacks" I currently see to be useful are saving and closing files, since action callbacks seem to be fine with most other cases.

The documentation could then be updated to simply recommened using the save event callback, but I don't know if there's practical disadvantages compared with onSave.