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.

Sign up for GitHub

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

Feature/current source #2678

Merged
merged 3 commits into from
Nov 3, 2016
Merged

Feature/current source #2678

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be8341d
@chemoish add the ability to obtain current source object + current s…
Aug 26, 2016
5248d86
@chemoish when the src redefines the current source, with a single so…
Aug 26, 2016
3a4b293
refactor(player): Update `currentSources` to return `[]` and `current…
chemoish Sep 22, 2016
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
28 changes: 28 additions & 0 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,10 +614,13 @@ class Player extends Component {

if (source) {
this.currentType_ = source.type;

if (source.src === this.cache_.src && this.cache_.currentTime > 0) {
techOptions.startTime = this.cache_.currentTime;
}

this.cache_.sources = null;
this.cache_.source = source;
this.cache_.src = source.src;
}

Expand Down Expand Up @@ -1975,7 +1978,10 @@ class Player extends Component {
// the tech loop to check for a compatible technology
this.sourceList_([source]);
} else {
this.cache_.sources = null;
this.cache_.source = source;
this.cache_.src = source.src;

this.currentType_ = source.type || '';

// wait until the tech is ready to set the source
Expand Down Expand Up @@ -2025,6 +2031,8 @@ class Player extends Component {
// load this technology with the chosen source
this.loadTech_(sourceTech.tech, sourceTech.source);
}

this.cache_.sources = sources;
} else {
// We need to wrap this in a timeout to give folks a chance to add error event handlers
this.setTimeout(function() {
Expand Down Expand Up @@ -2061,6 +2069,26 @@ class Player extends Component {
return this;
}

/**
* Returns the current source objects.
*
* @return {Object[]} The current source objects
* @method currentSources
*/
currentSources() {
Copy link
Member Author

Choose a reason for hiding this comment

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

updated to return [] when src is ''

return this.cache_.sources || [this.currentSource()];
}

/**
* Returns the current source object.
*
* @return {Object} The current source object
* @method currentSource
*/
currentSource() {
Copy link
Member Author

Choose a reason for hiding this comment

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

updated to return {} when src is ''

return this.cache_.source || { src: this.currentSrc() };
Copy link
Member Author

@chemoish chemoish Aug 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

if this.currentSrc() returns undefined should this method return {} instead?

https://github.com/videojs/video.js/compare/master...gesinger:feature/current-source-obj?expand=1#diff-3b0266ff1c037b289ec624ab25b0272eR2092

Copy link
Member

Choose a reason for hiding this comment

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

That sounds fine to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial thought is to throw away all the code for currentSrc() since now is very duplicative and loses information such as type. Though this is a breaking change and there is probably a big hole in my logic.

Atm, currentSrc() returns a string or undefined…

I think it makes sense to translate that across those methods.

currentSrc() === undefined
currentSource() === undefined
currentSources() === undefined

vs.

currentSrc() === undefined
currentSource() === { src: undefined } or {}
currentSources() === [{ src: undefined }] or []

So I would recommend undefined or {} and [].

Copy link
Member

Choose a reason for hiding this comment

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

I lean toward all three methods returning undefined if there is no source.

My second preference would be {} or [] for the two new ones, but I feel like in that case, it would make more sense for currentSrc to return '' instead of undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, undefined would be not backwards compatible but also would break away from what the video element does for currentSrc:

document.createElement('video').currentSrc
// ""

As for the others, returning an empty object and empty array would make it consistent. Though, would be a bit annoying to check for whether it's empty or not.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! I thought currentSrc returned undefined. In that case, I vote for currentSource to return undefined and currentSources to return [].

Copy link
Member

Choose a reason for hiding this comment

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

Having currentSource return undefined seems inconsistent to me. Both from it's internal type signature but also between the 3 methods.
currentSrc always returns a string and currentSources always returns an array, why should currentSource be different?

}

/**
* Returns the fully qualified URL of the current source value e.g. http://mysite.com/video.mp4
* Can be used in conjuction with `currentType` to assist in rebuilding the current source object.
Expand Down
130 changes: 130 additions & 0 deletions test/unit/player.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,136 @@ QUnit.test('should get tag, source, and track settings', function(assert) {
assert.equal(player.el(), null, 'player el killed');
});

QUnit.test('should get current source from source tag', function(assert) {
const fixture = document.getElementById('qunit-fixture');

const html = [
'<video id="example_1" class="video-js" preload="none">',
'<source src="http://google.com" type="video/mp4">',
'<source src="http://hugo.com" type="video/webm">',
'</video>'
].join('');

fixture.innerHTML += html;

const tag = document.getElementById('example_1');
const player = TestHelpers.makePlayer({}, tag);

assert.ok(player.currentSource().src === 'http://google.com');
assert.ok(player.currentSource().type === 'video/mp4');
});

QUnit.test('should get current sources from source tag', function(assert) {
const fixture = document.getElementById('qunit-fixture');

const html = [
'<video id="example_1" class="video-js" preload="none">',
'<source src="http://google.com" type="video/mp4">',
'<source src="http://hugo.com" type="video/webm">',
'</video>'
].join('');

fixture.innerHTML += html;

const tag = document.getElementById('example_1');
const player = TestHelpers.makePlayer({}, tag);

assert.ok(player.currentSources()[0].src === 'http://google.com');
assert.ok(player.currentSources()[0].type === 'video/mp4');
assert.ok(player.currentSources()[1].src === 'http://hugo.com');
assert.ok(player.currentSources()[1].type === 'video/webm');

// when redefining src expect sources to update accordingly
player.src('http://google.com');

assert.ok(player.currentSources()[0].src === 'http://google.com');
assert.ok(player.currentSources()[0].type === undefined);
assert.ok(player.currentSources()[1] === undefined);
});

QUnit.test('should get current source from src set', function(assert) {
const fixture = document.getElementById('qunit-fixture');

const html = '<video id="example_1" class="video-js" preload="none"></video>';

fixture.innerHTML += html;

const tag = document.getElementById('example_1');
const player = TestHelpers.makePlayer({}, tag);

player.loadTech_('Html5');

// check for matching undefined src
assert.ok(player.currentSource().src === player.currentSrc());

player.src('http://google.com');

assert.ok(player.currentSource().src === 'http://google.com');
assert.ok(player.currentSource().type === undefined);

player.src({
src: 'http://google.com'
});

assert.ok(player.currentSource().src === 'http://google.com');
assert.ok(player.currentSource().type === undefined);

player.src({
src: 'http://google.com',
type: 'video/mp4'
});

assert.ok(player.currentSource().src === 'http://google.com');
assert.ok(player.currentSource().type === 'video/mp4');
});

QUnit.test('should get current sources from src set', function(assert) {
const fixture = document.getElementById('qunit-fixture');

const html = '<video id="example_1" class="video-js" preload="none"></video>';

fixture.innerHTML += html;

const tag = document.getElementById('example_1');
const player = TestHelpers.makePlayer({}, tag);

player.loadTech_('Html5');

// check for matching undefined src
assert.ok(player.currentSource().src === player.currentSrc());

player.src([{
src: 'http://google.com'
}, {
src: 'http://hugo.com'
}]);

assert.ok(player.currentSources()[0].src === 'http://google.com');
assert.ok(player.currentSources()[0].type === undefined);
assert.ok(player.currentSources()[1].src === 'http://hugo.com');
assert.ok(player.currentSources()[1].type === undefined);

player.src([{
src: 'http://google.com',
type: 'video/mp4'
}, {
src: 'http://hugo.com',
type: 'video/webm'
}]);

assert.ok(player.currentSources()[0].src === 'http://google.com');
assert.ok(player.currentSources()[0].type === 'video/mp4');
assert.ok(player.currentSources()[1].src === 'http://hugo.com');
assert.ok(player.currentSources()[1].type === 'video/webm');

// when redefining src expect sources to update accordingly
player.src('http://hugo.com');

assert.ok(player.currentSources()[0].src === 'http://hugo.com');
assert.ok(player.currentSources()[0].type === undefined);
assert.ok(player.currentSources()[1] === undefined);
});

QUnit.test('should asynchronously fire error events during source selection', function(assert) {
assert.expect(2);

Expand Down