Fix rotation when auto-rotate is disabled

[blackbox87]

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

1. Enable auto-rotate and watch a video in landscape
2. Disable auto-rotate while watching the video in landscape

Previously NewPipe would return you to your default orientation, but now you'll remain stuck in landscape and NewPipe's fullscreen button won't do anything no matter how many times you press on it.

With this change NewPipe will remain in landscape mode, but when you press on the fullscreen button it'll correctly change the orientation to portrait.

Testing apk

app-debug.zip

Agreement

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

[opusforlife2]

APK to test.

[avently]

Works, thanks!

[opusforlife2]

Yup! Thanks for the fix!

[opusforlife2]

Oh no. Bug. First two steps are the same as yours.

1. Enable auto-rotate and watch a video in landscape
2. Disable auto-rotate while watching the video in landscape

Then tap back. The app will try to rotate to portrait, but then fail and go back to landscape. It will continue playing through this, and if you pause and tap back, it will start playing when it fails and goes back to landscape.

In the latest unified debug apk, tapping back in this situation pauses the video and shows you video details in landscape.

[avently]

@opusforlife2 nice catch! I would really like to see you using my app. Do you like trading on crypto?:)

[opusforlife2]

@avently For that you need money of your own first. ( ;´༎ຶД༎ຶ`)

Which app, though?

[blackbox87]

Then tap back. The app will try to rotate to portrait, but then fail and go back to landscape. It will continue playing through this, and if you pause and tap back, it will start playing when it fails and goes back to landscape.

That seems to be an existing issue that's caused by the fullscreen/rotate button and the back button behaving differently. I'll see if I can fix that too though.

[opusforlife2]

If you mean that the back button pauses the video, but the full screen button keeps playing it when you go back to portrait, I think that's intentional. Am I wrong, @avently?

[blackbox87]

If you mean that the back button pauses the video, but the full screen button keeps playing it when you go back to portrait, I think that's intentional.

Yes. But I'm not sure what else you'd mean?

If I do steps 1 & 2 and then press my back button it'll correctly change to portrait. And then if I want to go back to landscape all I need to do is tap on the fullscreen button again. That works perfectly for me on my device and emulators.

[opusforlife2]

Oh. I'll make a screenrecord, then.

[blackbox87]

@opusforlife2 This is what it looks like on my device and emulators.

I can only get it to do what you describe if I launch the unpatched dev version of NewPipe.

• Ignore how the video isn't centered. I'm currently testing changes for the other PR.

[opusforlife2]

ScreenRecord.mp4.gz

Here's my screen record. I have shown the problem happening twice.

[blackbox87]

@opusforlife2 What phone is that and which version of Android is it using?

At the moment I can only assume that it's a device specific issue. It's as if applying SCREEN_ORIENTATION_PORTRAIT works and then the device immediately reverts back to a user or sensor preference.

[opusforlife2]

Xiaomi Mi 5 running LOS 16 (Android 9).

[avently]

For that you need money of your own first. ( ;´༎ຶД༎ຶ`)

There is a demo mode included (in-app exchange), even zero balance on real exchange is ok for start learning:)

Which app, though?

Called Evraon. In Google Play already, so you can find it if you want.

[blackbox87]

@opusforlife2 Try this. It looks like the issue was caused by some existing code, which shouldn't be required.

app-debug.zip

[opusforlife2]

Worked! Thanks!

[blackbox87]

Yeah, it's just really janky. I don't know what can be done about it though, unless it was reverted to how it's done in v0.19.8.

This commit is ready for merging as it puts the unified player in a slightly better state than it was, but if anyone else can come up with a better solution then I'd close the PR.

[Stypox]

Thank you! The apk works fine on my 7.0 phone, on 10 phone and tablet emulator, and code looks good :-D
@avently could you confirm this can be merged?

[Stypox]

Actually, I have found an issue, probably unrelated to this PR: the fullscreen button is missing in tablets in portrait, thus requiring to rotate the phone to go to landscape (which for tablets does not mean fullscreen) and then press fullscreen. I don't know if this is intended behaviour, since it does make some sense. @avently @blackbox87 what do you think?

[blackbox87]

@Stypox Yea, it's unrelated to this PR. But I did noticed that issue myself, which is one of the reasons why I said that the rotation behaviour is "janky" at the moment.

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

Lines 926 to 929 in e0f02d4

	final boolean tabletInLandscape = DeviceUtils.isTablet(service) && service.isLandscape();
	final boolean showButton = videoPlayerSelected()
	&& (orientationLocked || isVerticalVideo || tabletInLandscape);
	screenRotationButton.setVisibility(showButton ? View.VISIBLE : View.GONE);

The code was added by @avently.

There's another issue too, which I don't think was fixed. And if you read the thread a little you'll see that most of these changes were made by avently to suit his personal taste and not make the unified players rotation behaviour match the original player.

[avently]

@Stypox

have found an issue, probably unrelated to this PR

Unrelated to this PR and @blackbox87 can't do anything with this (at least not with a correct way) until #4288 is merged. Because here after screen orientation change the player is null https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java#L1496

After performance PR is merged in this place the player will not be null after fragment creation (for the second time) so you'll be able to check fullscreen state correctly.

could you confirm this can be merged?

Unfortunately, no, this can't be merged in it's current state:

• @blackbox87 you added some custom behavior into toggleFullscreen() which is used many times in the app and the app doesn't expect such custom things. I prefer that such important methods should do as small piece of work as possible
• right now if you have locked orientation in landscape, and watching a video in landscape (fullscreen), and then long pressing on arrow down button (from top right corner), then pressing on background playback or popup button, you'll see some weird orientation changes
• since you probably need to rotate the orientation from landscape locked to portrait locked only in one place you can do it without code inside toggleFullscreen() but with adding the code to, for example, https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java#L791 (and in other places if needed)
• void restoreDefaultOrientation should be renamed to something like exitFromFullscreenIfNeeded

[blackbox87]

@avently Maybe you could try to do it yourself from #4288 since that includes orientation changes already?

I'm not familiar enough with the code, which is why it was done this way in the first place. And I don't agree with your vision for how any of this should behave, so whatever I do is probably going to conflict with how you want it done.

[avently]

@blackbox87 it looks like you always blame my code but unable to do better, am I right? Here and there I see that you disagree with me but can't give a better alternative. I'm ok with that and understand your intent - you want to create something great too, so it's fine. But take a look at the code and you can find a better way than already exists, I believe you can.

Maybe you could try to do it yourself

I can try to do everything but it's not what I want to do.

since that includes orientation changes already

It doesn't. It's about performance, not orientation.

I'm not familiar enough with the code

I wasn't familiar with the code when I started Unified player some years ago too.

so whatever I do is probably going to conflict with how you want it done

It's not true based on how I feel like about your code. It's pretty fine, no conflicts. You just missed some important context and I said about that. No problem here.

[blackbox87]

it looks like you always blame my code but unable to do better, am I right?

Here and there I see that you disagree with me but can't give a better alternative

I suggest alternatives, but I'm not about to put time into making pull requests while you're making pull requests that contain multiple fixes instead of separate pull requests for each fix. It creates too much conflict.

I don't like the changes that you've made to the rotation behaviour, but your changes keep getting merged and I don't have the time to go back and forth over issues. So this PR can either be accepted as it is (since it does fix the bug described) or closed.

[avently]

since it does fix the bug described

But the fact that it makes some more bugs it's ok for you, right?

[avently]

but I'm not about to put time into making pull requests while you're making pull requests that contain multiple fixes instead of separate pull requests for each fix. It creates too much conflict.

Your PR will be merged before my PRs so it's not an argument here. I will base my PRs on top of your changes.

[blackbox87]

But the fact that it makes some more bugs it's ok for you, right?

I'm fine with it, since #4154 was knowingly merged with issues. And you say that further changes will be required to fix the issue that @Stypox mentioned and the one I found, so someone else can do that once #4288 is also merged.

[avently]

I'm fine with it, since #4154 was knowingly merged with issues

It's not merged with issues, it didn't fix ALL issues.

And you say that further changes will be required to fix the issue

I said that it's not related to your PR and you shouldn't do anything with it.

And I already fixed the issue mentioned by @Stypox in my local branch on top of performance PR. I also fixed #4322. I just thinking about to upload the changes inside performance PR or to wait until it gets merged because it's unrelated to performance.

[blackbox87]

It's not merged with issues, it didn't fix ALL issues.

I posted about the issues with it before it was merged, which is why I say that it was knowingly merged with issues.

Like if I go back to you saying this...

right now if you have locked orientation in landscape, and watching a video in landscape (fullscreen), and then long pressing on arrow down button (from top right corner), then pressing on background playback or popup button, you'll see some weird orientation changes

I don't see any weird orientation changes, which is why I suggested that you should fix it yourself in #4288 since that already includes orientation changes.

I don't want to keep making changes to this PR and would honestly rather close it, since like I've already explained, I don't like how rotation is handled anymore. And at this point I have no desire to contribute anything more to the project and would rather help out with NewPipe Legacy where possible, since that might not support the unified player due to API requirements.

I also fixed #4322

I don't know if that would also fix the issue described here.

[avently]

@Stypox regarding the fullscreen button in tablets: perf-with-orientation.zip

This apk is built on top of performance PR. It will work on top of #4272 partially. So I can merge this code into #4272 but it will work fully only after merge of #4288. I don't have a time on waiting the merge of Performance PR so I think it's ok to merge into Small fixes2, what do you think?

[avently]

I don't like how rotation is handled anymore

What's you vision? How you would do all rotation logic?

[blackbox87]

What's you vision? How you would do all rotation logic?

#4288 (comment)

I want it to behave exactly as it did in v0.19.8. Even if that means that when auto-rotate is disabled you'll see an additional button.

As things are I'd rather use NewPipe Legacy or fork NewPipe and revert all of the unified player changes.

[avently]

@blackbox87 I fixed the issue that you tried to fix in this PR in a couple of lines of code. Do you want me to apply my changes to Small fixes2 or you'll want to find a solution yourself? You did correct things but in incorrect place.

As things are I'd rather use NewPipe Legacy or fork NewPipe and revert all of the unified player changes.

It's sad to hear that you don't like the unified player. I tried to make it awesome and useful. I believe that you'll find it useful too.

[blackbox87]

@blackbox87 I fixed the issue that you tried to fix in this PR in a couple of lines of code. Do you want me to apply my changes to Small fixes2 or you'll want to find a solution yourself? You did correct things but in incorrect place.

You can apply it to your PR and I'll close this one. That way everything is where you'd like it to be and it's easier for you to maintain.

It's sad to hear that you don't like the unified player. I tried to make it awesome and useful. I believe that you'll find it useful too.

As someone who often has auto-rotate disabled I just really don't like how the merged rotate/fullscreen button changes depending on the type of device you're using or if you're playing a vertical or horizontal video. It's very inconsistent and seems unintuitive to me.

It'll be interesting to see if others agree with me when an update goes out to everyone.

If I started a playlist, locked my device to landscape and then put my device down then I'd expect every video to honor my orientation choice by playing in landscape. If it doesn't do that then it might eventually get to a vertical video and suddenly the image on my screen is sideways, which requires me to alter how I'm holding the device. So that doesn't respect my orientation choice and it's going to annoy people.

Use NewPipe v0.19.8 with your orientation locked to portrait, start a mixed playlist and then press on NewPipe's button to change the orientation to landscape. It'll then respect your orientation choice and play every video in landscape until you exit.

[avently]

@blackbox87
Uploaded changes:
#4272 (comment)

@Stypox take a look too. Fixed the issue you mentioned.

Fix in one line: df98239#diff-57eb59a31e38fed8f965fb4a544e3b9fL1965

[blackbox87]

@avently It fixes what's described in my original post, but it doesn't fix the secondary issue.

• Enable auto-rotate and play a vertical video
• Rotate your device so that it's landscape and then disable auto-rotate
• Press the rotate/fullscreen button

You'd expect it to rotate the fullscreen video since that's what horizontal videos always do, but instead it keeps taking you in and out of fullscreen.

https://github.com/TeamNewPipe/NewPipe/pull/4254/files#diff-61ebd1c28e394d4dbe7ffd9c9402e610

That's why I put the fix where it is and even commented the code. It catches the issue and makes the button rotate the video instead, which is exactly what people want. Consistency!

[avently]

You'd expect it to rotate the fullscreen video since that's what horizontal videos always do, but instead it keeps taking you in and out of fullscreen.

I don't expect but if you do here is what should be done in order to support this use-case:

Replace this line

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

Line 809 in df98239

if (!isVerticalVideo) {

with:

if (!isVerticalVideo || (!DeviceUtils.isTablet(service) && service.isLandscape())) {

If this fixes the problem you mentioned and it will make you happy I'll make a new commit in smallFixes2. Let me know.

Don't expect that the vertical video will stay in fullscreen after rotation because it is fixed inside performance PR (not in smallFixes2).

[blackbox87]

I don't expect but if you do here is what should be done in order to support this use-case

If you disabled auto-rotate while watching a horizontal video then the button will rotate the video, but if you do the same thing while watching a vertical video in landscape then you'll be stuck in landscape as the button doesn't do the same thing. So this restores some consistency.

The lack of consistency with the unified player is why I said that it seems unintuitive to me earlier.

If this fixes the problem you mentioned and it will make you happy I'll make a new commit in smallFixes2. Let me know.

I did a quick test with the suggested change on my device and it seems to work. So if you merge that I'll close this PR.

[avently]

So this restores some consistency

Agree, Added the commit 9a0c2c4

Final code is a little bit different, this line 9a0c2c4#diff-61ebd1c28e394d4dbe7ffd9c9402e610R818

[blackbox87]

@avently I can't seem to trick it to get stuck in landscape, so it's working perfectly. Thanks.

I'll close this PR and hopefully they can get #4272, #4288 and #3178 merged soon.

[TobiGr]

@blackbox87 thanks for the effort!