Przeglądaj źródła

Changed: Moved some logic from MarkerOperation to MarkerCollection.

Szymon Cofalik 9 lat temu
rodzic
commit
7bccd79816

+ 12 - 5
packages/ckeditor5-engine/src/model/markercollection.js

@@ -79,8 +79,15 @@ export default class MarkerCollection {
 	 */
 	set( markerOrName, range ) {
 		const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName;
+		const oldMarker = this._markers.get( markerName );
+
+		if ( oldMarker ) {
+			const oldRange = oldMarker.getRange();
+
+			if ( oldRange.isEqual( range ) ) {
+				return oldMarker;
+			}
 
-		if ( this._markers.has( markerName ) ) {
 			this.remove( markerName );
 		}
 
@@ -101,13 +108,13 @@ export default class MarkerCollection {
 	 */
 	remove( markerOrName ) {
 		const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName;
-		const marker = this._markers.get( markerName );
+		const oldMarker = this._markers.get( markerName );
 
-		if ( marker ) {
+		if ( oldMarker ) {
 			this._markers.delete( markerName );
-			this.fire( 'remove', marker );
+			this.fire( 'remove', oldMarker );
 
-			this._destroyMarker( marker );
+			this._destroyMarker( oldMarker );
 
 			return true;
 		}

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

@@ -86,17 +86,7 @@ export default class MarkerOperation extends Operation {
 	_execute() {
 		const type = this.newRange ? 'set' : 'remove';
 
-		if ( type == 'remove' && this._markers.has( this.name ) ) {
-			// Remove marker only if the marker exists.
-			this._markers.remove( this.name );
-		} else if ( type == 'set' ) {
-			const oldMarker = this._markers.get( this.name );
-
-			if ( oldMarker === null || !oldMarker.getRange().isEqual( this.newRange ) ) {
-				// Set marker only if it does not exist or it has different range.
-				this._markers.set( this.name, this.newRange );
-			}
-		}
+		this._markers[ type ]( this.name, this.newRange );
 
 		return { name: this.name, type: type };
 	}

+ 13 - 5
packages/ckeditor5-engine/tests/model/markercollection.js

@@ -52,15 +52,12 @@ describe( 'MarkerCollection', () => {
 		} );
 
 		it( 'should fire remove event, and create a new marker if marker with given name was in the collection', () => {
-			markers.set( 'name', range );
-			const marker1 = markers.get( 'name' );
+			const marker1 = markers.set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const result = markers.set( 'name', range2 );
-			const marker2 = markers.get( 'name' );
+			const marker2 = markers.set( 'name', range2 );
 
-			expect( result ).to.equal( marker2 );
 			expect( markers.fire.calledWithExactly( 'remove', marker1 ) ).to.be.true;
 			expect( markers.fire.calledWithExactly( 'add', marker2 ) ).to.be.true;
 
@@ -70,6 +67,17 @@ describe( 'MarkerCollection', () => {
 			expect( marker1 ).not.to.equal( marker2 );
 		} );
 
+		it( 'should not fire event and return the same marker if given marker has a range equal to given range', () => {
+			const marker1 = markers.set( 'name', range );
+
+			sinon.spy( markers, 'fire' );
+
+			const marker2 = markers.set( 'name', range );
+
+			expect( marker1 ).to.equal( marker2 );
+			expect( markers.fire.notCalled ).to.be.true;
+		} );
+
 		it( 'should accept marker instance instead of name', () => {
 			markers.set( 'name', range );
 			const marker1 = markers.get( 'name' );