From 3dc405254a7764c85f6cf4ef85a5271e13c72361 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 16 Oct 2018 18:07:44 +0200 Subject: [PATCH 1/3] Fix: `MoveOperation` x `SplitOperation` transformation for a case when graveyard element is moved. --- src/model/operation/transform.js | 73 ++++++++++++++++++------ src/model/range.js | 11 ++-- tests/model/operation/transform/merge.js | 56 +++++++++++++++++- 3 files changed, 115 insertions(+), 25 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 51852430b..f2dd78341 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1669,12 +1669,21 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => { // The default case. // const transformed = moveRange._getTransformedBySplitOperation( b ); + const ranges = [ transformed ]; - a.sourcePosition = transformed.start; - a.howMany = transformed.end.offset - transformed.start.offset; - a.targetPosition = newTargetPosition; + // Case 5: + // + // Moved range contains graveyard element used by split operation. Add extra move operation to the result. + // + if ( b.graveyardPosition ) { + const movesGraveyardElement = moveRange.start.isEqual( b.graveyardPosition ) || moveRange.containsPosition( b.graveyardPosition ); - return [ a ]; + if ( a.howMany > 1 && movesGraveyardElement ) { + ranges.push( Range.createFromPositionAndShift( b.insertionPosition, 1 ) ); + } + } + + return _makeMoveOperationsFromRanges( ranges, newTargetPosition ); } ); setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { @@ -1692,16 +1701,30 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { // removed nodes might be unexpected. This means that in this scenario we will reverse merging and remove the element. // if ( !context.aWasUndone ) { - const gyMoveTarget = Position.createFromPosition( b.graveyardPosition ); - const gyMove = new MoveOperation( b.graveyardPosition, 1, gyMoveTarget, 0 ); + const results = []; - const targetPositionPath = b.graveyardPosition.path.slice(); + let gyMoveSource = Position.createFromPosition( b.graveyardPosition ); + let splitNodesMoveSource = Position.createFromPosition( b.targetPosition ); + + if ( a.howMany > 1 ) { + results.push( new MoveOperation( a.sourcePosition, a.howMany - 1, a.targetPosition, 0 ) ); + gyMoveSource = gyMoveSource._getTransformedByInsertion( a.targetPosition, a.howMany - 1 ); + splitNodesMoveSource = splitNodesMoveSource._getTransformedByMove( a.sourcePosition, a.targetPosition, a.howMany - 1 ); + } + + const gyMoveTarget = b.deletionPosition._getCombined( a.sourcePosition, a.targetPosition ); + const gyMove = new MoveOperation( gyMoveSource, 1, gyMoveTarget, 0 ); + + const targetPositionPath = gyMove.getMovedRangeStart().path.slice(); targetPositionPath.push( 0 ); - return [ - gyMove, - new MoveOperation( b.targetPosition, b.howMany, new Position( a.targetPosition.root, targetPositionPath ), 0 ) - ]; + const splitNodesMoveTarget = new Position( gyMove.targetPosition.root, targetPositionPath ); + const splitNodesMove = new MoveOperation( splitNodesMoveSource, b.howMany, splitNodesMoveTarget, 0 ); + + results.push( gyMove ); + results.push( splitNodesMove ); + + return results; } } else { // Case 2: @@ -1934,14 +1957,21 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { if ( a.graveyardPosition ) { // Case 1: // - // Split operation graveyard node was moved. In this case move operation is stronger and the split insertion position - // should be corrected. + // Split operation graveyard node was moved. In this case move operation is stronger. Since graveyard element + // is already moved to the correct position, we need to only move the nodes after the split position. + // This will be done by `MoveOperation` instead of `SplitOperation`. // - if ( rangeToMove.containsPosition( a.graveyardPosition ) || rangeToMove.start.isEqual( a.graveyardPosition ) ) { - a.insertionPosition = Position.createFromPosition( b.targetPosition ); - a.splitPosition = a.splitPosition._getTransformedByMoveOperation( b ); + if ( rangeToMove.start.isEqual( a.graveyardPosition ) || rangeToMove.containsPosition( a.graveyardPosition ) ) { + const sourcePosition = a.splitPosition._getTransformedByMoveOperation( b ); - return [ a ]; + const newParentPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); + const newTargetPath = newParentPosition.path.slice(); + newTargetPath.push( 0 ); + + const newTargetPosition = new Position( newParentPosition.root, newTargetPath ); + const moveOp = new MoveOperation( sourcePosition, a.howMany, newTargetPosition, 0 ); + + return [ moveOp ]; } a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); @@ -2172,7 +2202,14 @@ function _makeMoveOperationsFromRanges( ranges, targetPosition ) { for ( let i = 0; i < ranges.length; i++ ) { // Create new operation out of a range and target position. const range = ranges[ i ]; - const op = new MoveOperation( range.start, range.end.offset - range.start.offset, targetPosition, 0 ); + const op = new MoveOperation( + range.start, + range.end.offset - range.start.offset, + // If the target is the end of the move range this operation doesn't really move anything. + // In this case, it is better for OT to use range start instead of range end. + targetPosition.isEqual( range.end ) ? range.start : targetPosition, + 0 + ); operations.push( op ); diff --git a/src/model/range.js b/src/model/range.js index 143c83a8a..9b9340cf2 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -517,13 +517,16 @@ export default class Range { */ _getTransformedBySplitOperation( operation ) { const start = this.start._getTransformedBySplitOperation( operation ); - - let end; + let end = this.end._getTransformedBySplitOperation( operation ); if ( this.end.isEqual( operation.insertionPosition ) ) { end = this.end.getShiftedBy( 1 ); - } else { - end = this.end._getTransformedBySplitOperation( operation ); + } + + // Below may happen when range contains graveyard element used by split operation. + if ( start.root != end.root ) { + // End position was next to the moved graveyard element and was moved with it. Fix it. + end = this.end.getShiftedBy( -1 ); } return new Range( start, end ); diff --git a/tests/model/operation/transform/merge.js b/tests/model/operation/transform/merge.js index 4e3a54dff..306299561 100644 --- a/tests/model/operation/transform/merge.js +++ b/tests/model/operation/transform/merge.js @@ -174,9 +174,7 @@ describe( 'transform', () => { kate.undo(); syncClients(); - expectClients( - 'Foo' - ); + expectClients( 'Foo' ); } ); } ); @@ -222,6 +220,40 @@ describe( 'transform', () => { expectClients( 'FooBar' ); } ); + + it( 'remove merged element then undo #3', () => { + john.setData( '[AB]C' ); + kate.setData( 'A[]BC' ); + + kate.merge(); + john.remove(); + + syncClients(); + expectClients( 'C' ); + + john.undo(); + kate.undo(); + + syncClients(); + expectClients( 'ABC' ); + } ); + + it( 'remove merged element then undo #4', () => { + john.setData( 'A[BC]' ); + kate.setData( 'A[]BC' ); + + kate.merge(); + john.remove(); + + syncClients(); + expectClients( 'A' ); + + john.undo(); + kate.undo(); + + syncClients(); + expectClients( 'ABC' ); + } ); } ); describe( 'by delete', () => { @@ -409,5 +441,23 @@ describe( 'transform', () => { ); } ); } ); + + describe( 'by split', () => { + it( 'merge element which got split (the element is in blockquote) and undo', () => { + john.setData( 'Foo
[]Bar
' ); + kate.setData( 'Foo
B[]ar
' ); + + john._processExecute( 'delete' ); + kate.split(); + + syncClients(); + expectClients( 'FooBar' ); + + john.undo(); + + syncClients(); + expectClients( 'Foo
Bar
' ); + } ); + } ); } ); } ); From 623280d50596f3c85e1f866857fc324a63aac4a5 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 24 Oct 2018 13:34:49 +0200 Subject: [PATCH 2/3] Fix: Fixed CC drop in `.sort` comparing function. --- src/model/differ.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/differ.js b/src/model/differ.js index ed1e8c0ca..69ab36b51 100644 --- a/src/model/differ.js +++ b/src/model/differ.js @@ -423,7 +423,7 @@ export default class Differ { // If change happens at the same position... if ( a.position.isEqual( b.position ) ) { // Keep chronological order of operations. - return a.changeCount < b.changeCount ? -1 : 1; + return a.changeCount - b.changeCount; } // If positions differ, position "on the left" should be earlier in the result. From 87fdbd6afcedb723946e555f4abb9c3cb41f51b5 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 24 Oct 2018 13:49:35 +0200 Subject: [PATCH 3/3] Docs: Clarified a comment. --- src/model/range.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/model/range.js b/src/model/range.js index 9b9340cf2..61b3d340e 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -525,7 +525,8 @@ export default class Range { // Below may happen when range contains graveyard element used by split operation. if ( start.root != end.root ) { - // End position was next to the moved graveyard element and was moved with it. Fix it. + // End position was next to the moved graveyard element and was moved with it. + // Fix it by using old `end` which has proper `root`. end = this.end.getShiftedBy( -1 ); }