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

Improve the detection of taps #62

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

gm-vm
Copy link

@gm-vm gm-vm commented Mar 26, 2023

The current time limit (500ms) only applies to the calculation of the distance. This means that the fingers can rest on the trackpad for an arbitrary amount and still trigger a middle click. This fixes that by checking the duration of the tap when the fingers are released.

On top of that, the current thresholds (time and distance) can easily cause conflicts with system gestures.

A maximum distance of 0.4f means that you can swipe on the trackpad and still trigger a middle click (the position is normalized and it is always in the range [0..1]). Lower values, such as 0.05f, work better for detecting just taps.

A maximum duration of 500ms is also quite a lot for a tap. Something between 150ms/200ms works better for detecting just taps.

Instead of changing the default values, which maybe work for some, this adds two new preferences that allow to change those thresholds.

For example, to set the maximum distance to 0.05f and the maximum duration to 200ms, run:

defaults write com.rouge41.middleClick maxDistanceDelta 0.05
defaults write com.rouge41.middleClick maxTimeDelta 200

The current limit only applies to the calculation of the distance.
This means that the fingers can rest on the trackpad for an arbitrary
amount and still trigger a middle click.
@gm-vm
Copy link
Author

gm-vm commented Mar 27, 2023

I have noticed that there is an open issue for this: #39

I don't if this should change the default values as well, I kept everything as is to not break anyone's workflow. However, this might need a couple of lines in the README of the project.

@gm-vm
Copy link
Author

gm-vm commented Mar 29, 2023

I've just realized that there is a "Tap to click" option that may not work properly with this (EDIT: I meant to say that disabling the option may break things). I think I need to revisit these changes.


Reading the code better, I see that "Tap to click" controls needToClick, so the changes have no impact there. I assumed they did because the options seems to not work here. I even reinstalled a fresh copy of the application resetting the preferences to make sure I did not cause the problem.

The current thresholds can easily cause conflicts with other gestures.

A maximum distance of 0.4f means you can swipe on the trackpad and
still trigger a middle click (the position is normalized and it is
always in the range [0..1]). Lower values, such as 0.05f, work better
for detecting just taps.

A maximum duration of 400ms is also quite a lot for a tap. Something
between 150ms/200ms works better for detecting taps.

Instead of changing the default values, which maybe work for some, this
adds two new preferences that allow to change those thresholds.

For example, to set the maximum distance to 0.05f and the maximum duration
to 200, run:

 defaults write com.rouge41.middleClick maxDistanceDelta 0.05
 defaults write com.rouge41.middleClick maxTimeDelta 200
@artginzburg
Copy link
Owner

This is a very nice improvement! The code is now more consistent and readable. And looks like it actually fixes #39.
I also adore that you separated your commits — one for a functional change, and another for a code-only change.

The one thing that bothers me: since clicks initiated with "Tap to click" are processed through the very same logic branch as taps (in touchCallback()), checking their elapsedTime makes them feel like something between a click and a tap — taps should be quick, and clicks can be any time long, but what we see here is that clicks are now also required to be quick.

If "Tap to click" is disabled, everything works fine, since the else to if (needToClick) never activates.

@gm-vm
Copy link
Author

gm-vm commented Mar 31, 2023

If I'm reading the code correctly, this is the branch used when "Tap to click" is not enabled (i.e., needToClick is true). These changes only affect the else block.

I still don't know why it does not work for me without "Tap to click", but I've verified that it happens even with the unmodified app.

@artginzburg artginzburg merged commit 879bdf0 into artginzburg:main Jun 5, 2023
@jessejoe
Copy link

Will there be a release with these changes? I'd love to try them.

@gm-vm
Copy link
Author

gm-vm commented Jun 20, 2023

@jessejoe I can't help you there, but maybe you can try what I did to do these changes. You only need the xcode command line tools, which can be installed very easily, and be able to move around in a terminal and launch some commands.

I pushed all that is needed here (the last two commits): https://github.com/gm-vm/MiddleClick-Ventura/tree/makefile

With that you can run make from inside the directory with the Makefile. That will generate a MiddleClick executable that you can use to replace that of the already installed application. To do that, close any running instance of MiddleClick, remove the accessibility permission from the system settings (important: do not do this while the application is running, not with the current release at least) and replace the executable that is inside /Applications/MiddleClick.app/Contents/MacOS/MiddleClick.

@artginzburg by the way, maybe the new preferences should be documented somewhere, or the default changed if the current ones are really broken for everyone.

@artginzburg
Copy link
Owner

artginzburg commented Jun 22, 2023

I'll add some info on these preferences to the ReadMe.

I've been using this version for 2 weeks now. It feels better than the previous versions, I've been getting much less misclicks (but still some, probably because I did not tweak the preferences).

The 500ms time delta definitely seems to be more than necessary for tapping. I wonder if there's a way to steal the actual interval for tapping from macOS (other than testing it manually and finding the threshold). It's probably somewhere around 300ms, but can't be sure.

The max distance of 0.4f surely allows for some amount of swipe to occur. I've just set it to 0.05f and everything seems to work fine.

P.S. While testing the smaller deltas, I've somehow managed to repeatedly trigger a middle-click with just 2 fingers, when my fingers setting is 3, by repeatedly tapping around 5-10 times in a row with two fingers. I can only do that purposefully though, never happened in everyday usage, but seems like it needs to be investigated in the future. Tested again on v2.6.1, confirmed that this has nothing to do with this PR, just needed to write it down somewhere quickly.

@gm-vm
Copy link
Author

gm-vm commented Jun 22, 2023

Even 300ms is quite a lot IMO, generally 100ms is enough for a single tap on touchscreen devices, 300ms is what I'd use for detecting double taps.

As for the distance, there's barely any movement when tapping, so I'm pretty confident 0.4f is wrong. 0.4f allows you to drag the fingers for almost half the length of the trackpad.

200ms and 0.05 have been working great here, but I'm keeping two fingers swipe gestures disabled because I used to accidentally trigger middle clicks with them. Probably tweaking distance and duration would allow me to re-enable them, but I already got used to three fingers gestures

@artginzburg
Copy link
Owner

I agree, 300ms is more than necessary, but for the default I want to match macOS built-ins as much as possible.

Now, I wonder if changing the defaults is a breaking change. Should it be released as a major or as a minor update? It does not break my workflow, just makes it better, but I’m not sure whether someone needs the old defaults. Hell, this thing could use some statistics collection.

@jessejoe
Copy link

Just for some anecdotal experience, I stopped using middleclick because I got so many phantom middleclicks. I'm excited to try this out when I get a chance (thanks @gm-vm), but IMO I would prefer an update "fix" the thresholds than to just make it available to be changed.

@artginzburg
Copy link
Owner

Hey everyone, I've been using these prefs since June, they seem fine:

maxDistanceDelta = "0.05";
maxTimeDelta = 300;

Should we set them as default and cut a new release closing #39?

@gm-vm
Copy link
Author

gm-vm commented Oct 22, 2023

I'd change the default values for sure, the distance threshold is enough to catch most of the unwanted taps.

I've been using 200ms since the beginning and never changed it, so I don't know how well 300ms works. It just seems a bit high to me, at least for single taps, but it's probably fine. I also have to mention that I use two fingers for middle clicks as that's what I'm used to, things may work differently with three fingers.

I don't know what macOS uses for "Tap to click", but I know some open-source projects that deal with this problem. Here their defaults:

These are just some examples, they likely all depend on how each driver detects taps.

In any case, if you like 300ms, go ahead with it, I'm sure it works fine and users can change it if they want.

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.

Even extremely long "tap"s are registered as middle clicks
3 participants