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

OT: NoOperation and "no delta" should be used carefully. #4097

Closed
scofalik opened this issue Jul 5, 2017 · 1 comment
Closed

OT: NoOperation and "no delta" should be used carefully. #4097

scofalik opened this issue Jul 5, 2017 · 1 comment
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:bug This issue reports a buggy (incorrect) behavior.

Comments

@scofalik
Copy link
Contributor

scofalik commented Jul 5, 2017

(Follow-up to: ckeditor/ckeditor5-engine#977)

Long story short: NoOperation (and no delta, that is a Delta that has just one or more NoOperations) are dangerous when it comes to OT. We should never change transformed operation to NoOperation and transformed delta to "no delta".

Since, at the entry point (model.Document#transformDeltas), it is assumed that "incoming" deltas are always stronger (whether they come from undo or collaborative editing), we need to check all algorithms if they may return NoOperation / "no delta" when context.isStrong == true.

Then, we have to check if it is safe to have NoOperation as a result at all during operation. From what I've been thinking about it, it should be safe, but maybe there are some complicated cases when it is harmful.

Also we need to check whether it is safe to have some deltas with NoOperation instead of MoveOperation.


Here is an explanation why this change is important. You may read it to have a better understanding of the issue. Consider two following case that might happen in collaborative editing:

  1. User A changes node's attribute bold to true. Then they change it to false.
  2. At the same time User B changes node's attribute to bold.
  3. User B's operation goes to User A and is transformed by "change bold to true". Since both operations are same, User B's operation might be changes to NoOperation. But then we lose all the context. In next operation, User A "reverts" his change, but User B's operation is already a NoOperation and we have no way to get it back to "change bold to true".

It's plain that we cannot change transformed operation to NoOperation because we are losing information this way. Above example is simple but there might be other cases that went unnoticed.

@scofalik
Copy link
Contributor Author

I think this was already more or less done and I don't recall us having this issue anymore. Also, deltas will be changed to operations soon and this will probably lead to some slight refactor in OT algorithms. I will remember about it then.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added module:model resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:bug This issue reports a buggy (incorrect) behavior. 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). type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

2 participants