T/undo 2

[scofalik]

This PR includes changes in the engine that came up when fixing and re-writting Undo feature. It also "touches" ckeditor/ckeditor5#3729 (added batch type to batch constructor) and finally closes ckeditor/ckeditor5#3710.

I don't think that ckeditor/ckeditor5#3729 is done though, because we haven't come up with closed list of batch types and I also have hope that we could get rid off redo batch type and maybe even undo batch type. https://github.com/ckeditor/ckeditor5-undo/issues/13

Biggest changes introduced by this PR is:

• CompressedHistory that stores deltas applied to the document but provides API to remove those deltas or updated deltas that are stored in history.
• RemoveOperation now creates "holder" elements in graveyard root - this was needed for CompressedHistory, so we can really remove deltas from history - earlier removing deltas from history messed up OT when graveyard was involved.
• OT changes for new RemoveOperation behavior

[Reinmar]

I pushed a small commit with API doc fixes. The code looks clean an elegant and since it works for undo it's hard to say that something's wrong.

[pjasiun]

CompressedHistory has nothing to do with compression. Also I do not see any reason to keep CompressedHistory and History separated.

[pjasiun]

Will the fact that now there might be multiple deltas with the same baseVersion, not break collaboration?

[scofalik]

It should be fine. AFAICS there are three things that should be considered:

1. Transforming incoming deltas.

2. Sending changes to other clients.

3. Repeating history when new client joins.

4. There are several cases here to consider. As long as you have correct state in compressed history and correctly "pre-transform" incoming operation it will be fine. I will write proper explanation at the bottom of this post. Anyway, the operational/delta transformations got a bit complicated and their base versions are fixed on multiple levels. I actually think it could be a good idea to get rid of fixing (incrementing) base version in OT algorithms. A feature that transforms deltas should be responsible for setting proper base version.

5. Deltas are created and applied by Batch API. Nothing changed there. Each created delta will have a new, next number. OT plugin will listen for them and propagate them to server (BTW. I thought about it yesterday and I don't think change event is enough for this -- we might consider introducing delta event and/or change engine.model.Document#applyOperation to #applyDelta). This means that compressed history having multiple deltas with same base version are enclosed for each client.

6. When new client joins, it's editor contents have to be initialized. But you don't have to repeat history to do so. All you need is to make a "snapshot" of current model state and pass it as one big initial InsertDelta. Or you just send the "snapshot" itself and OT plugin on client side will convert it to initial InsertDelta (or just use setData method). All you need to do afterwards is to set engine.model.Document#baseVersion to correct value.

Here are some cases of what might happen when you get incoming delta while your history got compressed. Let's assume given history stack. ---- means that there was delta there but it got removed.

   A
   B
---C1---
---C2---
   Da without C
   Db without C
   E without C
---C1'---
---C2'---
   F

What happened here is that batch C got reverted and removed. Delta D and E are updated, but D got updated into two deltas (keep in mind this is a very rare scenario and I am not sure it could even happen :)). Delta F was added after undo happened.

Let's assume there is incoming, external delta X. Remember that the remote client will not have any updated/removed deltas, just the originals. Let's crack down following scenarios. It could base on:

1. delta A, B, or any delta before C1.
2. delta C1
3. delta C2
4. delta D, E, or any delta between C2 and C1'
5. delta C2'
6. delta F.

Keep in mind, that the delta can't base on Da or Db because these are updated deltas and we don't send them to the server/other clients.

In fact, as I think of it now, it could also base on C1', which I haven't anticipated, because C1' and C2' are applied one after another, but they don't have to happen right after another on other client machine. Maybe we should/will have to introduce some control mechanisms, that one delta can be send/applied only together with other deltas?

Here are solutions:

1. There are no changed deltas above, so both client's histories were same when delta X got created. We can simply transform delta X by history. It's the same case as when undoing deltas.

2. Delta X bases on delta C1 that got reversed and removed from history. Deltas below remember the state "without delta C1". We have to convert delta X to the same state. To do so, we have to convert it by reversed C1 - this way it will "forget" about C1. After delta X is transformed to X without C it can be normally transformed by compressed history state. Keep in mind that converting by reversed deltas is not an efficiency cost -- because we will not transform X later by C1' etc. And we would have to do so if we keep original history. Also notice that it's what we do normally while undoing deltas.

3. and 4) are actually the same scenarios as 2). We have to transform by all previously reversed deltas to get X to the proper state.

4. and 6) delta X is already in a state after C happened and after C got reversed, so there is no need to do any special things.

If X bases on C1' it would need to be converted by C2' and all following deltas. So this would be another case, but similar to 2), 3) and 4). Anyway, for all those cases to work, we need to keep a registry of original/removed deltas, because we will be in need of transforming by them.

[pjasiun]

Thanks for the analysis. The remaining question is what should happen if one user undo. That reversed delta propagate, but how it should be applied. If it will be applied as a regular data we will have similar situation we had with undo v.1. It does not necessarily need to be an issue, since you can not undo other users changes. Alternatively we could compress deltas there too, but then we need to move the mechanism from the undo feature to the engine. Anyway, there are solution, what is enough for now.

[scofalik]

Yes, deltas send to the remote client are applied as they are. Efficiency might be a problem because those deltas are not removed on remote client. But there should not be any problem with correctness. I mean, after all, remote client has no idea whether the delta was generated by user "by hand", by undo feature or by other feature. The problems with correct results are only connected to undo mechanism.

As we discussed, it's not the dead end. Solutions exist if problems will really pop out.

[scofalik]

For now available batch types will be default and transparent - for batches that should be ignored by other features (initial batch, OT batches).

[pjasiun]

Please drop a comment there with decisions we agreed.

[scofalik]

Done.

[scofalik]

After I removed https://github.com/ckeditor/ckeditor5-engine/pull/499/files#diff-1ad6f65de8a59ca2d68c3601ab2c2aa5R164 a lot of errors popped up in tests because in early days we just applied operations without deltas. It's ~53 tests, with multiple operations in some of them. Let's move this issue to a follow-up: #501.