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/Redo plugin #129

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Undo/Redo plugin #129

wants to merge 23 commits into from

Conversation

izak
Copy link

@izak izak commented Feb 14, 2014

Yet another undo-redo plugin.

This does something very simple: it simply keeps ten copies of the older elements on a stack and restores that element if asked to. To save space, it only stores the immediately bounding element where the change occured, so that in the general case it should only store copies of paragraphs.

A few things are still broken, this PR is here merely as a starting point.

  • It should bind to ctrl+z and ctrl+shift+z keys, perhaps also to ctrl+y.
  • If you undo and redo math, it is horribly broken afterwards.
  • At the moment it takes snapshots whenever there is two seconds silence. It should also distinguish between dom changes and character data changes, so that it does not remove multiple paragraphs if you type very quickly (with no two second delay).

@kathi-fletcher
Copy link
Member

Today is not my lucky day for testing. I made test branches at all three levels for undo.

Then I went to oerpub17/new-book and typed stuff in the only module.

Upon typing stuff, I get Uncaught TypeError: Cannot call method 'cloneNode' of null

@izak
Copy link
Author

izak commented Feb 17, 2014

Then I went to oerpub17/new-book and typed stuff in the only module.

Upon typing stuff, I get Uncaught TypeError: Cannot call method
'cloneNode' of null

Yes, that was the bug that suddenly raused its ugly head when I tried to
demo on Friday. It was working fine until 30 minutes before. Will just have
to find out how this escaped me.

@kathi-fletcher
Copy link
Member

So, it behaves very wacky when I try to undo.

I have seen the thing you mentioned where undo undoes two actions, and then redo redoes just one of them.

But I have also seen undo remove existing content (that wasn't edited). And I have deleted a semantic block and not been able to undo that until I delete another block, and then undo puts them both back.

@izak
Copy link
Author

izak commented Feb 21, 2014

Exactly. To some extent it seems to be caused by plugins and similar that
does not properly call smartContentChange after they change something.

We need a way to chop the number of changes into smaller chunks that each
constitutes a change. Normally, whenever a smartContentChange event
arrives, that would be a pretty good indication.

Fwiw, while I looked through the upcoming undoManager implementation that
narrowly missed shipping with html5, I see the same problem there.
Everything needs to be done in transactions, specifically so that the
manager can undo (and redo) them in the same chunked way. This requires
either that all changes made to the DOM passes through the undoManager, or
it requires that you record what has changed (as we do now with the
mutationObserver) and then generate an automaticTransaction with the list
of changes you recorded yourself, which is pretty much what I'm doing now
anyway.

izak added 12 commits February 21, 2014 15:14
Most important one, just before performing an undo/redo, make sure there is no
unprocessed changes.
It is a little scary how well this works. I suppose the byg reports have
yet to come in.
More work of a similar nature us required, but this shows how it works.
The problem with MutationObservers are that their callbacks are only
called after the immediate call stack is empty. You must therefore call
the disconnect outside of your own script, either using setTimeout, or
in the mutation observer itself. The downside to this is that you cannot
stop watching mutations in the middle of a script, it will essentially
record the tail-end of your script in all cases.

For this reason, the callback parameter is now optional. You can also call
transact() without a callback, to record everything up to the end of the
current script.
This monkey-patches jquery-ui so that when you stop dragging a semantic
block, it records DOM changes until script end, which causes your block-add
to become undoable.
Conflicts:
	src/plugins/oer/semanticblock/lib/semanticblock-plugin.js
Sadly this does not work as well as I hoped. But the idea is worth keeping.
Conflicts:
	src/plugins/oer/semanticblock/lib/semanticblock-plugin.js
@kathi-fletcher
Copy link
Member

Oops, doesn't merge

izak added 4 commits March 14, 2014 10:43
Conflicts:
	src/plugins/oer/semanticblock/lib/semanticblock-plugin.coffee
	src/plugins/oer/semanticblock/lib/semanticblock-plugin.js
Because the MutationObserver is async and only processes the queue when
the local calling stack is empty, the entire tail-end of your code is
always recorded. This change allows you to defer the tail-end until after
the transaction is recorded. The idea was to use this for semantic block
activation, but in the end it wasn't useful for that.
The replacement of the placeholder was split into a separate remove
followed by adding the element in the same location. Adding the new
element is then recorded using undoredo.transact, so it can be undone.
@izak izak mentioned this pull request Mar 14, 2014
3 tasks
@izak
Copy link
Author

izak commented Mar 14, 2014

I merged in the changes from #145, so that I could hook insertOverPlaceholder and turn that into an undoable transaction. This means images and media can now be undone.

Normal semantic blocks will also work when dragged onto the page, but I found out today that the same code fires if you drag a block to another spot on the page, and it doesn't work at all yet.

Nevertheless, I think the insertOverPlaceholder example can serve as a very neat example of how to give your plugin undo capabilities.

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

Successfully merging this pull request may close these issues.

2 participants