-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
fix(android): Resize keyboard on orientation change #11484
Conversation
User Test ResultsTest specification and instructions
Test Artifacts |
android/KMEA/app/src/main/java/com/keyman/engine/KMManager.java
Outdated
Show resolved
Hide resolved
android/KMEA/app/src/main/java/com/keyman/engine/KMManager.java
Outdated
Show resolved
Hide resolved
|
||
this.dismissHelpBubble(); | ||
DisplayMetrics dms = context.getResources().getDisplayMetrics(); | ||
int kbWidth = (int) (dms.widthPixels / dms.density); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific line - kbWidth
- has two other instances in the codebase already. This simply makes a third.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than approving these changes, I think I will install and use this PR build on my phone for a few days. I experience the OSK height issues most days, so that should be enough to tell us if the patch improves the situation.
Test ResultsI tested this issue with the attached "Keyman 18.0.39-alpha-test-11484" build on an Android 12 & 13 & 14 emulator & physical environment: Here is my observation.
|
sadly... definitely worse than before. This update doesn't come good if I switch to another keyboard and back, unlike 17.0.325. Rotation and restore are both broken. In 17.0.325, rotation seems fine, and it is only restore that seems to sometimes go wrong. So I think we need to leave the rotation handler alone and look at the restore handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as 'request changes' to take it out of the review queue -- given the issues noted in my earlier comments 😁
This information looks to be very useful; that's more description than I think I had before. Upon investigating the current code for I happened to notice there's specific handling for this in-app that lacks a clear parallel in the system keyboard, and your answer would likely help clarify that and point me in the right direction. |
I almost never use the in-app keyboard so don't have data to answer this question sorry.
I've now linked this PR and #11604 to #10054 (the original issue I opened on this problem) for posterity. #10054 said this:
|
We're having far better success with #11604, so I'm closing this PR out now. |
This PR attempts to resolve the tricky wrong-keyboard-size bug that continues to occur within our Android app, despite our best efforts.
After analyzing the codebase, I feel that there are two different types of orientation-change triggers within the code:
onResume
, when restoring the app/keyboard from the background. We just need to set whatever the current orientation is in place, even if it changed since being backgrounded.onConfigurationChanged
, during an actual orientation-change event.The solution for point 1 - reliant on the
Display
-based pathway, seems to work quite well for reading the current orientation when it's not being mutated.For point 2, we currently ignore the provided parameter for
onConfigurationChange
in favor of reusing the same solution as is used foronResume
. (This seems to have been established in either #10373 or #10442.) We used to read it fromactivity.getResources().getConfiguration().orientation
instead... until we found that sometimes reading it specifically from that consistent source could be prone to a known bug on certain devices. Interestingly, the external references I could find to that bug... require accessing it in that manner, rather than fromonConfigurationChange
's parameter.keyman/android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/MainActivity.java
Lines 268 to 276 in 4718b5b
.orientation
field was explicitly referenced by the equivalent to the block above.If we instead use the value directly from the
onConfigurationChanged
parameter when possible, perhaps that may allow us to dodge that particular bug issue entirely. It's worth noting that this is the "standard solution" I found, repeatedly, when searching for how to detect device orientation - usingonConfigurationChanged
and the.orientation
field of its parameter.Meanwhile, my current "best guess" for the current problem is that the
onResume
pathway, reliant uponDisplay
- isn't intended for use during an orientation change. If it's updated in a manner susceptible to race-conditions, we could be getting the old orientation via theDisplay
-based pathway when the "race" is "lost". It's hard to say for sure without thorough testing and/or a solid repro that could be used as proof, though. I'm not seeing much external mention of this as a possibility, but this is the possibility that makes most sense to me via process of elimination.User Testing
TEST_OSK - Verifies OSK is full width after using keyboard picker
TEST_LANDSCAPE_KEYBOARD_PICKER - Verifies menu to adjust keyboard height works
TEST_OSK_ROTATION_ANDROID_12
TEST_OSK_ROTATION_ANDROID_13