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

Added option to disable watching progress #91

Merged
merged 2 commits into from
Apr 24, 2017

Conversation

forbesjo
Copy link
Contributor

Description

This PR adds a flag to disable watching the progress event. The reason for this is that progress coming from the video element doesn't really mean anything when using MSE.

Specific Changes proposed

Added player.errors.disableProgress() to disable watching progress events. Defaults to false.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
    • Docs/guides updated
  • Reviewed by Two Core Contributors

@mjneil
Copy link
Contributor

mjneil commented Apr 19, 2017

If a user wants to call player.errors(customOptions) to re-initialize the plugin with custom options, I think this would cause progress events to be un-ignored if the plugin was re-initialized after the disableProgress api is called.

@forbesjo
Copy link
Contributor Author

That's true, the new API would also not be available when using a source element.

@misteroneill
Copy link
Member

@mjneil That's correct, but I would say that's a feature not a bug. When you reinitialize this plugin, I think you'd expect whatever options you have provided to be the new options (not merged with the previous invocation).

@forbesjo Not sure what you mean by "when using a source element"...

@mjneil
Copy link
Contributor

mjneil commented Apr 19, 2017

I assume its related to this

@forbesjo
Copy link
Contributor Author

<video id=example-video width=600 height=300 class="video-js vjs-default-skin" controls>
  <source
     src="https://example.com/dash.mpd"
     type="application/dash+xml">
</video>

I'm under the impression that when setting a source this way the source handler cannot create a reference to the player and therefore cannot access its plugins. We saw a similar problem with the qualityLevels plugin

@misteroneill
Copy link
Member

That seems... wrong, but maybe I'm not thinking clearly. Regardless, that doesn't sound like it stops this PR from moving forward.

@forbesjo
Copy link
Contributor Author

It was either using the source element or setting a source as an option through videojs(). Either way this a niche setting that will be done the proper way eventually with middleware

@@ -258,6 +262,10 @@ const initPlugin = function(player, options) {
options.errors = videojs.mergeOptions(options.errors, errors);
};

reInitPlugin.disableProgress = function(disabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we call onPlayStartMonitor in here? That'll cleanup the current monitors, specifically, the progress monitor, and then re-adds the listeners.

@forbesjo forbesjo merged commit 387e4ca into videojs:master Apr 24, 2017
@forbesjo forbesjo deleted the feature/disable-progress branch April 24, 2017 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants