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

Portrait Orientation #45

Open
xmha97 opened this issue Dec 28, 2022 · 7 comments
Open

Portrait Orientation #45

xmha97 opened this issue Dec 28, 2022 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@xmha97
Copy link

xmha97 commented Dec 28, 2022

Please support portrait mode.

@pserwylo
Copy link
Collaborator

pserwylo commented Dec 28, 2022

Thanks for the feedback @xmha97. Although technically possible, would you be able to please elaborate on the rationale?

Sometimes apps like this are often best kept very simple, instead of adding settings to allow each different option. My opinion is that a keyboard app is almost always going to want to be used in landscape (this is often the case with certain games too).

In order to support portrait mode, typically apps should follow the orientation preferences of the device, but that means that most users will see a portrait mode by default and need to subsequently rotate the phone (which will be the most commonly desired orientation). It also means kids will accidently change orientation as they play with the app.

The other option is to have a setting to explicitly set portrait or landscape mode. This is also non-intuitive for many (because the default expectation is that apps rotate with the phone). It also adds to the list of settings the app supports.

If there is a compelling use case presented then it would be great to discuss further, but otherwise, I'd personally prefer to close this.

@xmha97
Copy link
Author

xmha97 commented Dec 28, 2022

The only reason I have for adding this feature is taste. I prefer to use this app in portrait mode.

For the piano app, it may not be appropriate to follow the sensor and it needs to be rotated by pressing a button, Like this app:

Screenshot_2022-12-28-08-44-36-176_jp.genit.verticalpiano.jpg

Screenshot_2022-12-28-08-44-30-418_jp.genit.verticalpiano.jpg

Anyway, if you find that the portrait mode is hard to implement, ignore it and close this issue.

@pserwylo
Copy link
Collaborator

Ah, I see. Yes, the author of that app has the same opinion as both of us that following device rotation is not ideal and it needs to be explicit.

Given this app is designed for children, rather than perhaps the screenshots you have which may be more designed at those trying to play more complex and engaging piano pieces, I'd still recommend we keep it simple.

However, I'll leave it to @nicolasbrailo to perhaps cast a vote. @nicolasbrailo please let us know what you think if you get the chance. If you'd like to remain with the status quo, we can probably close this off. Otherwise lets leave it open in case people in the community are able to address with a contribution.

@nicolasbrailo
Copy link
Owner

Hi @xmha97. Thanks for the feedback, it's always appreciated.

As Peter said, we want to keep this app simple. Having said that, I wouldn't be against a PR to add portrait mode support, as long as

  1. It's explicitly set by a user in the config screen (or maybe a selection out of three options, landscape, portrait and follow sensor)
  2. The default remains landscape
  3. We can ensure there are enough sounds to support more keys
  4. The color palette can be extended in a way that makes sense (we're not doing anything particularly smart about now, and I worry in portrait mode we'll end up with layouts that have all the same color in a column)

1 should be simple, as Peter redesigned the settings screen and it looks fairly easy to extend now. 3 may be the hardest, as the sounds are not dynamically generated: there's a file for each note being played, and if we need more notes then we need to re-generate the entire sound set. I don't know how hard #4 may be.

For the actual change, you'd probably need to change most of the implementation of Piano.java, as I'm sure a lot of the code there assumes a single key per column. I would also advise looking at this PR, to understand the complicated use cases we support with multi finger touches (when I say we I really mean Jules, the contributor of that patch, as I just botched the original implementation horribly)

All in all, portrait support is a pretty major change, and while I'd welcome a PR for it I don't think it's something I'll be working on anytime soon. @xmha97, if you would like to submit a PR, I'd be happy to help you plan it.

@xmha97
Copy link
Author

xmha97 commented Dec 29, 2022

Thank you for your explanation and welcome to the portrait mode.
I would really like to contribute, but I'm not an Android developer.
Also, at first, I didn't know that this app was made for children, and I think that landscape mode is more suitable for them.

@nicolasbrailo
Copy link
Owner

@xmha97 it's awesome you found the app useful beyond keeping children entertained. Being an app for children, some features probably make a lot more sense (like pinning the app, and making the configuration hard to get to)

I would really like to contribute, but I'm not an Android developer.

If you'd like to change that, we can get you some getting-started pointers. I may not the best person to ask (I'm also very far from being an Android developer!) but I'm sure I can find ways to get you started.

@xmha97
Copy link
Author

xmha97 commented Dec 29, 2022

If you'd like to change that, we can get you some getting-started pointers. I may not the best person to ask (I'm also very far from being an Android developer!) but I'm sure I can find ways to get you started.

I see the app source code in my free time and if I have any questions, I will ask you in this issue. But it is possible that I will not be able to contribute and you should consider this point.

@juleskers juleskers added enhancement New feature or request help wanted Extra attention is needed labels Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants