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

ANDROID: Fix touchscreen stylus check #4260

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

Conversation

jacobhyman
Copy link

@jacobhyman jacobhyman commented Sep 7, 2022

ANDROID: Fix touchscreen stylus check

Fix issue where all touch input on some tablets was being treated
as mouse input. Check the tool type used instead of the input device
because SOURCE_STYLUS only indicates a device is capable of obtaining
input from a stylus. This should fix Ticket #11711.

@lephilousophe
Copy link
Member

lephilousophe commented Sep 10, 2022

I suspect that every pointer index should be checked.
For example if you use a stylus and put some finger on your screen, there is no guarantee that the tool 0 has type stylus.
You can look at backends/platform/android/org/scummvm/scummvm/ScummVMEventsBase.java line 507 about how I did it.

In addition, the style is not really proper as you mix the sources mask check with this new type of check.
I even wonder if this should be generalized for other types.

@lephilousophe lephilousophe requested a review from antoniou79 Sep 10, 2022
@jacobhyman
Copy link
Author

jacobhyman commented Sep 10, 2022

Thanks for the input.

I thought about the multi-touch case, but when looking for examples I found this one that claimed when a stylus is used it clears all other pointers (and would imply you can’t use both a stylus and finger at the same time):
https://stackoverflow.com/questions/41282789/how-do-i-handle-touch-event-with-stylus-first-strategy-in-android

That’s far from a definitive answer and I couldn’t find anything else on this, so I will to update the code to check all pointer indexes as you suggest. I can’t test this use case directly as I don’t have a stylus.

I will follow whatever stylistic conventions are suggested. The sequence of checks already had a call to an external method, and this is the only place I believe this check is currently needed as key events don’t have tool types. So, I just put the check directly in line. If I’m going to check all the pointer indexes, it probably makes sense to break this out to a separate method like isTouchpad. I will keep the isX method calls together and the source type checks together for clarity.

@lephilousophe
Copy link
Member

lephilousophe commented Sep 11, 2022

Thanks for the input.

I thought about the multi-touch case, but when looking for examples I found this one that claimed when a stylus is used it clears all other pointers (and would imply you can’t use both a stylus and finger at the same time): https://stackoverflow.com/questions/41282789/how-do-i-handle-touch-event-with-stylus-first-strategy-in-android

That’s far from a definitive answer and I couldn’t find anything else on this, so I will to update the code to check all pointer indexes as you suggest. I can’t test this use case directly as I don’t have a stylus.

Well, after looking at AOSP code to find some confirmation it's not really obvious: these events come directly from Linux kernel and in Linux directly from the hardware (HID events). Nothing can really guarantee that all other touches are really cleared but maybe the hardware doesn't support this anyway.

I will follow whatever stylistic conventions are suggested. The sequence of checks already had a call to an external method, and this is the only place I believe this check is currently needed as key events don’t have tool types. So, I just put the check directly in line. If I’m going to check all the pointer indexes, it probably makes sense to break this out to a separate method like isTouchpad. I will keep the isX method calls together and the source type checks together for clarity.

Yes, my remark was just you have this new check in the middle of the other same ones: it's just a matter of reordering to have this new check just before the isTrackball call.

Another thing where I am not sure at all is that we constantly look at the device to get its sources although we seem to be able to use the e.getSource() call.
@antoniou79 as you worked on this code, you will know better. :)

@antoniou79
Copy link
Contributor

antoniou79 commented Sep 11, 2022

Another thing where I am not sure at all is that we constantly look at the device to get its sources although we seem to be able to use the e.getSource() call. @antoniou79 as you worked on this code, you will know better. :)

To be honest it's been a while but I think I only copied this (existing) approach for isTrackball(MotionEvent e) and it seems that we do that when the argument is MotionEvent and not for the KeyEvent version of the methods. Maybe that is or was the recommended way to check the source in this case?

On the PR itself, the idea for the fix seems correct, based on the developer documentation on SOURCE_STYLUS [https://developer.android.com/reference/android/view/InputDevice#SOURCE_STYLUS], but I also am concerned about only checking the first pointer index.

The same documentation also seems to suggest that we should keep checking every pointer (btw, I hate it when it a crucial sentence they forget a verb and we are left to guess :D -- happens more often than I'd like) and that we should really be checking tool type for finger touch (TOOL_TYPE_FINGER) too.

A single touch event may multiple pointers with different tool types, such as an event that has one pointer with tool type MotionEvent#TOOL_TYPE_FINGER and another pointer with tool type MotionEvent#TOOL_TYPE_STYLUS. So it is important to examine the tool type of each pointer, regardless of the source reported by MotionEvent#getSource().

And apparently there's a TOOL_TYPE_MOUSE too and a TOOL_TYPE_UNKNOWN (which should be handled and switch to some fall back code maybe?). [https://developer.android.com/reference/android/view/MotionEvent#getToolType(int)]

I don't own a device that has stylus support, so I guess my main interest is that we fix the issue with the devices that do and do not work in mouse mode (without breaking existing finger touch and physical mouse support).

We can of course extend the fix on top of the suggested in this PR, in the future, in which case my main issue is checking the rest of the pointer indices -- so I am in favor of using a separate method for it. And I'd suggest to add comments with links to the pertinent documentation or pertinent quotes from it, so as to avoid confusion and indicate points that need to be revisited or extended later on.

@jacobhyman
Copy link
Author

jacobhyman commented Sep 11, 2022

I am away from my computer for a few days, but after that I can submit an update to:

  • create a separate isStylus method that checks every pointer index
  • Move the check next to the isTrackball check
  • Add a link to the documentation

The only android device I have is a Fire tablet and ironically I don’t think it supports an active stylus despite the touch screen input device having the flag set. I can confirm for the Fire, this change resolved an issue where every touch input was being treated as an immediate left click regardless of the input mode selected and allows Multi-touch to works as described in the Android documentation for ScummVm. I expect this will resolve a similar issue for any device that supports a stylus, but the user just using the touch screen.

@jacobhyman
Copy link
Author

jacobhyman commented Sep 14, 2022

The best I can tell from the documentation is that SOURCE_XXX is supposed to indicate the capabilities of a device (some devices having more than one) and lets you know how to treat the coordinate values. The TOOL_TYPE_XXX is intended to let you know what the specific type of tool being used on a motion event.

It appears the isMouse(MotionEvent) is only used at ScummVMEventsBase:499 and would more accurately be named "isNotFinger" with the current implementation. I'm not sure if this was the intent, but that check functionally seems to treat any motion event coming from a device that isn't a finger (stylus, trackball, mouse, etc.) as a left mouse button click.

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