Skip to content

Commit

Permalink
Use explicit isReady property to guard player methods
Browse files Browse the repository at this point in the history 
Fixes #18
  • Loading branch information
@cookpete
cookpete committed Dec 28, 2015
1 parent 0e24bf4 commit ef17dcc
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 17 deletions.
2 changes: 2 additions & 0 deletions src/players/Base.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ export default class Base extends Component {
shouldComponentUpdate (nextProps) {
return this.props.url !== nextProps.url
}
isReady = false
onReady = () => {
this.setVolume(this.props.volume)
if (this.props.playing || this.preloading) {
this.preloading = false
this.isReady = true
this.play()
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/players/FilePlayer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ export default class FilePlayer extends Base {
this.player.volume = fraction
}
getFractionPlayed () {
if (this.player.readyState === 0) return 0
if (!this.isReady) return 0
return this.player.currentTime / this.player.duration
}
getFractionLoaded () {
if (this.player.readyState === 0) return 0
if (!this.isReady) return 0
return this.player.buffered.end(0) / this.player.duration
}
render () {
Expand Down
14 changes: 7 additions & 7 deletions src/players/SoundCloud.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,31 +74,31 @@ export default class SoundCloud extends Base {
ondataerror: this.props.onError
}
play () {
if (!this.player) return
if (!this.isReady) return
this.player.play()
}
pause () {
if (!this.player) return
if (!this.isReady) return
this.player.pause()
}
stop () {
if (!this.player) return
if (!this.isReady) return
this.player.stop()
}
seekTo (fraction) {
if (!this.player) return
if (!this.isReady) return
this.player.seek(this.player.getDuration() * fraction)
}
setVolume (fraction) {
if (!this.player) return
if (!this.isReady) return
this.player.setVolume(fraction)
}
getFractionPlayed () {
if (!this.player) return 0
if (!this.isReady) return 0
return this.player.getCurrentPosition() / this.player.getDuration()
}
getFractionLoaded () {
if (!this.player) return 0
if (!this.isReady) return 0
return this.player.getLoadedPosition() / this.player.getDuration()
}
render () {
Expand Down
16 changes: 8 additions & 8 deletions src/players/YouTube.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export default class YouTube extends Base {
}
load (url, playing) {
const id = url && url.match(MATCH_URL)[1]
if (this.player) {
if (this.isReady) {
this.stop()
if (playing) {
this.player.loadVideoById(id)
Expand Down Expand Up @@ -77,31 +77,31 @@ export default class YouTube extends Base {
if (state.data === YT.PlayerState.ENDED) this.props.onEnded()
}
play () {
if (!this.player) return
if (!this.isReady) return
this.player.playVideo()
}
pause () {
if (!this.player) return
if (!this.isReady) return
this.player.pauseVideo()
}
stop () {
if (!this.player) return
if (!this.isReady) return
this.player.stopVideo()
}
seekTo (fraction) {
if (!this.player) return
if (!this.isReady) return
this.player.seekTo(this.player.getDuration() * fraction)
}
setVolume (fraction) {
if (!this.player) return
if (!this.isReady) return
this.player.setVolume(fraction * 100)
}
getFractionPlayed () {
if (!this.player || !this.player.getCurrentTime) return 0
if (!this.isReady) return 0
return this.player.getCurrentTime() / this.player.getDuration()
}
getFractionLoaded () {
if (!this.player || !this.player.getVideoLoadedFraction) return 0
if (!this.isReady) return 0
return this.player.getVideoLoadedFraction()
}
render () {
Expand Down

11 comments on commit ef17dcc

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

geordi_thumb

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

The next step is to queue these method calls and execute them once the SDK ready callback fires. Maybe we could wrap everything in a new this.onReady method?

setVolume (fraction) {
  this.onReady(() => {
    this.player.setVolume(fraction * 100)
  })
}

Right now I'm thinking my initial calls to setVolume will fail, and the player will initialize with the incorrect volume set.

@cookpete
Copy link
Owner Author

Choose a reason for hiding this comment

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

Volume should be fine, as the volume is already set when players become ready, but this is not the case for seeking. Perhaps we could set this.seekOnReady in the method if !this.isReady, then when onReady is called, check to see if seekOnReady is set, and seekTo if so..

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it gets a little weird. I'm not sure i'd always want the command to go through if the SDK loads like ~2 seconds later...

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

This still seems to be an issue. this.player.playVideo isn't available after the onReady for some reason. I'll keep looking into it.

@cookpete
Copy link
Owner Author

Choose a reason for hiding this comment

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

this.player.playVideo isn't available after the onReady for some reason.

I'd be interested to know how this is happening considering it should be the player itself that causes onReady to fire 😕

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

This only happens when I immediately load a video and try to play it. Maybe there's some kind of race condition where the API methods don't exist while a video is being loaded?

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

All I really know is, inside onReady this.player.playVideo is a function, but then when play is executed, it isn't. this.player is still a YouTube player API instance, but the method just doesn't exist at that exact moment.

@Fauntleroy
Copy link
Contributor

Choose a reason for hiding this comment

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

For now I'm just going to make a custom build of this that sanity checks the method before attempting to call it. I'm not sure how you want to handle this in the official build, though.

@cookpete
Copy link
Owner Author

Choose a reason for hiding this comment

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

Happy to sanity check methods for now. I'll publish a patch and create a issue to follow up.

@cookpete
Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed in v0.3.3 and I've opened #20 to follow it up.

Please sign in to comment.