Add undo handler to allow undoing actions in apps

[ChristophWurst]

For NC11 we want to finally implement undo file deletion. Since this is a generic software pattern, I'd like to implement it in a reusable undo-handler that can later be used by Nextcloud apps too (e.g. we want to have a fake-undo for sending messages in Mail).

This PR is based on the command pattern, where the handler holds reference of a single command at a time. As soon as another command is added, we execute the previous one. That means that the commands are actually executed deferred, but I'm not yet sure if that is the preferred way for all actions we want to have an undo. Maybe some can be executed immediately, then we could add an option to the command. The current design should be flexible enough to work with both.

@nextcloud/javascript let me know what you think.

TODO:

• listen to the onpageunload event on window and execute the command if one is set
• add unit tests
• fully implement one undo action (e.g. file deletion)

[mention-bot]

@ChristophWurst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @icewind1991 and @LukasReschke to be potential reviewers.

[LukasReschke]

I'd prefer if text would be escaped.

[ChristophWurst]

sure, I'll add Handlebars to render that html string

[ChristophWurst]

After thinking about this a bit more I'm a bit worried about handling errors in the execute/undo callbacks and asynchronous actions, e.g. an ajax call to delete a file. I am unsure how to handle those.

@nextcloud/javascript does anybody have experience with implementing undo in js?

It will also be tricky to add the undo feature to existing application logic, because you'd have to make sure the previously added undoable action is executed when another action is triggered that depends on the first one to have finished.
Example: the user deletes file Pictures/cat.jpg. We defer the action for 7 sec and show the undo notification. Within those 7 sec the user navigates to the parent folder / and renames Pictures to Videos. The problem: if we don't push another command to the undo handler (e.g. because we don't want to have undo for rename) then the first command won't be executed and the file deletion will fail after the 7 sec have elapsed as the path is invalid then.
One solution would be to run OC.Undo.execute() to clear the current undo queue. Alternatively, we could add a silent command, that does not show up as notification and is therefore not really undoable. In any case, we'd have to touch all application code to make that happen, AFAIK there's not way of doing this automatically.

[jancborchardt]

(Btw @ChristophWurst let me know as soon as there is something to review for me. :)

[tcitworld]

The problem: if we don't push another command to the undo handler (e.g. because we don't want to have undo for rename) then the first command won't be executed and the file deletion will fail after the 7 sec have elapsed as the path is invalid then.

I think the undo queue should be cleared each time an user does an action.

[mammuth]

I think the undo queue should be cleared each time an user does an action.

That's how Google Inbox handles the undo feature as well (in android and the webapp at inbox.google.com).

So I guess from a UX perspective this is probably the expected result, or at least the user won't wonder about this method.

[jancborchardt]

@ChristophWurst any open questions here btw? As said, let me know if something is testable, I’m really excited to have this back. :)

[ChristophWurst]

@ChristophWurst any open questions here btw? As said, let me know if something is testable, I’m really excited to have this back. :)

I don't see this happening because we'd either have to 1. adjust/rewrite most of the js code or 2. do major hacks that will probably cause more troubles than we currently have. Adding undo to existing code where the architecture was not designed for it is a major change to the existing functionalities.
I've listed some problems above that will obviously occur if we don't implement this properly.

[skjnldsv]

I see two possibility in my mind right now.

1. We only develop this for the file app and we can add like a progressbar on the ui for when the fille is completely deleted
2. We want to create a undo modal dialog system on core. Like this any app could create and use a popup with a revert action customisation!

[ChristophWurst]

@skjnldsv if we add that progressbar – what if the user navigates to another folder? what if the user opens another app? what if the user renames another file to the same name? what if the user uploads a file with the same name? what if the user navigates one level up and deletes the folder?
In all cases, we'd have to make sure that the delayed action (deleting that file) is executed before we execute the next action right?

[skjnldsv]

Like @tcitworld said. i think a user interraction should clear the queue too! :)
And if the user upload a file with the same name, isn't the default nextcloud action to override the previous file? If so maybe we could use a timestamp comparison? Like if deleteaction < fileuploadtime then canceldeletion ? :)

[jancborchardt]

@skjnldsv a very present progress bar like this would not really be in line with our simple and nonblocking design philosophy, would it? ;) I mean, when you navigate away and something is still deleting, we could show an alert if there’s really no other option … but not show something in every case.

[skjnldsv]

Well, we can make this discreet and subtle! :D
To be honest, I would prefer a popup on a corner or like in the notification area with a slidebar and an undo button. Same concept but smaller and that doesn't clear when the user go away from the deleted file!

[MorrisJobke]

@ChristophWurst @karlitschek I moved this to 12.0 milestone as discussed

[go2sh]

How about keeping the undo manager in the session? So you can load/execute/modify it when ever you need it and easily change pages, etc.

[ChristophWurst]

I'm closing this now because the approach I tried won't work for various reasons (see my comments above). Let's re-think this and implement it properly.

[jancborchardt]

Opened an issue so we don’t forget about this! #3216