Skip to content

Commit

Permalink
Merge pull request #9003 from ckeditor/i/8976-ot-fix
Browse files Browse the repository at this point in the history
Fix (engine): Undoing the deletion of merged paragraphs should result in the original tree. Closes #8976.

Internal (engine): The `SplitOperation` is now expecting `insertionPosition` as a constructor argument.
  • Loading branch information
scofalik authored Feb 17, 2021
2 parents 801e238 + 571bcef commit ecba70b
Show file tree
Hide file tree
Showing 11 changed files with 196 additions and 69 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,7 @@ export default class MergeOperation extends Operation {
const path = this.sourcePosition.path.slice( 0, -1 );
const insertionPosition = new Position( this.sourcePosition.root, path )._getTransformedByMergeOperation( this );

const split = new SplitOperation( targetPosition, this.howMany, this.graveyardPosition, this.baseVersion + 1 );
split.insertionPosition = insertionPosition;

return split;
return new SplitOperation( targetPosition, this.howMany, insertionPosition, this.graveyardPosition, this.baseVersion + 1 );
}

/**
Expand Down
16 changes: 6 additions & 10 deletions packages/ckeditor5-engine/src/model/operation/splitoperation.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ export default class SplitOperation extends Operation {
*
* @param {module:engine/model/position~Position} splitPosition Position at which an element should be split.
* @param {Number} howMany Total offset size of elements that are in the split element after `position`.
* @param {module:engine/model/position~Position} insertionPosition Position at which the clone of split element
* (or element from graveyard) will be inserted.
* @param {module:engine/model/position~Position|null} graveyardPosition Position in the graveyard root before the element which
* should be used as a parent of the nodes after `position`. If it is not set, a copy of the the `position` parent will be used.
* @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation
* can be applied or `null` if the operation operates on detached (non-document) tree.
*/
constructor( splitPosition, howMany, graveyardPosition, baseVersion ) {
constructor( splitPosition, howMany, insertionPosition, graveyardPosition, baseVersion ) {
super( baseVersion );

/**
Expand All @@ -58,7 +60,7 @@ export default class SplitOperation extends Operation {
*
* @member {module:engine/model/position~Position} module:engine/model/operation/splitoperation~SplitOperation#insertionPosition
*/
this.insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
this.insertionPosition = insertionPosition;

/**
* Position in the graveyard root before the element which should be used as a parent of the nodes after `position`.
Expand Down Expand Up @@ -116,10 +118,7 @@ export default class SplitOperation extends Operation {
* @returns {module:engine/model/operation/splitoperation~SplitOperation} Clone of this operation.
*/
clone() {
const split = new this.constructor( this.splitPosition, this.howMany, this.graveyardPosition, this.baseVersion );
split.insertionPosition = this.insertionPosition;

return split;
return new this.constructor( this.splitPosition, this.howMany, this.insertionPosition, this.graveyardPosition, this.baseVersion );
}

/**
Expand Down Expand Up @@ -244,10 +243,7 @@ export default class SplitOperation extends Operation {
const insertionPosition = Position.fromJSON( json.insertionPosition, document );
const graveyardPosition = json.graveyardPosition ? Position.fromJSON( json.graveyardPosition, document ) : null;

const split = new this( splitPosition, json.howMany, graveyardPosition, json.baseVersion );
split.insertionPosition = insertionPosition;

return split;
return new this( splitPosition, json.howMany, insertionPosition, graveyardPosition, json.baseVersion );
}

// @if CK_DEBUG_ENGINE // toString() {
Expand Down
57 changes: 36 additions & 21 deletions packages/ckeditor5-engine/src/model/operation/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -497,9 +497,16 @@ class ContextFactory {
case MoveOperation: {
if ( opA.splitPosition.isEqual( opB.sourcePosition ) || opA.splitPosition.isBefore( opB.sourcePosition ) ) {
this._setRelation( opA, opB, 'splitBefore' );
}
} else {
const range = Range._createFromPositionAndShift( opB.sourcePosition, opB.howMany );

break;
if ( opA.splitPosition.hasSameParentAs( opB.sourcePosition ) && range.containsPosition( opA.splitPosition ) ) {
const howMany = range.end.offset - opA.splitPosition.offset;
const offset = opA.splitPosition.offset - range.start.offset;

this._setRelation( opA, opB, { howMany, offset } );
}
}
}
}

Expand Down Expand Up @@ -2046,8 +2053,7 @@ setTransformation( SplitOperation, MergeOperation, ( a, b, context ) => {
const splitPosition = new Position( b.graveyardPosition.root, splitPath );
const insertionPosition = SplitOperation.getInsertionPosition( new Position( b.graveyardPosition.root, splitPath ) );

const additionalSplit = new SplitOperation( splitPosition, 0, null, 0 );
additionalSplit.insertionPosition = insertionPosition;
const additionalSplit = new SplitOperation( splitPosition, 0, insertionPosition, null, 0 );

a.splitPosition = a.splitPosition._getTransformedByMergeOperation( b );
a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );
Expand Down Expand Up @@ -2107,6 +2113,32 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {

// Case 2:
//
// Split is at a position where nodes were moved.
//
// This is a scenario described in `MoveOperation` x `SplitOperation` transformation but from the
// "split operation point of view".
//
const splitAtTarget = a.splitPosition.isEqual( b.targetPosition );

if ( splitAtTarget && ( context.baRelation == 'insertAtSource' || context.abRelation == 'splitBefore' ) ) {
a.howMany += b.howMany;
a.splitPosition = a.splitPosition._getTransformedByDeletion( b.sourcePosition, b.howMany );
a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );

return [ a ];
}

if ( splitAtTarget && context.abRelation && context.abRelation.howMany ) {
const { howMany, offset } = context.abRelation;

a.howMany += howMany;
a.splitPosition = a.splitPosition.getShiftedBy( offset );

return [ a ];
}

// Case 3:
//
// 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.
//
Expand Down Expand Up @@ -2136,23 +2168,6 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
return [ a ];
}

// Case 3:
//
// Split is at a position where nodes were moved.
//
// This is a scenario described in `MoveOperation` x `SplitOperation` transformation but from the
// "split operation point of view".
//
const splitAtTarget = a.splitPosition.isEqual( b.targetPosition );

if ( splitAtTarget && ( context.baRelation == 'insertAtSource' || context.abRelation == 'splitBefore' ) ) {
a.howMany += b.howMany;
a.splitPosition = a.splitPosition._getTransformedByDeletion( b.sourcePosition, b.howMany );
a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );

return [ a ];
}

// The default case.
// Don't change `howMany` if move operation does not really move anything.
//
Expand Down
4 changes: 3 additions & 1 deletion packages/ckeditor5-engine/src/model/writer.js
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,9 @@ export default class Writer {
do {
const version = splitElement.root.document ? splitElement.root.document.version : null;
const howMany = splitElement.maxOffset - position.offset;
const split = new SplitOperation( position, howMany, null, version );

const insertionPosition = SplitOperation.getInsertionPosition( position );
const split = new SplitOperation( position, howMany, insertionPosition, null, version );

this.batch.addOperation( split );
this.model.applyOperation( split );
Expand Down
7 changes: 5 additions & 2 deletions packages/ckeditor5-engine/tests/model/differ.js
Original file line number Diff line number Diff line change
Expand Up @@ -1407,7 +1407,9 @@ describe( 'Differ', () => {

model.change( () => {
const position = new Position( root, [ 0, 3 ] );
const operation = new SplitOperation( position, 3, new Position( doc.graveyard, [ 0 ] ), doc.version );
const insertionPosition = SplitOperation.getInsertionPosition( position );

const operation = new SplitOperation( position, 3, insertionPosition, new Position( doc.graveyard, [ 0 ] ), doc.version );

model.applyOperation( operation );

Expand Down Expand Up @@ -1991,7 +1993,8 @@ describe( 'Differ', () => {

function split( position ) {
const howMany = position.parent.maxOffset - position.offset;
const operation = new SplitOperation( position, howMany, null, doc.version );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const operation = new SplitOperation( position, howMany, insertionPosition, null, doc.version );

model.applyOperation( operation );
}
Expand Down
5 changes: 4 additions & 1 deletion packages/ckeditor5-engine/tests/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -1808,7 +1808,10 @@ describe( 'DocumentSelection', () => {
} );

const batch = new Batch();
const splitOperation = new SplitOperation( new Position( root, [ 1, 2 ] ), 4, null, 0 );
const splitPosition = new Position( root, [ 1, 2 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const splitOperation = new SplitOperation( splitPosition, 4, insertionPosition, null, 0 );

batch.addOperation( splitOperation );
model.applyOperation( splitOperation );
Expand Down
78 changes: 60 additions & 18 deletions packages/ckeditor5-engine/tests/model/operation/splitoperation.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,37 @@ describe( 'SplitOperation', () => {
} );

it( 'should have proper type', () => {
const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
const splitPosition = new Position( root, [ 1, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );

expect( split.type ).to.equal( 'split' );
} );

it( 'should have proper insertionPosition', () => {
const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
const splitPosition = new Position( root, [ 1, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );

expect( split.insertionPosition.path ).to.deep.equal( [ 2 ] );
} );

it( 'should have proper moveTargetPosition', () => {
const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
const splitPosition = new Position( root, [ 1, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );

expect( split.moveTargetPosition.path ).to.deep.equal( [ 2, 0 ] );
} );

it( 'should have proper movedRange', () => {
const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
const splitPosition = new Position( root, [ 1, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );

expect( split.movedRange.start.path ).to.deep.equal( [ 1, 3 ] );
expect( split.movedRange.end.path ).to.deep.equal( [ 1, Number.POSITIVE_INFINITY ] );
Expand All @@ -53,7 +65,10 @@ describe( 'SplitOperation', () => {

root._insertChild( 0, [ p1 ] );

model.applyOperation( new SplitOperation( new Position( root, [ 0, 3 ] ), 3, null, doc.version ) );
const splitPosition = new Position( root, [ 0, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

model.applyOperation( new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version ) );

expect( doc.version ).to.equal( 1 );
expect( root.maxOffset ).to.equal( 2 );
Expand All @@ -74,7 +89,10 @@ describe( 'SplitOperation', () => {
root._insertChild( 0, [ p1 ] );
gy._insertChild( 0, [ p2 ] );

model.applyOperation( new SplitOperation( new Position( root, [ 0, 3 ] ), 3, gyPos, doc.version ) );
const splitPosition = new Position( root, [ 0, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

model.applyOperation( new SplitOperation( splitPosition, 3, insertionPosition, gyPos, doc.version ) );

expect( doc.version ).to.equal( 1 );
expect( root.maxOffset ).to.equal( 2 );
Expand All @@ -91,7 +109,10 @@ describe( 'SplitOperation', () => {
} );

it( 'should create a proper MergeOperation as a reverse', () => {
const operation = new SplitOperation( new Position( root, [ 1, 3 ] ), 3, null, doc.version );
const splitPosition = new Position( root, [ 1, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );
const reverse = operation.getReversed();

expect( reverse ).to.be.an.instanceof( MergeOperation );
Expand All @@ -107,7 +128,10 @@ describe( 'SplitOperation', () => {

root._insertChild( 0, [ p1 ] );

const operation = new SplitOperation( new Position( root, [ 0, 3 ] ), 3, null, doc.version );
const splitPosition = new Position( root, [ 0, 3 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );

model.applyOperation( operation );
model.applyOperation( operation.getReversed() );
Expand All @@ -124,7 +148,10 @@ describe( 'SplitOperation', () => {

root._insertChild( 0, [ p1 ] );

const operation = new SplitOperation( new Position( root, [ 0, 8 ] ), 3, null, doc.version );
const splitPosition = new Position( root, [ 0, 8 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );

expectToThrowCKEditorError( () => operation._validate(), /split-operation-position-invalid/, model );
} );
Expand All @@ -134,7 +161,10 @@ describe( 'SplitOperation', () => {

root._insertChild( 0, [ p1 ] );

const operation = new SplitOperation( new Position( root, [ 0, 0 ] ), 3, null, doc.version );
const splitPosition = new Position( root, [ 0, 0 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );
operation.splitPosition = new Position( root, [ 1 ] );

expectToThrowCKEditorError( () => operation._validate(), /split-operation-split-in-root/, model );
Expand All @@ -145,7 +175,10 @@ describe( 'SplitOperation', () => {

root._insertChild( 0, [ p1 ] );

const operation = new SplitOperation( new Position( root, [ 0, 2 ] ), 6, null, doc.version );
const splitPosition = new Position( root, [ 0, 2 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const operation = new SplitOperation( splitPosition, 6, insertionPosition, null, doc.version );

expectToThrowCKEditorError( () => operation._validate(), /split-operation-how-many-invalid/, model );
} );
Expand All @@ -155,18 +188,22 @@ describe( 'SplitOperation', () => {

root._insertChild( 0, [ p1 ] );

const operation = new SplitOperation( new Position( root, [ 0, 2 ] ), 4, gyPos, doc.version );
const splitPosition = new Position( root, [ 0, 2 ] );
const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );

const operation = new SplitOperation( splitPosition, 4, insertionPosition, gyPos, doc.version );

expectToThrowCKEditorError( () => operation._validate(), /split-operation-graveyard-position-invalid/, model );
} );
} );

it( 'should create SplitOperation with the same parameters when cloned #1', () => {
const position = new Position( root, [ 1, 2 ] );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const howMany = 4;
const baseVersion = doc.version;

const op = new SplitOperation( position, howMany, null, baseVersion );
const op = new SplitOperation( position, howMany, insertionPosition, null, baseVersion );

const clone = op.clone();

Expand All @@ -183,10 +220,11 @@ describe( 'SplitOperation', () => {

it( 'should create SplitOperation with the same parameters when cloned #2', () => {
const position = new Position( root, [ 1, 2 ] );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const howMany = 4;
const baseVersion = doc.version;

const op = new SplitOperation( position, howMany, gyPos, baseVersion );
const op = new SplitOperation( position, howMany, insertionPosition, gyPos, baseVersion );

const clone = op.clone();

Expand All @@ -204,7 +242,8 @@ describe( 'SplitOperation', () => {
describe( 'toJSON', () => {
it( 'should create proper json object #1', () => {
const position = new Position( root, [ 0, 3 ] );
const op = new SplitOperation( position, 2, null, doc.version );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const op = new SplitOperation( position, 2, insertionPosition, null, doc.version );

const serialized = op.toJSON();

Expand All @@ -220,7 +259,8 @@ describe( 'SplitOperation', () => {

it( 'should create proper json object #2', () => {
const position = new Position( root, [ 0, 3 ] );
const op = new SplitOperation( position, 2, gyPos, doc.version );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const op = new SplitOperation( position, 2, insertionPosition, gyPos, doc.version );

const serialized = op.toJSON();

Expand All @@ -238,7 +278,8 @@ describe( 'SplitOperation', () => {
describe( 'fromJSON', () => {
it( 'should create proper SplitOperation from json object #1', () => {
const position = new Position( root, [ 0, 3 ] );
const op = new SplitOperation( position, 2, null, doc.version );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const op = new SplitOperation( position, 2, insertionPosition, null, doc.version );

const serialized = op.toJSON();

Expand All @@ -249,7 +290,8 @@ describe( 'SplitOperation', () => {

it( 'should create proper SplitOperation from json object #2', () => {
const position = new Position( root, [ 0, 3 ] );
const op = new SplitOperation( position, 2, gyPos, doc.version );
const insertionPosition = SplitOperation.getInsertionPosition( position );
const op = new SplitOperation( position, 2, insertionPosition, gyPos, doc.version );

const serialized = op.toJSON();

Expand Down
Loading

0 comments on commit ecba70b

Please sign in to comment.