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

Select the closest key on swipe #138

Merged
merged 5 commits into from May 8, 2022
Merged

Conversation

Rodrigodd
Copy link
Contributor

I have seen a Google Play review (in Portuguese) suggesting (among others) that 0 could be recognized with an up swipe, regardless of the direction (since the 0 has a different direction from the other numbers, probably).

So I decided to solve this by always choosing the closest key to the swipe direction. So instead of computing the index of the key in onTouchMove, I compute the direction of the swipe (from 1 to 8, 0 is no swipe) and I replaced Key.getValue by Key.getAtDirection, which finds the closest key and handles edgekeys. Now Pointer keeps track of selected_value instead of value_index.

Copy link
Owner

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Very interesting, thanks !
The computation of direction is very elegant and it seems to improves accuracy.

However the getAtDirection function seems a bit buggy. For example swiping right on the backspace key will hit backspace but down-right will hit delete. It also doesn't seem to have a consistent "closest key" behavior depending on direction.
I've modified this function, let me know what you think.

There's a bug remaining in this function: _handler.onPointerSwipe is allowed to return null, meaning it can remove a key depending on flags. The previous code tried hard to behave the same as if there were no key at all.
For example, on the QWERTY layout, the p key has placeholder keys for F11 and F12 that are normally hidden, they interfere with the calculation for the 0 key.
To fix that, the loop should be moved back to the Pointers class and should try _handler.onPointerSwipe on possible keys before returning.

@@ -146,34 +146,42 @@ public void onTouchMove(float x, float y, int pointerId)
Pointer ptr = getPtr(pointerId);
if (ptr == null)
return;

// The position in a IME windows is clampled to view.
// For a better up swipe behaviour, set the y position to a negative value when clamped.
Copy link
Owner

Choose a reason for hiding this comment

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

Not the case on my devices! Nice bug fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forget to mention that. I tried to find a workaround or documentation for that, but I only found this Stack Overflow post: https://stackoverflow.com/a/41208273.

I see this behavior on my Android 11 device. Maybe I test that on my old Android 6 device to see if this happens there.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, thanks.

@Rodrigodd
Copy link
Contributor Author

Rodrigodd commented May 8, 2022

However the getAtDirection function seems a bit buggy. For example swiping right on the backspace key will hit backspace but down-right will hit delete.

The keys key2 and key3 of the fourth closer switch case of my implementation are inverted. I revised the function and this appear to be the only error.

I've modified this function, let me know what you think.

To be honest, it took me some time to understand how the loop works, but I think it is better than my. Your function is currently getting the first or second-closest key, which is a more intuitive behavior than getting even the opposite key as my function was doing, and even if not, you can easily adjust the range by only changing the loop condition.

There's a bug remaining in this function: _handler.onPointerSwipe is allowed to return null, meaning it can remove a key depending on flags. The previous code tried hard to behave the same as if there were no key at all.

I have found about it early today. If you want, I can fix that on the next weekend, but feel free to merge this in the current state.

@Julow
Copy link
Owner

Julow commented May 8, 2022

Your function is currently getting the first or second-closest key

You are right, the condition is a bit too soon. i > -3 would be better, i > -4 is reasonable too.
I'll fix the "removed keys" bug and merge. Of course, open a new PR if you find an improvement later.

Rodrigodd and others added 5 commits May 8, 2022 16:05
getAtDirection was too hard to maintain and might contain bugs.
Change slightly the meaning of directions and implement a the nearest
key calculation as a loop.
The "closest key" logic must be careful not to reveal keys removed by a
modifier.

Must check [_handler.onPointerSwipe] for every candidate values.
[selected_value] is changed back to [selected_direction].

This adds a new bug: When the direction change, the selected value might
not change but a vibration will be triggered anyway.
Fix the bug introduced in the parent commit.
@Julow Julow merged commit 82d3290 into Julow:master May 8, 2022
@sdrapha
Copy link
Contributor

sdrapha commented May 10, 2022

That was a great improvement! I just wanted to say thanks!!! 😁

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.

None yet

3 participants