SRGSSR / srgmediaplayer-apple

An advanced media player library, simple and reliable
MIT License
158 stars 33 forks source link

playerDestructionBlock fix #47

Closed tooolbox closed 6 years ago

tooolbox commented 6 years ago

Fixes #46

This is more of a suggestion than anything; I didn't touch your tests.

Thanks for the great library! 😄

tooolbox commented 6 years ago

Hm, there's definitely some points I don't understand about the code, as well. In my app, the creation & destruction blocks are called each time I play a video, but the code looks like it should only call the creation block if there's no previous player, and only call the destruction block if there's no new player. Neither of which make sense...

Anyway, just commenting on that because I might have gotten some aspects of this PR wrong, but for sure the mediaPlayer's player is nil by the time the destruction block is called, so this is my attempt to handle that.

pyby commented 6 years ago

Hi @tooolbox ,

Thank you for your sharing code.

We'll come back to you about #46.

tooolbox commented 6 years ago
defagos commented 6 years ago

Thanks for your PR. The documentation was clearly incorrect.

Your PR correctly fixes the code so that it matches the documentation. I pushed an additional commit to improve the player lifecycle documentation, which was not clear enough about the fact that the creation and destruction blocks can be called several times over the course of the controller lifetime, as @pyby said. I also updated the tests to take into account the new block parameter.

What could still be discussed is whether the destruction block must be called right before destruction, or after destruction has occurred. I would say that, for the time being, having the destruction block called before the creation block (in the case of a URL switch, for example) is better, most notably if you perform AVAudioSession changes.

We'll discuss the PR with @pyby and merge it accordingly. Thanks for your help!

pyby commented 6 years ago

Thank you @tooolbox for the PR. 🎉

tooolbox commented 6 years ago

Absolutely guys, glad to help. Thank you!