From e04c27d5af5e910225a9e6895b3d7efefc13b1f8 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 23 Aug 2017 12:49:29 +0200 Subject: [PATCH] Fixed: Fixes for deltas transformations. --- src/model/delta/basic-transformations.js | 70 ++++++++++++++++------ tests/model/delta/transform/insertdelta.js | 24 ++++++++ tests/model/delta/transform/mergedelta.js | 32 ++++++++++ tests/model/delta/transform/splitdelta.js | 42 +++++++++++-- 4 files changed, 147 insertions(+), 21 deletions(-) diff --git a/src/model/delta/basic-transformations.js b/src/model/delta/basic-transformations.js index 9c3c07744..3192e4f37 100644 --- a/src/model/delta/basic-transformations.js +++ b/src/model/delta/basic-transformations.js @@ -17,6 +17,7 @@ import Position from '../position'; import NoOperation from '../operation/nooperation'; import AttributeOperation from '../operation/attributeoperation'; import InsertOperation from '../operation/insertoperation'; +import ReinsertOperation from '../operation/reinsertoperation'; import Delta from './delta'; import AttributeDelta from './attributedelta'; @@ -96,10 +97,12 @@ addTransformationCase( AttributeDelta, SplitDelta, ( a, b, context ) => { // Add special case for InsertDelta x MergeDelta transformation. addTransformationCase( InsertDelta, MergeDelta, ( 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 // didn't happen) and then apply the original insert operation. This is "mirrored" in MergeDelta x InsertDelta // transformation below, where we simply do not apply MergeDelta. - if ( a.position.isEqual( b.position ) ) { + if ( !undoMode && a.position.isEqual( b.position ) ) { return [ b.getReversed(), a.clone() @@ -155,9 +158,11 @@ addTransformationCase( MoveDelta, MergeDelta, ( a, b, context ) => { // Add special case for MergeDelta x InsertDelta transformation. addTransformationCase( MergeDelta, InsertDelta, ( 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 // may be unexpected and very weird. Even if we do some "magic" we don't know what really are users' expectations. - if ( a.position.isEqual( b.position ) ) { + if ( !undoMode && a.position.isEqual( b.position ) ) { return [ noDelta() ]; } @@ -182,8 +187,14 @@ addTransformationCase( MergeDelta, MoveDelta, ( a, b, context ) => { return defaultTransform( a, b, context ); } ); -// Add special case for SplitDelta x SplitDelta transformation. addTransformationCase( SplitDelta, SplitDelta, ( a, b, context ) => { + const undoMode = context.aWasUndone || context.bWasUndone; + + // Do not apply special transformation case if transformation is in undo mode. + if ( undoMode ) { + return defaultTransform( a, b, context ); + } + // Do not apply special transformation case if `SplitDelta` has `NoOperation` as the second operation. if ( !a.position || !b.position ) { return defaultTransform( a, b, context ); @@ -194,23 +205,48 @@ addTransformationCase( SplitDelta, SplitDelta, ( a, b, context ) => { // The special case is for splits inside the same parent. if ( a.position.root == b.position.root && compareArrays( pathA, pathB ) == 'same' ) { - const newContext = Object.assign( {}, context ); + a = a.clone(); + + if ( a.position.offset <= b.position.offset ) { + // If both first operations are `ReinsertOperation`s, we might need to transform `a._cloneOperation`, + // so it will take correct node from graveyard. + if ( + a._cloneOperation instanceof ReinsertOperation && b._cloneOperation instanceof ReinsertOperation && + a._cloneOperation.sourcePosition.offset > b._cloneOperation.sourcePosition.offset + ) { + a._cloneOperation.sourcePosition.offset--; + } - // If `a` delta splits in further location, make sure that it will move some nodes by forcing it to be strong. - // Similarly, if `a` splits closer, make sure that it is transformed accordingly. - if ( a.position.offset != b.position.offset ) { - // We need to ensure that incoming operation is strong / weak. - newContext.isStrong = a.position.offset > b.position.offset; - } + // `a` splits closer or at same offset. + // Change how many nodes are moved. Do not move nodes that were moved by delta `b`. + const aRange = Range.createFromPositionAndShift( a.position, a._moveOperation.howMany ); + const bRange = Range.createFromPositionAndShift( b.position, b._moveOperation.howMany ); - // Then, default transformation is almost good. - // We need to change insert operations offsets, though. - // We will use `context.insertBefore` for this (but only if it is not set!) - if ( context.insertBefore === undefined ) { - newContext.insertBefore = newContext.isStrong; - } + const diff = aRange.getDifference( bRange ); + + let newHowMany = 0; + + for ( const range of diff ) { + newHowMany += range.end.offset - range.start.offset; + } - return defaultTransform( a, b, newContext ); + if ( newHowMany === 0 ) { + a.operations.pop(); // Remove last operation (`MoveOperation`). + a.addOperation( new NoOperation( a.operations[ 0 ].baseVersion + 1 ) ); // Add `NoOperation` instead. + } else { + a.operations[ 1 ].howMany = newHowMany; + } + + return [ a ]; + } else { + // `a` splits further. + // This is more complicated case, thankfully we can solve it using default transformation and setting proper context. + const newContext = Object.assign( {}, context ); + newContext.isStrong = true; + newContext.insertBefore = true; + + return defaultTransform( a, b, newContext ); + } } return defaultTransform( a, b, context ); diff --git a/tests/model/delta/transform/insertdelta.js b/tests/model/delta/transform/insertdelta.js index b982331fc..d78e61b80 100644 --- a/tests/model/delta/transform/insertdelta.js +++ b/tests/model/delta/transform/insertdelta.js @@ -132,6 +132,30 @@ describe( 'transform', () => { expect( nodesAndText ).to.equal( 'XXXXXabcdXAABBPabcfoobarxyzP' ); } ); + it( 'merge in same position as insert - undo mode', () => { + // In undo mode, default transformation algorithm should be used. + const mergeDelta = getMergeDelta( insertPosition, 4, 12, baseVersion ); + + context.bWasUndone = true; + 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 664b1772f..a2f6e8d4f 100644 --- a/tests/model/delta/transform/mergedelta.js +++ b/tests/model/delta/transform/mergedelta.js @@ -88,6 +88,38 @@ describe( 'transform', () => { expect( nodesAndText ).to.equal( 'XXXXXabcdXAABBPabcfoobarxyzP' ); } ); + it( 'insert at same position as merge - undo mode', () => { + // In undo mode, default transformation algorithm should be used. + const insertDelta = getInsertDelta( mergePosition, [ nodeA, nodeB ], baseVersion ); + + context.bWasUndone = true; + 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: RemoveOperation, + sourcePosition: new Position( root, [ 3, 3, 5 ] ), + howMany: 1, + targetPosition: mergeDelta.operations[ 1 ].targetPosition, + 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 ); diff --git a/tests/model/delta/transform/splitdelta.js b/tests/model/delta/transform/splitdelta.js index 31da275ce..649c11ea0 100644 --- a/tests/model/delta/transform/splitdelta.js +++ b/tests/model/delta/transform/splitdelta.js @@ -56,7 +56,7 @@ describe( 'transform', () => { } ); describe( 'SplitDelta', () => { - it( 'split in same parent and offset', () => { + it( 'split in same parent and offset - isStrong = false', () => { const splitDeltaB = getSplitDelta( splitPosition, new Element( 'p' ), 9, baseVersion ); const transformed = transform( splitDelta, splitDeltaB, context ); @@ -86,8 +86,41 @@ describe( 'transform', () => { const nodesAndText = getNodesAndText( Range.createFromPositionAndShift( new Position( root, [ 3, 3, 0 ] ), 6 ) ); - // Incoming split delta is discarded. Only one new element is created after applying both split deltas. - // There are no empty P elements. + expect( nodesAndText ).to.equal( 'XXXXXabcdXPabcPPPPfoobarxyzP' ); + } ); + + it( 'split in same parent and offset - isStrong = true', () => { + const splitDeltaB = getSplitDelta( splitPosition, new Element( 'p' ), 9, baseVersion ); + + context.isStrong = true; + const transformed = transform( splitDelta, splitDeltaB, context ); + + baseVersion = splitDeltaB.operations.length; + + expect( transformed.length ).to.equal( 1 ); + + expectDelta( transformed[ 0 ], { + type: SplitDelta, + operations: [ + { + type: InsertOperation, + position: new Position( root, [ 3, 3, 4 ] ), + baseVersion + }, + { + type: NoOperation, + baseVersion: baseVersion + 1 + } + ] + } ); + + // Test if deltas do what they should after applying transformed delta. + + applyDelta( splitDeltaB, doc ); + applyDelta( transformed[ 0 ], doc ); + + const nodesAndText = getNodesAndText( Range.createFromPositionAndShift( new Position( root, [ 3, 3, 0 ] ), 6 ) ); + expect( nodesAndText ).to.equal( 'XXXXXabcdXPabcPPPPfoobarxyzP' ); } ); @@ -98,7 +131,8 @@ describe( 'transform', () => { const splitDeltaB = getSplitDelta( splitPosition, new Element( 'p' ), 9, baseVersion ); const transformed = transform( splitDelta, splitDeltaB, { isStrong: false, - insertBefore: true + insertBefore: true, + bWasUndone: true // If `insertBefore` was set it means that delta `b` had to be undone. } ); baseVersion = splitDeltaB.operations.length;