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

adding a currentType method to get current source type if known #1320

Merged
merged 1 commit into from Jul 29, 2014

Conversation

Projects
None yet
3 participants
@mattosborn
Contributor

mattosborn commented Jul 1, 2014

Alternative solution for the problem posed in #1313. See that PR for discussion.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 1, 2014

Member

Unsetting srcType_ when a src without a type was passed in is missing.

Member

gkatsev commented Jul 1, 2014

Unsetting srcType_ when a src without a type was passed in is missing.

@heff

View changes

Show outdated Hide outdated src/js/player.js
@@ -1172,6 +1178,10 @@ vjs.Player.prototype.currentSrc = function(){
return this.techGet('currentSrc') || this.cache_.src || '';
};
vjs.Player.prototype.currentType = function(){
return this.srcType_ || '';

This comment has been minimized.

@heff

heff Jul 1, 2014

Member

Use currentType_ instead of srcType_ for the private property. That's how other properties are set up.

@heff

heff Jul 1, 2014

Member

Use currentType_ instead of srcType_ for the private property. That's how other properties are set up.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 1, 2014

Member

Great stuff, thanks Matt! Still working on a few more inline comments.

Member

heff commented Jul 1, 2014

Great stuff, thanks Matt! Still working on a few more inline comments.

@heff

View changes

Show outdated Hide outdated src/js/player.js
@@ -1144,16 +1156,10 @@ vjs.Player.prototype.src = function(source){
if (!this.isReady_) {

This comment has been minimized.

@heff

heff Jul 1, 2014

Member

I think this ready check will need to happen before anytime we set source. Could probably move it into setSource?

@heff

heff Jul 1, 2014

Member

I think this ready check will need to happen before anytime we set source. Could probably move it into setSource?

@heff

View changes

Show outdated Hide outdated src/js/player.js
if (this.options_['autoplay']) {
this.play();
}
this.setSource(source);

This comment has been minimized.

@heff

heff Jul 1, 2014

Member

So what might be easiest here would be to take the string and create a source object out of it and call src again. That will then reset currentType too.

else if (typeof source === 'string') {
  this.src({ src: source, type: '' });
}
@heff

heff Jul 1, 2014

Member

So what might be easiest here would be to take the string and create a source object out of it and call src again. That will then reset currentType too.

else if (typeof source === 'string') {
  this.src({ src: source, type: '' });
}

@heff heff added confirmed labels Jul 1, 2014

@mattosborn

This comment has been minimized.

Show comment
Hide comment
@mattosborn

mattosborn Jul 2, 2014

Contributor

@gkatsev passing no argument results in srcType_ === undefined

Contributor

mattosborn commented Jul 2, 2014

@gkatsev passing no argument results in srcType_ === undefined

@mattosborn

This comment has been minimized.

Show comment
Hide comment
@mattosborn

mattosborn Jul 2, 2014

Contributor

Thanks @heff - have pushed some more changes based on your comments.

Contributor

mattosborn commented Jul 2, 2014

Thanks @heff - have pushed some more changes based on your comments.

@heff

View changes

Show outdated Hide outdated src/js/player.js
if (this.options_['autoplay']) {
this.play();
}
}).bind(this);

This comment has been minimized.

@heff

heff Jul 3, 2014

Member

ie8 unfortunately doesn't support bind. There's a vjs.bind method in lib.js though.

@heff

heff Jul 3, 2014

Member

ie8 unfortunately doesn't support bind. There's a vjs.bind method in lib.js though.

@heff

View changes

Show outdated Hide outdated src/js/player.js
}
}).bind(this);
this.isReady_ ? _set() : this.ready(_set);

This comment has been minimized.

@heff

heff Jul 3, 2014

Member

I just realized the isReady check here (which existed previously) is redundant. It's doing the same as the ready function in Component, so you could just always call the ready function, and that might simplify this a little.

@heff

heff Jul 3, 2014

Member

I just realized the isReady check here (which existed previously) is redundant. It's doing the same as the ready function in Component, so you could just always call the ready function, and that might simplify this a little.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 3, 2014

Member

This is looking good. I'll be able to get it pulled in next week.

Member

heff commented Jul 3, 2014

This is looking good. I'll be able to get it pulled in next week.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 29, 2014

Member

Made a PR against this branch here: https://github.com/guardian/video.js/pull/1

Member

heff commented Jul 29, 2014

Made a PR against this branch here: https://github.com/guardian/video.js/pull/1

@heff heff merged commit 11524ca into videojs:master Jul 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

heff added a commit that referenced this pull request Jul 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment