Ckeditor5: Setting data doesn't clear undo stack

[Reinmar]

1. Open any sample.
2. Type few letters (or do any other change).
3. Call editor.setData( '<p>foo</p>' );
4. See that undo is enabled.
5. Undo. Error is thrown.

We have two things here:

• It should be possible to manage whether setting data resets the history (whether it's undoable or not) or not. Or, like in CKE4, there should be a way to tell whether setData() should create undo step and a way to clear the stack. This is more granular but boils down to the same story.

• If setData() was meant to create an undo step then undoing shouldn't throw an error.

  I also thought that features like typing may need to reset their stored batch on such actions. I think that ChangeBuffer should be doing this reliably now but it may be good to write a test for it (if there's none).

[Reinmar]

@scofalik wrote in another ticket:

We wanted initial setData to be not undoable. This makes sense. You don't want to have an undo step just after loading editor. To manage this, batch created in editor#setData is transparent.

However this breaks later setData calls. Imagine initial batch, then some changes, then setData and then when you undo you will try to undo "some changes" - in best case scenario nothing will happen. In worst case scenario something will blow up because new editor contents have nothing to do with old, after setData.

My proposal is that:

• setData should somehow clear undo stack. I don't know how to connect those two in reasonable way, but probably setData would have to just check whether undo plugin is available and just call appropriate method, or
• make setData create non-transparent batches by default but add an option to change it.

BTW. I know that in perfect world, with super-stable undo nothing bad should happen. But ATM editor throws in this scenario (when restoring selection). We don't have to make those changes now but it's good to have this problem in mind.

[Reinmar]

In https://github.com/ckeditor/ckeditor5-paragraph/pull/27 we noticed that we stopped autoparagraphing on setData() because setData() uses transparent batches. Initial autoparagraphing works now because it's triggered on dataReady. Then, it only works if e.g. someone mistakenly remove the entire content, but not if that change was transparent (e.g. done by setData()).

[Reinmar]

The above is not true anymore because now post-fixers don't check the type of batch.

[Reinmar]

I'm raising the priority of this ticket. It'd be good to first gather some feedback. What options do developers need? What API will be most useful?

In https://github.com/ckeditor/ckeditor5-autosave/issues/6 we've seen people doing subsequent seData() calls.

[Reinmar]

In https://github.com/ckeditor/ckeditor5-autosave/issues/6#issuecomment-404776461 I commented on the proposal to have setData() and initData(). Right now, I don't see many use cases for the former semantics, so I'm against having two separate methods.

I'd rather see something like:

editor.setData( data ); // does "init data"
editor.setData( data, { mode: 'change' } ); // does "set data as nothing happened, keep undo, etc."

[pjasiun]

Working on the comments integration we were also missing the option to re-init editors data. With @oskarwrobel it took us a while to find a way to load comments and content in the way which does not look like a total hack, and we are still not very happy with what we get. Loading data should be simple and there should be no hacks needed.

It is not only about cleaning the undo stack, but also about resetting some plugins (remove pending actions, stop uploads, remove comments, etc.). Also, note that collaborative editing feature will integrate differently with it (basically will not allow you to re-init data if the collaborative editing plugin is enabled).

Since we already have editor.data.init method we should make it clean undo stack and fire special event which causes reset plugins data.

When it comes to the editor.setData interface, I agree that there are no many use cases for setData without resetting data (I can not imagine any), and we could have:

editor.setData( data ); // -> editor.data.init
editor.setData( data, { mode: 'change' } ); // -> editor.data.set

But I am not sure if it won't be missleading :/

[Reinmar]

I think we all agree that the main usecase for setData() is resetting the editor, so that's a sensible default. I'm ok with doing this change. However, editor.data.init() is asynchronous so bam, we can't use it here.

[scofalik]

Since we already have editor.data.init() I don't understand why setData() uses transparent batch.

It should either use normal batch and let undoing (makes sense) or it should re-init the whole editor, i.e. use transparent batch, clear history and fire an event on which plugins can listen and clear themselves.

Now we have the worst option, TBH, which is prone to generate errors.

[oskarwrobel]

or it should re-init the whole editor, i.e. use transparent batch, clear history and fire an event on which plugins can listen and clear themselves.

👍

[Reinmar]

Reported on Gitter: https://gitter.im/ckeditor/ckeditor5?at=5cb89dfb375bac7470c6613a

[Reinmar]

It'd be good to know if there's some quick workaround - a method or something you can do after calling setData() to clear the undo stack. cc @scofalik

[scofalik]

Citing myself from Gitter:

Well, _stack property in BaseCommand in undo is protected so you can't do that nicely. If you don't care and the issue is important to you, then you can simply clear _stack array in UndoCommand and RedoCommand (get the commands through editor.commands.get( 'undo' ) and 'redo'. This is, of course, not recommended.

Out of the top of my head, there's one small issue with that - removed nodes, which are stored in $graveyard root (and are no longer needed as you cleared undo stack) won't be cleared which is a memory leak issue. Clearing them is again a thing that there's no public API for, though.

The go-to solution would be, in my opinion:

1. Fire an event on setData or decorate it.
2. Have all interested plugins (like undo) listen to that event and clean themselves on it.

For now I am afraid that there's no nice solution.

[baturian]

Any changes in that issue? Still can't clear undo stack by public API. And setData() still doesn't clear undo stack as well.

[Reinmar]

@baturian not yet, but we've just added this ticket to the sprint so most likely it will land at the end of July.

The decision was to have setData() always clear the undo stack. We can't really think about valid use cases for not clearing it.

We want to implement a solution described by https://github.com/ckeditor/ckeditor5/issues/4060#issuecomment-484808750. It's not yet clear whether DataController#set() should fire a new event or whether it's enough if it will be decorated (as DataController#init() is already).