浏览代码

Changed: RemoveOperation now inserts "holder" elements in graveyard for removed nodes.

Szymon Cofalik 9 年之前
父节点
当前提交
1ca82284ee

+ 10 - 1
packages/ckeditor5-engine/src/model/delta/basic-transformations.js

@@ -230,10 +230,19 @@ addTransformationCase( UnwrapDelta, SplitDelta, ( a, b, isStrong ) => {
 	// If incoming unwrap delta tries to unwrap node that got split we should unwrap the original node and the split copy.
 	// This can be achieved either by reverting split and applying unwrap to singular node, or creating additional unwrap delta.
 	if ( compareArrays( a.position.path, b.position.getParentPath() ) === 'SAME' ) {
-		return [
+		const transformed = [
 			b.getReversed(),
 			a.clone()
 		];
+
+		// It's a kind of magic-magic-magic-maaaaagiiic!
+		transformed[ 1 ].operations[ 1 ].targetPosition.path[ 0 ]++;
+		// But seriously, we have to fix RemoveOperation in the second delta because reversed UnwrapDelta creates
+		// MergeDelta which also has RemoveOperation. Those two operations cannot point to the same "holder" element
+		// in the graveyard, so we fix it by hand. This is the only case where it happens in "special" transformation
+		// cases, and it won't happen for "default" transformation apart of RemoveDelta, where it is okay.
+
+		return transformed;
 	}
 
 	return defaultTransform( a, b, isStrong );

+ 59 - 4
packages/ckeditor5-engine/src/model/operation/removeoperation.js

@@ -7,6 +7,7 @@
 
 import MoveOperation from './moveoperation.js';
 import Position from '../position.js';
+import Element from '../element.js';
 import ReinsertOperation from './reinsertoperation.js';
 
 /**
@@ -25,17 +26,53 @@ export default class RemoveOperation extends MoveOperation {
 	 * @param {Number} baseVersion {@link engine.model.Document#version} on which operation can be applied.
 	 */
 	constructor( position, howMany, baseVersion ) {
-		// Position in a graveyard where nodes were moved.
-		const graveyardPosition = Position.createFromParentAndOffset( position.root.document.graveyard, 0 );
+		const graveyard = position.root.document.graveyard;
 
-		super( position, howMany, graveyardPosition, baseVersion );
+		super( position, howMany, new Position( graveyard, [ graveyard.getChildCount(), 0 ] ), baseVersion );
 	}
 
+	/**
+	 * @inheritDoc
+	 */
 	get type() {
 		return 'remove';
 	}
 
 	/**
+	 * Returns the offset of graveyard "holder" element, in which nodes removed by this operation are stored.
+	 *
+	 * @private
+	 * @type {Number}
+	 */
+	get _holderElementOffset() {
+		return this.targetPosition.path[ 0 ];
+	}
+
+	/**
+	 * Flag informing whether this operation should insert "holder" element (`true`) or should remove nodes
+	 * into existing "holder" element (`false`). It is `true` for each `RemoveOperation` that is the first `RemoveOperation`
+	 * in it's delta which points to given holder element.
+	 *
+	 * @protected
+	 * @type {Boolean}
+	 */
+	get _insertHolderElement() {
+		if ( this.delta ) {
+			for ( let operation of this.delta.operations ) {
+				if ( operation instanceof RemoveOperation ) {
+					if ( operation == this ) {
+						return true;
+					} else if ( operation._holderElementOffset == this._holderElementOffset ) {
+						return false;
+					}
+				}
+			}
+		}
+
+		return true;
+	}
+
+	/**
 	 * @returns {engine.model.operation.ReinsertOperation}
 	 */
 	getReversed() {
@@ -46,7 +83,25 @@ export default class RemoveOperation extends MoveOperation {
 	 * @returns {engine.model.operation.RemoveOperation}
 	 */
 	clone() {
-		return new RemoveOperation( this.sourcePosition, this.howMany, this.baseVersion );
+		let removeOperation = new RemoveOperation( this.sourcePosition, this.howMany, this.baseVersion );
+		removeOperation.targetPosition = Position.createFromPosition( this.targetPosition );
+		removeOperation.movedRangeStart = Position.createFromPosition( this.movedRangeStart );
+
+		return removeOperation;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	_execute() {
+		if ( this._insertHolderElement ) {
+			const graveyard = this.targetPosition.root;
+			const holderElement = new Element( '$graveyardHolder' );
+
+			graveyard.insertChildren( this.targetPosition.path[ 0 ], holderElement );
+		}
+
+		return super._execute();
 	}
 
 	/**

+ 39 - 3
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -146,6 +146,18 @@ const ot = {
 			// This will aggregate transformed ranges.
 			let ranges = [];
 
+			// Special case when MoveOperation is in fact a RemoveOperation. RemoveOperation not only moves nodes but also
+			// creates a "holder" element for them in graveyard. If there was a RemoveOperation pointing to an offset
+			// before this AttributeOperation, we have to increment AttributeOperation's offset.
+			if ( b instanceof RemoveOperation && b._insertHolderElement &&
+				a.range.root == b.targetPosition.root && a.range.start.path[ 0 ] >= b.targetPosition.path[ 0 ]
+			) {
+				// Do not change original operation!
+				a = a.clone();
+				a.range.start.path[ 0 ]++;
+				a.range.end.path[ 0 ]++;
+			}
+
 			// Difference is a part of changed range that is modified by AttributeOperation but is not affected
 			// by MoveOperation. This can be zero, one or two ranges (if moved range is inside changed range).
 			// Right now we will make a simplification and join difference ranges and transform them as one. We will cover rangeB later.
@@ -243,7 +255,22 @@ const ot = {
 				return [ b.getReversed() ];
 			}
 
-			// If one of operations is actually a remove operation, we force remove operation to be the "stronger" one
+			// Special case when both operations are RemoveOperations. RemoveOperation not only moves nodes but also
+			// (usually) creates a "holder" element for them in graveyard. Each RemoveOperation should move nodes to different
+			// "holder" element. If `a` operation points after `b` operation, we move `a` offset to acknowledge
+			// "holder" element insertion.
+			if ( a instanceof RemoveOperation && b instanceof RemoveOperation && b._insertHolderElement ) {
+				const aTarget = a.targetPosition.path[ 0 ];
+				const bTarget = b.targetPosition.path[ 0 ];
+
+				if ( aTarget > bTarget || ( aTarget == bTarget && isStrong ) ) {
+					// Do not change original operation!
+					a = a.clone();
+					a.targetPosition.path[ 0 ]++;
+				}
+			}
+
+			// If only one of operations is a remove operation, we force remove operation to be the "stronger" one
 			// to provide more expected results.
 			if ( a instanceof RemoveOperation && !( b instanceof RemoveOperation ) ) {
 				isStrong = true;
@@ -290,7 +317,10 @@ const ot = {
 			// transform `a` operation. Normally, when same nodes are moved, we stick with stronger operation's target.
 			// Here it is a move inside larger range so there is no conflict because after all, all nodes from
 			// smaller range will be moved to larger range target. The effect of this transformation feels natural.
-			let aIsInside = rangeB.containsRange( rangeA ) && rangeB.containsPosition( a.targetPosition );
+			// Also if we wouldn't do that, we would get different results on both sides of transformation (i.e. in
+			// collaborative editing).
+			let aIsInside = rangeB.containsRange( rangeA ) &&
+				( rangeB.containsPosition( a.targetPosition ) || rangeB.start.isEqual( a.targetPosition ) || rangeB.end.isEqual( a.targetPosition ) );
 
 			if ( common !== null && ( aCompB === 'EXTENSION' || ( aCompB === 'SAME' && isStrong ) || aIsInside ) && !bTargetsToA ) {
 				// Here we do not need to worry that newTargetPosition is inside moved range, because that
@@ -314,7 +344,13 @@ const ot = {
 			}
 
 			// Target position also could be affected by the other MoveOperation. We will transform it.
-			let newTargetPosition = a.targetPosition.getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !isStrong, b.isSticky );
+			let newTargetPosition = a.targetPosition.getTransformedByMove(
+				b.sourcePosition,
+				b.targetPosition,
+				b.howMany,
+				!isStrong,
+				b.isSticky || aIsInside
+			);
 
 			// Map transformed range(s) to operations and return them.
 			return ranges.reverse().map( ( range ) => {

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

@@ -94,7 +94,7 @@ describe( 'transform', () => {
 					operations: [
 						{
 							type: ReinsertOperation,
-							sourcePosition: new Position( gy, [ 0 ] ),
+							sourcePosition: new Position( gy, [ 0, 0 ] ),
 							howMany: 1,
 							targetPosition: new Position( root, [ 3, 3, 3 ] ),
 							baseVersion: baseVersion

+ 2 - 6
packages/ckeditor5-engine/tests/model/delta/transform/mergedelta.js

@@ -112,14 +112,10 @@ describe( 'transform', () => {
 							baseVersion: baseVersion
 						},
 						{
-							// This is `MoveOperation` instead of `RemoveOperation` because during OT,
-							// `RemoveOperation` may get converted to `MoveOperation`. Still, this expectation is
-							// correct because `RemoveOperation` is deriving from `MoveOperation`. So we can expect
-							// that something that was `RemoveOperation` is a `MoveOperation`.
-							type: MoveOperation,
+							type: RemoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 3 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0 ] ),
+							targetPosition: new Position( gy, [ 0, 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]

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

@@ -72,7 +72,7 @@ describe( 'transform', () => {
 							// is treated in OT as `MoveOperation` and might be converted to it. This is why we have to
 							// check whether the operation type is `MoveOperation`. This is all perfectly valid.
 							type: MoveOperation,
-							sourcePosition: new Position( gy, [ 0 ] ),
+							sourcePosition: new Position( gy, [ 0, 0 ] ),
 							howMany: 1,
 							targetPosition: new Position( root, [ 3, 3, 3 ] ),
 							baseVersion: baseVersion

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

@@ -68,7 +68,7 @@ describe( 'transform', () => {
 					operations: [
 						{
 							type: MoveOperation,
-							sourcePosition: new Position( gy, [ 0 ] ),
+							sourcePosition: new Position( gy, [ 0, 0 ] ),
 							howMany: 1,
 							targetPosition: new Position( root, [ 3, 3, 3 ] ),
 							baseVersion: baseVersion

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

@@ -73,7 +73,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 4 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0 ] ),
+							targetPosition: new Position( gy, [ 0, 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]
@@ -90,11 +90,10 @@ describe( 'transform', () => {
 							baseVersion: baseVersion + 2
 						},
 						{
-							// `RemoveOperation` as `MoveOperation`
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 15 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0 ] ),
+							targetPosition: new Position( gy, [ 1, 0 ] ),
 							baseVersion: baseVersion + 3
 						}
 					]
@@ -135,7 +134,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 4, 12 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0 ] ),
+							targetPosition: new Position( gy, [ 0, 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]

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

@@ -77,7 +77,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 4 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0 ] ),
+							targetPosition: new Position( gy, [ 0, 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]

+ 4 - 2
packages/ckeditor5-engine/tests/model/document/change-event.js

@@ -93,13 +93,15 @@ describe( 'Document change event', () => {
 
 		expect( changes ).to.have.length( 2 );
 
+		const holderElement = graveyard.getChild( 0 );
+
 		expect( types[ 0 ] ).to.equal( 'remove' );
-		expect( changes[ 0 ].range ).to.deep.equal( Range.createFromParentsAndOffsets( graveyard, 0, graveyard, 3 ) );
+		expect( changes[ 0 ].range ).to.deep.equal( Range.createFromParentsAndOffsets( holderElement, 0, holderElement, 3 ) );
 		expect( changes[ 0 ].sourcePosition ).to.deep.equal( Position.createFromParentAndOffset( root, 0 ) );
 
 		expect( types[ 1 ] ).to.equal( 'reinsert' );
 		expect( changes[ 1 ].range ).to.deep.equal( Range.createFromParentsAndOffsets( root, 0, root, 3 ) );
-		expect( changes[ 1 ].sourcePosition ).to.deep.equal( Position.createFromParentAndOffset( graveyard, 0 ) );
+		expect( changes[ 1 ].sourcePosition ).to.deep.equal( Position.createFromParentAndOffset( holderElement, 0 ) );
 	} );
 
 	it( 'should be fired when attribute is inserted', () => {

+ 8 - 8
packages/ckeditor5-engine/tests/model/operation/reinsertoperation.js

@@ -12,6 +12,7 @@ import ReinsertOperation from '/ckeditor5/engine/model/operation/reinsertoperati
 import RemoveOperation from '/ckeditor5/engine/model/operation/removeoperation.js';
 import MoveOperation from '/ckeditor5/engine/model/operation/moveoperation.js';
 import Position from '/ckeditor5/engine/model/position.js';
+import Element from '/ckeditor5/engine/model/element.js';
 import { jsonParseStringify } from '/tests/engine/model/_utils/utils.js';
 
 describe( 'ReinsertOperation', () => {
@@ -22,7 +23,7 @@ describe( 'ReinsertOperation', () => {
 		root = doc.createRoot();
 		graveyard = doc.graveyard;
 
-		graveyardPosition = new Position( graveyard, [ 0 ] );
+		graveyardPosition = new Position( graveyard, [ 0, 0 ] );
 		rootPosition = new Position( root, [ 0 ] );
 
 		operation = new ReinsertOperation(
@@ -70,28 +71,27 @@ describe( 'ReinsertOperation', () => {
 		expect( reverse.baseVersion ).to.equal( 1 );
 		expect( reverse.howMany ).to.equal( 2 );
 		expect( reverse.sourcePosition.isEqual( rootPosition ) ).to.be.true;
-		expect( reverse.targetPosition.isEqual( graveyardPosition ) ).to.be.true;
+		expect( reverse.targetPosition.root ).to.equal( graveyardPosition.root );
 	} );
 
 	it( 'should undo reinsert set of nodes by applying reverse operation', () => {
 		let reverse = operation.getReversed();
 
-		graveyard.insertChildren( 0, 'bar' );
+		const element = new Element();
+		element.insertChildren( 0, 'xx' );
+		graveyard.insertChildren( 0, element );
 
 		doc.applyOperation( operation );
 
 		expect( doc.version ).to.equal( 1 );
 		expect( root.getChildCount() ).to.equal( 2 );
+		expect( element.getChildCount() ).to.equal( 0 );
 
 		doc.applyOperation( reverse );
 
 		expect( doc.version ).to.equal( 2 );
 		expect( root.getChildCount() ).to.equal( 0 );
-		expect( graveyard.getChildCount() ).to.equal( 3 );
-
-		expect( graveyard.getChild( 0 ).character ).to.equal( 'b' );
-		expect( graveyard.getChild( 1 ).character ).to.equal( 'a' );
-		expect( graveyard.getChild( 2 ).character ).to.equal( 'r' );
+		// Don't check `element` - nodes are moved to new holder element.
 	} );
 
 	describe( 'toJSON', () => {

+ 37 - 4
packages/ckeditor5-engine/tests/model/operation/removeoperation.js

@@ -53,7 +53,7 @@ describe( 'RemoveOperation', () => {
 		expect( operation ).to.be.instanceof( MoveOperation );
 	} );
 
-	it( 'should remove set of nodes and append them to graveyard root', () => {
+	it( 'should remove set of nodes and append them to holder element in graveyard root', () => {
 		root.insertChildren( 0, 'fozbar' );
 
 		doc.applyOperation(
@@ -68,9 +68,42 @@ describe( 'RemoveOperation', () => {
 		expect( root.getChildCount() ).to.equal( 4 );
 		expect( root.getChild( 2 ).character ).to.equal( 'a' );
 
-		expect( graveyard.getChildCount() ).to.equal( 2 );
-		expect( graveyard.getChild( 0 ).character ).to.equal( 'z' );
-		expect( graveyard.getChild( 1 ).character ).to.equal( 'b' );
+		expect( graveyard.getChildCount() ).to.equal( 1 );
+		expect( graveyard.getChild( 0 ).getChild( 0 ).character ).to.equal( 'z' );
+		expect( graveyard.getChild( 0 ).getChild( 1 ).character ).to.equal( 'b' );
+	} );
+
+	it( 'should create new holder element for each remove operation', () => {
+		root.insertChildren( 0, 'fozbar' );
+
+		doc.applyOperation(
+			new RemoveOperation(
+				new Position( root, [ 0 ] ),
+				1,
+				doc.version
+			)
+		);
+
+		doc.applyOperation(
+			new RemoveOperation(
+				new Position( root, [ 0 ] ),
+				1,
+				doc.version
+			)
+		);
+
+		doc.applyOperation(
+			new RemoveOperation(
+				new Position( root, [ 0 ] ),
+				1,
+				doc.version
+			)
+		);
+
+		expect( graveyard.getChildCount() ).to.equal( 3 );
+		expect( graveyard.getChild( 0 ).getChild( 0 ).character ).to.equal( 'f' );
+		expect( graveyard.getChild( 1 ).getChild( 0 ).character ).to.equal( 'o' );
+		expect( graveyard.getChild( 2 ).getChild( 0 ).character ).to.equal( 'z' );
 	} );
 
 	it( 'should create RemoveOperation with same parameters when cloned', () => {

+ 58 - 3
packages/ckeditor5-engine/tests/model/operation/transform.js

@@ -7,22 +7,27 @@
 
 'use strict';
 
+import transform from '/ckeditor5/engine/model/operation/transform.js';
+
+import Document from '/ckeditor5/engine/model/document.js';
 import RootElement from '/ckeditor5/engine/model/rootelement.js';
 import Node from '/ckeditor5/engine/model/node.js';
 import Position from '/ckeditor5/engine/model/position.js';
 import Range from '/ckeditor5/engine/model/range.js';
-import transform from '/ckeditor5/engine/model/operation/transform.js';
+
 import InsertOperation from '/ckeditor5/engine/model/operation/insertoperation.js';
 import AttributeOperation from '/ckeditor5/engine/model/operation/attributeoperation.js';
 import RootAttributeOperation from '/ckeditor5/engine/model/operation/rootattributeoperation.js';
 import MoveOperation from '/ckeditor5/engine/model/operation/moveoperation.js';
+import RemoveOperation from '/ckeditor5/engine/model/operation/removeoperation.js';
 import NoOperation from '/ckeditor5/engine/model/operation/nooperation.js';
 
 describe( 'transform', () => {
-	let root, op, nodeA, nodeB, expected, baseVersion;
+	let doc, root, op, nodeA, nodeB, expected, baseVersion;
 
 	beforeEach( () => {
-		root = new RootElement( null );
+		doc = new Document();
+		root = doc.createRoot();
 
 		nodeA = new Node();
 		nodeB = new Node();
@@ -1330,6 +1335,56 @@ describe( 'transform', () => {
 				} );
 			} );
 		} );
+
+		describe( 'by RemoveOperation', () => {
+			beforeEach( () => {
+				start = new Position( doc.graveyard, [ 2, 0 ] );
+				end = new Position( doc.graveyard, [ 2, 4 ] );
+
+				range = new Range( start, end );
+
+				op = new AttributeOperation( range, 'foo', 'abc', 'bar', baseVersion );
+
+				expected.range = new Range( start, end );
+			} );
+
+			it( 'remove operation inserted holder element before attribute operation range: increment path', () => {
+				let transformBy = new RemoveOperation(
+					new Position( root, [ 0 ] ),
+					2,
+					baseVersion
+				);
+
+				transformBy.targetPosition.path = [ 0, 0 ];
+				transformBy.movedRangeStart.path = [ 0, 0 ];
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expected.range.start.path = [ 3, 0 ];
+				expected.range.end.path = [ 3, 4 ];
+
+				expectOperation( transOp[ 0 ], expected );
+			} );
+
+			it( 'remove operation inserted holder element after attribute operation range: do nothing', () => {
+				let transformBy = new RemoveOperation(
+					new Position( root, [ 0 ] ),
+					2,
+					baseVersion
+				);
+
+				transformBy.targetPosition.path = [ 4, 0 ];
+				transformBy.movedRangeStart.path = [ 4, 0 ];
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expectOperation( transOp[ 0 ], expected );
+			} );
+		} );
 	} );
 
 	describe( 'RootAttributeOperation', () => {