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

T/144: Refactor EmitterMixin methods. #190

Merged
merged 20 commits into from
Dec 11, 2017
Merged

T/144: Refactor EmitterMixin methods. #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
08c64ed
Other: Make `EmitterMixin.listenTo` the main method instead of `Emitt…
jodator Oct 17, 2017
9201e95
Fix: `DomEmitterMixin` works as before after refactoring `EmitterMixi…
jodator Oct 17, 2017
70aca99
Tests: Update coverage in DomEmitterMixin tests.
jodator Oct 17, 2017
2a9a431
Other: Make `EmitterMixin.off()` use `stopListening()` internally.
jodator Oct 18, 2017
450829b
Code style: Rename private method to `removeCallback` in EmitterMixin.
jodator Oct 18, 2017
9d0a033
Docs: Fix license banner in emittermixin.js.
jodator Oct 24, 2017
3e0964e
Other: Use spread operator for passed arguments in `DomEmitterMixin.l…
jodator Oct 24, 2017
9414e9b
Other: Remove isDomNode function that overlaps import in emittermixin.
jodator Nov 9, 2017
4cf7e4a
Tests: Ignore safety check in emittermixin's private method.
jodator Nov 9, 2017
bb5a85f
Improved API docs.
Reinmar Nov 10, 2017
f1a92c1
Fixed broken license header.
Reinmar Nov 10, 2017
86c8efc
Even more API docs fixes.
Reinmar Nov 10, 2017
4e3c1e9
Removed unncessary code and added missing tests.
Reinmar Nov 11, 2017
0ec921c
Tests: Added more tests for namespaced events.
jodator Nov 13, 2017
d3dbefb
Docs: Fixed links to dom/ProxyEmitter methods.
jodator Nov 14, 2017
57feb2b
Tests: Added more test cases for dom/EmitterMixin.
jodator Nov 14, 2017
809f249
Tests: Remove failing tests as not a part of an API yet.
jodator Nov 17, 2017
beedb07
Tests: Add tests for current `once()` behavior.
jodator Nov 17, 2017
ceaaff8
Merge branch 'master' into t/144-new
Reinmar Dec 11, 2017
cd21a75
Docs: Minor improvement.
Reinmar Dec 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 23 additions & 35 deletions src/dom/emittermixin.js
View file
Open in desktop
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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS, event can be undefined. Which means calling emitter.detach() but its first param is not optional. Could you verify that it all works fine in cases where stopListening()'s params are not passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean besides tests? The event parameter is check by the EmitterMixin.stopListening() and also in here dom/EmitterMixinProxy.detach(): https://github.com/ckeditor/ckeditor5-utils/pull/190/files/86c8efc9cff0415a01cdd3cd2a29aef81e1aadce#diff-99d3cd5d9460b6839252663e7c38b1c6R218 - actually it is checked if given event is defined in _domListeners so if it is falsy then detach will do nothing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's handled very poorly there, because if event is undefined then we do listeners[ undefined ]. And the API docs of detach() are also incorrect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words – if we expect event to be undefined then it should be handled explicitly. However, it's not the problem of your PR – it was like that in the past.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it seems to be a bug: https://github.com/ckeditor/ckeditor5-utils/issues/199

}
},

/**
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
Open in desktop
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