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

Fixes to help avoid progressbar jumping around during track load. #392

Merged
merged 1 commit into from
Feb 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
56 changes: 33 additions & 23 deletions packages/core/src/PlayerContextProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import getRepeatStrategy from './utils/getRepeatStrategy';
import convertToNumberWithinIntervalBounds from './utils/convertToNumberWithinIntervalBounds';
import { logError, logWarning } from './utils/console';
import getDisplayText from './utils/getDisplayText';
import parseTimeString from './utils/parseTimeString';
import getInitialDuration from './utils/getInitialDuration';
import { repeatStrategyOptions } from './constants';

function playErrorHandler(err) {
Expand Down Expand Up @@ -90,16 +90,8 @@ function getGoToTrackState({
const isNewTrack = prevState.activeTrackIndex !== index;
const shouldLoadAsNew = Boolean(isNewTrack || shouldForceLoad);
const currentTime = track.startingTime || 0;
let duration = 0;
if (track.duration) {
if (typeof track.duration === 'string') {
duration = parseTimeString(track.duration);
} else {
duration = track.duration;
}
}
return {
duration,
duration: getInitialDuration(track),
activeTrackIndex: index,
trackLoading: shouldLoadAsNew,
mediaCannotPlay: prevState.mediaCannotPlay && !shouldLoadAsNew,
Expand All @@ -123,7 +115,8 @@ export class PlayerContextProvider extends Component {
props.startingTrackIndex,
0
);
if (isPlaylistValid(props.playlist) && props.playlist[activeTrackIndex]) {
const playlistIsValid = isPlaylistValid(props.playlist);
if (playlistIsValid && props.playlist[activeTrackIndex]) {
currentTime = props.playlist[activeTrackIndex].startingTime || 0;
}
const { initialStateSnapshot } = props;
Expand Down Expand Up @@ -165,9 +158,12 @@ export class PlayerContextProvider extends Component {
// true if user is currently dragging mouse to change the volume
setVolumeInProgress: false,
// initialize shouldRequestPlayOnNextUpdate from autoplay prop
shouldRequestPlayOnNextUpdate:
props.autoplay && isPlaylistValid(props.playlist),
shouldRequestPlayOnNextUpdate: props.autoplay && playlistIsValid,
awaitingForceLoad: false,
// duration might be set on track object
duration: getInitialDuration(
playlistIsValid && props.playlist[activeTrackIndex]
),
// playlist prop copied to state (for getDerivedStateFromProps)
__playlist__: props.playlist,
// load overrides from previously-captured state snapshot
Expand Down Expand Up @@ -725,10 +721,12 @@ export class PlayerContextProvider extends Component {
handleMediaTimeupdate() {
const { currentTime, played } = this.media;
const { onTimeUpdate, playlist } = this.props;
const { activeTrackIndex } = this.state;
if (this.state.trackLoading) {
// correct currentTime to preset, if applicable, during load
this.media.currentTime = this.state.currentTime;
const { activeTrackIndex, trackLoading } = this.state;
if (trackLoading) {
// we'll get another time update when the track loads
// but for now this helps us avoid unnecessarily
// jumping back to currentTime: 0 in the UI while
// the track is loading.
return;
}
this.setState(state => ({
Expand Down Expand Up @@ -925,9 +923,12 @@ export class PlayerContextProvider extends Component {
...baseStateUpdate,
awaitingResumeOnSeekComplete: paused
? awaitingResumeOnSeekComplete
: true
: true,
currentTime: targetTime
}));
this.media.currentTime = targetTime;
if (!this.state.trackLoading) {
this.media.currentTime = targetTime;
}
if (!this.state.paused) {
this.togglePause(true);
}
Expand All @@ -937,9 +938,12 @@ export class PlayerContextProvider extends Component {
...baseStateUpdate,
awaitingResumeOnSeekComplete: paused
? awaitingResumeOnSeekComplete
: true
: true,
currentTime: targetTime
}));
this.media.currentTime = targetTime;
if (!this.state.trackLoading) {
this.media.currentTime = targetTime;
}
if (this.state.awaitingResumeOnSeekComplete && !this.media.ended) {
// if we earlier encountered an 'ended' state,
// un-pausing becomes necessary to resume playback
Expand All @@ -953,7 +957,11 @@ export class PlayerContextProvider extends Component {
}

seekComplete(targetTime) {
const { seekPreviewTime, awaitingResumeOnSeekComplete } = this.state;
const {
seekPreviewTime,
awaitingResumeOnSeekComplete,
trackLoading
} = this.state;
const baseStateUpdate = {
seekInProgress: false,
awaitingResumeOnSeekComplete: false
Expand All @@ -974,7 +982,9 @@ export class PlayerContextProvider extends Component {
*/
currentTime
});
this.media.currentTime = currentTime;
if (!trackLoading) {
this.media.currentTime = currentTime;
}
if (awaitingResumeOnSeekComplete) {
if (this.media.ended) {
this.forwardSkip();
Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/utils/getInitialDuration.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import parseTimeString from './parseTimeString';

function getInitialDuration(track) {
let duration = 0;
if (track.duration) {
if (typeof track.duration === 'string') {
duration = parseTimeString(track.duration);
} else {
duration = track.duration;
}
}
return duration;
}

export default getInitialDuration;