Browse Source

Fixed: MarkerOperation execution should always result in firing Document#event:change.

Szymon Cofalik 9 years ago
parent
commit
66f86328cf

+ 9 - 11
packages/ckeditor5-engine/src/model/operation/markeroperation.js

@@ -94,19 +94,17 @@ export default class MarkerOperation extends Operation {
 	 * @inheritDoc
 	 * @inheritDoc
 	 */
 	 */
 	_execute() {
 	_execute() {
-		if ( this.oldRange === null && this.newRange === null ) {
-			return { name: this.name, type: 'remove' };
-		}
+		// Make a change in Document#markers only when something actually changes, that is
+		// `this.oldRange` and `this.newRange` are different.
+		const changeInMarkersCollection =
+			!( this.oldRange === null && this.newRange === null ) &&
+			!( this.oldRange !== null && this.newRange !== null && this.oldRange.isEqual( this.newRange ) );
 
 
-		const document = ( this.oldRange || this.newRange ).root.document;
-		let type;
+		const document = changeInMarkersCollection && ( this.oldRange || this.newRange ).root.document;
+		const type = this.newRange ? 'set' : 'remove';
 
 
-		if ( this.newRange ) {
-			type = 'set';
-			document.markers.set( this.name, this.newRange );
-		} else {
-			type = 'remove';
-			document.markers.remove( this.name );
+		if ( changeInMarkersCollection ) {
+			document.markers[ type ]( this.name, this.newRange );
 		}
 		}
 
 
 		return { name: this.name, type: type };
 		return { name: this.name, type: type };

+ 21 - 1
packages/ckeditor5-engine/tests/model/operation/markeroperation.js

@@ -94,8 +94,9 @@ describe( 'MarkerOperation', () => {
 		expect( doc.fire.called ).to.be.true;
 		expect( doc.fire.called ).to.be.true;
 	} );
 	} );
 
 
-	it( 'should cause a change event when applied if oldRange and newRange are null', () => {
+	it( 'should fire document change event but not document markers remove event if oldRange and newRange are null', () => {
 		sinon.spy( doc, 'fire' );
 		sinon.spy( doc, 'fire' );
+		sinon.spy( doc.markers, 'fire' );
 
 
 		doc.on( 'change', ( evt, type, changes ) => {
 		doc.on( 'change', ( evt, type, changes ) => {
 			expect( type ).to.equal( 'marker' );
 			expect( type ).to.equal( 'marker' );
@@ -108,6 +109,25 @@ describe( 'MarkerOperation', () => {
 		) );
 		) );
 
 
 		expect( doc.fire.calledWith( 'change', 'marker' ) ).to.be.true;
 		expect( doc.fire.calledWith( 'change', 'marker' ) ).to.be.true;
+		expect( doc.markers.fire.notCalled ).to.be.true;
+	} );
+
+	it( 'should fire document change event but not document markers remove event if oldRange and newRange are equal', () => {
+		sinon.spy( doc, 'fire' );
+		sinon.spy( doc.markers, 'fire' );
+
+		doc.on( 'change', ( evt, type, changes ) => {
+			expect( type ).to.equal( 'marker' );
+			expect( changes.name ).to.equal( 'name' );
+			expect( changes.type ).to.equal( 'set' );
+		} );
+
+		doc.applyOperation( wrapInDelta(
+			new MarkerOperation( 'name', range, range, doc.version )
+		) );
+
+		expect( doc.fire.calledWith( 'change', 'marker' ) ).to.be.true;
+		expect( doc.markers.fire.notCalled ).to.be.true;
 	} );
 	} );
 
 
 	it( 'should return MarkerOperation with swapped ranges as reverse operation', () => {
 	it( 'should return MarkerOperation with swapped ranges as reverse operation', () => {