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

Techs can change the player poster only once #4910

Closed
ghost opened this issue Jan 28, 2018 · 4 comments
Closed

Techs can change the player poster only once #4910

ghost opened this issue Jan 28, 2018 · 4 comments

Comments

@ghost
Copy link

ghost commented Jan 28, 2018

Description

New techs are unable to integrate into the poster lifecycle and can switch a poster only once which triggers a posterchange event.

Example

The region of code that seems to be responsible is handleTechPosterChange_

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.

  1. Create a tech that triggers a posterchange event and changes the poster
  2. Implement the poster method on the new tech
  3. Change the poster and trigger a posterchange event

Results

Expected

A new poster

Actual

The same old poster.

Error output

None

Additional Information

Please include any additional information necessary here. Including the following:

versions

videojs

5 + 6, most likely older versions as well

browsers

firefox, chrome,

OSes

tested on Kubuntu 16.04

plugins

None

@gkatsev
Copy link
Member

gkatsev commented Jan 30, 2018

Interesting. That's definitely an unintended side-effect of the code. The idea was that if the user sets a poster the tech shouldn't override it but the way that the code accepts a poster from the tech means that it only gets set once. It totally makes sense for the techs to be able to change their poster as much as possible assuming we don't change the behavior that a user-set poster isn't overridden. Would you be able to and interested in submitting a fix?
Thanks.

@ghost
Copy link
Author

ghost commented Jan 30, 2018

Yeah, sure. I wouldn't mind. There'd be a few things to clear up first, since I see a few possibilities here:

Introduce techs that have the power to override any poster

Could add a static field to the tech that is checked in the posterchange handler e.g

if( (!this.poster_ || this.tech_.canOverridePoster) && this.tech_.poster ){
  this.poster_ = this.tech_.poster();
  this.trigger('posterchange'); 
}

Introduce boolean option in player to allow techs to override the poster

if( (!this.poster_ || this.options_.techCanOverridePoster) && this.tech_.poster ){
  this.poster_ = this.tech_.poster();
  this.trigger('posterchange'); 
}

Track when poster has been set by player.setPoster

This could be done with a boolean in the player.setPoster and setting the poster to an empty string will allow techs to override the poster once again. Maybe setting it to false will indicate that it should remain unset.


All of these might still require to track when the poster was set by the tech, in order to clear the poster when the tech is disposed of.

That's what comes to mind. Thoughts?

@gkatsev
Copy link
Member

gkatsev commented Jan 30, 2018

I'd be inclined to leave out techs that can override the poster for now, at least until we see people asking for that. Unless you're asking for it? :)
Having an option like techCanOverridePoster sounds good to me. Thoughts @videojs/core-committers?
And yeah, you're right, we probably need to track whether the poster was set via the tech or by the user.

@ldayananda
Copy link
Contributor

The option techCanOverridePoster seems like the best choice to me

@gkatsev gkatsev closed this as completed in 8706941 Mar 7, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants