-
Notifications
You must be signed in to change notification settings - Fork 1.3k
actions: Execute possible callbacks on Y/N prompts #3885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| if !yes && !canceled { | ||
| if callback != nil { | ||
| callback() | ||
| } | ||
| } else if yes && !canceled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The callback is only called if the save was successful
I am confused. How does the callback() know if the save happened or not?
This might also affect any plugins that use this function since it is a public function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The callback is only called if the save was successful
True for SaveAsCB() and saveBufToFile(), but what does it mean when the user cancels the sudo-prompt, because it shouldn't be written that way? The save wasn't even performed then...by user intention...is it then successful or not? 🤔
I am confused. How does the
callback()know if the save happened or not?
Right now it can't, since we don't pass any arguments. Maybe we should?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The save wasn't even performed then...by user intention...is it then successful or not? 🤔
To me, the comment reads as the callback is called only when the save is done successfully, so no it wouldn't count as "successful". It is reflected in the code logic as well.
I am confused. How does the
callback()know if the save happened or not?Right now it can't, since we don't pass any arguments. Maybe we should?
My main gripe is backward compatibility for existing plugins.
Looking at the code now, here's the callstack: Quit() -> SaveCB() -> saveBufToFile() / SaveAsCB()
SaveCB() promises to perform the callback at the end (Which is not atm), but the other two only perform the callback when it is successful.
We can change saveBufToFile() however we want, but can't do so for SaveAsCB().
Maybe we should change saveBufToFile() to have 2 callbacks, one for success and one for failure.
Then we rename SaveAsCB() to saveAsCB() and have the same 2 callbacks.
Finally, we keep the same signature for SaveAsCB() but pass nil for the failure callback to saveAsCB()
This way we can have the ability to always perform a callback without breaking any backward compatibility.
WDYT? @JoeKar
I am not sure about SaveCB() tbh, maybe we can "correct" it back judging the existing code is expecting it to call the callback no matter what at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should change
saveBufToFile()to have 2 callbacks, one for success and one for failure. Then we renameSaveAsCB()tosaveAsCB()and have the same 2 callbacks. Finally, we keep the same signature forSaveAsCB()but passnilfor the failure callback tosaveAsCB()This way we can have the ability to always perform a callback without breaking any backward compatibility.
WDYT? @JoeKar
Or saveBufToFile() and saveAsCB() will just use one callback, but with argument. SaveAsCB() can create it locally and call the injected callback depending on its own with the argument (success only). We can stick to the promise defined and exported by SaveAsCB() and thus keep the compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or
saveBufToFile()andsaveAsCB()will just use one callback, but with argument
Nice, this works as well.
f934a4f to
da58578
Compare
da58578 to
a90b1c9
Compare
...and keep backward compatibility with `SaveAsCB()` where the callback shall be performed only in case the save was successful.
a90b1c9 to
5de12c4
Compare
This will execute the callback also in case the answer was "no", like we did with #3719.
Fixes #3878