Explorar o código

Position.getTransformedByMove introduced, Position.getCombined become protected method.

Szymon Cofalik %!s(int64=10) %!d(string=hai) anos
pai
achega
b302bab944

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

@@ -126,7 +126,7 @@ CKEDITOR.define( [
 				// Transformed operations are always new instances, not references to the original operations.
 				const transformed = a.clone();
 
-				// Transform operation position by the other operation position.
+				// Transform insert position by the other operation position.
 				transformed.position = transformed.position.getTransformedByInsertion( b.position, b.nodeList.length, !isStrong );
 
 				return [ transformed ];
@@ -139,20 +139,8 @@ CKEDITOR.define( [
 			MoveOperation( a, b, isStrong ) {
 				const transformed = a.clone();
 
-				// MoveOperation removes nodes from their original position. We acknowledge this by proper transformation.
-				const newPosition = a.position.getTransformedByDeletion( b.sourcePosition, b.howMany );
-
-				if ( newPosition === null ) {
-					// This operation's position was inside a node moved by MoveOperation. We substitute that position by
-					// the combination of move target position and insert position. This reflects changes done by MoveOperation.
-
-					transformed.position = transformed.position.getCombined( b.sourcePosition, b.targetPosition );
-				} else {
-					// Here we have the insert position after some nodes has been removed by MoveOperation.
-					// Next step is to reflect pasting nodes by MoveOperation, which might further affect the position.
-
-					transformed.position = newPosition.getTransformedByInsertion( b.targetPosition, b.howMany, !isStrong );
-				}
+				// Transform insert position by the other operation parameters.
+				transformed.position = a.position.getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !isStrong );
 
 				return [ transformed ];
 			}
@@ -243,14 +231,17 @@ CKEDITOR.define( [
 					// 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( newTargetPosition, b.howMany, false );
 				}
 
 				if ( common !== null ) {
-					// We substitute original position by the combination of target position and original position.
-					// This reflects that those nodes were moved to another place by MoveOperation.
-					common.start = common.start.getCombined( b.sourcePosition, newTargetPosition );
-					common.end = common.end.getCombined( b.sourcePosition, newTargetPosition );
+					// 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, newTargetPosition );
+					common.end = common.end._getCombined( b.sourcePosition, newTargetPosition );
 
 					ranges.push( common );
 				}
@@ -330,6 +321,8 @@ CKEDITOR.define( [
 					// 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 );
 				}
 
@@ -345,39 +338,25 @@ CKEDITOR.define( [
 					const common = rangeA.getIntersection( rangeB );
 
 					if ( common !== null ) {
-						// We substitute original position by the combination of target position and original position.
-						// This reflects that those nodes were moved to another place by MoveOperation.
-						common.start = common.start.getCombined( b.sourcePosition, moveTargetPosition );
-						common.end = common.end.getCombined( b.sourcePosition, moveTargetPosition );
+						// 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 );
 
 						ranges.push( common );
 					}
 				}
 
-				// At this point we transformed this operation's source range. Now, we need to transform target position.
-
-				// First, transform target position by deletion of the other operation's range.
-				let newTargetPosition = a.targetPosition.getTransformedByDeletion( b.sourcePosition, b.howMany );
-
-				if ( newTargetPosition === null ) {
-					// Transformed operation target position was inside a node moved by the other MoveOperation.
-					// We substitute that position by the combination of the other move target position and
-					// transformed operation target position. This reflects changes done by the other MoveOperation.
-
-					newTargetPosition = a.targetPosition.getCombined( b.sourcePosition, moveTargetPosition );
-				} else {
-					// Here we have transformed operation target position after some nodes has been removed by MoveOperation.
-					// Next step is to reflect pasting nodes by MoveOperation, which might further affect the position.
-
-					newTargetPosition = newTargetPosition.getTransformedByInsertion( moveTargetPosition, b.howMany, true );
-				}
-
-				// If we haven't got any ranges this far it means that both operations tried to move the same nodes and
-				// transformed operation is less important. We return NoOperation in this case.
+				// At this point we transformed this operation's source ranges it means that nothing should be changed.
+				// But since we need to return an instance of Operation we return an array with NoOperation.
 				if ( ranges.length === 0 ) {
 					return [ new NoOperation( a.baseVersion ) ];
 				}
 
+				// 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]

+ 41 - 12
packages/ckeditor5-engine/src/document/position.js

@@ -283,19 +283,47 @@ CKEDITOR.define( [ 'document/rootelement', 'utils', 'ckeditorerror' ], ( RootEle
 			}
 		}
 
+		/**
+		 * Returns this position after being updated by moving `howMany` attributes from `sourcePosition` to `targetPosition`.
+		 *
+		 * @param {document.Position} sourcePosition Position before the first element to move.
+		 * @param {document.Position} targetPosition Position where moved elements will be inserted.
+		 * @param {Number} howMany How many consecutive nodes to move, starting from `sourcePosition`.
+		 * @param {Boolean} insertBefore Flag indicating whether moved nodes are pasted before or after `insertPosition`.
+		 * This is important only when `targetPosition` and this position are same. If that is the case and the flag is
+		 * set to true, this position will get transformed by range insertion. If the flag is set to false, it won't.
+		 * @returns {document.Position} Transformed position.
+		 */
+		getTransformedByMove( sourcePosition, targetPosition, howMany, insertBefore ) {
+			// Moving a range removes nodes from their original position. We acknowledge this by proper transformation.
+			let transformed = this.getTransformedByDeletion( sourcePosition, howMany );
+
+			if ( transformed !== null ) {
+				// This position is not inside a removed node.
+				// Next step is to reflect pasting nodes, which might further affect the position.
+				transformed = transformed.getTransformedByInsertion( targetPosition, howMany, insertBefore );
+			} else {
+				// This position is inside a removed node. In this case, we are unable to simply transform it by range insertion.
+				// Instead, we calculate a combination of this position, move source position and target position.
+				transformed = this._getCombined( sourcePosition, targetPosition );
+			}
+
+			return transformed;
+		}
+
 		/**
 		 * Returns a new position that is a combination of this position and given positions. The combined
 		 * position is this position transformed by moving a range starting at `from` to `to` position.
-		 * It is expected that `original` position is inside the moved range.
+		 * It is expected that this position is inside the moved range.
 		 *
-		 * In other words, this method in a smart way "cuts out" `from` path from this position and
-		 * injects `to` path in it's place, while doing necessary fixes in order to get a correct path.
+		 * In other words, this method in a smart way "cuts out" `source` path from this position and
+		 * injects `target` path in it's place, while doing necessary fixes in order to get a correct path.
 		 *
 		 * Example:
 		 * 	let original = new Position( [ 2, 3, 1 ], root );
-		 * 	let from = new Position( [ 2, 2 ], root );
-		 * 	let to = new Position( [ 1, 1, 3 ], otherRoot );
-		 * 	let combined = original.getCombined( from, to );
+		 * 	let source = new Position( [ 2, 2 ], root );
+		 * 	let target = new Position( [ 1, 1, 3 ], otherRoot );
+		 * 	let combined = original.getCombined( source, target );
 		 * 	// combined.path is [ 1, 1, 4, 1 ], combined.root is otherRoot
 		 *
 		 * Explanation:
@@ -306,20 +334,21 @@ CKEDITOR.define( [ 'document/rootelement', 'utils', 'ckeditorerror' ], ( RootEle
 		 * took care of `[ 2, 3 ]` part of it. Now we have to add the rest of the original path to the transformed path.
 		 * Finally, the transformed position will point to `[ 1, 1, 4, 1 ]`.
 		 *
-		 * @param {document.Position} from Beginning of the moved range.
-		 * @param {document.Position} to Position where the range is moved.
+		 * @protected
+		 * @param {document.Position} source Beginning of the moved range.
+		 * @param {document.Position} target Position where the range is moved.
 		 * @returns {document.Position} Combined position.
 		 */
-		getCombined( from, to ) {
-			const i = from.path.length - 1;
+		_getCombined( source, target ) {
+			const i = source.path.length - 1;
 
 			// The first part of a path to combined position is a path to the place where nodes were moved.
-			let combined = to.clone();
+			let combined = target.clone();
 
 			// Then we have to update the rest of the path.
 
 			// Fix the offset because this position might be after `from` position and we have to reflect that.
-			combined.offset = combined.offset + this.path[ i ] - from.offset;
+			combined.offset = combined.offset + this.path[ i ] - source.offset;
 
 			// Then, add the rest of the path.
 			// If this position is at the same level as `from` position nothing will get added.

+ 41 - 16
packages/ckeditor5-engine/tests/document/position.js

@@ -237,16 +237,6 @@ describe( 'position', () => {
 		expect( clone.path ).not.to.be.equal( position.path ); // make sure the paths are not the same array
 	} );
 
-	it( 'should return correct combination of this and given positions', () => {
-		const position = new Position( [ 1, 3, 4, 2 ], root );
-		const prefixPosition = new Position( [ 1, 1 ], root );
-		const targetPosition = new Position( [ 2, 5 ], root );
-
-		const combined = position.getCombined( prefixPosition, targetPosition );
-
-		expect( combined.path ).to.deep.equal( [ 2, 7, 4, 2 ] );
-	} );
-
 	describe( 'isBefore', () => {
 		it( 'should return true if given position has same root and is before this position', () => {
 			let position = new Position( [ 1, 1, 2 ], root );
@@ -423,44 +413,79 @@ describe( 'position', () => {
 
 		it( 'should decrement offset if deletion is in the same parent and closer offset', () => {
 			const position = new Position( [ 1, 2, 7 ], root );
-			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 2 ], root ), 2, false );
+			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 2 ], root ), 2 );
 
 			expect( transformed.offset ).to.equal( 5 );
 		} );
 
 		it( 'should set position offset to deletion offset if position is between removed nodes', () => {
 			const position = new Position( [ 1, 2, 4 ], root );
-			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 3 ], root ), 5, false );
+			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 3 ], root ), 5 );
 
 			expect( transformed.offset ).to.equal( 3 );
 		} );
 
 		it( 'should not decrement offset if deletion position is in different root', () => {
 			const position = new Position( [ 1, 2, 3 ], root );
-			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 1 ], otherRoot ), 2, false );
+			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 1 ], otherRoot ), 2 );
 
 			expect( transformed.offset ).to.equal( 3 );
 		} );
 
 		it( 'should not decrement offset if deletion is in the same parent and further offset', () => {
 			const position = new Position( [ 1, 2, 3 ], root );
-			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 4 ], root ), 2, false );
+			const transformed = position.getTransformedByDeletion( new Position( [ 1, 2, 4 ], root ), 2 );
 
 			expect( transformed.offset ).to.equal( 3 );
 		} );
 
 		it( 'should update path if deletion position parent is a node from that path and offset is before next node on that path', () => {
 			const position = new Position( [ 1, 2, 3 ], root );
-			const transformed = position.getTransformedByDeletion( new Position( [ 1, 0 ], root ), 2, false );
+			const transformed = position.getTransformedByDeletion( new Position( [ 1, 0 ], root ), 2 );
 
 			expect( transformed.path ).to.deep.equal( [ 1, 0, 3 ] );
 		} );
 
 		it( 'should not update path if deletion position parent is a node from that path and offset is after next node on that path', () => {
 			const position = new Position( [ 1, 2, 3 ], root );
-			const transformed = position.getTransformedByDeletion( new Position( [ 1, 3 ], root ), 2, false );
+			const transformed = position.getTransformedByDeletion( new Position( [ 1, 3 ], root ), 2 );
 
 			expect( transformed.path ).to.deep.equal( [ 1, 2, 3 ] );
 		} );
 	} );
+
+	describe( 'getTransformedByMove', () => {
+		it( 'should increment offset if a range was moved to the same parent and closer offset', () => {
+			const position = new Position( [ 1, 2, 3 ], root );
+			const transformed = position.getTransformedByMove( new Position( [ 2 ], root ), new Position( [ 1, 2, 0 ], root ), 3, false );
+
+			expect( transformed.path ).to.deep.equal( [ 1, 2, 6 ] );
+		} );
+
+		it( 'should decrement offset if a range was moved from the same parent and closer offset', () => {
+			const position = new Position( [ 1, 2, 3 ], root );
+			const transformed = position.getTransformedByMove( new Position( [ 1, 2, 0 ], root ), new Position( [ 2 ], root ), 3, false );
+
+			expect( transformed.path ).to.deep.equal( [ 1, 2, 0 ] );
+		} );
+
+		it( 'should update path if a range contained this position', () => {
+			const position = new Position( [ 1, 2, 3 ], root );
+			const transformed = position.getTransformedByMove( new Position( [ 1, 1 ], root ), new Position( [ 2, 1 ], root ), 3, false );
+
+			expect( transformed.path ).to.deep.equal( [ 2, 2, 3 ] );
+		} );
+	} );
+
+	describe( '_getCombined', () => {
+		it( 'should return correct combination of this and given positions', () => {
+			const position = new Position( [ 1, 3, 4, 2 ], root );
+			const sourcePosition = new Position( [ 1, 1 ], root );
+			const targetPosition = new Position( [ 2, 5 ], root );
+
+			const combined = position._getCombined( sourcePosition, targetPosition );
+
+			expect( combined.path ).to.deep.equal( [ 2, 7, 4, 2 ] );
+		} );
+	} );
 } );