Expandable player with unified UI

[avently]

Hello!
This is my second attempt in making a PR with unified player. I made conclusions from previous situation and this one should be better:

• less useless code changes like indentation, refactoring of old code, etc
• no files renaming
• no removing of old unsused code

It will help to make a review more productive.

Youtube screencast of changes

Here is what I'm introducing:

• main, background, popup players now connected via one service, one view, one fragment, one activity and one gesture listener
• main player located in a view with comments, description, etc. So you don't need to open another window for viewing a video. Everything in one place
• playback position is synchronized between players. Easy to switch from one to another
• expandable player at the bottom of the screen with cool animation and additional features like long click to open channel of a video, play/pause/close buttons and swipe down to dismiss
• in-player integrated buttons for opening in browser, playing with Kodi, sharing a video
• better background playback that can be activated in settings. Allows to automatically switch to audio-only mode when going to background and then switching to video-mode when returning to the app
• player service will be stopped automatically when user removes the app from recents
• there are two different bahaviors related to orientation changes:
  -- with locked global orientation the player will change orientation to landscape and will change it back after onBackPressed() (this method was made previously after @theScrabi requested it)
  -- with enabled global autorotation the player will listen orientation changes and will change UI to fullscreen or default size
• previous features like brightness control, open popup player when going to background, action chooser when clicking on a link, etc are working as expected
• tablet screens are supported too.

There are some bugs that are not fixed yet. Maybe somebody knows how to fix them because I didn't find a solution yet:

• R.id.playQueue can not be scrolled. Actions inside it work but not scrolling. This is related to appbar. Intercepting touches didn't help. This is not a major issue because short playlists can be viewed without problems
• when the player rotates screen orientation programmatically (if global orientation is locked) there is a black flash because of dead activity. It happens not as fast as I want. When orientation rotates with global autorotation enabled there is no such flash.

All unused code I left on the same place and will clean everything after review.
Thanks everyone for answers in #2882

• [ v ] I carefully read the contribution guidelines and agree to them.

[mauriciocolli]

Pushed a commit as I was writing, fixing some issues that I had listed. Nice!

---

I compiled a list of some issues that I found (mainly focused on the behavior for now, following reviews should focus more on the code):

• Theming issues due to calls to ThemeHelper#setTheme with the default arguments, overriding the already set for the context.
• When choosing a video below the video (scrolled down), the view doesn't return to the start, causing bad UX and leaving the user staring a blank screen.
• Fullscreen layout resizes when showing the soft keys, should stay under it like before.
  • As an alternative (as this would require some arrangement of the layouts), don't modify the fullscreen state when clicking, only the visibility of the controls.
  • If the alternative is done, however, it'd require the user to use the default gestures (swiping near the edge to make the navigation buttons appear) which could interfere with the other gestures. That's why the soft keys worked that way in the original implementation.
• Too little padding/margin around the fullscreen elements. Hard to click/swipe, as it is too close to the edge.
• Empty top-left corner when expanded. I think this was requested in your discussion issue, but at least something needs to be there, specially when the options menu is open.
  • The obvious choice would be the collapse button like YouTube does, although I never used in their UI, would help eliminating the feeling that the area is missing something.
• Popup buttons react immediately even though they are hidden. The original behavior should be kept: show the controls first.
• It seems to be possible to minimize a video in fullscreen. This introduces a inconsistency that when unpausing a video being in landscape, it will always alternate to the fullscreen state. But when minimizing a video in landscape and expanding it, the video will be playing in the expanded screen.
  • IMO the better option would be to just play the video normally and introduce a fullscreen button.
• When minimized, the elements of the player screen are still visible (play button, title). This is specially noticeable when paused and when minimizing from the fullscreen state (which has the title/subtitle views enabled).
• When sending audio only streams to the background, the play button doesn't take control of the player like it does when handling videos.
• When the play queue is opened, the player will alternate to the fullscreen state and will stay that way even if the queue is closed. This gives some glitches when minimizing or closing the panel.
• With the action of "Minimize on app switch" set to "None", the notification will still be displayed. It also confuses the user because it can't be unpaused.
• Cannot open playlist using "Play All" reliably.
  • It seems to be various calls to setInitialData, cleanUp...
• Cannot switch from players playing playlists reliably.
  • Same as above, it seems that the play queue is being replaced by some methods.
• Hard to understand/debug the VideoDetailFragment class.
  • Related to the 2 previous points, shouldn't this class be only a interface/UI to the player instance? As in, no play queue/state management allowed (delegating all the work to the player itself)? Let me know what you think about this.

(Some code styling issues, but that can wait for following reviews.)

---

About the issue that you mentioned:

R.id.playQueue can not be scrolled. [...] This is not a major issue because short playlists can be viewed without problems

It'd be nonetheless very bad to let something like this land in a release.

Although the state of just scrolling the video detail fragment, as you can test yourself, is very poor at the moment (unrelated to this PR), fling for example doesn't work correctly and the normal scrolling is bad aswell.

I bet if we could fix this, it'd be easier to make it all work smoothly. It should be worked on and fixed before the next release, I'll try to do it in the next few days.

[avently]

Thank you for detailed review!
I will look into all issues you mentioned. I agree with many of them.

Fullscreen layout resizes when showing the soft keys, should stay under it like before

I would like to get this behavior too

Empty top-left corner when expanded

This is what I talked about two years ago:) Probably I'll place the button like you said or find any good alternative since we already use the arrow button on the right top corner. Three dots looks not good for right side expandable menu.

But when minimizing a video in landscape and expanding it, the video will be playing in the expanded screen

I forgot to make the video playing in fullscreen when mini player expands in a landscape orientation. I think a user should be allowed to watch a video and read description in fullscreen if it wants to.
So to clarify what I mean:
--- when global orientation locked
user in portrate orientation clicks on play button -> orientation changes to landscape. If he presses Back -> orientation changes to portrate again and player is in non-fullscreen mode. But if he collapsed the video to mini player system UI shows and he can browse in landscape orientation. Then he expands mini player and video should play in fullscreen mode with hidden system UI.

--- when global autorotation enabled:
user in portrate orientation clicks on play button -> video plays, nothing more. User rotates phone to lanscape -> video plays in fullscreen mode. User presses Back button -> video pauses and exists in fullscreen mode. User presses Play -> video opens in fullscreen mode again.

It's hard to write about these things in an understandable way...

When sending audio only streams to the background, the play button doesn't take control of the player like it does when handling videos.

Can you give a link to such audio-only stream? I tested SoundCloud and it works ok.

When the play queue is opened, the player will alternate to the fullscreen state and will stay that way even if the queue is closed

I made it intentionally when playQueue contains more than 4 entries. But maybe you right and this behavour should be disabled.

Cannot open playlist using "Play All" reliably.
It seems to be various calls to setInitialData, cleanUp...

I can't reproduce it. For me remote playlists, local playlists and channels work well with Play All button.

Cannot switch from players playing playlists reliably.
Same as above, it seems that the play queue is being replaced by some methods.

It works a little bit unexpected, yes. For example, when you open playlist first time without switching to the next item in playlist you can switch players. But you play next video in playlist switching will only open current item. I'll fix it somehow. It is related to backstack.

shouldn't this class be only a interface/UI to the player instance?

Yes, it should but if I want to support backstack I need fragment to manage a playQueue as well. Because the playQueue is being saved in backstack and once user press Back the playQueue should be replaced.

[Redirion]

@avently it would be nice if you could upload your latest debug apk after each significant commit so that we can easily test.

[avently]

3 days I have been working on a code for fullscreen mode. The problem was on-screen controls that didn't work well with SystemUI.
So the job was to show the controls and NavigationBar together without overlaping. As you may know when NavigationBar appears 1) a view will be shorter on a height of NavigationBar or 2) the view will be under NavigationBar as well as the controls. It's not good in the case because our video view will be shrinked or the controls overlapped.

What's the must obvious way to make the video view with a width equal to screen but with NavigationBar shown and with controls fully visible?
Right, to make video under navbar AND to adjust the controls position based on a height of navbar. But the problem is that navbar can be so different from a phone to phone. I seached over the internet in different search engines but found nothing except some tricky ways to make this happen. By tricky I mean unstable and doesn't work in all situations like non-standart navbars, notches, etc. So I ended up with this idea and tried to make a "right solution" using Android way of doing such things. It's WindowInsets.

The problem is WindowInsets are supported in API 20+ but we have API 19 as a min supported version. Ok, things went wild and I created a CustomFrameLayout that fully supports WindowInsets as well as old method FitsSystemWindows (FrameLayout don't support insets at all). Then I changed all FrameLayouts in the way to a player view in a layout hierarchy to my custom implementation. Then I changed more pieces of code related to WindowInsets. It was strange because some Insets ruins one views but heals others. That's because insets are often used in simple scenarios without multiple layouts work together. The simple scenario is an Activity with a video player. But here I need to support normal work of NavigationBar/status bar with fragments and non-standart work with the controls and the video view. After many hours of work I got a result that I wanted. Everything worked well in Android 9 and 4.4.
BUT
I added hundreds of non-useful lines of code and this was really unneeded part of code for that simple task. Ok, I removed everything I done.

Then made a CustomSurfaceView and tried to make low-level method of calculating frame size. I overrided some methods and got half of the needed result. Bad news: that was a bug in Android probably (it shouldn't work at all) because that was impossible to make this method fully working (or maybe I don't know how). Anyway, new start.

I returned to the first idea with adjusting margin on the controls. This time I started to read source code of Android classes. It took a couple of minutes and I found a method which could give me what I wanted - the size of a usable space in a window in all scenarios. So I didn't actually need a NavBar size. I needed a size of a view that shows everything except NavBar, notches, etc. I tested the method and it works in Android 9 and Android 4.4, with one notch or two, or without any.

TLDR: please, install the app (or build it youself with the latest commits) and check that fullscreen mode works well with your device. If (yes) { I can continue }.

apk-inside2.zip

[Stypox]

I get the following error when installing both apk-inside and apk-inside2: "This package appears to be corrupt"

[avently]

@Stypox this is just zip archive. Extract apk from it and install via computer like:
adb install -r -t pathtoapk.apk

Note that you can already have installed debug version with different certificate and this may prevent from installing my build. But you can sign the apk with your key and install it the same way.
Or just build it yourself, it's easier if you want to review the code in the future.

P.s. github doesn't allow to upload .apk, only inside .zip archive.

[Redirion]

P.s. github doesn't allow to upload .apk, only inside .zip archive.

you can draft a new release in your repository and upload the apk there

[Stypox]

@avently I know that that's an archive, and that there has to be no conflicting debug package installed, and that I can build the apk myself (e.g. see what I did in #2517). ;-)
But why doesn't it install as other APKs do (by extracting the apk on the phone and then opening it), and it requires adb? Is it because it is not signed?

When I tell Android Studio to build an apk I usually get an apk without signature (and which does not require one at all) that can be installed anywhere without using adb. Most of the times I take a look at APKs or review PRs I am on mobile, and I don't have access to a Desktop computer with Android Studio or adb installed (or with enough internet connection to clone the repo, update gradle and the many Android Studio dependencies, etc., in a reasonable amount of time) so that's why I ask for a ready-to-install apk (and afaik it does require as much hassle as building the apks you built: two button clicks in Android Studio, so that should not be a problem).

Here is the APK I got after buiding it myself (Build -> Build Bundle(s) / APK(s) -> Build APK(s) in Android Studio, note that I did not click on Generate signed Bundle / APK); it installs correctly on my phone: app-debug.zip

[Stypox]

I found an issue: after clicking on the popup notification I get to the play queue screen and its title is Background player, even though that's the universal queue, if I understand correctly.

[avently]

But why doesn't it install as other APKs do

Because IDEA or Gradle adds TEST_ONLY flag to apk that started with Run action. Apk I uploaded here was received by copying my debug apk. I didn't use the method you wrote APK(s) -> Build APK(s). By using this method I can get apk without TEST_ONLY flag. So you helped to resolve this issue, thank you.

I usually get an apk without signature that can be installed anywhere without using adb

It's not true. No one can install apk without signature. You just got an apk from build tool that signed the apk with debug keystore for you.

after clicking on the popup notification I get to the play queue screen and its title is Background player

It's not a bug. I think the title is self-explanatory. User see an activity with a list of streams in background. It's a hidden activity for popup and audio players. What's the difference in this case? The content in popup and background activity was the same, just different title:)

[Stypox]

Oh ok, thank you for the clarifications :-D
I had some misconceptions about the apk building process ;-)

The content in popup and background activity was the same, just different title:)

The title is not different: in the screenshot below the title is Background player even though I was using the popup. I see two options here:

• Make a unique title, since the content is the same independently of the player that is playing it: something like "Player queue"
• Change the title based on whether the queue is being played on popup or background, like it is now

The first option is imo the best one, since it would better convey the idea of a unified player and also probably requires less effort.

[avently]

The title is not different

I mean the title is different in current git repo but the content inside the popup/background activities is the same. So what's a point of changing this title to another if the idea behind it could be applied to my implementation too.

I didn't remove "Background player" title because so many translations are already exist for it. But if it doesn't matter I can replace the title with "Player queue".

This is the next build (made from IDEA -> Build APK):
apk-inside3.zip
EDIT: popup will crash the app here, I found and fixed the issue. Will upload changes with a next block of fixes.

@mauriciocolli only 4 issues from your list exist now:
Empty top-left corner when expanded
Cannot open playlist using "Play All" reliably
Cannot switch from players playing playlists reliably
Hard to understand/debug the VideoDetailFragment class.

Other issues I fixed I hope.

[ghost]

Is there a way to enter full screen (landscape) without rotating my phone and having auto-rotate on? Mostly I love these changes (a big improvment imo) but, having auto-rotate always off makes it annoying to enter full screen. How about a full screen or rotate button?

[avently]

having auto-rotate always off makes it annoying to enter full screen

I'll make this behavior switchable from settings. So you'll have two options:

• autorotate orientation when global autorotation is locked
• don't do anything automatic but show fullscreen button by pressing on which orientation will be changed. It will be located at the right side of total video time label like in YouTube app.

I'm not sure about when I make this feature: in this PR or in the next PR.

Guys, what do you think about blank space at the left top corner in non-fullscreen mode? I mean what button or text you think would be great to have at that place? I want it to be something useful but I have no idea what it can be.

The obvious choice would be the collapse button like YouTube does

Maybe there is a better choice.

[ghost]

I think that there should always be an orientation overide option like is currently present in full screen mode (a rotate button in the overflow menu). It is common in media players (VLC, Youtube and current Newpipe have it) and is really useful if you don't want autorotate on, have a faulty autorotate or want to watch in landscape while your phone is in protrait. I may be a minority in this but a full screen button is pretty much essential for me with my current watching.

Thank you

[avently]

Added a new commit with fixes of every bug I could find.
@mauriciocolli all issues you mentioned except button at the top left corner are fixed. Please, re-review.

Three remaining tasks are:

• top left corner
• remove "show info" from RouterActivity because it doesn't make sense in current UI since "video player" option exists
• title for background activity.

I think the PR could be re-reviewed closely.

apk-inside4.zip

[Redirion]

videos start automatically playing. Is this intended?

I have also observed one crash. IllegalStateException for MediaButtonReceiver (2 services found instead of one). Sorry, I don't have the StackTrace. Wasn't yet able to reproduce it again.

[avently]

videos start automatically playing. Is this intended?

Yes. With current UI I think it's a good choice.

IllegalStateException for MediaButtonReceiver (2 services found instead of one).

Didn't see this crash. Hope you will be able to reproduce it

[avently]

@opusforlife2
For "positions in list" there is a settings key named enable_playback_state_lists_key

Interesting thing is that this key is used NOwhere except by me inside VideoDetailFragment
After this commit the key is not in use:
https://github.com/TeamNewPipe/NewPipe/pull/2517/files#diff-5b97b6c787e8fbad31d373380daed5aeL175

So in 0.19.8 this setting does nothing.

Anyway, regarding your question:

Strange. Playback position indicators in lists aren't showing anymore

Show a screenrecord with this behavior

@XiangRongLin

I just checked the 0.19.8 version and when i cancel background there, it also takes a while until the notification disappears

So, it's device/android specific thing. Next time before filling any issues check a behaviour in the previous versions. It will save time for everyone (except you :) )

[opusforlife2]

Show a screenrecord with this behavior

I tried to find an older apk above that had the behaviour I was thinking of, but then I remembered that there is a straight jump from the 0.18.0 base to a very recent one, so the old ones don't work anymore.

Instead, here's a screenshot of what used to happen with just "Positions in lists" enabled. Now this same behaviour only happens if you enable both "Watch history" and "Resume playback" as well. "Positions in lists" by itself does nothing, as you say.

I played a video, then closed it. Then I did the same with two more. Since the already open channel page can't get updated instantly, I backed out and opened it again, showing:

In the current release, these toggles are in a hierarchy. "Positions in lists" is greyed out if "Resume Playback" is disabled. Both are greyed out if "Watch history" is disabled. So you can only have Positions shown if Resume and Watch history are also enabled.

