From 0e99fc5b7eb47ac9602380f085c5f93242363019 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 24 Aug 2017 16:40:57 +0200 Subject: [PATCH] Fixed: MergeDelta special case transformation throws when second operation is NoOperation. --- src/model/delta/basic-transformations.js | 22 ++++++++- tests/model/delta/transform/insertdelta.js | 25 ++++++++++ tests/model/delta/transform/mergedelta.js | 57 ++++++++++++++++++++++ tests/model/delta/transform/movedelta.js | 27 ++++++++++ 4 files changed, 129 insertions(+), 2 deletions(-) diff --git a/src/model/delta/basic-transformations.js b/src/model/delta/basic-transformations.js index 3192e4f37..8ff89dc2d 100644 --- a/src/model/delta/basic-transformations.js +++ b/src/model/delta/basic-transformations.js @@ -97,6 +97,11 @@ addTransformationCase( AttributeDelta, SplitDelta, ( a, b, context ) => { // Add special case for InsertDelta x MergeDelta transformation. addTransformationCase( InsertDelta, MergeDelta, ( a, b, context ) => { + // Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation. + if ( !b.position ) { + return defaultTransform( a, b, context ); + } + const undoMode = context.aWasUndone || context.bWasUndone; // If insert is applied at the same position where merge happened, we reverse the merge (we treat it like it @@ -136,10 +141,14 @@ addTransformationCase( MarkerDelta, RenameDelta, transformMarkerDelta ); // Add special case for MoveDelta x MergeDelta transformation. addTransformationCase( MoveDelta, MergeDelta, ( a, b, context ) => { + // Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation. + if ( !b.position ) { + return defaultTransform( a, b, context ); + } + // If move delta is supposed to move a node that has been merged, we reverse the merge (we treat it like it // didn't happen) and then apply the original move operation. This is "mirrored" in MergeDelta x MoveDelta // transformation below, where we simply do not apply MergeDelta. - const operateInSameParent = a.sourcePosition.root == b.position.root && compareArrays( a.sourcePosition.getParentPath(), b.position.getParentPath() ) === 'same'; @@ -158,6 +167,11 @@ addTransformationCase( MoveDelta, MergeDelta, ( a, b, context ) => { // Add special case for MergeDelta x InsertDelta transformation. addTransformationCase( MergeDelta, InsertDelta, ( a, b, context ) => { + // Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation. + if ( !a.position ) { + return defaultTransform( a, b, context ); + } + const undoMode = context.aWasUndone || context.bWasUndone; // If merge is applied at the same position where we inserted a range of nodes we cancel the merge as it's results @@ -171,9 +185,13 @@ addTransformationCase( MergeDelta, InsertDelta, ( a, b, context ) => { // Add special case for MergeDelta x MoveDelta transformation. addTransformationCase( MergeDelta, MoveDelta, ( a, b, context ) => { + // Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation. + if ( !a.position ) { + return defaultTransform( a, b, context ); + } + // If merge is applied at the position between moved nodes we cancel the merge as it's results may be unexpected and // very weird. Even if we do some "magic" we don't know what really are users' expectations. - const operateInSameParent = a.position.root == b.sourcePosition.root && compareArrays( a.position.getParentPath(), b.sourcePosition.getParentPath() ) === 'same'; diff --git a/tests/model/delta/transform/insertdelta.js b/tests/model/delta/transform/insertdelta.js index d78e61b80..f883ec9ec 100644 --- a/tests/model/delta/transform/insertdelta.js +++ b/tests/model/delta/transform/insertdelta.js @@ -18,6 +18,7 @@ import SplitDelta from '../../../../src/model/delta/splitdelta'; import InsertOperation from '../../../../src/model/operation/insertoperation'; import MoveOperation from '../../../../src/model/operation/moveoperation'; import ReinsertOperation from '../../../../src/model/operation/reinsertoperation'; +import NoOperation from '../../../../src/model/operation/nooperation'; import { getNodesAndText } from '../../../../tests/model/_utils/utils'; @@ -156,6 +157,30 @@ describe( 'transform', () => { } ); } ); + it( 'merge in same position as insert - undo mode', () => { + // If MergeDelta second operation is NoOperation, default transformation algorithm should be used. + const mergeDelta = getMergeDelta( insertPosition, 4, 12, baseVersion ); + mergeDelta.operations[ 1 ] = new NoOperation( 1 ); + + const transformed = transform( insertDelta, mergeDelta, context ); + + baseVersion = mergeDelta.operations.length; + + expect( transformed.length ).to.equal( 1 ); + + expectDelta( transformed[ 0 ], { + type: InsertDelta, + operations: [ + { + type: InsertOperation, + position: Position.createFromPosition( insertPosition ), + nodes: [ nodeA, nodeB ], + baseVersion + } + ] + } ); + } ); + it( 'merge the node that is parent of insert position (sticky move test)', () => { const mergeDelta = getMergeDelta( new Position( root, [ 3, 3 ] ), 1, 4, baseVersion ); const transformed = transform( insertDelta, mergeDelta, context ); diff --git a/tests/model/delta/transform/mergedelta.js b/tests/model/delta/transform/mergedelta.js index a2f6e8d4f..10e0efb14 100644 --- a/tests/model/delta/transform/mergedelta.js +++ b/tests/model/delta/transform/mergedelta.js @@ -120,6 +120,34 @@ describe( 'transform', () => { } ); } ); + it( 'insert at same position as merge - merge second operation is NoOperation', () => { + // If MergeDelta second operation is NoOperation, default transformation algorithm should be used. + mergeDelta.operations[ 1 ] = new NoOperation( 1 ); + const insertDelta = getInsertDelta( mergePosition, [ nodeA, nodeB ], baseVersion ); + const transformed = transform( mergeDelta, insertDelta, context ); + + baseVersion = insertDelta.operations.length; + + expect( transformed.length ).to.equal( 1 ); + + expectDelta( transformed[ 0 ], { + type: MergeDelta, + operations: [ + { + type: MoveOperation, + sourcePosition: new Position( root, [ 3, 3, 5, 0 ] ), + howMany: mergeDelta.operations[ 0 ].howMany, + targetPosition: mergeDelta.operations[ 0 ].targetPosition, + baseVersion + }, + { + type: NoOperation, + baseVersion: baseVersion + 1 + } + ] + } ); + } ); + it( 'insert inside merged node (sticky move test)', () => { const insertDelta = getInsertDelta( new Position( root, [ 3, 3, 3, 12 ] ), [ nodeA, nodeB ], baseVersion ); const transformed = transform( mergeDelta, insertDelta, context ); @@ -227,6 +255,35 @@ describe( 'transform', () => { // MoveDelta is applied. MergeDelta is discarded. expect( nodesAndText ).to.equal( 'DIVXabcdabcfoobarxyzXXXXXDIV' ); } ); + + it( 'node on the left side of merge was moved - second merge operation is NoOperation', () => { + // If second MergeDelta operation is NoOperation default algorithm should be used. + mergeDelta.operations[ 1 ] = new NoOperation( 1 ); + + const moveDelta = getMoveDelta( new Position( root, [ 3, 3, 2 ] ), 1, new Position( root, [ 3, 3, 0 ] ), baseVersion ); + const transformed = transform( mergeDelta, moveDelta, context ); + + expect( transformed.length ).to.equal( 1 ); + + baseVersion = moveDelta.operations.length; + + expectDelta( transformed[ 0 ], { + type: MergeDelta, + operations: [ + { + type: MoveOperation, + sourcePosition: new Position( root, [ 3, 3, 3, 0 ] ), + howMany: 12, + targetPosition: new Position( root, [ 3, 3, 0, 4 ] ), + baseVersion + }, + { + type: NoOperation, + baseVersion: baseVersion + 1 + } + ] + } ); + } ); } ); describe( 'MergeDelta', () => { diff --git a/tests/model/delta/transform/movedelta.js b/tests/model/delta/transform/movedelta.js index d1dc9944d..b5134f77e 100644 --- a/tests/model/delta/transform/movedelta.js +++ b/tests/model/delta/transform/movedelta.js @@ -15,6 +15,7 @@ import MoveDelta from '../../../../src/model/delta/movedelta'; import SplitDelta from '../../../../src/model/delta/splitdelta'; import MoveOperation from '../../../../src/model/operation/moveoperation'; +import NoOperation from '../../../../src/model/operation/nooperation'; import { getNodesAndText } from '../../../../tests/model/_utils/utils'; @@ -159,6 +160,32 @@ describe( 'transform', () => { ] } ); } ); + + it( 'node on the right side of merge was moved - second merge operation is NoOperation', () => { + // If second MergeDelta operation is NoOperation default algorithm should be used. + const mergePosition = new Position( root, [ 3, 3, 3 ] ); + const mergeDelta = getMergeDelta( mergePosition, 4, 12, baseVersion ); + mergeDelta.operations[ 1 ] = new NoOperation( 1 ); + + const transformed = transform( moveDelta, mergeDelta, context ); + + expect( transformed.length ).to.equal( 1 ); + + baseVersion = mergeDelta.operations.length; + + expectDelta( transformed[ 0 ], { + type: MoveDelta, + operations: [ + { + type: MoveOperation, + sourcePosition: moveDelta._moveOperation.sourcePosition, + howMany: moveDelta._moveOperation.howMany, + targetPosition: moveDelta._moveOperation.targetPosition, + baseVersion + } + ] + } ); + } ); } ); } ); } );