8
0
Просмотр исходного кода

Changed: model.Selection events fired only when range/attribute changed and with additional parameters.

Szymon Cofalik 9 лет назад
Родитель
Сommit
bc75ca415d

+ 128 - 62
packages/ckeditor5-engine/src/model/selection.js

@@ -9,6 +9,7 @@ import EmitterMixin from '../../utils/emittermixin.js';
 import CKEditorError from '../../utils/ckeditorerror.js';
 import mix from '../../utils/mix.js';
 import toMap from '../../utils/tomap.js';
+import mapsEqual from '../../utils/mapsequal.js';
 
 /**
  * `Selection` is a group of {@link engine.model.Range ranges} which has a direction specified by
@@ -244,7 +245,7 @@ export default class Selection {
 		this._pushRange( range );
 		this._lastRangeBackward = !!isBackward;
 
-		this.fire( 'change:range' );
+		this.fire( 'change:range', { directChange: true } );
 	}
 
 	/**
@@ -253,9 +254,10 @@ export default class Selection {
 	 * @fires engine.model.Selection#change:range
 	 */
 	removeAllRanges() {
-		this._ranges = [];
-
-		this.fire( 'change:range' );
+		if ( this._ranges.length > 0 ) {
+			this._removeAllRanges();
+			this.fire( 'change:range', { directChange: true } );
+		}
 	}
 
 	/**
@@ -269,8 +271,22 @@ export default class Selection {
 	 * @param {Boolean} [isLastBackward=false] Flag describing if last added range was selected forward - from start to end (`false`)
 	 * or backward - from end to start (`true`).
 	 */
-	setRanges( newRanges, isLastBackward ) {
-		this._ranges = [];
+	setRanges( newRanges, isLastBackward = false ) {
+		newRanges = Array.from( newRanges );
+
+		// Check whether there is any range in new ranges set that is different than all already added ranges.
+		const anyNewRange = newRanges.some( ( newRange ) => {
+			return this._ranges.every( ( oldRange ) => {
+				return !oldRange.isEqual( newRange );
+			} );
+		} );
+
+		// Don't do anything if nothing changed.
+		if ( newRanges.length === this._ranges.length && !anyNewRange ) {
+			return;
+		}
+
+		this._removeAllRanges();
 
 		for ( let range of newRanges ) {
 			this._pushRange( range );
@@ -278,7 +294,7 @@ export default class Selection {
 
 		this._lastRangeBackward = !!isLastBackward;
 
-		this.fire( 'change:range' );
+		this.fire( 'change:range', { directChange: true } );
 	}
 
 	/**
@@ -337,6 +353,45 @@ export default class Selection {
 		}
 	}
 
+	/**
+	 * Sets {@link engine.model.Selection#focus} to the specified location.
+	 *
+	 * The location can be specified in the same form as {@link engine.model.Position.createAt} parameters.
+	 *
+	 * @fires engine.model.Selection#change:range
+	 * @param {engine.model.Item|engine.model.Position} itemOrPosition
+	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * first parameter is a {@link engine.model.Item model item}.
+	 */
+	setFocus( itemOrPosition, offset ) {
+		if ( this.anchor === null ) {
+			/**
+			 * Cannot set selection focus if there are no ranges in selection.
+			 *
+			 * @error model-selection-setFocus-no-ranges
+			 */
+			throw new CKEditorError( 'model-selection-setFocus-no-ranges: Cannot set selection focus if there are no ranges in selection.' );
+		}
+
+		const newFocus = Position.createAt( itemOrPosition, offset );
+
+		if ( newFocus.compareWith( this.focus ) == 'same' ) {
+			return;
+		}
+
+		const anchor = this.anchor;
+
+		if ( this._ranges.length ) {
+			this._popRange();
+		}
+
+		if ( newFocus.compareWith( anchor ) == 'before' ) {
+			this.addRange( new Range( newFocus, anchor ), true );
+		} else {
+			this.addRange( new Range( anchor, newFocus ) );
+		}
+	}
+
 	/**
 	 * Gets an attribute value for given key or `undefined` if that attribute is not set on the selection.
 	 *
@@ -381,87 +436,81 @@ export default class Selection {
 	/**
 	 * Removes all attributes from the selection.
 	 *
+	 * If there were any attributes in selection, fires {@link engine.model.Selection#event:change change event} with
+	 * removed attributes' keys.
+	 *
 	 * @fires engine.model.Selection#change:attribute
 	 */
 	clearAttributes() {
-		this._attrs.clear();
+		if ( this._attrs.size > 0 ) {
+			const attributeKeys = Array.from( this._attrs.keys() );
+			this._attrs.clear();
 
-		this.fire( 'change:attribute' );
+			this.fire( 'change:attribute', { attributeKeys, directChange: true } );
+		}
 	}
 
 	/**
 	 * Removes an attribute with given key from the selection.
 	 *
+	 * If given attribute was set on the selection, fires {@link engine.model.Selection#event:change change event} with
+	 * removed attribute key.
+	 *
 	 * @fires engine.model.Selection#change:attribute
 	 * @param {String} key Key of attribute to remove.
 	 */
 	removeAttribute( key ) {
-		this._attrs.delete( key );
+		if ( this.hasAttribute( key ) ) {
+			this._attrs.delete( key );
 
-		this.fire( 'change:attribute' );
+			this.fire( 'change:attribute', { attributeKeys: [ key ], directChange: true } );
+		}
 	}
 
 	/**
 	 * Sets attribute on the selection. If attribute with the same key already is set, it's value is overwritten.
 	 *
+	 * If the attribute value has changed, fires {@link engine.model.Selection#event:change change event} with
+	 * the attribute key.
+	 *
 	 * @fires engine.model.Selection#change:attribute
 	 * @param {String} key Key of attribute to set.
 	 * @param {*} value Attribute value.
 	 */
 	setAttribute( key, value ) {
-		this._attrs.set( key, value );
+		if ( this.getAttribute( key ) !== value ) {
+			this._attrs.set( key, value );
 
-		this.fire( 'change:attribute' );
+			this.fire( 'change:attribute', { attributeKeys: [ key ], directChange: true } );
+		}
 	}
 
 	/**
 	 * Removes all attributes from the selection and sets given attributes.
 	 *
+	 * If given set of attributes is different than set of attributes already added to selection, fires
+	 * {@link engine.model.Selection#event:change change event} with keys of attributes that changed.
+	 *
 	 * @fires engine.model.Selection#change:attribute
 	 * @param {Iterable|Object} attrs Iterable object containing attributes to be set.
 	 */
 	setAttributesTo( attrs ) {
-		this._attrs = toMap( attrs );
+		attrs = toMap( attrs );
 
-		this.fire( 'change:attribute' );
-	}
+		if ( !mapsEqual( attrs, this._attrs ) ) {
+			// Create a set from keys of old and new attributes.
+			const changed = new Set( Array.from( attrs.keys() ).concat( Array.from( this._attrs.keys() ) ) );
 
-	/**
-	 * Sets {@link engine.model.Selection#focus} to the specified location.
-	 *
-	 * The location can be specified in the same form as {@link engine.model.Position.createAt} parameters.
-	 *
-	 * @fires engine.model.Selection#change:range
-	 * @param {engine.model.Item|engine.model.Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
-	 * first parameter is a {@link engine.model.Item model item}.
-	 */
-	setFocus( itemOrPosition, offset ) {
-		if ( this.anchor === null ) {
-			/**
-			 * Cannot set selection focus if there are no ranges in selection.
-			 *
-			 * @error model-selection-setFocus-no-ranges
-			 */
-			throw new CKEditorError( 'model-selection-setFocus-no-ranges: Cannot set selection focus if there are no ranges in selection.' );
-		}
-
-		const newFocus = Position.createAt( itemOrPosition, offset );
-
-		if ( newFocus.compareWith( this.focus ) == 'same' ) {
-			return;
-		}
+			for ( let [ key, value ] of attrs ) {
+				// If the attribute remains unchanged, remove it from changed set.
+				if ( this._attrs.get( key ) === value ) {
+					changed.delete( key );
+				}
+			}
 
-		const anchor = this.anchor;
+			this._attrs = attrs;
 
-		if ( this._ranges.length ) {
-			this._popRange();
-		}
-
-		if ( newFocus.compareWith( anchor ) == 'before' ) {
-			this.addRange( new Range( newFocus, anchor ), true );
-		} else {
-			this.addRange( new Range( anchor, newFocus ) );
+			this.fire( 'change:attribute', { attributeKeys: Array.from( changed ), directChange: true } );
 		}
 	}
 
@@ -527,18 +576,35 @@ export default class Selection {
 	_popRange() {
 		this._ranges.pop();
 	}
-}
 
-mix( Selection, EmitterMixin );
+	/**
+	 * Deletes ranges from internal range array. Uses {@link engine.model.Selection#_popRange _popRange} to
+	 * ensure proper ranges removal.
+	 *
+	 * @private
+	 */
+	_removeAllRanges() {
+		while ( this._ranges.length > 0 ) {
+			this._popRange();
+		}
+	}
 
-/**
- * Fired whenever selection ranges are changed through {@link engine.model.Selection Selection API}.
- *
- * @event engine.model.Selection#change:range
- */
+	/**
+	 * Fired whenever selection ranges are changed.
+	 *
+	 * @event engine.model.Selection#change:range
+	 * @param {Boolean} directChange Specifies whether the range change was caused by direct usage of `Selection` API (`true`)
+	 * or by changes done to {@link engine.model.Document model document} using {@link engine.model.Batch Batch} API (`false`).
+	 */
 
-/**
- * Fired whenever selection attributes are changed.
- *
- * @event engine.model.Selection#change:attribute
- */
+	/**
+	 * Fired whenever selection attributes are changed.
+	 *
+	 * @event engine.model.Selection#change:attribute
+	 * @param {Boolean} directChange Specifies whether the attributes changed by direct usage of `Selection` API (`true`)
+	 * or by changes done to {@link engine.model.Document model document} using {@link engine.model.Batch Batch} API (`false`).
+	 * @param {Array.<String>} attributeKeys Array containing keys of attributes that changed.
+	 */
+}
+
+mix( Selection, EmitterMixin );

+ 145 - 34
packages/ckeditor5-engine/tests/model/selection.js

@@ -457,40 +457,48 @@ describe( 'Selection', () => {
 	} );
 
 	describe( 'removeAllRanges', () => {
-		let spy, ranges;
+		let spy;
 
-		beforeEach( () => {
+		it( 'should remove all stored ranges', () => {
+			selection.addRange( liveRange );
+			selection.addRange( range );
+
+			selection.removeAllRanges();
+
+			expect( Array.from( selection.getRanges() ).length ).to.equal( 0 );
+		} );
+
+		it( 'should fire exactly one change:range event', () => {
 			selection.addRange( liveRange );
 			selection.addRange( range );
 
 			spy = sinon.spy();
 			selection.on( 'change:range', spy );
 
-			ranges = selection._ranges;
-
 			selection.removeAllRanges();
-		} );
 
-		it( 'should remove all stored ranges', () => {
-			expect( Array.from( selection.getRanges() ).length ).to.equal( 0 );
+			expect( spy.calledOnce ).to.be.true;
 		} );
 
-		it( 'should fire exactly one update event', () => {
-			expect( spy.calledOnce ).to.be.true;
+		it( 'should not fire change:range event if there were no ranges', () => {
+			spy = sinon.spy();
+			selection.on( 'change:range', spy );
+
+			selection.removeAllRanges();
+
+			expect( spy.called ).to.be.false;
 		} );
 	} );
 
 	describe( 'setRanges', () => {
 		let newRanges, spy, oldRanges;
 
-		before( () => {
+		beforeEach( () => {
 			newRanges = [
 				new Range( new Position( root, [ 4 ] ), new Position( root, [ 5 ] ) ),
 				new Range( new Position( root, [ 5, 0 ] ), new Position( root, [ 6, 0 ] ) )
 			];
-		} );
 
-		beforeEach( () => {
 			selection.addRange( liveRange );
 			selection.addRange( range );
 
@@ -500,20 +508,11 @@ describe( 'Selection', () => {
 			oldRanges = selection._ranges;
 		} );
 
-		it( 'should throw an error when range is invalid', () => {
-			expect( () => {
-				selection.setRanges( [ { invalid: 'range' } ] );
-			} ).to.throw( CKEditorError, /model-selection-added-not-range/ );
-		} );
-
 		it( 'should remove all ranges and add given ranges', () => {
 			selection.setRanges( newRanges );
 
-			let ranges = selection._ranges;
-
-			expect( ranges.length ).to.equal( 2 );
-			expect( ranges[ 0 ].isEqual( newRanges[ 0 ] ) ).to.be.true;
-			expect( ranges[ 1 ].isEqual( newRanges[ 1 ] ) ).to.be.true;
+			let ranges = Array.from( selection.getRanges() );
+			expect( ranges ).to.deep.equal( newRanges );
 		} );
 
 		it( 'should use last range from given array to get anchor and focus position', () => {
@@ -528,15 +527,28 @@ describe( 'Selection', () => {
 			expect( selection.focus.path ).to.deep.equal( [ 5, 0 ] );
 		} );
 
-		it( 'should fire exactly one update event', () => {
+		it( 'should fire exactly one change:range event', () => {
 			selection.setRanges( newRanges );
 			expect( spy.calledOnce ).to.be.true;
 		} );
+
+		it( 'should fire change:range event with correct parameters', () => {
+			selection.on( 'change:range', ( evt, data ) => {
+				expect( data.directChange ).to.be.true;
+			} );
+
+			selection.setRanges( newRanges );
+		} );
+
+		it( 'should not fire change:range event if given ranges are the same', () => {
+			selection.setRanges( [ liveRange, range ] );
+			expect( spy.calledOnce ).to.be.false;
+		} );
 	} );
 
 	describe( 'setTo', () => {
 		it( 'should set selection to be same as given selection, using setRanges method', () => {
-			sinon.spy( selection, 'setRanges' );
+			const spy = sinon.spy( selection, 'setRanges' );
 
 			const otherSelection = new Selection();
 			otherSelection.addRange( range1 );
@@ -547,6 +559,7 @@ describe( 'Selection', () => {
 			expect( Array.from( selection.getRanges() ) ).to.deep.equal( [ range1, range2 ] );
 			expect( selection.isBackward ).to.be.true;
 			expect( selection.setRanges.calledOnce ).to.be.true;
+			spy.restore();
 		} );
 	} );
 
@@ -679,11 +692,27 @@ describe( 'Selection', () => {
 	describe( 'collapseToStart', () => {
 		it( 'should collapse to start position and fire change event', () => {
 			selection.setRanges( [ range2, range1, range3 ] );
+
+			const spy = sinon.spy();
+			selection.on( 'change:range', spy );
+
 			selection.collapseToStart();
 
 			expect( selection.rangeCount ).to.equal( 1 );
 			expect( selection.isCollapsed ).to.be.true;
 			expect( selection.getFirstPosition().isEqual( range1.start ) ).to.be.true;
+			expect( spy.calledOnce ).to.be.true;
+		} );
+
+		it( 'should do nothing if selection was already collapsed', () => {
+			selection.collapse( range1.start );
+
+			const spy = sinon.spy( selection, 'fire' );
+
+			selection.collapseToStart();
+
+			expect( spy.notCalled ).to.be.true;
+			spy.restore();
 		} );
 
 		it( 'should do nothing if no ranges present', () => {
@@ -697,22 +726,38 @@ describe( 'Selection', () => {
 	} );
 
 	describe( 'collapseToEnd', () => {
-		it( 'should collapse to start position and fire change event', () => {
+		it( 'should collapse to start position and fire change:range event', () => {
 			selection.setRanges( [ range2, range3, range1 ] );
+
+			const spy = sinon.spy();
+			selection.on( 'change:range', spy );
+
 			selection.collapseToEnd();
 
 			expect( selection.rangeCount ).to.equal( 1 );
 			expect( selection.isCollapsed ).to.be.true;
 			expect( selection.getLastPosition().isEqual( range3.end ) ).to.be.true;
+			expect( spy.calledOnce ).to.be.true;
 		} );
 
-		it( 'should do nothing if no ranges present', () => {
+		it( 'should do nothing if selection was already collapsed', () => {
+			selection.collapse( range1.start );
+
 			const spy = sinon.spy( selection, 'fire' );
 
 			selection.collapseToEnd();
 
+			expect( spy.notCalled ).to.be.true;
 			spy.restore();
+		} );
+
+		it( 'should do nothing if selection has no ranges', () => {
+			const spy = sinon.spy( selection, 'fire' );
+
+			selection.collapseToEnd();
+
 			expect( spy.notCalled ).to.be.true;
+			spy.restore();
 		} );
 	} );
 
@@ -756,13 +801,24 @@ describe( 'Selection', () => {
 				expect( selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
 			} );
 
-			it( 'should fire change:attribute event', () => {
+			it( 'should fire change:attribute event with correct parameters', () => {
+				selection.on( 'change:attribute', ( evt, data ) => {
+					expect( data.directChange ).to.be.true;
+					expect( data.attributeKeys ).to.deep.equal( [ 'foo' ] );
+				} );
+
+				selection.setAttribute( 'foo', 'bar' );
+			} );
+
+			it( 'should not fire change:attribute event if attribute with same key and value was already set', () => {
+				selection.setAttribute( 'foo', 'bar' );
+
 				let spy = sinon.spy();
 				selection.on( 'change:attribute', spy );
 
 				selection.setAttribute( 'foo', 'bar' );
 
-				expect( spy.called ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 		} );
 
@@ -821,13 +877,24 @@ describe( 'Selection', () => {
 				expect( selection.getAttribute( 'abc' ) ).to.be.undefined;
 			} );
 
-			it( 'should fire change:attribute event', () => {
+			it( 'should fire change:attribute event with correct parameters', () => {
+				selection.setAttribute( 'foo', 'bar' );
+
+				selection.on( 'change:attribute', ( evt, data ) => {
+					expect( data.directChange ).to.be.true;
+					expect( data.attributeKeys ).to.deep.equal( [ 'foo' ] );
+				} );
+
+				selection.clearAttributes();
+			} );
+
+			it( 'should not fire change:attribute event if there were no attributes', () => {
 				let spy = sinon.spy();
 				selection.on( 'change:attribute', spy );
 
 				selection.clearAttributes();
 
-				expect( spy.called ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 		} );
 
@@ -840,25 +907,69 @@ describe( 'Selection', () => {
 				expect( selection.getAttribute( 'foo' ) ).to.be.undefined;
 			} );
 
-			it( 'should fire change:attribute event', () => {
+			it( 'should fire change:attribute event with correct parameters', () => {
+				selection.setAttribute( 'foo', 'bar' );
+
+				selection.on( 'change:attribute', ( evt, data ) => {
+					expect( data.directChange ).to.be.true;
+					expect( data.attributeKeys ).to.deep.equal( [ 'foo' ] );
+				} );
+
+				selection.removeAttribute( 'foo' );
+			} );
+
+			it( 'should not fire change:attribute event if such attribute did not exist', () => {
 				let spy = sinon.spy();
 				selection.on( 'change:attribute', spy );
 
 				selection.removeAttribute( 'foo' );
 
-				expect( spy.called ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 		} );
 
 		describe( 'setAttributesTo', () => {
 			it( 'should remove all attributes set on element and set the given ones', () => {
-				selection.setRanges( [ rangeInFullP ] );
 				selection.setAttribute( 'abc', 'xyz' );
 				selection.setAttributesTo( { foo: 'bar' } );
 
 				expect( selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
 				expect( selection.getAttribute( 'abc' ) ).to.be.undefined;
 			} );
+
+			it( 'should fire only one change:attribute event', () => {
+				selection.setAttributesTo( { foo: 'bar', xxx: 'yyy' } );
+
+				let spy = sinon.spy();
+				selection.on( 'change:attribute', spy );
+
+				selection.setAttributesTo( { foo: 'bar', abc: 'def' } );
+
+				expect( spy.calledOnce ).to.be.true;
+			} );
+
+			it( 'should fire change:attribute event with correct parameters', () => {
+				selection.setAttributesTo( { foo: 'bar', xxx: 'yyy' } );
+
+				selection.on( 'change:attribute', ( evt, data ) => {
+					expect( data.directChange ).to.be.true;
+					expect( data.attributeKeys ).to.deep.equal( [ 'abc', 'xxx' ] );
+				} );
+
+				selection.setAttributesTo( { foo: 'bar', abc: 'def' } );
+			} );
+
+			it( 'should not fire change:attribute event if attributes had not changed', () => {
+				selection.setRanges( [ rangeInFullP ] );
+				selection.setAttributesTo( { foo: 'bar', xxx: 'yyy' } );
+
+				let spy = sinon.spy();
+				selection.on( 'change:attribute', spy );
+
+				selection.setAttributesTo( { xxx: 'yyy', foo: 'bar' } );
+
+				expect( spy.called ).to.be.false;
+			} );
 		} );
 	} );
 } );