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(android): Pass longpress delay to KeymanWeb #12185

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

darcywong00
Copy link
Contributor

@darcywong00 darcywong00 commented Aug 14, 2024

Follows #12170 and fixes #877

This adds KMManager APIs to get/set the longpress duration between 300 ms and 1500 ms. Default KeymanWeb longpress delay is 500ms.

Since the keyboard needs to be loaded for the setting to take effect, I put the initialization in MainActivity.onKeyboardLoaded

User Testing

Setup - Install the PR build of Keyman for Android on a device/emulator
Note: the reported issue on flick gestures is handled on a separate PR #12187

TEST_LONGPRESS - Verifies longpress delay changes with the menu

  1. Launch Keyman for Android and dismiss the "Get Started" menu
  2. On the default SIL EuroLatin keyboard, longpress a key and observe the long press delay
  3. Verify long press appears after about 0.5 seconds.
  4. From Keyman Settings --> Adjust longpress delay
  5. From the "Adjust longpress delay" menu, increase the delay to 1.5 seconds and exit the menu
  6. On the Keyman keyboard, longpress a key and observe the long press delay
  7. Verify long press appears after about 1.5 seconds.
  8. From Keyman Settings --> Adjust longpress delay
  9. From the "Adjust longpress delay" menu, decrease the delay to 0.3 seconds and exit the menu
  10. On the Keyman keyboard, longpress a key and observe the long press delay
  11. Verify long press appears after about 0.3 seconds

@darcywong00 darcywong00 requested a review from sgschantz as a code owner August 14, 2024 11:55
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Aug 14, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 14, 2024

@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman 18.0.89-alpha-test-12185" build on the Android 14 physical device and Android 9(emulator) Here is my observation.

  • TEST_LONGPRESS (failed):
  1. Installed the "Keyman-18.0.89.apk" file and gave all permissions to the application.
  2. Checked the "Enable Keyman as system-wide keyboard" and set the keyboard as the default keyboard box on the settings page.
  3. Selected the "SIL EuroLatin" default keyboard.
  4. Open the Keyman app. Enable the "Predictions" to install the "Dictionary."
  5. Added a sentence to the notepad and Chrome (search).
  6. "Adjust long-press delay" appears under the settings.
  7. Changed the delay as 03s or 1.5s
  8. Press the key and then the sub-menu appears based on the delay set in the "Adjust long-press delay"
    The delay works after pressing the keys.
    Press the "q,w,e,r,t" key and pull down to select the number. here, The number does not appear after the pull-down. Hence, unable to select the numbers & special characters by pressing the key and pull down. So, I failed this test case. Thank you.
    Please refer to the attached video below.
    https://github.com/user-attachments/assets/5b4694d9-134e-409d-9f63-d47389b093b3
    Thanks.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Aug 14, 2024
@darcywong00
Copy link
Contributor Author

darcywong00 commented Aug 15, 2024

@jahorton - Dina and I have seen changing the longpress delay from the default 0.5 seconds to 0.3 seconds seems to prevent the flick down gesture from activating. Is this a side effect in KeymanWeb?

From my testing, immediately trying to flick down is triggering a longpress from the row below (e.g. flick down from f is triggering longpresses on the c key)

When I set longpress delay to 0.7 seconds, flick gestures are still disabled. Gonna retest on master...

@darcywong00
Copy link
Contributor Author

... so I can't get the down flick gestures to work on master branch either. Maybe a side effect from #12176?

jahorton
jahorton previously approved these changes Aug 15, 2024
@jahorton
Copy link
Contributor

... so I can't get the down flick gestures to work on master branch either. Maybe a side effect from #12176?

That it was. See #12187, which will fix it.

@darcywong00
Copy link
Contributor Author

While I started to update the KMManager API on help.keyman.com, I realized I needed to do a slight refactor to separate setLongpressDelay (store the new preference, then calls applyLongpressDelay) and applyLongpressDelay (sends the updated value to KeymanWeb)

The flick issue will resolve after merging #12187
@keymanapp-test-bot retest

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Aug 15, 2024
@darcywong00 darcywong00 marked this pull request as draft August 15, 2024 03:30
@darcywong00 darcywong00 dismissed jahorton’s stale review August 15, 2024 03:31

Added some refactoring

@mcdurdin
Copy link
Member

While I started to update the KMManager API on help.keyman.com, I realized I needed to do a slight refactor to separate setLongpressDelay (store the new preference, then calls applyLongpressDelay) and applyLongpressDelay (sends the updated value to KeymanWeb)

That was going to be my first comment on the PR 😁 -

@darcywong00 darcywong00 marked this pull request as ready for review August 15, 2024 06:27
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

Basically LGTM, just want to move ownership of the longpressDelay value entirely into KMManager.

