From 3c0244e2746e3f85526f57d3b44ccaeccfd8bd6a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 9 Oct 2018 16:36:31 +0200 Subject: [PATCH 1/4] Docs: Removed old docs entry. --- src/model/model.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/model/model.js b/src/model/model.js index 8e8b747a3..f6532d549 100644 --- a/src/model/model.js +++ b/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. * From 52e9f0813ea194941f0e9fe67cac08bdbfcadb63 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 9 Oct 2018 16:37:08 +0200 Subject: [PATCH 2/4] Fix: Corrected `MergeOperation` x `MergeOperation` transformation for pasting case. --- src/model/operation/transform.js | 13 +++++++++-- tests/model/operation/transform/undo.js | 31 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 02a4992fe..0b2c77a3a 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -505,6 +505,12 @@ class ContextFactory { break; } + + case SplitOperation: { + if ( opA.sourcePosition.isEqual( opB.splitPosition ) ) { + this._setRelation( opA, opB, 'splitAtSource' ); + } + } } break; @@ -1159,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'; @@ -1338,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 ); diff --git a/tests/model/operation/transform/undo.js b/tests/model/operation/transform/undo.js index 0ebe051a9..867fb1f36 100644 --- a/tests/model/operation/transform/undo.js +++ b/tests/model/operation/transform/undo.js @@ -344,4 +344,35 @@ describe( 'transform', () => { 'XABCD' ); } ); + + // https://github.com/ckeditor/ckeditor5/issues/1287 TC1 + it( 'undo and redo pasting', () => { + john.setData( 'Fo[oB]ar' ); + + // 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( '1' ); + + john.setSelection( [ 1 ] ); + john.merge(); + + john.setSelection( [ 1 ] ); + john.insert( '2' ); + + john.setSelection( [ 2 ] ); + john.merge(); + } ); + + expectClients( 'Fo12ar' ); + + john.undo(); + + expectClients( 'FooBar' ); + } ); } ); From 260a22f09065310eb1d72bdd68d613167f35a39d Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 11 Oct 2018 16:06:02 +0200 Subject: [PATCH 3/4] Fix: Corrected transformation in pasting scenarios. --- src/model/operation/transform.js | 47 ++++++++++++++++++------- tests/model/operation/transform/undo.js | 16 +++++++-- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 0b2c77a3a..c98a84f2d 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1405,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; } @@ -1692,7 +1692,15 @@ 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 gyMove = new MoveOperation( b.graveyardPosition, 1, a.targetPosition.getShiftedBy( -1 ), 0 ); + + const targetPositionPath = gyMove.getMovedRangeStart().path.slice(); + targetPositionPath.push( 0 ); + + return [ + gyMove, + new MoveOperation( b.targetPosition, b.howMany, new Position( a.targetPosition.root, targetPositionPath ), 0 ) + ]; } } else { // Case 2: @@ -1920,11 +1928,25 @@ 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 and the split insertion position + // should be corrected. + // + if ( rangeToMove.containsPosition( a.graveyardPosition ) || rangeToMove.start.isEqual( a.graveyardPosition ) ) { + a.insertionPosition = Position.createFromPosition( b.targetPosition ); + a.splitPosition = a.splitPosition._getTransformedByMoveOperation( b ); + + return [ a ]; + } + 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. @@ -1941,8 +1963,6 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { // After split: // AdXbcyz // - 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; @@ -1957,7 +1977,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { return [ a ]; } - // Case 2: + // Case 3: // // Split is at a position where nodes were moved. // @@ -1975,13 +1995,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. diff --git a/tests/model/operation/transform/undo.js b/tests/model/operation/transform/undo.js index 867fb1f36..c8f054cc1 100644 --- a/tests/model/operation/transform/undo.js +++ b/tests/model/operation/transform/undo.js @@ -276,7 +276,7 @@ describe( 'transform', () => { expectClients( 'Foo' ); } ); - it( 'undo pasting', () => { + it( 'pasting on collapsed selection undo and redo', () => { john.setData( 'Foo[]Bar' ); // Below simulates pasting. @@ -297,8 +297,16 @@ describe( 'transform', () => { expectClients( 'Foo12Bar' ); john.undo(); + expectClients( 'FooBar' ); + + john.redo(); + expectClients( 'Foo12Bar' ); + john.undo(); expectClients( 'FooBar' ); + + john.redo(); + expectClients( 'Foo12Bar' ); } ); it( 'selection attribute setting: split, bold, merge, undo, undo, undo', () => { @@ -346,7 +354,7 @@ describe( 'transform', () => { } ); // https://github.com/ckeditor/ckeditor5/issues/1287 TC1 - it( 'undo and redo pasting', () => { + it( 'pasting on non-collapsed selection undo and redo', () => { john.setData( 'Fo[oB]ar' ); // Below simulates pasting. @@ -372,7 +380,9 @@ describe( 'transform', () => { expectClients( 'Fo12ar' ); john.undo(); - expectClients( 'FooBar' ); + + john.redo(); + expectClients( 'Fo12ar' ); } ); } ); From 6a37874760bdbb9f7970468ae34dccdc253e0026 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 11 Oct 2018 17:01:04 +0200 Subject: [PATCH 4/4] Fix: Corrected transformations for pasting and undo scenarios. --- src/model/operation/transform.js | 5 +++-- tests/model/operation/transform/undo.js | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index c98a84f2d..51852430b 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1692,9 +1692,10 @@ 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 ) { - const gyMove = new MoveOperation( b.graveyardPosition, 1, a.targetPosition.getShiftedBy( -1 ), 0 ); + const gyMoveTarget = Position.createFromPosition( b.graveyardPosition ); + const gyMove = new MoveOperation( b.graveyardPosition, 1, gyMoveTarget, 0 ); - const targetPositionPath = gyMove.getMovedRangeStart().path.slice(); + const targetPositionPath = b.graveyardPosition.path.slice(); targetPositionPath.push( 0 ); return [ diff --git a/tests/model/operation/transform/undo.js b/tests/model/operation/transform/undo.js index c8f054cc1..63b328337 100644 --- a/tests/model/operation/transform/undo.js +++ b/tests/model/operation/transform/undo.js @@ -384,5 +384,8 @@ describe( 'transform', () => { john.redo(); expectClients( 'Fo12ar' ); + + john.undo(); + expectClients( 'FooBar' ); } ); } );