8
0
Просмотр исходного кода

Changed: Changed how MoveOperation is broken on multiple MoveOperations during transformation.

Szymon Cofalik 8 лет назад
Родитель
Сommit
eaa9069e3e

+ 71 - 53
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -429,7 +429,7 @@ const ot = {
 				rangeA.start = rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
 				rangeA.end = rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
 
-				return makeMoveOperationsFromRanges( a, [ rangeA ], newTargetPosition );
+				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
 
 			//
@@ -445,7 +445,7 @@ const ot = {
 				rangeA.start = rangeA.start._getCombined( b.sourcePosition, b.getMovedRangeStart() );
 				rangeA.end = rangeA.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
 
-				return makeMoveOperationsFromRanges( a, [ rangeA ], newTargetPosition );
+				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
 			//
 			// End of special case #2.
@@ -467,7 +467,7 @@ const ot = {
 				rangeA.start = rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
 				rangeA.end = rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
 
-				return makeMoveOperationsFromRanges( a, [ rangeA ], newTargetPosition );
+				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
 			//
 			// End of special case #3.
@@ -492,7 +492,7 @@ const ot = {
 
 			// Handle operation's source ranges - check how `rangeA` is affected by `b` operation.
 			// This will aggregate transformed ranges.
-			let ranges = [];
+			const ranges = [];
 
 			// Get the "difference part" of `a` operation source range.
 			// This is an array with one or two ranges. Two ranges if `rangeB` is inside `rangeA`.
@@ -500,10 +500,14 @@ const ot = {
 
 			for ( const range of difference ) {
 				// Transform those ranges by `b` operation. For example if `b` moved range from before those ranges, fix those ranges.
-				range.start = range.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
-				range.end = range.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
+				range.start = range.start._getTransformedByDeletion( b.sourcePosition, b.howMany );
+				range.end = range.end._getTransformedByDeletion( b.sourcePosition, b.howMany );
+
+				// If `b` operation targets into `rangeA` on the same level, spread `rangeA` into two ranges.
+				const shouldSpread = compareArrays( range.start.getParentPath(), b.getMovedRangeStart().getParentPath() ) == 'same';
+				const newRanges = range._getTransformedByInsertion( b.getMovedRangeStart(), b.howMany, shouldSpread, includeB );
 
-				ranges.push( range );
+				ranges.push( ...newRanges );
 			}
 
 			// Then, we have to manage the "common part" of both move ranges.
@@ -525,7 +529,7 @@ const ot = {
 				}
 				// If there is one difference range, we need to check whether common part was before it or after it.
 				else if ( ranges.length == 1 ) {
-					if ( rangeB.start.isBefore( rangeA.start ) ) {
+					if ( rangeB.start.isBefore( rangeA.start ) || rangeB.start.isEqual( rangeA.start ) ) {
 						ranges.unshift( common );
 					} else {
 						ranges.push( common );
@@ -544,30 +548,7 @@ const ot = {
 				return [ new NoOperation( a.baseVersion ) ];
 			}
 
-			// At this moment we have some ranges and a target position, to which those ranges should be moved.
-			// Order in `ranges` array is the go-to order of after transformation. We can reverse the `ranges`
-			// array so the last range will be moved first. This will ensure that each transformed `MoveOperation`
-			// can have same `targetPosition` and the order of ranges will be kept.
-			ranges = ranges.reverse();
-
-			// We are almost done. We have `ranges` and `targetPosition` to make operations from.
-			// Unfortunately, those operations may affect each other. Precisely, first operation after move
-			// may affect source range of second and third operation. Same with second operation affecting third.
-			// We need to fix those source ranges once again, before converting `ranges` to operations.
-			// Keep in mind that `targetPosition` is already set in stone thanks to the trick above.
-			for ( let i = 1; i < ranges.length; i++ ) {
-				for ( let j = 0; j < i; j++ ) {
-					const howMany = ranges[ j ].end.offset - ranges[ j ].start.offset;
-
-					// Thankfully, all ranges in `ranges` array are:
-					// * non-intersecting (these are part of original `a` operation source range), and
-					// * `newTargetPosition` does not target into them (opposite would mean that `a` operation targets "inside itself").
-					// This means that the transformation will be "clean" and always return one result.
-					ranges[ i ] = ranges[ i ]._getTransformedByMove( ranges[ j ].start, newTargetPosition, howMany )[ 0 ];
-				}
-			}
-
-			return makeMoveOperationsFromRanges( a, ranges, newTargetPosition );
+			return makeMoveOperationsFromRanges( ranges, newTargetPosition, a );
 		}
 	}
 };
@@ -650,29 +631,66 @@ function joinRanges( ranges ) {
 	}
 }
 
-// Map transformed range(s) to operations and return them.
-function makeMoveOperationsFromRanges( a, ranges, targetPosition ) {
-	return ranges.map( range => {
-		// We want to keep correct operation class.
-		let OperationClass;
-
-		if ( targetPosition.root.rootName == '$graveyard' ) {
-			OperationClass = RemoveOperation;
-		} else if ( range.start.root.rootName == '$graveyard' ) {
-			OperationClass = ReinsertOperation;
-		} else {
-			OperationClass = MoveOperation;
+// Helper function for `MoveOperation` x `MoveOperation` transformation.
+// Convert given ranges and target position to move operations and return them.
+// Ranges and target position will be transformed on-the-fly when generating operations.
+// Given `ranges` should be in the order of how they were in the original transformed operation.
+// Given `targetPosition` is the target position of the first range from `ranges`.
+function makeMoveOperationsFromRanges( ranges, targetPosition, a ) {
+	// At this moment we have some ranges and a target position, to which those ranges should be moved.
+	// Order in `ranges` array is the go-to order of after transformation.
+	//
+	// We are almost done. We have `ranges` and `targetPosition` to make operations from.
+	// Unfortunately, those operations may affect each other. Precisely, first operation after move
+	// may affect source range and target position of second and third operation. Same with second
+	// operation affecting third.
+	//
+	// We need to fix those source ranges and target positions once again, before converting `ranges` to operations.
+	const operations = [];
+
+	// Keep in mind that nothing will be transformed if there is just one range in `ranges`.
+	for ( let i = 0; i < ranges.length; i++ ) {
+		// Create new operation out of a range and target position.
+		const op = makeMoveOperation( ranges[ i ], targetPosition, a.isSticky );
+
+		operations.push( op );
+
+		// Transform other ranges by the generated operation.
+		for ( let j = i + 1; j < ranges.length; j++ ) {
+			// All ranges in `ranges` array should be:
+			// * non-intersecting (these are part of original operation source range), and
+			// * `targetPosition` does not target into them (opposite would mean that transformed operation targets "inside itself").
+			//
+			// This means that the transformation will be "clean" and always return one result.
+			ranges[ j ] = ranges[ j ]._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany )[ 0 ];
 		}
 
-		const result = new OperationClass(
-			range.start,
-			range.end.offset - range.start.offset,
-			targetPosition,
-			0 // Is corrected anyway later.
-		);
+		targetPosition = targetPosition._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany, true, false );
+	}
+
+	return operations;
+}
+
+function makeMoveOperation( range, targetPosition, isSticky ) {
+	// We want to keep correct operation class.
+	let OperationClass;
+
+	if ( targetPosition.root.rootName == '$graveyard' ) {
+		OperationClass = RemoveOperation;
+	} else if ( range.start.root.rootName == '$graveyard' ) {
+		OperationClass = ReinsertOperation;
+	} else {
+		OperationClass = MoveOperation;
+	}
+
+	const result = new OperationClass(
+		range.start,
+		range.end.offset - range.start.offset,
+		targetPosition,
+		0 // Is corrected anyway later.
+	);
 
-		result.isSticky = a.isSticky;
+	result.isSticky = isSticky;
 
-		return result;
-	} );
+	return result;
 }

+ 4 - 4
packages/ckeditor5-engine/tests/model/delta/transform/transform.js

@@ -115,7 +115,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 1, 0 ] ),
+						sourcePosition: new Position( root, [ 0, 2 ] ),
 						howMany: 1,
 						baseVersion: 3
 					}
@@ -127,7 +127,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 0, 2 ] ),
+						sourcePosition: new Position( root, [ 1, 0 ] ),
 						howMany: 1,
 						baseVersion: 4
 					}
@@ -163,7 +163,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 1, 0 ] ),
+						sourcePosition: new Position( root, [ 0, 2 ] ),
 						howMany: 1,
 						baseVersion: 3
 					}
@@ -175,7 +175,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 0, 2 ] ),
+						sourcePosition: new Position( root, [ 1, 0 ] ),
 						howMany: 1,
 						baseVersion: 4
 					}

+ 49 - 28
packages/ckeditor5-engine/tests/model/operation/transform.js

@@ -37,18 +37,18 @@ describe( 'transform', () => {
 		for ( const i in params ) {
 			if ( params.hasOwnProperty( i ) ) {
 				if ( i == 'type' ) {
-					expect( op ).to.be.instanceof( params[ i ] );
+					expect( op, 'type' ).to.be.instanceof( params[ i ] );
 				}
 				else if ( params[ i ] instanceof Array ) {
-					expect( op[ i ].length ).to.equal( params[ i ].length );
+					expect( op[ i ].length, i ).to.equal( params[ i ].length );
 
 					for ( let j = 0; j < params[ i ].length; j++ ) {
 						expect( op[ i ][ j ] ).to.equal( params[ i ][ j ] );
 					}
 				} else if ( params[ i ] instanceof Position || params[ i ] instanceof Range ) {
-					expect( op[ i ].isEqual( params[ i ] ) ).to.be.true;
+					expect( op[ i ].isEqual( params[ i ] ), i ).to.be.true;
 				} else {
-					expect( op[ i ] ).to.equal( params[ i ] );
+					expect( op[ i ], i ).to.equal( params[ i ] );
 				}
 			}
 		}
@@ -2241,7 +2241,7 @@ describe( 'transform', () => {
 				expectOperation( transOp[ 0 ], expected );
 			} );
 
-			it( 'target inside transforming move range: expand move range', () => {
+			it( 'target inside transforming move range: split into two operations', () => {
 				const transformBy = new MoveOperation(
 					new Position( root, [ 4, 1, 0 ] ),
 					2,
@@ -2251,11 +2251,15 @@ describe( 'transform', () => {
 
 				const transOp = transform( op, transformBy );
 
-				expect( transOp.length ).to.equal( 1 );
-
-				expected.howMany = 4;
+				expect( transOp.length ).to.equal( 2 );
 
+				expected.howMany = 1;
 				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 6 ];
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
+				expected.baseVersion++;
+				expectOperation( transOp[ 1 ], expected );
 			} );
 
 			it( 'target at start boundary of transforming move range: increment source offset', () => {
@@ -2530,12 +2534,13 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
-				expected.sourcePosition.path = [ 2, 2, 3 ];
+				expected.sourcePosition = new Position( otherRoot, [ 4, 1, 1 ] );
 				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
 
-				expected.sourcePosition = new Position( otherRoot, [ 4, 1, 1 ] );
+				expected.sourcePosition = new Position( root, [ 2, 2, 3 ] );
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
@@ -2569,18 +2574,19 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
-				expected.sourcePosition.path = [ 4, 1, 0 ];
+				expected.sourcePosition = sourcePosition;
 				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
 
-				expected.sourcePosition.path = op.sourcePosition.path;
+				expected.sourcePosition = new Position( root, [ 4, 1, 0 ] );
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
 			} );
 
-			it( 'range intersects on left side, target inside transforming range and is important: expand move range', () => {
+			it( 'range intersects on left side, target inside transforming range and is important: split into two operations', () => {
 				op.howMany = 4;
 
 				const transformBy = new MoveOperation(
@@ -2592,15 +2598,22 @@ describe( 'transform', () => {
 
 				const transOp = transform( op, transformBy );
 
-				expect( transOp.length ).to.equal( 1 );
+				expect( transOp.length ).to.equal( 2 );
 
 				expected.sourcePosition.path = [ 2, 2, 3 ];
-				expected.howMany = 5;
+				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 5 ];
+				expected.howMany = 2;
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 1 ], expected );
 			} );
 
-			it( 'range intersects on left side, target inside transforming range and is less important: expand move range', () => {
+			it( 'range intersects on left side, target inside transforming range and is less important: split into two operations', () => {
 				op.howMany = 4;
 
 				const transformBy = new MoveOperation(
@@ -2612,12 +2625,19 @@ describe( 'transform', () => {
 
 				const transOp = transform( op, transformBy, { isStrong: true } );
 
-				expect( transOp.length ).to.equal( 1 );
+				expect( transOp.length ).to.equal( 2 );
 
 				expected.sourcePosition.path = [ 2, 2, 3 ];
-				expected.howMany = 5;
+				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 5 ];
+				expected.howMany = 2;
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 1 ], expected );
 			} );
 
 			it( 'range intersects on right side of transforming range and is important: shrink range', () => {
@@ -2648,12 +2668,13 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
-				expected.sourcePosition.path = [ 4, 1, 0 ];
+				expected.sourcePosition = sourcePosition;
 				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
 
-				expected.sourcePosition.path = op.sourcePosition.path;
+				expected.sourcePosition = new Position( root, [ 4, 1, 0 ] );
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
@@ -2673,15 +2694,14 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
-				expected.sourcePosition.path = [ 4, 1, 0 ];
+				expected.sourcePosition.path = [ 2, 2, 4 ];
 				expected.targetPosition.path = [ 4, 1, 2 ];
 				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
 
-				expected.sourcePosition.path = [ 2, 2, 4 ];
-				expected.targetPosition.path = [ 4, 1, 2 ];
-				expected.howMany = 1;
+				expected.sourcePosition.path = [ 4, 1, 0 ];
+				expected.targetPosition.path = [ 4, 1, 3 ];
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
@@ -2722,13 +2742,12 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
+				expected.sourcePosition.path = [ 2, 2, 4 ];
 				expected.howMany = 1;
-				expected.sourcePosition.path = [ 2, 2, 5 ];
 
 				expectOperation( transOp[ 0 ], expected );
 
-				expected.howMany = 1;
-				expected.sourcePosition.path = [ 2, 2, 4 ];
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
@@ -2748,18 +2767,20 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 3 );
 
-				expected.sourcePosition.path = [ 2, 2, 5 ];
+				expected.sourcePosition.path = [ 2, 2, 4 ];
 				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
 
 				expected.sourcePosition.path = [ 4, 1, 0 ];
+				expected.targetPosition = targetPosition.getShiftedBy( 1 );
 				expected.howMany = 2;
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
 
 				expected.sourcePosition.path = [ 2, 2, 4 ];
+				expected.targetPosition = targetPosition.getShiftedBy( 3 );
 				expected.howMany = 1;
 				expected.baseVersion++;