Ver Fonte

Changed Range, Position to make move operations greedy and sticky.

Szymon Cofalik há 10 anos atrás
pai
commit
85f030d8b1

+ 15 - 12
packages/ckeditor5-engine/src/treemodel/position.js

@@ -204,10 +204,11 @@ export default class Position {
 			// If nodes are removed from the node that is pointed by this position...
 			if ( deletePosition.offset < this.offset ) {
 				// And are removed from before an offset of that position...
-				// Decrement the offset accordingly.
 				if ( deletePosition.offset + howMany > this.offset ) {
-					transformed.offset = deletePosition.offset;
+					// Position is in removed range, it's no longer in the tree.
+					return null;
 				} else {
+					// Decrement the offset accordingly.
 					transformed.offset -= howMany;
 				}
 			}
@@ -215,7 +216,7 @@ export default class Position {
 			// If nodes are removed from a node that is on a path to this position...
 			const i = deletePosition.path.length - 1;
 
-			if ( deletePosition.offset < this.path[ i ] ) {
+			if ( deletePosition.offset <= this.path[ i ] ) {
 				// And are removed from before next node of that path...
 				if ( deletePosition.offset + howMany > this.path[ i ] ) {
 					// If the next node of that path is removed return null
@@ -271,7 +272,7 @@ export default class Position {
 	}
 
 	/**
-	 * Returns this position after being updated by moving `howMany` attributes from `sourcePosition` to `targetPosition`.
+	 * Returns this position after being updated by moving `howMany` nodes from `sourcePosition` to `targetPosition`.
 	 *
 	 * @param {core.treeModel.Position} sourcePosition Position before the first element to move.
 	 * @param {core.treeModel.Position} targetPosition Position where moved elements will be inserted.
@@ -279,20 +280,22 @@ export default class Position {
 	 * @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.
+	 * @param {Boolean} [sticky] Flag indicating whether this position "sticks" to range, that is if it should be moved
+	 * with the moved range if it is equal to one of range's boundaries.
 	 * @returns {core.treeModel.Position} Transformed position.
 	 */
-	getTransformedByMove( sourcePosition, targetPosition, howMany, insertBefore ) {
+	getTransformedByMove( sourcePosition, targetPosition, howMany, insertBefore, sticky ) {
 		// 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.
+		if ( transformed === null || ( transformed.isEqual( sourcePosition ) && sticky ) ) {
+			// This position is inside moved range (or sticks to it).
+			// In this case, we calculate a combination of this position, move source position and target position.
 			transformed = this._getCombined( sourcePosition, targetPosition );
+		} else {
+			// This position is not inside a removed range.
+			// In next step, we simply reflect inserting `howMany` nodes, which might further affect the position.
+			transformed = transformed.getTransformedByInsertion( targetPosition, howMany, insertBefore );
 		}
 
 		return transformed;

+ 15 - 15
packages/ckeditor5-engine/src/treemodel/range.js

@@ -337,7 +337,7 @@ export default class Range {
 	/**
 	 * Returns an array containing one or two {core.treeModel.Range ranges} that are a result of transforming this
 	 * {@link core.treeModel.Range range} by inserting `howMany` nodes at `insertPosition`. Two {@link core.treeModel.Range ranges} are
-	 * returned if the insertion was inside this {@link core.treeModel.Range range}.
+	 * returned if the insertion was inside this {@link core.treeModel.Range range} and `spread` is set to `true`.
 	 *
 	 * Examples:
 	 *
@@ -345,11 +345,14 @@ export default class Range {
 	 *		let transformed = range.getTransformedByInsertion( new Position( root, [ 1 ] ), 2 );
 	 *		// transformed array has one range from [ 4, 7 ] to [ 6, 0, 1 ]
 	 *
+	 *		transformed = range.getTransformedByInsertion( new Position( root, [ 4, 0, 0 ] ), 4 );
+	 *		// transformed array has one range from [ 2, 7 ] to [ 4, 0, 5 ]
+	 *
 	 *		transformed = range.getTransformedByInsertion( new Position( root, [ 3, 2 ] ), 4 );
-	 *		// transformed array has two ranges: from [ 2, 7 ] to [ 3, 2 ] and from [ 3, 6 ] to [ 4, 0, 1 ]
+	 *		// transformed array has one range, which is equal to original range
 	 *
 	 *		transformed = range.getTransformedByInsertion( new Position( root, [ 3, 2 ] ), 4, true );
-	 *		// transformed array has one range which is equal to `range`. This is because of spreadOnlyOnSameLevel flag.
+	 *		// transformed array has two ranges: from [ 2, 7 ] to [ 3, 2 ] and from [ 3, 6 ] to [ 4, 0, 1 ]
 	 *
 	 * @param {core.treeModel.Position} insertPosition Position where nodes are inserted.
 	 * @param {Number} howMany How many nodes are inserted.
@@ -357,12 +360,8 @@ export default class Range {
 	 * if insertion was inside a node from this {core.treeModel.Range range} but not in the range itself.
 	 * @returns {Array.<core.treeModel.Range>} Result of the transformation.
 	 */
-	getTransformedByInsertion( insertPosition, howMany, spreadOnlyOnSameLevel ) {
-		// Flag indicating whether this whole range and given insertPosition is on the same tree level.
-		const areOnSameLevel = utils.compareArrays( this.start.getParentPath(), this.end.getParentPath() ) == 'SAME' &&
-			utils.compareArrays( this.start.getParentPath(), insertPosition.getParentPath() ) == 'SAME';
-
-		if ( this.containsPosition( insertPosition ) && ( !spreadOnlyOnSameLevel || areOnSameLevel ) ) {
+	getTransformedByInsertion( insertPosition, howMany, spread ) {
+		if ( spread && this.containsPosition( insertPosition ) ) {
 			// Range has to be spread. The first part is from original start to the spread point.
 			// The other part is from spread point to the original end, but transformed by
 			// insertion to reflect insertion changes.
@@ -371,17 +370,18 @@ export default class Range {
 				new Range( this.start, insertPosition ),
 				new Range(
 					insertPosition.getTransformedByInsertion( insertPosition, howMany, true ),
-					this.end.getTransformedByInsertion( insertPosition, howMany, true )
+					this.end.getTransformedByInsertion( insertPosition, howMany, false )
 				)
 			];
 		} else {
-			// If insertion is not inside the range, simply transform range boundaries (positions) by the insertion.
-			// Both, one or none of them might be affected by the insertion.
-
 			const range = Range.createFromRange( this );
 
-			range.start = range.start.getTransformedByInsertion( insertPosition, howMany, true );
-			range.end = range.end.getTransformedByInsertion( insertPosition, howMany, false );
+			// We want "greedy" transformation if spread is set to false and "non-greedy" if it is set to true.
+			// To achieve "greedy" transformation we want range.start insertBefore be false, and range.end insertBefore true.
+			// Opposite is for "non-greedy" transformation.
+
+			range.start = range.start.getTransformedByInsertion( insertPosition, howMany, spread );
+			range.end = range.end.getTransformedByInsertion( insertPosition, howMany, !spread );
 
 			return [ range ];
 		}

+ 16 - 1
packages/ckeditor5-engine/tests/treemodel/operation/transform.js

@@ -1363,7 +1363,7 @@ describe( 'transform', () => {
 
 			it( 'target at offset after range offset: no operation update', () => {
 				let transformBy = new InsertOperation(
-					new Position( root, [ 2, 2, 6 ] ),
+					new Position( root, [ 2, 2, 7 ] ),
 					[ nodeA, nodeB ],
 					baseVersion
 				);
@@ -1523,6 +1523,21 @@ describe( 'transform', () => {
 
 				expectOperation( transOp[ 1 ], expected );
 			} );
+
+			it( 'target at offset same as range end boundary: expand range', () => {
+				let transformBy = new InsertOperation(
+					new Position( root, [ 2, 2, 6 ] ),
+					[ nodeA, nodeB ],
+					baseVersion
+				);
+
+				let transOp = transform( op, transformBy );
+
+				expected.howMany = 4;
+
+				expect( transOp.length ).to.equal( 1 );
+				expectOperation( transOp[ 0 ], expected );
+			} );
 		} );
 
 		describe( 'by AttributeOperation', () => {

+ 16 - 2
packages/ckeditor5-engine/tests/treemodel/position.js

@@ -472,11 +472,11 @@ describe( 'position', () => {
 			expect( transformed.offset ).to.equal( 5 );
 		} );
 
-		it( 'should set position offset to deletion offset if position is between removed nodes', () => {
+		it( 'should return null if original position is between removed nodes', () => {
 			const position = new Position( root, [ 1, 2, 4 ] );
 			const transformed = position.getTransformedByDeletion( new Position( root, [ 1, 2, 3 ] ), 5 );
 
-			expect( transformed.offset ).to.equal( 3 );
+			expect( transformed ).to.be.null;
 		} );
 
 		it( 'should not decrement offset if deletion position is in different root', () => {
@@ -517,12 +517,26 @@ describe( 'position', () => {
 		} );
 
 		it( 'should decrement offset if a range was moved from the same parent and closer offset', () => {
+			const position = new Position( root, [ 1, 2, 6 ] );
+			const transformed = position.getTransformedByMove( new Position( root, [ 1, 2, 0 ] ), new Position( root, [ 2 ] ), 3, false );
+
+			expect( transformed.path ).to.deep.equal( [ 1, 2, 3 ] );
+		} );
+
+		it( 'should decrement offset if position was at the end of a range and move was not sticky', () => {
 			const position = new Position( root, [ 1, 2, 3 ] );
 			const transformed = position.getTransformedByMove( new Position( root, [ 1, 2, 0 ] ), new Position( root, [ 2 ] ), 3, false );
 
 			expect( transformed.path ).to.deep.equal( [ 1, 2, 0 ] );
 		} );
 
+		it( 'should update path if position was at the end of a range and move was sticky', () => {
+			const position = new Position( root, [ 1, 2, 3 ] );
+			const transformed = position.getTransformedByMove( new Position( root, [ 1, 2, 0 ] ), new Position( root, [ 2 ] ), 3, false, true );
+
+			expect( transformed.path ).to.deep.equal( [ 5 ] );
+		} );
+
 		it( 'should update path if a range contained this position', () => {
 			const position = new Position( root, [ 1, 2, 3 ] );
 			const transformed = position.getTransformedByMove( new Position( root, [ 1, 1 ] ), new Position( root, [ 2, 1 ] ), 3, false );

+ 37 - 3
packages/ckeditor5-engine/tests/treemodel/range.js

@@ -360,9 +360,19 @@ describe( 'Range', () => {
 			expect( transformed[ 0 ].end.path[ 0 ] ).to.equal( 6 );
 		} );
 
-		it( 'should return array with two ranges and updated positions if insertion was in the middle of range', () => {
+		it( 'should expand range if insertion was in the middle of range', () => {
 			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 5, 4 ] ) );
-			const transformed = range.getTransformedByInsertion( new Position( root, [ 4, 1, 6 ] ), 4 );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 5, 0 ] ), 4 );
+
+			expect( transformed.length ).to.equal( 1 );
+
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 5, 8 ] );
+		} );
+
+		it( 'should return array with two ranges if insertion was in the middle of range and spread flag was set', () => {
+			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 5, 4 ] ) );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 4, 1, 6 ] ), 4, true );
 
 			expect( transformed.length ).to.equal( 2 );
 
@@ -373,10 +383,34 @@ describe( 'Range', () => {
 			expect( transformed[ 1 ].end.path ).to.deep.equal( [ 5, 4 ] );
 		} );
 
-		it( 'should not updated positions if insertion is after range', () => {
+		it( 'should expand range if insertion is equal to start boundary of the range', () => {
+			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 3, 8 ] ) );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 3, 2 ] ), 3 );
+
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 3, 11 ] );
+		} );
+
+		it( 'should not update positions if insertion is before range (but not equal to the start boundary)', () => {
+			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 3, 8 ] ) );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 3, 1 ] ), 3 );
+
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 5 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 3, 11 ] );
+		} );
+
+		it( 'should expand range if insertion is equal to end boundary of the range', () => {
 			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 4, 4 ] ) );
 			const transformed = range.getTransformedByInsertion( new Position( root, [ 4, 4 ] ), 3 );
 
+			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 4, 7 ] );
+		} );
+
+		it( 'should not update positions if insertion is after range (but not equal to the end boundary)', () => {
+			const range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 4, 4 ] ) );
+			const transformed = range.getTransformedByInsertion( new Position( root, [ 4, 5 ] ), 3 );
+
 			expect( transformed[ 0 ].start.path ).to.deep.equal( [ 3, 2 ] );
 			expect( transformed[ 0 ].end.path ).to.deep.equal( [ 4, 4 ] );
 		} );