From 981d8b923a016888d6a323c56526b214eb9a09f8 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 30 Jul 2014 22:28:39 -0400 Subject: [PATCH] Evented#removeListener As a conscious TODO: need to properly handle attaching the same event listener multiple times (define its behavior), and maybe even provide an API to configure that behavior in some way. --- src/event/Evented.js | 113 +++++++++++++++++--- test/event/EventedTest.js | 213 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 313 insertions(+), 13 deletions(-) diff --git a/src/event/Evented.js b/src/event/Evented.js index ae49314..c7467ca 100644 --- a/src/event/Evented.js +++ b/src/event/Evented.js @@ -20,7 +20,10 @@ */ var Trait = require( 'easejs' ).Trait, - isArray = require( '../std/Array' ).isArray; + isArray = require( '../std/Array' ).isArray, + + // used for hiding cached event indexes + _evid = Symbol(); /** @@ -42,6 +45,12 @@ module.exports = Trait( 'Evented', */ _events: {}, + /** + * List of gaps in event callback lists + * @type {Object} + */ + _gaps: {}, + /** * Defines a list of events by unique identifier @@ -89,7 +98,8 @@ module.exports = Trait( 'Evented', } // will contain each callback associated with this event - this._events[ ids[ i ] ] = []; + this._events[ id ] = []; + this._gaps[ id ] = []; } }, @@ -131,7 +141,7 @@ module.exports = Trait( 'Evented', * provides different guarantees. * * @param {string} ev defined event id - * @param {Array.} evc event callbacks (hooks) + * @param {Array.} evc event callbacks (listeners) * @param {Array} args argument list to apply to callbacks * * @return {Evented} self @@ -140,8 +150,9 @@ module.exports = Trait( 'Evented', { var i = evc.length; + // note that there may be gaps (see _gaps) while ( i-- ) { - evc[ i ].apply( null, args ); + evc[ i ] && evc[ i ].apply( null, args ); } return this; @@ -149,28 +160,104 @@ module.exports = Trait( 'Evented', /** - * Invoke callback when an event is emitted + * Invoke listener when an event is emitted * - * The event EV must be defined and CALLBACK must be a function. The + * The event EV must be defined and LISTENER must be a function. The * context in which the callback is invoked (the value of `this') is * undefined. * + * @O {#hookEvent} depends on hooking algorithm + * * @param {string} ev defined event id - * @param {Function} callback function to invoke when event is emitted + * @param {Function} listener function to invoke when event is emitted * * @return {Evented} self */ - on( ev, callback ) + on( ev, listener ) { if ( !( this._events[ ev ] ) ) { - throw Error( "Cannot hook undefined event `" + ev + "'" ); + throw Error( `Cannot hook undefined event \`${ev}'` ); } - else if ( typeof callback !== 'function' ) { - throw TypeError( "Event callback must be a function" ); + else if ( typeof listener !== 'function' ) { + throw TypeError( "Event listener must be a function" ); } - this._events[ ev ].push( callback ); + this.hookEvent( ev, listener ); return this; - } + }, + + + /** + * Adds event listener, ensuring that gaps created by removing listeners + * are filled if available + * + * Listeners are added in constant time. A field is set on LISTENER to + * cache the index, allowing for removing listeners in constant time. + * + * Subtypes may use this to alter hook behavior by preventing the hook + * (by not forwarding the listener to __supet) and instead invoke + * __super with a new listener, possibly wrapping the original. + * + * @param {string} ev defined event id + * @param {Function} listener function to invoke when event is emitted + * + * @return {Evented} self + */ + 'virtual protected hookEvent'( ev, listener ) + { + let evls = this._events[ ev ], + avail = this._gaps[ ev ].pop(); + + if ( avail === undefined ) { + listener[ _evid ] = evls.length; + evls.push( listener ); + } + else { + listener[ _evid ] = avail; + evls[ avail ] = listener; + } + + return this; + }, + + + /** + * Removes a previously hooked listener in constant time, preventing it + * from being invoked on future emits of event EV + * + * EV must be a valid event identifier. If LISTENER is not registered + * for event id EV, no error will occur (since it fulfills the criteria + * that it will not be subsequently invoked by event EV). + * + * Listeners are compared by reference---the exact listener that was + * registered with EV must be passed for removal. + * + * @param {string} ev defined event id + * @param {Function} listener listener to remove + * + * @return {Evented} self + * + * @throws {Error} when EV is not a defined event identifier + */ + removeListener( ev, listener ) + { + if ( !( this._events[ ev ] ) ) { + throw Error( `Cannot unhook undefined event \`${ev}'` ); + } + + let evls = this._events[ ev ], + index = listener[ _evid ]; + + // this is important, since we (a) cannot necessarily trust that the + // cached index hasn't been forged and (b) if a listener is removed + // multiple times, the index may contain a different listener + if ( evls[ index ] === listener ) + { + evls[ index ] = undefined; + this._gaps[ ev ].push( index ); + } + + return this; + }, } ); diff --git a/test/event/EventedTest.js b/test/event/EventedTest.js index 574958a..77f4286 100644 --- a/test/event/EventedTest.js +++ b/test/event/EventedTest.js @@ -353,5 +353,218 @@ describe( 'event.Evented', () => .evEmit( ev ); } ); } ); + + + /** + * Permits additional actions on hook and overriding event hooking + * storage behavior + */ + describe( '#hookEvent', function() + { + var ev = 'foo', + f = ()=>{}; + + it( 'can be overridden by subtypes', ( done ) => + { + common.checkOverride( EvStub, 'hookEvent', + ( given_ev, given_callback ) => + { + expect( given_ev ).to.equal( ev ); + expect( given_callback ).to.equal( f ); + done(); + } + )().evDefineEvents( [ ev ] ).on( ev, f ); + } ); + + + /** + * Should provide full control over how the listeners are handled. + */ + it( 'can prevent listeners from being added', () => + { + var ev = 'foo'; + + // override does nothing + expect( () => + { + EvStub.extend( + { + 'override hookEvent': ( _, __ ) => {} + } )() + .evDefineEvents( [ ev ] ) + .on( ev, () => + { + throw Error( "Event called!" ); + } ).evEmit( ev ); + } ).to.not.throw( Error ); + } ); + } ); + + + describe( '#removeListener', () => + { + it( 'rejects unknown event ids', () => + { + var ev = 'foo'; + expect( () => stub.removeListener( ev , ()=>{} ) ) + .to.throw( Error, ev ); + } ); + + + /** + * The purpose of removing a listener is to prevent it from being + * called when an event is emitted. We don't really care how this is + * done; just don't call it. + */ + it( 'prevents removed listener from being called', () => + { + var ev = 'foo', + called = false, + listener = ( () => called = true ); + + stub.evDefineEvents( [ ev ] ); + + // we have already proved that adding a listener works; + // immediately remove it and ensure that it's not called + stub.on( ev, listener ) + .removeListener( ev, listener ); + + stub.evEmit( ev ); + expect( called ).to.equal( false ); + } ); + + + /** + * If the given listener to remove was never registered (or no + * longer is), then we will shall act as though it was successfully + * removed (since, according to our initial requirement, we + * succeeded in ensuring that the listener will not be called on + * subsequent emits). + */ + it( 'does not fail and returns self on missing listener', () => + { + let ev = 'foo'; + + expect( + stub.evDefineEvents( [ ev ] ) + .removeListener( ev, () => {} ) + ).to.equal( stub ); + } ); + + + it( 'returns self on success', () => + { + let ev = 'foo', + f = () => {}; + + expect( + stub.evDefineEvents( [ ev ] ) + .on( ev, f ) + .removeListener( ev, f ) + ).to.equal( stub ); + } ); + + + /** + * These are tests that are not design per se, but are intended to + * comprehensively guard against future design changes, likely or + * not. + */ + describe( 'sanity check', () => + { + /** + * Removing a single listener for an event should not touch + * others for that same event + */ + it( 'does not remove all listeners for event', () => + { + var ev = 'foo', + f1c = false, + f2c = false, + f1 = () => f1c = true, + f2 = () => f2c = true; + + stub.evDefineEvents( [ ev ] ) + .on( ev, f1 ) + .on( ev, f2 ) + .removeListener( ev, f1 ) + .evEmit( ev ); + + expect( f1c ).to.be.false; + expect( f2c ).to.be.true; + } ); + + + /** + * The same listener may be added to multiple events---we should + * only ever remove it from the one that was requested. We have + * already proven in previous tests that event listeners are + * actually removed as expected, so we need only ensure that it + * is *not* removed from another event that it is attached to. + */ + it( 'does not remove same listener on other events', () => + { + var evs = [ 'foo', 'bar' ], + called = false, + f = () => called = true; + + stub.evDefineEvents( evs ) + .on( evs[ 0 ], f ) + .on( evs[ 1 ], f ) + .removeListener( evs[ 0 ], f ) + .evEmit( evs[ 1 ] ); + + expect( called ).to.be.true; + } ); + + + /** + * Removing a listener may introduce, depending on + * implementation details that we do not care about here, gaps + * in the data structure holding the listeners. The idea here is + * to ensure that this gap, if immediately filled, will not + * cause problems. + */ + it( 'can add listener after removal', () => + { + var ev = 'foo', + called = 0, + frm = () => {}, + fok = () => called++, + fok2 = () => called += 2; + + stub.evDefineEvents( [ ev ] ) + .on( ev, frm ) + .removeListener( ev, frm ) + .on( ev, fok ) + .on( ev, fok2 ) + .evEmit( ev ); + + expect( called ).to.equal( 3 ); + } ); + + + /** + * Similar to the above test, but ensures that the system is not + * marking a listener for deletion in such a way that it won't + * be recognized later. This would be an odd implementation, but + * people do odd things sometimes. + */ + it( 'can re-add removed listener', () => + { + var ev = 'foo', + called = false, + f = () => called = true; + + stub.evDefineEvents( [ ev ] ) + .on( ev, f ) + .removeListener( ev, f ) + .on( ev, f ) + .evEmit( ev ); + + expect( called ).to.be.true; + } ); + } ); + } ); } );