Skip to content

Commit

Permalink
Updated the ready event to always be async
Browse files Browse the repository at this point in the history
closes #1667

Fix text track tests.
Now that ready is async, we need to tick the clock so the ready handler
fires.

Remove unneeded ready test
  • Loading branch information
heff committed May 23, 2015
1 parent 1ff361a commit a575801
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 15 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ CHANGELOG
* @heff Cleaned up and documented src/js/video.js and DOM functions ([view](https://github.com/videojs/video.js/pull/2182))
* @mmcc Changed to pure CSS slider handles ([view](https://github.com/videojs/video.js/pull/2132))
* @mister-ben updated language support to handle language codes with regions ([view](https://github.com/videojs/video.js/pull/2177))
* @heff changed the 'ready' event to always be asynchronous ([view](https://github.com/videojs/video.js/pull/2188))

--------------------

Expand Down
23 changes: 13 additions & 10 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,8 @@ class Component {
ready(fn) {
if (fn) {
if (this.isReady_) {
fn.call(this);
// Ensure function is always called asynchronously
this.setTimeout(fn, 1);
} else {
this.readyQueue_ = this.readyQueue_ || [];
this.readyQueue_.push(fn);
Expand All @@ -732,20 +733,22 @@ class Component {
triggerReady() {
this.isReady_ = true;

let readyQueue = this.readyQueue_;
// Ensure ready is triggerd asynchronously
this.setTimeout(function(){
let readyQueue = this.readyQueue_;

if (readyQueue && readyQueue.length > 0) {
if (readyQueue && readyQueue.length > 0) {
readyQueue.forEach(function(fn){
fn.call(this);
}, this);

for (let i = 0; i < readyQueue.length; i++) {
readyQueue[i].call(this);
// Reset Ready Queue
this.readyQueue_ = [];
}

// Reset Ready Queue
this.readyQueue_ = [];

// Allow for using event listeners also, in case you want to do something everytime a source is ready.
// Allow for using event listeners also
this.trigger('ready');
}
}, 1);
}

/**
Expand Down
5 changes: 3 additions & 2 deletions test/globals-shim.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import document from 'global/document';
import window from 'global/window';
import sinon from 'sinon';

Expand All @@ -6,5 +7,5 @@ window.sinon = sinon;

// This may not be needed anymore, but double check before removing
window.fixture = document.createElement('div');
fixture.id = 'qunit-fixture';
document.body.appendChild(fixture);
window.fixture.id = 'qunit-fixture';
document.body.appendChild(window.fixture);
3 changes: 3 additions & 0 deletions test/unit/component.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -360,11 +360,14 @@ test('should trigger a listener when ready', function(){
var comp = new Component(getFakePlayer(), {}, optionsReadyListener);

comp.triggerReady();
this.clock.tick(1);

comp.ready(methodReadyListener);
this.clock.tick(1);

// First two listeners should only be fired once and then removed
comp.triggerReady();
this.clock.tick(1);
});

test('should add and remove a CSS class', function(){
Expand Down
23 changes: 20 additions & 3 deletions test/unit/tracks/text-track-controls.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,27 @@ import TextTrackMenuItem from '../../../src/js/control-bar/text-track-controls/t
import TestHelpers from '../test-helpers.js';
import * as browser from '../../../src/js/utils/browser.js';

q.module('Text Track Controls');
q.module('Text Track Controls', {
'setup': function() {
this.clock = sinon.useFakeTimers();
},
'teardown': function() {
this.clock.restore();
}
});

var track = {
kind: 'captions',
label: 'test'
};

test('should be displayed when text tracks list is not empty', function() {
var player = TestHelpers.makePlayer({

This comment has been minimized.

Copy link
@OwenEdwards

OwenEdwards Jul 11, 2015

Member

How come you changed this 'var' to a 'let', but not others in this module?

This comment has been minimized.

Copy link
@heff

heff Jul 14, 2015

Author Member

We haven't done and exhaustive search for var/let changes. It's more of a gradual process, but let is correct in basically all cases.

let player = TestHelpers.makePlayer({
tracks: [track]
});

this.clock.tick(1000);

ok(!player.controlBar.captionsButton.hasClass('vjs-hidden'), 'control is displayed');
equal(player.textTracks().length, 1, 'textTracks contains one item');
});
Expand Down Expand Up @@ -49,7 +58,11 @@ test('menu should contain "Settings", "Off" and one track', function() {
var player = TestHelpers.makePlayer({
tracks: [track]
}),
menuItems = player.controlBar.captionsButton.items;
menuItems;

this.clock.tick(1000);

menuItems = player.controlBar.captionsButton.items;

equal(menuItems.length, 3, 'menu contains three items');
equal(menuItems[0].track.label, 'captions settings', 'menu contains "captions settings"');
Expand All @@ -62,6 +75,8 @@ test('menu should update with addRemoteTextTrack', function() {
tracks: [track]
});

this.clock.tick(1000);

player.addRemoteTextTrack(track);

equal(player.controlBar.captionsButton.items.length, 4, 'menu does contain added track');
Expand All @@ -73,6 +88,8 @@ test('menu should update with removeRemoteTextTrack', function() {
tracks: [track, track]
});

this.clock.tick(1000);

player.removeRemoteTextTrack(player.textTracks()[0]);

equal(player.controlBar.captionsButton.items.length, 3, 'menu does not contain removed track');
Expand Down

2 comments on commit a575801

@OwenEdwards
Copy link
Member

Choose a reason for hiding this comment

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

The addition of this.clock.tick(1000); to fix text track tests seems incomplete; why is it done in test 'menu should update with addRemoteTextTrack', but not in test 'should not be displayed when last text track is removed'? Although this latter test passes, shouldn't it have the clock.tick addition?

@heff
Copy link
Member Author

@heff heff commented on a575801 Jul 14, 2015

Choose a reason for hiding this comment

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

@gkatsev will have to answer this one.

Please sign in to comment.