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

Fix DoubleTapped on touch #7213

Merged
merged 19 commits into from
Dec 22, 2021
Merged

Fix DoubleTapped on touch #7213

merged 19 commits into from
Dec 22, 2021

Conversation

Takoooooo
Copy link
Contributor

@Takoooooo Takoooooo commented Dec 20, 2021

Before this PR DoubleTapped event cant be triggered by touch input. Now it should work correctly.
Other frameworks like WPF\WinForms doesn't seem to count more than 3\4 clicks but I think there could be some use-cases for that so our implementation just basically counts clicks without any limitations. I don't have any devices with Touch so I can't test it, please test it by yourself. If more tests are needed I can add some. Click counting logic is copy-pasted(mostly) from MouseDevice but I don't think I can refactor it or improve in any way.
Also fixes DoubleTapped to be raised even if you click 4,6,8..etc times in a row, it matches UWP behaviour.
Also fixes Tapped. Now it wouldn't be raised twice when you click twice, Tapped would be raised once and DoubleTapped once,matches UWP behaviour.
Note:
On this line I hardcoded those values because we don't have separate settings for touch and it is really hard to hit into the size of the square declared for the mouse with touch. It is temporal change.

Checklist

  • Added unit tests (if possible)?

Obsoletions / Deprecations

ClickCount is not Obsolete anymore.

Fixed issues

Closes #5412
Closes #6949

@Takoooooo
Copy link
Contributor Author

I wonder where those warnings come from and how can I see them on Windows?
image

@Takoooooo Takoooooo marked this pull request as draft December 20, 2021 13:29
@Takoooooo Takoooooo closed this Dec 20, 2021
@Takoooooo Takoooooo reopened this Dec 21, 2021
@Takoooooo Takoooooo marked this pull request as ready for review December 21, 2021 12:37
@@ -6,6 +6,7 @@ namespace Avalonia.Input
{
public static class Gestures
{
private static bool _isDoubleTapped = false;
Copy link
Member

Choose a reason for hiding this comment

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

That's a global variable. We usually try to avoid those

Copy link
Contributor

Choose a reason for hiding this comment

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

Device and input state information must usually be maintained and managed centrally. It might help to suggest an alternative. Otherwise, I expect to see things like this myself.

Copy link
Member

Choose a reason for hiding this comment

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

The app might run on 2 independent touch screens. Or even on 1 touch screen with 2 windows on it. Because of such global variables we'll misinterpret tap sequences caused by the user tapping on those windows sequentially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to temporarily leave it as is

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing is more permanent than temporary :) I would comment in the code that it should change in the future and why.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing is more permanent than temporary

Yes, the original static variable in that file dates back to 2016 when DoubleTapped event was introduced. So we aren't introducing any new problems.

@Takoooooo Takoooooo enabled auto-merge (squash) December 22, 2021 14:02
@Takoooooo Takoooooo merged commit 1fbcd61 into master Dec 22, 2021
@Takoooooo Takoooooo deleted the fix-doubletapped-on-touch branch December 22, 2021 14:19
@grokys
Copy link
Member

grokys commented Dec 22, 2021

I'm getting an intermittent test failure in Tapped_Event_Is_Fired_With_Touch after this PR; the tests in TouchDeviceTests register services outside of the UnitTestApplication (meaning that those services are getting registered for all tests that are run in that test run) and Tapped_Event_Is_Fired_With_Touch doesn't register those services, so it will fail if ran before the others.

All service registrations for unit tests need to be done inside the using (UnitTestApplication.Start()) blocks in order to make sure they're cleaned up for when other tests run, and the registrations also need to be done in Tapped_Event_Is_Fired_With_Touch.

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