Fix opening VideoDetailFragment and much more

[Stypox]

What is it?

• Bugfix (user facing)
• Feature (user facing)
• Codebase improvement (dev facing)
• Meta improvement to the project (dev facing)

Description of the changes in your PR

• Unify all ways of opening the VideoDetailFragment into two methods: openVideoDetailFragment() which sends the new data directly to the fragment manager, openVideoDetail() which sends an Intent received by MainActivity which in turn calls openVideoDetailFragment().
• The arguments taken by openVideoDetailFragment() have changed: autoPlay is not there anymore, and switchingPlayers was added to be able to always determine correctly if the function was called as a way to switch players. autoPlay is now calculated based on the user's preference, on switchingPlayers and on whether there is already a background/popup player running. For more info see these lines. This makes it far more clear when the mini player will start playing (I think some situations were handled correcly only by accident before)
• Disable player stream preloading only if the current stream is going to be replaced for sure (see this). equals() was implemented for PlayQueueItems, so that (only) the url is compared when checking them.
• Add @Nullable and @NotNull to some variables in VideoDetailFragment, allowing me to enforce or remove some null checks. Rename name to title for consistency.
• Remove static qualifier from currentInfo and playQueue in VideoDetailFragment: they were never accessed as static members, and having them static caused random problems while replacing the VideoDetailFragment with a new instance of itself.
• Do not force reopen MainActivity when sharing something to NewPipe and pressing on "Show info". This was introduced in 67d2b91 by @yausername while handling urls in comments, but at that point there was no unified player, so probably without that it didn't work. But now everything works good even by keeping the current MainActivity instance, and this yields many improvements: opening urls in descriptions in NewPipe now does not erase backstack, nor does clicking Open details on queue items, and probably something else. :-D
• Replace "Video player" with "Show info" when sharing a video to NewPipe but the background or popup players are running, and thus autoPlay would be false.
• Handle "Video player" in the exact same way as "Show info" for streams, i.e. do not start a service just to fetch the info, since VideoDetailFragment can fetch data itself without issues.
• When sharing a video to NewPipe and a play button is pressed, replace the current queue instead of enqueueing. Enqueueing can still be achieved opening info and then enqueueing, but enqueueing directly was bad UX since the user would not see what happened.
• When opening NewPipe from scratch but a player is already running, show the bottom mini player Player: app does not show bottom bar player after exiting NewPipe with active background player #4712
• Make sure play/pause state is preserved when switching players via the specific buttons. Also replace START_PAUSED with PLAY_WHEN_READY for better consistency (i.e. not inverting twice).
• Move menu listener to switch players from BackgroundPlayerActivity to ServicePlayerActivity, thus removing a useless abstract function.
• Remove some unused code in RouterActivity
• Refactor getPlayerIntent() functions in NavigationHelper and removed unused navigation functions.
• Remove testing-only code in PlayQueue reportingReactor was used to capture and log broadcast events in debug builds. This can be undone if unwanted.
• Remove unused preference autoplay_through_intent_key (left there by accident after 2907, probably)
• Some refactoring here and there: fix warnings in openDownloadDialog, in openDownloads, in PlayerHelper and more

Fixes the following issue(s)

Fixes #4541
Fixes #4432
Fixes #4403
Fixes #4687
Fixes #4712

APK testing

Since this PR changes the logic of many things, the more people can test, the better.
@opusforlife2 @Jean757 @shadow00 @nbmrjuhneibkr could you test if the new share behaviour suits you, i.e. #4403?
@Anotherlife @pew-pew could you test if there are no more issues with playback breaking after opening details, i.e. #4541?
@aand18 @Medard22 @UserX404 @ventilaar @floral-qua-floral @Idiotten could you test if this solves #4432, i.e. background playback stopping at random after some (constant) time?
app-debug.zip

Due diligence

• I read the contribution guidelines.

[Medard22]

So far seems it seems good. I didn´t encounter any problems with the playback. I will test it thoroughly tommorow.

[avently]

Disable player stream preloading only if the current stream is going to be replaced for sure (see this ) @avently is this ok?

I don't think so.
Why did you add all those checks?

newQueue != null && playQueue != null
!Objects.equals(newQueue.getItem(), playQueue.getItem())

What's the reason for them to exist? They will not fix the issue with preloading of background & popup because the fix would be to apply disabled preloading for player.videoPlayerSelected(). In your case you probably will see a situation when Objects.equals(newQueue.getItem(), playQueue.getItem()) is always true even for the same streams because of different restore position (or maybe I'm wrong, don't remember the contents of playqueueitem). But it will not fix the problem with preloading anyway.

Why did you delete lines after https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java#L858 ? They were here for a reason. If you found another way to do the same thing, then ok.

You made here many good things (based on description). It's hard to see all new code on a phone but fixes and features you introduced is very important!

[avently]

Ok. Some things I found:

• the result of removing the code I mentioned and objects.equals checks:
  Open any video in audio player
  Tap on notification
  Choose to play in main player in three dots menu
  Player is not changed to main, still audio only (problem)
  If you tap on notification and choose another time on timeline - endless loading - another problem.

• second thing is present on 0.20.0, so skipping it

[VGkav]

