From 01ffc6f44fd217b1c50e98f72fe0a66f42a3c21e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 27 Mar 2017 11:23:46 +0200 Subject: [PATCH 1/2] Changed: reversed `ReinsertOperation` now removes nodes back to the same graveyard holder. --- src/model/operation/reinsertoperation.js | 7 ++++++- src/model/operation/transform.js | 2 +- tests/model/operation/reinsertoperation.js | 9 +++++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/model/operation/reinsertoperation.js b/src/model/operation/reinsertoperation.js index fea97cf1e..10fd26f25 100644 --- a/src/model/operation/reinsertoperation.js +++ b/src/model/operation/reinsertoperation.js @@ -45,7 +45,12 @@ export default class ReinsertOperation extends MoveOperation { * @returns {module:engine/model/operation/removeoperation~RemoveOperation} */ getReversed() { - return new RemoveOperation( this.targetPosition, this.howMany, this.baseVersion + 1 ); + const removeOp = new RemoveOperation( this.targetPosition, this.howMany, this.baseVersion + 1 ); + + // Make sure that nodes are put back into the `$graveyardHolder` from which they got reinserted. + removeOp.targetPosition = this.sourcePosition; + + return removeOp; } /** diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index e1ddf26fd..42bb99e6b 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -388,7 +388,7 @@ const ot = { const aTarget = a.targetPosition.path[ 0 ]; const bTarget = b.targetPosition.path[ 0 ]; - if ( aTarget >= bTarget && isStrong ) { + if ( aTarget > bTarget || ( aTarget == bTarget && isStrong ) ) { // Do not change original operation! a = a.clone(); a.targetPosition.path[ 0 ]++; diff --git a/tests/model/operation/reinsertoperation.js b/tests/model/operation/reinsertoperation.js index 9dec11f16..f8a90ef1b 100644 --- a/tests/model/operation/reinsertoperation.js +++ b/tests/model/operation/reinsertoperation.js @@ -61,14 +61,19 @@ describe( 'ReinsertOperation', () => { expect( clone.baseVersion ).to.equal( operation.baseVersion ); } ); - it( 'should create a RemoveOperation as a reverse', () => { + it( 'should create a correct RemoveOperation as a reverse', () => { + // Test reversed operation's target position. + graveyard.appendChildren( new Element( '$graveyardHolder' ) ); + let 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.root ).to.equal( graveyardPosition.root ); + + // Reversed `ReinsertOperation` should target back to the same graveyard holder. + expect( reverse.targetPosition.isEqual( graveyardPosition ) ).to.be.true; } ); it( 'should undo reinsert set of nodes by applying reverse operation', () => { From 97f8b51c7cabacf70411c25567f2fb9ee862b098 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 28 Mar 2017 11:49:30 +0200 Subject: [PATCH 2/2] Changed: `RemoveOperation#_needsHolderElement` will be set manually in scripts that create `RemoveOperation` instance. --- src/model/operation/reinsertoperation.js | 1 + src/model/operation/removeoperation.js | 54 +++++++---------- src/model/operation/transform.js | 21 +++++-- tests/model/operation/reinsertoperation.js | 3 + tests/model/operation/removeoperation.js | 70 ++++++---------------- tests/model/operation/transform.js | 41 +++++++++++++ 6 files changed, 99 insertions(+), 91 deletions(-) diff --git a/src/model/operation/reinsertoperation.js b/src/model/operation/reinsertoperation.js index 10fd26f25..75bbe7cfb 100644 --- a/src/model/operation/reinsertoperation.js +++ b/src/model/operation/reinsertoperation.js @@ -49,6 +49,7 @@ export default class ReinsertOperation extends MoveOperation { // Make sure that nodes are put back into the `$graveyardHolder` from which they got reinserted. removeOp.targetPosition = this.sourcePosition; + removeOp._needsHolderElement = false; return removeOp; } diff --git a/src/model/operation/removeoperation.js b/src/model/operation/removeoperation.js index c4116293b..30412cd60 100644 --- a/src/model/operation/removeoperation.js +++ b/src/model/operation/removeoperation.js @@ -30,6 +30,25 @@ export default class RemoveOperation extends MoveOperation { const graveyardPosition = new Position( graveyard, [ graveyard.maxOffset, 0 ] ); super( position, howMany, graveyardPosition, baseVersion ); + + /** + * Flag informing whether this operation should insert "holder" element (`true`) or should move removed nodes + * into existing "holder" element (`false`). + * + * The flag should be set to `true` for each "new" `RemoveOperation` that is each `RemoveOperation` originally + * created to remove some nodes from document (most likely created through `Batch` API). + * + * The flag should be set to `false` for each `RemoveOperation` that got created by splitting the original + * `RemoveOperation`, for example during operational transformation. + * + * The flag should be set to `false` whenever removing nodes that were re-inserted from graveyard. This will + * ensure correctness of all other operations that might change something on those nodes. This will also ensure + * that redundant empty graveyard holder elements are not created. + * + * @protected + * @type {Boolean} + */ + this._needsHolderElement = true; } /** @@ -59,39 +78,6 @@ export default class RemoveOperation extends MoveOperation { this.targetPosition.path[ 0 ] = offset; } - /** - * Flag informing whether this operation should insert "holder" element (`true`) or should move removed nodes - * into existing "holder" element (`false`). - * - * It is `true` for each `RemoveOperation` that is the first `RemoveOperation` in it's delta that points to given holder element. - * This way only one `RemoveOperation` in given delta will insert "holder" element. - * - * @protected - * @type {Boolean} - */ - get _needsHolderElement() { - if ( this.delta ) { - // Let's look up all operations from this delta in the same order as they are in the delta. - for ( let operation of this.delta.operations ) { - // We are interested only in `RemoveOperation`s. - if ( operation instanceof RemoveOperation ) { - // If the first `RemoveOperation` in the delta is this operation, this operation - // needs to insert holder element in the graveyard. - if ( operation == this ) { - return true; - } else if ( operation._holderElementOffset == this._holderElementOffset ) { - // If there is a `RemoveOperation` in this delta that "points" to the same holder element offset, - // that operation will already insert holder element at that offset. We should not create another holder. - return false; - } - } - } - } - - // By default `RemoveOperation` needs holder element, so set it so, if the operation does not have delta. - return true; - } - /** * @inheritDoc * @returns {module:engine/model/operation/reinsertoperation~ReinsertOperation} @@ -152,7 +138,9 @@ export default class RemoveOperation extends MoveOperation { let sourcePosition = Position.fromJSON( json.sourcePosition, document ); const removeOp = new RemoveOperation( sourcePosition, json.howMany, json.baseVersion ); + removeOp.targetPosition = Position.fromJSON( json.targetPosition, document ); + removeOp._needsHolderElement = json._needsHolderElement; return removeOp; } diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 42bb99e6b..82acb4a36 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -355,7 +355,11 @@ const ot = { ); result.isSticky = a.isSticky; - result._holderElementOffset = a._holderElementOffset; + + if ( a instanceof RemoveOperation ) { + result._needsHolderElement = a._needsHolderElement; + result._holderElementOffset = a._holderElementOffset; + } return [ result ]; }, @@ -462,9 +466,10 @@ const ot = { } } - // At this point we transformed this operation's source ranges it means that nothing should be changed. - // But since we need to return an instance of Operation we return an array with NoOperation. if ( ranges.length === 0 ) { + // At this point we transformed this operation's source ranges it means that nothing should be changed. + // But since we need to return an instance of Operation we return an array with NoOperation. + if ( a instanceof RemoveOperation ) { // If `a` operation was RemoveOperation, we cannot convert it to NoOperation. // This is because RemoveOperation creates a holder in graveyard. @@ -492,7 +497,7 @@ const ot = { ); // Map transformed range(s) to operations and return them. - return ranges.reverse().map( ( range ) => { + return ranges.reverse().map( ( range, i ) => { // We want to keep correct operation class. let result = new a.constructor( range.start, @@ -502,7 +507,13 @@ const ot = { ); result.isSticky = a.isSticky; - result._holderElementOffset = a._holderElementOffset; + + if ( a instanceof RemoveOperation ) { + // Transformed `RemoveOperation` needs graveyard holder only when the original operation needed it. + // If `RemoveOperation` got split into two or more operations, only first operation needs graveyard holder. + result._needsHolderElement = a._needsHolderElement && i === 0; + result._holderElementOffset = a._holderElementOffset; + } return result; } ); diff --git a/tests/model/operation/reinsertoperation.js b/tests/model/operation/reinsertoperation.js index f8a90ef1b..2e755955e 100644 --- a/tests/model/operation/reinsertoperation.js +++ b/tests/model/operation/reinsertoperation.js @@ -74,6 +74,9 @@ describe( 'ReinsertOperation', () => { // Reversed `ReinsertOperation` should target back to the same graveyard holder. expect( reverse.targetPosition.isEqual( graveyardPosition ) ).to.be.true; + + // Reversed `ReinsertOperation` should not create new graveyard holder. + expect( reverse._needsHolderElement ).to.be.false; } ); it( 'should undo reinsert set of nodes by applying reverse operation', () => { diff --git a/tests/model/operation/removeoperation.js b/tests/model/operation/removeoperation.js index c077ca71f..93e28f8e8 100644 --- a/tests/model/operation/removeoperation.js +++ b/tests/model/operation/removeoperation.js @@ -10,7 +10,6 @@ import MoveOperation from '../../../src/model/operation/moveoperation'; import Position from '../../../src/model/position'; import Text from '../../../src/model/text'; import Element from '../../../src/model/element'; -import Delta from '../../../src/model/delta/delta'; import { jsonParseStringify, wrapInDelta } from '../../../tests/model/_utils/utils'; describe( 'RemoveOperation', () => { @@ -52,7 +51,7 @@ describe( 'RemoveOperation', () => { expect( operation ).to.be.instanceof( MoveOperation ); } ); - it( 'should remove set of nodes and append them to holder element in graveyard root', () => { + it( 'should be able to remove set of nodes and append them to holder element in graveyard root', () => { root.insertChildren( 0, new Text( 'fozbar' ) ); doc.applyOperation( wrapInDelta( @@ -71,64 +70,24 @@ describe( 'RemoveOperation', () => { expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'zb' ); } ); - it( 'should create new holder element for remove operations in different deltas', () => { + it( 'should be able to remove set of nodes and append them to existing element in graveyard root', () => { root.insertChildren( 0, new Text( 'fozbar' ) ); + graveyard.appendChildren( new Element( '$graveyardHolder' ) ); - doc.applyOperation( wrapInDelta( - new RemoveOperation( - new Position( root, [ 0 ] ), - 1, - doc.version - ) - ) ); - - doc.applyOperation( wrapInDelta( - new RemoveOperation( - new Position( root, [ 0 ] ), - 1, - doc.version - ) - ) ); - - doc.applyOperation( wrapInDelta( - new RemoveOperation( - new Position( root, [ 0 ] ), - 1, - doc.version - ) - ) ); - - expect( graveyard.maxOffset ).to.equal( 3 ); - expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f' ); - expect( graveyard.getChild( 1 ).getChild( 0 ).data ).to.equal( 'o' ); - expect( graveyard.getChild( 2 ).getChild( 0 ).data ).to.equal( 'z' ); - } ); - - it( 'should not create new holder element for remove operation if it was already created for given delta', () => { - root.insertChildren( 0, new Text( 'fozbar' ) ); - - let delta = new Delta(); - - // This simulates i.e. RemoveOperation that got split into two operations during OT. - let removeOpA = new RemoveOperation( - new Position( root, [ 1 ] ), - 1, - doc.version - ); - let removeOpB = new RemoveOperation( + const op = new RemoveOperation( new Position( root, [ 0 ] ), 1, - doc.version + 1 + doc.version ); - delta.addOperation( removeOpA ); - delta.addOperation( removeOpB ); + // Manually set holder element properties. + op._needsHolderElement = false; + op._holderElementOffset = 0; - doc.applyOperation( removeOpA ); - doc.applyOperation( removeOpB ); + doc.applyOperation( wrapInDelta( op ) ); - expect( graveyard.childCount ).to.equal( 1 ); - expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'fo' ); + expect( graveyard.maxOffset ).to.equal( 1 ); + expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f' ); } ); it( 'should create RemoveOperation with same parameters when cloned', () => { @@ -197,6 +156,8 @@ describe( 'RemoveOperation', () => { doc.version ); + op._needsHolderElement = false; + const serialized = jsonParseStringify( op ); expect( serialized ).to.deep.equal( { @@ -205,7 +166,8 @@ describe( 'RemoveOperation', () => { howMany: 2, isSticky: false, sourcePosition: jsonParseStringify( op.sourcePosition ), - targetPosition: jsonParseStringify( op.targetPosition ) + targetPosition: jsonParseStringify( op.targetPosition ), + _needsHolderElement: false } ); } ); } ); @@ -218,6 +180,8 @@ describe( 'RemoveOperation', () => { doc.version ); + op._needsHolderElement = false; + doc.graveyard.appendChildren( [ new Element( '$graveyardHolder' ), new Element( '$graveyardHolder' ) ] ); const serialized = jsonParseStringify( op ); diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index 63cb5952b..6cb6383e5 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -2757,6 +2757,47 @@ describe( 'transform', () => { } ); describe( 'RemoveOperation', () => { + describe( 'by InsertOperation', () => { + it( 'should not need new graveyard holder if original operation did not needed it either', () => { + let op = new RemoveOperation( new Position( root, [ 1 ] ), 1, baseVersion ); + op._needsHolderElement = false; + + let transformBy = new InsertOperation( new Position( root, [ 0 ] ), [ new Node() ], baseVersion ); + + let transOp = transform( op, transformBy )[ 0 ]; + + expect( transOp._needsHolderElement ).to.be.false; + } ); + } ); + + describe( 'by MoveOperation', () => { + it( 'should create not more than RemoveOperation that needs new graveyard holder', () => { + let op = new RemoveOperation( new Position( root, [ 1 ] ), 4, baseVersion ); + let transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( root, [ 8 ] ), baseVersion ); + + let transOp = transform( op, transformBy ); + + expect( transOp.length ).to.equal( 2 ); + + expect( transOp[ 0 ]._needsHolderElement ).to.be.true; + expect( transOp[ 1 ]._needsHolderElement ).to.be.false; + } ); + + it( 'should not need new graveyard holder if original operation did not needed it either', () => { + let op = new RemoveOperation( new Position( root, [ 1 ] ), 4, baseVersion ); + op._needsHolderElement = false; + + let transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( root, [ 8 ] ), baseVersion ); + + let transOp = transform( op, transformBy ); + + expect( transOp.length ).to.equal( 2 ); + + expect( transOp[ 0 ]._needsHolderElement ).to.be.false; + expect( transOp[ 1 ]._needsHolderElement ).to.be.false; + } ); + } ); + describe( 'by RemoveOperation', () => { it( 'removes same nodes and transformed is weak: change howMany to 0', () => { let position = new Position( root, [ 2, 1 ] );