diff --git a/src/conversion/modelconversiondispatcher.js b/src/conversion/modelconversiondispatcher.js index d0feb483f..2a42e2b4e 100644 --- a/src/conversion/modelconversiondispatcher.js +++ b/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; diff --git a/src/model/document.js b/src/model/document.js index a19c63f3e..1e903533c 100644 --- a/src/model/document.js +++ b/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 ); } /** diff --git a/src/model/operation/attributeoperation.js b/src/model/operation/attributeoperation.js index 504911d42..f08f46b94 100644 --- a/src/model/operation/attributeoperation.js +++ b/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 }; } diff --git a/src/model/operation/nooperation.js b/src/model/operation/nooperation.js index a2fb99d15..184f0b3e6 100644 --- a/src/model/operation/nooperation.js +++ b/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 {}; } /** diff --git a/src/model/operation/renameoperation.js b/src/model/operation/renameoperation.js index 4c7078926..502cd6828 100644 --- a/src/model/operation/renameoperation.js +++ b/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 }; } diff --git a/tests/conversion/modelconversiondispatcher.js b/tests/conversion/modelconversiondispatcher.js index 9cd202bd6..335d216cf 100644 --- a/tests/conversion/modelconversiondispatcher.js +++ b/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', () => { diff --git a/tests/model/operation/attributeoperation.js b/tests/model/operation/attributeoperation.js index b9f586cfb..e02569c2c 100644 --- a/tests/model/operation/attributeoperation.js +++ b/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 ] ) ); diff --git a/tests/model/operation/nooperation.js b/tests/model/operation/nooperation.js index 410fb8e74..05e075640 100644 --- a/tests/model/operation/nooperation.js +++ b/tests/model/operation/nooperation.js @@ -19,6 +19,10 @@ 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(); @@ -26,7 +30,7 @@ describe( '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 ); diff --git a/tests/model/operation/renameoperation.js b/tests/model/operation/renameoperation.js index 73fc38d3c..8f91f8817 100644 --- a/tests/model/operation/renameoperation.js +++ b/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 );