Refactor player and extract UI components

[Stypox]

Description of the changes in your PR

This big PR extracts ~2500 lines of code from the Player, namely the code that handles the various UI states, into plug-and-play ui classes. The UI classes I created so far are:

• PlayerUi: this is the base class with all of the base methods with an empty implementation. It holds a reference to the player.
• NotificationPlayerUi: contains all of the calls to the NotificationUtil class. Maybe we could merge it with NotificationUtil? Tell me what you think
• VideoPlayerUi: contains the code that main and popup player have in common, for example to handle controls, button presses, video surface, ... It also contains the reference to the VideoPlayerBinding, that is, the instantiation of player.xml that currently is the same for both the popup and the main player (and probably it's ok to be that way for now). This class is where most of the Player code went, as it is ~1500 lines long.
• MainPlayerUi: contains the code specific to the main player: handling fullscreen, stream segments, chapters & play queue list, minimizing to popup/background, main player gesture listener ...
• PopupPlayerUi: contains the code specific to the popup player: window management, close overlay, popup player gesture listener, ...

The Player object now holds a PlayerUiList UIs member variable through which UIs can be added, removed or queried. I used stream API and Optional to simplify things: when the player wants to call some method on all of its UIs it just has to call e.g. UIs.call(PlayerUi::onComplete);. And when classes from outside want to make changes to a specific UI, they can do player.UIs().get(MainPlayerUi.class) and use the Optional API on the returned object.

MainPlayer was renamed to PlayerService as that's what it actually is. I also removed the unused PlayerState and PlayerServiceBinder classes that were cluttering the .player package.

Highlights

• Now the background player doesn't keep all of the player views loaded in the background, since the Main/PopupPlayerUi is not added when starting the player in background, and removed when switching to it!
• The PlayerGestureListener class made no sense before the refactoring: why make both a base abstract class and a subclass, if they need to keep interchanging information and there is no difference in the nature of the code in the two classes?
• The exo player's video surface can be setup only when the surface view root has a parent and is not just a view floating around. But other than that, the setupVideoSurfaceIfNeeded method can be called anytime without issues, since the player works fine even without surface, and will start rendering happily as soon as a valid surface is connected. Btw, for Android >= API23, I used setVideoSurfaceHolder instead of manually extracting the surface from the holder and then calling setVideoSurface, see VideoPlayerUi line ~1500 (@Redirion is this ok)?
• When switching from main to popup player and viceversa, the view binding is reused, like before.
• For some reason the queue activity was using the queue adapter of the player to show the queue items in the list... Now it uses its own adapter as expected.

TODO / known issues

• Fix all TODO TODO comments
• Investigate why the initial video stream loading is far slower than when switching resolution Seems to also happen on dev
• Move notification calls to NotificationPlayerUi
• Understand what to do about BackgroundPlayerUi: it can probably be removed -- removed
• Turn the media session handling into a MediaSessionPlayerUi to do in another PR, note that this might solve problems with the thumbnail not being loaded correctly
• Maybe turn fragment/activity player listeners into ListenerPlayerUi or something like that to do in another PR
• Remove the playback speeds PopupMenu in the popup player and just use the usual PlaybackParametersDialog, to save a few lines of code, complexity and inconsistency. to do in another PR
• Improve //region comments
• Solve conflicts (to be done at the end)
• Reenable checkstyle (to be done at the end)
• Understand what to do with Sonar's analysis -- some warning solved, others are unrelated
• Test tést tEST Test tesT tEst tèSt -- I tested on API 19 emulated, API 31 emulated and API 30 on my Fairphone. Opus also tested. We both couldn't find any issue.

Fixes the following issue(s)

Fixes part of #7939
Fixes #7262 according to user testing
Fixes #8428

Please test!

Could you test and tell me all of the issues you can find?

[opusforlife2]

And so it begins.

Could you test and tell me all of the issues you can find?

Do you want bug reports only related to this PR? Or bug reports for the player in general, like playback exceptions?

[Stypox]

@opusforlife2 possibly only related to this PR, but if in doubt send anyway.

[opusforlife2]

Do you intend to fix some bugs along the way or is this strictly a 1:1 refactor without any change in what the code actually does, bugs included?

[Stypox]

The code had to change in many places, and so what I rewritten may work differently than before and solve some bugs. But the point of the PR is the refactor, not the bug fixing (which will hopefully become simpler after this PR is merged).

[Stypox]

I solved almost all TODOs. I rebased on top of dev (which contained #8020, and I have a few questions about that PR I posted there).

I used the first diff below here when rebasing, so that I could make sure that I didn't miss any changes in the Player class. You can take a look at the second diff for a comparison.

1. The changes that happened from the dev commit this PR was based on before rebasing (ac00c8f) and the dev commit this PR is currently based on after rebasing (4040cb3): https://github.com/TeamNewPipe/NewPipe/compare/ac00c8f6aebb3164fababe175d80eac15c2d6bf9...4040cb36bb8280fa6fc218e23414a8d22d2fb45b?expand=1
2. The diff between this PR before rebasing and this PR after rebasing: https://github.com/TeamNewPipe/NewPipe/compare/b9e6a6a411a2e35a91c2e2a0ee32a3b1e9f5b67f...1d3dd1545951347cffb9ed78d79374bc2073068e?expand=1

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
2 Security Hotspots
27 Code Smells

0.0% Coverage
0.0% Duplication

[Stypox]

I finished the TODO list, now this PR is completely ready for review (to be done at the beginning of 0.24.0, if possible).
I didn't solve all of the Sonar issues as most were only related to the code I moved, not the code I wrote anew. In particular the two security hotspots are just sendBroadcast being called in a way that allows all apps to see the broadcast, but that's not an issue that should be solved here.

[Redirion]

which broadcasts do you mean? Just create the Intent not directly in sendBroadcast and call intent.setPackage("org.schabi.newpipe") -> "security" problem solved

[litetex]

Thank you for the PR!
This cleanup was long overdue and it has max priority on my personal PR todo-list (so that we don't end up with a ton of merge-conflicts).

Disclaimer: Didn't test it so far.

Your coding style is really good, found only some minor things while reviewing (might be leftovers from the migration/previous code):

Also please rebase the PR (merge conflicts).

PS: You might also have a look at Sonarcloud

[Stypox]

@litetex I won't do much to this PR until 0.23.1 with delivery methods is released, as we decided a while ago. This PR will go into 0.24.0, which will also be the first version to drop support for KitKat.

[Stypox]

@litetex

• rebased on top of dev (it was not so difficult)

• fixed an issue with the volume slider (it was using the incorrect multiplier)

• applied your review comments (see my answers)

• You might also have a look at Sonarcloud

  I already did it. I had a look again and fixed some more code smells, but most things are only related to the code I copied over so I won't dedicate that much attention to it.

• which broadcasts do you mean? Just create the Intent not directly in sendBroadcast and call intent.setPackage("org.schabi.newpipe") -> "security" problem solved

  Solved. This should remove those two Sonarcloud security hotspots. It didn't solve them, should I revert the commit?

• remember that I did not write the code in *Ui classes, I just copied it over from Player (with many modifications), so I won't all of a sudden work on TODOs unrelated to this PR or solve the meaning of unclear comments.

[litetex]

@Stypox Looks pretty good to me now.

I think a few things are still not resolved/commented (see my previous review above) but the rest looks good.
I will install the debug APK and test it for a few days on my smartphone to see if I find any additional problems.

[opusforlife2]

Whoa, the player now reorients to portrait when it detects the video's aspect ratio with 'always fullscreen' on! Thanks, @Stypox!

It's much better than doing it manually, fumbling with the button, and having to rewind.

[Stypox]

I think a few things are still not resolved/commented (see my previous review above) but the rest looks good.

Oh yes, sorry. I pushed a commit regarding the PlayerType and added a comment about halt(0)

Whoa, the player now reorients to portrait when it detects the video's aspect ratio with 'always fullscreen' on!

Oh yeah, that's definitely something I worked hard on, yes! Uh, great, it was fixed by accident then ;-)

[litetex]

Tested the APK a few days, everything is working fine :)

Code also LGTM

[Stypox]

I rebased again, and I also used the apk for a few days without encountering issues. @litetex please merge before more conflicts arise ;-)

[opusforlife2]

Got a crash. I can't tell you the context of the crash because... I fell asleep while using the app and woke up minutes later to Scoop with a crash report. 😅

Edit: the APK I have is from 7 July, in case that renders this crash invalid or outdated.

FATAL EXCEPTION: main
Process: org.schabi.newpipe.debug.playerrefactor, PID: 31478
java.lang.NullPointerException: Attempt to read from field 'android.widget.FrameLayout org.schabi.newpipe.databinding.FragmentVideoDetailBinding.playerPlaceholder' on a null object reference
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.lambda$addVideoPlayerView$18$org-schabi-newpipe-fragments-detail-VideoDetailFragment(VideoDetailFragment.java:1316)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment$$ExternalSyntheticLambda18.accept(Unknown Source:4)
	at j$.util.Optional.ifPresent(Optional.java:159)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.lambda$addVideoPlayerView$19$org-schabi-newpipe-fragments-detail-VideoDetailFragment(VideoDetailFragment.java:1314)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment$$ExternalSyntheticLambda6.run(Unknown Source:2)
	at android.os.Handler.handleCallback(Handler.java:883)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at android.os.Looper.loop(Looper.java:214)
	at android.app.ActivityThread.main(ActivityThread.java:7356)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:940)

[Stypox]

@opusforlife2 thank you for reporting; should be fixed in 0e8cc72 :-)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
2 Security Hotspots
23 Code Smells

0.0% Coverage
0.0% Duplication

[litetex]

I also got 2 random crashes over the past week with the debug APK from 2022-07-10 however I was unable to see what happened because there was no ACRA report and I didn't have a PC with ADB near me. But I guess it was the same problem as opus pointed out above.

Merging for now to ensure no conflicts.

Subsequent problems should be addressed by new PRs.

[PJansky]

I've found a minor problem with some videos with non standard aspect ratios like album covers. After opening the video in portrait mode, the video is stretched to use the full width of the screen. It's only reproducible with videos from "Artist - Topic" Youtube channels, videos from other channels with squarish aspect ratios are somehow not effected. Going into full screen playback and back to portrait resolves this issue.

The testing APK otherwise works flawlessly in my two days of use.

Screenshot of problem below:

[opusforlife2]

Confirmed ^

[Stypox]

@opusforlife2 @PJansky see #8731

[opusforlife2]

From the OP:

Maybe we could merge it with NotificationUtil? Tell me what you think

This might have been addressed in the review comments, but just in case it wasn't, I'm leaving this here for future reference.

[Stypox]

No, it wasn't addressed ;-)