Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat (player): optional rotate button in player and setting to enable auto rotate based on video orientation #813

Merged
merged 1 commit into from Dec 19, 2023

Conversation

ghost
Copy link

@ghost ghost commented Dec 13, 2023

  • icon added
  • Strings might be incorrect
  • code needs review

setting page:
image

with rotate enabled:
image

@fire-light42
Copy link
Collaborator

This is a really good addition, and the code looks nice. However it fucks up a bit with the current code located at

open fun lockOrientation(activity: Activity) {
that keeps the player locked in LANDSCAPE when clicking the lock symbol.

So if you want this merged you can keep this behavior however you need to fix the updateOrientation function to not cause any bugs. By pressing lock you lock the current orientation completely otherwise you do sensor landscape/portrait.

@ghost
Copy link
Author

ghost commented Dec 14, 2023

@fire-light42 can you share some desc on how updateorientarion and lockorientation is working? when are they triggered?

@ghost
Copy link
Author

ghost commented Dec 14, 2023

where can I get the icons?

@fire-light42
Copy link
Collaborator

@fire-light42 can you share some desc on how updateorientarion and lockorientation is working? when are they triggered?

Right now when the user enters the player they will be turned into sensor landscape and when pressing the lock button it will lock to the exact orientation that the player is currently in. updateOrientation is called when entering the player along with when you toggle the lock button.

So in your case you need to add a bool like IsVerticalOrientation that is set when first loading a video by overriding playerDimensionsLoaded, then calling updateOrientation. updateOrientation should lock the player in the exact orientation the user is currenty in if isLocked otherwise it should use the sensor version of IsVerticalOrientation. Then the button just changes IsVerticalOrientation = !IsVerticalOrientation; updateOrientation();

If you want icons then look at https://fonts.google.com/icons

image
https://developer.android.com/guide/topics/manifest/activity-element.html#screen

@ghost
Copy link
Author

ghost commented Dec 16, 2023

thank you for explaining in detail.

@ghost ghost changed the title feat (player): optional rotate button in player and setting to enable auto rotate based on aspect ratio feat (player): optional rotate button in player and setting to enable auto rotate based on video orientation Dec 16, 2023
@ghost
Copy link
Author

ghost commented Dec 16, 2023

@fire-light42 changes are done. pls review.

Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, however I have not tested this yet so you might get some more feedback soon

ActivityInfo.SCREEN_ORIENTATION_REVERSE_LANDSCAPE

Configuration.ORIENTATION_SQUARE, Configuration.ORIENTATION_UNDEFINED ->
dynamicOrientation(activity)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause a bug as orientation will still be zero and 0 = SCREEN_ORIENTATION_LANDSCAPE, so this function will be called then it will call activity.requestedOrientation = SCREEN_ORIENTATION_LANDSCAPE right under this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have the same problem in the other functons

Copy link
Author

@ghost ghost Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it was in lockOrientation -> Configuration.ORIENTATION_SQUARE, Configuration.ORIENTATION_UNDEFINED, Configuration.ORIENTATION_PORTRAIT -> ActivityInfo.SCREEN_ORIENTATION_SENSOR_LANDSCAPE
Is this doing anything? there is no assignment.

Now in function i have added dynamicOrientation in that it will set-> activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_SENSOR_LANDSCAPE

So what should be the change? should I keep it how it was? how to test this Configuration.ORIENTATION_SQUARE, Configuration.ORIENTATION_UNDEFINED?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that if you read the code
image
Then it will set activity.requestedOrientation = orientation and orientation = 0 when currentOrientation = Configuration.ORIENTATION_SQUARE, Configuration.ORIENTATION_UNDEFINED so dynamicOrientation wont do anything as it will be overridden 2 lines later

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think dynamicOrientation should return what orientation it should be and then set orientation = dynamicOrientation(activity) inside the when statments

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohk I missed that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fire-light42 thanks. Pls chk now.

@ghost
Copy link
Author

ghost commented Dec 18, 2023

@fire-light42 i have fixed the issue you've shared. pls review and let me know if there is any other issue.

@Mostafazarzora
Copy link

Mostafazarzora commented Dec 18, 2023 via email

Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, tested it now and it worked like a charm! 👍

@fire-light42 fire-light42 merged commit a5f7920 into recloudstream:master Dec 19, 2023
2 checks passed
@ghost ghost deleted the player-rotate branch December 20, 2023 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants