Introduced context for OT. Refactored: History, RemoveOperation, OT and other. #977

scofalik · 2017-06-22T21:09:36Z

Suggested merge commit message (convention)

Feature: OT uses context information for better conflict resolving.

This includes refactoring of: History, RemoveOperation, operational transformation algorithms, delta transformation algorithms and more.

Context will be used instead of removing deltas from history, which caused bugs in more complicated scenarios. This mostly affects undo algorithms.

BREAKING CHANGE: History API for deleting undone deltas has been removed.
BREAKING CHANGE: deltaTransform#transformDeltaSets is now protected. Use model.Document#transformDeltas instead.

Additional information:

RemoveOperation no longer creates $graveyardHolder elements. We are not using $graveyardHolder anywhere anymore.
RemoveOperation no longer sets its targetPosition on its own in constructor. targetPosition has to be passed to the constructor instead.
Batch#remove now creates multiple RemoveDeltas each with one RemoveOperation instead of one delta with multiple operations.
Introduced deltas normalization. Each delta has its set structure, for example MoveDelta has exactly one MoveOperation, SplitDelta has exactly InsertOperation and MoveOperation (or NoOperation) in this order, etc. After delta transformation using OT algorithms (default case), MoveOperation may get split into two, creating "not normal" deltas. For example SplitDelta with InsertOperation and two MoveOperations. This is normalized to one SplitDelta (insert + first move) and one MoveDelta (second move).
If RemoveOperation has been undone, it is not treated as "always more important" than the other operation, during RemoveOperation transformation.
Removed deltas priority. They were used to change isStrong flag (now context.isStrong). In this new approach it might make too much mess. Deltas priority was supposed to fix some problems in delta transformations. After refactoring and removing this mechanisms no tests fail anyway. Maybe some of problems were fixed along the way, maybe they are more hidden. Anyway there will be a followup to revise all delta transformation special cases, so we will return to that problem later.

scofalik · 2017-06-22T21:10:01Z

This needs PRs in other repos.

ckeditor/ckeditor5-undo#63
ckeditor/ckeditor5-typing#104
ckeditor/ckeditor5-upload#39

Reinmar · 2017-06-23T08:57:14Z

src/model/range.js

 	 * @returns {Boolean} `true` if given {@link ~Range range} boundaries are contained by this range, `false` otherwise.
 	 */
-	containsRange( otherRange ) {
-		return this.containsPosition( otherRange.start ) && this.containsPosition( otherRange.end );
+	containsRange( otherRange, strict = true ) {


The default of a boolean flag should be false.

You shouldn't need to pass false as a param.

Yes, I've seen you commented on it previously. I understand your point. My point was that this is creating double negation (nonStrict == false) which is sometimes confusing to read and understand when the code is longer (which is not the case here, but still I don't like having negation in variable/parameter name).

I know. So maybe weak?

I think that loose is a better term.

scofalik · 2017-06-24T17:40:54Z

I've added some more commits but messed up a bit and rebased instead of merge with master, so sorry for this :).

scofalik · 2017-06-24T22:53:22Z

I've created all needed PRs in other repos.

scofalik · 2017-06-25T23:00:07Z

I found a bug in undo during manual testing, fixing it right now.

… view.Range#containsRange.

…ltas as "undo pairs".

…caused the live range to change.

…oes not set `targetPosition` on its own. Added: `RemoveOperation#isPermanent`. Changed: `Batch#remove` now creates separate delta for each `RemoveOperation` instead of creating one delta with multiple operations. Added: `isPermanent` parameter in `Batch#remove`.

… in `LiveRange` and `RemoveOperation`.

…ations on reverse.

…for deltas transformation (instead of `deltaTransform.transformDeltaSets` which is protected).

…ame and default value from strict to loose.

…rsed()` were incorrect after `RemoveOperation` no longer creates `$graveyardHolder`.

…sformations since `RemoveOperation` no longer creates `graveyardHolder`.

…ration.log()`.

…ration.

…tion is `NoOperation`.

…sformation.

…eOperation` x `MoveOperation` transformation.

…ds on document.history, better context setting, deltas normalization.

…done to operations and deltas transformations.

scofalik · 2017-07-05T09:55:56Z

Okay, I've rewritten and fixed a lot of stuff in deltas transformations and operations transformations. For now it is okay, however I will create several followups.

… needed for correct transformation.

scofalik · 2017-07-05T12:52:50Z

I've updated related PRs and checked that all branches together work fine.

…n `MoveOperation#fromJson`.

scofalik · 2017-07-05T14:05:16Z

I've added follow up issues.

Reinmar · 2017-07-05T17:05:46Z

I pushed remove-refactor to ckeditor5 with the branches setup.

Reinmar · 2017-07-05T17:06:16Z

Please resolve conflicts in ckeditor/ckeditor5-undo#63.

Reinmar · 2017-07-06T14:51:51Z

src/model/operation/transform.js

-				// Here we do not need to worry that newTargetPosition is inside moved range, because that
-				// would mean that the MoveOperation targets into itself, and that is incorrect operation.
-				// Instead, we calculate the new position of that part of original range.
+			// // If MoveOperations has common range it can be one of two:


Was it left like this intentionally?

Reinmar · 2017-07-06T14:53:20Z

src/model/operation/transform.js

@@ -52,12 +53,11 @@ import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
 * `a` by `b` and also `b` by `a`. In both transformations the same operation has to be the important one. If we assume
 * that first or the second passed operation is always more important we won't be able to solve this case.
 *
- * @external module:engine/model/model.operation
+ * @protected


I'll remove this because it creates a precedence that we have a protected export. We should rather invest in marking internal modules all across the code.

Reinmar · 2017-07-06T14:53:46Z

src/model/operation/transform.js

-			} );
+			return makeMoveOperationsFromRanges( a, ranges, newTargetPosition );
+
+			// // Map transformed range(s) to operations and return them.


And here again – was it left intentionally?

Reinmar · 2017-07-06T14:55:52Z

src/model/range.js

 	}

 	/**
-	 * Two ranges are equal if their {@link #start start} and
-	 * {@link #end end} positions are equal.
+	 * Two ranges are equal if their {@link #start start} and {@link #end end} positions are equal.


No need to type #start start. Just leave the property or class name unless you want to change the wording a bit.

Internal: Undo algorithms were simplified thanks to changes in the model. See ckeditor/ckeditor5-engine#977.

Reinmar · 2017-07-06T16:06:40Z

Please close tickets which should be closed now (and please cross-link them for the future reference).

Also, please check issues that you assigned to yourself, whether you're still working on them.

scofalik · 2017-07-07T08:19:54Z

The commented out code was left there by mistake. I already pushed a commit to master fixing it.

Reinmar reviewed Jun 23, 2017

View reviewed changes

This was referenced Jun 23, 2017

Simplified undo commands due to simplification in engine.model.History. ckeditor/ckeditor5-undo#63

Merged

Tests: Updated test due to selection restoring algorithm change in undo. ckeditor/ckeditor5-typing#104

Merged

scofalik force-pushed the remove-refactor branch from f4ba06f to fb2bd15 Compare June 24, 2017 17:40

scofalik force-pushed the remove-refactor branch from fb2bd15 to 4e60a99 Compare June 24, 2017 17:43

scofalik mentioned this pull request Jun 24, 2017

T/38 Make remove on image abortion permanent ckeditor/ckeditor5-upload#39

Merged

scofalik added the status:review- label Jun 25, 2017

scofalik added 19 commits July 5, 2017 11:53

Added: Introduced strict parameter in model.Range#containsRange and…

8a0561e

… view.Range#containsRange.

Changed: removed deleting deltas from model.History, added marking de…

8e16b45

…ltas as "undo pairs".

Added: model.LiveRange now passes data about change in document that …

a15c43b

…caused the live range to change.

Changed: update LiveSelection range fixing algorithms after changes…

8ecb14b

… in `LiveRange` and `RemoveOperation`.

Changed: MergeDelta and SplitDelta should create sticky move oper…

6715a44

…ations on reverse.

Added: introduce context in operations and delta transformations.

aeab5c5

Added: Introduced model.Document#transformDeltas as an entry point …

fd3d3e3

…for deltas transformation (instead of `deltaTransform.transformDeltaSets` which is protected).

Changed: model.Range and view.Range#containsRange changed parameter n…

f5e8eb1

…ame and default value from strict to loose.

Fixed: RemoveOperation#isPermanent sometimes had no effect during OT.

e92c85d

Fixed: RemoveOperation#getReversed() and `ReinsertOperation#getReve…

3288c53

…rsed()` were incorrect after `RemoveOperation` no longer creates `$graveyardHolder`.

Fixed: Remove and reinsert change types needs fixes during range tran…

d9602ad

…sformations since `RemoveOperation` no longer creates `graveyardHolder`.

Changed: Added logging whether MoveOperation is sticky for `MoveOpe…

e9c1399

…ration.log()`.

Fixed: Prevent errors in AttributeDelta if one of operations is NoOpe…

b3e50b9

…ration.

Fixed: SplitDelta#_moveOperation should return null if second opera…

f5dc130

…tion is `NoOperation`.

Docs: Added "missing" example in a comment.

dc32f66

Changed: context.insertBefore now also affects InsertOperation tran…

5632999

…sformation.

Changed: refactored MoveOperation x MoveOperation transformation.

e2ae4e4

Tests: fixed tests due to "move operation normalization" done in `Mov…

1dd3adc

…eOperation` x `MoveOperation` transformation.

scofalik added 2 commits July 5, 2017 11:53

Fixed: Fixes in deltas transformations: context.forceWeakRemove depen…

94becc4

…ds on document.history, better context setting, deltas normalization.

Tests: Fixed and added tests for delta transformations after changes …

ce3aaa2

…done to operations and deltas transformations.

scofalik force-pushed the remove-refactor branch from 4e60a99 to ce3aaa2 Compare July 5, 2017 09:53

scofalik removed the status:review- label Jul 5, 2017

Removed: RemoveOperation#isPermanent was removed as it is no longer…

5761db3

… needed for correct transformation.

Changed: Removed RemoveOperation#fromJson that did nothing more tha…

7d41642

…n `MoveOperation#fromJson`.

Reinmar reviewed Jul 6, 2017

View reviewed changes

Minor API docs fixes.

1a9933e

Reinmar merged commit 481eb9b into master Jul 6, 2017

Reinmar deleted the remove-refactor branch July 6, 2017 16:03

Reinmar added a commit to ckeditor/ckeditor5-undo that referenced this pull request Jul 6, 2017

Merge pull request #63 from ckeditor/ckeditor5-engine/remove-refactor

2843005

Internal: Undo algorithms were simplified thanks to changes in the model. See ckeditor/ckeditor5-engine#977.

Reinmar mentioned this pull request Jul 31, 2017

📅 News and updates: July 31, 2017 ckeditor/ckeditor5-design#178

Open

Reinmar mentioned this pull request Sep 4, 2017

🏁 Release: Iteration 11 – 11th developer preview ckeditor/ckeditor5-design#180

Open

Reinmar mentioned this pull request Oct 9, 2019

Selection restoring – take two ckeditor/ckeditor5#2717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced context for OT. Refactored: History, RemoveOperation, OT and other. #977

Introduced context for OT. Refactored: History, RemoveOperation, OT and other. #977

scofalik commented Jun 22, 2017 •

edited

Loading

scofalik commented Jun 22, 2017 •

edited

Loading

Reinmar Jun 23, 2017

Reinmar Jun 23, 2017

scofalik Jun 23, 2017 •

edited

Loading

Reinmar Jun 23, 2017

scofalik Jun 23, 2017

scofalik commented Jun 24, 2017

scofalik commented Jun 24, 2017

scofalik commented Jun 25, 2017

scofalik commented Jul 5, 2017

scofalik commented Jul 5, 2017

scofalik commented Jul 5, 2017

Reinmar commented Jul 5, 2017

Reinmar commented Jul 5, 2017

Reinmar Jul 6, 2017

Reinmar Jul 6, 2017

Reinmar Jul 6, 2017

Reinmar Jul 6, 2017

Reinmar commented Jul 6, 2017

scofalik commented Jul 7, 2017

Introduced context for OT. Refactored: History, RemoveOperation, OT and other. #977

Introduced context for OT. Refactored: History, RemoveOperation, OT and other. #977

Conversation

scofalik commented Jun 22, 2017 • edited Loading

Suggested merge commit message (convention)

scofalik commented Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik commented Jun 24, 2017

scofalik commented Jun 24, 2017

scofalik commented Jun 25, 2017

scofalik commented Jul 5, 2017

scofalik commented Jul 5, 2017

scofalik commented Jul 5, 2017

Reinmar commented Jul 5, 2017

Reinmar commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Jul 6, 2017

scofalik commented Jul 7, 2017

scofalik commented Jun 22, 2017 •

edited

Loading

scofalik commented Jun 22, 2017 •

edited

Loading

scofalik Jun 23, 2017 •

edited

Loading