Skip to content
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

Undo functionality #10

Open
LeaVerou opened this issue Feb 4, 2016 · 2 comments
Open

Undo functionality #10

LeaVerou opened this issue Feb 4, 2016 · 2 comments

Comments

@LeaVerou
Copy link
Member

LeaVerou commented Feb 4, 2016

Edit to make this current:

Currently, the only data edit that can be undone is deletion. That is special-cased, implemented via a rather clumsy mechanism that involves a lot of special casing in expressions etc, it’s buggy, increases the amount of stuff that needs to be styled, both by Mavo and by the author, and complicates the eventual data mutation API.
Instead, soft deletion should be abolished, and all data mutations should be added to an undo stack that can be undone and redone via shortcuts and potentially Mavo bar buttons.
This will also make it much easier to implement diff-based (incremental) saving by storage backends, if there is a record of what actions have happened since the last save.

Original text (which doesn’t make any sense now):

If #6 is implemented, then edit, save, cancel will become global (no need to edit items separately if the editing is just-in-time).
However, that makes it imperative to have a different mechanism for error recovery than a cancel button: Undo. If I can edit all my talks at once, I don't want to have to cancel ALL my edits because I made a mistake.

@LeaVerou
Copy link
Member Author

After thinking about this for a bit, some issues to address:

What goes on the stack?

Since the stack will be used for both undoing and incremental saving (each code branch can choose to ignore certain entries), which changes should be on it? It seems logical that computed properties should not be on it, since they are neither saved, nor undone. However, as far as Mavo is concerned, there is no such thing as computed properties. Computed properties are just properties with both mv-storage="none" (so that they are not saved by default) andmv-mode="read". Which of the two is the defining one?

If we say that mv-mode="read" properties are not on the stack, then we lose constants, but we do want to save them if they change. If we say that mv-storage="none" properties are not on the stack, then we cannot undo them, which seems useful.

Perhaps it's the intersection of the two that is not on the stack? That seems arbitrary though, and what if the stack is used for other purposes in the future? Or we could just add everything on the stack and depend on each code path to filter out what it doesn't need. The last one probably seems the safest option, though it will bloat the stack.

How many stack entries to retain?

Should the stack be cleared after each save, or should we just add a special element that means "we saved at this point"? Undoing after save is certainly useful, but incremental saving does need to know when we saved.

Never clearing the stack comes with certain performance challenges. Keeping pointers to every node that existed in the lifetime of the app means nothing can ever be garbage collected. So, perhaps it's better to clear the stack after each save, or at least truncate it?

How does the undo stack interact with incremental rendering?

It looks like any change to the data should be on the stack, even if it's neither undoable, nor saved. Otherwise, references become meaningless. E.g. deleting a collection item might be stored as "delete item at index 3". Now, if incremental rendering happens that adds or deletes items before that index, the entry becomes meaningless.

@LeaVerou
Copy link
Member Author

LeaVerou commented Oct 2, 2017

After thinking about it some more (and using @svgeesus as my rubber duck 😛), I've decided to proceed in the following way, at least for a first version of this feature:

  • The stack is cleared after saving. This simplifies a lot of things, and causes no loss of functionality over the current situation. One cannot undo deletion after saving today either.
  • Changes to properties that are not saved, are not added to the stack. If they're not being saved, they are not important data so what's the point of cluttering the edit list with them? E.g. if editing data and you have some filters, you expect undo to undo changes to your data, not your filters! This saves the computed property problem in one fell swoop :)
  • We are not going to deal with render() called externally, just like if you set the value of a textarea via JS it wipes your undo history. If people want to avoid that, they can use incremental methods like add() and delete() instead of render() or manually adjust the edit list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant