8
0
Pārlūkot izejas kodu

Using HighlihtDescriptor.id to compare and insert descriptors on HighlightStack.

Szymon Kupś 8 gadi atpakaļ
vecāks
revīzija
aaf7965fca

+ 45 - 44
packages/ckeditor5-widget/src/highlightstack.js

@@ -42,7 +42,7 @@ export default class HighlightStack {
 		const newTop = stack[ 0 ];
 
 		// When new object is at the top and stores different information.
-		if ( oldTop !== newTop && !cmp( oldTop, newTop ) ) {
+		if ( oldTop !== newTop && !compareDescriptors( oldTop, newTop ) ) {
 			this.fire( 'change:top', {
 				oldDescriptor: oldTop,
 				newDescriptor: newTop
@@ -58,51 +58,43 @@ export default class HighlightStack {
 	 */
 	remove( descriptor ) {
 		const stack = this._stack;
-		const length = stack.length;
 
-		if ( length === 0 ) {
-			return;
-		}
-
-		let i = 0;
-
-		while ( stack[ i ] && !compareDescriptors( descriptor, stack[ i ] ) ) {
-			i++;
-
-			// Descriptor not found.
-			if ( i >= stack.length ) {
-				return;
-			}
-		}
-
-		stack.splice( i, 1 );
-
-		// Element from stack top was removed - fire `change:top` event with new first element. It might be `undefined`
-		// which informs that no descriptor is currently at the top.
-		if ( i === 0 ) {
-			const data = {
-				oldDescriptor: descriptor
-			};
-
-			if ( stack[ 0 ] ) {
-				const newDescriptor = stack[ 0 ];
-				data.newDescriptor = newDescriptor;
-			}
+		const oldTop = stack[ 0 ];
+		this._removeDescriptor( descriptor );
+		const newTop = stack[ 0 ];
 
-			this.fire( 'change:top', data );
+		// When new object is at the top and stores different information.
+		if ( oldTop !== newTop && !compareDescriptors( oldTop, newTop ) ) {
+			this.fire( 'change:top', {
+				oldDescriptor: oldTop,
+				newDescriptor: newTop
+			} );
 		}
 	}
 
+	/**
+	 * Inserts given descriptor in correct place in the stack. It also takes care about updating information when
+	 * descriptor with same id is already present.
+	 *
+	 * @private
+	 * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptor
+	 */
 	_insertDescriptor( descriptor ) {
 		const stack = this._stack;
 		const index = stack.findIndex( item => item.id === descriptor.id );
 
-		// If descriptor with same id is on the list - remove it.
+		// Inserting exact same descriptor - do nothing.
+		if ( compareDescriptors( descriptor, stack[ index ] ) ) {
+			return;
+		}
+
+		// If descriptor with same id but with different information is on the stack - remove it.
 		if ( index > -1 ) {
 			stack.splice( index, 1 );
 		}
 
 		// Find correct place to insert descriptor in the stack.
+		// It have different information (for example priority) so it must be re-inserted in correct place.
 		let i = 0;
 
 		while ( stack[ i ] && shouldABeBeforeB( stack[ i ], descriptor ) ) {
@@ -112,25 +104,34 @@ export default class HighlightStack {
 		stack.splice( i, 0, descriptor );
 	}
 
+	/**
+	 * Removes descriptor with given id from the stack.
+	 *
+	 * @private
+	 * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptor
+	 */
+	_removeDescriptor( descriptor ) {
+		const stack = this._stack;
+		const index = stack.findIndex( item => item.id === descriptor.id );
+
+		// If descriptor with same id is on the list - remove it.
+		if ( index > -1 ) {
+			stack.splice( index, 1 );
+		}
+	}
 }
 
 mix( HighlightStack, EmitterMixin );
 
-function cmp( a, b ) {
-	return a && b && a.id == b.id && classesToString( a.class ) == classesToString( b.class ) && a.priority == b.priority;
-}
-
-// Compares two highlight descriptors by priority and CSS class names. Returns `true` when both descriptors are
-// considered equal.
+// Compares two descriptors by checking their priority and class list.
 //
-// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptorA
-// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptorB
-// @returns {Boolean}
-function compareDescriptors( descriptorA, descriptorB ) {
-	return descriptorA.id == descriptorB.id;
+// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} a
+// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} b
+// @returns {Boolean} Returns true if both descriptors are defined and have same priority and classes.
+function compareDescriptors( a, b ) {
+	return a && b && a.priority == b.priority && classesToString( a.class ) == classesToString( b.class );
 }
 
-
 // Checks whenever first descriptor should be placed in the stack before second one.
 //
 // @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} a

+ 25 - 8
packages/ckeditor5-widget/tests/highlightstack.js

@@ -38,6 +38,23 @@ describe( 'HighlightStack', () => {
 		expect( spy.secondCall.args[ 1 ].oldDescriptor ).to.equal( descriptor );
 	} );
 
+	it( 'should fire event when top element has updated', () => {
+		const spy = sinon.spy();
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 11, class: 'css-class', id: 'descriptor-1' };
+
+		stack.on( 'change:top', spy );
+		stack.add( descriptor );
+		stack.add( secondDescriptor );
+
+		sinon.assert.calledTwice( spy );
+		expect( spy.firstCall.args[ 1 ].newDescriptor ).to.equal( descriptor );
+		expect( spy.firstCall.args[ 1 ].oldDescriptor ).to.be.undefined;
+
+		expect( spy.secondCall.args[ 1 ].newDescriptor ).to.equal( secondDescriptor );
+		expect( spy.secondCall.args[ 1 ].oldDescriptor ).to.equal( descriptor );
+	} );
+
 	it( 'should not fire event when element with lower priority was added', () => {
 		const spy = sinon.spy();
 		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
@@ -133,10 +150,10 @@ describe( 'HighlightStack', () => {
 		expect( spy.firstCall.args[ 1 ].oldDescriptor ).to.equal( descriptor );
 	} );
 
-	it.only( 'should not fire event when new top descriptor is same as previous', () => {
+	it( 'should not fire event when new top descriptor is same as previous', () => {
 		const spy = sinon.spy();
 		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
-		const secondDescriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 10, class: 'css-class', id: 'descriptor-2' };
 
 		stack.add( descriptor );
 		stack.add( secondDescriptor );
@@ -148,9 +165,9 @@ describe( 'HighlightStack', () => {
 
 	it( 'should sort by class when priorities are the same', () => {
 		const spy = sinon.spy();
-		const descriptorA = { priority: 10, class: 'css-a' };
-		const descriptorB = { priority: 10, class: 'css-b' };
-		const descriptorC = { priority: 10, class: 'css-c' };
+		const descriptorA = { priority: 10, class: 'css-a', id: 'descriptor-1' };
+		const descriptorB = { priority: 10, class: 'css-b', id: 'descriptor-2' };
+		const descriptorC = { priority: 10, class: 'css-c', id: 'descriptor-3' };
 
 		stack.on( 'change:top', spy );
 		stack.add( descriptorB );
@@ -167,9 +184,9 @@ describe( 'HighlightStack', () => {
 
 	it( 'should sort by class when priorities are the same - array of CSS classes', () => {
 		const spy = sinon.spy();
-		const descriptorA = { priority: 10, class: [ 'css-a', 'css-z' ] };
-		const descriptorB = { priority: 10, class: [ 'css-a', 'css-y' ] };
-		const descriptorC = { priority: 10, class: 'css-c' };
+		const descriptorA = { priority: 10, class: [ 'css-a', 'css-z' ], id: 'descriptor-1' };
+		const descriptorB = { priority: 10, class: [ 'css-a', 'css-y' ], id: 'descriptor-2' };
+		const descriptorC = { priority: 10, class: 'css-c', id: 'descriptor-3' };
 
 		stack.on( 'change:top', spy );
 		stack.add( descriptorB );

+ 15 - 15
packages/ckeditor5-widget/tests/utils.js

@@ -59,10 +59,10 @@ describe( 'widget utils', () => {
 			expect( typeof set ).to.equal( 'function' );
 			expect( typeof remove ).to.equal( 'function' );
 
-			set( element, { priority: 1, class: 'highlight' } );
+			set( element, { priority: 1, class: 'highlight', id: 'highlight' } );
 			expect( element.hasClass( 'highlight' ) ).to.be.true;
 
-			remove( element, { priority: 1, class: 'highlight' } );
+			remove( element, { priority: 1, class: 'highlight', id: 'highlight' } );
 			expect( element.hasClass( 'highlight' ) ).to.be.false;
 		} );
 
@@ -75,11 +75,11 @@ describe( 'widget utils', () => {
 			expect( typeof set ).to.equal( 'function' );
 			expect( typeof remove ).to.equal( 'function' );
 
-			set( element, { priority: 1, class: [ 'highlight', 'foo' ] } );
+			set( element, { priority: 1, class: [ 'highlight', 'foo' ], id: 'highlight' } );
 			expect( element.hasClass( 'highlight' ) ).to.be.true;
 			expect( element.hasClass( 'foo' ) ).to.be.true;
 
-			remove( element, { priority: 1, class: [ 'foo', 'highlight' ] } );
+			remove( element, { priority: 1, class: [ 'foo', 'highlight' ], id: 'highlight' } );
 			expect( element.hasClass( 'highlight' ) ).to.be.false;
 			expect( element.hasClass( 'foo' ) ).to.be.false;
 		} );
@@ -174,7 +174,7 @@ describe( 'widget utils', () => {
 		} );
 
 		it( 'should call highlight methods when descriptor is added and removed', () => {
-			const descriptor = { priority: 10, class: 'highlight' };
+			const descriptor = { priority: 10, class: 'highlight', id: 'highlight' };
 
 			set( element, descriptor );
 			remove( element, descriptor );
@@ -187,8 +187,8 @@ describe( 'widget utils', () => {
 		} );
 
 		it( 'should call highlight methods when next descriptor is added', () => {
-			const descriptor = { priority: 10, class: 'highlight' };
-			const secondDescriptor = { priority: 11, class: 'highlight' };
+			const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' };
+			const secondDescriptor = { priority: 11, class: 'highlight', id: 'highlight-2' };
 
 			set( element, descriptor );
 			set( element, secondDescriptor );
@@ -199,8 +199,8 @@ describe( 'widget utils', () => {
 		} );
 
 		it( 'should not call highlight methods when descriptor with lower priority is added', () => {
-			const descriptor = { priority: 10, class: 'highlight' };
-			const secondDescriptor = { priority: 9, class: 'highlight' };
+			const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' };
+			const secondDescriptor = { priority: 9, class: 'highlight', id: 'highlight-2' };
 
 			set( element, descriptor );
 			set( element, secondDescriptor );
@@ -210,8 +210,8 @@ describe( 'widget utils', () => {
 		} );
 
 		it( 'should call highlight methods when descriptor is removed changing active descriptor', () => {
-			const descriptor = { priority: 10, class: 'highlight' };
-			const secondDescriptor = { priority: 11, class: 'highlight' };
+			const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' };
+			const secondDescriptor = { priority: 11, class: 'highlight', id: 'highlight-2' };
 
 			set( element, descriptor );
 			set( element, secondDescriptor );
@@ -228,8 +228,8 @@ describe( 'widget utils', () => {
 		} );
 
 		it( 'should call highlight methods when descriptor is removed not changing active descriptor', () => {
-			const descriptor = { priority: 10, class: 'highlight' };
-			const secondDescriptor = { priority: 9, class: 'highlight' };
+			const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' };
+			const secondDescriptor = { priority: 9, class: 'highlight', id: 'highlight-2' };
 
 			set( element, descriptor );
 			set( element, secondDescriptor );
@@ -242,8 +242,8 @@ describe( 'widget utils', () => {
 		} );
 
 		it( 'should call highlight methods - CSS class array', () => {
-			const descriptor = { priority: 10, class: [ 'highlight', 'a' ] };
-			const secondDescriptor = { priority: 10, class: [ 'highlight', 'b' ] };
+			const descriptor = { priority: 10, class: [ 'highlight', 'a' ], id: 'highlight-1' };
+			const secondDescriptor = { priority: 10, class: [ 'highlight', 'b' ], id: 'highlight-2' };
 
 			set( element, descriptor );
 			set( element, secondDescriptor );