From 532c318906665314eec98a40c29453030e86e680 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 2 Aug 2018 12:31:46 +0200 Subject: [PATCH 01/14] Changed: Introduced `MergeOperation#howMany` and `SplitOperation#howMany`. --- src/model/operation/mergeoperation.js | 18 ++++-- src/model/operation/splitoperation.js | 18 ++++-- src/model/operation/transform.js | 87 ++++++++++++++++++++++++--- src/model/writer.js | 5 +- tests/model/documentselection.js | 2 +- tests/model/liverange.js | 1 + tests/model/range.js | 13 ++-- 7 files changed, 119 insertions(+), 25 deletions(-) diff --git a/src/model/operation/mergeoperation.js b/src/model/operation/mergeoperation.js index a8c0a25be..5b0b148f9 100644 --- a/src/model/operation/mergeoperation.js +++ b/src/model/operation/mergeoperation.js @@ -30,12 +30,13 @@ export default class MergeOperation extends Operation { * * @param {module:engine/model/position~Position} sourcePosition Position inside the merged element. All nodes from that * element after that position will be moved to {@link ~#targetPosition}. + * @param {Number} howMany * @param {module:engine/model/position~Position} targetPosition Position which the nodes from the merged elements will be moved to. * @param {module:engine/model/position~Position} graveyardPosition Position in graveyard to which the merged element will be moved. * @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( sourcePosition, targetPosition, graveyardPosition, baseVersion ) { + constructor( sourcePosition, howMany, targetPosition, graveyardPosition, baseVersion ) { super( baseVersion ); /** @@ -56,6 +57,8 @@ export default class MergeOperation extends Operation { // is it? think about reversed split operations, undo, etc. this.graveyardPosition = Position.createFromPosition( graveyardPosition ); + + this.howMany = howMany; } /** @@ -94,7 +97,7 @@ export default class MergeOperation extends Operation { * @returns {module:engine/model/operation/mergeoperation~MergeOperation} Clone of this operation. */ clone() { - return new this.constructor( this.sourcePosition, this.targetPosition, this.graveyardPosition, this.baseVersion ); + return new this.constructor( this.sourcePosition, this.howMany, this.targetPosition, this.graveyardPosition, this.baseVersion ); } /** @@ -103,7 +106,7 @@ export default class MergeOperation extends Operation { * @returns {module:engine/model/operation/splitoperation~SplitOperation} */ getReversed() { - return new SplitOperation( this.targetPosition, this.graveyardPosition, this.baseVersion + 1 ); + return new SplitOperation( this.targetPosition, this.howMany, this.graveyardPosition, this.baseVersion + 1 ); } /** @@ -128,6 +131,13 @@ export default class MergeOperation extends Operation { * @error merge-operation-target-position-invalid */ throw new CKEditorError( 'merge-operation-target-position-invalid: Merge target position is invalid.' ); + } else if ( this.howMany != sourceElement.maxOffset ) { + /** + * Merge operation specifies wrong number of nodes to move. + * + * @error merge-operation-how-many-invalid + */ + throw new CKEditorError( 'merge-operation-how-many-invalid: Merge operation specifies wrong number of nodes to move.' ); } } @@ -161,6 +171,6 @@ export default class MergeOperation extends Operation { const targetPosition = Position.fromJSON( json.targetPosition, document ); const graveyardPosition = Position.fromJSON( json.graveyardPosition, document ); - return new this( sourcePosition, targetPosition, graveyardPosition, json.baseVersion ); + return new this( sourcePosition, json.howMany, targetPosition, graveyardPosition, json.baseVersion ); } } diff --git a/src/model/operation/splitoperation.js b/src/model/operation/splitoperation.js index 4ed0267c1..4050a3426 100644 --- a/src/model/operation/splitoperation.js +++ b/src/model/operation/splitoperation.js @@ -26,12 +26,13 @@ export default class SplitOperation extends Operation { * Creates a split operation. * * @param {module:engine/model/position~Position} position Position at which an element should be split. + * @param {Number} * @param {module:engine/model/position~Position|null} graveyardPosition Position in graveyard 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( position, graveyardPosition, baseVersion ) { + constructor( position, howMany, graveyardPosition, baseVersion ) { super( baseVersion ); /** @@ -47,6 +48,8 @@ export default class SplitOperation extends Operation { if ( this.graveyardPosition ) { this.graveyardPosition.stickiness = 'toNext'; } + + this.howMany = howMany; } /** @@ -104,7 +107,7 @@ export default class SplitOperation extends Operation { * @returns {module:engine/model/operation/splitoperation~SplitOperation} Clone of this operation. */ clone() { - return new this.constructor( this.position, this.graveyardPosition, this.baseVersion ); + return new this.constructor( this.position, this.howMany, this.graveyardPosition, this.baseVersion ); } /** @@ -116,7 +119,7 @@ export default class SplitOperation extends Operation { const graveyard = this.position.root.document.graveyard; const graveyardPosition = new Position( graveyard, [ 0 ] ); - return new MergeOperation( this.moveTargetPosition, this.position, graveyardPosition, this.baseVersion + 1 ); + return new MergeOperation( this.moveTargetPosition, this.howMany, this.position, graveyardPosition, this.baseVersion + 1 ); } /** @@ -134,6 +137,13 @@ export default class SplitOperation extends Operation { * @error split-operation-position-invalid */ throw new CKEditorError( 'split-operation-position-invalid: Split position is invalid.' ); + } else if ( this.howMany != element.maxOffset - this.position.offset ) { + /** + * Split operation specifies wrong number of nodes to move. + * + * @error split-operation-how-many-invalid + */ + throw new CKEditorError( 'split-operation-how-many-invalid: Split operation specifies wrong number of nodes to move.' ); } } @@ -174,6 +184,6 @@ export default class SplitOperation extends Operation { const position = Position.fromJSON( json.position, document ); const graveyardPosition = json.graveyardPosition ? Position.fromJSON( json.graveyardPosition, document ) : null; - return new this( position, graveyardPosition, json.baseVersion ); + return new this( position, json.howMany, graveyardPosition, json.baseVersion ); } } diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index bca951da0..60d3029d6 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -839,6 +839,10 @@ setTransformation( MarkerOperation, UnwrapOperation, ( a, b ) => { // ----------------------- setTransformation( MergeOperation, InsertOperation, ( a, b ) => { + if ( a.sourcePosition.hasSameParentAs( b.position ) ) { + a.howMany += b.howMany; + } + a.sourcePosition = a.sourcePosition._getTransformedByInsertOperation( b ); a.targetPosition = a.targetPosition._getTransformedByInsertOperation( b ); @@ -855,6 +859,7 @@ setTransformation( MergeOperation, MergeOperation, ( a, b, context ) => { path.push( 0 ); a.sourcePosition = new Position( b.graveyardPosition.root, path ); + a.howMany = 0; return [ a ]; } @@ -864,6 +869,10 @@ setTransformation( MergeOperation, MergeOperation, ( a, b, context ) => { // The default case. // + if ( a.sourcePosition.hasSameParentAs( b.targetPosition ) ) { + a.howMany += b.howMany; + } + a.sourcePosition = a.sourcePosition._getTransformedByMergeOperation( b ); a.targetPosition = a.targetPosition._getTransformedByMergeOperation( b ); @@ -892,6 +901,14 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { } } + if ( a.sourcePosition.hasSameParentAs( b.targetPosition ) ) { + a.howMany += b.howMany; + } + + if ( a.sourcePosition.hasSameParentAs( b.sourcePosition ) ) { + a.howMany -= b.howMany; + } + a.sourcePosition = a.sourcePosition._getTransformedByMoveOperation( b ); a.targetPosition = a.targetPosition._getTransformedByMoveOperation( b ); @@ -922,16 +939,17 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => { // For example, if the split would be after `F`, `targetPosition` should also be transformed. // // There is an exception though. It is when merge operation targets into inside of an element. - // Such merge operation can be a result of merge x merge transformation, when merges are identical. - // Such merge operation's source position is in graveyard and we will use that to recognize it - // (although a more precise method would be more correct). // - if ( a.targetPosition.isEqual( b.position ) && a.sourcePosition.root.rootName == '$graveyard' ) { + if ( a.targetPosition.isEqual( b.position ) && b.howMany != 0 ) { a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); return [ a ]; } + if ( a.sourcePosition.hasSameParentAs( b.position ) ) { + a.howMany = b.position.offset; + } + a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b ); @@ -943,6 +961,10 @@ setTransformation( MergeOperation, WrapOperation, ( a, b ) => { a.graveyardPosition = a.graveyardPosition._getTransformedByDeletion( b.graveyardPosition, 1 ); } + if ( a.sourcePosition.hasSameParentAs( b.position ) ) { + a.howMany = a.howMany + 1 - b.howMany; + } + // Case 1: Wrap is with an element from graveyard, which also is merge operation target. This happens // in some undo scenarios. In this case, the target position needs to be properly transformed. // @@ -995,10 +1017,15 @@ setTransformation( MergeOperation, UnwrapOperation, ( a, b, context ) => { path.push( 0 ); a.sourcePosition = new Position( b.graveyardPosition.root, path ); + a.howMany = 0; return [ a ]; } + if ( a.sourcePosition.hasSameParentAs( b.targetPosition ) ) { + a.howMany = a.howMany - 1 + b.howMany; + } + a.sourcePosition = a.sourcePosition._getTransformedByUnwrapOperation( b ); a.targetPosition = a.targetPosition._getTransformedByUnwrapOperation( b ); @@ -1466,15 +1493,25 @@ setTransformation( RootAttributeOperation, RootAttributeOperation, ( a, b, conte setTransformation( SplitOperation, InsertOperation, ( a, b, context ) => { if ( a.position.isEqual( b.position ) && context.baRelation == 'insertAtSource' ) { + a.howMany += b.howMany; + return [ a ]; } + if ( a.position.hasSameParentAs( b.position ) && a.position.offset < b.position.offset ) { + a.howMany += b.howMany; + } + a.position = a.position._getTransformedByInsertOperation( b ); return [ a ]; } ); setTransformation( SplitOperation, MergeOperation, ( a, b ) => { + if ( a.position.hasSameParentAs( b.targetPosition ) ) { + a.howMany += b.howMany; + } + a.position = a.position._getTransformedByMergeOperation( b ); if ( a.graveyardPosition ) { @@ -1507,12 +1544,20 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { const rangeToMove = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); if ( a.position.hasSameParentAs( b.sourcePosition ) && rangeToMove.containsPosition( a.position ) ) { + const howManyRemoved = b.howMany - ( a.position.offset - b.sourcePosition.offset ); + a.howMany -= howManyRemoved; + + if ( a.position.hasSameParentAs( b.targetPosition ) && a.position.offset < b.targetPosition.offset ) { + a.howMany += b.howMany; + } + a.position = Position.createFromPosition( b.sourcePosition ); return [ a ]; } if ( a.position.isEqual( b.targetPosition ) && context.abRelation == 'splitBefore' ) { + a.howMany += b.howMany; a.position = a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); return [ a ]; @@ -1520,6 +1565,14 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { // The default case. // + if ( a.position.hasSameParentAs( b.sourcePosition ) && a.position.offset < b.sourcePosition.offset ) { + a.howMany -= b.howMany; + } + + if ( a.position.hasSameParentAs( b.targetPosition ) && a.position.offset < b.sourcePosition.offset ) { + a.howMany += b.howMany; + } + a.position = a.position._getTransformedByMoveOperation( b ); return [ a ]; @@ -1534,9 +1587,15 @@ setTransformation( SplitOperation, SplitOperation, ( a, b, context ) => { if ( a.graveyardPosition && b.graveyardPosition && a.graveyardPosition.isEqual( b.graveyardPosition ) ) { return getNoOp(); } + + a.howMany = 0; } else if ( a.position.isEqual( b.insertionPosition ) && context.abRelation == 'splitBefore' ) { return [ a ]; } else { + if ( a.position.hasSameParentAs( b.position ) && a.position.offset < b.position.offset ) { + a.howMany -= b.howMany; + } + a.position = a.position._getTransformedBySplitOperation( b ); } @@ -1567,7 +1626,12 @@ setTransformation( SplitOperation, WrapOperation, ( a, b, context ) => { if ( a.position.isEqual( b.position ) && context.abRelation == 'splitInside' ) { a.position = b.targetPosition; + a.howMany = b.howMany; } else { + if ( a.position.hasSameParentAs( b.position ) && a.position.offset < b.position.offset ) { + a.howMany = a.howMany + 1 - b.howMany; + } + a.position = a.position._getTransformedByWrapOperation( b ); } @@ -1586,7 +1650,12 @@ setTransformation( SplitOperation, UnwrapOperation, ( a, b, context ) => { path.push( 0 ); a.position = new Position( b.graveyardPosition.root, path ); + a.howMany = 0; } else { + if ( a.position.hasSameParentAs( b.targetPosition ) && a.position.offset < b.targetPosition.offset ) { + a.howMany = a.howMany - 1 + b.howMany; + } + a.position = a.position._getTransformedByUnwrapOperation( b ); } @@ -1884,11 +1953,11 @@ setTransformation( UnwrapOperation, MergeOperation, ( a, b, context ) => { return [ b.getReversed(), a ]; } - // // Case 2: The element to unwrap was merged-to and has new nodes. - // // - // if ( a.position.hasSameParentAs( b.targetPosition ) ) { - // // Merge operation needs `howMany`! - // } + // Case 2: The element to unwrap was merged-to and has new nodes. + // + if ( a.position.hasSameParentAs( b.targetPosition ) ) { + a.howMany += b.howMany; + } if ( a.position.hasSameParentAs( b.graveyardPosition ) ) { a.howMany++; diff --git a/src/model/writer.js b/src/model/writer.js index 8b6e00043..35f1381e9 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -567,7 +567,7 @@ export default class Writer { const version = position.root.document.version; - const merge = new MergeOperation( sourcePosition, targetPosition, graveyardPosition, version ); + const merge = new MergeOperation( sourcePosition, position.nodeAfter.maxOffset, targetPosition, graveyardPosition, version ); this.batch.addOperation( merge ); this.model.applyOperation( merge ); @@ -644,7 +644,8 @@ export default class Writer { do { const version = splitElement.root.document ? splitElement.root.document.version : null; - const split = new SplitOperation( position, null, version ); + const howMany = splitElement.maxOffset - position.offset; + const split = new SplitOperation( position, howMany, null, version ); this.batch.addOperation( split ); this.model.applyOperation( split ); diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index a81f594a0..a77ada163 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -1028,7 +1028,7 @@ describe( 'DocumentSelection', () => { } ); const batch = new Batch(); - const splitOperation = new SplitOperation( new Position( root, [ 1, 2 ] ), null, 0 ); + const splitOperation = new SplitOperation( new Position( root, [ 1, 2 ] ), 4, null, 0 ); batch.addOperation( splitOperation ); model.applyOperation( splitOperation ); diff --git a/tests/model/liverange.js b/tests/model/liverange.js index dd74cf780..0ee6f0513 100644 --- a/tests/model/liverange.js +++ b/tests/model/liverange.js @@ -178,6 +178,7 @@ describe( 'LiveRange', () => { const merge = new MergeOperation( new Position( root, [ 0, 0 ] ), + 10, new Position( gy, [ 0, 0 ] ), new Position( gy, [ 0 ] ), model.document.version + 1 diff --git a/tests/model/range.js b/tests/model/range.js index 2f7bfbcac..074f1c5a4 100644 --- a/tests/model/range.js +++ b/tests/model/range.js @@ -944,7 +944,7 @@ describe( 'Range', () => { it( 'split inside range', () => { range = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 0, 4 ] ) ); - const op = new SplitOperation( new Position( root, [ 0, 3 ] ), gyPos, 1 ); + const op = new SplitOperation( new Position( root, [ 0, 3 ] ), 6, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); expect( transformed.length ).to.equal( 1 ); @@ -955,7 +955,7 @@ describe( 'Range', () => { it( 'split at the beginning of multi-element range', () => { range = new Range( new Position( root, [ 0, 4 ] ), new Position( root, [ 1, 2 ] ) ); - const op = new SplitOperation( new Position( root, [ 0, 4 ] ), gyPos, 1 ); + const op = new SplitOperation( new Position( root, [ 0, 4 ] ), 5, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); expect( transformed.length ).to.equal( 1 ); @@ -966,7 +966,7 @@ describe( 'Range', () => { it( 'split inside range which starts at the beginning of split element', () => { range = new Range( new Position( root, [ 0, 0 ] ), new Position( root, [ 0, 4 ] ) ); - const op = new SplitOperation( new Position( root, [ 0, 3 ] ), gyPos, 1 ); + const op = new SplitOperation( new Position( root, [ 0, 3 ] ), 6, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); expect( transformed.length ).to.equal( 1 ); @@ -977,7 +977,7 @@ describe( 'Range', () => { it( 'split inside range which end is at the end of split element', () => { range = new Range( new Position( root, [ 0, 3 ] ), new Position( root, [ 0, 6 ] ) ); - const op = new SplitOperation( new Position( root, [ 0, 4 ] ), gyPos, 1 ); + const op = new SplitOperation( new Position( root, [ 0, 4 ] ), 5, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); expect( transformed.length ).to.equal( 1 ); @@ -988,7 +988,7 @@ describe( 'Range', () => { it( 'split element which has collapsed range at the end', () => { range = new Range( new Position( root, [ 0, 6 ] ), new Position( root, [ 0, 6 ] ) ); - const op = new SplitOperation( new Position( root, [ 0, 3 ] ), gyPos, 1 ); + const op = new SplitOperation( new Position( root, [ 0, 3 ] ), 6, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); expect( transformed.length ).to.equal( 1 ); @@ -1004,6 +1004,7 @@ describe( 'Range', () => { const op = new MergeOperation( new Position( root, [ 1, 0 ] ), + 4, new Position( root, [ 0, 3 ] ), gyPos, 1 @@ -1022,6 +1023,7 @@ describe( 'Range', () => { const op = new MergeOperation( new Position( root, [ 0, 0 ] ), + 4, new Position( root, [ 1, 3 ] ), gyPos, 1 @@ -1045,6 +1047,7 @@ describe( 'Range', () => { const op = new MergeOperation( new Position( root, [ 1, 0 ] ), + 4, new Position( root, [ 0, 1 ] ), gyPos, 1 From 6aee22650aeb13bee7be6a5376addc0d5d0ae632 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 3 Aug 2018 12:21:26 +0200 Subject: [PATCH 02/14] Fixed: Improvements in OT. --- src/model/operation/transform.js | 127 +++++++++++++--------- tests/model/operation/transform/unwrap.js | 15 +++ 2 files changed, 91 insertions(+), 51 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 60d3029d6..8a3729844 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -54,7 +54,9 @@ export function transform( a, b, context = {} ) { const transformationFunction = getTransformation( a, b ); try { - return transformationFunction( a.clone(), b, context ); + a = a.clone(); + + return transformationFunction( a, b, context ); } catch ( e ) { log.error( 'Error during operation transformation!' ); log.error( 'Transformed operation', a ); @@ -130,8 +132,6 @@ export function transformSets( operationsA, operationsB, options ) { if ( options.useContext ) { updateOriginalOperation( context, opA, newOpA ); updateOriginalOperation( context, opB, newOpB ); - - updateRelations( context, opA, opB ); } opsA.splice( k, 1, ...newOpA ); @@ -224,6 +224,8 @@ function updateRelations( context, opA, opB ) { if ( opA.targetPosition.isEqual( opB.sourcePosition ) || opB.movedRange.containsPosition( opA.targetPosition ) ) { setRelation( context, opA, opB, 'insertAtSource' ); setRelation( context, opB, opA, 'splitBefore' ); + } else if ( opA.targetPosition.isEqual( opB.deletionPosition ) ) { + setRelation( context, opA, opB, 'insertBetween' ); } break; @@ -294,6 +296,8 @@ function updateRelations( context, opA, opB ) { case MergeOperation: { if ( opA.position.isEqual( opB.sourcePosition ) || opB.movedRange.containsPosition( opA.position ) ) { setRelation( context, opA, opB, 'insertAtSource' ); + } else if ( opA.position.isEqual( opB.deletionPosition ) ) { + setRelation( context, opA, opB, 'insertBetween' ); } break; @@ -522,52 +526,39 @@ setTransformation( AttributeOperation, MoveOperation, ( a, b ) => { } ); function breakRangeByMoveOperation( range, moveOp, includeCommon ) { - let ranges; - - const movedRange = Range.createFromPositionAndShift( moveOp.sourcePosition, moveOp.howMany ); - - if ( movedRange.containsRange( range, true ) ) { - ranges = [ ...range._getTransformedByMoveOperation( moveOp ) ]; - } else if ( range.start.hasSameParentAs( moveOp.sourcePosition ) ) { - ranges = range.getDifference( movedRange ); + let common = null; + let difference = []; - _flatMoveTransform( ranges, moveOp ); + const moveRange = Range.createFromPositionAndShift( moveOp.sourcePosition, moveOp.howMany ); - if ( includeCommon ) { - const common = range.getIntersection( movedRange ); - - if ( common ) { - ranges.push( ...common._getTransformedByMoveOperation( moveOp ) ); - } - } + if ( moveRange.containsRange( range, true ) ) { + common = range; + } else if ( range.start.hasSameParentAs( moveRange.start ) ) { + difference = range.getDifference( moveRange ); + common = includeCommon ? range.getIntersection( moveRange ) : null; } else { - ranges = [ range ]; - - _flatMoveTransform( ranges, moveOp ); + difference = [ range ]; } - return ranges; -} - -function _flatMoveTransform( ranges, moveOp ) { - const targetPosition = moveOp.getMovedRangeStart(); + const result = []; - for ( let i = 0; i < ranges.length; i++ ) { - let range = ranges[ i ]; + for ( let diff of difference ) { + diff = diff._getTransformedByDeletion( moveOp.sourcePosition, moveOp.howMany ); - if ( range.start.hasSameParentAs( moveOp.sourcePosition ) ) { - range = range._getTransformedByDeletion( moveOp.sourcePosition, moveOp.howMany ); - } + const targetPosition = moveOp.getMovedRangeStart(); + const spread = diff.start.hasSameParentAs( targetPosition ); + diff = diff._getTransformedByInsertion( targetPosition, moveOp.howMany, spread ); - if ( range.start.hasSameParentAs( targetPosition ) ) { - const result = range._getTransformedByInsertion( targetPosition, moveOp.howMany, true ); + result.push( ...diff ); + } - ranges.splice( i, 1, ...result ); - i = i - 1 + result.length; - } else { - ranges[ i ] = range; - } + if ( common ) { + result.push( + common._getTransformedByMove( moveOp.sourcePosition, moveOp.targetPosition, moveOp.howMany, false )[ 0 ] + ); } + + return result; } setTransformation( AttributeOperation, SplitOperation, ( a, b ) => { @@ -921,6 +912,10 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { setTransformation( MergeOperation, SplitOperation, ( a, b ) => { if ( b.graveyardPosition ) { + if ( a.deletionPosition.isEqual( b.graveyardPosition ) ) { + a.howMany = b.howMany; + } + a.graveyardPosition = a.graveyardPosition._getTransformedByDeletion( b.graveyardPosition, 1 ); } @@ -938,12 +933,38 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => { // This means that `targetPosition` needs to be transformed. This is the default case though. // For example, if the split would be after `F`, `targetPosition` should also be transformed. // - // There is an exception though. It is when merge operation targets into inside of an element. + // There are two exception, though, when we want to keep `targetPosition` as it was. + // + // First exception is when the merge target position is inside an element (not at the end, as usual). This + // happens when the merge operation earlier was transformed by "the same" merge operation. If merge operation + // targets inside the element we want to keep the original target position (and not transform it) because + // we have additional context telling us that we want to merge to the original element. We can check if the + // merge operation points inside element by checking what is `SplitOperation#howMany`. Since merge target position + // is same as split position, if `howMany` is non-zero, it means that the merge target position is inside an element. + // + // Second exception is when the element to merge is in the graveyard and split operation uses it. In that case + // if target position would be transformed, the merge operation would target at the source position: + // + // root:

Foo

graveyard:

+ // + // SplitOperation: root [ 0, 3 ] using graveyard [ 0 ] (howMany = 0) + // MergeOperation: graveyard [ 0, 0 ] -> root [ 0, 3 ] (howMany = 0) + // + // Since split operation moves the graveyard node back to the root, the merge operation source position changes. + // We would like to merge from the empty

to the "Foo"

: + // + // root:

Foo

graveyard: + // + // MergeOperation#sourcePosition = root [ 1, 0 ] // - if ( a.targetPosition.isEqual( b.position ) && b.howMany != 0 ) { - a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); + // If `targetPosition` is transformed, it would become root [ 1, 0 ] as well. It has to be kept as it was. + // + if ( a.targetPosition.isEqual( b.position ) ) { + if ( b.howMany != 0 || ( b.graveyardPosition && a.deletionPosition.isEqual( b.graveyardPosition ) ) ) { + a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); - return [ a ]; + return [ a ]; + } } if ( a.sourcePosition.hasSameParentAs( b.position ) ) { @@ -1065,17 +1086,18 @@ 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; + let insertBefore = !context.aIsStrong; if ( context.abRelation == 'insertBefore' ) { - aIsStrong = true; + insertBefore = true; } else if ( context.abRelation == 'insertAfter' ) { - aIsStrong = false; + insertBefore = false; } // `a.targetPosition` could be affected by the `b` operation. We will transform it. let newTargetPosition; - if ( a.targetPosition.isEqual( b.targetPosition ) && aIsStrong ) { + if ( a.targetPosition.isEqual( b.targetPosition ) && insertBefore ) { newTargetPosition = a.targetPosition._getTransformedByDeletion( b.sourcePosition, b.howMany @@ -1167,9 +1189,9 @@ setTransformation( MoveOperation, MoveOperation, ( a, b, context ) => { // // If only one of operations is a remove operation, we force remove operation to be the "stronger" one // to provide more expected results. - if ( a.type == 'remove' && b.type != 'remove' ) { + if ( a.type == 'remove' && b.type != 'remove' && !context.aWasUndone ) { aIsStrong = true; - } else if ( a.type != 'remove' && b.type == 'remove' ) { + } else if ( a.type != 'remove' && b.type == 'remove' && !context.bWasUndone ) { aIsStrong = false; } @@ -1282,10 +1304,13 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => { a.sourcePosition = transformed.start; a.howMany = transformed.end.offset - transformed.start.offset; - a.targetPosition = newTargetPosition; if ( a.targetPosition.isEqual( b.position ) && context.abRelation == 'insertAtSource' ) { - a.targetPosition = b.targetPosition; + a.targetPosition = b.moveTargetPosition; + } else if ( a.targetPosition.isEqual( b.insertionPosition ) && context.abRelation == 'insertBetween' ) { + a.targetPosition = a.targetPosition; + } else { + a.targetPosition = newTargetPosition; } return [ a ]; @@ -1569,7 +1594,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { a.howMany -= b.howMany; } - if ( a.position.hasSameParentAs( b.targetPosition ) && a.position.offset < b.sourcePosition.offset ) { + if ( a.position.hasSameParentAs( b.targetPosition ) && a.position.offset < b.targetPosition.offset ) { a.howMany += b.howMany; } diff --git a/tests/model/operation/transform/unwrap.js b/tests/model/operation/transform/unwrap.js index 8cba9be85..6ccd022ab 100644 --- a/tests/model/operation/transform/unwrap.js +++ b/tests/model/operation/transform/unwrap.js @@ -132,6 +132,21 @@ describe( 'transform', () => { 'Bar' ); } ); + + it( 'unwrap merge target element', () => { + john.setData( '
[]Foo
Bar
' ); + kate.setData( '
Foo
[]
Bar
' ); + + john.unwrap(); + kate.merge(); + + syncClients(); + + expectClients( + 'Foo' + + 'Bar' + ); + } ); } ); } ); } ); From 3a965cc519a85c5a8695c94bf9d420115e3691bf Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 3 Aug 2018 15:11:47 +0200 Subject: [PATCH 03/14] Fix: Fixed incorrect Position#_getTransformedByMove for moves that didn't really move any nodes. --- src/model/operation/transform.js | 2 +- src/model/position.js | 20 ++++++++++++-------- tests/model/operation/transform/delete.js | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 8a3729844..f649b7610 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1540,7 +1540,7 @@ setTransformation( SplitOperation, MergeOperation, ( a, b ) => { a.position = a.position._getTransformedByMergeOperation( b ); if ( a.graveyardPosition ) { - a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 ); + a.graveyardPosition = a.graveyardPosition._getTransformedByMergeOperation( b ); } return [ a ]; diff --git a/src/model/position.js b/src/model/position.js index c8be10b7a..036eb5b5e 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -717,12 +717,17 @@ export default class Position { * @returns {module:engine/model/position~Position} Transformed position. */ _getTransformedByMove( sourcePosition, targetPosition, howMany ) { - // Moving a range removes nodes from their original position. We acknowledge this by proper transformation. - let transformed = this._getTransformedByDeletion( sourcePosition, howMany ); - - // Then we update target position, as it could be affected by nodes removal too. + // Update target position, as it could be affected by nodes removal. targetPosition = targetPosition._getTransformedByDeletion( sourcePosition, howMany ); + if ( sourcePosition.isEqual( targetPosition ) ) { + // If `targetPosition` is equal to `sourcePosition` this isn't really any move. Just return position as it is. + return Position.createFromPosition( this ); + } + + // Moving a range removes nodes from their original position. We acknowledge this by proper transformation. + const transformed = this._getTransformedByDeletion( sourcePosition, howMany ); + const isMoved = transformed === null || ( sourcePosition.isEqual( this ) && this.stickiness == 'toNext' ) || ( sourcePosition.getShiftedBy( howMany ).isEqual( this ) && this.stickiness == 'toPrevious' ); @@ -730,14 +735,13 @@ export default class Position { if ( isMoved ) { // This position is inside moved range (or sticks to it). // In this case, we calculate a combination of this position, move source position and target position. - transformed = this._getCombined( sourcePosition, targetPosition ); + return this._getCombined( sourcePosition, targetPosition ); } else { // This position is not inside a removed range. + // // In next step, we simply reflect inserting `howMany` nodes, which might further affect the position. - transformed = transformed._getTransformedByInsertion( targetPosition, howMany ); + return transformed._getTransformedByInsertion( targetPosition, howMany ); } - - return transformed; } /** diff --git a/tests/model/operation/transform/delete.js b/tests/model/operation/transform/delete.js index 93105f90b..2d22888ef 100644 --- a/tests/model/operation/transform/delete.js +++ b/tests/model/operation/transform/delete.js @@ -39,6 +39,24 @@ describe( 'transform', () => { expectClients( 'Fobc' ); } ); + + // https://github.com/ckeditor/ckeditor5-engine/issues/1492 + it( 'delete same content from a few elements', () => { + john.setData( 'F[ooBarAb]c' ); + kate.setData( 'F[ooBarAb]c' ); + + john.delete(); + kate.delete(); + + syncClients(); + expectClients( 'Fc' ); + + john.undo(); + kate.undo(); + + syncClients(); + expectClients( 'FooBarAbc' ); + } ); } ); } ); } ); From 6bd9363c4606c3a31a093008054e6192b21630bf Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 6 Aug 2018 11:46:02 +0200 Subject: [PATCH 04/14] Fix: Changed transforming algorithm and wrap x move OT algorithm. --- src/model/operation/transform.js | 118 +++++++++++------------- tests/model/operation/transform/wrap.js | 33 +++++++ 2 files changed, 89 insertions(+), 62 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index f649b7610..905632430 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -79,6 +79,8 @@ export function transformSets( operationsA, operationsB, options ) { return { operationsA, operationsB }; } + const toTransform = operationsA.map( op => ( { opA: op, transformFrom: 0 } ) ); + const data = { nextBaseVersionA: operationsA[ operationsA.length - 1 ].baseVersion + 1, nextBaseVersionB: operationsB[ operationsB.length - 1 ].baseVersion + 1, @@ -88,70 +90,56 @@ export function transformSets( operationsA, operationsB, options ) { const context = initializeContext( operationsA, operationsB, options ); - // For each operation from `operationsA` set... - for ( let i = 0; i < operationsA.length; ) { - const opsA = [ operationsA[ i ] ]; - - // Get an operation from `operationsB` set, and transform them. - for ( let j = 0; j < operationsB.length; ) { - const opsB = [ operationsB[ j ] ]; - - // After transformation both operation `a` and `b` might be broken into two operations. - // When operation `a` is broken on two operations, it is needed to continue transforming both operations - // by operations from `operationsB` set. Hence, there is a need for additional loop, that will loop - // through all currently processed operation. For the same reason, additional loop is needed for operation - // `b` which may be broken into two operations after transforming by first operation from `opsA`. - for ( let k = 0; k < opsA.length; ) { - for ( let l = 0; l < opsB.length; ) { - const opA = opsA[ k ]; - const opB = opsB[ l ]; - - if ( options.useContext ) { - updateRelations( context, opA, opB ); - } + for ( let i = 0; i < toTransform.length; i++ ) { + const transformFrom = toTransform[ i ].transformFrom; - const contextAB = { - aIsStrong: true, - aWasUndone: context.wasUndone( opA ), - bWasUndone: context.wasUndone( opB ), - abRelation: context.getRelation( opA, opB ), - baRelation: context.getRelation( opB, opA ) - }; - - const contextBA = { - aIsStrong: false, - aWasUndone: context.wasUndone( opB ), - bWasUndone: context.wasUndone( opA ), - abRelation: context.getRelation( opB, opA ), - baRelation: context.getRelation( opA, opB ) - }; - - const newOpA = transform( opA, opB, contextAB ); - const newOpB = transform( opB, opA, contextBA ); - - if ( options.useContext ) { - updateOriginalOperation( context, opA, newOpA ); - updateOriginalOperation( context, opB, newOpB ); - } + for ( let j = transformFrom; j < operationsB.length; ) { + const opA = toTransform[ i ].opA; + const opB = operationsB[ j ]; - opsA.splice( k, 1, ...newOpA ); - k = k + newOpA.length; + if ( options.useContext ) { + updateRelations( context, opA, opB ); + } - opsB.splice( l, 1, ...newOpB ); - l = l + newOpB.length; - } + const contextAB = { + aIsStrong: true, + aWasUndone: context.wasUndone( opA ), + bWasUndone: context.wasUndone( opB ), + abRelation: context.getRelation( opA, opB ), + baRelation: context.getRelation( opB, opA ) + }; + + const contextBA = { + aIsStrong: false, + aWasUndone: context.wasUndone( opB ), + bWasUndone: context.wasUndone( opA ), + abRelation: context.getRelation( opB, opA ), + baRelation: context.getRelation( opA, opB ) + }; + + const newOpsA = transform( opA, opB, contextAB ); + const newOpsB = transform( opB, opA, contextBA ); + + if ( options.useContext ) { + updateOriginalOperation( context, opA, newOpsA ); + updateOriginalOperation( context, opB, newOpsB ); } - operationsB.splice( j, 1, ...opsB ); - j = j + opsB.length; - data.extraOpsB += opsB.length - 1; - } + const newTransformFrom = j + newOpsB.length; + const newToTransform = newOpsA.map( op => ( { opA: op, transformFrom: newTransformFrom } ) ); + + toTransform.splice( i, 1, ...newToTransform ); + + operationsB.splice( j, 1, ...newOpsB ); + j = j + newOpsB.length; - operationsA.splice( i, 1, ...opsA ); - i = i + opsA.length; - data.extraOpsA += opsA.length - 1; + data.extraOpsA += newOpsA.length - 1; + data.extraOpsB += newOpsB.length - 1; + } } + operationsA = toTransform.map( item => item.opA ); + if ( options.padWithNoOps ) { padWithNoOps( operationsA, data.extraOpsB - data.extraOpsA ); padWithNoOps( operationsB, data.extraOpsA - data.extraOpsB ); @@ -1742,14 +1730,20 @@ setTransformation( WrapOperation, MoveOperation, ( a, b, context ) => { return [ a ]; } - const ranges = breakRangeByMoveOperation( a.wrappedRange, b, false ); + const moveRange = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); + const wrappedRange = a.wrappedRange; - return ranges.reverse().map( range => { - const howMany = range.end.offset - range.start.offset; - const elementOrGraveyardPosition = a.graveyardPosition ? a.graveyardPosition : a.element; + if ( moveRange.containsRange( wrappedRange, true ) ) { + a.position = a.position._getCombined( b.sourcePosition, b.getMovedRangeStart() ); + } else { + let transformed = wrappedRange._getTransformedByDeletion( b.sourcePosition, b.howMany ); + transformed = transformed._getTransformedByInsertion( b.targetPosition, b.howMany, false )[ 0 ]; - return new WrapOperation( range.start, howMany, elementOrGraveyardPosition, 0 ); - } ); + a.position = transformed.start; + a.howMany = transformed.end.offset - transformed.start.offset; + } + + return [ a ]; } ); setTransformation( WrapOperation, SplitOperation, ( a, b, context ) => { diff --git a/tests/model/operation/transform/wrap.js b/tests/model/operation/transform/wrap.js index e03efc0e5..9b6b11621 100644 --- a/tests/model/operation/transform/wrap.js +++ b/tests/model/operation/transform/wrap.js @@ -209,6 +209,39 @@ describe( 'transform', () => { '' ); } ); + + it( 'delete all wrapped content', () => { + john.setData( '[FooBarAbc]' ); + kate.setData( '[FooBarAb]c' ); + + john.wrap( 'blockQuote' ); + kate.delete(); + + syncClients(); + expectClients( '
c
' ); + } ); + + it.skip( 'delete all wrapped content and undo', () => { + john.setData( '[FooBarAbc]' ); + kate.setData( '[FooBarAb]c' ); + + john.wrap( 'blockQuote' ); + kate.delete(); + + syncClients(); + expectClients( '
c
' ); + + john.undo(); + + // There is a bug in undo for Kate. + // Kate's content is: '
c
'. + // Then goes undo and it returns "Foo" paragraph into block quote, but "Bar" goes after it. + // There is a move (reinsert) x wrap transformation and the move is not included inside the wrap. + kate.undo(); + + syncClients(); + expectClients( 'FooBarAbc' ); + } ); } ); describe( 'by remove', () => { From 750067fb6bf80f3b90c53d9b8f8e3c3d1a543ca2 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 6 Aug 2018 12:58:20 +0200 Subject: [PATCH 05/14] Fix: Range#_getTransformedByMergeOperation will no longer return ranges that have boundaries in different roots. --- src/model/range.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/model/range.js b/src/model/range.js index 30746b6af..9c1f015de 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -496,6 +496,18 @@ export default class Range { let start = this.start._getTransformedByMergeOperation( operation ); let end = this.end._getTransformedByMergeOperation( operation ); + if ( start.root != end.root ) { + // This happens when only start or end was next to the merged (deleted) element. In this case we need to fix + // the range cause its boundaries would be in different roots. + if ( start.root != this.root ) { + // Fix start position root at it was the only one that was moved. + start = this.start; + } else { + // Fix end position root. + end = this.end.getShiftedBy( -1 ); + } + } + if ( start.isAfter( end ) ) { // This happens in the following two, similar cases: // From ba5351adc08b635b5b8edf8e69c360e6e6fb446c Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 6 Aug 2018 14:30:45 +0200 Subject: [PATCH 06/14] Fix: Improved AttributeOperation x InsertOperation (and vice versa) transformation. --- src/model/operation/transform.js | 51 +++++++++++++------- tests/model/operation/transform/attribute.js | 18 +++++++ 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 905632430..2d661344e 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -382,10 +382,10 @@ setTransformation( AttributeOperation, InsertOperation, ( a, b ) => { // operation range needs to be split. For text insertion, extra operation needs to be generated. // if ( a.range.start.hasSameParentAs( b.position ) && a.range.containsPosition( b.position ) ) { - // This will spread the range into two. - const ranges = a.range._getTransformedByInsertion( b.position, b.howMany, true ); - const result = ranges.map( range => { - return new AttributeOperation( range, a.key, a.oldValue, a.newValue, a.baseVersion ); + // This will return an array with a single, expanded range. + const range = a.range._getTransformedByInsertion( b.position, b.howMany, !b.shouldReceiveAttributes ); + const result = range.map( r => { + return new AttributeOperation( r, a.key, a.oldValue, a.newValue, a.baseVersion ); } ); // `AttributeOperation#range` includes some newly inserted text. @@ -400,9 +400,8 @@ setTransformation( AttributeOperation, InsertOperation, ( a, b ) => { // Bold should be applied also on the new text: //

Fo<$text bold=true>zxxbar

// - // Instead of expanding the attribute operation range, it is needed to create a new attribute operation. - // This is because the inserted text might have already an attribute applied and the `oldValue` property - // of the attribute operation might be wrong: + // There is a special case to consider here, which will dictate how the final solution will look like. The inserted + // text might have already an attribute applied and the `oldValue` property of the attribute operation might be wrong: // // Attribute `highlight="yellow"` should be applied on the following range: //

Fo[zb]ar

@@ -410,29 +409,45 @@ setTransformation( AttributeOperation, InsertOperation, ( a, b ) => { // New text with `highlight="red"` is typed: //

Fo[z<$text highlight="red">xa]r

// - // In this case three operations are needed: `oldValue=null, newValue="yellow"` for `z`, `oldValue="red", - // newValue="yellow"` for `x` and `oldValue=null, newValue="yellow"` for `a`. It could even happen that - // there are multiple nodes with different `oldValue`s are inserted, so multiple new operations might be added. + // In this case we cannot simply apply operation: `oldValue=null, newValue="yellow"` for the whole range + // because that will led to an exception (`oldValue` is incorrect). We also cannot break the original range + // as this would mess up following insert operations: + // + // Attribute operation is to apply bold on below `[]` range, but multiple characters are inserted at `^`. + // Assume ranges breaking: + // + //

F[o^zba]r

+ //

F[o][x][zba]

+ //

F[o][x]x[zba]

+ //

F[o][x]xx[zba]

+ // + // As you can see, characters after the first one are not included in any range when the original attribute range + // is broken at first `x` insertion. + // + // So, the final solution is to just expand the original range, but "fix" all the nodes that have "incorrect" `oldValue`. + // For example, in the highlight case above, we would need an operation to change `highlight="red"` to `null` so that + // the original attribute operation `null` -> `yellow` would be correct. // if ( b.shouldReceiveAttributes ) { - result.splice( 1, 0, ..._getComplementaryAttributeOperations( a, b ) ); + // Be sure to add those operations before the original operation. + result.unshift( ..._getComplementaryAttributeOperations( b, a.key, a.oldValue ) ); } // If nodes should not receive new attribute, just leave the spread ranges as they are. return result; } - // If insert operation is not spreading the attribute operation range, simply transform the range. + // If insert operation is not expanding the attribute operation range, simply transform the range. a.range = a.range._getTransformedByInsertOperation( b )[ 0 ]; return [ a ]; } ); -function _getComplementaryAttributeOperations( attributeOperation, insertOperation ) { +function _getComplementaryAttributeOperations( insertOperation, key, newValue ) { const nodes = insertOperation.nodes; const result = []; // At the beginning we store the attribute value from the first node. - let val = nodes.getNode( 0 ).getAttribute( attributeOperation.key ); + let val = nodes.getNode( 0 ).getAttribute( key ); // This stores the last index of node list where the attribute value has changed. // We need it to create separate `AttributeOperation`s for nodes with different attribute values. @@ -443,14 +458,14 @@ function _getComplementaryAttributeOperations( attributeOperation, insertOperati for ( let i = 1; i < nodes.length; i++ ) { const node = nodes.getNode( i ); - const nodeAttrVal = node.getAttribute( attributeOperation.key ); + const nodeAttrVal = node.getAttribute( key ); // If previous node has different attribute value, we will create an operation to the point before current node. // So all nodes with the same attributes up to this point will be included in one `AttributeOperation`. if ( nodeAttrVal != val ) { // New operation is created only when it is needed. If given node already has proper value for this // attribute we simply skip it without adding a new operation. - if ( val != attributeOperation.newValue ) { + if ( val != newValue ) { addOperation(); } @@ -474,7 +489,7 @@ function _getComplementaryAttributeOperations( attributeOperation, insertOperati ); result.push( - new AttributeOperation( range, attributeOperation.key, val, attributeOperation.newValue, 0 ) + new AttributeOperation( range, key, val, newValue, 0 ) ); } } @@ -667,7 +682,7 @@ setTransformation( InsertOperation, AttributeOperation, ( a, b ) => { const result = [ a ]; if ( a.shouldReceiveAttributes && b.range.containsPosition( a.position ) ) { - result.push( ..._getComplementaryAttributeOperations( b, a ) ); + result.push( ..._getComplementaryAttributeOperations( a, b.key, b.newValue ) ); } return result; diff --git a/tests/model/operation/transform/attribute.js b/tests/model/operation/transform/attribute.js index f67e237e1..662cc4a6f 100644 --- a/tests/model/operation/transform/attribute.js +++ b/tests/model/operation/transform/attribute.js @@ -263,6 +263,24 @@ describe( 'transform', () => { 'Abc' ); } ); + + it( 'multiple typing', () => { + john.setData( '[Foo]' ); + kate.setData( 'Fo[]o' ); + + john.setAttribute( 'bold', true ); + + kate.setSelection( [ 0, 2 ] ); + kate.type( 'x' ); + kate.setSelection( [ 0, 3 ] ); + kate.type( 'x' ); + kate.setSelection( [ 0, 4 ] ); + kate.type( 'x' ); + + syncClients(); + + expectClients( '<$text bold="true">Foxxxo' ); + } ); } ); describe( 'by move', () => { From ddf93f293425fdb3a5dc03773ac06c0541a65532 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 6 Aug 2018 17:45:05 +0200 Subject: [PATCH 07/14] Fix: Improved WrapOperation x WrapOperation transformation. --- src/model/operation/transform.js | 93 +++++++++++++------------ tests/model/operation/transform/wrap.js | 37 ++++++++++ 2 files changed, 85 insertions(+), 45 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 2d661344e..a058fe997 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1879,23 +1879,53 @@ setTransformation( WrapOperation, WrapOperation, ( a, b, context ) => { return [ a ]; } // Ranges intersect. - else if ( ranges.length == 1 ) { - if ( context.aIsStrong ) { - // If the incoming wrap operation is strong, we need to reverse the previous wrap, then apply the incoming - // operation as is, then re-wrap the other nodes that were wrapped in the previous wrap. - // - // Content already wrapped into `blockQuote` but that wrap is not strong: - //

Foo

Bar

Xyz

- // - // Unwrap: - //

Foo

Bar

Xyz

- // - // Wrap with stronger wrap (`a`): - //

Foo

Bar

Xyz

- // - // Re-wrap: - //

Foo

Bar

Xyz

- // + else { + // Range from `b` has some extra nodes other than nodes from `a`. + if ( !a.wrappedRange.containsRange( b.wrappedRange, true ) ) { + if ( context.aIsStrong ) { + // If the incoming wrap operation is strong, we need to reverse the previous wrap, then apply the incoming + // operation as is, then re-wrap the other nodes that were wrapped in the previous wrap. + // + // Content already wrapped into `blockQuote` but that wrap is not strong: + //

Foo

Bar

Xyz

+ // + // Unwrap: + //

Foo

Bar

Xyz

+ // + // Wrap with stronger wrap (`a`): + //

Foo

Bar

Xyz

+ // + // Re-wrap: + //

Foo

Bar

Xyz

+ // + const reversed = b.getReversed(); + + // Unwrap operation (reversed wrap) always puts a node into a graveyard. Not every wrap operation pulls a node + // from the graveyard, though. This means that after reversing a wrap operation, there might be a need to + // update a position in graveyard. + if ( !b.graveyardPosition && a.graveyardPosition ) { + a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( reversed.graveyardPosition, 1 ); + } + + const bOnlyRange = b.wrappedRange.getDifference( a.wrappedRange )[ 0 ]; + const rewrapRange = bOnlyRange._getTransformedByWrapOperation( a ); + const rewrapHowMany = rewrapRange.end.offset - rewrapRange.start.offset; + const rewrap = new WrapOperation( rewrapRange.start, rewrapHowMany, reversed.graveyardPosition, 0 ); + + return [ reversed, a, rewrap ]; + } else { + // If the incoming wrap operation is not strong, just wrap those nodes which were not wrapped already. + const range = ranges[ 0 ]._getTransformedByWrapOperation( b ); + + a.position = range.start; + a.howMany = range.end.offset - range.start.offset; + a.graveyardPosition = newGraveyardPosition; + + return [ a ]; + } + } + // Range from `b` is contained in range from `a`. Reverse operation `b` in addition to operation `a`. + else { const reversed = b.getReversed(); // Unwrap operation (reversed wrap) always puts a node into a graveyard. Not every wrap operation pulls a node @@ -1905,35 +1935,8 @@ setTransformation( WrapOperation, WrapOperation, ( a, b, context ) => { a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( reversed.graveyardPosition, 1 ); } - const bOnlyRange = b.wrappedRange.getDifference( a.wrappedRange )[ 0 ]; - const rewrapRange = bOnlyRange._getTransformedByWrapOperation( a ); - const rewrapHowMany = rewrapRange.end.offset - rewrapRange.start.offset; - const rewrap = new WrapOperation( rewrapRange.start, rewrapHowMany, reversed.graveyardPosition, 0 ); - - return [ reversed, a, rewrap ]; - } else { - // If the incoming wrap operation is not strong, just wrap those nodes which were not wrapped already. - const range = ranges[ 0 ]._getTransformedByWrapOperation( b ); - - a.position = range.start; - a.howMany = range.end.offset - range.start.offset; - a.graveyardPosition = newGraveyardPosition; - - return [ a ]; - } - } - // Range from `b` is contained in range from `a`. Reverse operation `b` in addition to operation `a`. - else { - const reversed = b.getReversed(); - - // Unwrap operation (reversed wrap) always puts a node into a graveyard. Not every wrap operation pulls a node - // from the graveyard, though. This means that after reversing a wrap operation, there might be a need to - // update a position in graveyard. - if ( !b.graveyardPosition && a.graveyardPosition ) { - a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( reversed.graveyardPosition, 1 ); + return [ reversed, a ]; } - - return [ reversed, a ]; } } diff --git a/tests/model/operation/transform/wrap.js b/tests/model/operation/transform/wrap.js index 9b6b11621..3ac69053b 100644 --- a/tests/model/operation/transform/wrap.js +++ b/tests/model/operation/transform/wrap.js @@ -80,6 +80,23 @@ describe( 'transform', () => { ); } ); + it( 'intersecting wrap #3', () => { + john.setData( '[FooBar]Abc' ); + kate.setData( '[Foo]BarAbc' ); + + john.wrap( 'blockQuote' ); + kate.wrap( 'div' ); + + syncClients(); + expectClients( + '
' + + 'Foo' + + 'Bar' + + '
' + + 'Abc' + ); + } ); + it( 'intersecting wrap, then undo #1', () => { john.setData( '[FooBar]Abc' ); kate.setData( 'Foo[BarAbc]' ); @@ -135,6 +152,26 @@ describe( 'transform', () => { expectClients( 'FooBarAbc' ); } ); + it( 'intersecting wrap #3, then undo', () => { + john.setData( '[FooBar]Abc' ); + kate.setData( '[Foo]BarAbc' ); + + john.wrap( 'blockQuote' ); + kate.wrap( 'div' ); + + syncClients(); + + john.undo(); + kate.undo(); + + syncClients(); + expectClients( + 'Foo' + + 'Bar' + + 'Abc' + ); + } ); + it( 'element and text', () => { john.setData( '[Foo]' ); kate.setData( '[Foo]' ); From c95dc7c0d2aae4b5835d296b66dfb8f8a14c94e7 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 6 Aug 2018 18:35:44 +0200 Subject: [PATCH 08/14] Fix: AttributeOperation x InsertOperation OT alogirthm improvements. --- src/model/operation/transform.js | 4 ++-- tests/model/operation/transform/attribute.js | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index a058fe997..5643442a3 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -437,7 +437,7 @@ setTransformation( AttributeOperation, InsertOperation, ( a, b ) => { } // If insert operation is not expanding the attribute operation range, simply transform the range. - a.range = a.range._getTransformedByInsertOperation( b )[ 0 ]; + a.range = a.range._getTransformedByInsertion( b.position, b.howMany, false )[ 0 ]; return [ a ]; } ); @@ -681,7 +681,7 @@ setTransformation( AttributeOperation, UnwrapOperation, ( a, b ) => { setTransformation( InsertOperation, AttributeOperation, ( a, b ) => { const result = [ a ]; - if ( a.shouldReceiveAttributes && b.range.containsPosition( a.position ) ) { + if ( a.shouldReceiveAttributes && a.position.hasSameParentAs( b.range.start ) && b.range.containsPosition( a.position ) ) { result.push( ..._getComplementaryAttributeOperations( a, b.key, b.newValue ) ); } diff --git a/tests/model/operation/transform/attribute.js b/tests/model/operation/transform/attribute.js index 662cc4a6f..93bc98a76 100644 --- a/tests/model/operation/transform/attribute.js +++ b/tests/model/operation/transform/attribute.js @@ -281,6 +281,18 @@ describe( 'transform', () => { expectClients( '<$text bold="true">Foxxxo' ); } ); + + it( 'type inside element which attribute changes', () => { + john.setData( '[]' ); + kate.setData( '[]' ); + + john.setAttribute( 'bold', true ); + kate.type( 'x' ); + + syncClients(); + + expectClients( 'x' ); + } ); } ); describe( 'by move', () => { From 33381a4cd36ffce455baad63964572ef1bf7298e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 8 Aug 2018 12:22:41 +0200 Subject: [PATCH 09/14] Fix: Improved MoveOperation x MergeOperation transformation. --- src/model/operation/transform.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 5643442a3..5d500c876 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1339,7 +1339,14 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { // it would be a move operation that moves 0 nodes, so maybe it is better just to return no-op. // if ( a.howMany == 1 ) { - return getNoOp(); + if ( !context.bWasUndone ) { + return getNoOp(); + } else { + a.sourcePosition = Position.createFromPosition( b.graveyardPosition ); + a.targetPosition = a.targetPosition._getTransformedByMergeOperation( b ); + + return [ a ]; + } } } } From 1aa3f703f5414976d96c951b5cd9c37141a5ed0d Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 15:56:09 +0200 Subject: [PATCH 10/14] Docs: Added description for `MergeOperation#howMany`. --- src/model/operation/mergeoperation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/operation/mergeoperation.js b/src/model/operation/mergeoperation.js index 5b0b148f9..ebf71d523 100644 --- a/src/model/operation/mergeoperation.js +++ b/src/model/operation/mergeoperation.js @@ -30,7 +30,7 @@ export default class MergeOperation extends Operation { * * @param {module:engine/model/position~Position} sourcePosition Position inside the merged element. All nodes from that * element after that position will be moved to {@link ~#targetPosition}. - * @param {Number} howMany + * @param {Number} howMany Summary offset size of nodes which will be moved from the merged element to the new parent. * @param {module:engine/model/position~Position} targetPosition Position which the nodes from the merged elements will be moved to. * @param {module:engine/model/position~Position} graveyardPosition Position in graveyard to which the merged element will be moved. * @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation From a83adb6cde095410aa762e52ed9c8be2fd1d4841 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 16:27:51 +0200 Subject: [PATCH 11/14] Changed: Improved new transforming algorithm. --- src/model/operation/transform.js | 91 ++++++++++++++++++-------------- 1 file changed, 51 insertions(+), 40 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 5d500c876..0553c1bff 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -79,7 +79,9 @@ export function transformSets( operationsA, operationsB, options ) { return { operationsA, operationsB }; } - const toTransform = operationsA.map( op => ( { opA: op, transformFrom: 0 } ) ); + for ( const op of operationsA ) { + op.transformBy = 0; + } const data = { nextBaseVersionA: operationsA[ operationsA.length - 1 ].baseVersion + 1, @@ -90,55 +92,64 @@ export function transformSets( operationsA, operationsB, options ) { const context = initializeContext( operationsA, operationsB, options ); - for ( let i = 0; i < toTransform.length; i++ ) { - const transformFrom = toTransform[ i ].transformFrom; + let i = 0; - for ( let j = transformFrom; j < operationsB.length; ) { - const opA = toTransform[ i ].opA; - const opB = operationsB[ j ]; + while ( i < operationsA.length ) { + const opA = operationsA[ i ]; - if ( options.useContext ) { - updateRelations( context, opA, opB ); - } + const transformBy = opA.transformBy; - const contextAB = { - aIsStrong: true, - aWasUndone: context.wasUndone( opA ), - bWasUndone: context.wasUndone( opB ), - abRelation: context.getRelation( opA, opB ), - baRelation: context.getRelation( opB, opA ) - }; - - const contextBA = { - aIsStrong: false, - aWasUndone: context.wasUndone( opB ), - bWasUndone: context.wasUndone( opA ), - abRelation: context.getRelation( opB, opA ), - baRelation: context.getRelation( opA, opB ) - }; - - const newOpsA = transform( opA, opB, contextAB ); - const newOpsB = transform( opB, opA, contextBA ); - - if ( options.useContext ) { - updateOriginalOperation( context, opA, newOpsA ); - updateOriginalOperation( context, opB, newOpsB ); - } + if ( transformBy == operationsB.length ) { + i++; + continue; + } + + const opB = operationsB[ transformBy ]; - const newTransformFrom = j + newOpsB.length; - const newToTransform = newOpsA.map( op => ( { opA: op, transformFrom: newTransformFrom } ) ); + if ( options.useContext ) { + updateRelations( context, opA, opB ); + } - toTransform.splice( i, 1, ...newToTransform ); + const contextAB = { + aIsStrong: true, + aWasUndone: context.wasUndone( opA ), + bWasUndone: context.wasUndone( opB ), + abRelation: context.getRelation( opA, opB ), + baRelation: context.getRelation( opB, opA ) + }; + + const contextBA = { + aIsStrong: false, + aWasUndone: context.wasUndone( opB ), + bWasUndone: context.wasUndone( opA ), + abRelation: context.getRelation( opB, opA ), + baRelation: context.getRelation( opA, opB ) + }; + + const newOpsA = transform( opA, opB, contextAB ); + const newOpsB = transform( opB, opA, contextBA ); + + if ( options.useContext ) { + updateOriginalOperation( context, opA, newOpsA ); + updateOriginalOperation( context, opB, newOpsB ); + } - operationsB.splice( j, 1, ...newOpsB ); - j = j + newOpsB.length; + const newTransformBy = transformBy + newOpsB.length; - data.extraOpsA += newOpsA.length - 1; - data.extraOpsB += newOpsB.length - 1; + for ( const newOpA of newOpsA ) { + newOpA.transformBy = newTransformBy; } + + operationsA.splice( i, 1, ...newOpsA ); + operationsB.splice( transformBy, 1, ...newOpsB ); + + data.extraOpsA += newOpsA.length - 1; + data.extraOpsB += newOpsB.length - 1; } - operationsA = toTransform.map( item => item.opA ); + for ( const op of operationsA ) { + delete op.transformBy; + } if ( options.padWithNoOps ) { padWithNoOps( operationsA, data.extraOpsB - data.extraOpsA ); From 95ef2799c2e7855dee534e8e3d64fc689b22147d Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 17:11:07 +0200 Subject: [PATCH 12/14] Docs: Improved descriptions in transformation algorithms. --- src/model/operation/transform.js | 40 +++++++++++++++++--------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 0553c1bff..8576dff87 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -417,27 +417,23 @@ setTransformation( AttributeOperation, InsertOperation, ( a, b ) => { // Attribute `highlight="yellow"` should be applied on the following range: //

Fo[zb]ar

// - // New text with `highlight="red"` is typed: + // But before that, new text with `highlight="red"` is typed: //

Fo[z<$text highlight="red">xa]r

// - // In this case we cannot simply apply operation: `oldValue=null, newValue="yellow"` for the whole range - // because that will led to an exception (`oldValue` is incorrect). We also cannot break the original range - // as this would mess up following insert operations: + // In this case we cannot simply apply operation changing the attribute value from `null` to `"yellow"` for the whole range + // because that would lead to an exception (`oldValue` is incorrect for `x`). // - // Attribute operation is to apply bold on below `[]` range, but multiple characters are inserted at `^`. - // Assume ranges breaking: + // We also cannot break the original range as this would mess up a scenario when there are multiple following insert operations. // - //

F[o^zba]r

- //

F[o][x][zba]

- //

F[o][x]x[zba]

- //

F[o][x]xx[zba]

+ // So, the attribute range will be expanded, no matter what attributes are set on the inserted nodes: // - // As you can see, characters after the first one are not included in any range when the original attribute range - // is broken at first `x` insertion. + //

Fo[z<$text highlight="red">xa]r

<--- Change from `null` to `yellow`, still throwing an exception. // - // So, the final solution is to just expand the original range, but "fix" all the nodes that have "incorrect" `oldValue`. - // For example, in the highlight case above, we would need an operation to change `highlight="red"` to `null` so that - // the original attribute operation `null` -> `yellow` would be correct. + // But before that operation would be applied, we will add an additional attribute operation that will change + // attributes on inserted nodes in a way which would make the original operation correct: + // + //

Fo[z{<$text highlight="red">}xa]r

<--- Change `{}` from `red` to `null`. + //

Fo[zxa]r

<--- Now change from `null` to `yellow` is completely fine. // if ( b.shouldReceiveAttributes ) { // Be sure to add those operations before the original operation. @@ -934,16 +930,22 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => { } // Case 1: Merge operation moves nodes to the place where split happens. + // This is a classic situation when there are two paragraphs, and there is a split (enter) after the first + // paragraph and there is a merge (delete) at the beginning of the second paragraph: // - // Split is after `Foo`, while merge is from `Bar` to the end of `Foo`: - //

Foo

Bar

+ //

Foo{}

[]Bar

. // - // After split: + // Split is after `Foo`, while merge is from `Bar` to the end of `Foo`. + // + // State after split: //

Foo

Bar

// - // In this case, `Bar` should be merged to the new paragraph: + // Now, `Bar` should be merged to the new paragraph: //

Foo

Bar

// + // Instead of merging it to the original paragraph: + //

FooBar

+ // // This means that `targetPosition` needs to be transformed. This is the default case though. // For example, if the split would be after `F`, `targetPosition` should also be transformed. // From d165a1ab03c4f45cd93cf2ed46c1a935de60f63a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 17:11:51 +0200 Subject: [PATCH 13/14] Changed: Used `WeakMap` to store indexes of next operations to transform in `transformSets`. --- src/model/operation/transform.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 8576dff87..8afe56c1b 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -79,8 +79,10 @@ export function transformSets( operationsA, operationsB, options ) { return { operationsA, operationsB }; } + const nextTransformIndex = new WeakMap(); + for ( const op of operationsA ) { - op.transformBy = 0; + nextTransformIndex.set( op, 0 ); } const data = { @@ -97,14 +99,14 @@ export function transformSets( operationsA, operationsB, options ) { while ( i < operationsA.length ) { const opA = operationsA[ i ]; - const transformBy = opA.transformBy; + const transformByIndex = nextTransformIndex.get( opA ); - if ( transformBy == operationsB.length ) { + if ( transformByIndex == operationsB.length ) { i++; continue; } - const opB = operationsB[ transformBy ]; + const opB = operationsB[ transformByIndex ]; if ( options.useContext ) { updateRelations( context, opA, opB ); @@ -134,23 +136,17 @@ export function transformSets( operationsA, operationsB, options ) { updateOriginalOperation( context, opB, newOpsB ); } - const newTransformBy = transformBy + newOpsB.length; - for ( const newOpA of newOpsA ) { - newOpA.transformBy = newTransformBy; + nextTransformIndex.set( newOpA, transformByIndex + newOpsB.length ); } operationsA.splice( i, 1, ...newOpsA ); - operationsB.splice( transformBy, 1, ...newOpsB ); + operationsB.splice( transformByIndex, 1, ...newOpsB ); data.extraOpsA += newOpsA.length - 1; data.extraOpsB += newOpsB.length - 1; } - for ( const op of operationsA ) { - delete op.transformBy; - } - if ( options.padWithNoOps ) { padWithNoOps( operationsA, data.extraOpsB - data.extraOpsA ); padWithNoOps( operationsB, data.extraOpsA - data.extraOpsB ); From da9b4aca1b3ac90bd8c2ca02900b65cabb885fa1 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 17:13:42 +0200 Subject: [PATCH 14/14] Docs: Improvement in docs. --- src/model/operation/transform.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 8afe56c1b..0194d610a 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1908,7 +1908,7 @@ setTransformation( WrapOperation, WrapOperation, ( a, b, context ) => { // Unwrap: //

Foo

Bar

Xyz

// - // Wrap with stronger wrap (`a`): + // Wrap with stronger wrap: //

Foo

Bar

Xyz

// // Re-wrap: