Ver Fonte

Merge branch 'master' into t/1439

Aleksander Nowodzinski há 7 anos atrás
pai
commit
37f153b71b

+ 1 - 1
packages/ckeditor5-engine/CONTRIBUTING.md

@@ -1,4 +1,4 @@
 Contributing
 ========================================
 
-Information about contributing can be found at the following page: <https://github.com/ckeditor/ckeditor5/blob/master/CONTRIBUTING.md>.
+See the [official contributors' guide to CKEditor 5](https://ckeditor.com/docs/ckeditor5/latest/framework/guides/contributing/contributing.html) to learn more.

+ 1 - 1
packages/ckeditor5-engine/src/model/differ.js

@@ -423,7 +423,7 @@ export default class Differ {
 			// If change happens at the same position...
 			if ( a.position.isEqual( b.position ) ) {
 				// Keep chronological order of operations.
-				return a.changeCount < b.changeCount ? -1 : 1;
+				return a.changeCount - b.changeCount;
 			}
 
 			// If positions differ, position "on the left" should be earlier in the result.

+ 0 - 1
packages/ckeditor5-engine/src/model/model.js

@@ -349,7 +349,6 @@ export default class Model {
 	 * @fires deleteContent
 	 * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
 	 * Selection of which the content should be deleted.
-	 * @param {module:engine/model/batch~Batch} batch Batch to which the operations will be added.
 	 * @param {Object} [options]
 	 * @param {Boolean} [options.leaveUnmerged=false] Whether to merge elements after removing the content of the selection.
 	 *

+ 92 - 20
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -449,6 +449,8 @@ class ContextFactory {
 							this._setRelation( opA, opB, 'insertAtSource' );
 						} else if ( opA.targetPosition.isEqual( opB.deletionPosition ) ) {
 							this._setRelation( opA, opB, 'insertBetween' );
+						} else if ( opA.targetPosition.isAfter( opB.sourcePosition ) ) {
+							this._setRelation( opA, opB, 'moveTargetAfter' );
 						}
 
 						break;
@@ -503,6 +505,12 @@ class ContextFactory {
 
 						break;
 					}
+
+					case SplitOperation: {
+						if ( opA.sourcePosition.isEqual( opB.splitPosition ) ) {
+							this._setRelation( opA, opB, 'splitAtSource' );
+						}
+					}
 				}
 
 				break;
@@ -1157,7 +1165,10 @@ setTransformation( MergeOperation, MergeOperation, ( a, b, context ) => {
 	//
 	// If neither or both operations point to graveyard, then let `aIsStrong` decide.
 	//
-	if ( a.sourcePosition.isEqual( b.sourcePosition ) && !a.targetPosition.isEqual( b.targetPosition ) && !context.bWasUndone ) {
+	if (
+		a.sourcePosition.isEqual( b.sourcePosition ) && !a.targetPosition.isEqual( b.targetPosition ) &&
+		!context.bWasUndone && context.abRelation != 'splitAtSource'
+	) {
 		const aToGraveyard = a.targetPosition.root.rootName == '$graveyard';
 		const bToGraveyard = b.targetPosition.root.rootName == '$graveyard';
 
@@ -1336,7 +1347,7 @@ setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => {
 	// In this scenario the merge operation is now transformed by the split which has undone the previous merge operation.
 	// So now we are fixing situation which was skipped in `MergeOperation` x `MergeOperation` case.
 	//
-	if ( a.sourcePosition.isEqual( b.splitPosition ) && context.abRelation == 'mergeSameElement' ) {
+	if ( a.sourcePosition.isEqual( b.splitPosition ) && ( context.abRelation == 'mergeSameElement' || a.sourcePosition.offset > 0 ) ) {
 		a.sourcePosition = Position.createFromPosition( b.moveTargetPosition );
 		a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );
 
@@ -1394,9 +1405,9 @@ setTransformation( MoveOperation, MoveOperation, ( a, b, context ) => {
 	let insertBefore = !context.aIsStrong;
 
 	// If the relation is set, then use it to decide nodes order.
-	if ( context.abRelation == 'insertBefore' ) {
+	if ( context.abRelation == 'insertBefore' || context.baRelation == 'insertAfter' ) {
 		insertBefore = true;
-	} else if ( context.abRelation == 'insertAfter' ) {
+	} else if ( context.abRelation == 'insertAfter' || context.baRelation == 'insertBefore' ) {
 		insertBefore = false;
 	}
 
@@ -1568,7 +1579,7 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => {
 	// Do not transform if target position is same as split insertion position and this split comes from undo.
 	// This should be done on relations but it is too much work for now as it would require relations working in collaboration.
 	// We need to make a decision how we will resolve such conflict and this is less harmful way.
-	if ( !a.targetPosition.isEqual( b.insertionPosition ) || !b.graveyardPosition ) {
+	if ( !a.targetPosition.isEqual( b.insertionPosition ) || !b.graveyardPosition || context.abRelation == 'moveTargetAfter' ) {
 		newTargetPosition = a.targetPosition._getTransformedBySplitOperation( b );
 	}
 
@@ -1658,12 +1669,21 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => {
 	// The default case.
 	//
 	const transformed = moveRange._getTransformedBySplitOperation( b );
+	const ranges = [ transformed ];
 
-	a.sourcePosition = transformed.start;
-	a.howMany = transformed.end.offset - transformed.start.offset;
-	a.targetPosition = newTargetPosition;
+	// Case 5:
+	//
+	// Moved range contains graveyard element used by split operation. Add extra move operation to the result.
+	//
+	if ( b.graveyardPosition ) {
+		const movesGraveyardElement = moveRange.start.isEqual( b.graveyardPosition ) || moveRange.containsPosition( b.graveyardPosition );
 
-	return [ a ];
+		if ( a.howMany > 1 && movesGraveyardElement ) {
+			ranges.push( Range.createFromPositionAndShift( b.insertionPosition, 1 ) );
+		}
+	}
+
+	return _makeMoveOperationsFromRanges( ranges, newTargetPosition );
 } );
 
 setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => {
@@ -1681,7 +1701,30 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => {
 			// removed nodes might be unexpected. This means that in this scenario we will reverse merging and remove the element.
 			//
 			if ( !context.aWasUndone ) {
-				return [ b.getReversed(), a ];
+				const results = [];
+
+				let gyMoveSource = Position.createFromPosition( b.graveyardPosition );
+				let splitNodesMoveSource = Position.createFromPosition( b.targetPosition );
+
+				if ( a.howMany > 1 ) {
+					results.push( new MoveOperation( a.sourcePosition, a.howMany - 1, a.targetPosition, 0 ) );
+					gyMoveSource = gyMoveSource._getTransformedByInsertion( a.targetPosition, a.howMany - 1 );
+					splitNodesMoveSource = splitNodesMoveSource._getTransformedByMove( a.sourcePosition, a.targetPosition, a.howMany - 1 );
+				}
+
+				const gyMoveTarget = b.deletionPosition._getCombined( a.sourcePosition, a.targetPosition );
+				const gyMove = new MoveOperation( gyMoveSource, 1, gyMoveTarget, 0 );
+
+				const targetPositionPath = gyMove.getMovedRangeStart().path.slice();
+				targetPositionPath.push( 0 );
+
+				const splitNodesMoveTarget = new Position( gyMove.targetPosition.root, targetPositionPath );
+				const splitNodesMove = new MoveOperation( splitNodesMoveSource, b.howMany, splitNodesMoveTarget, 0 );
+
+				results.push( gyMove );
+				results.push( splitNodesMove );
+
+				return results;
 			}
 		} else {
 			// Case 2:
@@ -1909,11 +1952,32 @@ setTransformation( SplitOperation, MergeOperation, ( a, b, context ) => {
 } );
 
 setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
+	const rangeToMove = Range.createFromPositionAndShift( b.sourcePosition, b.howMany );
+
 	if ( a.graveyardPosition ) {
+		// Case 1:
+		//
+		// Split operation graveyard node was moved. In this case move operation is stronger. Since graveyard element
+		// is already moved to the correct position, we need to only move the nodes after the split position.
+		// This will be done by `MoveOperation` instead of `SplitOperation`.
+		//
+		if ( rangeToMove.start.isEqual( a.graveyardPosition ) || rangeToMove.containsPosition( a.graveyardPosition ) ) {
+			const sourcePosition = a.splitPosition._getTransformedByMoveOperation( b );
+
+			const newParentPosition = a.graveyardPosition._getTransformedByMoveOperation( b );
+			const newTargetPath = newParentPosition.path.slice();
+			newTargetPath.push( 0 );
+
+			const newTargetPosition = new Position( newParentPosition.root, newTargetPath );
+			const moveOp = new MoveOperation( sourcePosition, a.howMany, newTargetPosition, 0 );
+
+			return [ moveOp ];
+		}
+
 		a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b );
 	}
 
-	// Case 1:
+	// Case 2:
 	//
 	// If the split position is inside the moved range, we need to shift the split position to a proper place.
 	// The position cannot be moved together with moved range because that would result in splitting of an incorrect element.
@@ -1930,8 +1994,6 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
 	// After split:
 	// <paragraph>A</paragraph><paragraph>d</paragraph><paragraph>Xbcyz</paragraph>
 	//
-	const rangeToMove = Range.createFromPositionAndShift( b.sourcePosition, b.howMany );
-
 	if ( a.splitPosition.hasSameParentAs( b.sourcePosition ) && rangeToMove.containsPosition( a.splitPosition ) ) {
 		const howManyRemoved = b.howMany - ( a.splitPosition.offset - b.sourcePosition.offset );
 		a.howMany -= howManyRemoved;
@@ -1946,7 +2008,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
 		return [ a ];
 	}
 
-	// Case 2:
+	// Case 3:
 	//
 	// Split is at a position where nodes were moved.
 	//
@@ -1964,13 +2026,16 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
 	}
 
 	// The default case.
+	// Don't change `howMany` if move operation does not really move anything.
 	//
-	if ( a.splitPosition.hasSameParentAs( b.sourcePosition ) && a.splitPosition.offset <= b.sourcePosition.offset ) {
-		a.howMany -= b.howMany;
-	}
+	if ( !b.sourcePosition.isEqual( b.targetPosition ) ) {
+		if ( a.splitPosition.hasSameParentAs( b.sourcePosition ) && a.splitPosition.offset <= b.sourcePosition.offset ) {
+			a.howMany -= b.howMany;
+		}
 
-	if ( a.splitPosition.hasSameParentAs( b.targetPosition ) && a.splitPosition.offset < b.targetPosition.offset ) {
-		a.howMany += b.howMany;
+		if ( a.splitPosition.hasSameParentAs( b.targetPosition ) && a.splitPosition.offset < b.targetPosition.offset ) {
+			a.howMany += b.howMany;
+		}
 	}
 
 	// Change position stickiness to force a correct transformation.
@@ -2137,7 +2202,14 @@ function _makeMoveOperationsFromRanges( ranges, targetPosition ) {
 	for ( let i = 0; i < ranges.length; i++ ) {
 		// Create new operation out of a range and target position.
 		const range = ranges[ i ];
-		const op = new MoveOperation( range.start, range.end.offset - range.start.offset, targetPosition, 0 );
+		const op = new MoveOperation(
+			range.start,
+			range.end.offset - range.start.offset,
+			// If the target is the end of the move range this operation doesn't really move anything.
+			// In this case, it is better for OT to use range start instead of range end.
+			targetPosition.isEqual( range.end ) ? range.start : targetPosition,
+			0
+		);
 
 		operations.push( op );
 

+ 8 - 4
packages/ckeditor5-engine/src/model/range.js

@@ -517,13 +517,17 @@ export default class Range {
 	 */
 	_getTransformedBySplitOperation( operation ) {
 		const start = this.start._getTransformedBySplitOperation( operation );
-
-		let end;
+		let end = this.end._getTransformedBySplitOperation( operation );
 
 		if ( this.end.isEqual( operation.insertionPosition ) ) {
 			end = this.end.getShiftedBy( 1 );
-		} else {
-			end = this.end._getTransformedBySplitOperation( operation );
+		}
+
+		// Below may happen when range contains graveyard element used by split operation.
+		if ( start.root != end.root ) {
+			// End position was next to the moved graveyard element and was moved with it.
+			// Fix it by using old `end` which has proper `root`.
+			end = this.end.getShiftedBy( -1 );
 		}
 
 		return new Range( start, end );

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

@@ -174,9 +174,7 @@ describe( 'transform', () => {
 				kate.undo();
 				syncClients();
 
-				expectClients(
-					'<paragraph>Foo</paragraph>'
-				);
+				expectClients( '<paragraph>Foo</paragraph>' );
 			} );
 		} );
 
@@ -222,6 +220,40 @@ describe( 'transform', () => {
 
 				expectClients( '<paragraph>FooBar</paragraph>' );
 			} );
+
+			it( 'remove merged element then undo #3', () => {
+				john.setData( '[<paragraph>A</paragraph><paragraph>B</paragraph>]<paragraph>C</paragraph>' );
+				kate.setData( '<paragraph>A</paragraph>[]<paragraph>B</paragraph><paragraph>C</paragraph>' );
+
+				kate.merge();
+				john.remove();
+
+				syncClients();
+				expectClients( '<paragraph>C</paragraph>' );
+
+				john.undo();
+				kate.undo();
+
+				syncClients();
+				expectClients( '<paragraph>A</paragraph><paragraph>B</paragraph><paragraph>C</paragraph>' );
+			} );
+
+			it( 'remove merged element then undo #4', () => {
+				john.setData( '<paragraph>A</paragraph>[<paragraph>B</paragraph><paragraph>C</paragraph>]' );
+				kate.setData( '<paragraph>A</paragraph>[]<paragraph>B</paragraph><paragraph>C</paragraph>' );
+
+				kate.merge();
+				john.remove();
+
+				syncClients();
+				expectClients( '<paragraph>A</paragraph>' );
+
+				john.undo();
+				kate.undo();
+
+				syncClients();
+				expectClients( '<paragraph>A</paragraph><paragraph>B</paragraph><paragraph>C</paragraph>' );
+			} );
 		} );
 
 		describe( 'by delete', () => {
@@ -409,5 +441,23 @@ describe( 'transform', () => {
 				);
 			} );
 		} );
+
+		describe( 'by split', () => {
+			it( 'merge element which got split (the element is in blockquote) and undo', () => {
+				john.setData( '<paragraph>Foo</paragraph><blockQuote><paragraph>[]Bar</paragraph></blockQuote>' );
+				kate.setData( '<paragraph>Foo</paragraph><blockQuote><paragraph>B[]ar</paragraph></blockQuote>' );
+
+				john._processExecute( 'delete' );
+				kate.split();
+
+				syncClients();
+				expectClients( '<paragraph>FooB</paragraph><paragraph>ar</paragraph>' );
+
+				john.undo();
+
+				syncClients();
+				expectClients( '<paragraph>Foo</paragraph><blockQuote><paragraph>B</paragraph><paragraph>ar</paragraph></blockQuote>' );
+			} );
+		} );
 	} );
 } );

+ 69 - 2
packages/ckeditor5-engine/tests/model/operation/transform/undo.js

@@ -238,7 +238,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
 	} );
 
-	it.skip( 'delete split paragraphs', () => {
+	it( 'delete split paragraphs', () => {
 		john.setData( '<paragraph>Foo</paragraph><paragraph>B[]ar</paragraph>' );
 
 		john.split();
@@ -276,7 +276,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Foo</paragraph>' );
 	} );
 
-	it( 'undo pasting', () => {
+	it( 'pasting on collapsed selection undo and redo', () => {
 		john.setData( '<paragraph>Foo[]Bar</paragraph>' );
 
 		// Below simulates pasting.
@@ -297,8 +297,16 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Foo1</paragraph><paragraph>2Bar</paragraph>' );
 
 		john.undo();
+		expectClients( '<paragraph>FooBar</paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Foo1</paragraph><paragraph>2Bar</paragraph>' );
 
+		john.undo();
 		expectClients( '<paragraph>FooBar</paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Foo1</paragraph><paragraph>2Bar</paragraph>' );
 	} );
 
 	it( 'selection attribute setting: split, bold, merge, undo, undo, undo', () => {
@@ -321,4 +329,63 @@ describe( 'transform', () => {
 		john.undo();
 		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
 	} );
+
+	// https://github.com/ckeditor/ckeditor5/issues/1288
+	it( 'remove two groups of blocks then undo, undo', () => {
+		john.setData(
+			'<paragraph>X</paragraph><paragraph>A</paragraph><paragraph>B[</paragraph><paragraph>C</paragraph><paragraph>D]</paragraph>'
+		);
+
+		john.delete();
+		john.setSelection( [ 0, 1 ], [ 2, 1 ] );
+		john.delete();
+
+		expectClients( '<paragraph>X</paragraph>' );
+
+		john.undo();
+
+		expectClients( '<paragraph>X</paragraph><paragraph>A</paragraph><paragraph>B</paragraph>' );
+
+		john.undo();
+
+		expectClients(
+			'<paragraph>X</paragraph><paragraph>A</paragraph><paragraph>B</paragraph><paragraph>C</paragraph><paragraph>D</paragraph>'
+		);
+	} );
+
+	// https://github.com/ckeditor/ckeditor5/issues/1287 TC1
+	it( 'pasting on non-collapsed selection undo and redo', () => {
+		john.setData( '<paragraph>Fo[o</paragraph><paragraph>B]ar</paragraph>' );
+
+		// Below simulates pasting.
+		john.editor.model.change( () => {
+			john.editor.model.deleteContent( john.document.selection );
+
+			john.setSelection( [ 0, 2 ] );
+			john.split();
+
+			john.setSelection( [ 1 ] );
+			john.insert( '<paragraph>1</paragraph>' );
+
+			john.setSelection( [ 1 ] );
+			john.merge();
+
+			john.setSelection( [ 1 ] );
+			john.insert( '<paragraph>2</paragraph>' );
+
+			john.setSelection( [ 2 ] );
+			john.merge();
+		} );
+
+		expectClients( '<paragraph>Fo1</paragraph><paragraph>2ar</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Fo1</paragraph><paragraph>2ar</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+	} );
 } );