Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #190 from ckeditor/t/144-new
Browse files Browse the repository at this point in the history 
Other: Aligned behaviors of `EmitterMixin` methods responsible for adding end removing listeners. Closes #144.

The `emitter.on()` now has the same behavior as `emitter.listenTo( emitter )` as well as `emitter.off()` is the same as `emitter.stopListening( emitter )`. This made `emitter.stopListening()` correctly remove all listeners added in any way to it which prevents memory leaks.
  • Loading branch information
@Reinmar
Reinmar authored Dec 11, 2017
2 parents 84ccda2 + cd21a75 commit 460d7f4
Show file tree
Hide file tree
Showing 4 changed files with 371 additions and 104 deletions.
58 changes: 23 additions & 35 deletions src/dom/emittermixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
* Registers a callback function to be executed when an event is fired in a specific Emitter or DOM Node.
* It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}.
*
* @method module:utils/dom/emittermixin~EmitterMixin#listenTo
* @param {module:utils/emittermixin~Emitter|Node} emitter The object that fires the event.
* @param {String} event The name of the event.
* @param {Function} callback The function to be called on event.
Expand All @@ -49,20 +50,20 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
* order they were added.
* @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered
* listener before being dispatched to any EventTarget beneath it in the DOM tree.
*
* @method module:utils/dom/emittermixin~EmitterMixin#listenTo
*/
listenTo( ...args ) {
const emitter = args[ 0 ];

listenTo( emitter, ...rest ) {
// Check if emitter is an instance of DOM Node. If so, replace the argument with
// corresponding ProxyEmitter (or create one if not existing).
if ( isDomNode( emitter ) || isWindow( emitter ) ) {
args[ 0 ] = this._getProxyEmitter( emitter ) || new ProxyEmitter( emitter );
const proxy = this._getProxyEmitter( emitter ) || new ProxyEmitter( emitter );

proxy.attach( ...rest );

emitter = proxy;
}

// Execute parent class method with Emitter (or ProxyEmitter) instance.
EmitterMixin.listenTo.apply( this, args );
EmitterMixin.listenTo.call( this, emitter, ...rest );
},

/**
Expand All @@ -74,17 +75,14 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
* * To stop listening to all events fired by a specific object.
* * To stop listening to all events fired by all object.
*
* @method module:utils/dom/emittermixin~EmitterMixin#stopListening
* @param {module:utils/emittermixin~Emitter|Node} [emitter] The object to stop listening to. If omitted, stops it for all objects.
* @param {String} [event] (Requires the `emitter`) The name of the event to stop listening to. If omitted, stops it
* for all events from `emitter`.
* @param {Function} [callback] (Requires the `event`) The function to be removed from the call list for the given
* `event`.
*
* @method module:utils/dom/emittermixin~EmitterMixin#stopListening
*/
stopListening( ...args ) {
const emitter = args[ 0 ];

stopListening( emitter, event, callback ) {
// Check if emitter is an instance of DOM Node. If so, replace the argument with corresponding ProxyEmitter.
if ( isDomNode( emitter ) || isWindow( emitter ) ) {
const proxy = this._getProxyEmitter( emitter );
Expand All @@ -94,11 +92,15 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
return;
}

args[ 0 ] = proxy;
emitter = proxy;
}

// Execute parent class method with Emitter (or ProxyEmitter) instance.
EmitterMixin.stopListening.apply( this, args );
EmitterMixin.stopListening.call( this, emitter, event, callback );

if ( emitter instanceof ProxyEmitter ) {
emitter.detach( event );
}
},

/**
Expand Down Expand Up @@ -173,21 +175,14 @@ extend( ProxyEmitter.prototype, EmitterMixin, {
* It attaches a native DOM listener to the DOM Node. When fired,
* a corresponding Emitter event will also fire with DOM Event object as an argument.
*
* @method module:utils/dom/emittermixin~ProxyEmitter#attach
* @param {String} event The name of the event.
* @param {Function} callback The function to be called on event.
* @param {Object} [options={}] Additional options.
* @param {module:utils/priorities~PriorityString|Number} [options.priority='normal'] The priority of this event callback. The higher
* the priority value the sooner the callback will be fired. Events having the same priority are called in the
* order they were added.
* @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered
* listener before being dispatched to any EventTarget beneath it in the DOM tree.
*
* @method module:utils/dom/emittermixin~ProxyEmitter#on
*/
on( event, callback, options = {} ) {
// Execute parent class method first.
EmitterMixin.on.call( this, event, callback, options );

attach( event, callback, options = {} ) {
// If the DOM Listener for given event already exist it is pointless
// to attach another one.
if ( this._domListeners && this._domListeners[ event ] ) {
Expand All @@ -211,34 +206,27 @@ extend( ProxyEmitter.prototype, EmitterMixin, {
/**
* Stops executing the callback on the given event.
*
* @method module:utils/dom/emittermixin~ProxyEmitter#detach
* @param {String} event The name of the event.
* @param {Function} callback The function to stop being called.
*
* @method module:utils/dom/emittermixin~ProxyEmitter#off
*/
off( event, callback ) {
// Execute parent class method first.
EmitterMixin.off.call( this, event, callback );

detach( event ) {
let events;

// Remove native DOM listeners which are orphans. If no callbacks
// are awaiting given event, detach native DOM listener from DOM Node.
// See: {@link on}.
// See: {@link attach}.

if ( this._domListeners[ event ] && ( !( events = this._events[ event ] ) || !events.callbacks.length ) ) {
this._domListeners[ event ].removeListener();
}
},

/**
* Create a native DOM listener callback. When the native DOM event
* Creates a native DOM listener callback. When the native DOM event
* is fired it will fire corresponding event on this ProxyEmitter.
* Note: A native DOM Event is passed as an argument.
*
* @private
* @param {String} event
*
* @method module:utils/dom/emittermixin~ProxyEmitter#_createDomListener
* @param {String} event The name of the event.
* @param {Boolean} useCapture Indicates whether the listener was created for capturing event.
Expand All @@ -251,7 +239,7 @@ extend( ProxyEmitter.prototype, EmitterMixin, {

// Supply the DOM listener callback with a function that will help
// detach it from the DOM Node, when it is no longer necessary.
// See: {@link off}.
// See: {@link detach}.
domListener.removeListener = () => {
this._domNode.removeEventListener( event, domListener, useCapture );
delete this._domListeners[ event ];
Expand Down
140 changes: 77 additions & 63 deletions src/emittermixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,8 @@ const EmitterMixin = {
/**
* Registers a callback function to be executed when an event is fired.
*
* Events can be grouped in namespaces using `:`.
* When namespaced event is fired, it additionally fires all callbacks for that namespace.
*
* myEmitter.on( 'myGroup', genericCallback );
* myEmitter.on( 'myGroup:myEvent', specificCallback );
*
* // genericCallback is fired.
* myEmitter.fire( 'myGroup' );
* // both genericCallback and specificCallback are fired.
* myEmitter.fire( 'myGroup:myEvent' );
* // genericCallback is fired even though there are no callbacks for "foo".
* myEmitter.fire( 'myGroup:foo' );
*
* An event callback can {@link module:utils/eventinfo~EventInfo#stop stop the event} and
* set the {@link module:utils/eventinfo~EventInfo#return return value} of the {@link #fire} method.
* Shorthand for {@link #listenTo `this.listenTo( this, event, callback, options )`} (it makes the emitter
* listen on itself).
*
* @method #on
* @param {String} event The name of the event.
Expand All @@ -49,34 +36,7 @@ const EmitterMixin = {
* order they were added.
*/
on( event, callback, options = {} ) {
createEventNamespace( this, event );
const lists = getCallbacksListsForNamespace( this, event );
const priority = priorities.get( options.priority );

callback = {
callback,
priority
};

// Add the callback to all callbacks list.
for ( const callbacks of lists ) {
// Add the callback to the list in the right priority position.
let added = false;

for ( let i = 0; i < callbacks.length; i++ ) {
if ( callbacks[ i ].priority < priority ) {
callbacks.splice( i, 0, callback );
added = true;

break;
}
}

// Add at the end, if right place was not found.
if ( !added ) {
callbacks.push( callback );
}
}
this.listenTo( this, event, callback, options );
},

/**
Expand All @@ -101,33 +61,41 @@ const EmitterMixin = {
};

// Make a similar on() call, simply replacing the callback.
this.on( event, onceCallback, options );
this.listenTo( this, event, onceCallback, options );
},

/**
* Stops executing the callback on the given event.
* Shorthand for {@link #stopListening `this.stopListening( this, event, callback )`}.
*
* @method #off
* @param {String} event The name of the event.
* @param {Function} callback The function to stop being called.
*/
off( event, callback ) {
const lists = getCallbacksListsForNamespace( this, event );

for ( const callbacks of lists ) {
for ( let i = 0; i < callbacks.length; i++ ) {
if ( callbacks[ i ].callback == callback ) {
// Remove the callback from the list (fixing the next index).
callbacks.splice( i, 1 );
i--;
}
}
}
this.stopListening( this, event, callback );
},

/**
* Registers a callback function to be executed when an event is fired in a specific (emitter) object.
*
* Events can be grouped in namespaces using `:`.
* When namespaced event is fired, it additionally fires all callbacks for that namespace.
*
* // myEmitter.on( ... ) is a shorthand for myEmitter.listenTo( myEmitter, ... ).
* myEmitter.on( 'myGroup', genericCallback );
* myEmitter.on( 'myGroup:myEvent', specificCallback );
*
* // genericCallback is fired.
* myEmitter.fire( 'myGroup' );
* // both genericCallback and specificCallback are fired.
* myEmitter.fire( 'myGroup:myEvent' );
* // genericCallback is fired even though there are no callbacks for "foo".
* myEmitter.fire( 'myGroup:foo' );
*
* An event callback can {@link module:utils/eventinfo~EventInfo#stop stop the event} and
* set the {@link module:utils/eventinfo~EventInfo#return return value} of the {@link #fire} method.
*
* @method #listenTo
* @param {module:utils/emittermixin~Emitter} emitter The object that fires the event.
* @param {String} event The name of the event.
Expand All @@ -137,7 +105,7 @@ const EmitterMixin = {
* the priority value the sooner the callback will be fired. Events having the same priority are called in the
* order they were added.
*/
listenTo( emitter, event, callback, options ) {
listenTo( emitter, event, callback, options = {} ) {
let emitterInfo, eventCallbacks;

// _listeningTo contains a list of emitters that this object is listening to.
Expand Down Expand Up @@ -180,7 +148,34 @@ const EmitterMixin = {
eventCallbacks.push( callback );

// Finally register the callback to the event.
emitter.on( event, callback, options );
createEventNamespace( emitter, event );
const lists = getCallbacksListsForNamespace( emitter, event );
const priority = priorities.get( options.priority );

const callbackDefinition = {
callback,
priority
};

// Add the callback to all callbacks list.
for ( const callbacks of lists ) {
// Add the callback to the list in the right priority position.
let added = false;

for ( let i = 0; i < callbacks.length; i++ ) {
if ( callbacks[ i ].priority < priority ) {
callbacks.splice( i, 0, callbackDefinition );
added = true;

break;
}
}

// Add at the end, if right place was not found.
if ( !added ) {
callbacks.push( callbackDefinition );
}
}
},

/**
Expand All @@ -189,7 +184,7 @@ const EmitterMixin = {
* * To stop listening to a specific callback.
* * To stop listening to a specific event.
* * To stop listening to all events fired by a specific object.
* * To stop listening to all events fired by all object.
* * To stop listening to all events fired by all objects.
*
* @method #stopListening
* @param {module:utils/emittermixin~Emitter} [emitter] The object to stop listening to. If omitted, stops it for all objects.
Expand All @@ -211,13 +206,14 @@ const EmitterMixin = {

// All params provided. off() that single callback.
if ( callback ) {
emitter.off( event, callback );
removeCallback( emitter, event, callback );
}
// Only `emitter` and `event` provided. off() all callbacks for that event.
else if ( eventCallbacks ) {
while ( ( callback = eventCallbacks.pop() ) ) {
emitter.off( event, callback );
removeCallback( emitter, event, callback );
}

delete emitterInfo.callbacks[ event ];
}
// Only `emitter` provided. off() all events for that emitter.
Expand Down Expand Up @@ -246,7 +242,7 @@ const EmitterMixin = {
* @param {String|module:utils/eventinfo~EventInfo} eventOrInfo The name of the event or `EventInfo` object if event is delegated.
* @param {...*} [args] Additional arguments to be passed to the callbacks.
* @returns {*} By default the method returns `undefined`. However, the return value can be changed by listeners
* through modification of the {@link module:utils/eventinfo~EventInfo#return}'s value (the event info
* through modification of the {@link module:utils/eventinfo~EventInfo#return `evt.return`}'s property (the event info
* is the first param of every callback).
*/
fire( eventOrInfo, ...args ) {
Expand Down Expand Up @@ -277,7 +273,7 @@ const EmitterMixin = {
// Remove the called mark for the next calls.
delete eventInfo.off.called;

this.off( event, callbacks[ i ].callback );
removeCallback( this, event, callbacks[ i ].callback );
}

// Do not execute next callbacks if stop() was called.
Expand Down Expand Up @@ -508,7 +504,6 @@ function createEventNamespace( source, eventName ) {
// Gets an array containing callbacks list for a given event and it's more specific events.
// I.e. if given event is foo:bar and there is also foo:bar:abc event registered, this will
// return callback list of foo:bar and foo:bar:abc (but not foo).
// Returns empty array if given event has not been yet registered.
function getCallbacksListsForNamespace( source, eventName ) {
const eventNode = getEvents( source )[ eventName ];

Expand Down Expand Up @@ -570,6 +565,25 @@ function fireDelegatedEvents( destinations, eventInfo, fireArgs ) {
}
}

// Removes callback from emitter for given event.
//
// @param {module:utils/emittermixin~Emitter} emitter
// @param {String} event
// @param {Function} callback
function removeCallback( emitter, event, callback ) {
const lists = getCallbacksListsForNamespace( emitter, event );

for ( const callbacks of lists ) {
for ( let i = 0; i < callbacks.length; i++ ) {
if ( callbacks[ i ].callback == callback ) {
// Remove the callback from the list (fixing the next index).
callbacks.splice( i, 1 );
i--;
}
}
}
}

/**
* Interface representing classes which mix in {@link module:utils/emittermixin~EmitterMixin}.
*
Expand Down
Loading

0 comments on commit 460d7f4

Please sign in to comment.