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

Introduce ModelDocument.applyDelta #3768

Closed
scofalik opened this issue Jun 24, 2016 · 8 comments
Closed

Introduce ModelDocument.applyDelta #3768

scofalik opened this issue Jun 24, 2016 · 8 comments
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@scofalik
Copy link
Contributor

We should introduce applyDelta method in engine.model.Document and use it from now on. We decided that operations should always be applied in deltas. Not having such method right now cripples us a bit, because in many places of code we need to make redundant loops through all operations in delta + we don't have a one way to inform the editor that delta was applied (from start to end). We need this for OT plugin, because OT should recognize when delta starts and when delta ends and should send operations packed in deltas.

@pjasiun
Copy link

pjasiun commented Jun 24, 2016

Although such method would be useful for OT I am unsure how do you see regular deltas working with it. For instance what split delta do is creating a new element and moving nodes to this element. The element need to be created, so the insert operation need to be applied, to move nodes to it.

We could create a move operation on the future position, not existing yet, but it makes deltas creation much more complicated.

@scofalik
Copy link
Contributor Author

It's not like creating deltas is your everyday bread and butter and it's not like it becomes 10 lines of code instead of 1. We will need some improvements in this part of API soon anyway, so I'd keep it in milestone 0.2 as it is and we can discuss it.

@scofalik
Copy link
Contributor Author

scofalik commented Sep 7, 2016

We have two ways to create deltas:

  1. Through Batch API - then each operation is created "alone", added and applied to the document, one after one.
  2. Through Collaborative Editing and Undo - then the whole delta is created at once by operational transformation. When you apply first operation of such delta, it already has all other operations added.

This causes problems. I.e. in Typing feature, in ChangeBuffer class, it checked whether there is one operation in the batch. That caused a bug, because deltas created in Undo did not pass that check and in return did not reset ChangeBuffer batch.

There are more and more reasons to introduce applyDelta and get rid off operation-by-operation approach. Honestly, I multiple times had to write some work-around code because of how deltas were written originally.

And as far as SplitDelta or MergeDelta are concerned. It is very easy to calculate positions for operations inside those deltas, so this will not be a problem.

@Reinmar
Copy link
Member

Reinmar commented Sep 8, 2016

And what's the reason for how the API looks currently?

@scofalik
Copy link
Contributor Author

scofalik commented Sep 8, 2016

Historical reasons. We first had operations and we based editors foundations on them, without knowing if deltas are good idea.

@pjasiun
Copy link

pjasiun commented Sep 9, 2016

C'mon. It's not a historical reason. This is to let you create deltas in an easy way: creating operation one after another. So now you:

  • create operation 1,
  • apply operation 1,
  • create operation 2, which is simply because it operates on the document it will be applied,
  • apply operation 2.

If we want to apply the whole delta you need to create operation 2 in the way it will work fine on the different state of the document. And the idea is that the delta API is scalable, any plugin should be able to create and add delta. And calculating position seems to be tricky for 3-rd party develpers.

On the other hand, to be honest, I do not believe that 3-rd party developers will create new deltas, so applyDelta might be the way to go. It will complicate the deltas API, but simplified others. But do not say that it is "historical reason" since you know it is not.

@scofalik
Copy link
Contributor Author

Well, we kept applyOperation because Deltas are using this API - you are right in this regard. But the API itself was created like this before we even had deltas concept finished.

@scofalik
Copy link
Contributor Author

Since we will change deltas to operations, this issue is invalid.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

4 participants