Center videos correctly & fix padding for controls #4255

blackbox87 · 2020-09-08T21:24:29Z

What is it?

Bug fix (user facing)
Feature (user facing)
Code base improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Centers vertical and landscape videos correctly on devices with cutouts
Prevents the controls from being placed under corner cutouts by adding a little extra padding while in landscape

Fixes the following issue(s)

It improves on #4154 and fully fixes #4040

Testing apk

app-debug.zip

Agreement

I carefully read the contribution guidelines and agree to them.

ghost · 2020-09-08T21:28:52Z

@blackbox87 this fix also this #4179 ?

blackbox87 · 2020-09-08T21:44:59Z

@domiuns On my device it only takes me 1 tap to see both the status bar and the video controls and this commit does nothing to change that.

blackbox87 · 2020-09-09T00:29:06Z

Sorry about the forced pushes. I messed up and needed to correct things.

Prevents the controls from being placed under corner cutouts by adding a little extra padding while in landscape

Basically it's difficult to accurately detect if you've got a cutout, it's position and it's size. So without the padding you might end up having the controls or the videos title placed under a cutout. That's why padding is now being applied to all devices as a precaution.

I've tested the changes on my old phone without a cutout and my new phone with a cutout. It looks good to me.

opusforlife2 · 2020-09-09T07:04:43Z

Could upload a test apk so other people can test it?

wb9688 · 2020-09-09T07:32:46Z

@blackbox87: Can't you use https://developer.android.com/reference/android/view/WindowInsets#getDisplayCutout()?

Btw force pushing isn't a problem for PRs, we like having a clean (but not "too clean") commit history.

Also, do you post, then delete and repost a comment? I get multiple notifications from your comments, but if that's the case, could you please just edit your comment?

blackbox87 · 2020-09-09T15:29:41Z

@opusforlife2 Done.

@wb9688 Not easily, no. Mainly because you want the controls to always be centered on the screen, but when using that API you'll need to calculate things to center the controls and it needs to change as you rotate your device.

I opted to do it the easier way, which is to add enough padding to the left and right sides of the screen so that the controls are centered. And although it's possible not to apply that extra padding on devices without a cutout I haven't done that for the sake of consistency.

Also, do you post, then delete and repost a comment? I get multiple notifications from your comments, but if that's the case, could you please just edit your comment?

I don't believe I have, at least not deliberately.

avently · 2020-09-09T17:07:31Z

@blackbox87 I thought extra padding will look bad but actually it is quite nice with your code. I didn't tested everything, just launched the app to see the changes of look.

The only thing I want to ask you is to remove extra padding for devices without cutout and here is why. I don't want to pay for someone's ugly displays. Yes, I hate these big terrible cutouts and never liked it. You or someone else may disagree but if you like it, not a problem, just don't add extra padding for those who has displays without cutouts.

Big thank you that you found a way to work around this big Android API problem.

avently · 2020-09-09T17:30:56Z

Found an issue while testing.
To reproduce:
Use vertical screen orientation
Open vertical video (billie eilish - bad guy, for example, our classic :))
Add any non-vertical video to queue (if you have a checkbox Autoplay checked, it will be added automatically). To make this just long press on a video from related videos and choose Enqueue in background
Press on thumbnail to start playing in main player
Click on fullscreen button to enlarge the video
Click on next video arrow button

You'll see that padding on top and bottom is removed, so controls located under navigation bar, and status bar, and cutout.

Let me know if I need to make a screenrecord.

blackbox87 · 2020-09-09T17:53:10Z

@avently

Let me know if I need to make a screenrecord.

Please do as I'm probably doing something wrong, but I can't replicate that issue. Seeing it would help.

I'm probably probably going to adjust the code slightly anyway since I just tested it on Android 4.4 emulator and I don't like how the black navbar makes the controls look off center, even though they are centered.

opusforlife2 · 2020-09-09T18:14:07Z

Open vertical video (billie eilish - bad guy, for example, our classic :))

You mean "you should see me in a crown" :P

avently · 2020-09-09T18:19:39Z

ScreenRecord-2020-09-09-20-57-21.zip

blackbox87 · 2020-09-09T19:12:32Z

@avently
So it looks like setControlsSize() should be called when switching between videos. Most of this code is yours, so any suggestions where the best place to put that is?

avently · 2020-09-09T19:18:18Z

So it looks like setControlsSize() should be called when switching between videos.

