Browse Source

Range.containsRange introduced, operation/transform.js fixed order of returned operations, special case for one of MoveOperation by MoveOperation transformation scenarios.

Szymon Cofalik 10 years ago
parent
commit
0652307cd8

+ 53 - 43
packages/ckeditor5-engine/src/document/operation/transform.js

@@ -86,7 +86,7 @@ CKEDITOR.define( [
 				const ranges = a.range.getTransformedByInsertion( b.position, b.nodeList.length );
 
 				// Map transformed range(s) to operations and return them.
-				return ranges.map( ( range ) => {
+				return ranges.reverse().map( ( range ) => {
 					return new ChangeOperation(
 						range,
 						a.oldAttr,
@@ -167,7 +167,7 @@ CKEDITOR.define( [
 					// previously transformed target position.
 					// Note that we do not use Position.getTransformedByMove on range boundaries because we need to
 					// transform by insertion a range as a whole, since newTargetPosition might be inside that range.
-					ranges = difference.getTransformedByInsertion( newTargetPosition, b.howMany, false );
+					ranges = difference.getTransformedByInsertion( newTargetPosition, b.howMany, false ).reverse();
 				}
 
 				if ( common !== null ) {
@@ -204,7 +204,7 @@ CKEDITOR.define( [
 				const ranges = rangeB.getTransformedByInsertion( b.position, b.nodeList.length, true );
 
 				// Map transformed range(s) to operations and return them.
-				return ranges.map( ( range ) => {
+				return ranges.reverse().map( ( range ) => {
 					return new MoveOperation(
 						range.start,
 						newTargetPosition.clone(),
@@ -219,7 +219,7 @@ CKEDITOR.define( [
 			// Transforms MoveOperation `a` by MoveOperation `b`. Accepts a flag stating whether `a` is more important
 			// than `b` when it comes to resolving conflicts. Returns results as an array of operations.
 			MoveOperation( a, b, isStrong ) {
-				// There is a special case when both move operations' target positions are inside nodes that are
+				// Special case when both move operations' target positions are inside nodes that are
 				// being moved by the other move operation. So in other words, we move ranges into inside of each other.
 				// This case can't be solved reasonably (on the other hand, it should not happen often).
 				if ( moveTargetIntoMovedRange( a, b ) && moveTargetIntoMovedRange( b, a ) ) {
@@ -228,37 +228,55 @@ CKEDITOR.define( [
 					return [ b.getReversed() ];
 				}
 
-				// Get target position from the state "after" nodes specified by other MoveOperation are "detached".
-				const moveTargetPosition = b.targetPosition.getTransformedByDeletion( b.sourcePosition, b.howMany );
-
-				// This will aggregate transformed ranges.
-				let ranges = [];
-
 				// Create ranges from MoveOperations properties.
 				const rangeA = Range.createFromPositionAndOffset( a.sourcePosition, a.howMany );
 				const rangeB = Range.createFromPositionAndOffset( b.sourcePosition, b.howMany );
 
-				// MoveOperations ranges may intersect.
+				// Special case when transformed range contains both the other operation's whole range and target.
+				// In such case, operations are not really conflicting and we should leave transformed operation as it is.
+				// Without this we would have 3 or 4 operations and the transformation result would probably be not intuitive.
+				if ( rangeA.containsRange( rangeB ) && rangeA.containsPosition( b.targetPosition ) ) {
+					return [ a.clone() ];
+				}
+				// Mirror situation for the case above - now transformed range is wholly contained in the other
+				// operation's range and also targets to that range. Without this special treatment we would
+				// transform this operation into NoOperation, but this would not be compatible with the result
+				// generated by the special case above.
+				else if ( rangeB.containsRange( rangeA ) && rangeB.containsPosition( a.targetPosition ) ) {
+					return [
+						new MoveOperation(
+							a.sourcePosition._getCombined( b.sourcePosition, b.targetPosition ),
+							a.targetPosition._getCombined( b.sourcePosition, b.targetPosition ),
+							a.howMany,
+							a.baseVersion
+						)
+					];
+				}
+
+				// All the other non-special cases are treated by generic algorithm below.
 
-				// First, we take care of that part of the range that is only modified by transformed operation.
-				// If two ranges were returned it means that rangeB was inside rangeA. We will cover rangeB later.
-				// Right now we will make a simplification and join difference ranges and transform them as one.
-				const difference = joinRanges( rangeA.getDifference( rangeB ) );
+				const differenceSet = rangeA.getDifference( rangeB );
+				const common = rangeA.getIntersection( rangeB );
 
-				if ( difference !== null ) {
+				// This will aggregate transformed ranges.
+				let ranges = [];
+
+				// Get target position from the state "after" nodes specified by other MoveOperation are "detached".
+				const moveTargetPosition = b.targetPosition.getTransformedByDeletion( b.sourcePosition, b.howMany );
+
+				// First, we take care of that part of the range that is only modified by transformed operation.
+				for ( let i = 0; i < differenceSet.length; i++ ) {
 					// MoveOperation removes nodes from their original position. We acknowledge this by proper transformation.
 					// Take the start and the end of the range and transform them by deletion of moved nodes.
-					// Note that if rangeB was inside rangeA, only difference.end will be transformed.
-					// This nicely covers the joining simplification we did in the previous step.
-					difference.start = difference.start.getTransformedByDeletion( b.sourcePosition, b.howMany );
-					difference.end = difference.end.getTransformedByDeletion( b.sourcePosition, b.howMany );
+					differenceSet[ i ].start = differenceSet[ i ].start.getTransformedByDeletion( b.sourcePosition, b.howMany );
+					differenceSet[ i ].end = differenceSet[ i ].end.getTransformedByDeletion( b.sourcePosition, b.howMany );
 
 					// MoveOperation pastes nodes into target position. We acknowledge this by proper transformation.
 					// Note that since we operate on transformed difference range, we should transform by
 					// previously transformed target position.
 					// Note that we do not use Position.getTransformedByMove on range boundaries because we need to
 					// transform by insertion a range as a whole, since newTargetPosition might be inside that range.
-					ranges = difference.getTransformedByInsertion( moveTargetPosition, b.howMany, true );
+					ranges = ranges.concat( differenceSet[ i ].getTransformedByInsertion( moveTargetPosition, b.howMany, true ) );
 				}
 
 				// Then, we have to manage the common part of both move ranges.
@@ -269,17 +287,21 @@ CKEDITOR.define( [
 				// So, we will handle common range if it is "deeper" or if transformed operation is more important.
 				let isDeeper = utils.compareArrays( b.sourcePosition.getParentPath(), a.sourcePosition.getParentPath() ) == utils.compareArrays.PREFIX;
 
-				if ( isDeeper || isStrong ) {
-					const common = rangeA.getIntersection( rangeB );
-
-					if ( common !== null ) {
-						// 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.
-						common.start = common.start._getCombined( b.sourcePosition, moveTargetPosition );
-						common.end = common.end._getCombined( b.sourcePosition, moveTargetPosition );
+				if ( common !== null && ( isDeeper || isStrong ) ) {
+					// 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.
+					common.start = common.start._getCombined( b.sourcePosition, moveTargetPosition );
+					common.end = common.end._getCombined( b.sourcePosition, moveTargetPosition );
 
+					// We have to take care of proper range order.
+					// Note that both push, splice and unshift do the same if there are no ranges in the array.
+					if ( rangeB.end.isAfter( rangeA.end ) ) {
 						ranges.push( common );
+					} else if ( rangeB.start.isBefore( rangeA.start ) ) {
+						ranges.unshift( common );
+					} else {
+						ranges.splice( 1, 0, common );
 					}
 				}
 
@@ -292,20 +314,8 @@ CKEDITOR.define( [
 				// Target position also could be affected by the other MoveOperation. We will transform it.
 				let newTargetPosition = a.targetPosition.getTransformedByMove( b.sourcePosition, moveTargetPosition, b.howMany, !isStrong );
 
-				// At this point we might have multiple ranges. All ranges will be moved to the same, newTargetPosition.
-				// To keep them in the right order, we need to move them starting from the last one.
-				// [|ABC||DEF||GHI|] ==> [^], [|ABC||DEF|] ==> [^GHI], [|ABC|] ==> [^DEFGHI], [] ==> [ABCDEFGHI]
-				// To achieve this, we sort ranges by their starting position in descending order.
-				ranges.sort( ( r1, r2 ) => {
-					if ( r1.start.root != r2.start.root ) {
-						return r1.start.root == a.sourcePosition.root ? 1 : -1;
-					}
-
-					return r1.start.isBefore( r2.start ) ? 1 : -1;
-				} );
-
 				// Map transformed range(s) to operations and return them.
-				return ranges.map( ( range ) => {
+				return ranges.reverse().map( ( range ) => {
 					return new MoveOperation(
 						range.start,
 						newTargetPosition,

+ 16 - 6
packages/ckeditor5-engine/src/document/range.js

@@ -55,15 +55,25 @@ CKEDITOR.define( [ 'document/position', 'document/positioniterator', 'utils' ],
 		}
 
 		/**
-		 * Checks whether this {document.Range range} contains given {@link document.Position position}.
+		 * Checks whether this contains given {@link document.Position position}.
 		 *
-		 * @param {@link document.Position} position Position to check.
+		 * @param {document.Position} position Position to check.
 		 * @returns {Boolean} True if given {@link document.Position position} is contained.
 		 */
 		containsPosition( position ) {
 			return position.isAfter( this.start ) && position.isBefore( this.end );
 		}
 
+		/**
+		 * Checks whether this range contains given {@link document.Range range}.
+		 *
+		 * @param {document.Range} otherRange Range to check.
+		 * @returns {Boolean} True if given {@link document.Range range} boundaries are contained by this range.
+		 */
+		containsRange( otherRange ) {
+			return this.containsPosition( otherRange.start ) && this.containsPosition( otherRange.end );
+		}
+
 		/**
 		 * Gets a part of this {@link document.Range range} which is not a part of given {@link document.Range range}. Returned
 		 * array contains zero, one or two {@link document.Range ranges}.
@@ -197,13 +207,13 @@ CKEDITOR.define( [ 'document/position', 'document/positioniterator', 'utils' ],
 				// insertion to reflect insertion changes.
 
 				return [
-					new Range(
-						insertPosition.getTransformedByInsertion( insertPosition, howMany, true ),
-						this.end.getTransformedByInsertion( insertPosition, howMany, true )
-					),
 					new Range(
 						this.start.clone(),
 						insertPosition.clone()
+					),
+					new Range(
+						insertPosition.getTransformedByInsertion( insertPosition, howMany, true ),
+						this.end.getTransformedByInsertion( insertPosition, howMany, true )
 					)
 				];
 			} else {

+ 204 - 38
packages/ckeditor5-engine/tests/document/operation/transform.js

@@ -1949,7 +1949,47 @@ describe( 'transform', () => {
 				expectOperation( transOp[ 0 ], expected );
 			} );
 
-			it( 'range intersects on left with transforming range and is important: shrink range', () => {
+			it( 'range contains transforming range and target and is important: update range path and target', () => {
+				op.targetPosition.path = [ 2, 2, 7 ];
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 3 ], root ),
+					new Position( [ 4, 1, 0 ], root ),
+					5,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expected.sourcePosition.path = [ 4, 1, 1 ];
+				expected.targetPosition.path = [ 4, 1, 4 ];
+
+				expectOperation( transOp[ 0 ], expected );
+			} );
+
+			it( 'range contains transforming range and target and is less important: update range path and target', () => {
+				op.targetPosition.path = [ 2, 2, 7 ];
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 3 ], root ),
+					new Position( [ 4, 1, 0 ], root ),
+					5,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy, true );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expected.sourcePosition.path = [ 4, 1, 1 ];
+				expected.targetPosition.path = [ 4, 1, 4 ];
+
+				expectOperation( transOp[ 0 ], expected );
+			} );
+
+			it( 'range intersects on left side of transforming range and is important: shrink range', () => {
 				let transformBy = new MoveOperation(
 					new Position( [ 2, 2, 3 ], root ),
 					new Position( [ 4, 1, 0 ], root ),
@@ -1966,7 +2006,7 @@ describe( 'transform', () => {
 				expectOperation( transOp[ 0 ], expected );
 			} );
 
-			it( 'range intersects on left with transforming range and is less important: split into two operations', () => {
+			it( 'range intersects on left side of transforming range and is less important: split into two operations', () => {
 				// Get more test cases and better code coverage
 				let otherRoot = new RootElement( null );
 
@@ -1981,22 +2021,18 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
-				expectOperation( transOp[ 0 ], {
-					type: MoveOperation,
-					sourcePosition: new Position( [ 4, 1, 1 ], otherRoot ),
-					targetPosition: expected.targetPosition,
-					howMany: 1,
-					baseVersion: expected.baseVersion
-				} );
-
 				expected.sourcePosition.path = [ 2, 2, 3 ];
 				expected.howMany = 1;
+
+				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition = new Position( [ 4, 1, 1 ], otherRoot );
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
 			} );
 
-			it( 'range intersects on right with transforming range and is important: shrink range', () => {
+			it( 'range intersects on right side of transforming range and is important: shrink range', () => {
 				let transformBy = new MoveOperation(
 					new Position( [ 2, 2, 5 ], root ),
 					new Position( [ 4, 1, 0 ], root ),
@@ -2012,7 +2048,7 @@ describe( 'transform', () => {
 				expectOperation( transOp[ 0 ], expected );
 			} );
 
-			it( 'range intersects on right with transforming range and is less important: split into two operations', () => {
+			it( 'range intersects on right side of transforming range and is less important: split into two operations', () => {
 				let transformBy = new MoveOperation(
 					new Position( [ 2, 2, 5 ], root ),
 					new Position( [ 4, 1, 0 ], root ),
@@ -2024,72 +2060,164 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 2 );
 
-				expectOperation( transOp[ 0 ], {
-					type: MoveOperation,
-					sourcePosition: new Position( [ 4, 1, 0 ], root ),
-					targetPosition: expected.targetPosition,
-					howMany: 1,
-					baseVersion: expected.baseVersion
-				} );
+				expected.sourcePosition.path = [ 4, 1, 0 ];
+				expected.howMany = 1;
+
+				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = op.sourcePosition.path;
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 1 ], expected );
+			} );
+
+			it( 'range intersects on left side, target inside transforming range and is important: split into two operations', () => {
+				op.howMany = 4;
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 3 ], root ),
+					new Position( [ 2, 2, 6 ], root ),
+					2,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 2 );
+
+				expected.sourcePosition.path = [ 2, 2, 6 ];
+				expected.howMany = 2;
+
+				expectOperation( transOp[ 0 ], expected );
 
+				expected.sourcePosition.path = [ 2, 2, 3 ];
 				expected.howMany = 1;
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
 			} );
 
-			it( 'range inside transforming range and is important: shrink range', () => {
+			it( 'range intersects on left side, target inside transforming range and is less important: split into three operations', () => {
+				op.howMany = 4;
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 3 ], root ),
+					new Position( [ 2, 2, 6 ], root ),
+					2,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy, true );
+
+				expect( transOp.length ).to.equal( 3 );
+
+				expected.sourcePosition.path = [ 2, 2, 6 ];
+				expected.howMany = 2;
+
+				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 3 ];
+				expected.howMany = 1;
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 1 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 5 ];
+				expected.howMany = 1;
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 2 ], expected );
+			} );
+
+			it( 'range intersects on right side, target inside transforming range and is important: split into two operations', () => {
 				op.howMany = 4;
 
 				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 7 ], root ),
 					new Position( [ 2, 2, 5 ], root ),
-					new Position( [ 4, 1, 0 ], root ),
 					2,
 					baseVersion
 				);
 
 				let transOp = transform( op, transformBy );
 
+				expect( transOp.length ).to.equal( 2 );
+
+				expected.sourcePosition.path = [ 2, 2, 7 ];
 				expected.howMany = 2;
 
-				expect( transOp.length ).to.equal( 1 );
 				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 4 ];
+				expected.howMany = 1;
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 1 ], expected );
 			} );
 
-			it( 'range inside transforming range and is less important: split into two operations', () => {
+			it( 'range intersects on right side, target inside transforming range and is less important: split into three operations', () => {
 				op.howMany = 4;
 
 				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 7 ], root ),
 					new Position( [ 2, 2, 5 ], root ),
-					new Position( [ 4, 1, 0 ], root ),
 					2,
 					baseVersion
 				);
 
 				let transOp = transform( op, transformBy, true );
 
-				expect( transOp.length ).to.equal( 2 );
+				expect( transOp.length ).to.equal( 3 );
 
-				expectOperation( transOp[ 0 ], {
-					type: MoveOperation,
-					sourcePosition: new Position( [ 4, 1, 0 ], root ),
-					targetPosition: expected.targetPosition,
-					howMany: 2,
-					baseVersion: expected.baseVersion
-				} );
+				expected.sourcePosition.path = [ 2, 2, 5 ];
+				expected.howMany = 1;
 
+				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 7 ];
 				expected.howMany = 2;
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 4 ];
+				expected.howMany = 1;
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 2 ], expected );
 			} );
 
-			it( 'range and target inside transforming range and is less important: split into three operations', () => {
-				op.howMany = 6;
+			it( 'range inside transforming range and is important: split into two operations', () => {
+				op.howMany = 4;
 
 				let transformBy = new MoveOperation(
 					new Position( [ 2, 2, 5 ], root ),
-					new Position( [ 2, 2, 9 ], root ),
+					new Position( [ 4, 1, 0 ], root ),
+					2,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 2 );
+
+				expected.sourcePosition.path = [ 2, 2, 5 ];
+				expected.howMany = 1;
+
+				expectOperation( transOp[ 0 ], expected );
+
+				expected.sourcePosition.path = [ 2, 2, 4 ];
+				expected.baseVersion++;
+
+				expectOperation( transOp[ 1 ], expected );
+			} );
+
+			it( 'range inside transforming range and is less important: split into three operations', () => {
+				op.howMany = 4;
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 5 ], root ),
+					new Position( [ 4, 1, 0 ], root ),
 					2,
 					baseVersion
 				);
@@ -2098,23 +2226,61 @@ describe( 'transform', () => {
 
 				expect( transOp.length ).to.equal( 3 );
 
-				expected.sourcePosition.path = [ 2, 2, 9 ];
+				expected.sourcePosition.path = [ 2, 2, 5 ];
 				expected.howMany = 1;
 
 				expectOperation( transOp[ 0 ], expected );
 
-				expected.sourcePosition.path = [ 2, 2, 7 ];
+				expected.sourcePosition.path = [ 4, 1, 0 ];
 				expected.howMany = 2;
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 1 ], expected );
 
 				expected.sourcePosition.path = [ 2, 2, 4 ];
-				expected.howMany = 3;
+				expected.howMany = 1;
 				expected.baseVersion++;
 
 				expectOperation( transOp[ 2 ], expected );
 			} );
+
+			it( 'range and target inside transforming range and is important: no operation update', () => {
+				op.howMany = 6;
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 5 ], root ),
+					new Position( [ 2, 2, 9 ], root ),
+					2,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expected.howMany = 6;
+
+				expectOperation( transOp[ 0 ], expected );
+			} );
+
+			it( 'range and target inside transforming range and is less important: no operation update', () => {
+				op.howMany = 6;
+
+				let transformBy = new MoveOperation(
+					new Position( [ 2, 2, 5 ], root ),
+					new Position( [ 2, 2, 9 ], root ),
+					2,
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy, true );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expected.howMany = 6;
+
+				expectOperation( transOp[ 0 ], expected );
+			} );
 		} );
 
 		describe( 'by NoOperation', () => {

+ 4 - 4
packages/ckeditor5-engine/tests/document/range.js

@@ -194,11 +194,11 @@ describe( 'Range', () => {
 
 			expect( transformed.length ).to.equal( 2 );
 
-			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 4, 1, 10 ] );
-			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 5, 4 ] );
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 4, 1, 6 ] );
 
-			expect( transformed[ 1 ].start.path ).to.deep.equal( [ 3, 2 ] );
-			expect( transformed[ 1 ].end.path ).to.deep.equal( [ 4, 1, 6 ] );
+			expect( transformed[ 1 ].start.path ).to.deep.equal( [ 4, 1, 10 ] );
+			expect( transformed[ 1 ].end.path ).to.deep.equal( [ 5, 4 ] );
 		} );
 
 		it( 'should not updated positions if insertion is after range', () => {