Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OT fix for transforming split by move #9003

Merged
merged 12 commits into from
Feb 17, 2021
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 @@ -2105,6 +2111,32 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b );
}

// 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 ];
}

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

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

return [ a ];
}

// Case 2:
//
// If the split position is inside the moved range, we need to shift the split position to a proper place.
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