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

docs: Update API's for adjusting Android longpress delay #1500

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

darcywong00
Copy link
Contributor

Documents Keyman Engine for Android 18.0 API changes for keymanapp/keyman#12170 and keymanapp/keyman#12185

  • getLongpressDelay
  • sendKeyboardOptions
  • setLongpressDelay

@darcywong00 darcywong00 added this to the A18S9 milestone Aug 26, 2024

The following code illustrates the use of `setLongpressDelay()`:
```java
int currentDelayTimeMS = 250;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a min/max range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Keyman for Android menu has a range of 300ms to 1500 ms. But the API doesn't have any gating on what keyboard developers may use.

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping this information would be added to the docs. Also, we should really be putting limits into public API functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can document a range and add gating to KMManager.

300-1500 ms? Or will somebody eventually want a 2000ms delay?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should match what Keyman app supports

Comment on lines +16 to +19
Use this method to update options in the KeymanWeb keyboard.
* Number of milliseconds to trigger a longpress gesture

This method requires a keyboard to be loaded for the values to take effect.
Copy link
Member

Choose a reason for hiding this comment

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

Why would this be needed? Why can't KMManager handle this itself internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There may be other keyboard preferences that change/get stored.

This API propagates the value to KeymanWeb to use.

Copy link
Member

Choose a reason for hiding this comment

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

KMManager already knows the value though -- so why can't it be responsible for telling KMW? It seems like exposing internals to have this API at all (and I know I approved the previous PR, just thinking about this a bit more now, sorry!). I think we should review that because it's definitely less than ideal to push this responsibility of managing internal preference updates onto the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was waiting on merging this PR to see if we needed a Monday design discussion about the interactions between: Keyman Engine API + KeymanWeb + UI

Copy link
Member

Choose a reason for hiding this comment

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

Can you write up a spec document on the various bits and pieces -- where we are at today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, I'll send you a Slack when I've jotted something into the Drive for you to squiz

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

Successfully merging this pull request may close these issues.

2 participants