Maybe this behaviour should be reverted to what the current release does? It's unrelated to this PR anyway (some user had asked for it and you magnanimously included it), so it can be analysed and implemented in a separate PR.

[avently]

@opusforlife2

Instead, here's a screenshot of what used to happen with just "Positions in lists" enabled. Now this same behaviour only happens if you enable both "Watch history" and "Resume playback" as well. "Positions in lists" by itself does nothing, as you say.

I told you that "Positions in lists" setting is only used in the videodetailfragment by me. There is no usage of it in the current release you mentioned. Basically, it was removed from code by a mistake and do nothing since 2019.
I also didn't change anything with this indicators on cards on the screenshot you uploaded. I only applied this settings inside videodetailfragment. So you have an ability to see this indicator on player page even if you have Resume playback disabled for videos inside your backstack but enabled position in list. Example:

• history is On, resume playback Off, positions in the list On
• play any video
• select any other video without hard closing previous
• tap back button
• position of the video will be shown on the player card.

This is why the setting is not depend on Resume playback now because positions can still be shown with existing of backstack.

If I still saying something unrelated to what you are asking about show a screenrecord with comparison of what we have now and what you want from the current release.

[opusforlife2]

No, no, you're absolutely right.

So you have an ability to see this indicator on player page even if you have Resume playback disabled for videos inside your backstack but enabled position in list.

If this was the purpose of decoupling the Positions in lists toggle, it works perfectly in my testing.

Actually, after testing 0.19.8 more thoroughly, it turns out that "Watch history" toggled on by itself (Resume playback and Positions in lists disabled/greyed out) is enough to show positions in lists! This is clearly a bug because it is doing something different from what the user is told.

I checked the unified player and the same behaviour happens here as well. Watch history toggled on shows positions in lists. So this is not a unified player bug, but a bug in the current release which has been carried over. Since that is the case, I'll open a new issue for this.

[ghost]

Everything is working good in version 7-5.2 debug. I found an minor issue i.e the botton minimized player covers the last video of the page. It overlaps the last video of list as shown in picture:

[avently]

@mrpr3s there is an issue opened for this problem: #4047

[MD77MD]

ok so there is an issue with brightness. I'll try my best to explain it. newPipe doesn't follow the device set brightness but follows the video brightness even after closing the video

1. open any video in full screen
2. swipe the brightness all the way up (max brightness just for demonstration)
3. minimize the video
4. close the video
5. everything is soooo bright
6. open newPipe settings or about which follows the device actually brightness to see the difference.

I have noticed this bug for awhile but I was waiting for someone to explain it in better words. so sorry if I'm not clear enough

[avently]

@MD77MD #4043

[test2a]

Thanks for your work!

Please open a new issue for the notification bug @XiangRongLin @avently

I created a debug APK to test with the latest changes, link https://github.com/TeamNewPipe/NewPipe/projects/17#card-42918515

@B0pol hey. i download this apk and there seems to be a minor issue. the expanded view works fine. The problem is, if the video is in full screen, i can click on resize button and video continues to run in small window mode fine but if i click on back button, the video resizes but it pauses. i have to click on the video to play it again. other than that, i guess the update is going smoothly.

[DI555]

any chance to make it as a new custom 'modern' fork ?
there's too much of changes , i'd like to keep the usual NP, its looking and feeling..
But really great to be another one 'modern' fork of NP ! (in another repo here, like the repo of 'legacy' version)
in the same way like we already have a 'legacy' fork, and it's great !

[nikhilCad]

Giving the newpipe version before this 'modern' pr a new legacy repo sounds like a better option imo. Something like 'Non-unified legacy Newpipe'. Though I thought this particular thread should not be used for any further discussion as its exploding already.

…

On Tue 11 Aug, 2020, 12:35 PM DI555, ***@***.***> wrote: any chance to make it as a new custom 'modern' fork ? there's too much of changes , i'd like to keep the usual NP, its looking and feeling.. But really great to be another one 'modern' fork of NP ! (in another repo here, like the repo of 'legacy' version) in the same way like we already have a 'legacy' fork, and it's great ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2907 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALIG6OQQEZ6FHSMUNRMW4ETSADURXANCNFSM4KBDLHVQ> .

[hunkjazz]

Would you happen to have a link to download a working apk?

[opusforlife2]

@hunkjazz Check the other issue.