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

Playback ready event logic issue #74

Open
kslimani opened this issue Jan 29, 2020 · 7 comments · May be fixed by #75
Open

Playback ready event logic issue #74

kslimani opened this issue Jan 29, 2020 · 7 comments · May be fixed by #75

Comments

@kslimani
Copy link
Contributor

I have done some debug with this plugin, to understand why the "CORE_READY" event is not trigger when a Clappr player instance is created.

In the current latest version, it seems that the "CORE_READY" event is trigger only when play() method is called.

I think this is because the PLAYBACK_READY event is trigger in _onShakaReady() method. (this method is called only after shaka instance is ready, and therefore after calling play() method).

It seems to be not consistent with other playbacks.

The first possible solution which come to my mind is to move the "PLAYBACK_READY" trigger in the _ready method which is currently "no-op". But if we do this, some changes will be required in several methods, to add additional checks. (ie: check if shaka instance is created, some getter may return null, etc...)

/ping @leandromoreira @jhonatangomes what do you think ?

@leandromoreira
Copy link
Member

I think it's best to add complexities as you mentioned than not to be complient with the API semantics of core ready. Btw, thanks for the work 🥂

@kslimani kslimani linked a pull request Jan 29, 2020 that will close this issue
@kslimani
Copy link
Contributor Author

I pushed #75 to attempt to fix this issue.

@leandromoreira
Copy link
Member

I'm in favor that we also update shaka to release the new version.

@kslimani
Copy link
Contributor Author

@leandromoreira please note that i added an additionnal commit to fix an issue with Safari on macOS. I explain why in the PR.

@kslimani
Copy link
Contributor Author

@leandromoreira do you want me to also add a commit in the PR to bump Shaka to 2.5.8 ?

@leandromoreira
Copy link
Member

I'm okay with that, what do you think @jhonatangomes ?

@kslimani
Copy link
Contributor Author

kslimani commented Mar 6, 2020

/bump @jhonatangomes 🐢 😁

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 a pull request may close this issue.

2 participants