소스 검색

ChangeOperation._execute() re-add removed error throws.

Szymon Cofalik 10 년 전
부모
커밋
7f2f73cde7

+ 31 - 1
packages/ckeditor5-utils/src/document/operation/changeoperation.js

@@ -82,13 +82,43 @@ CKEDITOR.define( [
 			// Remove or change.
 			if ( oldAttr !== null && newAttr === null ) {
 				for ( value of this.range ) {
-					value.node.removeAttr( oldAttr.key );
+					if ( !value.node.hasAttr( oldAttr ) ) {
+						/**
+						 * The attribute which should be removed does not exists for the given node.
+						 *
+						 * @error operation-change-no-attr-to-remove
+						 * @param {document.Node} node
+						 * @param {document.Attribute} attr
+						 */
+						throw new CKEditorError(
+							'operation-change-no-attr-to-remove: The attribute which should be removed does not exists for given node.',
+							{ node: value.node, attr: oldAttr } );
+					}
+
+					// There is no use in removing attribute if we will overwrite it later.
+					// Still it is profitable to run through the loop to check if all nodes in the range has old attribute.
+					if ( newAttr === null ) {
+						value.node.removeAttr( oldAttr.key );
+					}
 				}
 			}
 
 			// Insert or change.
 			if ( newAttr !== null ) {
 				for ( value of this.range ) {
+					if ( oldAttr === null && value.node.hasAttr( newAttr.key ) ) {
+						/**
+						 * The attribute with given key already exists for the given node.
+						 *
+						 * @error operation-change-attr-exists
+						 * @param {document.Node} node
+						 * @param {document.Attribute} attr
+						 */
+						throw new CKEditorError(
+							'operation-change-attr-exists: The attribute with given key already exists.',
+							{ node: value.node, attr: newAttr } );
+					}
+
 					value.node.setAttr( newAttr );
 				}
 			}

+ 29 - 22
packages/ckeditor5-utils/src/document/transformoperation.js

@@ -153,30 +153,37 @@ CKEDITOR.define( [
 			// Transforms ChangeOperation `a` by ChangeOperation `b`. Accepts a flag stating whether `a` is more important
 			// than `b` when it comes to resolving conflicts. Returns results as an array of operations.
 			ChangeOperation( a, b, isStrong ) {
-				if ( !isStrong && haveConflictingAttributes( a, b ) ) {
-					// If operations' attributes are in conflict and this operation is less important
-					// we have to check if operations' ranges intersect and manage them properly.
-
-					// We get the range(s) which are only affected by this operation.
-					const ranges = a.range.getDifference( b.range );
-
-					if ( ranges.length === 0 ) {
-						// If there are no such ranges, this operation should not do anything (as it is less important).
-						return [ new NoOperation( a.baseVersion ) ];
-					} else {
-						// If there are such ranges, map them to operations and then return.
-						return ranges.map( ( range ) => {
-							return new ChangeOperation(
-								range,
-								a.oldAttr,
-								a.newAttr,
-								a.baseVersion
-							);
-						} );
+				if ( haveConflictingAttributes( a, b ) ) {
+					// If operations attributes are in conflict, check if their ranges intersect and manage them properly.
+					let operations = [];
+
+					// First, we want to apply change to the part of a range that has not been changed by the other operation.
+					operations = operations.concat(
+						a.range.getDifference( b.range ).map( ( range ) => {
+							return new ChangeOperation( range, a.oldAttr, a.newAttr, a.baseVersion );
+						} )
+					);
+
+					if ( isStrong ) {
+						// If this operation is more important, we want 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 have to update oldAttr.
+						const common = a.range.getIntersection( b.range );
+
+						if ( common !== null ) {
+							operations.push( new ChangeOperation( common, b.oldAttr, a.newAttr, a.baseVersion ) );
+						}
 					}
+
+					// 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 or this operation is more important
-					// simply, return an array containing just a clone of this operation.
+					// If operations don't conflict simply, return an array containing just a clone of this operation.
 					return [ a.clone() ];
 				}
 			},

+ 45 - 12
packages/ckeditor5-utils/tests/document/operation/changeoperation.js

@@ -257,24 +257,57 @@ describe( 'ChangeOperation', () => {
 		expect( root.getChild( 2 ).hasAttr( fooAttr ) ).to.be.true;
 	} );
 
+	it( 'should throw an error when one try to remove and the attribute does not exists', () => {
+		let fooAttr = new Attribute( 'foo', true );
+
+		root.insertChildren( 0, 'x' );
+
+		expect( () => {
+			doc.applyOperation(
+				new ChangeOperation(
+					new Range( new Position( [ 0 ], root ), new Position( [ 1 ], root ) ),
+					fooAttr,
+					null,
+					doc.version
+				)
+			);
+		} ).to.throw( CKEditorError, /operation-change-no-attr-to-remove/ );
+	} );
+
+	it( 'should throw an error when one try to insert and the attribute already exists', () => {
+		let x1Attr = new Attribute( 'x', 1 );
+		let x2Attr = new Attribute( 'x', 2 );
+
+		root.insertChildren( 0, new Character( 'x', [ x1Attr ] ) );
+
+		expect( () => {
+			doc.applyOperation(
+				new ChangeOperation(
+					new Range( new Position( [ 0 ], root ), new Position( [ 1 ], root ) ),
+					null,
+					x2Attr,
+					doc.version
+				)
+			);
+		} ).to.throw( CKEditorError, /operation-change-attr-exists/ );
+	} );
+
 	it( 'should throw an error when one try to change and the new and old attributes have different keys', () => {
 		let fooAttr = new Attribute( 'foo', true );
 		let barAttr = new Attribute( 'bar', true );
 
 		root.insertChildren( 0, 'x' );
 
-		expect(
-			() => {
-				doc.applyOperation(
-					new ChangeOperation(
-						new Range( new Position( [ 0 ], root ), new Position( [ 1 ], root ) ),
-						fooAttr,
-						barAttr,
-						doc.version
-					)
-				);
-			}
-		).to.throw( CKEditorError, /operation-change-different-keys/ );
+		expect( () => {
+			doc.applyOperation(
+				new ChangeOperation(
+					new Range( new Position( [ 0 ], root ), new Position( [ 1 ], root ) ),
+					fooAttr,
+					barAttr,
+					doc.version
+				)
+			);
+		} ).to.throw( CKEditorError, /operation-change-different-keys/ );
 	} );
 
 	it( 'should create operation with the same parameters when cloned', () => {

파일 크기가 너무 크기때문에 변경 상태를 표시하지 않습니다.
+ 264 - 76
packages/ckeditor5-utils/tests/document/transformoperation.js


이 변경점에서 너무 많은 파일들이 변경되어 몇몇 파일들은 표시되지 않았습니다.