Merge pull request #892 from ckeditor/t/891

Fix: Reversed `ReinsertOperation` targets back to same graveyard holder from which the nodes were re-inserted. Closes #891.

Author: Reinmar

src/model/operation/reinsertoperation.js

@@ -45,7 +45,13 @@ export default class ReinsertOperation extends MoveOperation {
 	 * @returns {module:engine/model/operation/removeoperation~RemoveOperation}
 	 */
 	getReversed() {
-		return new RemoveOperation( this.targetPosition, this.howMany, this.baseVersion + 1 );
+		const removeOp = new RemoveOperation( this.targetPosition, this.howMany, this.baseVersion + 1 );
+
+		// Make sure that nodes are put back into the `$graveyardHolder` from which they got reinserted.
+		removeOp.targetPosition = this.sourcePosition;
+		removeOp._needsHolderElement = false;
+
+		return removeOp;
 	}
 
 	/**

src/model/operation/removeoperation.js

@@ -30,6 +30,25 @@ export default class RemoveOperation extends MoveOperation {
 		const graveyardPosition = new Position( graveyard, [ graveyard.maxOffset, 0 ] );
 
 		super( position, howMany, graveyardPosition, baseVersion );
+
+		/**
+		 * Flag informing whether this operation should insert "holder" element (`true`) or should move removed nodes
+		 * into existing "holder" element (`false`).
+		 *
+		 * The flag should be set to `true` for each "new" `RemoveOperation` that is each `RemoveOperation` originally
+		 * created to remove some nodes from document (most likely created through `Batch` API).
+		 *
+		 * The flag should be set to `false` for each `RemoveOperation` that got created by splitting the original
+		 * `RemoveOperation`, for example during operational transformation.
+		 *
+		 * The flag should be set to `false` whenever removing nodes that were re-inserted from graveyard. This will
+		 * ensure correctness of all other operations that might change something on those nodes. This will also ensure
+		 * that redundant empty graveyard holder elements are not created.
+		 *
+		 * @protected
+		 * @type {Boolean}
+		 */
+		this._needsHolderElement = true;
 	}
 
 	/**
@@ -59,39 +78,6 @@ export default class RemoveOperation extends MoveOperation {
 		this.targetPosition.path[ 0 ] = offset;
 	}
 
-	/**
-	 * Flag informing whether this operation should insert "holder" element (`true`) or should move removed nodes
-	 * into existing "holder" element (`false`).
-	 *
-	 * It is `true` for each `RemoveOperation` that is the first `RemoveOperation` in it's delta that points to given holder element.
-	 * This way only one `RemoveOperation` in given delta will insert "holder" element.
-	 *
-	 * @protected
-	 * @type {Boolean}
-	 */
-	get _needsHolderElement() {
-		if ( this.delta ) {
-			// Let's look up all operations from this delta in the same order as they are in the delta.
-			for ( let operation of this.delta.operations ) {
-				// We are interested only in `RemoveOperation`s.
-				if ( operation instanceof RemoveOperation ) {
-					// If the first `RemoveOperation` in the delta is this operation, this operation
-					// needs to insert holder element in the graveyard.
-					if ( operation == this ) {
-						return true;
-					} else if ( operation._holderElementOffset == this._holderElementOffset ) {
-						// If there is a `RemoveOperation` in this delta that "points" to the same holder element offset,
-						// that operation will already insert holder element at that offset. We should not create another holder.
-						return false;
-					}
-				}
-			}
-		}
-
-		// By default `RemoveOperation` needs holder element, so set it so, if the operation does not have delta.
-		return true;
-	}
-
 	/**
 	 * @inheritDoc
 	 * @returns {module:engine/model/operation/reinsertoperation~ReinsertOperation}
@@ -152,7 +138,9 @@ export default class RemoveOperation extends MoveOperation {
 		let sourcePosition = Position.fromJSON( json.sourcePosition, document );
 
 		const removeOp = new RemoveOperation( sourcePosition, json.howMany, json.baseVersion );
+
 		removeOp.targetPosition = Position.fromJSON( json.targetPosition, document );
+		removeOp._needsHolderElement = json._needsHolderElement;
 
 		return removeOp;
 	}

src/model/operation/transform.js

@@ -355,7 +355,11 @@ const ot = {
 			);
 
 			result.isSticky = a.isSticky;
-			result._holderElementOffset = a._holderElementOffset;
+
+			if ( a instanceof RemoveOperation ) {
+				result._needsHolderElement = a._needsHolderElement;
+				result._holderElementOffset = a._holderElementOffset;
+			}
 
 			return [ result ];
 		},
@@ -388,7 +392,7 @@ const ot = {
 				const aTarget = a.targetPosition.path[ 0 ];
 				const bTarget = b.targetPosition.path[ 0 ];
 
-				if ( aTarget >= bTarget && isStrong ) {
+				if ( aTarget > bTarget || ( aTarget == bTarget && isStrong ) ) {
 					// Do not change original operation!
 					a = a.clone();
 					a.targetPosition.path[ 0 ]++;
@@ -462,9 +466,10 @@ const ot = {
 				}
 			}
 
-			// 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 ) {
+				// 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 ( a instanceof RemoveOperation ) {
 					// If `a` operation was RemoveOperation, we cannot convert it to NoOperation.
 					// This is because RemoveOperation creates a holder in graveyard.
@@ -492,7 +497,7 @@ const ot = {
 			);
 
 			// Map transformed range(s) to operations and return them.
-			return ranges.reverse().map( ( range ) => {
+			return ranges.reverse().map( ( range, i ) => {
 				// We want to keep correct operation class.
 				let result = new a.constructor(
 					range.start,
@@ -502,7 +507,13 @@ const ot = {
 				);
 
 				result.isSticky = a.isSticky;
-				result._holderElementOffset = a._holderElementOffset;
+
+				if ( a instanceof RemoveOperation ) {
+					// Transformed `RemoveOperation` needs graveyard holder only when the original operation needed it.
+					// If `RemoveOperation` got split into two or more operations, only first operation needs graveyard holder.
+					result._needsHolderElement = a._needsHolderElement && i === 0;
+					result._holderElementOffset = a._holderElementOffset;
+				}
 
 				return result;
 			} );

tests/model/operation/reinsertoperation.js

@@ -61,14 +61,22 @@ describe( 'ReinsertOperation', () => {
 		expect( clone.baseVersion ).to.equal( operation.baseVersion );
 	} );
 
-	it( 'should create a RemoveOperation as a reverse', () => {
+	it( 'should create a correct RemoveOperation as a reverse', () => {
+		// Test reversed operation's target position.
+		graveyard.appendChildren( new Element( '$graveyardHolder' ) );
+
 		let reverse = operation.getReversed();
 
 		expect( reverse ).to.be.an.instanceof( RemoveOperation );
 		expect( reverse.baseVersion ).to.equal( 1 );
 		expect( reverse.howMany ).to.equal( 2 );
 		expect( reverse.sourcePosition.isEqual( rootPosition ) ).to.be.true;
-		expect( reverse.targetPosition.root ).to.equal( graveyardPosition.root );
+
+		// Reversed `ReinsertOperation` should target back to the same graveyard holder.
+		expect( reverse.targetPosition.isEqual( graveyardPosition ) ).to.be.true;
+
+		// Reversed `ReinsertOperation` should not create new graveyard holder.
+		expect( reverse._needsHolderElement ).to.be.false;
 	} );
 
 	it( 'should undo reinsert set of nodes by applying reverse operation', () => {

tests/model/operation/removeoperation.js

@@ -10,7 +10,6 @@ import MoveOperation from '../../../src/model/operation/moveoperation';
 import Position from '../../../src/model/position';
 import Text from '../../../src/model/text';
 import Element from '../../../src/model/element';
-import Delta from '../../../src/model/delta/delta';
 import { jsonParseStringify, wrapInDelta } from '../../../tests/model/_utils/utils';
 
 describe( 'RemoveOperation', () => {
@@ -52,7 +51,7 @@ describe( 'RemoveOperation', () => {
 		expect( operation ).to.be.instanceof( MoveOperation );
 	} );
 
-	it( 'should remove set of nodes and append them to holder element in graveyard root', () => {
+	it( 'should be able to remove set of nodes and append them to holder element in graveyard root', () => {
 		root.insertChildren( 0, new Text( 'fozbar' ) );
 
 		doc.applyOperation( wrapInDelta(
@@ -71,64 +70,24 @@ describe( 'RemoveOperation', () => {
 		expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'zb' );
 	} );
 
-	it( 'should create new holder element for remove operations in different deltas', () => {
+	it( 'should be able to remove set of nodes and append them to existing element in graveyard root', () => {
 		root.insertChildren( 0, new Text( 'fozbar' ) );
+		graveyard.appendChildren( new Element( '$graveyardHolder' ) );
 
-		doc.applyOperation( wrapInDelta(
-			new RemoveOperation(
-				new Position( root, [ 0 ] ),
-				1,
-				doc.version
-			)
-		) );
-
-		doc.applyOperation( wrapInDelta(
-			new RemoveOperation(
-				new Position( root, [ 0 ] ),
-				1,
-				doc.version
-			)
-		) );
-
-		doc.applyOperation( wrapInDelta(
-			new RemoveOperation(
-				new Position( root, [ 0 ] ),
-				1,
-				doc.version
-			)
-		) );
-
-		expect( graveyard.maxOffset ).to.equal( 3 );
-		expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f' );
-		expect( graveyard.getChild( 1 ).getChild( 0 ).data ).to.equal( 'o' );
-		expect( graveyard.getChild( 2 ).getChild( 0 ).data ).to.equal( 'z' );
-	} );
-
-	it( 'should not create new holder element for remove operation if it was already created for given delta', () => {
-		root.insertChildren( 0, new Text( 'fozbar' ) );
-
-		let delta = new Delta();
-
-		// This simulates i.e. RemoveOperation that got split into two operations during OT.
-		let removeOpA = new RemoveOperation(
-			new Position( root, [ 1 ] ),
-			1,
-			doc.version
-		);
-		let removeOpB = new RemoveOperation(
+		const op = new RemoveOperation(
 			new Position( root, [ 0 ] ),
 			1,
-			doc.version + 1
+			doc.version
 		);
 
-		delta.addOperation( removeOpA );
-		delta.addOperation( removeOpB );
+		// Manually set holder element properties.
+		op._needsHolderElement = false;
+		op._holderElementOffset = 0;
 
-		doc.applyOperation( removeOpA );
-		doc.applyOperation( removeOpB );
+		doc.applyOperation( wrapInDelta( op ) );
 
-		expect( graveyard.childCount ).to.equal( 1 );
-		expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'fo' );
+		expect( graveyard.maxOffset ).to.equal( 1 );
+		expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f' );
 	} );
 
 	it( 'should create RemoveOperation with same parameters when cloned', () => {
@@ -197,6 +156,8 @@ describe( 'RemoveOperation', () => {
 				doc.version
 			);
 
+			op._needsHolderElement = false;
+
 			const serialized = jsonParseStringify( op );
 
 			expect( serialized ).to.deep.equal( {
@@ -205,7 +166,8 @@ describe( 'RemoveOperation', () => {
 				howMany: 2,
 				isSticky: false,
 				sourcePosition: jsonParseStringify( op.sourcePosition ),
-				targetPosition: jsonParseStringify( op.targetPosition )
+				targetPosition: jsonParseStringify( op.targetPosition ),
+				_needsHolderElement: false
 			} );
 		} );
 	} );
@@ -218,6 +180,8 @@ describe( 'RemoveOperation', () => {
 				doc.version
 			);
 
+			op._needsHolderElement = false;
+
 			doc.graveyard.appendChildren( [ new Element( '$graveyardHolder' ), new Element( '$graveyardHolder' ) ] );
 
 			const serialized = jsonParseStringify( op );

tests/model/operation/transform.js

@@ -2757,6 +2757,47 @@ describe( 'transform', () => {
 	} );
 
 	describe( 'RemoveOperation', () => {
+		describe( 'by InsertOperation', () => {
+			it( 'should not need new graveyard holder if original operation did not needed it either', () => {
+				let op = new RemoveOperation( new Position( root, [ 1 ] ), 1, baseVersion );
+				op._needsHolderElement = false;
+
+				let transformBy = new InsertOperation( new Position( root, [ 0 ] ), [ new Node() ], baseVersion );
+
+				let transOp = transform( op, transformBy )[ 0 ];
+
+				expect( transOp._needsHolderElement ).to.be.false;
+			} );
+		} );
+
+		describe( 'by MoveOperation', () => {
+			it( 'should create not more than RemoveOperation that needs new graveyard holder', () => {
+				let op = new RemoveOperation( new Position( root, [ 1 ] ), 4, baseVersion );
+				let transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( root, [ 8 ] ), baseVersion );
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 2 );
+
+				expect( transOp[ 0 ]._needsHolderElement ).to.be.true;
+				expect( transOp[ 1 ]._needsHolderElement ).to.be.false;
+			} );
+
+			it( 'should not need new graveyard holder if original operation did not needed it either', () => {
+				let op = new RemoveOperation( new Position( root, [ 1 ] ), 4, baseVersion );
+				op._needsHolderElement = false;
+
+				let transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( root, [ 8 ] ), baseVersion );
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 2 );
+
+				expect( transOp[ 0 ]._needsHolderElement ).to.be.false;
+				expect( transOp[ 1 ]._needsHolderElement ).to.be.false;
+			} );
+		} );
+
 		describe( 'by RemoveOperation', () => {
 			it( 'removes same nodes and transformed is weak: change howMany to 0', () => {
 				let position = new Position( root, [ 2, 1 ] );