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

Prevent phantom clicks when scrolling, clicks when right clicking #69

Merged
merged 5 commits into from
Aug 18, 2023

Conversation

lazd
Copy link
Contributor

@lazd lazd commented Apr 1, 2023

This PR corrects some issues introduced by #67 and #69 where phantom clicks were happening when scrolling and before right clicking.

The fix is twofold:

  1. Move anything that does a left click into a timed function, cancel the clicks if a multitouch interaction starts, don't perform the clicks if a right click has happened (except for the special case of right click + drag)
  2. Ignore interactions that happen immediately after a multitouch interaction ends

Because of how this works, click_tick is no longer necessary and we're guaranteed to get several hovers before a click or drag happens.

Because drags are delayed, we have to be careful about where we start them, so we use the start location of the touch interaction, which seems to work well.

Todo

  • Figure out what's causing jittering during drags
  • Figure out what's cause mouse to stay down after dragging sometimes, resulting in selection
  • Figure out why second tap in different location can sometimes turn into drag (scroll, tap, tap)
  • Check stylus behavior due to changes that remove for loop from single touch interactions

Questions about past code that was modified

  • Find out if we need to send transducer->tip_switch.value() for finger touches or if LEFT_CLICK is fine
  • Find out why there was a for loop for transducers if there was always one
  • Find out why the old for loop went to count + 1
  • Find out if it's ever possible that the fetched transducer is null (could remove checks if not)
  • Double check that returning false for MULTITOUCH_TIMEOUT is a problem

@lazd
Copy link
Contributor Author

lazd commented Apr 1, 2023

@blankmac any insight on the questions I've added as comments on the PR? Thank you!

@blankmac
Copy link
Collaborator

blankmac commented Apr 1, 2023

I think you're finding out that making what amounts to a mouse or touchpad input behave like you'd expect a touchscreen to behave is a complicated thing! Unfortunately, I no longer have any hackintosh hardware with a touchscreen. With Apple moving towards their own CPUs I bit the bullet and purchased an M1 based laptop when they came out. I have a friend that still has an HP Elite X2 tho, which is what I originally used to write the touchscreen routines, and will get it from him in a few days and try your revisions. It's been so long since I've dug into this code that there are most certainly redundant portions and errors. It definitely needs an update!

Regarding the for loop and the "+1", IIRC that has to do with the transducer ordering. The stylus always comes first but I believe the catch was that it wouldn't appear in the transducers as a contact event until you had actually used it once. You might try using a stylus first and then seeing if your single touch / multi touch input behaves as expected.

It's unfortunate that so much complexity has to be added for such a simple thing as double clicking. Through 10.11, double clicking worked just fine and as expected. There's an area property in the HID device that I believe is the root of the problem but I was never successful at widening the area to allow for slightly different locations when executing a double click.

Anyway, I should be able to go through your revisions in a couple days. But don't let that stop you if you get it figured out prior to that. Thanks for working on it!

@lazd lazd marked this pull request as ready for review April 1, 2023 17:54
@lazd
Copy link
Contributor Author

lazd commented Apr 1, 2023

@blankmac thanks for the insight! I've purchased a Microsoft Surface Pen so I can test stylus interactions. I'll check and see if that for loop +1 index is still required.

@kprinssu
Copy link
Collaborator

@lazd Do you any assistance with getting this PR completed?

@lazd
Copy link
Contributor Author

lazd commented Jun 26, 2023

@kprinssu actually yeah, I can't get my Surface Go 2's touchscreen to react to a pen in macOS, so there's no way for me to test if pen support is still behaving as expected. I believe that's the only question that remains, the other two items under Questions about past code that was modified aren't as pressing.

If touch works, this could be merged.

@kprinssu
Copy link
Collaborator

Thanks @lazd, I'll take a look sometime this week and will add my comments to this PR.

@lazd
Copy link
Contributor Author

lazd commented Aug 18, 2023

@kprinssu I'd say go ahead and get this in for the 2.9 release, this fixes some behavior from my previous PRs. Let's see what users say, and if we broke pens I can fix it. Thank you!

@kprinssu
Copy link
Collaborator

Thanks @lazd

@kprinssu kprinssu merged commit 7ae5a29 into VoodooI2C:master Aug 18, 2023
@zhen-zen
Copy link
Contributor

Was too busy to debug my tablet recently. I tried the latest master and the touchscreen started to behave like a big touchpad. Single touch won't move the cursor to finger as usual, and swiping translates to relative movement of the cursor. The first commit in this PR started to cause the issue but I didn't dig into the code further.

@lazd
Copy link
Contributor Author

lazd commented Oct 12, 2023

Was too busy to debug my tablet recently. I tried the latest master and the touchscreen started to behave like a big touchpad. Single touch won't move the cursor to finger as usual, and swiping translates to relative movement of the cursor. The first commit in this PR started to cause the issue but I didn't dig into the code further.

Interesting, I'm running this on my Surface Go 2 and the touchscreen works great. It sounds like the driver isn't even being loaded for your touchscreen since it's being treated like a trackpad, perhaps there is a configuration error?

@zhen-zen
Copy link
Contributor

zhen-zen commented Oct 15, 2023

I don't think there's any change in configuration since Info.plist is untouched. Build of previous commit just works and the HEAD breaks one finger tracking. Also, single finger long press to double click is also broken.

image

@zhen-zen
Copy link
Contributor

Was able to dig into the code flow a bit, thanks to the cloudy weather today that blocks the ring of fire :(
I am suspecting checkFingerTouch as the report will be hand to multitouch_interface so it will then behave as a touchpad. Since IOLog won't work under interrupt context, I used setProperty to indicate the return of this method. And after the PR, it always return false. Not sure if I still have some time to continue debugging over the weekend.

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