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

Lock the orientation on Android and iOS devices to landscape #10347

Closed
wants to merge 3 commits into from

Conversation

tytydraco
Copy link
Contributor

Issue

The portrait orientation of osu! in the menu is stretched and overlaps itself.

  • The menu is cut off on either side
  • Auto rotations are very annoying

Photos

https://photos.app.goo.gl/G5d3h8G6uGDTqZ6g7

Solution

Restrict the orientation of the Android and iOS builds from FullSensor (aka all four orientations regardless of rotation lock) to Landscape.

This fixes the horrible menu UI stretching when the device is too narrow.
This fixes the horrible menu UI stretching.
@peppy
Copy link
Member

peppy commented Oct 3, 2020

This isn't what we want. For a starters, tablets play in portrait just file. The eventual goal is to fix remaining overlapping elements and allow the app to run in either orientation.

Does your device not have a rotation lock function?

@enoslayd
Copy link

enoslayd commented Oct 3, 2020

This isn't what we want. For a starters, tablets play in portrait just file. The eventual goal is to fix remaining overlapping elements and allow the app to run in either orientation.

Does your device not have a rotation lock function?

in my device, even if it is locked to portrait it still rotates

@bdach
Copy link
Collaborator

bdach commented Oct 3, 2020

Additionally, please note that we strongly discourage making contributions to code via the github web interface, as in our experience they tend to correlate with untested changes. Please read our contributing guidelines if you haven't already.

@tytydraco
Copy link
Contributor Author

This isn't what we want. For a starters, tablets play in portrait just file. The eventual goal is to fix remaining overlapping elements and allow the app to run in either orientation.
Does your device not have a rotation lock function?

in my device, even if it is locked to portrait it still rotates

That's because they use FullSensor.

@tytydraco
Copy link
Contributor Author

Additionally, please note that we strongly discourage making contributions to code via the github web interface, as in our experience they tend to correlate with untested changes. Please read our contributing guidelines if you haven't already.

I read it, it was just convenient for me. I double-checked my changes in the CLI before pushing.

@peppy
Copy link
Member

peppy commented Oct 3, 2020

I can confirm rotation lock works as expected on iOS without this change, so it's definitely not required there. Plus there are valid use cases for portrait, as mentioned, on tablet devices.

@tytydraco
Copy link
Contributor Author

This isn't what we want. For a starters, tablets play in portrait just file. The eventual goal is to fix remaining overlapping elements and allow the app to run in either orientation.

Does your device not have a rotation lock function?

Ah I see. However, does it not seem awkward in the screenshots? Regardless of the overlapping elements, the menu looks spare.

Also, yes, my device has a rotation lock function. However, as per Google's documentation:

"sensor" | The orientation is determined by the device orientation sensor. The orientation of the display depends on how the user is holding the device; it changes when the user rotates the device. Some devices, though, will not rotate to all four possible orientations, by default. To allow all four orientations, use "fullSensor" The sensor is used even if the user locked sensor-based rotation.

"fullSensor" | The orientation is determined by the device orientation sensor for any of the 4 orientations. This is similar to "sensor" except this allows any of the 4 possible screen orientations, regardless of what the device will normally do (for example, some devices won't normally use reverse portrait or reverse landscape, but this enables those). Added in API level 9.

It appears you would be better off using fullUser:

"fullUser" | If the user has locked sensor-based rotation, this behaves the same as user, otherwise it behaves the same as fullSensor and allows any of the 4 possible screen orientations. Added in API level 18.

@tytydraco
Copy link
Contributor Author

I can confirm rotation lock works as expected on iOS without this change, so it's definitely not required there. Plus there are valid use cases for portrait, as mentioned, on tablet devices.

iOS would work fine, the rotation lock is controlled system-side and the application cannot override it. See my most recent reply above.

@peppy
Copy link
Member

peppy commented Oct 3, 2020

I'm less familiar with the android API, but fullUser does sound like what we want, if it's what is required to allow user rotation lock overrides.

@tytydraco
Copy link
Contributor Author

I'm less familiar with the android API, but fullUser does sound like what we want, if it's what is required to allow user rotation lock overrides.

Would you like me to close this pull request and make a new one for fullUser? I wouldn't mind at all.

@tytydraco
Copy link
Contributor Author

Additionally, please note that we strongly discourage making contributions to code via the github web interface, as in our experience they tend to correlate with untested changes. Please read our contributing guidelines if you haven't already.

P.S.: I suggest you mandate non-web made commits. I read the message in the discord, it's nothing like "this code is untested, test it for me", it's more like I'd prefer to not whip out VS to edit 3 lines haha. I see where you're coming from, but in the contributing file, I don't think the word "discourage" gets the point across.

@peppy
Copy link
Member

peppy commented Oct 3, 2020

Just making changes in this commit would have been fine, but it'd be appreciated if you make a non-web version for everyone's sanity :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants