Request and handle cursor changes #613
Conversation
…uld inhibit the next multitap action. Request updates to cursor movement in KeyboardScreen.z
|
Also, shoutout to @patrickgold, florisboard/florisboard@e8ee49a put me on the right track for where to put onUpdateCursorAnchorInfo(), thanks! |
|
Also worth noting that this depends on the text field properly calling onUpdateCursorAnchorInfo(). I don't think the "Test out Thumb-Key" field does, so the bug is still present there. I used note to self on signal to test this out |
|
huge |
|
One edge case I just thought of, if you multitap at the right edge of the text box such that the space moves the cursor to the next line, that multitap will be blocked. Maybe only checking the vertical has changed if the horizontal is not zero would work? edit: then again, if the user moves the cursor to the left edge of the text box, it may incorrectly trigger the bug and be harder to notice since text was changed on the other side of the text box. I think it's better for edge cases to block multitap than trigger it when it shouldn't be triggered, since it's easier to notice that multitap failed than that the bug occurred imo edit 2: could have it where it allows multitap if the cursor moved down 1 line, the previous horizontal was large, and the new horizontal is 0 |
jm355
changed the title
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Outdated
Show resolved
Hide resolved
I'm using android studio. It builds on woodpecker as well. |
|
I'm guessing its an older android-studio version you're using, I'm at I'll let you fix this up tho, and eliminate the crash. |
|
No, I'm using the same version as you. That said this is pushing the limit of my android dev experience, there's a good chance I don't have it set up/configured correctly. Which crash are you talking about? |
|
Your recent update fixed the crash I was getting (API level 21 version of that requestCursorUpdates). |
|
If you'd like, I can finish up this PR. Usually people don't like that tho, which is why I'm always reticent to do that right away. |
|
You should just wrap it up, that doesn't bother me at all |
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Show resolved
Hide resolved
| @@ -302,7 +301,7 @@ fun KeyboardScreen( | |||
| } | |||
| } | |||
| } else { | |||
| if (ctx.currentInputConnection.requestCursorUpdates(CURSOR_UPDATE_MONITOR or CURSOR_UPDATE_FILTER_INSERTION_MARKER)) { | |||
There was a problem hiding this comment.
Is there a reason you disabled the insertion marker filter? According to https://developer.android.com/reference/android/view/inputmethod/InputConnection#CURSOR_UPDATE_IMMEDIATE, "by default all of CURSOR_UPDATE_FILTER_EDITOR_BOUNDS, CURSOR_UPDATE_FILTER_CHARACTER_BOUNDS, CURSOR_UPDATE_FILTER_VISIBLE_LINE_BOUNDS, CURSOR_UPDATE_FILTER_TEXT_APPEARANCE, and CURSOR_UPDATE_FILTER_INSERTION_MARKER, are included but specifying them can filter-out others. It can be CPU intensive to include all, filtering specific info is recommended" (emph. added)
There was a problem hiding this comment.
Note that CURSOR_UPDATE_MONITOR isn't a filter, they do it weird where they have flags for how to monitor and filter flags in one parameter. The API34 way with separate parameters makes more sense to me, but that's how this one is
There was a problem hiding this comment.
When I had that or CURSOR_UPDATE_FILTER_INSERTION_MARKER, it failed everywhere.
There was a problem hiding this comment.
that's weird, even in signal?
There was a problem hiding this comment.
I'd need to test again, but I'm pretty sure.
|
Can we merge this in to at least fix the bug for some text fields? Or do you want to switch to a check that compares against the previous character? |
I'd rather not have inconsistent behavior, especially since jetpack compose is the standard now for building android apps.
I'd rather not do that, since it has other potential problems. I didn't get any updates on my stackoverflow thread, so I've just opened up one now on google's issue tracker for this. I'll ping back when that gets any updates from the team. If they find that its a bug, then its on them, and I'll merge this. If they find that its not a bug, hopefully they'll direct me on how to fix. |
|
They got back to me, and said:
1.6 is still a release candidate, but I'll test out that RC within a few days. |
|
Verified that it works! |
|
Awesome! |
Request updates to cursor movement in KeyboardScreen, update IMEService to handle cursor movement, and track whether the cursor moved in a way that should inhibit the next multitap action.
Tested on my phone and it works as expected, resolves #242