From cddfcce490dd3273a45ad6e1009d36b4cb6c5b37 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 11:20:11 +0200 Subject: [PATCH 01/26] Changed: Introduced relations in OT. --- src/model/operation/transform.js | 303 ++++++++++++++++--- src/model/position.js | 1 + tests/model/operation/transform.js | 42 +-- tests/model/operation/transform/attribute.js | 2 +- tests/model/operation/transform/insert.js | 4 +- tests/model/operation/transform/marker.js | 4 +- tests/model/operation/transform/move.js | 8 +- tests/model/operation/transform/remove.js | 12 +- tests/model/operation/transform/split.js | 27 +- tests/model/operation/transform/unwrap.js | 5 +- tests/model/operation/transform/utils.js | 42 ++- tests/model/operation/transform/wrap.js | 52 +++- 12 files changed, 377 insertions(+), 125 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index f0a9dba73..ee05eee28 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -72,7 +72,7 @@ function updateBaseVersions( operations, baseVersion ) { return operations; } -function transform( a, b, context = { aIsStrong: false } ) { +function transform( a, b, context = { aIsStrong: false, wasUndone: () => false, getRelation: () => null } ) { const transformationFunction = getTransformation( a, b ); return transformationFunction( a.clone(), b, context ); @@ -124,6 +124,8 @@ 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 ); @@ -176,6 +178,7 @@ function initializeContext( opsA, opsB, options ) { } context.document = options.document; + context.relations = new Map(); context.wasUndone = function( op ) { if ( !options.useContext ) { @@ -187,9 +190,151 @@ function initializeContext( opsA, opsB, options ) { return this.document.history.isUndoneOperation( originalOp ); }; + context.getRelation = function( opA, opB ) { + if ( !options.useContext ) { + return null; + } + + const origB = this.originalOperations.get( opB ); + const undoneB = this.document.history.getUndoneOperation( origB ); + + if ( !undoneB ) { + return null; + } + + const origA = this.originalOperations.get( opA ); + const relationsA = this.relations.get( origA ); + + if ( relationsA ) { + return relationsA.get( undoneB ) || null; + } + + return null; + }; + return context; } +function updateRelations( context, opA, opB ) { + switch ( opA.constructor ) { + case MoveOperation: + case RemoveOperation: + case ReinsertOperation: { + switch ( opB.constructor ) { + case MergeOperation: { + if ( opA.targetPosition.isEqual( opB.sourcePosition ) || opB.movedRange.containsPosition( opA.targetPosition ) ) { + setRelation( context, opA, opB, 'insertAtSource' ); + setRelation( context, opB, opA, 'splitBefore' ); + } + + break; + } + + case MoveOperation: { + if ( opA.targetPosition.isEqual( opB.sourcePosition ) || opA.targetPosition.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'insertBefore' ); + setRelation( context, opB, opA, 'insertAfter' ); + } + + break; + } + + case UnwrapOperation: { + const isInside = opA.targetPosition.hasSameParentAs( opB.targetPosition ); + + if ( isInside ) { + setRelation( context, opA, opB, 'insertInside' ); + } + + break; + } + } + + break; + } + + case SplitOperation: { + switch ( opB.constructor ) { + case MergeOperation: { + if ( opA.position.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'splitBefore' ); + setRelation( context, opB, opA, 'splitAfter' ); + } + + break; + } + + case MoveOperation: { + if ( opA.position.isEqual( opB.sourcePosition ) || opA.position.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'splitBefore' ); + setRelation( context, opB, opA, 'insertAtSource' ); + } + + break; + } + + case UnwrapOperation: { + const isInside = opA.position.hasSameParentAs( opB.position ); + + if ( isInside ) { + setRelation( context, opA, opB, 'splitInside' ); + } + + break; + } + } + + break; + } + + case InsertOperation: { + switch ( opB.constructor ) { + case MergeOperation: { + if ( opA.position.isEqual( opB.sourcePosition ) || opB.movedRange.containsPosition( opA.position ) ) { + setRelation( context, opA, opB, 'insertAtSource' ); + } + + break; + } + + case MoveOperation: { + if ( opA.position.isEqual( opB.sourcePosition ) || opA.position.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'insertBefore' ); + } + + break; + } + + case UnwrapOperation: { + const isInside = opA.position.hasSameParentAs( opB.position ); + + if ( isInside ) { + setRelation( context, opA, opB, 'insertInside' ); + } + + break; + } + } + + break; + } + } +} + +function setRelation( context, opA, opB, relation ) { + const origA = context.originalOperations.get( opA ); + const origB = context.originalOperations.get( opB ); + + let relationsA = context.relations.get( origA ); + + if ( !relationsA ) { + relationsA = new Map(); + context.relations.set( origA, relationsA ); + } + + relationsA.set( origB, relation ); +} + function updateOriginalOperation( context, oldOp, newOps ) { const originalOp = context.originalOperations.get( oldOp ); @@ -345,8 +490,12 @@ setTransformation( AttributeOperation, MergeOperation, ( a, b ) => { // Case 1: Attribute change on the merged element. In this case, the merged element was moved to graveyard. // An additional attribute operation that will change the (re)moved element needs to be generated. + // Do it only, if there is more than one element in attribute range. If there is only one element, + // it will be handled by the default algorithm. // - if ( a.range.start.hasSameParentAs( b.deletionPosition ) ) { + const howMany = a.range.end.offset - a.range.start.offset; + + if ( howMany > 1 && a.range.start.hasSameParentAs( b.deletionPosition ) ) { if ( a.range.containsPosition( b.deletionPosition ) || a.range.start.isEqual( b.deletionPosition ) ) { ranges.push( Range.createFromPositionAndShift( b.graveyardPosition, 1 ) ); } @@ -541,13 +690,23 @@ setTransformation( InsertOperation, InsertOperation, ( a, b, context ) => { return [ a ]; } ); -setTransformation( InsertOperation, MoveOperation, ( a, b ) => { +setTransformation( InsertOperation, MoveOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.targetPosition ) && context.getRelation( a, b ) == 'insertBefore' ) { + return [ a ]; + } + a.position = a.position._getTransformedByMoveOperation( b ); return [ a ]; } ); -setTransformation( InsertOperation, SplitOperation, ( a, b ) => { +setTransformation( InsertOperation, SplitOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'insertAtSource' ) { + a.position = b.moveTargetPosition; + + return [ a ]; + } + a.position = a.position._getTransformedBySplitOperation( b ); return [ a ]; @@ -559,7 +718,13 @@ setTransformation( InsertOperation, MergeOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( InsertOperation, WrapOperation, ( a, b ) => { +setTransformation( InsertOperation, WrapOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'insertInside' ) { + a.position = b.targetPosition; + + return [ a ]; + } + a.position = a.position._getTransformedByWrapOperation( b ); return [ a ]; @@ -716,7 +881,7 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { a.sourcePosition = a.sourcePosition._getTransformedByMoveOperation( b ); a.targetPosition = a.targetPosition._getTransformedByMoveOperation( b ); - if ( !a.graveyardPosition.isEqual( b.targetPosition ) || !context.aIsStrong ) { + if ( !a.graveyardPosition.isEqual( b.targetPosition ) ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } @@ -747,9 +912,9 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => { // There is one exception though - when the split operation is a result of undo. In those cases, it is needed // to keep `targetPosition` intact, so the nodes are returned to the correct element. // - if ( a.targetPosition.isEqual( b.position ) && b.graveyardPosition ) { - return [ a ]; - } + // if ( a.targetPosition.isEqual( b.position ) && b.graveyardPosition ) { + // return [ a ]; + // } a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b ); @@ -814,14 +979,14 @@ setTransformation( MergeOperation, UnwrapOperation, ( a, b, context ) => { // ----------------------- -setTransformation( MoveOperation, InsertOperation, ( a, b ) => { +setTransformation( MoveOperation, InsertOperation, ( a, b, context ) => { const moveRange = Range.createFromPositionAndShift( a.sourcePosition, a.howMany ); const transformed = moveRange._getTransformedByInsertOperation( b, false )[ 0 ]; a.sourcePosition = transformed.start; a.howMany = transformed.end.offset - transformed.start.offset; - if ( !a.targetPosition.isEqual( b.position ) ) { + if ( !a.targetPosition.isEqual( b.position ) || context.getRelation( b, a ) == 'insertBefore' ) { a.targetPosition = a.targetPosition._getTransformedByInsertOperation( b ); } @@ -838,14 +1003,23 @@ 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 aIsStrong = context.aIsStrong || context.getRelation( a, b ) == 'insertBefore'; // `a.targetPosition` could be affected by the `b` operation. We will transform it. - const newTargetPosition = a.targetPosition._getTransformedByMove( - b.sourcePosition, - b.targetPosition, - b.howMany - ); + let newTargetPosition; + + if ( a.targetPosition.isEqual( b.targetPosition ) && aIsStrong ) { + newTargetPosition = a.targetPosition._getTransformedByDeletion( + b.sourcePosition, + b.howMany + ); + } else { + newTargetPosition = a.targetPosition._getTransformedByMove( + b.sourcePosition, + b.targetPosition, + b.howMany + ); + } // // Special case #1 + mirror. @@ -993,7 +1167,7 @@ setTransformation( MoveOperation, MoveOperation, ( a, b, context ) => { return makeMoveOperationsFromRanges( ranges, newTargetPosition ); } ); -setTransformation( MoveOperation, SplitOperation, ( a, b ) => { +setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => { const newTargetPosition = a.targetPosition._getTransformedBySplitOperation( b ); // Case 1: Last element in the moved range got split. @@ -1028,8 +1202,8 @@ setTransformation( MoveOperation, SplitOperation, ( a, b ) => { rightRange = rightRange._getTransformedBySplitOperation( b ); const ranges = [ - rightRange, - Range.createFromPositionAndShift( moveRange.start, b.position.offset - moveRange.start.offset ) + new Range( moveRange.start, b.position ), + rightRange ]; return makeMoveOperationsFromRanges( ranges, newTargetPosition ); @@ -1043,6 +1217,10 @@ setTransformation( MoveOperation, SplitOperation, ( a, b ) => { a.howMany = transformed.end.offset - transformed.start.offset; a.targetPosition = newTargetPosition; + if ( a.targetPosition.isEqual( b.position ) && context.getRelation( a, b ) == 'insertAtSource' ) { + a.targetPosition = b.targetPosition; + } + return [ a ]; } ); @@ -1081,7 +1259,7 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { return [ a ]; } ); -setTransformation( MoveOperation, WrapOperation, ( a, b ) => { +setTransformation( MoveOperation, WrapOperation, ( a, b, context ) => { const moveRange = Range.createFromPositionAndShift( a.sourcePosition, a.howMany ); const newTargetPosition = a.targetPosition._getTransformedByWrapOperation( b ); @@ -1133,6 +1311,10 @@ setTransformation( MoveOperation, WrapOperation, ( a, b ) => { a.howMany = transformed.end.offset - transformed.start.offset; a.targetPosition = newTargetPosition; + if ( a.targetPosition.isEqual( b.position ) && context.getRelation( a, b ) == 'insertInside' ) { + a.targetPosition = b.targetPosition; + } + return [ a ]; } ); @@ -1235,7 +1417,11 @@ setTransformation( RootAttributeOperation, RootAttributeOperation, ( a, b, conte // ----------------------- -setTransformation( SplitOperation, InsertOperation, ( a, b ) => { +setTransformation( SplitOperation, InsertOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( b, a ) == 'insertAtSource' ) { + return [ a ]; + } + a.position = a.position._getTransformedByInsertOperation( b ); return [ a ]; @@ -1251,7 +1437,7 @@ setTransformation( SplitOperation, MergeOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, MoveOperation, ( a, b ) => { +setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { if ( a.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } @@ -1279,6 +1465,12 @@ setTransformation( SplitOperation, MoveOperation, ( a, b ) => { return [ a ]; } + if ( a.position.isEqual( b.targetPosition ) && context.getRelation( a, b ) == 'splitBefore' ) { + a.position = a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); + + return [ a ]; + } + // The default case. // a.position = a.position._getTransformedByMoveOperation( b ); @@ -1286,7 +1478,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, SplitOperation, ( a, b ) => { +setTransformation( SplitOperation, SplitOperation, ( a, b, context ) => { if ( a.position.isEqual( b.position ) ) { if ( !a.graveyardPosition && !b.graveyardPosition ) { return getNoOp(); @@ -1295,6 +1487,8 @@ setTransformation( SplitOperation, SplitOperation, ( a, b ) => { if ( a.graveyardPosition && b.graveyardPosition && a.graveyardPosition.isEqual( b.graveyardPosition ) ) { return getNoOp(); } + } else if ( a.position.isEqual( b.insertionPosition ) && context.getRelation( a, b ) == 'splitBefore' ) { + return [ a ]; } else { a.position = a.position._getTransformedBySplitOperation( b ); } @@ -1306,7 +1500,7 @@ setTransformation( SplitOperation, SplitOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, WrapOperation, ( a, b ) => { +setTransformation( SplitOperation, WrapOperation, ( a, b, context ) => { // Case 1: If split position has been wrapped, reverse the wrapping so that split can be applied as intended. // This is an edge case scenario where it is difficult to find a correct solution. // Since it will be a rare (or only theoretical) scenario, the algorithm will perform the easy solution. @@ -1324,7 +1518,11 @@ setTransformation( SplitOperation, WrapOperation, ( a, b ) => { return [ reversed, a ]; } - a.position = a.position._getTransformedByWrapOperation( b ); + if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'splitInside' ) { + a.position = b.targetPosition; + } else { + a.position = a.position._getTransformedByWrapOperation( b ); + } if ( a.graveyardPosition && b.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByDeletion( b.graveyardPosition, 1 ); @@ -1333,8 +1531,17 @@ setTransformation( SplitOperation, WrapOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, UnwrapOperation, ( a, b ) => { - a.position = a.position._getTransformedByUnwrapOperation( b ); +setTransformation( SplitOperation, UnwrapOperation, ( a, b, context ) => { + const splitInside = a.position.hasSameParentAs( b.position ); + + if ( splitInside && !context.wasUndone( b ) ) { + const path = b.graveyardPosition.path.slice(); + path.push( 0 ); + + a.position = new Position( b.graveyardPosition.root, path ); + } else { + a.position = a.position._getTransformedByUnwrapOperation( b ); + } if ( a.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 ); @@ -1345,7 +1552,13 @@ setTransformation( SplitOperation, UnwrapOperation, ( a, b ) => { // ----------------------- -setTransformation( WrapOperation, InsertOperation, ( a, b ) => { +setTransformation( WrapOperation, InsertOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( b, a ) == 'insertInside' ) { + a.howMany += b.howMany; + + return [ a ]; + } + const transformed = a.wrappedRange._getTransformedByInsertOperation( b, false )[ 0 ]; a.position = transformed.start; @@ -1367,11 +1580,18 @@ setTransformation( WrapOperation, MergeOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( WrapOperation, MoveOperation, ( a, b ) => { +setTransformation( WrapOperation, MoveOperation, ( a, b, context ) => { if ( a.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } + if ( a.position.isEqual( b.targetPosition ) && context.getRelation( b, a ) == 'insertInside' ) { + a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); + a.howMany += b.howMany; + + return [ a ]; + } + const ranges = breakRangeByMoveOperation( a.wrappedRange, b, false ); if ( ranges.length == 0 ) { @@ -1388,10 +1608,14 @@ setTransformation( WrapOperation, MoveOperation, ( a, b ) => { } ); } ); -setTransformation( WrapOperation, SplitOperation, ( a, b ) => { +setTransformation( WrapOperation, SplitOperation, ( a, b, context ) => { // Case 1: If range to wrap got split by split operation cancel the wrapping. + // Do that only if this is not undo mode. If `b` operation was earlier transformed by unwrap operation + // and the split position was inside the unwrapped range, then proceed without special case. // - if ( a.position.hasSameParentAs( b.position ) && a.wrappedRange.containsPosition( b.position ) ) { + const isInside = a.position.hasSameParentAs( b.position ) && a.wrappedRange.containsPosition( b.position ); + + if ( isInside && context.getRelation( b, a ) !== 'splitInside' ) { // We cannot just return no-op in this case, because in the mirror case scenario the wrap is reversed, which // might introduce a new node in the graveyard (if the wrap didn't have `graveyardPosition`, then the wrap // created a new element which was put to the graveyard when the wrap was reversed). @@ -1401,7 +1625,7 @@ setTransformation( WrapOperation, SplitOperation, ( a, b ) => { const graveyard = a.position.root.document.graveyard; const graveyardPosition = new Position( graveyard, [ 0 ] ); - return new InsertOperation( graveyardPosition, a.element, 0 ); + return [ new InsertOperation( graveyardPosition, a.element, 0 ) ]; } else { return getNoOp(); } @@ -1622,7 +1846,7 @@ setTransformation( UnwrapOperation, MergeOperation, ( a, b, context ) => { return [ a ]; } ); -setTransformation( UnwrapOperation, MoveOperation, ( a, b, context ) => { +setTransformation( UnwrapOperation, MoveOperation, ( a, b ) => { // Case 1: Move operation moves nodes from the unwrapped element. // This does not have any impact on `UnwrapOperation#position`, but `#howMany` has to be changed. // @@ -1640,7 +1864,7 @@ setTransformation( UnwrapOperation, MoveOperation, ( a, b, context ) => { a.position = a.position._getTransformedByMoveOperation( b ); - if ( !a.graveyardPosition.isEqual( b.targetPosition ) || !context.aIsStrong ) { + if ( !a.graveyardPosition.isEqual( b.targetPosition ) ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } @@ -1691,6 +1915,10 @@ setTransformation( UnwrapOperation, WrapOperation, ( a, b ) => { a.howMany = a.howMany - b.howMany + 1; } + if ( b.graveyardPosition && compareArrays( a.position.getParentPath(), b.graveyardPosition.path ) == 'same' ) { + a.howMany = b.howMany; + } + // The default case. // a.position = a.position._getTransformedByWrapOperation( b ); @@ -1709,6 +1937,8 @@ setTransformation( UnwrapOperation, UnwrapOperation, ( a, b, context ) => { a.position = new Position( b.graveyardPosition.root, path ); a.howMany = 0; a.graveyardPosition = Position.createFromPosition( b.graveyardPosition ); + + return [ a ]; } a.position = a.position._getTransformedByUnwrapOperation( b ); @@ -1759,8 +1989,7 @@ function makeMoveOperationsFromRanges( ranges, targetPosition ) { ranges[ j ] = ranges[ j ]._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany )[ 0 ]; } - // targetPosition.stickiness = 'toPrevious'; - targetPosition = targetPosition._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany, true ); + targetPosition = targetPosition._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany ); } return operations; diff --git a/src/model/position.js b/src/model/position.js index ced1b8abc..21ec4d4b9 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -761,6 +761,7 @@ export default class Position { // The first part of a path to combined position is a path to the place where nodes were moved. const combined = Position.createFromPosition( target ); + combined.stickiness = this.stickiness; // Then we have to update the rest of the path. diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index fc1f222f6..d90a85111 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -53,6 +53,12 @@ describe( 'transform', () => { } } + const contextIsStrong = { + aIsStrong: true, + wasUndone: () => false, + getRelation: () => false + }; + describe( 'InsertOperation', () => { let nodeC, nodeD, position; @@ -119,7 +125,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -310,7 +316,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -862,7 +868,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.oldValue = 'xyz'; @@ -1102,7 +1108,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1241,7 +1247,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1567,7 +1573,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.sourcePosition.path = [ 4, 1, 0 ]; @@ -1599,7 +1605,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.sourcePosition.path = [ 4, 1, 1 ]; @@ -1637,7 +1643,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -1675,7 +1681,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1714,7 +1720,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1765,7 +1771,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1805,7 +1811,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1830,7 +1836,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1901,7 +1907,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 3 ); @@ -1952,7 +1958,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -1975,7 +1981,7 @@ describe( 'transform', () => { } ); it( 'should skip context.aIsStrong and be less important than RemoveOperation', () => { - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -2318,7 +2324,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -2611,7 +2617,7 @@ describe( 'transform', () => { const anotherRange = Range.createFromParentsAndOffsets( root, 2, root, 2 ); const transformBy = new MarkerOperation( 'name', oldRange, anotherRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.oldRange = anotherRange; diff --git a/tests/model/operation/transform/attribute.js b/tests/model/operation/transform/attribute.js index 6a5d0f775..3e8610442 100644 --- a/tests/model/operation/transform/attribute.js +++ b/tests/model/operation/transform/attribute.js @@ -795,7 +795,7 @@ describe( 'transform', () => { expectClients( 'FooB<$text bold="true">ar' ); } ); - it.skip( 'element into paragraph #2, then undo', () => { + it( 'element into paragraph #2, then undo', () => { john.setData( 'Foo[Bar]' ); kate.setData( 'Foo[]Bar' ); diff --git a/tests/model/operation/transform/insert.js b/tests/model/operation/transform/insert.js index 635116e14..3eb36286b 100644 --- a/tests/model/operation/transform/insert.js +++ b/tests/model/operation/transform/insert.js @@ -811,7 +811,7 @@ describe( 'transform', () => { expectClients( 'FooBarAbc' ); } ); - it.skip( 'element, then add marker, split and undo with type #2', () => { + it( 'element, then add marker, split and undo with type #2', () => { john.setData( 'Foo[]' ); kate.setData( '[Foo]' ); @@ -845,8 +845,6 @@ describe( 'transform', () => { syncClients(); - // Actual content: - // Ba expectClients( 'FooBarAbc' ); } ); } ); diff --git a/tests/model/operation/transform/marker.js b/tests/model/operation/transform/marker.js index 07b4a2f2b..bb3cf0902 100644 --- a/tests/model/operation/transform/marker.js +++ b/tests/model/operation/transform/marker.js @@ -183,7 +183,6 @@ describe( 'transform', () => { kate.remove(); syncClients(); - expectClients( '' ); john.undo(); @@ -191,8 +190,7 @@ describe( 'transform', () => { syncClients(); - // Actual result for Kate: - // Foo Bar + // Wrong markers transforming. expectClients( '' + 'Foo Bar' + diff --git a/tests/model/operation/transform/move.js b/tests/model/operation/transform/move.js index f54108ef8..a7868903b 100644 --- a/tests/model/operation/transform/move.js +++ b/tests/model/operation/transform/move.js @@ -335,7 +335,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'text in other user\'s selection', () => { + it( 'text in other user\'s selection', () => { john.setData( '[Foo] Bar' ); kate.setData( 'F[]oo Bar' ); @@ -366,7 +366,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'inside moved text', () => { + it( 'inside moved text', () => { john.setData( 'F[oo Ba]rAbc' ); kate.setData( 'Foo[] BarAbc' ); @@ -375,10 +375,6 @@ describe( 'transform', () => { syncClients(); - // Actual result for Kate: - // Fr BaooAbc - // Move operations have wrong targets when they target at same place. - expectClients( 'F' + 'r' + diff --git a/tests/model/operation/transform/remove.js b/tests/model/operation/transform/remove.js index 92cbc090b..8e2fdcf18 100644 --- a/tests/model/operation/transform/remove.js +++ b/tests/model/operation/transform/remove.js @@ -218,7 +218,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'element while removing, then undo', () => { + it( 'element while removing, then undo', () => { john.setData( 'Foo
[Bar]
' ); kate.setData( 'Foo
[]Bar
' ); @@ -226,15 +226,11 @@ describe( 'transform', () => { kate.unwrap(); syncClients(); - expectClients( 'Foo' ); john.undo(); syncClients(); - - // Actual result: - // Foo
expectClients( 'Foo' + 'Bar' @@ -443,7 +439,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'element into paragraph, then undo', () => { + it( 'element into paragraph, then undo', () => { john.setData( 'F[oo]Bar' ); kate.setData( 'Foo[]Bar' ); @@ -451,15 +447,11 @@ describe( 'transform', () => { kate.merge(); syncClients(); - expectClients( 'FBar' ); john.undo(); syncClients(); - - // Actual result: - // FoBar expectClients( 'FooBar' ); } ); diff --git a/tests/model/operation/transform/split.js b/tests/model/operation/transform/split.js index 7f02b915c..e1b46a707 100644 --- a/tests/model/operation/transform/split.js +++ b/tests/model/operation/transform/split.js @@ -90,7 +90,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'intersecting wrap', () => { + it( 'intersecting wrap', () => { john.setData( 'Fo[]o' ); kate.setData( 'F[oo]' ); @@ -98,10 +98,9 @@ describe( 'transform', () => { kate.wrap( 'div' ); syncClients(); - expectClients( - 'F' + - '
oo
' + 'Fo' + + 'o' ); } ); } ); @@ -122,7 +121,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'element in same position', () => { + it( 'element in same position', () => { john.setData( '
[]Foo
' ); kate.setData( '
[]Foo
' ); @@ -130,7 +129,6 @@ describe( 'transform', () => { kate.unwrap(); syncClients(); - expectClients( '
Foo
' ); } ); @@ -285,7 +283,7 @@ describe( 'transform', () => { expectClients( 'FooBar' ); } ); - it.skip( 'element into paragraph #3', () => { + it( 'element into paragraph #3', () => { john.setData( 'Foo[]Bar' ); kate.setData( 'Foo[]Bar' ); @@ -299,13 +297,10 @@ describe( 'transform', () => { kate.undo(); syncClients(); - // Actual content for Kate: - // FooBar - // There is a problem in Merge x Split transform in undo case. expectClients( 'FooBar' ); } ); - it.skip( 'element into paragraph #4', () => { + it( 'element into paragraph #4', () => { john.setData( 'FooB[]ar' ); kate.setData( 'Foo[]Bar' ); @@ -313,14 +308,16 @@ describe( 'transform', () => { kate.merge(); syncClients(); + expectClients( + 'FooB' + + 'ar' + ); john.undo(); kate.undo(); - expectClients( - 'Fo' + - 'oBar' - ); + syncClients(); + expectClients( 'FooBar' ); } ); } ); diff --git a/tests/model/operation/transform/unwrap.js b/tests/model/operation/transform/unwrap.js index 0b14ddd1b..8cba9be85 100644 --- a/tests/model/operation/transform/unwrap.js +++ b/tests/model/operation/transform/unwrap.js @@ -50,7 +50,7 @@ describe( 'transform', () => { expectClients( 'Foo' ); } ); - it.skip( 'the same element, then undo', () => { + it( 'the same element, then undo', () => { john.setData( '
[]Foo
' ); kate.setData( '
[]Foo
' ); @@ -63,9 +63,6 @@ describe( 'transform', () => { john.undo(); syncClients(); - // Actual content for Kate: - // Foo - // Kate has a different order of nodes in graveyard after syncing. expectClients( '
Foo
' ); } ); } ); diff --git a/tests/model/operation/transform/utils.js b/tests/model/operation/transform/utils.js index 8a0557151..016842f9c 100644 --- a/tests/model/operation/transform/utils.js +++ b/tests/model/operation/transform/utils.js @@ -253,16 +253,34 @@ function bufferOperations( operations, client ) { } export function syncClients() { - for ( const client of clients ) { - for ( const item of bufferedOperations ) { - const remoteOperations = item.operations.map( op => OperationFactory.fromJSON( JSON.parse( op ), client.document ) ); - const remoteClient = item.client; + const clientsOperations = {}; + + // For each client, flatten all buffered operations into one set. + for ( const item of bufferedOperations ) { + const name = item.client.name; + + if ( !clientsOperations[ name ] ) { + clientsOperations[ name ] = []; + } + + clientsOperations[ name ].push( ...item.operations ); + } - if ( remoteClient == client ) { + for ( const localClient of clients ) { + for ( const remoteClient of clients ) { + if ( remoteClient == localClient ) { continue; } - const clientOperations = Array.from( client.document.history.getOperations( client.syncedVersion ) ); + const remoteOperationsJson = clientsOperations[ remoteClient.name ]; + + if ( !remoteOperationsJson ) { + continue; + } + + const remoteOperations = remoteOperationsJson.map( op => OperationFactory.fromJSON( JSON.parse( op ), localClient.document ) ); + + const localOperations = Array.from( localClient.document.history.getOperations( localClient.syncedVersion ) ); let remoteOperationsTransformed = null; @@ -271,21 +289,21 @@ export function syncClients() { padWithNoOps: true }; - if ( client.orderNumber < remoteClient.orderNumber ) { - remoteOperationsTransformed = transform.transformSets( clientOperations, remoteOperations, options ).operationsB; + if ( localClient.orderNumber < remoteClient.orderNumber ) { + remoteOperationsTransformed = transform.transformSets( localOperations, remoteOperations, options ).operationsB; } else { - remoteOperationsTransformed = transform.transformSets( remoteOperations, clientOperations, options ).operationsA; + remoteOperationsTransformed = transform.transformSets( remoteOperations, localOperations, options ).operationsA; } - client.editor.model.enqueueChange( 'transparent', writer => { + localClient.editor.model.enqueueChange( 'transparent', writer => { for ( const operation of remoteOperationsTransformed ) { writer.batch.addOperation( operation ); - client.editor.model.applyOperation( operation ); + localClient.editor.model.applyOperation( operation ); } } ); } - client.syncedVersion = client.document.version; + localClient.syncedVersion = localClient.document.version; } bufferedOperations.clear(); diff --git a/tests/model/operation/transform/wrap.js b/tests/model/operation/transform/wrap.js index f7e125980..a24e011be 100644 --- a/tests/model/operation/transform/wrap.js +++ b/tests/model/operation/transform/wrap.js @@ -63,19 +63,24 @@ describe( 'transform', () => { ); } ); - it.skip( 'intersecting wrap #2', () => { - john.setData( '[Foo]' ); - kate.setData( 'F[o]o' ); + it( 'intersecting wrap #2', () => { + john.setData( '[FooBarAbc]' ); + kate.setData( 'Foo[Bar]Abc' ); - john.wrap( 'div' ); + john.wrap( 'blockQuote' ); kate.wrap( 'div' ); syncClients(); - - expectClients( '
Foo
' ); + expectClients( + '
' + + 'Foo' + + 'Bar' + + 'Abc' + + '
' + ); } ); - it.skip( 'intersecting wrap, then undo #1', () => { + it( 'intersecting wrap, then undo #1', () => { john.setData( '[FooBar]Abc' ); kate.setData( 'Foo[BarAbc]' ); @@ -83,12 +88,20 @@ describe( 'transform', () => { kate.wrap( 'div' ); syncClients(); + expectClients( + '
' + + 'Foo' + + 'Bar' + + '
' + + '
' + + 'Abc' + + '
' + ); john.undo(); kate.undo(); syncClients(); - expectClients( 'Foo' + 'Bar' + @@ -96,21 +109,30 @@ describe( 'transform', () => { ); } ); - it.skip( 'intersecting wrap, then undo #2', () => { - john.setData( '[Foo]' ); - kate.setData( 'F[o]o' ); + it( 'intersecting wrap, then undo #2', () => { + john.setData( '[FooBar]Abc' ); + kate.setData( 'Foo[BarAbc]' ); - john.wrap( 'div' ); + john.wrap( 'blockQuote' ); kate.wrap( 'div' ); syncClients(); + expectClients( + '
' + + 'Foo' + + 'Bar' + + '
' + + '
' + + 'Abc' + + '
' + ); john.undo(); kate.undo(); syncClients(); - expectClients( '
Foo
' ); + expectClients( 'FooBarAbc' ); } ); it( 'element and text', () => { @@ -227,14 +249,12 @@ describe( 'transform', () => { kate.merge(); syncClients(); - - expectClients( '
FooBar
' ); + expectClients( 'FooBar
' ); john.undo(); kate.undo(); syncClients(); - expectClients( 'FooBar' ); } ); } ); From 3f1cd6e3096a82707846eb9e20d69efdf736b59e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 13:14:07 +0200 Subject: [PATCH 02/26] Docs: Fixed docs errors. --- src/model/operation/operation.js | 12 ++---------- src/model/position.js | 2 +- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/model/operation/operation.js b/src/model/operation/operation.js index 965f365bc..9ad9e58fc 100644 --- a/src/model/operation/operation.js +++ b/src/model/operation/operation.js @@ -49,14 +49,6 @@ export default class Operation { * @member {String} #type */ - /** - * {@link module:engine/model/operation/operation~Operation Operation} which the operation is a part of. This property is set by the - * {@link module:engine/model/operation/operation~Operation operation} when the operations is added to it by the - * {@link module:engine/model/operation/operation~Operation#addOperation} method. - * - * @member {module:engine/model/operation/operation~Operation} #operation - */ - /** * Creates and returns an operation that has the same parameters as this operation. * @@ -70,8 +62,8 @@ export default class Operation { * operation execution. In other words, it reverses changes done by the original operation. * * Keep in mind that tree model state may change since executing the original operation, - * so reverse operation will be "outdated". In that case you will need to - * {@link module:engine/model/operation/transform~transform} it by all operations that were executed after the original operation. + * so reverse operation will be "outdated". In that case you will need to transform it by + * all operations that were executed after the original operation. * * @method #getReversed * @returns {module:engine/model/operation/operation~Operation} Reversed operation. diff --git a/src/model/position.js b/src/model/position.js index 21ec4d4b9..4a91b11f9 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -22,7 +22,7 @@ import Text from './text'; * * Since position in a model is represented by a {@link module:engine/model/position~Position#root position root} and * {@link module:engine/model/position~Position#path position path} it is possible to create positions placed in non-existing elements. - * This requirement is important for {@link module:engine/model/operation/transform~transform operational transformation}. + * This requirement is important for operational transformation. * * Also, {@link module:engine/model/operation/operation~Operation operations} * kept in {@link module:engine/model/document~Document#history document history} From b00e548bb3f99212fe8caff4e8934a69c2bb8444 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 14:26:10 +0200 Subject: [PATCH 03/26] Fix: Use numbers instead of booleans in Array.sort. --- src/dev-utils/model.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dev-utils/model.js b/src/dev-utils/model.js index 7f1aa5713..8b1529624 100644 --- a/src/dev-utils/model.js +++ b/src/dev-utils/model.js @@ -249,7 +249,7 @@ export function stringify( node, selectionOrPositionOrRange = null, markers = nu if ( markers ) { // To provide stable results, sort markers by name. - markers = Array.from( markers ).sort( ( a, b ) => a.name < b.name ); + markers = Array.from( markers ).sort( ( a, b ) => a.name < b.name ? 1 : -1 ); for ( const marker of markers ) { downcastDispatcher.convertMarkerAdd( marker.name, marker.getRange(), writer ); From 828cc082fd7eda76f6f23c15f818eddf2b692b13 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 15:44:18 +0200 Subject: [PATCH 04/26] Changed: Moved transformation error logging from debug tools to OT code. --- src/dev-utils/enableenginedebug.js | 19 ---------------- src/model/operation/transform.js | 17 +++++++++++++- tests/dev-utils/enableenginedebug.js | 34 ---------------------------- tests/model/operation/transform.js | 23 +++++++++++++++++++ 4 files changed, 39 insertions(+), 54 deletions(-) diff --git a/src/dev-utils/enableenginedebug.js b/src/dev-utils/enableenginedebug.js index a1f990100..45dda9372 100644 --- a/src/dev-utils/enableenginedebug.js +++ b/src/dev-utils/enableenginedebug.js @@ -25,7 +25,6 @@ import MoveOperation from '../model/operation/moveoperation'; import NoOperation from '../model/operation/nooperation'; import RenameOperation from '../model/operation/renameoperation'; import RootAttributeOperation from '../model/operation/rootattributeoperation'; -import transform from '../model/operation/transform'; import Model from '../model/model'; import ModelDocument from '../model/document'; import ModelDocumentFragment from '../model/documentfragment'; @@ -349,24 +348,6 @@ function enableLoggingTools() { `"${ this.key }": ${ JSON.stringify( this.oldValue ) } -> ${ JSON.stringify( this.newValue ) }, ${ this.root.rootName }`; } ); - const _transformTransform = transform.transform; - - sandbox.mock( transform, 'transform', function( a, b, context ) { - let results; - - try { - results = _transformTransform( a, b, context ); - } catch ( e ) { - logger.error( 'Error during operation transformation!' ); - logger.error( a.toString() + ( context.isStrong ? ' (important)' : '' ) ); - logger.error( b.toString() + ( context.isStrong ? '' : ' (important)' ) ); - - throw e; - } - - return results; - } ); - sandbox.mock( ViewText.prototype, 'toString', function() { return `#${ this.data }`; } ); diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index ee05eee28..1a0322d86 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -13,7 +13,9 @@ import UnwrapOperation from './unwrapoperation'; import NoOperation from './nooperation'; import Range from '../range'; import Position from '../position'; + import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays'; +import log from '@ckeditor/ckeditor5-utils/src/log'; const transformations = new Map(); @@ -75,7 +77,20 @@ function updateBaseVersions( operations, baseVersion ) { function transform( a, b, context = { aIsStrong: false, wasUndone: () => false, getRelation: () => null } ) { const transformationFunction = getTransformation( a, b ); - return transformationFunction( a.clone(), b, context ); + try { + return transformationFunction( a.clone(), b, context ); + } catch ( e ) { + log.error( 'Error during operation transformation!' ); + log.error( 'Transformed operation', a ); + log.error( 'Operation transformed by', b ); + log.error( 'context.aIsStrong', context.aIsStrong ); + log.error( 'context.wasUndone( a )', context.wasUndone( a ) ); + log.error( 'context.wasUndone( b )', context.wasUndone( b ) ); + log.error( 'context.getRelation( a, b )', context.getRelation( a, b ) ); + log.error( 'context.getRelation( b, a )', context.getRelation( b, a ) ); + + throw e; + } } function transformSets( operationsA, operationsB, options ) { diff --git a/tests/dev-utils/enableenginedebug.js b/tests/dev-utils/enableenginedebug.js index 4b586ac48..ffbdf8b7e 100644 --- a/tests/dev-utils/enableenginedebug.js +++ b/tests/dev-utils/enableenginedebug.js @@ -22,7 +22,6 @@ import NoOperation from '../../src/model/operation/nooperation'; import RenameOperation from '../../src/model/operation/renameoperation'; import RootAttributeOperation from '../../src/model/operation/rootattributeoperation'; import RemoveOperation from '../../src/model/operation/removeoperation'; -import transform from '../../src/model/operation/transform'; import Model from '../../src/model/model'; import ModelDocumentFragment from '../../src/model/documentfragment'; @@ -668,39 +667,6 @@ describe( 'debug tools', () => { } ); } ); - describe( 'should provide error logging for transformation', () => { - let model, document, root, otherRoot; - - beforeEach( () => { - model = new Model(); - document = model.document; - root = document.createRoot(); - otherRoot = document.createRoot( 'other', 'other' ); - } ); - - it.skip( 'with more important operation A', () => { - const opA = new MoveOperation( ModelPosition.createAt( root, 4 ), 4, ModelPosition.createAt( otherRoot, 4 ), 0 ); - const opB = new InsertOperation( ModelPosition.createAt( root, 0 ), new ModelText( 'a' ), 0 ); - - expect( () => { - transform.transform( opA, opB, { isStrong: true } ); - } ).to.throw( Error ); - expect( error.calledWith( opA.toString() + ' (important)' ) ).to.be.true; - expect( error.calledWith( opB.toString() ) ).to.be.true; - } ); - - it.skip( 'with more important operation B', () => { - const opA = new MoveOperation( ModelPosition.createAt( root, 4 ), 4, ModelPosition.createAt( otherRoot, 4 ), 0 ); - const opB = new InsertOperation( ModelPosition.createAt( root, 0 ), new ModelText( 'a' ), 0 ); - - testUtils.sinon.stub( transform, 'transform' ).throws( new Error() ); - - expect( () => transform.transform( opA, opB, { isStrong: true } ) ).to.throw( Error ); - expect( error.calledWith( opA.toString() ) ).to.be.true; - expect( error.calledWith( opB.toString() + ' (important)' ) ).to.be.true; - } ); - } ); - function expectLog( expectedLogMsg ) { expect( log.calledWithExactly( expectedLogMsg ) ).to.be.true; log.resetHistory(); diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index d90a85111..2ef0c60a8 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -20,6 +20,8 @@ import RemoveOperation from '../../../src/model/operation/removeoperation'; import RenameOperation from '../../../src/model/operation/renameoperation'; import NoOperation from '../../../src/model/operation/nooperation'; +import log from '@ckeditor/ckeditor5-utils/src/log'; + describe( 'transform', () => { let model, doc, root, op, nodeA, nodeB, expected; @@ -59,6 +61,27 @@ describe( 'transform', () => { getRelation: () => false }; + it( 'error logging', () => { + const spy = sinon.spy( log, 'error' ); + + const nodeA = new Node(); + const nodeB = new Node(); + + const position = new Position( root, [ 0 ] ); + + const a = new InsertOperation( position, [ nodeA ], 0 ); + const b = new InsertOperation( position, [ nodeB ], 0 ); + + // Modify an operation so it will throw an error. + a.position = null; + + expect( () => { + transform.transform( a, b ); + } ).to.throw(); + + sinon.assert.called( spy ); + } ); + describe( 'InsertOperation', () => { let nodeC, nodeD, position; From db1299dca4beb7df06dc32fd3beb171b645e2224 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 16:37:50 +0200 Subject: [PATCH 05/26] Docs: Improved error docs. --- src/model/selection.js | 16 +++++++++++++++- src/view/selection.js | 10 +++++++++- tests/model/documentselection.js | 2 +- tests/model/selection.js | 2 +- tests/view/documentselection.js | 4 ++-- tests/view/selection.js | 4 ++-- 6 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/model/selection.js b/src/model/selection.js index 4a2bf2c29..8e1c989ac 100644 --- a/src/model/selection.js +++ b/src/model/selection.js @@ -431,7 +431,21 @@ export default class Selection { // Check whether there is any range in new ranges set that is different than all already added ranges. const anyNewRange = newRanges.some( newRange => { if ( !( newRange instanceof Range ) ) { - throw new CKEditorError( 'model-selection-added-not-range: Trying to add an object that is not an instance of Range.' ); + /** + * Selection range set to an object that is not an instance of {@link module:engine/model/range~Range}. + * + * Only {@link module:engine/model/range~Range} instances can be used to set a selection. + * Common mistakes leading to this error are: + * + * * using DOM `Range` object, + * * incorrect CKEditor 5 installation with multiple `ckeditor5-engine` packages having different versions. + * + * @error model-selection-set-ranges-not-range + */ + throw new CKEditorError( + 'model-selection-set-ranges-not-range: ' + + 'Selection range set to an object that is not an instance of model.Range.' + ); } return this._ranges.every( oldRange => { diff --git a/src/view/selection.js b/src/view/selection.js index bd132dac8..e8b00e75a 100644 --- a/src/view/selection.js +++ b/src/view/selection.js @@ -643,7 +643,15 @@ export default class Selection { */ _addRange( range, isBackward = false ) { if ( !( range instanceof Range ) ) { - throw new CKEditorError( 'view-selection-invalid-range: Invalid Range.' ); + /** + * Selection range set to an object that is not an instance of {@link module:engine/view/range~Range}. + * + * @error view-selection-add-range-not-range + */ + throw new CKEditorError( + 'view-selection-add-range-not-range: ' + + 'Selection range set to an object that is not an instance of view.Range' + ); } this._pushRange( range ); diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index 8a5a89852..299b06913 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -334,7 +334,7 @@ describe( 'DocumentSelection', () => { it( 'should throw an error when range is invalid', () => { expect( () => { selection._setTo( [ { invalid: 'range' } ] ); - } ).to.throw( CKEditorError, /model-selection-added-not-range/ ); + } ).to.throw( CKEditorError, /model-selection-set-ranges-not-range/ ); } ); it( 'should log a warning when trying to set selection to the graveyard', () => { diff --git a/tests/model/selection.js b/tests/model/selection.js index 4439e9012..630420c5c 100644 --- a/tests/model/selection.js +++ b/tests/model/selection.js @@ -633,7 +633,7 @@ describe( 'Selection', () => { it( 'should throw an error when range is invalid', () => { expect( () => { selection._setRanges( [ { invalid: 'range' } ] ); - } ).to.throw( CKEditorError, /model-selection-added-not-range/ ); + } ).to.throw( CKEditorError, /model-selection-set-ranges-not-range/ ); } ); it( 'should remove all ranges and add given ranges', () => { diff --git a/tests/view/documentselection.js b/tests/view/documentselection.js index 4525b544e..78ca8a11d 100644 --- a/tests/view/documentselection.js +++ b/tests/view/documentselection.js @@ -113,7 +113,7 @@ describe( 'DocumentSelection', () => { expect( () => { // eslint-disable-next-line no-new new DocumentSelection( [ { invalid: 'range' } ] ); - } ).to.throw( CKEditorError, 'view-selection-invalid-range: Invalid Range.' ); + } ).to.throw( CKEditorError, /view-selection-add-range-not-range/ ); } ); it( 'should throw an error when ranges intersects', () => { @@ -1004,7 +1004,7 @@ describe( 'DocumentSelection', () => { it( 'should throw an error when range is invalid', () => { expect( () => { documentSelection._setTo( [ { invalid: 'range' } ] ); - } ).to.throw( CKEditorError, 'view-selection-invalid-range: Invalid Range.' ); + } ).to.throw( CKEditorError, /view-selection-add-range-not-range/ ); } ); it( 'should throw when range is intersecting with already added range', () => { diff --git a/tests/view/selection.js b/tests/view/selection.js index 910f20123..6c503e9d1 100644 --- a/tests/view/selection.js +++ b/tests/view/selection.js @@ -113,7 +113,7 @@ describe( 'Selection', () => { expect( () => { // eslint-disable-next-line no-new new Selection( [ { invalid: 'range' } ] ); - } ).to.throw( CKEditorError, 'view-selection-invalid-range: Invalid Range.' ); + } ).to.throw( CKEditorError, /view-selection-add-range-not-range/ ); } ); it( 'should throw an error when ranges intersects', () => { @@ -878,7 +878,7 @@ describe( 'Selection', () => { it( 'should throw an error when range is invalid', () => { expect( () => { selection.setTo( [ { invalid: 'range' } ] ); - } ).to.throw( CKEditorError, 'view-selection-invalid-range: Invalid Range.' ); + } ).to.throw( CKEditorError, /view-selection-add-range-not-range/ ); } ); it( 'should throw when range is intersecting with already added range', () => { From 63adf00e85dc4c432c426dc5ed33d175a9f6ce3e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 16:54:04 +0200 Subject: [PATCH 06/26] Changed: Improved WrapOperation implementation. --- src/model/operation/wrapoperation.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/model/operation/wrapoperation.js b/src/model/operation/wrapoperation.js index 5de2c6a68..7b7cf533b 100644 --- a/src/model/operation/wrapoperation.js +++ b/src/model/operation/wrapoperation.js @@ -148,7 +148,10 @@ export default class WrapOperation extends Operation { const targetPosition = new Position( this.position.root, targetPath ); if ( this.element ) { - _insert( insertPosition, this.element._clone() ); + const originalElement = this.element; + this.element = this.element._clone(); + + _insert( insertPosition, originalElement ); } else { _move( Range.createFromPositionAndShift( this.graveyardPosition, 1 ), insertPosition ); } From aae2bea5b7737a15e65acf42a3ed9f703d3de47f Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 30 Jul 2018 12:33:05 +0200 Subject: [PATCH 07/26] Changed: Schema#getValidRanges now returns flat ranges, compatible with the new AttributeOperation. --- src/model/schema.js | 76 ++++++++++++++----- tests/model/schema.js | 172 +++++++++++++++++++++++++----------------- 2 files changed, 161 insertions(+), 87 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index 61cca5ff2..da6a66aa3 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -668,35 +668,60 @@ export default class Schema { * @returns {Array.} Ranges in which the attribute is allowed. */ getValidRanges( ranges, attribute ) { - const validRanges = []; + ranges = convertToMinimalFlatRanges( ranges ); + const result = []; for ( const range of ranges ) { - let last = range.start; - let from = range.start; - const to = range.end; - - for ( const value of range.getWalker() ) { - if ( !this.checkAttribute( value.item, attribute ) ) { - if ( !from.isEqual( last ) ) { - validRanges.push( new Range( from, last ) ); - } + const validRanges = this._getValidRangesForRange( range, attribute ); - from = value.nextPosition; - } + result.push( ...validRanges ); + } + + return result; + } + + /** + * Takes a flat range and an attribute name. Traverses the range recursively and deeply to find and return all ranges + * inside the given range on which the attribute can be applied. + * + * This is a helper function for {@link ~Schema#getValidRanges}. + * + * @private + * @param {module:engine/model/range~Range} range Range to process. + * @param {String} attribute The name of the attribute to check. + * @returns {Array.} Ranges in which the attribute is allowed. + */ + _getValidRangesForRange( range, attribute ) { + const result = []; - last = value.nextPosition; + let start = range.start; + let end = range.start; + + for ( const item of range.getItems( { shallow: true } ) ) { + if ( item.is( 'element' ) ) { + result.push( ...this._getValidRangesForRange( Range.createIn( item ), attribute ) ); } - if ( from && !from.isEqual( to ) ) { - validRanges.push( new Range( from, to ) ); + if ( !this.checkAttribute( item, attribute ) ) { + if ( !start.isEqual( end ) ) { + result.push( new Range( start, end ) ); + } + + start = Position.createAfter( item ); } + + end = Position.createAfter( item ); } - return validRanges; + if ( !start.isEqual( end ) ) { + result.push( new Range( start, end ) ); + } + + return result; } /** - * Basing on given the `position`, finds and returns a {@link module:engine/model/range~Range range} which is + * Basing on given `position`, finds and returns a {@link module:engine/model/range~Range range} which is * nearest to that `position` and is a correct range for selection. * * The correct selection range might be collapsed when it is located in a position where the text node can be placed. @@ -1572,3 +1597,20 @@ function* combineWalkers( backward, forward ) { } } } + +// Takes an array of non-intersecting ranges. For each of them gets minimal flat ranges covering that range and returns +// all those minimal flat ranges. +// +// @param {Array.} ranges Ranges to process. +// @returns {Array.} Minimal flat ranges of given `ranges`. +function convertToMinimalFlatRanges( ranges ) { + const result = []; + + for ( const range of ranges ) { + const minimal = range.getMinimalFlatRanges(); + + result.push( ...minimal ); + } + + return result; +} diff --git a/tests/model/schema.js b/tests/model/schema.js index 55c01041b..fd85549a7 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1106,8 +1106,7 @@ describe( 'Schema', () => { } ); describe( 'getValidRanges()', () => { - const attribute = 'bold'; - let model, doc, root, schema, ranges; + let model, doc, root, schema; beforeEach( () => { model = new Model(); @@ -1116,112 +1115,145 @@ describe( 'Schema', () => { root = doc.createRoot(); schema.register( 'p', { inheritAllFrom: '$block' } ); - schema.register( 'h1', { inheritAllFrom: '$block' } ); - schema.register( 'img', { - allowWhere: '$text' - } ); + schema.register( 'img', { allowWhere: '$text' } ); - schema.addAttributeCheck( ( ctx, attributeName ) => { - // Allow 'bold' on p>$text. - if ( ctx.endsWith( 'p $text' ) && attributeName == 'bold' ) { - return true; - } + // This is a "hack" to allow setting any ranges in `setData` util. + schema.extend( '$text', { allowIn: '$root' } ); + } ); - // Allow 'bold' on $root>p. - if ( ctx.endsWith( '$root p' ) && attributeName == 'bold' ) { - return true; - } - } ); + function test( input, attribute, output ) { + setData( model, input ); - // Parse data string to model. - const parsedModel = parse( '

foobar

', model.schema, { context: [ root.name ] } ); + const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute ); + const sel = new Selection( validRanges ); - // Set parsed model data to prevent selection post-fixer from running. - model.change( writer => { - writer.insert( parsedModel, root ); - } ); + expect( stringify( root, sel ) ).to.equal( output ); + } - ranges = [ Range.createOn( root.getChild( 0 ) ) ]; + it( 'should return a range with p for an attribute allowed only on p', () => { + schema.extend( 'p', { allowAttributes: 'foo' } ); + + test( + '[

foobar

]', + 'foo', + '[

foobar

]' + ); } ); - it( 'should return unmodified ranges when attribute is allowed on each item (text is not allowed in img)', () => { - schema.extend( 'img', { allowAttributes: 'bold' } ); + it( 'should return ranges on text nodes for an attribute allowed only on text', () => { + schema.extend( '$text', { allowAttributes: 'bold' } ); - expect( schema.getValidRanges( ranges, attribute ) ).to.deep.equal( ranges ); + test( + '[

foobar

]', + 'bold', + '

[foo][bar]

' + ); } ); - it( 'should return unmodified ranges when attribute is allowed on each item (text is allowed in img)', () => { - schema.extend( 'img', { allowAttributes: 'bold' } ); - schema.extend( '$text', { allowIn: 'img' } ); + it( 'should return a range on img for an attribute allowed only on img', () => { + schema.extend( 'img', { allowAttributes: 'src' } ); - expect( schema.getValidRanges( ranges, attribute ) ).to.deep.equal( ranges ); + test( + '[

foobar

]', + 'src', + '

foo[]bar

' + ); } ); - it( 'should return two ranges when attribute is not allowed on one item', () => { + it( 'should return a range containing all children for an attribute allowed on all children', () => { + schema.extend( '$text', { allowAttributes: 'bold' } ); schema.extend( 'img', { allowAttributes: 'bold' } ); - schema.extend( '$text', { allowIn: 'img' } ); - setData( model, '

[fooxxxbar]

' ); + test( + '[

foobar

]', + 'bold', + '

[foobar]

' + ); + } ); - const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute ); - const sel = new Selection( validRanges ); + it( 'should return a range with p and a range on all children for an attribute allowed on p and its children', () => { + schema.extend( 'p', { allowAttributes: 'foo' } ); + schema.extend( '$text', { allowAttributes: 'foo' } ); + schema.extend( 'img', { allowAttributes: 'foo' } ); - expect( stringify( root, sel ) ).to.equal( '

[foo]xxx[bar]

' ); - } ); + setData( model, '[

foobar

]' ); - it( 'should return three ranges when attribute is not allowed on one element but is allowed on its child', () => { - schema.extend( '$text', { allowIn: 'img' } ); + const validRanges = schema.getValidRanges( doc.selection.getRanges(), 'foo' ); - schema.addAttributeCheck( ( ctx, attributeName ) => { - // Allow 'bold' on img>$text. - if ( ctx.endsWith( 'img $text' ) && attributeName == 'bold' ) { - return true; - } - } ); + expect( validRanges.length ).to.equal( 2 ); - setData( model, '

[fooxxxbar]

' ); + expect( validRanges[ 0 ].start.path ).to.deep.equal( [ 0, 0 ] ); + expect( validRanges[ 0 ].end.path ).to.deep.equal( [ 0, 7 ] ); - const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute ); - const sel = new Selection( validRanges ); + expect( validRanges[ 1 ].start.path ).to.deep.equal( [ 0 ] ); + expect( validRanges[ 1 ].end.path ).to.deep.equal( [ 1 ] ); + } ); + + it( 'should not break a range if children are not allowed to have the attribute', () => { + schema.extend( 'p', { allowAttributes: 'foo' } ); - expect( stringify( root, sel ) ).to.equal( '

[foo][xxx][bar]

' ); + test( + '[

foo

bar

]', + 'foo', + '[

foo

bar

]' + ); } ); - it( 'should not leak beyond the given ranges', () => { - setData( model, '

[foobar]x[barfoo]

' ); + it( 'should search deeply', () => { + schema.extend( '$text', { allowAttributes: 'bold', allowIn: 'img' } ); - const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute ); - const sel = new Selection( validRanges ); + test( + '[

fooxxxbar

]', + 'bold', + '

[foo][xxx][bar]

' + ); + } ); + + it( 'should work with multiple ranges', () => { + schema.extend( '$text', { allowAttributes: 'bold' } ); - expect( stringify( root, sel ) ).to.equal( '

[foo][bar]x[bar][foo]

' ); + test( + '[

a

b

]

c

[d]

', + 'bold', + '

[a]

[b]

c

[d]

' + ); } ); - it( 'should correctly handle a range which ends in a disallowed position', () => { - schema.extend( '$text', { allowIn: 'img' } ); + it( 'should work with non-flat ranges', () => { + schema.extend( '$text', { allowAttributes: 'bold' } ); - setData( model, '

[foobar]bom

' ); + test( + '[

a

b

c]

d

', + 'bold', + '

[a]

[b]

[c]

d

' + ); + } ); - const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute ); - const sel = new Selection( validRanges ); + it( 'should not leak beyond the given ranges', () => { + schema.extend( '$text', { allowAttributes: 'bold' } ); - expect( stringify( root, sel ) ).to.equal( '

[foo]barbom

' ); + test( + '[

foo

b]a[r

x]yz

', + 'bold', + '

[foo]

[b]a[r]

[x]yz

' + ); } ); - it( 'should split range into two ranges and omit disallowed element', () => { + it( 'should correctly handle a range which ends in a disallowed position', () => { + schema.extend( '$text', { allowAttributes: 'bold', allowIn: 'img' } ); + + // Disallow bold on text inside image. schema.addAttributeCheck( ( ctx, attributeName ) => { - // Disallow 'bold' on p>img. - if ( ctx.endsWith( 'p img' ) && attributeName == 'bold' ) { + if ( ctx.endsWith( 'img $text' ) && attributeName == 'bold' ) { return false; } } ); - const result = schema.getValidRanges( ranges, attribute ); - - expect( result ).to.length( 2 ); - expect( result[ 0 ].start.path ).to.members( [ 0 ] ); - expect( result[ 0 ].end.path ).to.members( [ 0, 3 ] ); - expect( result[ 1 ].start.path ).to.members( [ 0, 4 ] ); - expect( result[ 1 ].end.path ).to.members( [ 1 ] ); + test( + '[

fooxx]xbar

', + 'bold', + '

[foo]xxxbar

' + ); } ); } ); From 7b6057bddb1461c5e0adf161dfd2ff93e3fe72ff Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 30 Jul 2018 13:22:15 +0200 Subject: [PATCH 08/26] Changed: Remove `ReinsertOperation` and `RemoveOperation`. --- src/model/operation/detachoperation.js | 1 - src/model/operation/insertoperation.js | 6 +- src/model/operation/moveoperation.js | 6 + src/model/operation/operationfactory.js | 4 - src/model/operation/reinsertoperation.js | 76 -------- src/model/operation/removeoperation.js | 60 ------- src/model/operation/transform.js | 55 +----- src/model/writer.js | 5 +- tests/dev-utils/enableenginedebug.js | 5 +- tests/model/differ.js | 3 +- tests/model/documentselection.js | 11 +- tests/model/operation/insertoperation.js | 6 +- tests/model/operation/moveoperation.js | 20 ++- tests/model/operation/reinsertoperation.js | 157 ----------------- tests/model/operation/removeoperation.js | 191 --------------------- tests/model/operation/transform.js | 19 +- tests/model/range.js | 9 +- tests/model/writer.js | 2 +- 18 files changed, 57 insertions(+), 579 deletions(-) delete mode 100644 src/model/operation/reinsertoperation.js delete mode 100644 src/model/operation/removeoperation.js delete mode 100644 tests/model/operation/reinsertoperation.js delete mode 100644 tests/model/operation/removeoperation.js diff --git a/src/model/operation/detachoperation.js b/src/model/operation/detachoperation.js index 5441991fa..6da08e0d2 100644 --- a/src/model/operation/detachoperation.js +++ b/src/model/operation/detachoperation.js @@ -60,7 +60,6 @@ export default class DetachOperation extends Operation { if ( this.sourcePosition.root.document ) { /** * Cannot detach document node. - * Use {@link module:engine/model/operation/removeoperation~RemoveOperation remove operation} instead. * * @error detach-operation-on-document-node */ diff --git a/src/model/operation/insertoperation.js b/src/model/operation/insertoperation.js index a67577758..f5aa6a9dc 100644 --- a/src/model/operation/insertoperation.js +++ b/src/model/operation/insertoperation.js @@ -10,7 +10,7 @@ import Operation from './operation'; import Position from '../position'; import NodeList from '../nodelist'; -import RemoveOperation from './removeoperation'; +import MoveOperation from './moveoperation'; import { _insert, _normalizeNodes } from './utils'; import Text from '../text'; import Element from '../element'; @@ -87,13 +87,13 @@ export default class InsertOperation extends Operation { /** * See {@link module:engine/model/operation/operation~Operation#getReversed `Operation#getReversed()`}. * - * @returns {module:engine/model/operation/removeoperation~RemoveOperation} + * @returns {module:engine/model/operation/moveoperation~MoveOperation} */ getReversed() { const graveyard = this.position.root.document.graveyard; const gyPosition = new Position( graveyard, [ 0 ] ); - return new RemoveOperation( this.position, this.nodes.maxOffset, gyPosition, this.baseVersion + 1 ); + return new MoveOperation( this.position, this.nodes.maxOffset, gyPosition, this.baseVersion + 1 ); } /** diff --git a/src/model/operation/moveoperation.js b/src/model/operation/moveoperation.js index 3672cee28..6ba3e841c 100644 --- a/src/model/operation/moveoperation.js +++ b/src/model/operation/moveoperation.js @@ -64,6 +64,12 @@ export default class MoveOperation extends Operation { * @inheritDoc */ get type() { + if ( this.targetPosition.root.rootName == '$graveyard' ) { + return 'remove'; + } else if ( this.sourcePosition.root.rootName == '$graveyard' ) { + return 'reinsert'; + } + return 'move'; } diff --git a/src/model/operation/operationfactory.js b/src/model/operation/operationfactory.js index d30dbc7ae..b5c02c4e3 100644 --- a/src/model/operation/operationfactory.js +++ b/src/model/operation/operationfactory.js @@ -13,8 +13,6 @@ import MarkerOperation from '../operation/markeroperation'; import MoveOperation from '../operation/moveoperation'; import NoOperation from '../operation/nooperation'; import Operation from '../operation/operation'; -import ReinsertOperation from '../operation/reinsertoperation'; -import RemoveOperation from '../operation/removeoperation'; import RenameOperation from '../operation/renameoperation'; import RootAttributeOperation from '../operation/rootattributeoperation'; import SplitOperation from '../operation/splitoperation'; @@ -29,8 +27,6 @@ operations[ MarkerOperation.className ] = MarkerOperation; operations[ MoveOperation.className ] = MoveOperation; operations[ NoOperation.className ] = NoOperation; operations[ Operation.className ] = Operation; -operations[ ReinsertOperation.className ] = ReinsertOperation; -operations[ RemoveOperation.className ] = RemoveOperation; operations[ RenameOperation.className ] = RenameOperation; operations[ RootAttributeOperation.className ] = RootAttributeOperation; operations[ SplitOperation.className ] = SplitOperation; diff --git a/src/model/operation/reinsertoperation.js b/src/model/operation/reinsertoperation.js deleted file mode 100644 index 270884e7f..000000000 --- a/src/model/operation/reinsertoperation.js +++ /dev/null @@ -1,76 +0,0 @@ -/** - * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -/** - * @module engine/model/operation/reinsertoperation - */ - -import MoveOperation from './moveoperation'; -import RemoveOperation from './removeoperation'; -import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; - -/** - * Operation to reinsert previously removed nodes back to the non-graveyard root. This operation acts like - * {@link module:engine/model/operation/moveoperation~MoveOperation} but it returns - * {@link module:engine/model/operation/removeoperation~RemoveOperation} when reversed - * and fires different change event. - */ -export default class ReinsertOperation extends MoveOperation { - /** - * Position where nodes will be re-inserted. - * - * @type {module:engine/model/position~Position} - */ - get position() { - return this.targetPosition; - } - - /** - * @param {module:engine/model/position~Position} pos - */ - set position( pos ) { - this.targetPosition = pos; - } - - /** - * @inheritDoc - */ - get type() { - return 'reinsert'; - } - - /** - * See {@link module:engine/model/operation/operation~Operation#getReversed `Operation#getReversed()`}. - * - * @returns {module:engine/model/operation/removeoperation~RemoveOperation} - */ - getReversed() { - const newTargetPosition = this.sourcePosition._getTransformedByInsertion( this.targetPosition, this.howMany ); - - return new RemoveOperation( this.getMovedRangeStart(), this.howMany, newTargetPosition, this.baseVersion + 1 ); - } - - /** - * @inheritDoc - */ - _validate() { - super._validate(); - - if ( !this.sourcePosition.root.document ) { - throw new CKEditorError( 'reinsert-operation-on-detached-item: Cannot reinsert detached item.' ); - } - - if ( !this.targetPosition.root.document ) { - throw new CKEditorError( 'reinsert-operation-to-detached-parent: Cannot reinsert item to detached parent.' ); - } - } - - /** - * @inheritDoc - */ - static get className() { - return 'engine.model.operation.ReinsertOperation'; - } -} diff --git a/src/model/operation/removeoperation.js b/src/model/operation/removeoperation.js deleted file mode 100644 index ba26e3629..000000000 --- a/src/model/operation/removeoperation.js +++ /dev/null @@ -1,60 +0,0 @@ -/** - * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -/** - * @module engine/model/operation/removeoperation - */ - -import MoveOperation from './moveoperation'; -import ReinsertOperation from './reinsertoperation'; -import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; - -/** - * Operation to remove a range of nodes. - */ -export default class RemoveOperation extends MoveOperation { - /** - * @inheritDoc - */ - get type() { - return 'remove'; - } - - /** - * See {@link module:engine/model/operation/operation~Operation#getReversed `Operation#getReversed()`}. - * - * @returns {module:engine/model/operation/reinsertoperation~ReinsertOperation|module:engine/model/operation/nooperation~NoOperation} - */ - getReversed() { - const newTargetPosition = this.sourcePosition._getTransformedByInsertion( this.targetPosition, this.howMany ); - - return new ReinsertOperation( this.getMovedRangeStart(), this.howMany, newTargetPosition, this.baseVersion + 1 ); - } - - /** - * @inheritDoc - */ - _validate() { - super._validate(); - - if ( !this.sourcePosition.root.document ) { - /** - * Item that is going to be removed needs to be a {@link module:engine/model/document~Document document} child. - * To remove Item from detached document fragment use - * {@link module:engine/model/operation/detachoperation~DetachOperation DetachOperation}. - * - * @error remove-operation-on-detached-item - */ - throw new CKEditorError( 'remove-operation-on-detached-item: Cannot remove detached item.' ); - } - } - - /** - * @inheritDoc - */ - static get className() { - return 'engine.model.operation.RemoveOperation'; - } -} diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index f0a9dba73..4517ccee2 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -3,8 +3,6 @@ import AttributeOperation from './attributeoperation'; import RenameOperation from './renameoperation'; import MarkerOperation from './markeroperation'; import MoveOperation from './moveoperation'; -import RemoveOperation from './removeoperation'; -import ReinsertOperation from './reinsertoperation'; import RootAttributeOperation from './rootattributeoperation'; import MergeOperation from './mergeoperation'; import SplitOperation from './splitoperation'; @@ -29,32 +27,10 @@ function setTransformation( OperationA, OperationB, transformationFunction ) { } function getTransformation( a, b ) { - const OperationA = a.constructor; - const OperationB = b.constructor; + const aGroup = transformations.get( a.constructor ); - const aGroups = new Set(); - const aGroup = transformations.get( OperationA ); - - if ( aGroup ) { - aGroups.add( aGroup ); - } - - for ( const Operation of transformations.keys() ) { - if ( a instanceof Operation ) { - aGroups.add( transformations.get( Operation ) ); - } - } - - for ( const group of aGroups ) { - if ( group.has( OperationB ) ) { - return group.get( OperationB ); - } - - for ( const Operation of group.keys() ) { - if ( b instanceof Operation ) { - return group.get( Operation ); - } - } + if ( aGroup && aGroup.has( b.constructor ) ) { + return aGroup.get( b.constructor ); } return noUpdateTransformation; @@ -707,7 +683,7 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { // const removedRange = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); - if ( b instanceof RemoveOperation && !context.wasUndone( b ) ) { + if ( b.type == 'remove' && !context.wasUndone( b ) ) { if ( a.deletionPosition.hasSameParentAs( b.sourcePosition ) && removedRange.containsPosition( a.sourcePosition ) ) { return getNoOp(); } @@ -926,9 +902,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 instanceof RemoveOperation && !( b instanceof RemoveOperation ) ) { + if ( a.type == 'remove' && b.type != 'remove' ) { aIsStrong = true; - } else if ( !( a instanceof RemoveOperation ) && b instanceof RemoveOperation ) { + } else if ( a.type != 'remove' && b.type == 'remove' ) { aIsStrong = false; } @@ -1050,7 +1026,7 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { const movedRange = Range.createFromPositionAndShift( a.sourcePosition, a.howMany ); if ( b.deletionPosition.hasSameParentAs( a.sourcePosition ) && movedRange.containsPosition( b.sourcePosition ) ) { - if ( a instanceof RemoveOperation ) { + if ( a.type == 'remove' ) { // Case 1: The element to remove got merged. // Merge operation does support merging elements which are not siblings. So it would not be a problem // from technical point of view. However, if the element was removed, the intention of the user @@ -1767,25 +1743,12 @@ function makeMoveOperationsFromRanges( ranges, targetPosition ) { } function makeMoveOperation( range, targetPosition ) { - // We want to keep correct operation class. - let OperationClass; - - if ( targetPosition.root.rootName == '$graveyard' ) { - OperationClass = RemoveOperation; - } else if ( range.start.root.rootName == '$graveyard' ) { - OperationClass = ReinsertOperation; - } else { - OperationClass = MoveOperation; - } - targetPosition.stickiness = 'toNone'; - const result = new OperationClass( + return new MoveOperation( range.start, range.end.offset - range.start.offset, targetPosition, - 0 // Is corrected anyway later. + 0 ); - - return result; } diff --git a/src/model/writer.js b/src/model/writer.js index 41000fa43..8b6e00043 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -12,7 +12,6 @@ import DetachOperation from './operation/detachoperation'; import InsertOperation from './operation/insertoperation'; import MarkerOperation from './operation/markeroperation'; import MoveOperation from './operation/moveoperation'; -import RemoveOperation from './operation/removeoperation'; import RenameOperation from './operation/renameoperation'; import RootAttributeOperation from './operation/rootattributeoperation'; import SplitOperation from './operation/splitoperation'; @@ -1330,7 +1329,7 @@ function applyMarkerOperation( writer, name, oldRange, newRange, affectsData ) { model.applyOperation( operation ); } -// Creates `RemoveOperation` or `DetachOperation` that removes `howMany` nodes starting from `position`. +// Creates `MoveOperation` or `DetachOperation` that removes `howMany` nodes starting from `position`. // The operation will be applied on given model instance and added to given operation instance. // // @private @@ -1345,7 +1344,7 @@ function applyRemoveOperation( position, howMany, batch, model ) { const doc = model.document; const graveyardPosition = new Position( doc.graveyard, [ 0 ] ); - operation = new RemoveOperation( position, howMany, graveyardPosition, doc.version ); + operation = new MoveOperation( position, howMany, graveyardPosition, doc.version ); } else { operation = new DetachOperation( position, howMany ); } diff --git a/tests/dev-utils/enableenginedebug.js b/tests/dev-utils/enableenginedebug.js index 4b586ac48..c3eec4f3c 100644 --- a/tests/dev-utils/enableenginedebug.js +++ b/tests/dev-utils/enableenginedebug.js @@ -21,7 +21,6 @@ import MoveOperation from '../../src/model/operation/moveoperation'; import NoOperation from '../../src/model/operation/nooperation'; import RenameOperation from '../../src/model/operation/renameoperation'; import RootAttributeOperation from '../../src/model/operation/rootattributeoperation'; -import RemoveOperation from '../../src/model/operation/removeoperation'; import transform from '../../src/model/operation/transform'; import Model from '../../src/model/model'; import ModelDocumentFragment from '../../src/model/documentfragment'; @@ -532,8 +531,8 @@ describe( 'debug tools', () => { model.applyOperation( insert ); const graveyard = modelDoc.graveyard; - const remove = new RemoveOperation( ModelPosition.createAt( modelRoot, 1 ), 2, ModelPosition.createAt( graveyard, 0 ), 1 ); - model.applyOperation( remove ); + const move = new MoveOperation( ModelPosition.createAt( modelRoot, 1 ), 2, ModelPosition.createAt( graveyard, 0 ), 1 ); + model.applyOperation( move ); } ); log.resetHistory(); diff --git a/tests/model/differ.js b/tests/model/differ.js index d04e3b54c..549146cae 100644 --- a/tests/model/differ.js +++ b/tests/model/differ.js @@ -10,7 +10,6 @@ import Position from '../../src/model/position'; import Range from '../../src/model/range'; import InsertOperation from '../../src/model/operation/insertoperation'; -import RemoveOperation from '../../src/model/operation/removeoperation'; import MoveOperation from '../../src/model/operation/moveoperation'; import RenameOperation from '../../src/model/operation/renameoperation'; import AttributeOperation from '../../src/model/operation/attributeoperation'; @@ -1617,7 +1616,7 @@ describe( 'Differ', () => { function remove( sourcePosition, howMany ) { const targetPosition = Position.createAt( doc.graveyard, doc.graveyard.maxOffset ); - const operation = new RemoveOperation( sourcePosition, howMany, targetPosition, doc.version ); + const operation = new MoveOperation( sourcePosition, howMany, targetPosition, doc.version ); model.applyOperation( operation ); } diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index 8a5a89852..1f12e172d 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -13,7 +13,6 @@ import LiveRange from '../../src/model/liverange'; import DocumentSelection from '../../src/model/documentselection'; import InsertOperation from '../../src/model/operation/insertoperation'; import MoveOperation from '../../src/model/operation/moveoperation'; -import RemoveOperation from '../../src/model/operation/removeoperation'; import AttributeOperation from '../../src/model/operation/attributeoperation'; import SplitOperation from '../../src/model/operation/splitoperation'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; @@ -1131,12 +1130,12 @@ describe( 'DocumentSelection', () => { } ); } ); - describe( 'RemoveOperation', () => { + describe( 'MoveOperation to graveyard', () => { it( 'fix selection range if it ends up in graveyard #1', () => { selection._setTo( new Position( root, [ 1, 3 ] ) ); model.applyOperation( - new RemoveOperation( + new MoveOperation( new Position( root, [ 1, 2 ] ), 2, new Position( doc.graveyard, [ 0 ] ), @@ -1151,7 +1150,7 @@ describe( 'DocumentSelection', () => { selection._setTo( [ new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 1, 4 ] ) ) ] ); model.applyOperation( - new RemoveOperation( + new MoveOperation( new Position( root, [ 1, 2 ] ), 2, new Position( doc.graveyard, [ 0 ] ), @@ -1166,7 +1165,7 @@ describe( 'DocumentSelection', () => { selection._setTo( [ new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ) ] ); model.applyOperation( - new RemoveOperation( + new MoveOperation( new Position( root, [ 1 ] ), 2, new Position( doc.graveyard, [ 0 ] ), @@ -1179,7 +1178,7 @@ describe( 'DocumentSelection', () => { it( 'fix selection range if it ends up in graveyard #4 - whole content removed', () => { model.applyOperation( - new RemoveOperation( + new MoveOperation( new Position( root, [ 0 ] ), 3, new Position( doc.graveyard, [ 0 ] ), diff --git a/tests/model/operation/insertoperation.js b/tests/model/operation/insertoperation.js index 6f59cf3dc..92d7749a1 100644 --- a/tests/model/operation/insertoperation.js +++ b/tests/model/operation/insertoperation.js @@ -7,7 +7,7 @@ import Model from '../../../src/model/model'; import NodeList from '../../../src/model/nodelist'; import Element from '../../../src/model/element'; import InsertOperation from '../../../src/model/operation/insertoperation'; -import RemoveOperation from '../../../src/model/operation/removeoperation'; +import MoveOperation from '../../../src/model/operation/moveoperation'; import Position from '../../../src/model/position'; import Text from '../../../src/model/text'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; @@ -107,7 +107,7 @@ describe( 'InsertOperation', () => { expect( root.getChild( 0 ).data ).to.equal( 'fooxbar' ); } ); - it( 'should create a RemoveOperation as a reverse', () => { + it( 'should create a MoveOperation as a reverse', () => { const position = new Position( root, [ 0 ] ); const operation = new InsertOperation( position, @@ -117,7 +117,7 @@ describe( 'InsertOperation', () => { const reverse = operation.getReversed(); - expect( reverse ).to.be.an.instanceof( RemoveOperation ); + expect( reverse ).to.be.an.instanceof( MoveOperation ); expect( reverse.baseVersion ).to.equal( 1 ); expect( reverse.sourcePosition.isEqual( position ) ).to.be.true; expect( reverse.howMany ).to.equal( 7 ); diff --git a/tests/model/operation/moveoperation.js b/tests/model/operation/moveoperation.js index d2ff1edde..6e3a86a00 100644 --- a/tests/model/operation/moveoperation.js +++ b/tests/model/operation/moveoperation.js @@ -12,23 +12,27 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import { jsonParseStringify } from '../../../tests/model/_utils/utils'; describe( 'MoveOperation', () => { - let model, doc, root; + let model, doc, root, gy; beforeEach( () => { model = new Model(); doc = model.document; root = doc.createRoot(); + gy = doc.graveyard; } ); it( 'should have proper type', () => { - const op = new MoveOperation( - new Position( root, [ 0, 0 ] ), - 1, - new Position( root, [ 1, 0 ] ), - doc.version - ); + const move = new MoveOperation( new Position( root, [ 0, 0 ] ), 1, new Position( root, [ 1, 0 ] ), 0 ); + expect( move.type ).to.equal( 'move' ); + + const remove1 = new MoveOperation( new Position( root, [ 0, 0 ] ), 1, new Position( gy, [ 0 ] ), 0 ); + expect( remove1.type ).to.equal( 'remove' ); + + const remove2 = new MoveOperation( new Position( gy, [ 0 ] ), 1, new Position( gy, [ 1 ] ), 0 ); + expect( remove2.type ).to.equal( 'remove' ); - expect( op.type ).to.equal( 'move' ); + const reinsert = new MoveOperation( new Position( gy, [ 0 ] ), 1, new Position( root, [ 0, 0 ] ), 0 ); + expect( reinsert.type ).to.equal( 'reinsert' ); } ); it( 'should move from one node to another', () => { diff --git a/tests/model/operation/reinsertoperation.js b/tests/model/operation/reinsertoperation.js deleted file mode 100644 index da927dfc2..000000000 --- a/tests/model/operation/reinsertoperation.js +++ /dev/null @@ -1,157 +0,0 @@ -/** - * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -import Model from '../../../src/model/model'; -import ReinsertOperation from '../../../src/model/operation/reinsertoperation'; -import RemoveOperation from '../../../src/model/operation/removeoperation'; -import MoveOperation from '../../../src/model/operation/moveoperation'; -import Position from '../../../src/model/position'; -import DocumentFragment from '../../../src/model/documentfragment'; -import Element from '../../../src/model/element'; -import Text from '../../../src/model/text'; -import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; -import { jsonParseStringify } from '../../../tests/model/_utils/utils'; - -describe( 'ReinsertOperation', () => { - let model, doc, root, graveyard, operation, graveyardPosition, rootPosition; - - beforeEach( () => { - model = new Model(); - doc = model.document; - root = doc.createRoot(); - graveyard = doc.graveyard; - - graveyardPosition = new Position( graveyard, [ 0 ] ); - rootPosition = new Position( root, [ 0 ] ); - - operation = new ReinsertOperation( - graveyardPosition, - 2, - rootPosition, - doc.version - ); - } ); - - it( 'should have position property equal to the position where node will be reinserted', () => { - expect( operation.position.isEqual( rootPosition ) ).to.be.true; - - // Setting also works: - operation.position = new Position( root, [ 1 ] ); - expect( operation.position.isEqual( new Position( root, [ 1 ] ) ) ).to.be.true; - } ); - - it( 'should have proper type', () => { - expect( operation.type ).to.equal( 'reinsert' ); - } ); - - it( 'should extend MoveOperation class', () => { - expect( operation ).to.be.instanceof( MoveOperation ); - } ); - - it( 'should create ReinsertOperation with same parameters when cloned', () => { - const clone = operation.clone(); - - expect( clone ).to.be.instanceof( ReinsertOperation ); - expect( clone.sourcePosition.isEqual( operation.sourcePosition ) ).to.be.true; - expect( clone.targetPosition.isEqual( operation.targetPosition ) ).to.be.true; - expect( clone.howMany ).to.equal( operation.howMany ); - expect( clone.baseVersion ).to.equal( operation.baseVersion ); - } ); - - it( 'should create RemoveOperation as a reverse', () => { - graveyard._appendChild( new Element( 'x' ) ); - - const reverse = operation.getReversed(); - - expect( reverse ).to.be.an.instanceof( RemoveOperation ); - expect( reverse.baseVersion ).to.equal( 1 ); - expect( reverse.howMany ).to.equal( 2 ); - expect( reverse.sourcePosition.isEqual( rootPosition ) ).to.be.true; - expect( reverse.targetPosition.isEqual( graveyardPosition ) ).to.be.true; - } ); - - it( 'should create correct RemoveOperation when reversed if target position was in graveyard', () => { - const operation = new ReinsertOperation( new Position( doc.graveyard, [ 0 ] ), 1, new Position( doc.graveyard, [ 3 ] ), 0 ); - const reverse = operation.getReversed(); - - expect( reverse.sourcePosition.path ).to.deep.equal( [ 2 ] ); - expect( reverse.targetPosition.path ).to.deep.equal( [ 0 ] ); - } ); - - it( 'should undo reinsert set of nodes by applying reverse operation', () => { - const reverse = operation.getReversed(); - - graveyard._insertChild( 0, new Text( 'xx' ) ); - - model.applyOperation( operation ); - - expect( doc.version ).to.equal( 1 ); - expect( root.maxOffset ).to.equal( 2 ); - expect( graveyard.maxOffset ).to.equal( 0 ); - - model.applyOperation( reverse ); - - expect( doc.version ).to.equal( 2 ); - expect( root.maxOffset ).to.equal( 0 ); - expect( graveyard.maxOffset ).to.equal( 2 ); - } ); - - describe( '_validate()', () => { - it( 'should throw when target position is not in the document', () => { - const docFrag = new DocumentFragment(); - - graveyard._insertChild( 0, new Text( 'xx' ) ); - - operation = new ReinsertOperation( - graveyardPosition, - 1, - Position.createAt( docFrag ), - doc.version - ); - - expect( () => { - operation._validate(); - } ).to.throw( CKEditorError, /^reinsert-operation-to-detached-parent/ ); - } ); - - it( 'should throw when source position is not in the document', () => { - const docFrag = new DocumentFragment( new Text( 'xx' ) ); - - operation = new ReinsertOperation( - Position.createAt( docFrag ), - 1, - rootPosition, - doc.version - ); - - expect( () => { - operation._validate(); - } ).to.throw( CKEditorError, /^reinsert-operation-on-detached-item/ ); - } ); - } ); - - describe( 'toJSON', () => { - it( 'should create proper json object', () => { - const serialized = jsonParseStringify( operation ); - - expect( serialized ).to.deep.equal( { - __className: 'engine.model.operation.ReinsertOperation', - baseVersion: 0, - howMany: 2, - sourcePosition: jsonParseStringify( operation.sourcePosition ), - targetPosition: jsonParseStringify( operation.targetPosition ) - } ); - } ); - } ); - - describe( 'fromJSON', () => { - it( 'should create proper ReinsertOperation from json object', () => { - const serialized = jsonParseStringify( operation ); - const deserialized = ReinsertOperation.fromJSON( serialized, doc ); - - expect( deserialized ).to.deep.equal( operation ); - } ); - } ); -} ); diff --git a/tests/model/operation/removeoperation.js b/tests/model/operation/removeoperation.js deleted file mode 100644 index da9bf72b1..000000000 --- a/tests/model/operation/removeoperation.js +++ /dev/null @@ -1,191 +0,0 @@ -/** - * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -import Model from '../../../src/model/model'; -import ReinsertOperation from '../../../src/model/operation/reinsertoperation'; -import RemoveOperation from '../../../src/model/operation/removeoperation'; -import MoveOperation from '../../../src/model/operation/moveoperation'; -import Position from '../../../src/model/position'; -import DocumentFragment from '../../../src/model/documentfragment'; -import Element from '../../../src/model/element'; -import Text from '../../../src/model/text'; -import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; -import { jsonParseStringify } from '../../../tests/model/_utils/utils'; - -describe( 'RemoveOperation', () => { - let model, doc, root, graveyard; - - beforeEach( () => { - model = new Model(); - doc = model.document; - root = doc.createRoot(); - graveyard = doc.graveyard; - } ); - - it( 'should have proper type', () => { - const op = new RemoveOperation( - new Position( root, [ 2 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - doc.version - ); - - expect( op.type ).to.equal( 'remove' ); - } ); - - it( 'should extend MoveOperation class', () => { - const operation = new RemoveOperation( - new Position( root, [ 2 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - doc.version - ); - - expect( operation ).to.be.instanceof( MoveOperation ); - } ); - - it( 'should be able to remove set of nodes and append them to graveyard root', () => { - root._insertChild( 0, new Text( 'fozbar' ) ); - - model.applyOperation( - new RemoveOperation( - new Position( root, [ 2 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - doc.version - ) - ); - - expect( doc.version ).to.equal( 1 ); - expect( root.maxOffset ).to.equal( 4 ); - expect( root.getChild( 0 ).data ).to.equal( 'foar' ); - - expect( graveyard.maxOffset ).to.equal( 2 ); - expect( graveyard.getChild( 0 ).data ).to.equal( 'zb' ); - } ); - - it( 'should create RemoveOperation with same parameters when cloned', () => { - const pos = new Position( root, [ 2 ] ); - - const operation = new RemoveOperation( pos, 2, new Position( doc.graveyard, [ 0 ] ), doc.version ); - const clone = operation.clone(); - - expect( clone ).to.be.instanceof( RemoveOperation ); - expect( clone.sourcePosition.isEqual( pos ) ).to.be.true; - expect( clone.targetPosition.isEqual( operation.targetPosition ) ).to.be.true; - expect( clone.howMany ).to.equal( operation.howMany ); - expect( clone.baseVersion ).to.equal( operation.baseVersion ); - } ); - - it( 'should create ReinsertOperation when reversed', () => { - const position = new Position( root, [ 0 ] ); - const operation = new RemoveOperation( position, 2, new Position( doc.graveyard, [ 0 ] ), 0 ); - const reverse = operation.getReversed(); - - expect( reverse ).to.be.an.instanceof( ReinsertOperation ); - expect( reverse.baseVersion ).to.equal( 1 ); - expect( reverse.howMany ).to.equal( 2 ); - expect( reverse.sourcePosition.isEqual( operation.targetPosition ) ).to.be.true; - expect( reverse.targetPosition.isEqual( position ) ).to.be.true; - } ); - - it( 'should create correct ReinsertOperation when reversed if source range was in graveyard', () => { - const operation = new RemoveOperation( new Position( doc.graveyard, [ 2 ] ), 1, new Position( doc.graveyard, [ 0 ] ), 0 ); - const reverse = operation.getReversed(); - - expect( reverse.sourcePosition.path ).to.deep.equal( [ 0 ] ); - expect( reverse.targetPosition.path ).to.deep.equal( [ 3 ] ); - } ); - - it( 'should undo remove set of nodes by applying reverse operation', () => { - const position = new Position( root, [ 0 ] ); - const operation = new RemoveOperation( position, 3, new Position( doc.graveyard, [ 0 ] ), 0 ); - const reverse = operation.getReversed(); - - root._insertChild( 0, new Text( 'bar' ) ); - - model.applyOperation( operation ); - - expect( doc.version ).to.equal( 1 ); - expect( root.maxOffset ).to.equal( 0 ); - - model.applyOperation( reverse ); - - expect( doc.version ).to.equal( 2 ); - expect( root.maxOffset ).to.equal( 3 ); - expect( root.getChild( 0 ).data ).to.equal( 'bar' ); - } ); - - it( 'should properly remove a node that is already in a graveyard', () => { - doc.graveyard._appendChild( [ new Element( 'x' ), new Element( 'y' ), new Element( 'z' ) ] ); - - const position = new Position( doc.graveyard, [ 2 ] ); - const operation = new RemoveOperation( position, 1, new Position( doc.graveyard, [ 0 ] ), 0 ); - - model.applyOperation( operation ); - - expect( doc.graveyard.childCount ).to.equal( 3 ); - expect( doc.graveyard.getChild( 0 ).name ).to.equal( 'z' ); - expect( doc.graveyard.getChild( 1 ).name ).to.equal( 'x' ); - expect( doc.graveyard.getChild( 2 ).name ).to.equal( 'y' ); - } ); - - describe( '_validate()', () => { - it( 'should throw when is executed on detached item', () => { - const docFrag = new DocumentFragment(); - const item = new Element( 'foo' ); - - docFrag._appendChild( [ item ] ); - - const op = new RemoveOperation( - new Position( docFrag, [ 0 ] ), - 1, - new Position( doc.graveyard, [ 0 ] ), - doc.version - ); - - expect( () => { - op._validate(); - } ).to.throw( CKEditorError, /^remove-operation-on-detached-item/ ); - } ); - } ); - - describe( 'toJSON', () => { - it( 'should create proper json object', () => { - const op = new RemoveOperation( - new Position( root, [ 2 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - doc.version - ); - - const serialized = jsonParseStringify( op ); - - expect( serialized ).to.deep.equal( { - __className: 'engine.model.operation.RemoveOperation', - baseVersion: 0, - howMany: 2, - sourcePosition: jsonParseStringify( op.sourcePosition ), - targetPosition: jsonParseStringify( op.targetPosition ) - } ); - } ); - } ); - - describe( 'fromJSON', () => { - it( 'should create proper RemoveOperation from json object', () => { - const op = new RemoveOperation( - new Position( root, [ 2 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - doc.version - ); - - const serialized = jsonParseStringify( op ); - const deserialized = RemoveOperation.fromJSON( serialized, doc ); - - expect( deserialized ).to.deep.equal( op ); - } ); - } ); -} ); diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index fc1f222f6..0087152bd 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -16,7 +16,6 @@ import AttributeOperation from '../../../src/model/operation/attributeoperation' import RootAttributeOperation from '../../../src/model/operation/rootattributeoperation'; import MarkerOperation from '../../../src/model/operation/markeroperation'; import MoveOperation from '../../../src/model/operation/moveoperation'; -import RemoveOperation from '../../../src/model/operation/removeoperation'; import RenameOperation from '../../../src/model/operation/renameoperation'; import NoOperation from '../../../src/model/operation/nooperation'; @@ -688,7 +687,7 @@ describe( 'transform', () => { } ); } ); - describe( 'by RemoveOperation', () => { + describe( 'by MoveOperation to graveyard', () => { beforeEach( () => { start = new Position( doc.graveyard, [ 2, 0 ] ); end = new Position( doc.graveyard, [ 2, 4 ] ); @@ -701,7 +700,7 @@ describe( 'transform', () => { } ); it( 'remove operation inserted elements before attribute operation range: increment path', () => { - const transformBy = new RemoveOperation( + const transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( doc.graveyard, [ 0 ] ), @@ -721,7 +720,7 @@ describe( 'transform', () => { } ); it( 'remove operation inserted elements after attribute operation range: do nothing', () => { - const transformBy = new RemoveOperation( + const transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( doc.graveyard, [ 0 ] ), @@ -1962,11 +1961,11 @@ describe( 'transform', () => { } ); } ); - describe( 'by RemoveOperation', () => { + describe( 'by MoveOperation to graveyard', () => { let transformBy; beforeEach( () => { - transformBy = new RemoveOperation( + transformBy = new MoveOperation( Position.createFromPosition( op.sourcePosition ), op.howMany, new Position( doc.graveyard, [ 0 ] ), @@ -1974,7 +1973,7 @@ describe( 'transform', () => { ); } ); - it( 'should skip context.aIsStrong and be less important than RemoveOperation', () => { + it( 'should skip context.aIsStrong and be less important than MoveOperation', () => { const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); expect( transOp.length ).to.equal( 1 ); @@ -2020,10 +2019,10 @@ describe( 'transform', () => { } ); } ); - describe( 'RemoveOperation', () => { + describe( 'MoveOperation to graveyard', () => { describe( 'by MoveOperation', () => { it( 'should force removing content even if was less important', () => { - const op = new RemoveOperation( new Position( root, [ 8 ] ), 2, new Position( doc.graveyard, [ 0 ] ), 0 ); + const op = new MoveOperation( new Position( root, [ 8 ] ), 2, new Position( doc.graveyard, [ 0 ] ), 0 ); const targetPosition = Position.createFromPosition( op.targetPosition ); @@ -2036,7 +2035,7 @@ describe( 'transform', () => { expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { - type: RemoveOperation, + type: MoveOperation, howMany: 2, sourcePosition, targetPosition diff --git a/tests/model/range.js b/tests/model/range.js index feea61f39..2f7bfbcac 100644 --- a/tests/model/range.js +++ b/tests/model/range.js @@ -12,7 +12,6 @@ import TreeWalker from '../../src/model/treewalker'; import AttributeOperation from '../../src/model/operation/attributeoperation'; import InsertOperation from '../../src/model/operation/insertoperation'; import MoveOperation from '../../src/model/operation/moveoperation'; -import RemoveOperation from '../../src/model/operation/removeoperation'; import RenameOperation from '../../src/model/operation/renameoperation'; import MergeOperation from '../../src/model/operation/mergeoperation'; import SplitOperation from '../../src/model/operation/splitoperation'; @@ -898,10 +897,10 @@ describe( 'Range', () => { } ); } ); - describe( 'by RemoveOperation', () => { + describe( 'by MoveOperation to graveyard', () => { it( 'remove before range', () => { const start = new Position( root, [ 0 ] ); - const op = new RemoveOperation( start, 2, gyPos, 1 ); + const op = new MoveOperation( start, 2, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); @@ -910,7 +909,7 @@ describe( 'Range', () => { it( 'remove intersecting with range', () => { const start = new Position( root, [ 4 ] ); - const op = new RemoveOperation( start, 2, gyPos, 1 ); + const op = new MoveOperation( start, 2, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); @@ -922,7 +921,7 @@ describe( 'Range', () => { it( 'remove inside the range', () => { const start = new Position( root, [ 3 ] ); - const op = new RemoveOperation( start, 2, gyPos, 1 ); + const op = new MoveOperation( start, 2, gyPos, 1 ); const transformed = range.getTransformedByOperation( op ); diff --git a/tests/model/writer.js b/tests/model/writer.js index e31a5d0a4..fd466b90e 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -1545,7 +1545,7 @@ describe( 'Writer', () => { expect( batch.operations.length ).to.equal( 2 ); } ); - it( 'should use RemoveOperation', () => { + it( 'should use MoveOperation to graveyard', () => { batch = new Batch(); remove( div ); From a120c857ba78033579cf3b84f0d7436d543d0179 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 30 Jul 2018 14:27:10 +0200 Subject: [PATCH 09/26] Fixed: Fixed results of `Range#getTransformedByMerge` for edge cases. --- src/model/operation/transform.js | 6 ++++ src/model/position.js | 2 ++ src/model/range.js | 33 +++++++++++++++++---- tests/model/operation/transform/marker.js | 35 +++++++++++++++++++++++ 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index f0a9dba73..dc8e59dc0 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -1156,6 +1156,12 @@ setTransformation( RenameOperation, InsertOperation, ( a, b ) => { } ); setTransformation( RenameOperation, MergeOperation, ( a, b ) => { + if ( a.position.isEqual( b.deletionPosition ) ) { + a.position = Position.createFromPosition( b.graveyardPosition ); + + return [ a ]; + } + a.position = a.position._getTransformedByMergeOperation( b ); return [ a ]; diff --git a/src/model/position.js b/src/model/position.js index 7b3cfa5a4..9c5f06057 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -563,6 +563,8 @@ export default class Position { // Above happens during OT when the merged element is moved before the merged-to element. pos = pos._getTransformedByDeletion( operation.deletionPosition, 1 ); } + } else if ( this.isEqual( operation.deletionPosition ) ) { + pos = Position.createFromPosition( operation.deletionPosition ); } else { pos = this._getTransformedByMove( operation.deletionPosition, operation.graveyardPosition, 1 ); } diff --git a/src/model/range.js b/src/model/range.js index 4dc795e90..30746b6af 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -493,16 +493,37 @@ export default class Range { } _getTransformedByMergeOperation( operation ) { - const start = this.start._getTransformedByMergeOperation( operation ); - const end = this.end._getTransformedByMergeOperation( operation ); + let start = this.start._getTransformedByMergeOperation( operation ); + let end = this.end._getTransformedByMergeOperation( operation ); if ( start.isAfter( end ) ) { - // This happens in the following case: + // This happens in the following two, similar cases: // - //

abc

{

x}yz

->

abcx}yz

{ - //

abcx{yz

} + // Case 1: Range start is directly before merged node. + // Resulting range should include only nodes from the merged element: // - return new Range( end, start ); + // Before:

aa

{

b}b

cc

+ // Merge:

aab}b

{

cc

+ // Fix:

aa{b}b

cc

+ // + // Case 2: Range start is not directly before merged node. + // Result should include all nodes that were in the original range. + // + // Before:

aa

{

cc

b}b

+ // Merge:

aab}b

{

cc

+ // Fix:

aa{bb

cc

} + // + // The range is expanded by an additional `b` letter but it is better than dropping the whole `cc` paragraph. + // + if ( !operation.deletionPosition.isEqual( start ) ) { + // Case 2. + end = operation.deletionPosition; + } + + // In both cases start is at the end of the merge-to element. + start = operation.targetPosition; + + return new Range( start, end ); } return new Range( start, end ); diff --git a/tests/model/operation/transform/marker.js b/tests/model/operation/transform/marker.js index 07b4a2f2b..0b2fe0c78 100644 --- a/tests/model/operation/transform/marker.js +++ b/tests/model/operation/transform/marker.js @@ -789,6 +789,41 @@ describe( 'transform', () => { '' ); } ); + + it( 'only marker end is inside merged element #1', () => { + john.setData( 'Foo[B]ar' ); + kate.setData( 'Foo[]Bar' ); + + john.setMarker( 'm1' ); + kate.merge(); + + syncClients(); + + expectClients( 'FooBar' ); + } ); + + it( 'only marker end is inside merged element #2', () => { + john.setData( 'Foo[]Bar' ); + kate.setData( 'Foo[]Bar' ); + + kate.split(); + + syncClients(); + expectClients( 'FooBar' ); + + john.setSelection( [ 1 ] ); + john.insert( 'Xyz' ); + + syncClients(); + expectClients( 'FooXyzBar' ); + + john.setSelection( [ 1 ], [ 2, 1 ] ); + john.setMarker( 'm1' ); + kate.undo(); + + syncClients(); + expectClients( 'FooBarXyz' ); + } ); } ); describe( 'by rename', () => { From 977c81ef7e24e61f2443a1330fa3f10488cf5747 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 30 Jul 2018 15:46:07 +0200 Subject: [PATCH 10/26] Changed: `context.wasUndone` changed from function to properties. --- src/model/operation/transform.js | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index f0a9dba73..12f8db766 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -113,13 +113,17 @@ function transformSets( operationsA, operationsB, options ) { const opA = opsA[ k ]; const opB = opsB[ l ]; - context.aIsStrong = true; - const newOpA = transform( opA, opB, context ); - - context.aIsStrong = false; - const newOpB = transform( opB, opA, context ); - - delete context.aIsStrong; + const newOpA = transform( opA, opB, { + aIsStrong: true, + aWasUndone: context.wasUndone( opA ), + bWasUndone: context.wasUndone( opB ) + } ); + + const newOpB = transform( opB, opA, { + aIsStrong: false, + aWasUndone: context.wasUndone( opB ), + bWasUndone: context.wasUndone( opA ), + } ); if ( options.useContext ) { updateOriginalOperation( context, opA, newOpA ); @@ -269,7 +273,7 @@ setTransformation( AttributeOperation, InsertOperation, ( a, b ) => { //

Fo[zb]ar

// // New text with `highlight="red"` is typed: - //

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

+ //

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 @@ -707,7 +711,7 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { // const removedRange = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); - if ( b instanceof RemoveOperation && !context.wasUndone( b ) ) { + if ( b instanceof RemoveOperation && !context.bWasUndone ) { if ( a.deletionPosition.hasSameParentAs( b.sourcePosition ) && removedRange.containsPosition( a.sourcePosition ) ) { return getNoOp(); } @@ -1057,7 +1061,7 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { // deleting it was to have it all deleted. From user experience point of view, moving back the // removed nodes might be unexpected. This means that in this scenario we will reverse merging and remove the element. // - if ( !context.wasUndone( a ) ) { + if ( !context.aWasUndone ) { return [ b.getReversed(), a ]; } } else { From e30d09d5d1a626a2a960befcc2a81044976d7284 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 27 Jul 2018 11:20:11 +0200 Subject: [PATCH 11/26] Changed: Introduced relations in OT. --- src/model/operation/transform.js | 303 ++++++++++++++++--- src/model/position.js | 1 + tests/model/operation/transform.js | 42 +-- tests/model/operation/transform/attribute.js | 2 +- tests/model/operation/transform/insert.js | 4 +- tests/model/operation/transform/marker.js | 4 +- tests/model/operation/transform/move.js | 8 +- tests/model/operation/transform/remove.js | 12 +- tests/model/operation/transform/split.js | 27 +- tests/model/operation/transform/unwrap.js | 5 +- tests/model/operation/transform/utils.js | 42 ++- tests/model/operation/transform/wrap.js | 52 +++- 12 files changed, 377 insertions(+), 125 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 12f8db766..c8249cdcf 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -72,7 +72,7 @@ function updateBaseVersions( operations, baseVersion ) { return operations; } -function transform( a, b, context = { aIsStrong: false } ) { +function transform( a, b, context = { aIsStrong: false, wasUndone: () => false, getRelation: () => null } ) { const transformationFunction = getTransformation( a, b ); return transformationFunction( a.clone(), b, context ); @@ -128,6 +128,8 @@ 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 ); @@ -180,6 +182,7 @@ function initializeContext( opsA, opsB, options ) { } context.document = options.document; + context.relations = new Map(); context.wasUndone = function( op ) { if ( !options.useContext ) { @@ -191,9 +194,151 @@ function initializeContext( opsA, opsB, options ) { return this.document.history.isUndoneOperation( originalOp ); }; + context.getRelation = function( opA, opB ) { + if ( !options.useContext ) { + return null; + } + + const origB = this.originalOperations.get( opB ); + const undoneB = this.document.history.getUndoneOperation( origB ); + + if ( !undoneB ) { + return null; + } + + const origA = this.originalOperations.get( opA ); + const relationsA = this.relations.get( origA ); + + if ( relationsA ) { + return relationsA.get( undoneB ) || null; + } + + return null; + }; + return context; } +function updateRelations( context, opA, opB ) { + switch ( opA.constructor ) { + case MoveOperation: + case RemoveOperation: + case ReinsertOperation: { + switch ( opB.constructor ) { + case MergeOperation: { + if ( opA.targetPosition.isEqual( opB.sourcePosition ) || opB.movedRange.containsPosition( opA.targetPosition ) ) { + setRelation( context, opA, opB, 'insertAtSource' ); + setRelation( context, opB, opA, 'splitBefore' ); + } + + break; + } + + case MoveOperation: { + if ( opA.targetPosition.isEqual( opB.sourcePosition ) || opA.targetPosition.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'insertBefore' ); + setRelation( context, opB, opA, 'insertAfter' ); + } + + break; + } + + case UnwrapOperation: { + const isInside = opA.targetPosition.hasSameParentAs( opB.targetPosition ); + + if ( isInside ) { + setRelation( context, opA, opB, 'insertInside' ); + } + + break; + } + } + + break; + } + + case SplitOperation: { + switch ( opB.constructor ) { + case MergeOperation: { + if ( opA.position.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'splitBefore' ); + setRelation( context, opB, opA, 'splitAfter' ); + } + + break; + } + + case MoveOperation: { + if ( opA.position.isEqual( opB.sourcePosition ) || opA.position.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'splitBefore' ); + setRelation( context, opB, opA, 'insertAtSource' ); + } + + break; + } + + case UnwrapOperation: { + const isInside = opA.position.hasSameParentAs( opB.position ); + + if ( isInside ) { + setRelation( context, opA, opB, 'splitInside' ); + } + + break; + } + } + + break; + } + + case InsertOperation: { + switch ( opB.constructor ) { + case MergeOperation: { + if ( opA.position.isEqual( opB.sourcePosition ) || opB.movedRange.containsPosition( opA.position ) ) { + setRelation( context, opA, opB, 'insertAtSource' ); + } + + break; + } + + case MoveOperation: { + if ( opA.position.isEqual( opB.sourcePosition ) || opA.position.isBefore( opB.sourcePosition ) ) { + setRelation( context, opA, opB, 'insertBefore' ); + } + + break; + } + + case UnwrapOperation: { + const isInside = opA.position.hasSameParentAs( opB.position ); + + if ( isInside ) { + setRelation( context, opA, opB, 'insertInside' ); + } + + break; + } + } + + break; + } + } +} + +function setRelation( context, opA, opB, relation ) { + const origA = context.originalOperations.get( opA ); + const origB = context.originalOperations.get( opB ); + + let relationsA = context.relations.get( origA ); + + if ( !relationsA ) { + relationsA = new Map(); + context.relations.set( origA, relationsA ); + } + + relationsA.set( origB, relation ); +} + function updateOriginalOperation( context, oldOp, newOps ) { const originalOp = context.originalOperations.get( oldOp ); @@ -349,8 +494,12 @@ setTransformation( AttributeOperation, MergeOperation, ( a, b ) => { // Case 1: Attribute change on the merged element. In this case, the merged element was moved to graveyard. // An additional attribute operation that will change the (re)moved element needs to be generated. + // Do it only, if there is more than one element in attribute range. If there is only one element, + // it will be handled by the default algorithm. // - if ( a.range.start.hasSameParentAs( b.deletionPosition ) ) { + const howMany = a.range.end.offset - a.range.start.offset; + + if ( howMany > 1 && a.range.start.hasSameParentAs( b.deletionPosition ) ) { if ( a.range.containsPosition( b.deletionPosition ) || a.range.start.isEqual( b.deletionPosition ) ) { ranges.push( Range.createFromPositionAndShift( b.graveyardPosition, 1 ) ); } @@ -545,13 +694,23 @@ setTransformation( InsertOperation, InsertOperation, ( a, b, context ) => { return [ a ]; } ); -setTransformation( InsertOperation, MoveOperation, ( a, b ) => { +setTransformation( InsertOperation, MoveOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.targetPosition ) && context.getRelation( a, b ) == 'insertBefore' ) { + return [ a ]; + } + a.position = a.position._getTransformedByMoveOperation( b ); return [ a ]; } ); -setTransformation( InsertOperation, SplitOperation, ( a, b ) => { +setTransformation( InsertOperation, SplitOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'insertAtSource' ) { + a.position = b.moveTargetPosition; + + return [ a ]; + } + a.position = a.position._getTransformedBySplitOperation( b ); return [ a ]; @@ -563,7 +722,13 @@ setTransformation( InsertOperation, MergeOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( InsertOperation, WrapOperation, ( a, b ) => { +setTransformation( InsertOperation, WrapOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'insertInside' ) { + a.position = b.targetPosition; + + return [ a ]; + } + a.position = a.position._getTransformedByWrapOperation( b ); return [ a ]; @@ -720,7 +885,7 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { a.sourcePosition = a.sourcePosition._getTransformedByMoveOperation( b ); a.targetPosition = a.targetPosition._getTransformedByMoveOperation( b ); - if ( !a.graveyardPosition.isEqual( b.targetPosition ) || !context.aIsStrong ) { + if ( !a.graveyardPosition.isEqual( b.targetPosition ) ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } @@ -751,9 +916,9 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => { // There is one exception though - when the split operation is a result of undo. In those cases, it is needed // to keep `targetPosition` intact, so the nodes are returned to the correct element. // - if ( a.targetPosition.isEqual( b.position ) && b.graveyardPosition ) { - return [ a ]; - } + // if ( a.targetPosition.isEqual( b.position ) && b.graveyardPosition ) { + // return [ a ]; + // } a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b ); @@ -818,14 +983,14 @@ setTransformation( MergeOperation, UnwrapOperation, ( a, b, context ) => { // ----------------------- -setTransformation( MoveOperation, InsertOperation, ( a, b ) => { +setTransformation( MoveOperation, InsertOperation, ( a, b, context ) => { const moveRange = Range.createFromPositionAndShift( a.sourcePosition, a.howMany ); const transformed = moveRange._getTransformedByInsertOperation( b, false )[ 0 ]; a.sourcePosition = transformed.start; a.howMany = transformed.end.offset - transformed.start.offset; - if ( !a.targetPosition.isEqual( b.position ) ) { + if ( !a.targetPosition.isEqual( b.position ) || context.getRelation( b, a ) == 'insertBefore' ) { a.targetPosition = a.targetPosition._getTransformedByInsertOperation( b ); } @@ -842,14 +1007,23 @@ 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 aIsStrong = context.aIsStrong || context.getRelation( a, b ) == 'insertBefore'; // `a.targetPosition` could be affected by the `b` operation. We will transform it. - const newTargetPosition = a.targetPosition._getTransformedByMove( - b.sourcePosition, - b.targetPosition, - b.howMany - ); + let newTargetPosition; + + if ( a.targetPosition.isEqual( b.targetPosition ) && aIsStrong ) { + newTargetPosition = a.targetPosition._getTransformedByDeletion( + b.sourcePosition, + b.howMany + ); + } else { + newTargetPosition = a.targetPosition._getTransformedByMove( + b.sourcePosition, + b.targetPosition, + b.howMany + ); + } // // Special case #1 + mirror. @@ -997,7 +1171,7 @@ setTransformation( MoveOperation, MoveOperation, ( a, b, context ) => { return makeMoveOperationsFromRanges( ranges, newTargetPosition ); } ); -setTransformation( MoveOperation, SplitOperation, ( a, b ) => { +setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => { const newTargetPosition = a.targetPosition._getTransformedBySplitOperation( b ); // Case 1: Last element in the moved range got split. @@ -1032,8 +1206,8 @@ setTransformation( MoveOperation, SplitOperation, ( a, b ) => { rightRange = rightRange._getTransformedBySplitOperation( b ); const ranges = [ - rightRange, - Range.createFromPositionAndShift( moveRange.start, b.position.offset - moveRange.start.offset ) + new Range( moveRange.start, b.position ), + rightRange ]; return makeMoveOperationsFromRanges( ranges, newTargetPosition ); @@ -1047,6 +1221,10 @@ setTransformation( MoveOperation, SplitOperation, ( a, b ) => { a.howMany = transformed.end.offset - transformed.start.offset; a.targetPosition = newTargetPosition; + if ( a.targetPosition.isEqual( b.position ) && context.getRelation( a, b ) == 'insertAtSource' ) { + a.targetPosition = b.targetPosition; + } + return [ a ]; } ); @@ -1085,7 +1263,7 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => { return [ a ]; } ); -setTransformation( MoveOperation, WrapOperation, ( a, b ) => { +setTransformation( MoveOperation, WrapOperation, ( a, b, context ) => { const moveRange = Range.createFromPositionAndShift( a.sourcePosition, a.howMany ); const newTargetPosition = a.targetPosition._getTransformedByWrapOperation( b ); @@ -1137,6 +1315,10 @@ setTransformation( MoveOperation, WrapOperation, ( a, b ) => { a.howMany = transformed.end.offset - transformed.start.offset; a.targetPosition = newTargetPosition; + if ( a.targetPosition.isEqual( b.position ) && context.getRelation( a, b ) == 'insertInside' ) { + a.targetPosition = b.targetPosition; + } + return [ a ]; } ); @@ -1239,7 +1421,11 @@ setTransformation( RootAttributeOperation, RootAttributeOperation, ( a, b, conte // ----------------------- -setTransformation( SplitOperation, InsertOperation, ( a, b ) => { +setTransformation( SplitOperation, InsertOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( b, a ) == 'insertAtSource' ) { + return [ a ]; + } + a.position = a.position._getTransformedByInsertOperation( b ); return [ a ]; @@ -1255,7 +1441,7 @@ setTransformation( SplitOperation, MergeOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, MoveOperation, ( a, b ) => { +setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { if ( a.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } @@ -1283,6 +1469,12 @@ setTransformation( SplitOperation, MoveOperation, ( a, b ) => { return [ a ]; } + if ( a.position.isEqual( b.targetPosition ) && context.getRelation( a, b ) == 'splitBefore' ) { + a.position = a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); + + return [ a ]; + } + // The default case. // a.position = a.position._getTransformedByMoveOperation( b ); @@ -1290,7 +1482,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, SplitOperation, ( a, b ) => { +setTransformation( SplitOperation, SplitOperation, ( a, b, context ) => { if ( a.position.isEqual( b.position ) ) { if ( !a.graveyardPosition && !b.graveyardPosition ) { return getNoOp(); @@ -1299,6 +1491,8 @@ setTransformation( SplitOperation, SplitOperation, ( a, b ) => { if ( a.graveyardPosition && b.graveyardPosition && a.graveyardPosition.isEqual( b.graveyardPosition ) ) { return getNoOp(); } + } else if ( a.position.isEqual( b.insertionPosition ) && context.getRelation( a, b ) == 'splitBefore' ) { + return [ a ]; } else { a.position = a.position._getTransformedBySplitOperation( b ); } @@ -1310,7 +1504,7 @@ setTransformation( SplitOperation, SplitOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, WrapOperation, ( a, b ) => { +setTransformation( SplitOperation, WrapOperation, ( a, b, context ) => { // Case 1: If split position has been wrapped, reverse the wrapping so that split can be applied as intended. // This is an edge case scenario where it is difficult to find a correct solution. // Since it will be a rare (or only theoretical) scenario, the algorithm will perform the easy solution. @@ -1328,7 +1522,11 @@ setTransformation( SplitOperation, WrapOperation, ( a, b ) => { return [ reversed, a ]; } - a.position = a.position._getTransformedByWrapOperation( b ); + if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'splitInside' ) { + a.position = b.targetPosition; + } else { + a.position = a.position._getTransformedByWrapOperation( b ); + } if ( a.graveyardPosition && b.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByDeletion( b.graveyardPosition, 1 ); @@ -1337,8 +1535,17 @@ setTransformation( SplitOperation, WrapOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( SplitOperation, UnwrapOperation, ( a, b ) => { - a.position = a.position._getTransformedByUnwrapOperation( b ); +setTransformation( SplitOperation, UnwrapOperation, ( a, b, context ) => { + const splitInside = a.position.hasSameParentAs( b.position ); + + if ( splitInside && !context.wasUndone( b ) ) { + const path = b.graveyardPosition.path.slice(); + path.push( 0 ); + + a.position = new Position( b.graveyardPosition.root, path ); + } else { + a.position = a.position._getTransformedByUnwrapOperation( b ); + } if ( a.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 ); @@ -1349,7 +1556,13 @@ setTransformation( SplitOperation, UnwrapOperation, ( a, b ) => { // ----------------------- -setTransformation( WrapOperation, InsertOperation, ( a, b ) => { +setTransformation( WrapOperation, InsertOperation, ( a, b, context ) => { + if ( a.position.isEqual( b.position ) && context.getRelation( b, a ) == 'insertInside' ) { + a.howMany += b.howMany; + + return [ a ]; + } + const transformed = a.wrappedRange._getTransformedByInsertOperation( b, false )[ 0 ]; a.position = transformed.start; @@ -1371,11 +1584,18 @@ setTransformation( WrapOperation, MergeOperation, ( a, b ) => { return [ a ]; } ); -setTransformation( WrapOperation, MoveOperation, ( a, b ) => { +setTransformation( WrapOperation, MoveOperation, ( a, b, context ) => { if ( a.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } + if ( a.position.isEqual( b.targetPosition ) && context.getRelation( b, a ) == 'insertInside' ) { + a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); + a.howMany += b.howMany; + + return [ a ]; + } + const ranges = breakRangeByMoveOperation( a.wrappedRange, b, false ); if ( ranges.length == 0 ) { @@ -1392,10 +1612,14 @@ setTransformation( WrapOperation, MoveOperation, ( a, b ) => { } ); } ); -setTransformation( WrapOperation, SplitOperation, ( a, b ) => { +setTransformation( WrapOperation, SplitOperation, ( a, b, context ) => { // Case 1: If range to wrap got split by split operation cancel the wrapping. + // Do that only if this is not undo mode. If `b` operation was earlier transformed by unwrap operation + // and the split position was inside the unwrapped range, then proceed without special case. // - if ( a.position.hasSameParentAs( b.position ) && a.wrappedRange.containsPosition( b.position ) ) { + const isInside = a.position.hasSameParentAs( b.position ) && a.wrappedRange.containsPosition( b.position ); + + if ( isInside && context.getRelation( b, a ) !== 'splitInside' ) { // We cannot just return no-op in this case, because in the mirror case scenario the wrap is reversed, which // might introduce a new node in the graveyard (if the wrap didn't have `graveyardPosition`, then the wrap // created a new element which was put to the graveyard when the wrap was reversed). @@ -1405,7 +1629,7 @@ setTransformation( WrapOperation, SplitOperation, ( a, b ) => { const graveyard = a.position.root.document.graveyard; const graveyardPosition = new Position( graveyard, [ 0 ] ); - return new InsertOperation( graveyardPosition, a.element, 0 ); + return [ new InsertOperation( graveyardPosition, a.element, 0 ) ]; } else { return getNoOp(); } @@ -1626,7 +1850,7 @@ setTransformation( UnwrapOperation, MergeOperation, ( a, b, context ) => { return [ a ]; } ); -setTransformation( UnwrapOperation, MoveOperation, ( a, b, context ) => { +setTransformation( UnwrapOperation, MoveOperation, ( a, b ) => { // Case 1: Move operation moves nodes from the unwrapped element. // This does not have any impact on `UnwrapOperation#position`, but `#howMany` has to be changed. // @@ -1644,7 +1868,7 @@ setTransformation( UnwrapOperation, MoveOperation, ( a, b, context ) => { a.position = a.position._getTransformedByMoveOperation( b ); - if ( !a.graveyardPosition.isEqual( b.targetPosition ) || !context.aIsStrong ) { + if ( !a.graveyardPosition.isEqual( b.targetPosition ) ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } @@ -1695,6 +1919,10 @@ setTransformation( UnwrapOperation, WrapOperation, ( a, b ) => { a.howMany = a.howMany - b.howMany + 1; } + if ( b.graveyardPosition && compareArrays( a.position.getParentPath(), b.graveyardPosition.path ) == 'same' ) { + a.howMany = b.howMany; + } + // The default case. // a.position = a.position._getTransformedByWrapOperation( b ); @@ -1713,6 +1941,8 @@ setTransformation( UnwrapOperation, UnwrapOperation, ( a, b, context ) => { a.position = new Position( b.graveyardPosition.root, path ); a.howMany = 0; a.graveyardPosition = Position.createFromPosition( b.graveyardPosition ); + + return [ a ]; } a.position = a.position._getTransformedByUnwrapOperation( b ); @@ -1763,8 +1993,7 @@ function makeMoveOperationsFromRanges( ranges, targetPosition ) { ranges[ j ] = ranges[ j ]._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany )[ 0 ]; } - // targetPosition.stickiness = 'toPrevious'; - targetPosition = targetPosition._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany, true ); + targetPosition = targetPosition._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany ); } return operations; diff --git a/src/model/position.js b/src/model/position.js index 7b3cfa5a4..4a91b11f9 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -761,6 +761,7 @@ export default class Position { // The first part of a path to combined position is a path to the place where nodes were moved. const combined = Position.createFromPosition( target ); + combined.stickiness = this.stickiness; // Then we have to update the rest of the path. diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index fc1f222f6..d90a85111 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -53,6 +53,12 @@ describe( 'transform', () => { } } + const contextIsStrong = { + aIsStrong: true, + wasUndone: () => false, + getRelation: () => false + }; + describe( 'InsertOperation', () => { let nodeC, nodeD, position; @@ -119,7 +125,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -310,7 +316,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -862,7 +868,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.oldValue = 'xyz'; @@ -1102,7 +1108,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1241,7 +1247,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1567,7 +1573,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.sourcePosition.path = [ 4, 1, 0 ]; @@ -1599,7 +1605,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.sourcePosition.path = [ 4, 1, 1 ]; @@ -1637,7 +1643,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -1675,7 +1681,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1714,7 +1720,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1765,7 +1771,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1805,7 +1811,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1830,7 +1836,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 2 ); @@ -1901,7 +1907,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 3 ); @@ -1952,7 +1958,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -1975,7 +1981,7 @@ describe( 'transform', () => { } ); it( 'should skip context.aIsStrong and be less important than RemoveOperation', () => { - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -2318,7 +2324,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expect( transOp.length ).to.equal( 1 ); @@ -2611,7 +2617,7 @@ describe( 'transform', () => { const anotherRange = Range.createFromParentsAndOffsets( root, 2, root, 2 ); const transformBy = new MarkerOperation( 'name', oldRange, anotherRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy, { aIsStrong: true } ); + const transOp = transform.transform( op, transformBy, contextIsStrong ); expected.oldRange = anotherRange; diff --git a/tests/model/operation/transform/attribute.js b/tests/model/operation/transform/attribute.js index 6a5d0f775..3e8610442 100644 --- a/tests/model/operation/transform/attribute.js +++ b/tests/model/operation/transform/attribute.js @@ -795,7 +795,7 @@ describe( 'transform', () => { expectClients( 'FooB<$text bold="true">ar' ); } ); - it.skip( 'element into paragraph #2, then undo', () => { + it( 'element into paragraph #2, then undo', () => { john.setData( 'Foo[Bar]' ); kate.setData( 'Foo[]Bar' ); diff --git a/tests/model/operation/transform/insert.js b/tests/model/operation/transform/insert.js index 635116e14..3eb36286b 100644 --- a/tests/model/operation/transform/insert.js +++ b/tests/model/operation/transform/insert.js @@ -811,7 +811,7 @@ describe( 'transform', () => { expectClients( 'FooBarAbc' ); } ); - it.skip( 'element, then add marker, split and undo with type #2', () => { + it( 'element, then add marker, split and undo with type #2', () => { john.setData( 'Foo[]' ); kate.setData( '[Foo]' ); @@ -845,8 +845,6 @@ describe( 'transform', () => { syncClients(); - // Actual content: - // Ba expectClients( 'FooBarAbc' ); } ); } ); diff --git a/tests/model/operation/transform/marker.js b/tests/model/operation/transform/marker.js index 07b4a2f2b..bb3cf0902 100644 --- a/tests/model/operation/transform/marker.js +++ b/tests/model/operation/transform/marker.js @@ -183,7 +183,6 @@ describe( 'transform', () => { kate.remove(); syncClients(); - expectClients( '' ); john.undo(); @@ -191,8 +190,7 @@ describe( 'transform', () => { syncClients(); - // Actual result for Kate: - // Foo Bar + // Wrong markers transforming. expectClients( '' + 'Foo Bar' + diff --git a/tests/model/operation/transform/move.js b/tests/model/operation/transform/move.js index f54108ef8..a7868903b 100644 --- a/tests/model/operation/transform/move.js +++ b/tests/model/operation/transform/move.js @@ -335,7 +335,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'text in other user\'s selection', () => { + it( 'text in other user\'s selection', () => { john.setData( '[Foo] Bar' ); kate.setData( 'F[]oo Bar' ); @@ -366,7 +366,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'inside moved text', () => { + it( 'inside moved text', () => { john.setData( 'F[oo Ba]rAbc' ); kate.setData( 'Foo[] BarAbc' ); @@ -375,10 +375,6 @@ describe( 'transform', () => { syncClients(); - // Actual result for Kate: - // Fr BaooAbc - // Move operations have wrong targets when they target at same place. - expectClients( 'F' + 'r' + diff --git a/tests/model/operation/transform/remove.js b/tests/model/operation/transform/remove.js index 92cbc090b..8e2fdcf18 100644 --- a/tests/model/operation/transform/remove.js +++ b/tests/model/operation/transform/remove.js @@ -218,7 +218,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'element while removing, then undo', () => { + it( 'element while removing, then undo', () => { john.setData( 'Foo
[Bar]
' ); kate.setData( 'Foo
[]Bar
' ); @@ -226,15 +226,11 @@ describe( 'transform', () => { kate.unwrap(); syncClients(); - expectClients( 'Foo' ); john.undo(); syncClients(); - - // Actual result: - // Foo
expectClients( 'Foo' + 'Bar' @@ -443,7 +439,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'element into paragraph, then undo', () => { + it( 'element into paragraph, then undo', () => { john.setData( 'F[oo]Bar' ); kate.setData( 'Foo[]Bar' ); @@ -451,15 +447,11 @@ describe( 'transform', () => { kate.merge(); syncClients(); - expectClients( 'FBar' ); john.undo(); syncClients(); - - // Actual result: - // FoBar expectClients( 'FooBar' ); } ); diff --git a/tests/model/operation/transform/split.js b/tests/model/operation/transform/split.js index 7f02b915c..e1b46a707 100644 --- a/tests/model/operation/transform/split.js +++ b/tests/model/operation/transform/split.js @@ -90,7 +90,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'intersecting wrap', () => { + it( 'intersecting wrap', () => { john.setData( 'Fo[]o' ); kate.setData( 'F[oo]' ); @@ -98,10 +98,9 @@ describe( 'transform', () => { kate.wrap( 'div' ); syncClients(); - expectClients( - 'F' + - '
oo
' + 'Fo' + + 'o' ); } ); } ); @@ -122,7 +121,7 @@ describe( 'transform', () => { ); } ); - it.skip( 'element in same position', () => { + it( 'element in same position', () => { john.setData( '
[]Foo
' ); kate.setData( '
[]Foo
' ); @@ -130,7 +129,6 @@ describe( 'transform', () => { kate.unwrap(); syncClients(); - expectClients( '
Foo
' ); } ); @@ -285,7 +283,7 @@ describe( 'transform', () => { expectClients( 'FooBar' ); } ); - it.skip( 'element into paragraph #3', () => { + it( 'element into paragraph #3', () => { john.setData( 'Foo[]Bar' ); kate.setData( 'Foo[]Bar' ); @@ -299,13 +297,10 @@ describe( 'transform', () => { kate.undo(); syncClients(); - // Actual content for Kate: - // FooBar - // There is a problem in Merge x Split transform in undo case. expectClients( 'FooBar' ); } ); - it.skip( 'element into paragraph #4', () => { + it( 'element into paragraph #4', () => { john.setData( 'FooB[]ar' ); kate.setData( 'Foo[]Bar' ); @@ -313,14 +308,16 @@ describe( 'transform', () => { kate.merge(); syncClients(); + expectClients( + 'FooB' + + 'ar' + ); john.undo(); kate.undo(); - expectClients( - 'Fo' + - 'oBar' - ); + syncClients(); + expectClients( 'FooBar' ); } ); } ); diff --git a/tests/model/operation/transform/unwrap.js b/tests/model/operation/transform/unwrap.js index 0b14ddd1b..8cba9be85 100644 --- a/tests/model/operation/transform/unwrap.js +++ b/tests/model/operation/transform/unwrap.js @@ -50,7 +50,7 @@ describe( 'transform', () => { expectClients( 'Foo' ); } ); - it.skip( 'the same element, then undo', () => { + it( 'the same element, then undo', () => { john.setData( '
[]Foo
' ); kate.setData( '
[]Foo
' ); @@ -63,9 +63,6 @@ describe( 'transform', () => { john.undo(); syncClients(); - // Actual content for Kate: - // Foo - // Kate has a different order of nodes in graveyard after syncing. expectClients( '
Foo
' ); } ); } ); diff --git a/tests/model/operation/transform/utils.js b/tests/model/operation/transform/utils.js index 8a0557151..016842f9c 100644 --- a/tests/model/operation/transform/utils.js +++ b/tests/model/operation/transform/utils.js @@ -253,16 +253,34 @@ function bufferOperations( operations, client ) { } export function syncClients() { - for ( const client of clients ) { - for ( const item of bufferedOperations ) { - const remoteOperations = item.operations.map( op => OperationFactory.fromJSON( JSON.parse( op ), client.document ) ); - const remoteClient = item.client; + const clientsOperations = {}; + + // For each client, flatten all buffered operations into one set. + for ( const item of bufferedOperations ) { + const name = item.client.name; + + if ( !clientsOperations[ name ] ) { + clientsOperations[ name ] = []; + } + + clientsOperations[ name ].push( ...item.operations ); + } - if ( remoteClient == client ) { + for ( const localClient of clients ) { + for ( const remoteClient of clients ) { + if ( remoteClient == localClient ) { continue; } - const clientOperations = Array.from( client.document.history.getOperations( client.syncedVersion ) ); + const remoteOperationsJson = clientsOperations[ remoteClient.name ]; + + if ( !remoteOperationsJson ) { + continue; + } + + const remoteOperations = remoteOperationsJson.map( op => OperationFactory.fromJSON( JSON.parse( op ), localClient.document ) ); + + const localOperations = Array.from( localClient.document.history.getOperations( localClient.syncedVersion ) ); let remoteOperationsTransformed = null; @@ -271,21 +289,21 @@ export function syncClients() { padWithNoOps: true }; - if ( client.orderNumber < remoteClient.orderNumber ) { - remoteOperationsTransformed = transform.transformSets( clientOperations, remoteOperations, options ).operationsB; + if ( localClient.orderNumber < remoteClient.orderNumber ) { + remoteOperationsTransformed = transform.transformSets( localOperations, remoteOperations, options ).operationsB; } else { - remoteOperationsTransformed = transform.transformSets( remoteOperations, clientOperations, options ).operationsA; + remoteOperationsTransformed = transform.transformSets( remoteOperations, localOperations, options ).operationsA; } - client.editor.model.enqueueChange( 'transparent', writer => { + localClient.editor.model.enqueueChange( 'transparent', writer => { for ( const operation of remoteOperationsTransformed ) { writer.batch.addOperation( operation ); - client.editor.model.applyOperation( operation ); + localClient.editor.model.applyOperation( operation ); } } ); } - client.syncedVersion = client.document.version; + localClient.syncedVersion = localClient.document.version; } bufferedOperations.clear(); diff --git a/tests/model/operation/transform/wrap.js b/tests/model/operation/transform/wrap.js index f7e125980..a24e011be 100644 --- a/tests/model/operation/transform/wrap.js +++ b/tests/model/operation/transform/wrap.js @@ -63,19 +63,24 @@ describe( 'transform', () => { ); } ); - it.skip( 'intersecting wrap #2', () => { - john.setData( '[Foo]' ); - kate.setData( 'F[o]o' ); + it( 'intersecting wrap #2', () => { + john.setData( '[FooBarAbc]' ); + kate.setData( 'Foo[Bar]Abc' ); - john.wrap( 'div' ); + john.wrap( 'blockQuote' ); kate.wrap( 'div' ); syncClients(); - - expectClients( '
Foo
' ); + expectClients( + '
' + + 'Foo' + + 'Bar' + + 'Abc' + + '
' + ); } ); - it.skip( 'intersecting wrap, then undo #1', () => { + it( 'intersecting wrap, then undo #1', () => { john.setData( '[FooBar]Abc' ); kate.setData( 'Foo[BarAbc]' ); @@ -83,12 +88,20 @@ describe( 'transform', () => { kate.wrap( 'div' ); syncClients(); + expectClients( + '
' + + 'Foo' + + 'Bar' + + '
' + + '
' + + 'Abc' + + '
' + ); john.undo(); kate.undo(); syncClients(); - expectClients( 'Foo' + 'Bar' + @@ -96,21 +109,30 @@ describe( 'transform', () => { ); } ); - it.skip( 'intersecting wrap, then undo #2', () => { - john.setData( '[Foo]' ); - kate.setData( 'F[o]o' ); + it( 'intersecting wrap, then undo #2', () => { + john.setData( '[FooBar]Abc' ); + kate.setData( 'Foo[BarAbc]' ); - john.wrap( 'div' ); + john.wrap( 'blockQuote' ); kate.wrap( 'div' ); syncClients(); + expectClients( + '
' + + 'Foo' + + 'Bar' + + '
' + + '
' + + 'Abc' + + '
' + ); john.undo(); kate.undo(); syncClients(); - expectClients( '
Foo
' ); + expectClients( 'FooBarAbc' ); } ); it( 'element and text', () => { @@ -227,14 +249,12 @@ describe( 'transform', () => { kate.merge(); syncClients(); - - expectClients( '
FooBar
' ); + expectClients( 'FooBar
' ); john.undo(); kate.undo(); syncClients(); - expectClients( 'FooBar' ); } ); } ); From f526ee7fd5ae14bf5653d0840e0f3e02225dc34e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 30 Jul 2018 16:00:45 +0200 Subject: [PATCH 12/26] Changed: `context.getRelation` from function to properties. --- src/model/operation/transform.js | 38 ++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index c8249cdcf..c93a023f9 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -72,7 +72,7 @@ function updateBaseVersions( operations, baseVersion ) { return operations; } -function transform( a, b, context = { aIsStrong: false, wasUndone: () => false, getRelation: () => null } ) { +function transform( a, b, context = { isStrong: false } ) { const transformationFunction = getTransformation( a, b ); return transformationFunction( a.clone(), b, context ); @@ -116,13 +116,17 @@ function transformSets( operationsA, operationsB, options ) { const newOpA = transform( opA, opB, { aIsStrong: true, aWasUndone: context.wasUndone( opA ), - bWasUndone: context.wasUndone( opB ) + bWasUndone: context.wasUndone( opB ), + abRelation: context.getRelation( opA, opB ), + baRelation: context.getRelation( opB, opA ) } ); const newOpB = transform( opB, opA, { aIsStrong: false, aWasUndone: context.wasUndone( opB ), bWasUndone: context.wasUndone( opA ), + abRelation: context.getRelation( opB, opA ), + baRelation: context.getRelation( opA, opB ) } ); if ( options.useContext ) { @@ -695,7 +699,7 @@ setTransformation( InsertOperation, InsertOperation, ( a, b, context ) => { } ); setTransformation( InsertOperation, MoveOperation, ( a, b, context ) => { - if ( a.position.isEqual( b.targetPosition ) && context.getRelation( a, b ) == 'insertBefore' ) { + if ( a.position.isEqual( b.targetPosition ) && context.abRelation == 'insertBefore' ) { return [ a ]; } @@ -705,7 +709,7 @@ setTransformation( InsertOperation, MoveOperation, ( a, b, context ) => { } ); setTransformation( InsertOperation, SplitOperation, ( a, b, context ) => { - if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'insertAtSource' ) { + if ( a.position.isEqual( b.position ) && context.abRelation == 'insertAtSource' ) { a.position = b.moveTargetPosition; return [ a ]; @@ -723,7 +727,7 @@ setTransformation( InsertOperation, MergeOperation, ( a, b ) => { } ); setTransformation( InsertOperation, WrapOperation, ( a, b, context ) => { - if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'insertInside' ) { + if ( a.position.isEqual( b.position ) && context.abRelation == 'insertInside' ) { a.position = b.targetPosition; return [ a ]; @@ -990,7 +994,7 @@ setTransformation( MoveOperation, InsertOperation, ( a, b, context ) => { a.sourcePosition = transformed.start; a.howMany = transformed.end.offset - transformed.start.offset; - if ( !a.targetPosition.isEqual( b.position ) || context.getRelation( b, a ) == 'insertBefore' ) { + if ( !a.targetPosition.isEqual( b.position ) || context.abRelation == 'insertBefore' ) { a.targetPosition = a.targetPosition._getTransformedByInsertOperation( b ); } @@ -1007,7 +1011,7 @@ 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 || context.getRelation( a, b ) == 'insertBefore'; + let aIsStrong = context.aIsStrong || context.abRelation == 'insertBefore'; // `a.targetPosition` could be affected by the `b` operation. We will transform it. let newTargetPosition; @@ -1221,7 +1225,7 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => { a.howMany = transformed.end.offset - transformed.start.offset; a.targetPosition = newTargetPosition; - if ( a.targetPosition.isEqual( b.position ) && context.getRelation( a, b ) == 'insertAtSource' ) { + if ( a.targetPosition.isEqual( b.position ) && context.abRelation == 'insertAtSource' ) { a.targetPosition = b.targetPosition; } @@ -1315,7 +1319,7 @@ setTransformation( MoveOperation, WrapOperation, ( a, b, context ) => { a.howMany = transformed.end.offset - transformed.start.offset; a.targetPosition = newTargetPosition; - if ( a.targetPosition.isEqual( b.position ) && context.getRelation( a, b ) == 'insertInside' ) { + if ( a.targetPosition.isEqual( b.position ) && context.abRelation == 'insertInside' ) { a.targetPosition = b.targetPosition; } @@ -1422,7 +1426,7 @@ setTransformation( RootAttributeOperation, RootAttributeOperation, ( a, b, conte // ----------------------- setTransformation( SplitOperation, InsertOperation, ( a, b, context ) => { - if ( a.position.isEqual( b.position ) && context.getRelation( b, a ) == 'insertAtSource' ) { + if ( a.position.isEqual( b.position ) && context.baRelation == 'insertAtSource' ) { return [ a ]; } @@ -1469,7 +1473,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { return [ a ]; } - if ( a.position.isEqual( b.targetPosition ) && context.getRelation( a, b ) == 'splitBefore' ) { + if ( a.position.isEqual( b.targetPosition ) && context.abRelation == 'splitBefore' ) { a.position = a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); return [ a ]; @@ -1491,7 +1495,7 @@ setTransformation( SplitOperation, SplitOperation, ( a, b, context ) => { if ( a.graveyardPosition && b.graveyardPosition && a.graveyardPosition.isEqual( b.graveyardPosition ) ) { return getNoOp(); } - } else if ( a.position.isEqual( b.insertionPosition ) && context.getRelation( a, b ) == 'splitBefore' ) { + } else if ( a.position.isEqual( b.insertionPosition ) && context.abRelation == 'splitBefore' ) { return [ a ]; } else { a.position = a.position._getTransformedBySplitOperation( b ); @@ -1522,7 +1526,7 @@ setTransformation( SplitOperation, WrapOperation, ( a, b, context ) => { return [ reversed, a ]; } - if ( a.position.isEqual( b.position ) && context.getRelation( a, b ) == 'splitInside' ) { + if ( a.position.isEqual( b.position ) && context.abRelation == 'splitInside' ) { a.position = b.targetPosition; } else { a.position = a.position._getTransformedByWrapOperation( b ); @@ -1538,7 +1542,7 @@ setTransformation( SplitOperation, WrapOperation, ( a, b, context ) => { setTransformation( SplitOperation, UnwrapOperation, ( a, b, context ) => { const splitInside = a.position.hasSameParentAs( b.position ); - if ( splitInside && !context.wasUndone( b ) ) { + if ( splitInside && !context.bWasUndone ) { const path = b.graveyardPosition.path.slice(); path.push( 0 ); @@ -1557,7 +1561,7 @@ setTransformation( SplitOperation, UnwrapOperation, ( a, b, context ) => { // ----------------------- setTransformation( WrapOperation, InsertOperation, ( a, b, context ) => { - if ( a.position.isEqual( b.position ) && context.getRelation( b, a ) == 'insertInside' ) { + if ( a.position.isEqual( b.position ) && context.baRelation == 'insertInside' ) { a.howMany += b.howMany; return [ a ]; @@ -1589,7 +1593,7 @@ setTransformation( WrapOperation, MoveOperation, ( a, b, context ) => { a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } - if ( a.position.isEqual( b.targetPosition ) && context.getRelation( b, a ) == 'insertInside' ) { + if ( a.position.isEqual( b.targetPosition ) && context.baRelation == 'insertInside' ) { a.position._getTransformedByDeletion( b.sourcePosition, b.howMany ); a.howMany += b.howMany; @@ -1619,7 +1623,7 @@ setTransformation( WrapOperation, SplitOperation, ( a, b, context ) => { // const isInside = a.position.hasSameParentAs( b.position ) && a.wrappedRange.containsPosition( b.position ); - if ( isInside && context.getRelation( b, a ) !== 'splitInside' ) { + if ( isInside && context.baRelation !== 'splitInside' ) { // We cannot just return no-op in this case, because in the mirror case scenario the wrap is reversed, which // might introduce a new node in the graveyard (if the wrap didn't have `graveyardPosition`, then the wrap // created a new element which was put to the graveyard when the wrap was reversed). From 00c68f2a95a033fa0fb02c705c5a1b8e25b5da28 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 30 Jul 2018 16:06:27 +0200 Subject: [PATCH 13/26] Changed: Default value for `context.isStrong` is not needed. --- 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 c93a023f9..1dd415e1a 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -72,7 +72,7 @@ function updateBaseVersions( operations, baseVersion ) { return operations; } -function transform( a, b, context = { isStrong: false } ) { +function transform( a, b, context = {} ) { const transformationFunction = getTransformation( a, b ); return transformationFunction( a.clone(), b, context ); From 647971b6539a0454d5a88168a9f6d6c73eaf6e3e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 31 Jul 2018 12:52:57 +0200 Subject: [PATCH 14/26] Fix: Fixed MergeOperation x SplitOperation algorithm. --- src/model/operation/transform.js | 57 ++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 1dd415e1a..a4587e4a0 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -113,27 +113,32 @@ function transformSets( operationsA, operationsB, options ) { const opA = opsA[ k ]; const opB = opsB[ l ]; - const newOpA = transform( opA, opB, { + if ( options.useContext ) { + updateRelations( context, opA, opB ); + } + + const contextAB = { aIsStrong: true, aWasUndone: context.wasUndone( opA ), bWasUndone: context.wasUndone( opB ), abRelation: context.getRelation( opA, opB ), baRelation: context.getRelation( opB, opA ) - } ); + }; - const newOpB = transform( 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 ); - - updateRelations( context, opA, opB ); } opsA.splice( k, 1, ...newOpA ); @@ -238,10 +243,15 @@ function updateRelations( context, opA, opB ) { break; } - case MoveOperation: { + case MoveOperation: + case RemoveOperation: + case ReinsertOperation: { if ( opA.targetPosition.isEqual( opB.sourcePosition ) || opA.targetPosition.isBefore( opB.sourcePosition ) ) { setRelation( context, opA, opB, 'insertBefore' ); setRelation( context, opB, opA, 'insertAfter' ); + } else { + setRelation( context, opA, opB, 'insertAfter' ); + setRelation( context, opB, opA, 'insertBefore' ); } break; @@ -272,7 +282,9 @@ function updateRelations( context, opA, opB ) { break; } - case MoveOperation: { + case MoveOperation: + case RemoveOperation: + case ReinsertOperation: { if ( opA.position.isEqual( opB.sourcePosition ) || opA.position.isBefore( opB.sourcePosition ) ) { setRelation( context, opA, opB, 'splitBefore' ); setRelation( context, opB, opA, 'insertAtSource' ); @@ -305,7 +317,9 @@ function updateRelations( context, opA, opB ) { break; } - case MoveOperation: { + case MoveOperation: + case RemoveOperation: + case ReinsertOperation: { if ( opA.position.isEqual( opB.sourcePosition ) || opA.position.isBefore( opB.sourcePosition ) ) { setRelation( context, opA, opB, 'insertBefore' ); } @@ -897,8 +911,6 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => { } ); setTransformation( MergeOperation, SplitOperation, ( a, b ) => { - a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); - if ( b.graveyardPosition ) { a.graveyardPosition = a.graveyardPosition._getTransformedByDeletion( b.graveyardPosition, 1 ); } @@ -917,13 +929,18 @@ 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 one exception though - when the split operation is a result of undo. In those cases, it is needed - // to keep `targetPosition` intact, so the nodes are returned to the correct element. + // 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 ) && b.graveyardPosition ) { - // return [ a ]; - // } + if ( a.targetPosition.isEqual( b.position ) && a.sourcePosition.root.rootName == '$graveyard' ) { + a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); + return [ a ]; + } + + a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b ); a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b ); return [ a ]; @@ -1011,7 +1028,13 @@ 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 || context.abRelation == 'insertBefore'; + let aIsStrong = context.aIsStrong; + + if ( context.abRelation == 'insertBefore' ) { + aIsStrong = true; + } else if ( context.abRelation == 'insertAfter' ) { + aIsStrong = false; + } // `a.targetPosition` could be affected by the `b` operation. We will transform it. let newTargetPosition; From 252c0778bb1f3d03655482cc58cad03623f8c1ee Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 31 Jul 2018 13:33:02 +0200 Subject: [PATCH 15/26] Changed: Second parameter of `LiveRange#event:change` is now `deletionPosition` not operation that changed the range. --- src/model/documentselection.js | 4 +-- src/model/liverange.js | 19 +++++++++--- tests/model/liverange.js | 57 ++++++++++++++++++++++++---------- 3 files changed, 56 insertions(+), 24 deletions(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 3317be489..20d462d8a 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -750,14 +750,14 @@ class LiveSelection extends Selection { const liveRange = LiveRange.createFromRange( range ); - liveRange.on( 'change:range', ( evt, oldRange, operation ) => { + liveRange.on( 'change:range', ( evt, oldRange, deletionPosition ) => { this._hasChangedRange = true; // If `LiveRange` is in whole moved to the graveyard, save necessary data. It will be fixed on `Model#applyOperation` event. if ( liveRange.root == this._document.graveyard ) { this._fixGraveyardRangesData.push( { liveRange, - sourcePosition: operation.sourcePosition + sourcePosition: deletionPosition } ); } } ); diff --git a/src/model/liverange.js b/src/model/liverange.js index 8ce4b454a..6799bc391 100644 --- a/src/model/liverange.js +++ b/src/model/liverange.js @@ -86,9 +86,7 @@ export default class LiveRange extends Range { * @param {Object} data Object with additional information about the change. Those parameters are passed from * {@link module:engine/model/document~Document#event:change document change event}. * @param {String} data.type Change type. - * @param {module:engine/model/batch~Batch} data.batch Batch which changed the live range. - * @param {module:engine/model/range~Range} data.range Range containing the result of applied change. - * @param {module:engine/model/position~Position} data.sourcePosition Source position for move, remove and reinsert change types. + * @param {module:engine/model/position~Position|null} deletionPosition Source position for move, remove and reinsert change types. */ /** @@ -147,16 +145,27 @@ function transform( operation ) { const contentChanged = doesOperationChangeRangeContent( this, operation ); if ( boundariesChanged ) { + let deletionPosition = null; + + if ( result.root.rootName == '$graveyard' ) { + if ( operation.type == 'remove' ) { + deletionPosition = operation.sourcePosition; + } else { + // Merge operation. + deletionPosition = operation.deletionPosition; + } + } + // If range boundaries have changed, fire `change:range` event. const oldRange = Range.createFromRange( this ); this.start = result.start; this.end = result.end; - this.fire( 'change:range', oldRange, operation ); + this.fire( 'change:range', oldRange, deletionPosition ); } else if ( contentChanged ) { // If range boundaries have not changed, but there was change inside the range, fire `change:content` event. - this.fire( 'change:content', Range.createFromRange( this ), operation ); + this.fire( 'change:content', Range.createFromRange( this ), null ); } } diff --git a/tests/model/liverange.js b/tests/model/liverange.js index 1c3a9ea92..78a56ac7d 100644 --- a/tests/model/liverange.js +++ b/tests/model/liverange.js @@ -3,7 +3,6 @@ * For licensing, see LICENSE.md. */ -import Batch from '../../src/model/batch'; import Model from '../../src/model/model'; import Element from '../../src/model/element'; import Position from '../../src/model/position'; @@ -90,7 +89,7 @@ describe( 'LiveRange', () => { range.detach(); } ); - it( 'should fire change:range event with proper data when its boundaries are changed', () => { + it( 'should fire change:range event with when its boundaries are changed', () => { const live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) ); const copy = Range.createFromRange( live ); @@ -99,15 +98,11 @@ describe( 'LiveRange', () => { const sourcePosition = new Position( root, [ 2 ] ); const targetPosition = new Position( root, [ 0 ] ); - const batch = new Batch(); - let op = null; - model.enqueueChange( batch, writer => { + model.change( writer => { const sourceRange = Range.createFromPositionAndShift( sourcePosition, 1 ); writer.move( sourceRange, targetPosition ); - - op = batch.operations[ 0 ]; } ); expect( spy.calledOnce ).to.be.true; @@ -115,11 +110,11 @@ describe( 'LiveRange', () => { // First parameter available in event should be a range that is equal to the live range before the live range changed. expect( spy.args[ 0 ][ 1 ].isEqual( copy ) ).to.be.true; - // Second parameter is the operation that changed the range. - expect( spy.args[ 0 ][ 2 ] ).to.equal( op ); + // Second parameter is null for operations that did not move the range into graveyard. + expect( spy.args[ 0 ][ 2 ] ).to.be.null; } ); - it( 'should fire change:content event with proper data when content inside the range has changed', () => { + it( 'should fire change:content event when content inside the range has changed', () => { const live = new LiveRange( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 3 ] ) ); const spy = sinon.spy(); @@ -127,15 +122,11 @@ describe( 'LiveRange', () => { const sourcePosition = new Position( root, [ 0, 2, 0 ] ); const targetPosition = new Position( root, [ 0, 4, 0 ] ); - const batch = new Batch(); - let op = null; - model.enqueueChange( batch, writer => { + model.change( writer => { const sourceRange = Range.createFromPositionAndShift( sourcePosition, 2 ); writer.move( sourceRange, targetPosition ); - - op = batch.operations[ 0 ]; } ); expect( spy.calledOnce ).to.be.true; @@ -143,8 +134,40 @@ describe( 'LiveRange', () => { // First parameter available in event should be a range that is equal to the live range before the live range changed. expect( spy.args[ 0 ][ 1 ].isEqual( live ) ).to.be.true; - // Second parameter is the operation that changed the range. - expect( spy.args[ 0 ][ 2 ] ).to.equal( op ); + // Second parameter is null for operations that did not move the range into graveyard. + expect( spy.args[ 0 ][ 2 ] ).to.be.null; + } ); + + it( 'should pass deletion position if range was removed (remove)', () => { + const live = new LiveRange( new Position( root, [ 0, 2 ] ), new Position( root, [ 0, 4 ] ) ); + + const spy = sinon.spy(); + live.on( 'change:range', spy ); + + const sourcePosition = new Position( root, [ 0, 0 ] ); + + model.change( writer => { + writer.remove( Range.createFromPositionAndShift( sourcePosition, 6 ) ); + } ); + + // Second parameter is deletion position. + expect( spy.args[ 0 ][ 2 ].isEqual( sourcePosition ) ).to.be.true; + } ); + + it( 'should pass deletion position if range was removed (merge)', () => { + const live = new LiveRange( new Position( root, [ 1 ] ), new Position( root, [ 2 ] ) ); + + const spy = sinon.spy(); + live.on( 'change:range', spy ); + + const mergePosition = new Position( root, [ 1 ] ); + + model.change( writer => { + writer.merge( mergePosition ); + } ); + + // Second parameter is deletion position. + expect( spy.args[ 0 ][ 2 ].isEqual( mergePosition ) ).to.be.true; } ); describe( 'should get transformed and fire change:range if', () => { From 4637ce454cd026e005858edb50fb5ffaa31ebb3b Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 31 Jul 2018 16:35:05 +0200 Subject: [PATCH 16/26] Tests: Removed/fixed old transformation tests. --- tests/model/operation/transform.js | 54 ++---------------------------- 1 file changed, 2 insertions(+), 52 deletions(-) diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index d90a85111..ad10c9149 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -682,8 +682,8 @@ describe( 'transform', () => { expectOperation( transOp[ 0 ], expected ); - expected.range.start.path = [ 0, 2, 1 ]; - expected.range.end.path = [ 0, 2, 2 ]; + expected.range.start.path = [ 0, 2, 3 ]; + expected.range.end.path = [ 0, 2, 4 ]; expectOperation( transOp[ 1 ], expected ); @@ -693,56 +693,6 @@ describe( 'transform', () => { expectOperation( transOp[ 2 ], expected ); } ); } ); - - describe( 'by RemoveOperation', () => { - beforeEach( () => { - start = new Position( doc.graveyard, [ 2, 0 ] ); - end = new Position( doc.graveyard, [ 2, 4 ] ); - - range = new Range( start, end ); - - op = new AttributeOperation( range, 'foo', 'abc', 'bar', 0 ); - - expected.range = new Range( start, end ); - } ); - - it( 'remove operation inserted elements before attribute operation range: increment path', () => { - const transformBy = new RemoveOperation( - new Position( root, [ 0 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - 0 - ); - - transformBy.targetPosition.path = [ 0 ]; - - const transOp = transform.transform( op, transformBy ); - - expect( transOp.length ).to.equal( 1 ); - - expected.range.start.path = [ 4, 0 ]; - expected.range.end.path = [ 4, 4 ]; - - expectOperation( transOp[ 0 ], expected ); - } ); - - it( 'remove operation inserted elements after attribute operation range: do nothing', () => { - const transformBy = new RemoveOperation( - new Position( root, [ 0 ] ), - 2, - new Position( doc.graveyard, [ 0 ] ), - 0 - ); - - transformBy.targetPosition.path = [ 4 ]; - - const transOp = transform.transform( op, transformBy ); - - expect( transOp.length ).to.equal( 1 ); - - expectOperation( transOp[ 0 ], expected ); - } ); - } ); } ); describe( 'RootAttributeOperation', () => { From 975997fd67f0633fbadd9934becc7505a125ced6 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 31 Jul 2018 16:35:42 +0200 Subject: [PATCH 17/26] Fixed: Improved AttributeOperation and WrapOperation OT algorithms. --- src/model/operation/transform.js | 57 ++++++++++---------- tests/model/operation/transform/attribute.js | 2 +- tests/model/operation/transform/remove.js | 21 +++++++- 3 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index a4587e4a0..0462b04f6 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -532,20 +532,10 @@ setTransformation( AttributeOperation, MergeOperation, ( a, b ) => { } ); setTransformation( AttributeOperation, MoveOperation, ( a, b ) => { - const movedRange = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); const ranges = breakRangeByMoveOperation( a.range, b, true ); // Create `AttributeOperation`s out of the ranges. - return ranges.map( range => { - if ( movedRange.containsRange( range, true ) ) { - range = range._getTransformedByMoveOperation( b, false )[ 0 ]; - } else { - range = range._getTransformedByDeletion( b.sourcePosition, b.howMany ); - range = range._getTransformedByInsertion( b.targetPosition, b.howMany, false )[ 0 ]; - } - - return new AttributeOperation( range, a.key, a.oldValue, a.newValue, a.baseVersion ); - } ); + return ranges.map( range => new AttributeOperation( range, a.key, a.oldValue, a.newValue, a.baseVersion ) ); } ); function breakRangeByMoveOperation( range, moveOp, includeCommon ) { @@ -553,32 +543,48 @@ function breakRangeByMoveOperation( range, moveOp, includeCommon ) { const movedRange = Range.createFromPositionAndShift( moveOp.sourcePosition, moveOp.howMany ); - if ( range.start.hasSameParentAs( moveOp.sourcePosition ) ) { + if ( movedRange.containsRange( range, true ) ) { + ranges = [ ...range._getTransformedByMoveOperation( moveOp ) ]; + } else if ( range.start.hasSameParentAs( moveOp.sourcePosition ) ) { ranges = range.getDifference( movedRange ); + _flatMoveTransform( ranges, moveOp ); + if ( includeCommon ) { const common = range.getIntersection( movedRange ); if ( common ) { - ranges.push( common ); + ranges.push( ...common._getTransformedByMoveOperation( moveOp ) ); } } } else { ranges = [ range ]; + + _flatMoveTransform( ranges, moveOp ); } + return ranges; +} + +function _flatMoveTransform( ranges, moveOp ) { + const targetPosition = moveOp.getMovedRangeStart(); + for ( let i = 0; i < ranges.length; i++ ) { - const range = ranges[ i ]; + let range = ranges[ i ]; - if ( range.start.hasSameParentAs( moveOp.targetPosition ) && range.containsPosition( moveOp.targetPosition ) ) { - ranges.splice( i, 1, - new Range( range.start, moveOp.targetPosition ), - new Range( moveOp.targetPosition, range.end ) - ); + if ( range.start.hasSameParentAs( moveOp.sourcePosition ) ) { + range = range._getTransformedByDeletion( moveOp.sourcePosition, moveOp.howMany ); } - } - return ranges; + if ( range.start.hasSameParentAs( targetPosition ) ) { + const result = range._getTransformedByInsertion( targetPosition, moveOp.howMany, true ); + + ranges.splice( i, 1, ...result ); + i = i - 1 + result.length; + } else { + ranges[ i ] = range; + } + } } setTransformation( AttributeOperation, SplitOperation, ( a, b ) => { @@ -1625,12 +1631,6 @@ setTransformation( WrapOperation, MoveOperation, ( a, b, context ) => { const ranges = breakRangeByMoveOperation( a.wrappedRange, b, false ); - if ( ranges.length == 0 ) { - const range = a.wrappedRange._getTransformedByMoveOperation( b )[ 0 ]; - - ranges.push( range ); - } - return ranges.reverse().map( range => { const howMany = range.end.offset - range.start.offset; const elementOrGraveyardPosition = a.graveyardPosition ? a.graveyardPosition : a.element; @@ -1640,7 +1640,7 @@ setTransformation( WrapOperation, MoveOperation, ( a, b, context ) => { } ); setTransformation( WrapOperation, SplitOperation, ( a, b, context ) => { - // Case 1: If range to wrap got split by split operation cancel the wrapping. + // Case 1: If range to wrap got split cancel the wrapping. // Do that only if this is not undo mode. If `b` operation was earlier transformed by unwrap operation // and the split position was inside the unwrapped range, then proceed without special case. // @@ -1652,6 +1652,7 @@ setTransformation( WrapOperation, SplitOperation, ( a, b, context ) => { // created a new element which was put to the graveyard when the wrap was reversed). // // Instead, a node in graveyard will be inserted. + // if ( a.element ) { const graveyard = a.position.root.document.graveyard; const graveyardPosition = new Position( graveyard, [ 0 ] ); diff --git a/tests/model/operation/transform/attribute.js b/tests/model/operation/transform/attribute.js index 3e8610442..f67e237e1 100644 --- a/tests/model/operation/transform/attribute.js +++ b/tests/model/operation/transform/attribute.js @@ -325,7 +325,7 @@ describe( 'transform', () => { kate.setData( 'F[oo] Bar' ); john.setAttribute( 'bold', true ); - kate.move( [ 0, 7 ], [ 0, 1 ], [ 0, 3 ] ); + kate.move( [ 0, 7 ] ); syncClients(); diff --git a/tests/model/operation/transform/remove.js b/tests/model/operation/transform/remove.js index 8e2fdcf18..75635562f 100644 --- a/tests/model/operation/transform/remove.js +++ b/tests/model/operation/transform/remove.js @@ -158,13 +158,30 @@ describe( 'transform', () => { kate.wrap( 'blockQuote' ); syncClients(); + expectClients( 'Foo' ); + + john.undo(); + + syncClients(); + + expectClients( + 'Foo' + + '
' + + 'Bar' + + '
' + ); + } ); + it( 'element while removing, then undo #2', () => { + john.setData( 'Foo[Bar]' ); + kate.setData( 'Foo[Bar]' ); + + john.remove(); john.undo(); + kate.wrap( 'blockQuote' ); syncClients(); - // Actual content: - // FooBar
expectClients( 'Foo' + '
' + From 4cf77847d7a7fd967c64f99fd5e422273016c8d6 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 1 Aug 2018 12:59:51 +0200 Subject: [PATCH 18/26] Fixed: `SplitOperation` and `WrapOperation` `graveyardPosition` should stick to the node they "represent". --- src/model/operation/splitoperation.js | 4 ++++ src/model/operation/wrapoperation.js | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/model/operation/splitoperation.js b/src/model/operation/splitoperation.js index 0b00419d5..4ed0267c1 100644 --- a/src/model/operation/splitoperation.js +++ b/src/model/operation/splitoperation.js @@ -43,6 +43,10 @@ export default class SplitOperation extends Operation { this.position.stickiness = 'toNext'; this.graveyardPosition = graveyardPosition ? Position.createFromPosition( graveyardPosition ) : null; + + if ( this.graveyardPosition ) { + this.graveyardPosition.stickiness = 'toNext'; + } } /** diff --git a/src/model/operation/wrapoperation.js b/src/model/operation/wrapoperation.js index 5de2c6a68..4b502bd35 100644 --- a/src/model/operation/wrapoperation.js +++ b/src/model/operation/wrapoperation.js @@ -62,6 +62,10 @@ export default class WrapOperation extends Operation { this.element = elementOrPosition instanceof Element ? elementOrPosition : null; this.graveyardPosition = elementOrPosition instanceof Element ? null : Position.createFromPosition( elementOrPosition ); + + if ( this.graveyardPosition ) { + this.graveyardPosition.stickiness = 'toNext'; + } } /** From 69a90da0cd7998cc50b44c7e9ac1aa3cb6650005 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 1 Aug 2018 13:00:57 +0200 Subject: [PATCH 19/26] Fixed: Range#_getTransformedByUnwrapOperation if the operation is a special case of unwrapping a node that is already in the graveyard. --- src/model/position.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/model/position.js b/src/model/position.js index 4a91b11f9..a990b0b9b 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -599,15 +599,17 @@ export default class Position { unwrappedRange.start.isEqual( this ) || unwrappedRange.end.isEqual( this ); + let pos; + if ( isContained ) { - return this._getCombined( operation.position, operation.targetPosition ); + pos = this._getCombined( operation.position, operation.targetPosition ); } else if ( this.isEqual( operation.targetPosition ) ) { return Position.createFromPosition( this ); } else { - const pos = this._getTransformedByInsertion( operation.targetPosition, operation.howMany - 1 ); - - return pos._getTransformedByInsertion( operation.graveyardPosition, 1 ); + pos = this._getTransformedByInsertion( operation.targetPosition, operation.howMany ); } + + return pos._getTransformedByMove( operation.targetPosition.getShiftedBy( operation.howMany ), operation.graveyardPosition, 1 ); } /** From 20723dcd3043146faf89d90d3414c58870041a89 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 1 Aug 2018 13:02:18 +0200 Subject: [PATCH 20/26] Fixed: WrapOperation x MergeOperation will not create empty wrappings anymore. --- src/model/operation/transform.js | 51 +++++++++++++++++++------ tests/model/operation/transform/wrap.js | 4 +- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 0462b04f6..5cee039b6 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -970,6 +970,23 @@ setTransformation( MergeOperation, WrapOperation, ( a, b ) => { return [ a ]; } + // Case 2: Merged element is wrapped and this is the last (only) element in the wrap. + // Because of how this is resolved in `WrapOperation` x `MergeOperation`, we need to apply special handling here. + // If the last element from wrapped range is "removed" from it, the wrap is effectively on empty range. + // In that case, the wrapper element is moved to graveyard. This happens in `WrapOperation` x + // `MergeOperation` and we need to mirror it here. + // + if ( b.position.isEqual( a.deletionPosition ) && b.howMany == 1 ) { + // We need to change `MergeOperation#graveyardPosition` so the merged node is moved into the wrapper element. + // Since `UnwrapOperation` created from reverse has graveyard position at [ 0 ], we can safely set the path here to [ 0, 0 ]. + a.graveyardPosition = new Position( a.graveyardPosition.root, [ 0, 0 ] ); + + return [ + b.getReversed(), + a + ]; + } + a.sourcePosition = a.sourcePosition._getTransformedByWrapOperation( b ); a.targetPosition = a.targetPosition._getTransformedByWrapOperation( b ); @@ -1561,8 +1578,8 @@ setTransformation( SplitOperation, WrapOperation, ( a, b, context ) => { a.position = a.position._getTransformedByWrapOperation( b ); } - if ( a.graveyardPosition && b.graveyardPosition ) { - a.graveyardPosition = a.graveyardPosition._getTransformedByDeletion( b.graveyardPosition, 1 ); + if ( a.graveyardPosition ) { + a.graveyardPosition = a.graveyardPosition._getTransformedByWrapOperation( b ); } return [ a ]; @@ -1581,7 +1598,7 @@ setTransformation( SplitOperation, UnwrapOperation, ( a, b, context ) => { } if ( a.graveyardPosition ) { - a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 ); + a.graveyardPosition = a.graveyardPosition._getTransformedByUnwrapOperation( b ); } return [ a ]; @@ -1865,11 +1882,21 @@ setTransformation( UnwrapOperation, MergeOperation, ( a, b, context ) => { return [ b.getReversed(), a ]; } - const transformed = a.unwrappedRange._getTransformedByMergeOperation( b ); + // // Case 2: The element to unwrap was merged-to and has new nodes. + // // + // if ( a.position.hasSameParentAs( b.targetPosition ) ) { + // // Merge operation needs `howMany`! + // } - a.position = transformed.start; - a.position.stickiness = 'toPrevious'; - a.howMany = transformed.end.offset - transformed.start.offset; + if ( a.position.hasSameParentAs( b.graveyardPosition ) ) { + a.howMany++; + } + + if ( a.position.hasSameParentAs( b.deletionPosition ) ) { + a.howMany--; + } + + a.position = a.position._getTransformedByMergeOperation( b ); if ( !a.graveyardPosition.isEqual( b.graveyardPosition ) || !context.aIsStrong ) { a.graveyardPosition = a.graveyardPosition._getTransformedByMergeOperation( b ); @@ -1923,17 +1950,17 @@ setTransformation( UnwrapOperation, SplitOperation, ( a, b ) => { // Case 2: The split element is the last element in unwrapped element. In this case, we need to manually modify // `howMany` property because it wouldn't be correctly calculated by `_getTransformedBySplitOperation`. // - if ( b.insertionPosition.isEqual( a.position.getShiftedBy( a.howMany ) ) ) { + if ( a.position.hasSameParentAs( b.insertionPosition ) ) { a.howMany++; return [ a ]; } - const transformed = a.unwrappedRange._getTransformedBySplitOperation( b ); + a.position = a.position._getTransformedBySplitOperation( b ); - a.position = transformed.start; - a.position.stickiness = 'toPrevious'; - a.howMany = transformed.end.offset - transformed.start.offset; + if ( b.graveyardPosition && b.graveyardPosition.hasSameParentAs( a.position ) ) { + a.howMany--; + } a.graveyardPosition = a.graveyardPosition._getTransformedBySplitOperation( b ); diff --git a/tests/model/operation/transform/wrap.js b/tests/model/operation/transform/wrap.js index a24e011be..2e53adde2 100644 --- a/tests/model/operation/transform/wrap.js +++ b/tests/model/operation/transform/wrap.js @@ -241,7 +241,7 @@ describe( 'transform', () => { expectClients( 'FooBar' ); } ); - it.skip( 'element into paragraph, then undo', () => { + it( 'element into paragraph, then undo', () => { john.setData( 'Foo[Bar]' ); kate.setData( 'Foo[]Bar' ); @@ -249,7 +249,7 @@ describe( 'transform', () => { kate.merge(); syncClients(); - expectClients( 'FooBar
' ); + expectClients( 'FooBar' ); john.undo(); kate.undo(); From d7b475cb2ddc747068762f5abb0eebbfd052593d Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 1 Aug 2018 15:09:27 +0200 Subject: [PATCH 21/26] Fixed: Preventing "empty" wraps also in MoveOperation x WrapOperation cases. --- src/model/position.js | 11 ++- tests/model/operation/transform/wrap.js | 96 +++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/src/model/position.js b/src/model/position.js index a990b0b9b..8973e214b 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -604,12 +604,19 @@ export default class Position { if ( isContained ) { pos = this._getCombined( operation.position, operation.targetPosition ); } else if ( this.isEqual( operation.targetPosition ) ) { - return Position.createFromPosition( this ); + pos = Position.createFromPosition( this ); } else { pos = this._getTransformedByInsertion( operation.targetPosition, operation.howMany ); } - return pos._getTransformedByMove( operation.targetPosition.getShiftedBy( operation.howMany ), operation.graveyardPosition, 1 ); + const targetPosition = operation.targetPosition.getShiftedBy( operation.howMany ); + + if ( !targetPosition.isEqual( operation.graveyardPosition ) ) { + pos = pos._getTransformedByDeletion( targetPosition, 1 ); + pos = pos._getTransformedByInsertion( operation.graveyardPosition, 1 ); + } + + return pos; } /** diff --git a/tests/model/operation/transform/wrap.js b/tests/model/operation/transform/wrap.js index 2e53adde2..e03efc0e5 100644 --- a/tests/model/operation/transform/wrap.js +++ b/tests/model/operation/transform/wrap.js @@ -211,6 +211,102 @@ describe( 'transform', () => { } ); } ); + describe( 'by remove', () => { + it( 'remove the only wrapped element', () => { + john.setData( '[Foo]Bar' ); + kate.setData( '[Foo]Bar' ); + + john.wrap( 'blockQuote' ); + kate.remove(); + + syncClients(); + + expectClients( 'Bar' ); + } ); + + it( 'remove one of two wrapped elements', () => { + john.setData( '[FooBar]' ); + kate.setData( '[Foo]Bar' ); + + john.wrap( 'blockQuote' ); + kate.remove(); + + syncClients(); + + expectClients( '
Bar
' ); + } ); + + it( 'remove all wrapped elements', () => { + john.setData( '[FooBar]Xyz' ); + kate.setData( '[Foo]BarXyz' ); + + john.wrap( 'blockQuote' ); + + kate.remove(); + kate.setSelection( [ 0 ], [ 1 ] ); + kate.remove(); + + syncClients(); + + expectClients( 'Xyz' ); + } ); + + it( 'remove the only wrapped element with undo', () => { + john.setData( '[Foo]Bar' ); + kate.setData( '[Foo]Bar' ); + + john.wrap( 'blockQuote' ); + kate.remove(); + + syncClients(); + expectClients( 'Bar' ); + + john.undo(); + kate.undo(); + + syncClients(); + expectClients( 'FooBar' ); + } ); + + it( 'remove one of two wrapped elements with undo', () => { + john.setData( '[FooBar]' ); + kate.setData( '[Foo]Bar' ); + + john.wrap( 'blockQuote' ); + kate.remove(); + + syncClients(); + expectClients( '
Bar
' ); + + john.undo(); + kate.undo(); + + syncClients(); + expectClients( 'FooBar' ); + } ); + + it( 'remove all wrapped elements with undo', () => { + john.setData( '[FooBar]Xyz' ); + kate.setData( '[Foo]BarXyz' ); + + john.wrap( 'blockQuote' ); + + kate.remove(); + kate.setSelection( [ 0 ], [ 1 ] ); + kate.remove(); + + syncClients(); + expectClients( 'Xyz' ); + + john.undo(); + kate.undo(); + kate.undo(); + + syncClients(); + expectClients( 'FooBarXyz' ); + } ); + } ); + describe( 'by merge', () => { it( 'element into paragraph #1', () => { john.setData( '[Foo]Bar' ); From fd2f4f539750c527417457df0e9eb02d729e1db2 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 1 Aug 2018 17:02:51 +0200 Subject: [PATCH 22/26] Fix: Final OT followup fixes after merging several branches. --- src/model/operation/transform.js | 46 ++++++++++++++++++++---------- tests/model/liverange.js | 34 ++++++++++++++++++---- tests/model/operation/transform.js | 10 ++++++- 3 files changed, 69 insertions(+), 21 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 1d6a8f156..b1cd0724b 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -60,10 +60,10 @@ function transform( a, b, context = {} ) { log.error( 'Transformed operation', a ); log.error( 'Operation transformed by', b ); log.error( 'context.aIsStrong', context.aIsStrong ); - log.error( 'context.wasUndone( a )', context.wasUndone( a ) ); - log.error( 'context.wasUndone( b )', context.wasUndone( b ) ); - log.error( 'context.getRelation( a, b )', context.getRelation( a, b ) ); - log.error( 'context.getRelation( b, a )', context.getRelation( b, a ) ); + log.error( 'context.aWasUndone', context.aWasUndone ); + log.error( 'context.bWasUndone', context.bWasUndone ); + log.error( 'context.abRelation', context.abRelation ); + log.error( 'context.baRelation', context.baRelation ); throw e; } @@ -500,15 +500,18 @@ setTransformation( AttributeOperation, MergeOperation, ( a, b ) => { // Do it only, if there is more than one element in attribute range. If there is only one element, // it will be handled by the default algorithm. // - const howMany = a.range.end.offset - a.range.start.offset; - - if ( howMany > 1 && a.range.start.hasSameParentAs( b.deletionPosition ) ) { + if ( a.range.start.hasSameParentAs( b.deletionPosition ) ) { if ( a.range.containsPosition( b.deletionPosition ) || a.range.start.isEqual( b.deletionPosition ) ) { ranges.push( Range.createFromPositionAndShift( b.graveyardPosition, 1 ) ); } } - ranges.push( a.range._getTransformedByMergeOperation( b ) ); + const range = a.range._getTransformedByMergeOperation( b ); + + // Do not add empty (collapsed) ranges to the result. `range` may be collapsed if it contained only the merged element. + if ( !range.isCollapsed ) { + ranges.push( range ); + } // Create `AttributeOperation`s out of the ranges. return ranges.map( range => { @@ -605,10 +608,13 @@ setTransformation( AttributeOperation, SplitOperation, ( a, b ) => { if ( a.range.start.hasSameParentAs( b.position ) && a.range.containsPosition( b.position ) ) { const secondPart = a.clone(); - secondPart.range.start = Position.createFromPosition( b.moveTargetPosition ); - secondPart.range.end = a.range.end._getCombined( b.position, b.moveTargetPosition ); + secondPart.range = new Range( + Position.createFromPosition( b.moveTargetPosition ), + a.range.end._getCombined( b.position, b.moveTargetPosition ) + ); a.range.end = Position.createFromPosition( b.position ); + a.range.end.stickiness = 'toPrevious'; return [ a, secondPart ]; } @@ -1036,7 +1042,7 @@ 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 || context.getRelation( a, b ) == 'insertBefore'; + let aIsStrong = context.aIsStrong; if ( context.abRelation == 'insertBefore' ) { aIsStrong = true; @@ -1379,6 +1385,7 @@ setTransformation( RenameOperation, InsertOperation, ( a, b ) => { setTransformation( RenameOperation, MergeOperation, ( a, b ) => { if ( a.position.isEqual( b.deletionPosition ) ) { a.position = Position.createFromPosition( b.graveyardPosition ); + a.position.stickiness = 'toNext'; return [ a ]; } @@ -1613,15 +1620,24 @@ setTransformation( WrapOperation, InsertOperation, ( a, b, context ) => { } ); setTransformation( WrapOperation, MergeOperation, ( a, b ) => { + if ( a.graveyardPosition ) { + a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 ); + } + + // Case 1: The element to wrap got merged. + // + if ( a.position.isEqual( b.deletionPosition ) ) { + a.position = Position.createFromPosition( b.graveyardPosition ); + a.position.stickiness = 'toNext'; + + return [ a ]; + } + const transformed = a.wrappedRange._getTransformedByMergeOperation( b ); a.position = transformed.start; a.howMany = transformed.end.offset - transformed.start.offset; - if ( a.graveyardPosition ) { - a.graveyardPosition = a.graveyardPosition._getTransformedByInsertion( b.graveyardPosition, 1 ); - } - return [ a ]; } ); diff --git a/tests/model/liverange.js b/tests/model/liverange.js index 78a56ac7d..ed005f2b1 100644 --- a/tests/model/liverange.js +++ b/tests/model/liverange.js @@ -9,6 +9,8 @@ import Position from '../../src/model/position'; import LiveRange from '../../src/model/liverange'; import Range from '../../src/model/range'; import Text from '../../src/model/text'; +import MoveOperation from '../../src/model/operation/moveoperation'; +import MergeOperation from '../../src/model/operation/mergeoperation'; import { stringify, setData } from '../../src/dev-utils/model'; describe( 'LiveRange', () => { @@ -154,20 +156,42 @@ describe( 'LiveRange', () => { expect( spy.args[ 0 ][ 2 ].isEqual( sourcePosition ) ).to.be.true; } ); + // This scenario is hypothetically possible during OT if the element to merge-into was removed. + // In that case a live range inside the merged element will be merged into an element which is in graveyard. + // Because it may happen only in OT, in the test below we will generate operations by hand. it( 'should pass deletion position if range was removed (merge)', () => { - const live = new LiveRange( new Position( root, [ 1 ] ), new Position( root, [ 2 ] ) ); + const live = new LiveRange( new Position( root, [ 1, 0 ] ), new Position( root, [ 1, 1 ] ) ); const spy = sinon.spy(); live.on( 'change:range', spy ); - const mergePosition = new Position( root, [ 1 ] ); - model.change( writer => { - writer.merge( mergePosition ); + const batch = writer.batch; + const gy = model.document.graveyard; + + const remove = new MoveOperation( + new Position( root, [ 0 ] ), + 1, + new Position( gy, [ 0 ] ), + model.document.version + ); + + const merge = new MergeOperation( + new Position( root, [ 0, 0 ] ), + new Position( gy, [ 0, 0 ] ), + new Position( gy, [ 0 ] ), + model.document.version + 1 + ); + + batch.addOperation( remove ); + model.applyOperation( remove ); + + batch.addOperation( merge ); + model.applyOperation( merge ); } ); // Second parameter is deletion position. - expect( spy.args[ 0 ][ 2 ].isEqual( mergePosition ) ).to.be.true; + expect( spy.args[ 1 ][ 2 ].isEqual( new Position( root, [ 0 ] ) ) ).to.be.true; } ); describe( 'should get transformed and fire change:range if', () => { diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index 490ca2c1a..132df0102 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -73,10 +73,18 @@ describe( 'transform', () => { a.position = null; expect( () => { - transform.transform( a, b ); + transform.transform( a, b, { + aIsStrong: true, + aWasUndone: false, + bWasUndone: false, + abRelation: null, + baRelation: null + } ); } ).to.throw(); sinon.assert.called( spy ); + + log.error.restore(); } ); describe( 'InsertOperation', () => { From 62a3cc20fab3f08b5522114df8db6ceb7971373c Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 13:19:07 +0200 Subject: [PATCH 23/26] Change: Make second parameter of `LiveRange#event:change` an object. --- src/model/documentselection.js | 4 ++-- src/model/liverange.js | 7 +++---- tests/model/liverange.js | 8 ++++---- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 20d462d8a..853c1b748 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -750,14 +750,14 @@ class LiveSelection extends Selection { const liveRange = LiveRange.createFromRange( range ); - liveRange.on( 'change:range', ( evt, oldRange, deletionPosition ) => { + liveRange.on( 'change:range', ( evt, oldRange, data ) => { this._hasChangedRange = true; // If `LiveRange` is in whole moved to the graveyard, save necessary data. It will be fixed on `Model#applyOperation` event. if ( liveRange.root == this._document.graveyard ) { this._fixGraveyardRangesData.push( { liveRange, - sourcePosition: deletionPosition + sourcePosition: data.deletionPosition } ); } } ); diff --git a/src/model/liverange.js b/src/model/liverange.js index 6799bc391..73480f7b8 100644 --- a/src/model/liverange.js +++ b/src/model/liverange.js @@ -143,10 +143,9 @@ function transform( operation ) { const result = Range.createFromRanges( ranges ); const boundariesChanged = !result.isEqual( this ); const contentChanged = doesOperationChangeRangeContent( this, operation ); + let deletionPosition = null; if ( boundariesChanged ) { - let deletionPosition = null; - if ( result.root.rootName == '$graveyard' ) { if ( operation.type == 'remove' ) { deletionPosition = operation.sourcePosition; @@ -162,10 +161,10 @@ function transform( operation ) { this.start = result.start; this.end = result.end; - this.fire( 'change:range', oldRange, deletionPosition ); + this.fire( 'change:range', oldRange, { deletionPosition } ); } else if ( contentChanged ) { // If range boundaries have not changed, but there was change inside the range, fire `change:content` event. - this.fire( 'change:content', Range.createFromRange( this ), null ); + this.fire( 'change:content', Range.createFromRange( this ), { deletionPosition } ); } } diff --git a/tests/model/liverange.js b/tests/model/liverange.js index ed005f2b1..dd74cf780 100644 --- a/tests/model/liverange.js +++ b/tests/model/liverange.js @@ -113,7 +113,7 @@ describe( 'LiveRange', () => { expect( spy.args[ 0 ][ 1 ].isEqual( copy ) ).to.be.true; // Second parameter is null for operations that did not move the range into graveyard. - expect( spy.args[ 0 ][ 2 ] ).to.be.null; + expect( spy.args[ 0 ][ 2 ].deletionPosition ).to.be.null; } ); it( 'should fire change:content event when content inside the range has changed', () => { @@ -137,7 +137,7 @@ describe( 'LiveRange', () => { expect( spy.args[ 0 ][ 1 ].isEqual( live ) ).to.be.true; // Second parameter is null for operations that did not move the range into graveyard. - expect( spy.args[ 0 ][ 2 ] ).to.be.null; + expect( spy.args[ 0 ][ 2 ].deletionPosition ).to.be.null; } ); it( 'should pass deletion position if range was removed (remove)', () => { @@ -153,7 +153,7 @@ describe( 'LiveRange', () => { } ); // Second parameter is deletion position. - expect( spy.args[ 0 ][ 2 ].isEqual( sourcePosition ) ).to.be.true; + expect( spy.args[ 0 ][ 2 ].deletionPosition.isEqual( sourcePosition ) ).to.be.true; } ); // This scenario is hypothetically possible during OT if the element to merge-into was removed. @@ -191,7 +191,7 @@ describe( 'LiveRange', () => { } ); // Second parameter is deletion position. - expect( spy.args[ 1 ][ 2 ].isEqual( new Position( root, [ 0 ] ) ) ).to.be.true; + expect( spy.args[ 1 ][ 2 ].deletionPosition.isEqual( new Position( root, [ 0 ] ) ) ).to.be.true; } ); describe( 'should get transformed and fire change:range if', () => { From a2b3803ed41ac9d4f95079c88011f8aebeeff645 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 13:19:34 +0200 Subject: [PATCH 24/26] Change: Export functions in operation/transform.js as separate functions, not an object. --- src/model/operation/transform.js | 9 +- tests/model/operation/transform.js | 302 +++++++++++------------ tests/model/operation/transform/utils.js | 6 +- 3 files changed, 156 insertions(+), 161 deletions(-) diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index b1cd0724b..bca951da0 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -50,7 +50,7 @@ function updateBaseVersions( operations, baseVersion ) { return operations; } -function transform( a, b, context = {} ) { +export function transform( a, b, context = {} ) { const transformationFunction = getTransformation( a, b ); try { @@ -69,7 +69,7 @@ function transform( a, b, context = {} ) { } } -function transformSets( operationsA, operationsB, options ) { +export function transformSets( operationsA, operationsB, options ) { operationsA = operationsA.slice(); operationsB = operationsB.slice(); @@ -163,11 +163,6 @@ function transformSets( operationsA, operationsB, options ) { return { operationsA, operationsB }; } -export default { - transform, - transformSets -}; - function padWithNoOps( operations, howMany ) { for ( let i = 0; i < howMany; i++ ) { operations.push( new NoOperation( 0 ) ); diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index 132df0102..f7903230c 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -import transform from '../../../src/model/operation/transform'; +import { transform } from '../../../src/model/operation/transform'; import Model from '../../../src/model/model'; import RootElement from '../../../src/model/rootelement'; @@ -54,7 +54,7 @@ describe( 'transform', () => { } } - const contextIsStrong = { + const strongContext = { aIsStrong: true }; @@ -73,7 +73,7 @@ describe( 'transform', () => { a.position = null; expect( () => { - transform.transform( a, b, { + transform( a, b, { aIsStrong: true, aWasUndone: false, bWasUndone: false, @@ -112,7 +112,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -125,7 +125,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -139,7 +139,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -153,7 +153,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -166,7 +166,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -180,7 +180,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -193,7 +193,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.path[ 1 ] += 2; expect( transOp.length ).to.equal( 1 ); @@ -207,7 +207,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -224,7 +224,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -241,7 +241,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -257,7 +257,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -271,7 +271,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.offset--; expect( transOp.length ).to.equal( 1 ); @@ -286,7 +286,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -300,7 +300,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -315,7 +315,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -329,7 +329,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -344,7 +344,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expected.position.offset += 2; expect( transOp.length ).to.equal( 1 ); @@ -359,7 +359,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.path[ 1 ] -= 1; expect( transOp.length ).to.equal( 1 ); @@ -374,7 +374,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -388,7 +388,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.path[ 1 ] += 2; expect( transOp.length ).to.equal( 1 ); @@ -403,7 +403,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -417,7 +417,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.path = [ 1, 2, 2 ]; expect( transOp.length ).to.equal( 1 ); @@ -432,7 +432,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.position.path = [ 1, 2 ]; expect( transOp.length ).to.equal( 1 ); @@ -444,7 +444,7 @@ describe( 'transform', () => { it( 'no operation update', () => { const transformBy = new NoOperation( 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -455,7 +455,7 @@ describe( 'transform', () => { it( 'no position update', () => { const transformBy = new RenameOperation( new Position( root, [ 0, 2, 0 ] ), 'oldName', 'newName', 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -467,7 +467,7 @@ describe( 'transform', () => { const newRange = new Range( new Position( root, [ 0, 2, 0 ] ), new Position( root, [ 0, 2, 4 ] ) ); const transformBy = new MarkerOperation( 'name', null, newRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -503,7 +503,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.range.start.offset += 2; expected.range.end.offset += 2; @@ -519,7 +519,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.range.start.offset += 2; expected.range.end.offset += 2; @@ -538,7 +538,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.range.start.offset--; expected.range.end.offset--; @@ -555,7 +555,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.range.start.offset += 2; expected.range.end.offset += 2; @@ -572,7 +572,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 2 ); @@ -594,7 +594,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 2 ); @@ -617,7 +617,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.range.start.path = [ 2, 4, 2, 1 ]; expected.range.end.path = [ 2, 4, 2, 4 ]; @@ -634,7 +634,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 3 ); @@ -662,7 +662,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.range.start.path = [ 2, 4, 1 ]; expected.range.end.path = [ 2, 4, 4 ]; @@ -679,7 +679,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 2 ); @@ -701,7 +701,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 3 ); @@ -745,7 +745,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -765,7 +765,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -782,7 +782,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -797,7 +797,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -812,7 +812,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { @@ -829,7 +829,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { @@ -846,7 +846,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expected.oldValue = 'xyz'; @@ -864,7 +864,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -875,7 +875,7 @@ describe( 'transform', () => { it( 'no operation update', () => { const transformBy = new NoOperation( 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -886,7 +886,7 @@ describe( 'transform', () => { it( 'no position update', () => { const transformBy = new RenameOperation( new Position( root, [ 0 ] ), 'oldName', 'newName', 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -898,7 +898,7 @@ describe( 'transform', () => { const newRange = new Range( new Position( root, [ 0, 2, 0 ] ), new Position( root, [ 0, 2, 8 ] ) ); const transformBy = new MarkerOperation( 'name', null, newRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -935,7 +935,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -948,7 +948,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -961,7 +961,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.offset += 2; @@ -976,7 +976,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -989,7 +989,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.path[ 1 ] += 2; @@ -1004,7 +1004,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1017,7 +1017,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.offset += 2; @@ -1032,7 +1032,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1045,7 +1045,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.path[ 1 ] += 2; @@ -1060,7 +1060,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1073,7 +1073,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1086,7 +1086,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1099,7 +1099,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -1115,7 +1115,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1132,7 +1132,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1149,7 +1149,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1165,7 +1165,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1179,7 +1179,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.offset += 2; @@ -1195,7 +1195,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1209,7 +1209,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.offset -= 2; @@ -1225,7 +1225,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1239,7 +1239,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.path[ 1 ] += 2; @@ -1255,7 +1255,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1269,7 +1269,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.path[ 1 ] -= 1; @@ -1285,7 +1285,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1299,7 +1299,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.offset += 2; @@ -1315,7 +1315,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1329,7 +1329,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.offset -= 2; @@ -1345,7 +1345,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1359,7 +1359,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.path[ 1 ] += 2; @@ -1375,7 +1375,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1389,7 +1389,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.path[ 1 ] -= 2; @@ -1405,7 +1405,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1419,7 +1419,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 2 ); @@ -1439,7 +1439,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -1456,7 +1456,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1470,7 +1470,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1484,7 +1484,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.path = [ 4, 3, 4 ]; @@ -1500,7 +1500,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.targetPosition.path = [ 0, 2, 3 ]; @@ -1516,7 +1516,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); const reversed = transformBy.getReversed(); expected.sourcePosition = reversed.sourcePosition; @@ -1535,7 +1535,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { @@ -1551,7 +1551,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expected.sourcePosition.path = [ 4, 1, 0 ]; @@ -1567,7 +1567,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { @@ -1583,7 +1583,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expected.sourcePosition.path = [ 4, 1, 1 ]; @@ -1601,7 +1601,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -1621,7 +1621,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); @@ -1639,7 +1639,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.sourcePosition.path = [ 2, 2, 3 ]; expected.howMany = 1; @@ -1659,7 +1659,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 2 ); @@ -1682,7 +1682,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.howMany = 1; @@ -1698,7 +1698,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 2 ); @@ -1723,7 +1723,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 2 ); @@ -1749,7 +1749,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 2 ); @@ -1773,7 +1773,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.howMany = 1; @@ -1789,7 +1789,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 2 ); @@ -1814,7 +1814,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 2 ); @@ -1840,7 +1840,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -1861,7 +1861,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 2 ); @@ -1885,7 +1885,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 3 ); @@ -1917,7 +1917,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -1936,7 +1936,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); @@ -1959,7 +1959,7 @@ describe( 'transform', () => { } ); it( 'should skip context.aIsStrong and be less important than MoveOperation', () => { - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); @@ -1973,7 +1973,7 @@ describe( 'transform', () => { it( 'no operation update', () => { const transformBy = new NoOperation( 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1984,7 +1984,7 @@ describe( 'transform', () => { it( 'no position update', () => { const transformBy = new RenameOperation( new Position( root, [ 2, 2, 4 ] ), 'oldName', 'newName', 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -1996,7 +1996,7 @@ describe( 'transform', () => { const newRange = new Range( new Position( root, [ 2, 2, 3 ] ), new Position( root, [ 2, 2, 8 ] ) ); const transformBy = new MarkerOperation( 'name', null, newRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2015,7 +2015,7 @@ describe( 'transform', () => { const sourcePosition = Position.createFromPosition( transformBy.targetPosition ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2046,7 +2046,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2066,7 +2066,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2083,7 +2083,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2099,7 +2099,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2110,7 +2110,7 @@ describe( 'transform', () => { it( 'no operation update', () => { const transformBy = new NoOperation( 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2121,7 +2121,7 @@ describe( 'transform', () => { it( 'no position update', () => { const transformBy = new RenameOperation( new Position( root, [ 0, 2, 0 ] ), 'oldName', 'newName', 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2133,7 +2133,7 @@ describe( 'transform', () => { const newRange = new Range( new Position( root, [ 0, 2, 0 ] ), new Position( root, [ 0, 2, 8 ] ) ); const transformBy = new MarkerOperation( 'name', null, newRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2162,7 +2162,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2178,7 +2178,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2191,7 +2191,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2207,7 +2207,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2227,7 +2227,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2244,7 +2244,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2256,7 +2256,7 @@ describe( 'transform', () => { const newRange = new Range( new Position( root, [ 0, 2, 0 ] ), new Position( root, [ 0, 2, 8 ] ) ); const transformBy = new MarkerOperation( 'name', null, newRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2272,7 +2272,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2286,7 +2286,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { @@ -2302,7 +2302,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expect( transOp.length ).to.equal( 1 ); @@ -2320,7 +2320,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2337,7 +2337,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2354,7 +2354,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2371,7 +2371,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2388,7 +2388,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2405,7 +2405,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); @@ -2437,7 +2437,7 @@ describe( 'transform', () => { op.newRange = null; const transformBy = new InsertOperation( Position.createAt( root, 0 ), [ nodeA, nodeB ], 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.newRange = null; expected.oldRange.start.offset = 3; @@ -2452,7 +2452,7 @@ describe( 'transform', () => { op.oldRange = null; const transformBy = new InsertOperation( Position.createAt( root, 8 ), [ nodeA, nodeB ], 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.oldRange = null; expected.newRange.start.offset = 12; @@ -2476,7 +2476,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2489,7 +2489,7 @@ describe( 'transform', () => { op.newRange = null; const transformBy = new MoveOperation( Position.createAt( root, 0 ), 1, Position.createAt( root, 20 ), 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.newRange = null; expected.oldRange.start.offset = 0; @@ -2501,7 +2501,7 @@ describe( 'transform', () => { it( 'moved range contains oldRange and is before newRange: update oldRange and newRange', () => { const transformBy = new MoveOperation( Position.createAt( root, 2 ), 2, Position.createAt( root, 20 ), 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.oldRange.start.offset = 1; expected.oldRange.end.offset = 2; @@ -2517,7 +2517,7 @@ describe( 'transform', () => { op.oldRange = null; const transformBy = new MoveOperation( Position.createAt( root, 20 ), 2, Position.createAt( root, 11 ), 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.oldRange = null; expected.newRange.start.offset = 10; @@ -2529,7 +2529,7 @@ describe( 'transform', () => { it( 'target position is inside oldRange and before newRange: update oldRange and newRange', () => { const transformBy = new MoveOperation( Position.createAt( root, 20 ), 4, Position.createAt( root, 2 ), 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expected.oldRange.start.offset = 1; expected.oldRange.end.offset = 8; @@ -2551,7 +2551,7 @@ describe( 'transform', () => { 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2562,7 +2562,7 @@ describe( 'transform', () => { it( 'no operation update', () => { const transformBy = new RenameOperation( new Position( root, [ 1 ] ), 'oldName', 'newName', 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2573,7 +2573,7 @@ describe( 'transform', () => { it( 'different marker name: no operation update', () => { const transformBy = new MarkerOperation( 'otherName', oldRange, newRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], expected ); @@ -2583,7 +2583,7 @@ describe( 'transform', () => { const anotherRange = Range.createFromParentsAndOffsets( root, 2, root, 2 ); const transformBy = new MarkerOperation( 'name', oldRange, anotherRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy ); + const transOp = transform( op, transformBy ); expect( transOp.length ).to.equal( 1 ); expectOperation( transOp[ 0 ], { @@ -2595,7 +2595,7 @@ describe( 'transform', () => { const anotherRange = Range.createFromParentsAndOffsets( root, 2, root, 2 ); const transformBy = new MarkerOperation( 'name', oldRange, anotherRange, model.markers, 0 ); - const transOp = transform.transform( op, transformBy, contextIsStrong ); + const transOp = transform( op, transformBy, strongContext ); expected.oldRange = anotherRange; diff --git a/tests/model/operation/transform/utils.js b/tests/model/operation/transform/utils.js index 016842f9c..12cccb7bb 100644 --- a/tests/model/operation/transform/utils.js +++ b/tests/model/operation/transform/utils.js @@ -7,7 +7,7 @@ import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; import BlockQuoteEditing from '@ckeditor/ckeditor5-block-quote/src/blockquoteediting'; import { getData, parse } from '../../../../src/dev-utils/model'; -import transform from '../../../../src/model/operation/transform'; +import { transformSets } from '../../../../src/model/operation/transform'; import Position from '../../../../src/model/position'; import Range from '../../../../src/model/range'; import OperationFactory from '../../../../src/model/operation/operationfactory'; @@ -290,9 +290,9 @@ export function syncClients() { }; if ( localClient.orderNumber < remoteClient.orderNumber ) { - remoteOperationsTransformed = transform.transformSets( localOperations, remoteOperations, options ).operationsB; + remoteOperationsTransformed = transformSets( localOperations, remoteOperations, options ).operationsB; } else { - remoteOperationsTransformed = transform.transformSets( remoteOperations, localOperations, options ).operationsA; + remoteOperationsTransformed = transformSets( remoteOperations, localOperations, options ).operationsA; } localClient.editor.model.enqueueChange( 'transparent', writer => { From 6dca97ec7e35e2caec8cc954f5c508c17be5a274 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 14:28:54 +0200 Subject: [PATCH 25/26] Changed: Make `Schema#getValidRanges` a generator. --- src/model/schema.js | 37 +++++++++++-------------------------- tests/model/schema.js | 2 +- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index da6a66aa3..180ec353a 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -665,19 +665,14 @@ export default class Schema { * * @param {Array.} ranges Ranges to be validated. * @param {String} attribute The name of the attribute to check. - * @returns {Array.} Ranges in which the attribute is allowed. + * @returns {Iterator.} Ranges in which the attribute is allowed. */ - getValidRanges( ranges, attribute ) { + * getValidRanges( ranges, attribute ) { ranges = convertToMinimalFlatRanges( ranges ); - const result = []; for ( const range of ranges ) { - const validRanges = this._getValidRangesForRange( range, attribute ); - - result.push( ...validRanges ); + yield* this._getValidRangesForRange( range, attribute ); } - - return result; } /** @@ -689,22 +684,20 @@ export default class Schema { * @private * @param {module:engine/model/range~Range} range Range to process. * @param {String} attribute The name of the attribute to check. - * @returns {Array.} Ranges in which the attribute is allowed. + * @returns {Iterator.} Ranges in which the attribute is allowed. */ - _getValidRangesForRange( range, attribute ) { - const result = []; - + * _getValidRangesForRange( range, attribute ) { let start = range.start; let end = range.start; for ( const item of range.getItems( { shallow: true } ) ) { if ( item.is( 'element' ) ) { - result.push( ...this._getValidRangesForRange( Range.createIn( item ), attribute ) ); + yield* this._getValidRangesForRange( Range.createIn( item ), attribute ); } if ( !this.checkAttribute( item, attribute ) ) { if ( !start.isEqual( end ) ) { - result.push( new Range( start, end ) ); + yield new Range( start, end ); } start = Position.createAfter( item ); @@ -714,10 +707,8 @@ export default class Schema { } if ( !start.isEqual( end ) ) { - result.push( new Range( start, end ) ); + yield new Range( start, end ); } - - return result; } /** @@ -1602,15 +1593,9 @@ function* combineWalkers( backward, forward ) { // all those minimal flat ranges. // // @param {Array.} ranges Ranges to process. -// @returns {Array.} Minimal flat ranges of given `ranges`. -function convertToMinimalFlatRanges( ranges ) { - const result = []; - +// @returns {Iterator.} Minimal flat ranges of given `ranges`. +function* convertToMinimalFlatRanges( ranges ) { for ( const range of ranges ) { - const minimal = range.getMinimalFlatRanges(); - - result.push( ...minimal ); + yield* range.getMinimalFlatRanges(); } - - return result; } diff --git a/tests/model/schema.js b/tests/model/schema.js index fd85549a7..890341eb6 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1178,7 +1178,7 @@ describe( 'Schema', () => { setData( model, '[

foobar

]' ); - const validRanges = schema.getValidRanges( doc.selection.getRanges(), 'foo' ); + const validRanges = Array.from( schema.getValidRanges( doc.selection.getRanges(), 'foo' ) ); expect( validRanges.length ).to.equal( 2 ); From 3d9af07b85899c5350dc2118e42b002bd264e99c Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 9 Aug 2018 14:36:31 +0200 Subject: [PATCH 26/26] Tests: Change the name of helper function. --- tests/model/schema.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/model/schema.js b/tests/model/schema.js index 890341eb6..b02a606c8 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1121,7 +1121,7 @@ describe( 'Schema', () => { schema.extend( '$text', { allowIn: '$root' } ); } ); - function test( input, attribute, output ) { + function testValidRangesForAttribute( input, attribute, output ) { setData( model, input ); const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute ); @@ -1133,7 +1133,7 @@ describe( 'Schema', () => { it( 'should return a range with p for an attribute allowed only on p', () => { schema.extend( 'p', { allowAttributes: 'foo' } ); - test( + testValidRangesForAttribute( '[

foobar

]', 'foo', '[

foobar

]' @@ -1143,7 +1143,7 @@ describe( 'Schema', () => { it( 'should return ranges on text nodes for an attribute allowed only on text', () => { schema.extend( '$text', { allowAttributes: 'bold' } ); - test( + testValidRangesForAttribute( '[

foobar

]', 'bold', '

[foo][bar]

' @@ -1153,7 +1153,7 @@ describe( 'Schema', () => { it( 'should return a range on img for an attribute allowed only on img', () => { schema.extend( 'img', { allowAttributes: 'src' } ); - test( + testValidRangesForAttribute( '[

foobar

]', 'src', '

foo[]bar

' @@ -1164,7 +1164,7 @@ describe( 'Schema', () => { schema.extend( '$text', { allowAttributes: 'bold' } ); schema.extend( 'img', { allowAttributes: 'bold' } ); - test( + testValidRangesForAttribute( '[

foobar

]', 'bold', '

[foobar]

' @@ -1192,7 +1192,7 @@ describe( 'Schema', () => { it( 'should not break a range if children are not allowed to have the attribute', () => { schema.extend( 'p', { allowAttributes: 'foo' } ); - test( + testValidRangesForAttribute( '[

foo

bar

]', 'foo', '[

foo

bar

]' @@ -1202,7 +1202,7 @@ describe( 'Schema', () => { it( 'should search deeply', () => { schema.extend( '$text', { allowAttributes: 'bold', allowIn: 'img' } ); - test( + testValidRangesForAttribute( '[

fooxxxbar

]', 'bold', '

[foo][xxx][bar]

' @@ -1212,7 +1212,7 @@ describe( 'Schema', () => { it( 'should work with multiple ranges', () => { schema.extend( '$text', { allowAttributes: 'bold' } ); - test( + testValidRangesForAttribute( '[

a

b

]

c

[d]

', 'bold', '

[a]

[b]

c

[d]

' @@ -1222,7 +1222,7 @@ describe( 'Schema', () => { it( 'should work with non-flat ranges', () => { schema.extend( '$text', { allowAttributes: 'bold' } ); - test( + testValidRangesForAttribute( '[

a

b

c]

d

', 'bold', '

[a]

[b]

[c]

d

' @@ -1232,7 +1232,7 @@ describe( 'Schema', () => { it( 'should not leak beyond the given ranges', () => { schema.extend( '$text', { allowAttributes: 'bold' } ); - test( + testValidRangesForAttribute( '[

foo

b]a[r

x]yz

', 'bold', '

[foo]

[b]a[r]

[x]yz

' @@ -1249,7 +1249,7 @@ describe( 'Schema', () => { } } ); - test( + testValidRangesForAttribute( '[

fooxx]xbar

', 'bold', '

[foo]xxxbar

'