Bladeren bron

Removed: `RemoveOperation#isPermanent` was removed as it is no longer needed for correct transformation.

Szymon Cofalik 8 jaren geleden
bovenliggende
commit
f056e06991

+ 4 - 7
packages/ckeditor5-engine/src/model/delta/removedelta.js

@@ -28,7 +28,7 @@ export default class RemoveDelta extends MoveDelta {
 	}
 }
 
-function addRemoveDelta( batch, position, howMany, isPermanent ) {
+function addRemoveDelta( batch, position, howMany ) {
 	const delta = new RemoveDelta();
 	batch.addDelta( delta );
 
@@ -36,7 +36,6 @@ function addRemoveDelta( batch, position, howMany, isPermanent ) {
 	const gyPosition = new Position( graveyard, [ 0 ] );
 
 	const operation = new RemoveOperation( position, howMany, gyPosition, batch.document.version );
-	operation.isPermanent = isPermanent;
 	delta.addOperation( operation );
 	batch.document.applyOperation( operation );
 }
@@ -47,19 +46,17 @@ function addRemoveDelta( batch, position, howMany, isPermanent ) {
  * @chainable
  * @method module:engine/model/batch~Batch#remove
  * @param {module:engine/model/item~Item|module:engine/model/range~Range} itemOrRange Model item or range to remove.
- * @param {Boolean} isPermanent If set to `true`, creates
- * {@link module:engine/model/operation/removeoperation~RemoveOperation#isPermanent a permanent RemoveOperation}.
  */
-register( 'remove', function( itemOrRange, isPermanent = false ) {
+register( 'remove', function( itemOrRange ) {
 	if ( itemOrRange instanceof Range ) {
 		// The array is reversed, so the ranges to remove are in correct order and do not have to be updated.
 		const ranges = itemOrRange.getMinimalFlatRanges().reverse();
 
 		for ( const flat of ranges ) {
-			addRemoveDelta( this, flat.start, flat.end.offset - flat.start.offset, isPermanent );
+			addRemoveDelta( this, flat.start, flat.end.offset - flat.start.offset );
 		}
 	} else {
-		addRemoveDelta( this, Position.createBefore( itemOrRange ), 1, isPermanent );
+		addRemoveDelta( this, Position.createBefore( itemOrRange ), 1 );
 	}
 
 	return this;

+ 3 - 34
packages/ckeditor5-engine/src/model/operation/removeoperation.js

@@ -9,35 +9,12 @@
 
 import MoveOperation from './moveoperation';
 import ReinsertOperation from './reinsertoperation';
-import NoOperation from './nooperation';
 
 /**
  * Operation to remove a range of nodes.
  */
 export default class RemoveOperation extends MoveOperation {
 	/**
-	 * Creates a remove operation.
-	 *
-	 * @param {module:engine/model/position~Position} sourcePosition Position before the first
-	 * {@link module:engine/model/item~Item model item} to move.
-	 * @param {Number} howMany Offset size of moved range. Moved range will start from `sourcePosition` and end at
-	 * `sourcePosition` with offset shifted by `howMany`.
-	 * @param {module:engine/model/position~Position} targetPosition Position at which moved nodes will be inserted.
-	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which operation can be applied.
-	 */
-	constructor( sourcePosition, howMany, targetPosition, baseVersion ) {
-		super( sourcePosition, howMany, targetPosition, baseVersion );
-
-		/**
-		 * If `RemoveOperation` is permanent (`true`), nodes removed by it cannot be reinserted back to the model. This
-		 * setting affects operational transformation and {@link #getReversed reversing}.
-		 *
-		 * @member {Boolean} module:engine/model/operation/removeoperation~RemoveOperation#isPermanent
-		 */
-		this.isPermanent = false;
-	}
-
-	/**
 	 * @inheritDoc
 	 */
 	get type() {
@@ -50,13 +27,9 @@ export default class RemoveOperation extends MoveOperation {
 	 * @returns {module:engine/model/operation/reinsertoperation~ReinsertOperation|module:engine/model/operation/nooperation~NoOperation}
 	 */
 	getReversed() {
-		if ( this.isPermanent ) {
-			return new NoOperation( this.baseVersion + 1 );
-		} else {
-			const newTargetPosition = this.sourcePosition._getTransformedByInsertion( this.targetPosition, this.howMany );
+		const newTargetPosition = this.sourcePosition._getTransformedByInsertion( this.targetPosition, this.howMany );
 
-			return new ReinsertOperation( this.getMovedRangeStart(), this.howMany, newTargetPosition, this.baseVersion + 1 );
-		}
+		return new ReinsertOperation( this.getMovedRangeStart(), this.howMany, newTargetPosition, this.baseVersion + 1 );
 	}
 
 	/**
@@ -67,10 +40,6 @@ export default class RemoveOperation extends MoveOperation {
 	}
 
 	static fromJSON( json, document ) {
-		const op = super.fromJSON( json, document );
-
-		op.isPermanent = json.isPermanent;
-
-		return op;
+		return super.fromJSON( json, document );
 	}
 }

+ 3 - 6
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -476,13 +476,10 @@ const ot = {
 			// Modifier for default case.
 			// Modifies `context.isStrong` in certain conditions.
 			//
-			// If `b` is a permanent `RemoveOperation`, it is always more important than transformed operation.
-			if ( b instanceof RemoveOperation && b.isPermanent ) {
-				context.isStrong = false;
-			}
 			// If only one of operations is a remove operation, we force remove operation to be the "stronger" one
-			// to provide more expected results.
-			else if ( !context.forceWeakRemove ) {
+			// to provide more expected results. This is done only if `context.forceWeakRemove` is set to `false`.
+			// `context.forceWeakRemove` is set to `true` in certain conditions when transformation takes place during undo.
+			if ( !context.forceWeakRemove ) {
 				if ( a instanceof RemoveOperation && !( b instanceof RemoveOperation ) ) {
 					context.isStrong = true;
 				} else if ( !( a instanceof RemoveOperation ) && b instanceof RemoveOperation ) {

+ 0 - 8
packages/ckeditor5-engine/tests/model/delta/removedelta.js

@@ -73,14 +73,6 @@ describe( 'Batch', () => {
 
 			expect( doc.applyOperation.calledWith( correctDeltaMatcher ) ).to.be.true;
 		} );
-
-		it( 'should create permanent RemoveOperation', () => {
-			batch.remove( div, true );
-
-			const operation = batch.deltas[ 0 ].operations[ 0 ];
-
-			expect( operation.isPermanent ).to.be.true;
-		} );
 	} );
 } );
 

+ 0 - 12
packages/ckeditor5-engine/tests/model/operation/removeoperation.js

@@ -5,7 +5,6 @@
 
 import Document from '../../../src/model/document';
 import ReinsertOperation from '../../../src/model/operation/reinsertoperation';
-import NoOperation from '../../../src/model/operation/nooperation';
 import RemoveOperation from '../../../src/model/operation/removeoperation';
 import MoveOperation from '../../../src/model/operation/moveoperation';
 import Position from '../../../src/model/position';
@@ -108,16 +107,6 @@ describe( 'RemoveOperation', () => {
 		expect( reverse.targetPosition.path ).to.deep.equal( [ 3 ] );
 	} );
 
-	it( 'should create NoOperation when reversed if was permanent', () => {
-		const position = new Position( root, [ 0 ] );
-		const operation = new RemoveOperation( position, 2, new Position( doc.graveyard, [ 0 ] ), 0 );
-		operation.isPermanent = true;
-		const reverse = operation.getReversed();
-
-		expect( reverse ).to.be.an.instanceof( NoOperation );
-		expect( reverse.baseVersion ).to.equal( 1 );
-	} );
-
 	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 );
@@ -167,7 +156,6 @@ describe( 'RemoveOperation', () => {
 				baseVersion: 0,
 				howMany: 2,
 				isSticky: false,
-				isPermanent: false,
 				sourcePosition: jsonParseStringify( op.sourcePosition ),
 				targetPosition: jsonParseStringify( op.targetPosition )
 			} );

+ 0 - 32
packages/ckeditor5-engine/tests/model/operation/transform.js

@@ -2837,19 +2837,6 @@ describe( 'transform', () => {
 
 				expectOperation( transOp[ 0 ], expected );
 			} );
-
-			it( 'should skip context.isStrong and be less important if RemoveOperation is permanent', () => {
-				transformBy.isPermanent = true;
-
-				const transOp = transform( op, transformBy, { isStrong: true, forceWeakRemove: true } );
-
-				expect( transOp.length ).to.equal( 1 );
-
-				expectOperation( transOp[ 0 ], {
-					type: NoOperation,
-					baseVersion: baseVersion + 1
-				} );
-			} );
 		} );
 
 		describe( 'by NoOperation', () => {
@@ -2911,25 +2898,6 @@ describe( 'transform', () => {
 				} );
 			} );
 		} );
-
-		describe( 'by RemoveOperation', () => {
-			it( 'should use isPermanent flag even if both operations are remove operations', () => {
-				const op = new RemoveOperation( new Position( root, [ 8 ] ), 2, new Position( doc.graveyard, [ 0 ] ), baseVersion );
-
-				const transformBy = op.clone();
-				transformBy.isPermanent = true;
-
-				// `context.isStrong` will not be considered, because `transformBy` is permanent.
-				const transOp = transform( op, transformBy, { isStrong: true } );
-
-				expect( transOp.length ).to.equal( 1 );
-
-				expectOperation( transOp[ 0 ], {
-					type: NoOperation,
-					baseVersion: baseVersion + 1
-				} );
-			} );
-		} );
 	} );
 
 	describe( 'NoOperation', () => {