Browse Source

Using id to compare descriptors in HighlightStack.

Szymon Kupś 8 years ago
parent
commit
1651fb89d6

+ 37 - 35
packages/ckeditor5-widget/src/highlightstack.js

@@ -35,34 +35,18 @@ export default class HighlightStack {
 	 */
 	add( descriptor ) {
 		const stack = this._stack;
-		let i = 0;
 
-		// Find correct place to insert descriptor in the stack.
-		while ( stack[ i ] && shouldABeBeforeB( stack[ i ], descriptor ) ) {
-			i++;
-		}
-
-		stack.splice( i, 0, descriptor );
-
-		// New element at the stack top.
-		if ( i === 0 ) {
-			const data = {
-				newDescriptor: descriptor
-			};
-
-			// If old descriptor is present it was pushed down the stack.
-			if ( stack[ 1 ] ) {
-				const oldDescriptor = stack[ 1 ];
-
-				// New descriptor on the top is same as previous one - do not fire any event.
-				if ( compareDescriptors( descriptor, oldDescriptor ) ) {
-					return;
-				}
-
-				data.oldDescriptor = oldDescriptor;
-			}
-
-			this.fire( 'change:top', data );
+		// Save top descriptor and insert new one. If top is changed - fire event.
+		const oldTop = stack[ 0 ];
+		this._insertDescriptor( descriptor );
+		const newTop = stack[ 0 ];
+
+		// When new object is at the top and stores different information.
+		if ( oldTop !== newTop && !cmp( oldTop, newTop ) ) {
+			this.fire( 'change:top', {
+				oldDescriptor: oldTop,
+				newDescriptor: newTop
+			} );
 		}
 	}
 
@@ -102,22 +86,40 @@ export default class HighlightStack {
 
 			if ( stack[ 0 ] ) {
 				const newDescriptor = stack[ 0 ];
-
-				// New descriptor on the top is same as removed one - do not fire any event.
-				if ( compareDescriptors( descriptor, newDescriptor ) ) {
-					return;
-				}
-
 				data.newDescriptor = newDescriptor;
 			}
 
 			this.fire( 'change:top', data );
 		}
 	}
+
+	_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.
+		if ( index > -1 ) {
+			stack.splice( index, 1 );
+		}
+
+		// Find correct place to insert descriptor in the stack.
+		let i = 0;
+
+		while ( stack[ i ] && shouldABeBeforeB( stack[ i ], descriptor ) ) {
+			i++;
+		}
+
+		stack.splice( i, 0, descriptor );
+	}
+
 }
 
 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.
 //
@@ -125,10 +127,10 @@ mix( HighlightStack, EmitterMixin );
 // @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptorB
 // @returns {Boolean}
 function compareDescriptors( descriptorA, descriptorB ) {
-	return descriptorA.priority == descriptorB.priority &&
-		classesToString( descriptorA.class ) == classesToString( descriptorB.class );
+	return descriptorA.id == descriptorB.id;
 }
 
+
 // Checks whenever first descriptor should be placed in the stack before second one.
 //
 // @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} a

+ 17 - 17
packages/ckeditor5-widget/tests/highlightstack.js

@@ -14,7 +14,7 @@ describe( 'HighlightStack', () => {
 
 	it( 'should fire event when new descriptor is provided to an empty stack', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-id' };
 
 		stack.on( 'change:top', spy );
 		stack.add( descriptor );
@@ -26,8 +26,8 @@ describe( 'HighlightStack', () => {
 
 	it( 'should fire event when new top element has changed', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 11, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 11, class: 'css-class', id: 'descriptor-2' };
 
 		stack.on( 'change:top', spy );
 		stack.add( descriptor );
@@ -40,8 +40,8 @@ describe( 'HighlightStack', () => {
 
 	it( 'should not fire event when element with lower priority was added', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 9, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 9, class: 'css-class', id: 'descriptor-2' };
 
 		stack.on( 'change:top', spy );
 		stack.add( descriptor );
@@ -54,8 +54,8 @@ describe( 'HighlightStack', () => {
 
 	it( 'should fire event when top element was removed', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 11, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 11, class: 'css-class', id: 'descriptor-2' };
 
 		stack.add( descriptor );
 		stack.add( secondDescriptor );
@@ -71,8 +71,8 @@ describe( 'HighlightStack', () => {
 
 	it( 'should not fire event when other than top element is removed', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 11, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 11, class: 'css-class', id: 'descriptor-2' };
 
 		stack.add( descriptor );
 		stack.add( secondDescriptor );
@@ -86,8 +86,8 @@ describe( 'HighlightStack', () => {
 
 	it( 'should not fire event when same descriptor is added', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 10, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
 
 		stack.on( 'change:top', spy );
 		stack.add( descriptor );
@@ -110,8 +110,8 @@ describe( 'HighlightStack', () => {
 
 	it( 'should not fire when trying to remove descriptor which is not present', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 12, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 12, class: 'css-class', id: 'descriptor-2' };
 
 		stack.add( descriptor );
 		stack.on( 'change:top', spy );
@@ -122,7 +122,7 @@ describe( 'HighlightStack', () => {
 
 	it( 'should fire event when last element from stack was removed', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor' };
 
 		stack.add( descriptor );
 		stack.on( 'change:top', spy );
@@ -133,10 +133,10 @@ describe( 'HighlightStack', () => {
 		expect( spy.firstCall.args[ 1 ].oldDescriptor ).to.equal( descriptor );
 	} );
 
-	it( 'should not fire event when new top descriptor is same as previous', () => {
+	it.only( 'should not fire event when new top descriptor is same as previous', () => {
 		const spy = sinon.spy();
-		const descriptor = { priority: 10, class: 'css-class' };
-		const secondDescriptor = { priority: 10, class: 'css-class' };
+		const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
+		const secondDescriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' };
 
 		stack.add( descriptor );
 		stack.add( secondDescriptor );