Comment on lines 83 to 84
int longpressDelay = KMManager.getLongpressDelay(this);
KMManager.setLongpressDelay(longpressDelay);
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of an anti-pattern. We shouldn't be reading from KMManager's longPressDelay property and then writing back to it -- that looks like a no-op. Instead if we need to tell it to refresh we should be doing that with something like KMManager.sendLongpressDelay() (or perhaps better, KMManager.sendOptionsToKeyboard())

Comment on lines 248 to 250
// Initialize the longpress delay
int longpressDelay = KMManager.getLongpressDelay();
KMManager.applyLongpressDelay(longpressDelay);
Copy link
Member

Choose a reason for hiding this comment

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

I am not very comfortable with this. KMManager should be able to manage the value internally, and we shouldn't be passing the value back and forth. If we need to ask KMManager to re-apply the longpressDelay that it already knows, it should be just something like:

Suggested change
// Initialize the longpress delay
int longpressDelay = KMManager.getLongpressDelay();
KMManager.applyLongpressDelay(longpressDelay);
KMManager.applyLongpressDelay();

@@ -472,7 +472,7 @@ public boolean onKeyUp(int keycode, KeyEvent e) {

@Override
public void onKeyboardLoaded(KeyboardType keyboardType) {
// Do nothing
checkLongpressDelay();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
checkLongpressDelay();
KMManager.applyLongpressDelay();

Comment on lines 753 to 758
private void checkLongpressDelay() {
// Initialize the longpress delay
int longpressDelay = KMManager.getLongpressDelay();
KMManager.applyLongpressDelay(longpressDelay);
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private void checkLongpressDelay() {
// Initialize the longpress delay
int longpressDelay = KMManager.getLongpressDelay();
KMManager.applyLongpressDelay(longpressDelay);
}

This should not be needed if KMManager.applyLongpressDelay() does the work

Comment on lines 118 to 121
if (keyman.osk) {
keyman.osk.gestureParams.longpress.waitLength = delay;
console.debug('setLongpressDelay('+delay+')');
}
Copy link
Member

Choose a reason for hiding this comment

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

If keyman.osk is not assigned, we may need to log that?

*/
public static void setLongpressDelay(int longpressDelay) {
public static void applyLongpressDelay(int longpressDelay) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static void applyLongpressDelay(int longpressDelay) {
public static void applyLongpressDelay() {
int longpressDelay = getLongpressDelay();

editor.putInt(KMKey_LongpressDelay, longpressDelay);
editor.commit();

applyLongpressDelay(longpressDelay);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
applyLongpressDelay(longpressDelay);
applyLongpressDelay();

@darcywong00 darcywong00 linked an issue Aug 16, 2024 that may be closed by this pull request
@dinakaranr
Copy link

Test Results

  • TEST_LONGPRESS (Passed):
    I tested this issue with the attached "Keyman 18.0.89-alpha-test-12185" build on the Android 14 physical device and Android 12(emulator) Here is my observation.
  1. Installed the "Keyman-18.0.89.apk" file and gave all permissions to the application.
  2. Checked the "Enable Keyman as system-wide keyboard" and set the keyboard as the default keyboard box on the settings page.
  3. Selected the "SIL EuroLatin" default keyboard.
  4. Open the Keyman app. Enable the "Predictions" to install the "Dictionary."
  5. Added a sentence to the notepad and Chrome (search).
  6. "Adjust long-press delay" appears under the settings.
  7. Changed the delay as 03s or 1.5s
    Press the key and then the sub-menu appears based on the delay set in the "Adjust long-press delay". The delay works after pressing the keys.
    I have tested the PR 12187. The gesture works well when pressing & pulling down the key. I can select the numeric and special characters. It works when handing the multiple fingers.
    Hence, I resolved this task too. Thank you.

@dinakaranr
Copy link

Test Results

  • TEST_LONGPRESS (passed):
    I retested this issue with the attached "Keyman 18.0.92-alpha-test-12185" build on the Android 14 physical device and Android 12(emulator) Here is my observation.
  1. Installed the "Keyman-18.0.92.apk" file and gave all permissions to the application.
  2. Checked the "Enable Keyman as system-wide keyboard" and set the keyboard as the default keyboard box on the settings page.
  3. Selected the "SIL EuroLatin" default keyboard.
  4. Open the Keyman app. Enable the "Predictions" to install the "Dictionary."
  5. Added a sentence to the notepad and Chrome (editpad.org).
    Press the key and then the sub-menu appears based on the delay that is set in the "Adjust long-press delay".
    The delay works after pressing the keys.
    I can select the numeric and special characters. It works when handing the multiple fingers. Hence, I resolved this task. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 19, 2024
Base automatically changed from feat/android/longpress-delay-menu to master August 20, 2024 14:02
@darcywong00 darcywong00 merged commit 947bf19 into master Aug 20, 2024
5 checks passed
@darcywong00 darcywong00 deleted the feat/android/kmea-set-longpress-delay branch August 20, 2024 14:02
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.93-alpha

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

Successfully merging this pull request may close these issues.

feat(android/app): Feature Request: customize long-press delay time
5 participants