Browse Source

Changed: AttributeOperation x AttributeOperation transformation - "strong" AttributeOperation should be kept as AttributeOperation even if its oldValue and newValue are same.

Szymon Cofalik 9 years ago
parent
commit
89361fa9db

+ 8 - 13
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -11,7 +11,6 @@ import MoveOperation from './moveoperation.js';
 import RemoveOperation from './removeoperation.js';
 import NoOperation from './nooperation.js';
 import Range from '../range.js';
-import isEqual from '../../../utils/lib/lodash/isEqual.js';
 import compareArrays from '../../../utils/comparearrays.js';
 
 /**
@@ -110,24 +109,20 @@ const ot = {
 					return new AttributeOperation( range, a.key, a.oldValue, a.newValue, a.baseVersion );
 				} );
 
-				// Then we take care of the common part of ranges, but only if operations has different `newValue`.
-				if ( isStrong && !isEqual( a.newValue, b.newValue ) ) {
+				// Then we take care of the common part of ranges.
+				const common = a.range.getIntersection( b.range );
+
+				if ( common ) {
 					// If this operation is more important, we also want to apply change to the part of the
 					// original range that has already been changed by the other operation. Since that range
 					// got changed we also have to update `oldValue`.
-					const common = a.range.getIntersection( b.range );
-
-					if ( common !== null ) {
-						operations.push( new AttributeOperation( common, b.key, b.oldValue, a.newValue, a.baseVersion ) );
+					if ( isStrong ) {
+						operations.push( new AttributeOperation( common, b.key, b.newValue, a.newValue, a.baseVersion ) );
+					} else if ( operations.length === 0 ) {
+						operations.push( new NoOperation( 0 ) );
 					}
 				}
 
-				// If no operations has been added nothing should get updated, but since we need to return
-				// an instance of Operation we add NoOperation to the array.
-				if ( operations.length === 0 ) {
-					operations.push( new NoOperation( a.baseVersion ) );
-				}
-
 				return operations;
 			} else {
 				// If operations don't conflict, simply return an array containing just a clone of this operation.

+ 46 - 29
packages/ckeditor5-engine/tests/model/operation/transform.js

@@ -555,7 +555,7 @@ describe( 'transform', () => {
 				it( 'attributes have different key: no operation update', () => {
 					let transformBy = new AttributeOperation(
 						Range.createFromRange( range ),
-						'abc',
+						'differentKey',
 						true,
 						false,
 						baseVersion
@@ -585,34 +585,31 @@ describe( 'transform', () => {
 					} );
 				} );
 
-				it( 'both operations removes same attribute: convert to NoOperation', () => {
-					op.newValue = null;
+				describe( 'that is less important and', () => {
+					it( 'both operations removes same attribute: change old value to null', () => {
+						op.newValue = null;
+						expected.newValue = null;
 
-					let transformBy = new AttributeOperation(
-						new Range( new Position( root, [ 1, 1, 4 ] ), new Position( root, [ 3 ] ) ),
-						'foo',
-						'another',
-						null,
-						baseVersion
-					);
+						let transformBy = new AttributeOperation(
+							new Range( new Position( root, [ 1, 1, 4 ] ), new Position( root, [ 3 ] ) ),
+							'foo',
+							'another',
+							null,
+							baseVersion
+						);
 
-					let transOp = transform( op, transformBy, true );
+						let transOp = transform( op, transformBy, true );
 
-					expected.newValue = null;
+						expected.oldValue = null;
 
-					expect( transOp.length ).to.equal( 1 );
-					expectOperation( transOp[ 0 ], {
-						type: NoOperation,
-						baseVersion: baseVersion + 1
+						expect( transOp.length ).to.equal( 1 );
 					} );
-				} );
 
-				describe( 'that is less important and', () => {
 					it( 'range does not intersect original range: no operation update', () => {
 						let transformBy = new AttributeOperation(
 							new Range( new Position( root, [ 3, 0 ] ), new Position( root, [ 4 ] ) ),
 							'foo',
-							'another',
+							'abc',
 							null,
 							baseVersion
 						);
@@ -627,14 +624,14 @@ describe( 'transform', () => {
 						let transformBy = new AttributeOperation(
 							new Range( new Position( root, [ 1, 1, 4 ] ), new Position( root, [ 3 ] ) ),
 							'foo',
-							'another',
+							'abc',
 							null,
 							baseVersion
 						);
 
 						let transOp = transform( op, transformBy, true );
 
-						expected.oldValue = 'another';
+						expected.oldValue = null;
 
 						expect( transOp.length ).to.equal( 1 );
 						expectOperation( transOp[ 0 ], expected );
@@ -644,13 +641,14 @@ describe( 'transform', () => {
 					it( 'range intersects on left: split into two operations, update oldValue', () => {
 						// Get more test cases and better code coverage
 						op.newValue = null;
+						expected.newValue = null;
 
 						let transformBy = new AttributeOperation(
 							new Range( new Position( root, [ 1, 4, 2 ] ), new Position( root, [ 3 ] ) ),
 							'foo',
-							'another',
+							null,
 							// Get more test cases and better code coverage
-							'anothernew',
+							'another',
 							baseVersion
 						);
 
@@ -658,8 +656,6 @@ describe( 'transform', () => {
 
 						expect( transOp.length ).to.equal( 2 );
 
-						expected.newValue = null;
-
 						expected.range.end.path = [ 1, 4, 2 ];
 
 						expectOperation( transOp[ 0 ], expected );
@@ -677,7 +673,7 @@ describe( 'transform', () => {
 						let transformBy = new AttributeOperation(
 							new Range( new Position( root, [ 1 ] ), new Position( root, [ 2, 1 ] ) ),
 							'foo',
-							'another',
+							'abc',
 							null,
 							baseVersion
 						);
@@ -692,7 +688,7 @@ describe( 'transform', () => {
 
 						expected.range.start = op.range.start;
 						expected.range.end.path = [ 2, 1 ];
-						expected.oldValue = 'another';
+						expected.oldValue = null;
 						expected.baseVersion++;
 
 						expectOperation( transOp[ 1 ], expected );
@@ -702,7 +698,7 @@ describe( 'transform', () => {
 						let transformBy = new AttributeOperation(
 							new Range( new Position( root, [ 1, 4, 1 ] ), new Position( root, [ 2, 1 ] ) ),
 							'foo',
-							'another',
+							'abc',
 							null,
 							baseVersion
 						);
@@ -723,7 +719,7 @@ describe( 'transform', () => {
 
 						expected.range.start.path = [ 1, 4, 1 ];
 						expected.range.end.path = [ 2, 1 ];
-						expected.oldValue = 'another';
+						expected.oldValue = null;
 						expected.baseVersion++;
 
 						expectOperation( transOp[ 2 ], expected );
@@ -731,6 +727,27 @@ describe( 'transform', () => {
 				} );
 
 				describe( 'that is more important and', () => {
+					it( 'both operations removes same attribute: convert to NoOperation', () => {
+						op.newValue = null;
+						expected.newValue = null;
+
+						let transformBy = new AttributeOperation(
+							new Range( new Position( root, [ 1, 1, 4 ] ), new Position( root, [ 3 ] ) ),
+							'foo',
+							'another',
+							null,
+							baseVersion
+						);
+
+						let transOp = transform( op, transformBy );
+
+						expect( transOp.length ).to.equal( 1 );
+						expectOperation( transOp[ 0 ], {
+							type: NoOperation,
+							baseVersion: baseVersion + 1
+						} );
+					} );
+
 					it( 'range does not intersect original range: no operation update', () => {
 						let transformBy = new AttributeOperation(
 							new Range( new Position( root, [ 3, 0 ] ), new Position( root, [ 4 ] ) ),