Przeglądaj źródła

Fix: Final OT followup fixes after merging several branches.

Szymon Cofalik 7 lat temu
rodzic
commit
e390211642

+ 31 - 15
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -60,10 +60,10 @@ function transform( a, b, context = {} ) {
 		log.error( 'Transformed operation', a );
 		log.error( 'Operation transformed by', b );
 		log.error( 'context.aIsStrong', context.aIsStrong );
-		log.error( 'context.wasUndone( a )', context.wasUndone( a ) );
-		log.error( 'context.wasUndone( b )', context.wasUndone( b ) );
-		log.error( 'context.getRelation( a, b )', context.getRelation( a, b ) );
-		log.error( 'context.getRelation( b, a )', context.getRelation( b, a ) );
+		log.error( 'context.aWasUndone', context.aWasUndone );
+		log.error( 'context.bWasUndone', context.bWasUndone );
+		log.error( 'context.abRelation', context.abRelation );
+		log.error( 'context.baRelation', context.baRelation );
 
 		throw e;
 	}
@@ -500,15 +500,18 @@ setTransformation( AttributeOperation, MergeOperation, ( a, b ) => {
 	//			Do it only, if there is more than one element in attribute range. If there is only one element,
 	//			it will be handled by the default algorithm.
 	//
-	const howMany = a.range.end.offset - a.range.start.offset;
-
-	if ( howMany > 1 && a.range.start.hasSameParentAs( b.deletionPosition ) ) {
+	if ( a.range.start.hasSameParentAs( b.deletionPosition ) ) {
 		if ( a.range.containsPosition( b.deletionPosition ) || a.range.start.isEqual( b.deletionPosition ) ) {
 			ranges.push( Range.createFromPositionAndShift( b.graveyardPosition, 1 ) );
 		}
 	}
 
-	ranges.push( a.range._getTransformedByMergeOperation( b ) );
+	const range = a.range._getTransformedByMergeOperation( b );
+
+	// Do not add empty (collapsed) ranges to the result. `range` may be collapsed if it contained only the merged element.
+	if ( !range.isCollapsed ) {
+		ranges.push( range );
+	}
 
 	// Create `AttributeOperation`s out of the ranges.
 	return ranges.map( range => {
@@ -605,10 +608,13 @@ setTransformation( AttributeOperation, SplitOperation, ( a, b ) => {
 	if ( a.range.start.hasSameParentAs( b.position ) && a.range.containsPosition( b.position ) ) {
 		const secondPart = a.clone();
 
-		secondPart.range.start = Position.createFromPosition( b.moveTargetPosition );
-		secondPart.range.end = a.range.end._getCombined( b.position, b.moveTargetPosition );
+		secondPart.range = new Range(
+			Position.createFromPosition( b.moveTargetPosition ),
+			a.range.end._getCombined( b.position, b.moveTargetPosition )
+		);
 
 		a.range.end = Position.createFromPosition( b.position );
+		a.range.end.stickiness = 'toPrevious';
 
 		return [ a, secondPart ];
 	}
@@ -1036,7 +1042,7 @@ setTransformation( MoveOperation, MoveOperation, ( a, b, context ) => {
 
 	// Assign `context.aIsStrong` to a different variable, because the value may change during execution of
 	// this algorithm and we do not want to override original `context.aIsStrong` that will be used in later transformations.
-	let aIsStrong = context.aIsStrong || context.getRelation( a, b ) == 'insertBefore';
+	let aIsStrong = context.aIsStrong;
 
 	if ( context.abRelation == 'insertBefore' ) {
 		aIsStrong = true;
@@ -1379,6 +1385,7 @@ setTransformation( RenameOperation, InsertOperation, ( a, b ) => {
 setTransformation( RenameOperation, MergeOperation, ( a, b ) => {
 	if ( a.position.isEqual( b.deletionPosition ) ) {
 		a.position = Position.createFromPosition( b.graveyardPosition );
+		a.position.stickiness = 'toNext';
 
 		return [ a ];
 	}
@@ -1613,15 +1620,24 @@ setTransformation( WrapOperation, InsertOperation, ( a, b, context ) => {
 } );
 
 setTransformation( WrapOperation, MergeOperation, ( a, b ) => {
+	if ( a.graveyardPosition ) {
+		a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 );
+	}
+
+	// Case 1:	The element to wrap got merged.
+	//
+	if ( a.position.isEqual( b.deletionPosition ) ) {
+		a.position = Position.createFromPosition( b.graveyardPosition );
+		a.position.stickiness = 'toNext';
+
+		return [ a ];
+	}
+
 	const transformed = a.wrappedRange._getTransformedByMergeOperation( b );
 
 	a.position = transformed.start;
 	a.howMany = transformed.end.offset - transformed.start.offset;
 
-	if ( a.graveyardPosition ) {
-		a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 );
-	}
-
 	return [ a ];
 } );
 

+ 29 - 5
packages/ckeditor5-engine/tests/model/liverange.js

@@ -9,6 +9,8 @@ import Position from '../../src/model/position';
 import LiveRange from '../../src/model/liverange';
 import Range from '../../src/model/range';
 import Text from '../../src/model/text';
+import MoveOperation from '../../src/model/operation/moveoperation';
+import MergeOperation from '../../src/model/operation/mergeoperation';
 import { stringify, setData } from '../../src/dev-utils/model';
 
 describe( 'LiveRange', () => {
@@ -154,20 +156,42 @@ describe( 'LiveRange', () => {
 		expect( spy.args[ 0 ][ 2 ].isEqual( sourcePosition ) ).to.be.true;
 	} );
 
+	// This scenario is hypothetically possible during OT if the element to merge-into was removed.
+	// In that case a live range inside the merged element will be merged into an element which is in graveyard.
+	// Because it may happen only in OT, in the test below we will generate operations by hand.
 	it( 'should pass deletion position if range was removed (merge)', () => {
-		const live = new LiveRange( new Position( root, [ 1 ] ), new Position( root, [ 2 ] ) );
+		const live = new LiveRange( new Position( root, [ 1, 0 ] ), new Position( root, [ 1, 1 ] ) );
 
 		const spy = sinon.spy();
 		live.on( 'change:range', spy );
 
-		const mergePosition = new Position( root, [ 1 ] );
-
 		model.change( writer => {
-			writer.merge( mergePosition );
+			const batch = writer.batch;
+			const gy = model.document.graveyard;
+
+			const remove = new MoveOperation(
+				new Position( root, [ 0 ] ),
+				1,
+				new Position( gy, [ 0 ] ),
+				model.document.version
+			);
+
+			const merge = new MergeOperation(
+				new Position( root, [ 0, 0 ] ),
+				new Position( gy, [ 0, 0 ] ),
+				new Position( gy, [ 0 ] ),
+				model.document.version + 1
+			);
+
+			batch.addOperation( remove );
+			model.applyOperation( remove );
+
+			batch.addOperation( merge );
+			model.applyOperation( merge );
 		} );
 
 		// Second parameter is deletion position.
-		expect( spy.args[ 0 ][ 2 ].isEqual( mergePosition ) ).to.be.true;
+		expect( spy.args[ 1 ][ 2 ].isEqual( new Position( root, [ 0 ] ) ) ).to.be.true;
 	} );
 
 	describe( 'should get transformed and fire change:range if', () => {

+ 9 - 1
packages/ckeditor5-engine/tests/model/operation/transform.js

@@ -73,10 +73,18 @@ describe( 'transform', () => {
 		a.position = null;
 
 		expect( () => {
-			transform.transform( a, b );
+			transform.transform( a, b, {
+				aIsStrong: true,
+				aWasUndone: false,
+				bWasUndone: false,
+				abRelation: null,
+				baRelation: null
+			} );
 		} ).to.throw();
 
 		sinon.assert.called( spy );
+
+		log.error.restore();
 	} );
 
 	describe( 'InsertOperation', () => {