The "popup player stopping after viewing video details" problem does not happen with this apk from what I tested.

[Idiotten]

I have not encountered #4432 after a couple hours of queuing and such. Seems fixed for me.

[opusforlife2]

Yay! So many good changes! Gonna test now.

[opusforlife2]

Does this not include the decryption retry code? I got that error just now.

[TobiGr]

@opusforlife2 This branch is not based on the latest dev. that fix is not included

[opusforlife2]

Can confirm it fixes #4403. 👍

[MD77MD]

When sharing a video to NewPipe and a play button is pressed, replace the current queue instead of enqueueing. Enqueueing can still be achieved opening info and then enqueueing, but enqueueing directly was bad UX since the user would not see what happened.

this is great if share to player, but please make it that it sould enqueue when sharing to pup-up or background player.... or a separate enqueue option..

the reason:

1. its useful in those scenarios.
2. This is the only easy way to enqueue videos from other apps to newPipe

[Stypox]

This is the only way to enqueue videos from other apps to newPipe

@MD77MD As I explained, this is not true, since now if there is a video already playing the "Show info" button is shown, and from video details you can just long press on background or popup

[Stypox]

@avently About the preloading part, I then am not sure I understand when preloading should be disabled. From what I understood so far: when the current stream is being dismissed in favour of a new one, then preloading can be disabled for that track since it is soon going to be replaced. Therefore, when switching queues, we should make sure the newly selected video is different from the current one, otherwise preloading would be disabled forever. Ok, maybe if the video is the same and the timestamp differs we could disable preloading, but that's just a special case, so I wouldn't add complexity for that for now.

the result of removing the code I mentioned and objects.equals checks:

Do you mean that by removing my conditions for preloading it creates the explained issues, or that my code creates those?

Why did you delete lines after https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java#L858 ? They were here for a reason. If you found another way to do the same thing, then ok.

If you are talking about "Situation when user switches from players to main player. All needed data is here, we can start watching" then yeah, I am handling the switching players case directly here

[Stypox]

Another apk with the same changes but with updated extractor: app-debug.zip

[avently]

@Stypox here I provided steps for reproduce problems. You said that you made another way for switching players so that way should be used in order to switch to main player from BackgroundActivity like in my reproducible example. Because now you're not switching players when moving to video details from backgroundactivity.

About preloading part:
I think you just need to make sure it's actually works. If it is then current code is ok. But again in my reproducible example the code is not working properly.

Did you checked with logs, for example, that your code for preloading really getting called and not just skipped when one stream changes to another?

[Stypox]

@avently @TobiGr @opusforlife2 @B0pol I added more changes, and fixed the issue reported by avently:

• also have the popup fullscreen button and the player queue switch-to-main button follow the new method
• equals() was implemented for PlayQueueItems, so that (only) the url is compared when checking them.
• Make sure play/pause state is preserved when switching players via the specific buttons. Also replace START_PAUSED with PLAY_WHEN_READY for better consistency (i.e. not inverting twice).
• Move menu listener to switch players from BackgroundPlayerActivity to ServicePlayerActivity, thus removing a useless abstract function.
• Refactor getPlayerIntent() functions in NavigationHelper and removed unused navigation functions.
• Fix some warnings in PlayerHelper

app-debug.zip

[avently]

Changes sound good, the code for new commit looks good too. I always like optimizations:)

[opusforlife2]

@Stypox 2 of the 3 linked issues are reported to be fixed. What else needs to be tested, exactly?

[Stypox]

I guess a last round of tests about the things I changed would be enough, just to be sure the new behaviour is not broken. Other than that, yeah, I think this is ready

[opusforlife2]

I guess a last round of tests about the things I changed would be enough

@Stypox But... but the stuff you've changed is all programming jargon. :( That's why I don't know what to test.

[Stypox]

@opusforlife2 just opening the video detail fragment under many different conditions is enough

[Stypox]

@vkay94 there is nothing to be sorry about, accidental problems always happen ;-)

[vkay94]

Yeah, I know, but I through I checked it carefully :) @Stypox Can you add the fix with this PR? Simply replace both methods with each other ;)

[Stypox]

@vkay94 is this ok?
@opusforlife2 app-debug.zip

[opusforlife2]

Fixed.

[Stypox]

I rebased and tested the rebased version all day with multiple situations and everything worked flawlessly: app-debug.zip

The only issue I found, that is present even in NewPipe 0.20.2, and is thus not caused by this PR, is:

• enqueue a random video in the background
• open a local playlist, a remote playlist, a channel or the history fragment
• tap on "Play all"
• the main player does not start playing right away (as it would be expected), instead the video details of the first item of the playlist are shown without playing and the rest of the queue is not preserved

Since the above issue is not caused by this PR I will take care of fixing it in a future one, so I think this can be merged @TobiGr

[B0pol]

The only issue I found, that is present even in NewPipe 0.20.2, and is thus not caused by this PR

Then please open a new issue

[Stypox]

@B0pol I was going to ;-)

[jay00128]

#4678. my issue ：video names on Youtube are Chinese，but on NewPipe they became english titles，maybe it's because of the community contributions 。🤔

[Stypox]

@jay00128 please open an issue in english about what you are experiencing and follow the template