Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register multiple event listeners with the same callback in one call #1231

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7eb3886
Multiple event register:
themicp May 22, 2014
93fa94f
Merge branch 'master' into feature/multiple-event-register
themicp May 22, 2014
4ab3378
Create a generic function that parses the array of types given in vjs…
themicp May 22, 2014
3817a24
Use .forEach instead of .filter to pass through each item of the even…
themicp May 22, 2014
a07f627
Export vjs.on, vjs.off, vjs.one so we can access them using bracket n…
themicp May 22, 2014
9b22207
Include comments for vjs.forwardMultipleEvents, vjs.arrayForEach
themicp May 22, 2014
a10b54a
Optimize for() loop in arrayForEach -- keep the array length in a var…
themicp May 22, 2014
2f3c2b3
Merge branch 'master' into feature/multiple-event-register
themicp Jun 17, 2014
0648c0a
Created the vjs.arr object which contains all the array functions,
themicp Jun 17, 2014
0fd43f2
Code fixes in vjs.on, vjs.one and vjs.off,
themicp Jun 17, 2014
9fc5f3b
Coding style fixes
themicp Jun 23, 2014
1448638
Updated contrib install process
heff Jun 24, 2014
fc4d00c
Condensed the contrib commands
heff Jun 25, 2014
191574a
Merge branch 'feature/multiple-event-register' of github.com:themicp/…
heff Jun 25, 2014
a1dec35
Moved the multiple events function off the global and renamed it slig…
heff Jun 25, 2014
57699db
Updated arr.forEach to more closely match the lodash API
heff Jun 25, 2014
0954704
Added a test for the default context of arr.forEach
heff Jul 1, 2014
b76ea0d
Merge pull request #1 from heff/review-themicp-feature/multiple-event…
themicp Jul 2, 2014
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
33 changes: 30 additions & 3 deletions src/js/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,35 @@
* robust as jquery's, so there's probably some differences.
*/

/**
* Loops through an array of event types and calls the requested method for each type.
* @param {Element|Object} elem Element or object to bind listeners to
* @param {String} type Type of event to bind to.
* @param {Function} fn Event listener.
* @param {String} method Event method (on, off, one).
*/
vjs.forwardMultipleEvents = function( elem, type, fn, method ) {
Copy link
Member

Choose a reason for hiding this comment

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

By using the jsDoc comments and adding it to vjs it makes this a public function. I don't think it would get used outside of the core codebase so I think we could keep it private and allow closure compiler to mangle it. To make it public private you could change the name from vjs.forwardMultipleEvents to _forwardMultipleEvents and add the @private tag to the comment.

vjs.arrayForEach( type, function( type ) {
vjs[ method ](elem, type, fn); //Call the event method for each one of the types
});
};

/**
* Add an event listener to element
* It stores the handler function in a separate cache object
* and adds a generic handler to the element's event,
* along with a unique id (guid) to the element.
* @param {Element|Object} elem Element or object to bind listeners to
* @param {String} type Type of event to bind to.
* @param {String|Array} type Type of event to bind to.
* @param {Function} fn Event listener.
* @private
Copy link
Member

Choose a reason for hiding this comment

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

If we export these we need to remove these @private tags.

*/
vjs.on = function(elem, type, fn){
if (type instanceof Array){
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to pull in #1218 soon which adds a vjs.obj.isArray method, so we'll probably want to update this when that happens.

vjs.forwardMultipleEvents(elem, type, fn, 'on');
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Here I would just do return; instead of return false; since undefined is what gets returned otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

actually, you could just do return vjs.forwardMultipleEvents(elem, type, fn, 'on');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, noted.

}

var data = vjs.getData(elem);

// We need a place to store all our handler data
Expand Down Expand Up @@ -64,7 +82,7 @@ vjs.on = function(elem, type, fn){
/**
* Removes event listeners from an element
* @param {Element|Object} elem Object to remove listeners from
* @param {String=} type Type of listener to remove. Don't include to remove all events from element.
* @param {String|Array=} type Type of listener to remove. Don't include to remove all events from element.
* @param {Function} fn Specific listener to remove. Don't incldue to remove listeners for an event type.
* @private
*/
Expand All @@ -77,6 +95,11 @@ vjs.off = function(elem, type, fn) {
// If no events exist, nothing to unbind
if (!data.handlers) { return; }

if (type instanceof Array){
vjs.forwardMultipleEvents(elem, type, fn, 'off');
return false;
}

// Utility function
var removeType = function(t){
data.handlers[t] = [];
Expand Down Expand Up @@ -337,11 +360,15 @@ vjs.trigger = function(elem, event) {
/**
* Trigger a listener only once for an event
* @param {Element|Object} elem Element or object to
* @param {String} type
* @param {String|Array} type
* @param {Function} fn
* @private
*/
vjs.one = function(elem, type, fn) {
if (type instanceof Array){
vjs.forwardMultipleEvents(elem, type, fn, 'one');
return false;
}
var func = function(){
vjs.off(elem, type, func);
fn.apply(this, arguments);
Expand Down
4 changes: 4 additions & 0 deletions src/js/exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ goog.exportSymbol('videojs.SubtitlesButton', vjs.SubtitlesButton);
goog.exportSymbol('videojs.CaptionsButton', vjs.CaptionsButton);
goog.exportSymbol('videojs.ChaptersButton', vjs.ChaptersButton);

goog.exportSymbol('videojs.on', vjs.on);
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a specific use case where you would like to use these outside of the core codebase?

So far we've avoided exposing too many top-level utility functions like this because we're not trying to make a library like jquery or lodash, but the event functions are pretty powerful so I could see exporting these as long as we can point to a reason why we're doing it.

If we do export these, we'll need to remove the @private tags from the related comments.

Copy link
Member

Choose a reason for hiding this comment

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

One usecase for exporting it, is to allow videojs plugins the ability to use this so that they wont need to create their own cross-platform/browser solutions.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, plugins have added a new issue to the whole utility lib argument. When you're just extending components you have access to most of these functions, e.g. Component.prototype.on(), but general plugins don't get that.

It's probably worth opening another issue to discuss these things more deeply.

  • what are the positives and negatives of exposing utility functions?
  • if we don't provide util functions, what other options do devs have?
  • are there certain classes of util functions that are more valuable than others, or harder to get externally?
    I'll open an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I think if vjs has these utility methods, it should expose them to plugins. Why force plugin developers to rewrite stuff that's already available?
Totally agree that we should continue the discussion elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I exported those functions because I couldn't access them from forwardMultipleEvents using bracket notation since the compiler was renaming them. I can probably change the code a bit to remove the exports and use a switch() on the method parameter, something like this:

switch(method){
  case 'on': 
    vjs.on( .... );
    break;
  case 'one': 
    vjs.one( .... );
    break;
  case 'off': 
    vjs.on( .... );
    break;
  default:
    break;
}

Copy link
Member

Choose a reason for hiding this comment

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

Why not just give forwardMultipleEvents the function you want it to use?
So, the new method signature would be function forwardMultipleEvents(fn, event, type, callback). Then in vjs.on you'd do vjs.forwardMultipleEvents(vjs.on, event, type, fn)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks! That's how I'm going to do it.

goog.exportSymbol('videojs.off', vjs.off);
goog.exportSymbol('videojs.one', vjs.one);

goog.exportSymbol('videojs.MediaTechController', vjs.MediaTechController);
goog.exportProperty(vjs.MediaTechController.prototype, 'features', vjs.MediaTechController.prototype.features);
goog.exportProperty(vjs.MediaTechController.prototype.features, 'volumeControl', vjs.MediaTechController.prototype.features.volumeControl);
Expand Down
15 changes: 15 additions & 0 deletions src/js/lib.js
Original file line number Diff line number Diff line change
Expand Up @@ -832,3 +832,18 @@ vjs.findPosition = function(el) {
top: vjs.round(top)
};
};

/*
* Loops through an array and runs a function for each item inside it.
* @param {Array} array The array.
* @param {Function} fn The function to be run for each item.
*/
vjs.arrayForEach = function(array, fn) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this vjs.arr.forEach to match the vjs.obj methods? You'll need to create the vjs.arr object as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea I will update it soon.

if (!(array instanceof Array) || !(fn instanceof Function)) {
return false;
}

for (var i = 0, len = array.length; i < len; ++i) {
fn.apply( vjs, [array[i], i, array]);
Copy link
Member

Choose a reason for hiding this comment

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

You could use fn.call() here an avoid making the extra array. Not sure if there's really any benefit to that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be helpful for other uses of arrayForEach() to set the context to vjs. Maybe I could change that for the sake of simplicity.

}
};
67 changes: 67 additions & 0 deletions test/unit/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,32 @@ test('should add and remove an event listener to an element', function(){
vjs.trigger(el, 'click'); // No click should happen.
});

test('should add and remove multiple event listeners to an element with a single call', function(){
Copy link
Member

Choose a reason for hiding this comment

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

great tests!

expect(6);

var el = document.createElement('div');
var listener = function(){
ok(true, 'Callback triggered');
};

vjs.on(el, [ 'click', 'event1', 'event2' ], listener);

vjs.trigger(el, 'click');
vjs.trigger(el, 'click');
vjs.off(el, 'click', listener);
vjs.trigger(el, 'click'); // No click should happen.

vjs.trigger(el, 'event1');
vjs.trigger(el, 'event1');
vjs.off(el, 'event1', listener);
vjs.trigger(el, 'event1'); // No event1 should happen.

vjs.trigger(el, 'event2');
vjs.trigger(el, 'event2');
vjs.off(el, 'event2', listener);
vjs.trigger(el, 'event2'); // No event2 should happen.
});

test('should remove all listeners of a type', function(){
var el = document.createElement('div');
var clicks = 0;
Expand All @@ -36,6 +62,30 @@ test('should remove all listeners of a type', function(){
ok(clicks === 2, 'no click listeners fired');
});

test('should remove all listeners of an array of types', function(){
var el = document.createElement('div');
var calls = 0;
var listener = function(){
calls++;
};
var listener2 = function(){
calls++;
};

vjs.on(el, ['click', 'event1'], listener);
vjs.on(el, ['click', 'event1'], listener2);
vjs.trigger(el, 'click'); // 2 calls
vjs.trigger(el, 'event1'); // 2 calls

ok(calls === 4, 'both click listeners fired');

vjs.off(el, ['click', 'event1']);
vjs.trigger(el, 'click'); // No click should happen.
vjs.trigger(el, 'event1'); // No event1 should happen.

ok(calls === 4, 'no event listeners fired');
});

test('should remove all listeners from an element', function(){
expect(2);

Expand Down Expand Up @@ -73,6 +123,23 @@ test('should listen only once', function(){
vjs.trigger(el, 'click'); // No click should happen.
});

test( 'should listen only once in multiple events from a single call', function(){
expect(3);

var el = document.createElement('div');
var listener = function(){
ok(true, 'Callback Triggered');
};

vjs.one(el, [ 'click', 'event1', 'event2' ], listener);
vjs.trigger(el, 'click'); // 1 click
vjs.trigger(el, 'click'); // No click should happen.
vjs.trigger(el, 'event1'); // event1 must be handled.
vjs.trigger(el, 'event1'); // No event1 should be handled.
vjs.trigger(el, 'event2'); // event2 must be handled.
vjs.trigger(el, 'event2'); // No event2 should be handled.
});

test('should stop immediate propagtion', function(){
expect(1);

Expand Down
13 changes: 13 additions & 0 deletions test/unit/lib.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,3 +324,16 @@ test('should confirm logging functions work', function() {
console.error = origError;
}
});

test('should loop through each element of an array', function() {
expect(11);
var a = [1, 2, 3, 4, 5];
var sum = 0;
var i = 0;
vjs.arrayForEach(a, function(item, iterator, array) {
sum += item;
deepEqual(array, a);
equal(i++,iterator);
} );
ok(sum, 15);
} );