Merge pull request #1100 from ckeditor/t/1099

Other: From now, every operation execution will fire `model.Document#event:change`, even if the operation "does not do" anything (for example, if operation changes attribute to the same value). Closes #1099.

Author: Piotr Jasiun

src/conversion/modelconversiondispatcher.js

@@ -293,6 +293,11 @@ export default class ModelConversionDispatcher {
 	 * @param {*} newValue New attribute value or `null` if attribute has been removed.
 	 */
 	convertAttribute( type, range, key, oldValue, newValue ) {
+		if ( oldValue == newValue ) {
+			// Do not convert if the attribute did not change.
+			return;
+		}
+
 		// Create a list with attributes to consume.
 		const consumable = this._createConsumableForRange( range, type + ':' + key );
 
@@ -323,6 +328,11 @@ export default class ModelConversionDispatcher {
 	 * @param {String} oldName Name of the renamed element before it was renamed.
 	 */
 	convertRename( element, oldName ) {
+		if ( element.name == oldName ) {
+			// Do not convert if the name did not change.
+			return;
+		}
+
 		// Create fake element that will be used to fire remove event. The fake element will have the old element name.
 		const fakeElement = element.clone( true );
 		fakeElement.name = oldName;

src/model/document.js

@@ -168,10 +168,7 @@ export default class Document {
 
 		this.history.addDelta( operation.delta );
 
-		if ( changes ) {
-			// `NoOperation` returns no changes, do not fire event for it.
-			this.fire( 'change', operation.type, changes, operation.delta.batch, operation.delta.type );
-		}
+		this.fire( 'change', operation.type, changes, operation.delta.batch, operation.delta.type );
 	}
 
 	/**

src/model/operation/attributeoperation.js

@@ -141,18 +141,13 @@ export default class AttributeOperation extends Operation {
 					{ node: item, key: this.key }
 				);
 			}
-
-			// If value to set is same as old value, don't do anything.
-			// By returning `undefined`, this operation will be seen as `NoOperation` - that means
-			// that it won't generate any events, etc. `AttributeOperation` with such parameters may be
-			// a result of operational transformation.
-			if ( isEqual( this.oldValue, this.newValue ) ) {
-				return;
-			}
 		}
 
-		// Execution.
-		writer.setAttribute( this.range, this.key, this.newValue );
+		// If value to set is same as old value, don't do anything.
+		if ( !isEqual( this.oldValue, this.newValue ) ) {
+			// Execution.
+			writer.setAttribute( this.range, this.key, this.newValue );
+		}
 
 		return { range: this.range, key: this.key, oldValue: this.oldValue, newValue: this.newValue };
 	}

src/model/operation/nooperation.js

@@ -20,6 +20,10 @@ import Operation from './operation';
  * @extends module:engine/model/operation/operation~Operation
  */
 export default class NoOperation extends Operation {
+	get type() {
+		return 'noop';
+	}
+
 	/**
 	 * Creates and returns an operation that has the same parameters as this operation.
 	 *
@@ -42,7 +46,7 @@ export default class NoOperation extends Operation {
 	 * @inheritDoc
 	 */
 	_execute() {
-		// Do nothing.
+		return {};
 	}
 
 	/**

src/model/operation/renameoperation.js

@@ -106,15 +106,11 @@ export default class RenameOperation extends Operation {
 		}
 
 		// If value to set is same as old value, don't do anything.
-		// By not returning `undefined`, this operation will be seen as `NoOperation` - that means
-		// that it won't generate any events, etc. `RenameOperation` with such parameters may be
-		// a result of Operational Transformation.
-		if ( this.oldName == this.newName ) {
-			return;
+		if ( element.name != this.newName ) {
+			// Execution.
+			element.name = this.newName;
 		}
 
-		element.name = this.newName;
-
 		return { element, oldName: this.oldName };
 	}
 

tests/conversion/modelconversiondispatcher.js

@@ -10,6 +10,9 @@ import ModelElement from '../../src/model/element';
 import ModelRange from '../../src/model/range';
 import ModelPosition from '../../src/model/position';
 import RemoveOperation from '../../src/model/operation/removeoperation';
+import NoOperation from '../../src/model/operation/nooperation';
+import RenameOperation from '../../src/model/operation/renameoperation';
+import AttributeOperation from '../../src/model/operation/attributeoperation';
 import { wrapInDelta } from '../../tests/model/_utils/utils';
 
 describe( 'ModelConversionDispatcher', () => {
@@ -205,6 +208,37 @@ describe( 'ModelConversionDispatcher', () => {
 
 			expect( dispatcher.fire.called ).to.be.false;
 		} );
+
+		it( 'should not fire any event after NoOperation is applied', () => {
+			sinon.spy( dispatcher, 'fire' );
+
+			doc.applyOperation( wrapInDelta( new NoOperation( 0 ) ) );
+
+			expect( dispatcher.fire.called ).to.be.false;
+		} );
+
+		it( 'should not fire any event after RenameOperation with same old and new value is applied', () => {
+			sinon.spy( dispatcher, 'fire' );
+
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( [ new ModelElement( 'paragraph' ) ] );
+
+			doc.applyOperation( wrapInDelta( new RenameOperation( new ModelPosition( root, [ 0 ] ), 'paragraph', 'paragraph', 0 ) ) );
+
+			expect( dispatcher.fire.called ).to.be.false;
+		} );
+
+		it( 'should not fire any event after AttributeOperation with same old an new value is applied', () => {
+			sinon.spy( dispatcher, 'fire' );
+
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( [ new ModelElement( 'paragraph', { foo: 'bar' } ) ] );
+
+			const range = new ModelRange( new ModelPosition( root, [ 0 ] ), new ModelPosition( root, [ 0, 0 ] ) );
+			doc.applyOperation( wrapInDelta( new AttributeOperation( range, 'foo', 'bar', 'bar', 0 ) ) );
+
+			expect( dispatcher.fire.called ).to.be.false;
+		} );
 	} );
 
 	describe( 'convertInsert', () => {

tests/model/operation/attributeoperation.js

@@ -369,22 +369,6 @@ describe( 'AttributeOperation', () => {
 		expect( root.getChild( 1 ).data ).to.equal( 'bcxyz' );
 	} );
 
-	it( 'should return undefined upon execution if new value is same as old value', () => {
-		root.insertChildren( 0, new Text( 'bar', { foo: true } ) );
-
-		const operation = new AttributeOperation(
-			new Range( new Position( root, [ 0 ] ), new Position( root, [ 3 ] ) ),
-			'foo',
-			true,
-			true,
-			doc.version
-		);
-
-		const result = operation._execute();
-
-		expect( result ).to.be.undefined;
-	} );
-
 	describe( 'toJSON', () => {
 		it( 'should create proper serialized object', () => {
 			const range = new Range( new Position( root, [ 0 ] ), new Position( root, [ 2 ] ) );

tests/model/operation/nooperation.js

@@ -19,14 +19,18 @@ describe( 'NoOperation', () => {
 		expect( () => doc.applyOperation( wrapInDelta( noop ) ) ).to.not.throw( Error );
 	} );
 
+	it( 'should return empty object when executed', () => {
+		expect( noop._execute() ).to.deep.equal( {} );
+	} );
+
 	it( 'should create a NoOperation as a reverse', () => {
 		const reverse = noop.getReversed();
 
 		expect( reverse ).to.be.an.instanceof( NoOperation );
 		expect( reverse.baseVersion ).to.equal( 1 );
 	} );
 
-	it( 'should create a do-nothing operation having same parameters when cloned', () => {
+	it( 'should create NoOperation having same parameters when cloned', () => {
 		const clone = noop.clone();
 
 		expect( clone ).to.be.an.instanceof( NoOperation );

tests/model/operation/renameoperation.js

@@ -92,13 +92,6 @@ describe( 'RenameOperation', () => {
 		expect( clone.newName ).to.equal( newName );
 	} );
 
-	it( 'should return undefined on execution if old name and new name is same', () => {
-		const op = new RenameOperation( Position.createAt( root, 0 ), oldName, oldName, doc.version );
-		const result = op._execute();
-
-		expect( result ).to.be.undefined;
-	} );
-
 	describe( 'toJSON', () => {
 		it( 'should create proper serialized object', () => {
 			const op = new RenameOperation( Position.createAt( root, 'end' ), oldName, newName, doc.version );