Better to avoid it because you can call this in the wrong place (while video is rotating, for example). And the resulting values for screen elements will be wrong.
In the video you may see that padding was changed when video was changed, it means that you need to find a place in code where such change was happen and to adjust the code based on new information.

setControlsSize() is kind of magic. I do here what Android should do for us, so it's hard to say to you the correct place when the code should be changed and how. But the main rule is to avoid calling this method many times because it can be called in many device's UI states

avently · 2020-09-09T20:04:20Z

Maybe this is a problem of the issue:

NewPipe/app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java

Line 1542 in 6b27bce

topPad = !isLandscape && DeviceUtils.hasCutout(topPad, metrics) ? 0 : topPad;

Here you don't think about situation when a video can be in fullscreen but not in landscape. But it's looking at the code on github, didn't tested.

avently · 2020-09-09T20:06:03Z

Looks like some day we will end up by removing all this custom code in favor of UI without status bar and navigation bar that's simple but not so useful:) But for now let's do cool custom things:)

opusforlife2 · 2020-09-09T20:17:02Z

I have no cutouts. In a vertical video (we all know which one :P ) the bottom stuff (seekbar etc.) is padded, but the top stuff isn't.

But I agree with avently that devices without cutouts shouldn't be affected by this PR at all.

MD77MD · 2020-09-09T21:35:04Z

noooo @avently, anything but the status bar. like you said, let's stay cool 😎 and useful. 😊

ghost · 2020-09-09T21:36:45Z

Nice, this apk crash and block my device in loop because when i open an video app crash and phone try to reopen video. I have remove battery for unlock phone

blackbox87 · 2020-09-09T21:58:30Z

@domiuns None of my changes should cause that, so maybe the issue is with the current dev branch.

Instead of removing your battery you can probably hold your power button down for 10-20 seconds.

ghost · 2020-09-10T12:06:41Z

@blackbox87 demostration when i click on video https://streamable.com/e/ygkbwt

blackbox87 · 2020-09-10T16:02:36Z

@domiuns Try the 2020-09-08 version posted here > https:/TeamNewPipe/NewPipe/projects/17#card-42918515

Does it have the same issue?

@avently I fixed the bug that you reported, but now I'm stuck trying to detect cutouts correctly. Your hasCutout() method doesn't work because it assumes that the status bar height will be adjusted for a cutout, which isn't the case on my device. So I've tried using getDisplayCutout() instead, but if I start NewPipe in landscape then it'll often return null.

avently · 2020-09-10T17:55:05Z

@blackbox87 can't help you in this case because never did something with cutouts in my apps. If you will not find a solution, not a problem, will leave as the current state of dev branch. Not a big deal I think to have a non-centered vertical video and masked cutout space in landscape.

blackbox87 · 2020-09-10T18:46:47Z

Not a big deal I think to have a non-centered vertical video and masked cutout space in landscape.

I think it is a problem since it worked perfectly before the unified player was added. That's the only reason I'm still trying to fix this.

At the moment I can't say that I'm a fan of the unified player as it's introduced more issues than it solves.

avently · 2020-09-13T17:32:14Z

@blackbox87 cool! Thank you. So you're ready to upload the latest changes and to merge the PR? Or something is not ready yet?

blackbox87 · 2020-09-13T17:37:37Z

If everything looks good and nobody can find any issues then I'll update this PR and then it should be ready for merging.

I'm currently waiting to see if my latest changes also fix #4277.

avently · 2020-09-13T18:28:28Z

Based on my testing, all works perfectly:

phone without cutout, single cutout, double cutout
tablet without cutout
all orientations
multiwindow mode
vertical videos with single cutout and without (didn't tested other variants).

So, yeah, works great. Great job, man!

avently · 2020-09-13T19:07:53Z

But note that I use a phone with 16:9 aspect ratio, on other screens situation maybe different

TobiGr

thank you for the effort!

TobiGr · 2020-09-19T09:21:22Z

app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java

@@ -203,6 +207,11 @@
 private int cachedDuration;
 private String cachedDurationString;

+ private boolean gotCutout;


Suggested change

private boolean gotCutout;

private boolean hasCutout;

TobiGr · 2020-09-19T09:27:57Z

app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java

+ if (rotation == Surface.ROTATION_90) {
+ if (gotCutout) {
+ ctrlPadLeft = controlsPadding + insetLeft;
+ ctrlPadRight = controlsPadding
+ + (getNavBarMode() == 2 ? 0 : navBarHeight) + insetRight;
+ } else {
+ ctrlPadLeft = controlsPadding;
+ ctrlPadRight = controlsPadding;
+ }
+ } else {
+ if (gotCutout) {
+ ctrlPadLeft = controlsPadding
+ + (getNavBarMode() == 2 ? 0 : navBarHeight) + insetLeft;
+ ctrlPadRight = controlsPadding + insetRight;
+ } else {
+ ctrlPadLeft = controlsPadding;
+ ctrlPadRight = controlsPadding;
+ }
+ }


why do you only check for 90 deg, but not for 180?

@TobiGr When it's 90° in landscape then the navbar is on the right side of your screen, so the else statement handles the opposite. And this orientation check only applies to phones in landscape, so it always seems to work.

If you look at #4272 then you'll see that avently worked on a cleaner method, but we had a disagreement about handling a navbar feature that's found on a lot of custom Android 10 ROMs (LineageOS, ArrowOS, Resurrection Remix, PixelExperience, AICP etc).

avently · 2020-09-19T10:33:53Z

@TobiGr there is more recent method of doing the same thing as this PR but in more correct/universal way: #4272 (comment)

This PR contains one more thing (useful for @blackbox87) that can be ported over my PR: more padding for devices with hidden navbar and enabled gestures on custom ROMs. We have a talk related to all this in the link I wrote.

blackbox87 · 2020-09-24T19:05:45Z

@TobiGr @Stypox I'm closing this PR because #4272 handles the insets in a cleaner way, although I still think the overlapped gesture navbar issue should be handled by #4272 rather than a separate PR. And I'm sure Avently will disagree, but the only reason I opened this PR was because he didn't want to fix the issue until I showed that it could still be done. So originally I had to base my changes on top of his, then rebase to the dev branch and then Avently looked at my changes and bundled his own version of the cutout fix into a PR that fixes multiple different issues.

I won't submit another PR with a fix for hidden gesture navbars, but if someone else wants to do it then here's a fix that's been tested against a few Android 10 ROMs.

FarisZR · 2020-09-24T19:14:07Z

i can confirm this works with a galaxy s10+ with modpunk Linage os.

FarisZR · 2020-09-24T19:34:15Z

@TobiGr @Stypox I'm closing this PR because #4272 handles the insets in a cleaner way, although I still think the overlapped gesture navbar issue should be handled by #4272 rather than a separate PR. And I'm sure Avently will disagree, but the only reason I opened this PR was because he didn't want to fix the issue until I showed that it could still be done. So originally I had to base my changes on top of his, then rebase to the dev branch and then Avently looked at my changes and bundled his own version of the cutout fix into a PR that fixes multiple different issues.

I won't submit another PR with a fix for hidden gesture navbars, but if someone else wants to do it then here's a fix that's been tested against a few Android 10 ROMs.

#4272 doesn't fix the black notification bar problem.

avently · 2020-09-24T19:39:18Z

@blackbox87 thank you for your contribution! Without it I wouldn't start thinking about cleaner and better way of positioning the player. Your PR is really important for me because there are a few people who really do PRs here. And I was surprised that you started to to so. Even if we disagree with each other in some situations I think we did a great job and can do it in the future too.

blackbox87 · 2020-09-24T19:47:19Z

@fareszr Black notification bar problem? I'm not sure which that is, but I'd guess that once #3178, #4272 and #4288 are merged then it'll be fixed.

You could try using this or this? But you'll probably want to continue that discussion in #4272 if the issue occurs because of one of avently's commits.

FarisZR · 2020-09-24T19:49:19Z

@fareszr Black notification bar problem? I'm not sure which that is, but I'd guess that once #3178, #4272 and #4288 are merged then it'll be fixed.

You could try using this(#4272 (comment)) or this(#4288 (comment))? But you'll probably want to continue that discussion in #4272 if the issue occurs because of one of avently's commits.

like this

blackbox87 · 2020-09-24T20:08:38Z

@fareszr What's your phone model?

If your device has hardware buttons then how that looks might actually be correct, but if you've got the navbar hidden via a setting then that's likely the issue as Avently's version doesn't check the navbar heights or modes.

If I use Avently's version on my device running LineageOS 16 it looks correct.

FarisZR · 2020-09-24T20:12:10Z

@fareszr What's your phone model?

If your device has hardware buttons then how that looks might actually be correct, but if you've got the navbar hidden via a setting then that's likely the issue as Avently's version doesn't check the navbar heights or modes.

If I use Avently's version on my device running LineageOS 16 it looks correct.

i chose the zoom option to fill the screen.
on you version it fills the entire screen
but using #4272 it has the same issue from source.

device: S10+ with modpunk's Los 17.1

avently · 2020-09-24T20:35:13Z

@blackbox87

if you've got the navbar hidden via a setting then that's likely the issue as Avently's version doesn't check the navbar heights or modes.

I don't need it because Android adjusts view bounds for me. On screenshot it looks like he should see nav bar but it is invisible (or only layout was adjusted without showing buttons from SystemUI which is strange because others don't have such problem).

@fareszr

what apk did you install from my PRs? Give me a link, please
could you upload a screenshot from this PR where you see a correct result in the same situation? I want to compare working vs non-working look of the app
do you have any specific settings in your rom related to navigation bar?
what's navigation bar type di you have? Three buttons, two buttons, one button, no buttons at all?

And lets move to #4272 since that pr is a better place for such discussion

avently · 2020-09-24T20:43:41Z

@fareszr it's a known bug of your ROM
https://forum.xda-developers.com/showpost.php?p=82170539&postcount=3

Known bugs ->

in full screen apps there may be a black bar to hide the camera cutout which can not be disabled

So, will not be fixed, ask your rom developer

blackbox87 · 2020-09-24T20:50:46Z

@fareszr Oops, sorry. I see that you mentioned the phone model in your original post.

@avently Good catch. And I agree with not trying to fix bugs that are caused by unofficial ROMs.

I'm not entirely sure why my version still works though, unless it has something to do with the layout changes you've made (adding/removing fitsSystemWindows?). Everything else should be nearly the same, since we both only apply padding to the controls.

avently · 2020-09-24T20:57:15Z

@blackbox87

In your code you still have FLAG_LAYOUT_NO_LIMITS because you arrange the views manually. I don't do that so I don't have such flag and I let the system do all adjustments. His rom doesn't apply correct adjustments if flag FULLSCREEN is set

blackbox87 · 2020-09-24T21:16:01Z

@avently Ah! Of course, that'll do it.

I do agree with not fixing the issue in this case. I still don't agree on the other issue though, since almost every Android 10 and 11 ROM that isn't based on pure AOSP has the option to hide the gesture bar. But if someone wants to fix it then I've shared a little code that does the trick.

If nobody fixes the issue with the hidden gesture bar within a few months then I'll consider making the PR myself. At the moment I just don't have a lot of free time and personally I'd rather wait for commits to the unified player to slow down so that there's less conflicts.

FarisZR · 2020-09-24T22:21:59Z

well thats unfortunate i am stuck with non-unified player now, all other apps work correctly including youtube its only an issue with newpipe debug edition and avently PR.

avently · 2020-09-24T22:48:13Z

@fareszr that's interesting, so current release works ok for you?
Let's find out why. Unified and non-unified should work the same in case of navigation bar. I'll post some test apks for you in #4272.
Answer on these questions inside #4272:

what apk did you install from my PRs? Give me a link, please
could you upload a screenshot from current released version 0.19.8 where you see a correct result in the same situation? I want to compare working vs non-working look of the app
do you have any specific settings in your rom related to navigation bar?
what's navigation bar type do you have? Three buttons, two buttons, one button, no buttons at all?

FarisZR · 2020-09-24T23:07:18Z

@fareszr that's interesting, so current release works ok for you?
Let's find out why. Unified and non-unified should work the same in case of navigation bar. I'll post some test apks for you in #4272.
Answer on these questions inside #4272:
* what apk did you install from my PRs? Give me a link, please

* could you upload a screenshot from current released version 0.19.8 where you see a correct result in the same situation? I want to compare working vs non-working look of the app

* do you have any specific settings in your rom related to navigation bar?

* what's navigation bar type do you have? Three buttons, two buttons, one button, no buttons at all?

oh wow its fixed in the final2.zip file, i downloaded the initial test app from the main PR comment.
Thanks and sorry for wasting your time, it would have been better to update the description with the new links!.

avently · 2020-09-24T23:23:04Z

Glad there is no issue to fix:). Testing apk is a thing that changes often so when you test something from a PR take a look at the end of the page. The truth is always at the end:)

opusforlife2 added bug Issue is related to a bug GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) labels Sep 8, 2020

blackbox87 force-pushed the center-for-cutouts branch 2 times, most recently from 1005305 to 0bd2130 Compare September 8, 2020 21:44

blackbox87 force-pushed the center-for-cutouts branch from 0bd2130 to 6b27bce Compare September 8, 2020 22:06

avently mentioned this pull request Sep 14, 2020

Small fixes of issues with old devices support, brightness, etc #4272

Merged

2 tasks

blackbox87 force-pushed the center-for-cutouts branch 2 times, most recently from 4e90c5f to a5e125e Compare September 15, 2020 23:17

Center videos correctly & fix padding for controls

ffedf13

blackbox87 force-pushed the center-for-cutouts branch from a5e125e to ffedf13 Compare September 16, 2020 21:41

TobiGr reviewed Sep 19, 2020

View reviewed changes

Stypox mentioned this pull request Sep 24, 2020

Can't go directly to fullscreen when tapping thumbnail #4152

Closed

blackbox87 closed this Sep 24, 2020

Center videos correctly & fix padding for controls #4255

Center videos correctly & fix padding for controls #4255

Conversation

blackbox87 commented Sep 8, 2020 • edited Loading

What is it?

Description of the changes in your PR

Fixes the following issue(s)

Testing apk

Agreement

ghost commented Sep 8, 2020

blackbox87 commented Sep 8, 2020

blackbox87 commented Sep 9, 2020

opusforlife2 commented Sep 9, 2020

wb9688 commented Sep 9, 2020

blackbox87 commented Sep 9, 2020 • edited Loading

avently commented Sep 9, 2020

avently commented Sep 9, 2020

blackbox87 commented Sep 9, 2020

opusforlife2 commented Sep 9, 2020

avently commented Sep 9, 2020

blackbox87 commented Sep 9, 2020

avently commented Sep 9, 2020

avently commented Sep 9, 2020

avently commented Sep 9, 2020

opusforlife2 commented Sep 9, 2020

MD77MD commented Sep 9, 2020

ghost commented Sep 9, 2020 • edited by ghost Loading

blackbox87 commented Sep 9, 2020

ghost commented Sep 10, 2020 • edited by ghost Loading

blackbox87 commented Sep 10, 2020

avently commented Sep 10, 2020

blackbox87 commented Sep 10, 2020

avently commented Sep 13, 2020

blackbox87 commented Sep 13, 2020

avently commented Sep 13, 2020

avently commented Sep 13, 2020

TobiGr left a comment

Choose a reason for hiding this comment

TobiGr Sep 19, 2020

Choose a reason for hiding this comment

TobiGr Sep 19, 2020

Choose a reason for hiding this comment

blackbox87 Sep 19, 2020

Choose a reason for hiding this comment

avently commented Sep 19, 2020

blackbox87 commented Sep 24, 2020

FarisZR commented Sep 24, 2020

FarisZR commented Sep 24, 2020

avently commented Sep 24, 2020

blackbox87 commented Sep 24, 2020 • edited Loading

FarisZR commented Sep 24, 2020 • edited Loading

blackbox87 commented Sep 24, 2020

FarisZR commented Sep 24, 2020

avently commented Sep 24, 2020 • edited Loading

avently commented Sep 24, 2020

blackbox87 commented Sep 24, 2020 • edited Loading

avently commented Sep 24, 2020

blackbox87 commented Sep 24, 2020 • edited Loading

FarisZR commented Sep 24, 2020

avently commented Sep 24, 2020 • edited Loading

FarisZR commented Sep 24, 2020

avently commented Sep 24, 2020

blackbox87 commented Sep 8, 2020 •

edited

Loading

blackbox87 commented Sep 9, 2020 •

edited

Loading

ghost commented Sep 9, 2020 •

edited by ghost

Loading

ghost commented Sep 10, 2020 •

edited by ghost

Loading

blackbox87 commented Sep 24, 2020 •

edited

Loading

FarisZR commented Sep 24, 2020 •

edited

Loading

avently commented Sep 24, 2020 •

edited

Loading

blackbox87 commented Sep 24, 2020 •

edited

Loading

blackbox87 commented Sep 24, 2020 •

edited

Loading

avently commented Sep 24, 2020 •

edited

Loading