Major rework to player inheritance to use BaseFragment#2689
Major rework to player inheritance to use BaseFragment#2689Luna712 wants to merge 5 commits intorecloudstream:masterfrom
Conversation
This practically rebuilds the entire player, aimed to remove the FullScreenPlayer inheritance from ResultFragmentPhone, use BaseFragment for ResultFragmentPhone and the player, and make the player easier to maintain and expand in the future, as well overall cleanup to code readability and adding documentation to methods in the player.
|
This is mostly ready for review. Though there may still be a few bugs since I wrote a lot of the code from scratch in many places (some of the code is stuff I worked on ~6 months ago, for about two months but gave up as I ran into some issues I couldn't figure but finally finished it and expanded to all the new features since then now). If you feel this should be done a different way I could do that also, but this was just designed to make it easier in the future. |
fire-light42
left a comment
fire-light42
left a comment
There was a problem hiding this comment.
I have not looked in-depth at each line yet, given the sheer magnitude of this diff. It is hard to keep track of what is refactored, moved or unchanged. However, I like what I see so far.
| @LayoutRes | ||
| protected open var layout: Int = R.layout.fragment_player | ||
| /** The shared [PlayerView] host that owns all player state and view references. */ | ||
| protected lateinit var playerHostView: PlayerView |
There was a problem hiding this comment.
You use playerHostView like a nullable with checks, but mark it as lateinit? Why not just mark this as nullable, and force the check it with the ? operator.
There was a problem hiding this comment.
That was just a design decision. Probably a poor design decision though and you make a good point.
There was a problem hiding this comment.
Hmm, actually I think I did this to make it more explicit with checks using isInitialized instead which I figured would be easier debugging if the need arises then tracing where it is becoming null, it would be essier to trace if its initialized at all, I did write this part of the code 6 months ago though. I can change it if you want though? Still not sure if it was the best choice or not myself though, so if you do think otherwise I will definitely change it.
2 participants
This practically rebuilds the entire player (sans IPlayer/CS3IPlayer), aimed to remove the FullScreenPlayer inheritance from ResultFragmentPhone, use BaseFragment for ResultFragmentPhone and the player, and make the player easier to maintain and expand in the future, as well overall cleanup to code readability and adding documentation to methods in the player. Also aimed to not fully crash if the layouts have minor differences as well.
Eventually FullScreenPlayer could be entirely factored out into helper methods for GeneratorPlayer, and AbstractPlayerFragment removed completely, but to reduce the chance of harder to resolve merge conflicts with removed files, I decided to not do it in this PR.