feat(android): Add Grabbable Bar and settings status for keyboard resizing#15267
Conversation
User Test Results |
|
This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR. |
|
I knew it was coming, but I was quite surprised when KMManager had moved on master when I went to rebase! |
|
@mcdurdin Do we need to port this over to the FV app or KmSample2? |
I don't think so. Those apps don't have a menu to adjust the keyboard height. |
I'll update the PR description with a template for user testing. @MattGyverLee - you can adjust the test steps |
mcdurdin
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
I'd like this to be refactored to be much simpler and not make changes to KMManager. As it stands, it's adding a large amount of API surface to Keyman Engine for very little benefit. The five KMManager functions added are all trivial utility functions, which will increase the future maintenance burden for Keyman Engine for Android.
As a basic principle, API surfaces should be kept as simple and clean as possible, and utility functions should not be added to general APIs. Part of the problem here is that years ago we were not particularly careful with a lot of the API surfaces across Keyman as a whole. But that has caused ongoing problems with maintenance of Keyman Engine for Android. So where we are now, we don't want to perpetuate the problem further.
-
In my opinion, the function
calculateKeyboardHeightFromTouchis just unnecessary. It's a totally trivial calculation and is called just once; it takes 5 lines out of that single use and turns it into nearly 150 lines of very verbose documentation and code. -
The function
createKeyboardHeightStringdoes not belong in KMManager -- this is an app presentation detail. Furthermore, the string itself needs to be localizable as a whole with parameterization. I am also uncomfortable with the 'live' variant of the function -- that parameterization should be in a higher level function that callscreateKeyboardHeightString. -
Similarly
getKeyboardHeightPercentageis a trivial calculation. It could be a utility function but it should not be in Keyman Engine.
android/KMEA/app/src/main/java/com/keyman/engine/KMManager.java
Outdated
Show resolved
Hide resolved
android/KMAPro/kMAPro/src/main/res/drawable/resize_handle_background.xml
Outdated
Show resolved
Hide resolved
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
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
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
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
|
Thanks. The refactoring should be easy, those functions got moved to KMManager on one of the last commits. |
|
Ok, I just cut out a LOT of spaghetti code. Ready for another look. |
mcdurdin
left a comment
There was a problem hiding this comment.
This looks great, thank you for your contribution -- nice improvement which also retires a bit of tech debt.
android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/AdjustKeyboardHeightActivity.java
Show resolved
Hide resolved
android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/AdjustKeyboardHeightActivity.java
Show resolved
Hide resolved
android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/AdjustKeyboardHeightActivity.java
Outdated
Show resolved
Hide resolved
android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/AdjustKeyboardHeightActivity.java
Outdated
Show resolved
Hide resolved
android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/AdjustKeyboardHeightActivity.java
Outdated
Show resolved
Hide resolved
android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/AdjustKeyboardHeightActivity.java
Show resolved
Hide resolved
|
Note: this should probably be merged as a squash merge as it is an external contribution. |
Co-authored-by: Marc Durdin <marc@durdin.net>
…ustKeyboardHeightActivity.java Co-authored-by: Marc Durdin <marc@durdin.net>
…ustKeyboardHeightActivity.java Co-authored-by: Marc Durdin <marc@durdin.net>
|
Thanks! Back to you. |
you'll need to fix the call to |
…dback' into feat/grabbable-bar-and-feedback
|
Better? I learn a lot about improving code from your reviews. |
Awesome! Glad you are finding my feedback constructive 😁 Sadly, build failed: I am out of time to do any more review; I will be unavailable until January as I am travelling back to Australia tomorrow, @darcywong00 will take over code review from here. |
|
I had actually intended to merge those strings into one text box. Merged now with language-adaptive punctuation. @darcywong00 I hope this has also resolved the build error. |
|
@Meng-Heng - I'm not sure why the build artifacts weren't attached. |
|
I'll test this in the afternoon (my time). |
Network glitch potentially, or status.keyman.com hiccups. We'll monitor in case this is an ongoing issue. |
Test Specs
Test Results
|
|
Changes in this pull request will be available for download in Keyman version 19.0.173-alpha |
As dicussed on slack, I've ported the "grabbable" bar for keyboard resizing from the App builders. The bar is animated when you touch it. I've tested this in a build of the Keyman App. Resizing seems to work well after build, even though the app keeps giving (hopefully) unrelated errors about a null language. Oh, I set the % text on the resize screen to grey after I took the screenshots and merged the text boxes at the top.
User Testing
Setup - Install the PR build of Keyman for Android on a device/emulator
a. Drag the bar to adjust the height, rotate the phone and adjust again
b. Try the reset menu, then adjust to a non 100% height.
b. Verify the corresponding portrait/landscape % updates
Note: The settings should "stick" if you tap the back button on the top bar, the phone back button, or switch apps.
Note: The top of the keyboard image in the dialog should line up with the top of the keyboard. Both the draggable bar and the suggestion bar will show above that line.