vitorpamplona / amethyst

Nostr client for Android
MIT License
1.17k stars 158 forks source link

Change tap to fullscreen video behavior for livestreams. #1018

Closed KotlinGeekDev closed 3 months ago

KotlinGeekDev commented 3 months ago

The aim of the PR is to fix #539 , while taking into account other devices like foldables.

KotlinGeekDev commented 3 months ago

Please @vitorpamplona I don't know if it's a good idea to include Composable functions in the Quartz module. They are not UI-related, just utlilty functions.

vitorpamplona commented 3 months ago

Yeah, we should probably keep the utility function in the same package as the videoview

KotlinGeekDev commented 3 months ago

Yeah, we should probably keep the utility function in the same package as the videoview

Got it.

KotlinGeekDev commented 3 months ago

@vitorpamplona The PR is all set and ready.

vitorpamplona commented 3 months ago

Thank you @KotlinGeekDev

vitorpamplona commented 3 months ago

@KotlinGeekDev we need a fix for portrait videos. The device should not go to landscape on fullscreen if the video is portrait. Many videos in the video tab are like that.

Feature disabled for now: https://github.com/vitorpamplona/amethyst/commit/989cbb35d3228861dfb1fc7d3801eae4b6d97b21

KotlinGeekDev commented 3 months ago

@vitorpamplona does it mean we should just go to fullscreen only when the user willingly changes orientation?

vitorpamplona commented 3 months ago

It should go full screen all the time, but only rotate to landscape when the video is in landscape. Otherwise, the portrait video shows in landscape an only uses a tiny space in the middle of the screen

KotlinGeekDev commented 3 months ago

It should go full screen all the time, but only rotate to landscape when the video is in landscape. Otherwise, the portrait video shows in landscape an only uses a tiny space in the middle of the screen

I see. So basically the same behsviour as before?

vitorpamplona commented 3 months ago

If the video is portrait, then yes, same behavior as before. If the video is in landscape, then it should change orientation

KotlinGeekDev commented 3 months ago

If the video is portrait, then yes, same behavior as before. If the video is in landscape, then it should change orientation

Got it.

KotlinGeekDev commented 3 months ago

Please @vitorpamplona it therefore means disabling it as you did will be enough to fix the issue you brought up, right?

vitorpamplona commented 3 months ago

Disabling like that disables the entire PR, right? It won't rotate for landscape videos, when it should.

KotlinGeekDev commented 3 months ago

Disabling like that disables the entire PR, right? It won't rotate for landscape videos, when it should.

Interesting. From my experience testing the effects of disabling, it works even in landscape mode.

KotlinGeekDev commented 3 months ago

@vitorpamplona My understanding of the original issue is that the author wanted the NewPipe app experience, whereby, on portrait devices, tapping the fullscreen button changes the orientation to landscape, then going fullscreen. Please @anthony-robin correct me if I am wrong.

vitorpamplona commented 3 months ago

Interesting. From my experience testing the effects of disabling, it works even in landscape mode.

But how would it work if it is not calling any function to change orientation? In my device, it now doesn't change any orientation at all.

KotlinGeekDev commented 3 months ago

Interesting. From my experience testing the effects of disabling, it works even in landscape mode.

But how would it work if it is not calling any function to change orientation? In my device, it now doesn't change any orientation at all.

@vitorpamplona I think that the way the fullscreen experience currently works(the way it worked before my PR) is that the orientation was manually set by the user in the system settings, which affected the app.

anthony-robin commented 3 months ago

@vitorpamplona My understanding of the original issue is that the author wanted the NewPipe app experience, whereby, on portrait devices, tapping the fullscreen button changes the orientation to landscape, then going fullscreen. Please @anthony-robin correct me if I am wrong.

Hi @KotlinGeekDev, the original issue was about a NewPipe experience (that might not be the only app that does this but the one I had in mind when opening the issue) by having an auto fullscreen video mode when I turn my phone in a landscape position on a "Live stream" video page.

Also note that when the landscape fullscreen is turned back to portrait, fullscreen should be disabled back to the original rendering (if it is possible)

I didn't really thought about video that are recorded in portrait mode directly so I think it is better to keep it in fullscreen portrait with a good video rendering and not a tiny content with lot of black on horizontal sides.

Let me know if you have more questions or if things are not clear enough. Anyway, thank you for working on the subject :)

KotlinGeekDev commented 3 months ago

Hi @KotlinGeekDev, the original issue was about a NewPipe experience (that might not be the only app that does this but the one I had in mind when opening the issue) by having an auto fullscreen video mode when I turn my phone in a landscape position on a "Live stream" video page.

Also note that when the landscape fullscreen is turned back to portrait, fullscreen should be disabled back to the original rendering (if it is possible)

I didn't really thought about video that are recorded in portrait mode directly so I think it is better to keep it in fullscreen portrait with a good video rendering and not a tiny content with lot of black on horizontal sides.

Let me know if you have more questions or if things are not clear enough. Anyway, thank you for working on the subject :)

Oh @anthony-robin got it! Thanks for making it clearer.