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

fix: make sure sources, volume, and playback rate are reset along with the player #5676

Merged
merged 3 commits into from
Jan 3, 2019

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented Dec 12, 2018

Fixes #5675

This PR will fully clear the player.cache_ when calling Player#reset.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

assert.deepEqual(player.currentSource(), {}, 'currentSource is correct');
assert.deepEqual(player.currentSources(), [], 'currentSources is correct');
assert.strictEqual(player.playbackRate(), 1, 'playbackRate is correct');
assert.strictEqual(player.volume(), 0, 'volume is correct');
Copy link
Member Author

Choose a reason for hiding this comment

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

This is 0 because of TechFaker's default.

@misteroneill misteroneill changed the title fix: make sure sources, volume, and playback rate are reset along wit… fix: make sure sources, volume, and playback rate are reset along with the player Dec 12, 2018
resetCache_() {
this.cache_ = {
lastVolume: 1,
lastPlaybackRate: this.defaultPlaybackRate(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set currentTime, duration, selectedLanguage to null/empty string here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left those out because it appeared they got set in their getter/setters, but it probably makes sense to set a default for everything here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out setting selectedLanguage in here has all kinds of side effects. Probably best left to another PR.

@gkatsev gkatsev mentioned this pull request Dec 18, 2018
4 tasks
@gkatsev gkatsev added the patch This PR can be added to a patch release. label Dec 18, 2018
@gkatsev gkatsev merged commit 4c9e09d into master Jan 3, 2019
@gkatsev gkatsev deleted the fix/player-reset branch January 3, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Player#reset does not dispose cached sources.
3 participants