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: prevent double-tap-drag zoom gesture emitting a tap event #1796

Conversation

androidseb
Copy link
Contributor

@androidseb androidseb commented Jan 14, 2024

This change fixes a minor bug in the gesture system: when initiating a double-tap-drag-zoom gesture, a tap on the map would be registered through the MapOptions.onTap callback.

@androidseb androidseb marked this pull request as draft January 14, 2024 14:16
@androidseb
Copy link
Contributor Author

I tested this on mobile and web and it seems to work well, marking this as ready for review now.

@androidseb androidseb marked this pull request as ready for review January 14, 2024 14:41
@JaffaKetchup JaffaKetchup changed the title Bugfix: double-tap-drag-zoom gesture triggering a map tap event fix: prevent double-tap-drag zoom gesture emitting a tap event Jan 14, 2024
JaffaKetchup

This comment was marked as outdated.

@JaffaKetchup JaffaKetchup merged commit 51f0d60 into fleaflet:master Jan 15, 2024
7 checks passed
@josxha
Copy link
Contributor

josxha commented Jan 15, 2024

Sorry I haven't had time to record the videos earlier. I was able to reproduce the following bug on the master branch (pre merge):
After a double tap drag zoom gesture the next tap or double tap drag zoom gesture calls the onTap callback twice and emits two MapEventTap events. However i have doubts that this PR fixes this bug because I was able to reproduce it on the PR branch too.

master branchpull request
flutter_map.master.2.Mit.Clipchamp.erstellt.mp4
flutter.map.pr.Mit.Clipchamp.erstellt.mp4

And here is a showcase when performing a double tap drag zoom gesture and tap gesture afterwards using the pull request branch:

Bug_.onTap.callbacks.called.twice.Mit.Clipchamp.erstellt.1.mp4

@JaffaKetchup

This comment was marked as outdated.

@androidseb
Copy link
Contributor Author

@josxha thanks for finding those issues, I looked at your videos and tried to reproduce the problem on my computer - it took me a lot of tries to actually notice the problem because it was random:

  • On the master branch: the bug happens 100% of the time
  • On the PR branch (before it was reverted): it happens very randomly - sometimes I can perform the gesture 10 times in a row without problem, and sometimes after just 2 gestures it will send the duplicate onTap event

Were you also experiencing this randomness on your machine?

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Jan 15, 2024

(After retesting, I could reproduce without randomness as far as I could see.)

This PR has been reverted by #1799.

@josxha
Copy link
Contributor

josxha commented Jan 15, 2024

I had some struggle to reproduce it too every time, sometimes i even messed up because I failed to perform the double tap drag zoom gesture with my mouse. 😅
I believe it has some logic of when it occurs but couldn't tell what exactly.

FYI: There is an rewrite on the gesture system in #1733 that will get introduced with the next major flutter_map version (fingers crossed :D ). If you want you can check if the bug occurs there too, I tried it real quick and couldn't reproduce it on that branch.

@JaffaKetchup JaffaKetchup changed the title fix: prevent double-tap-drag zoom gesture emitting a tap event ~fix: prevent double-tap-drag zoom gesture emitting a tap event~ Jan 15, 2024
@JaffaKetchup JaffaKetchup changed the title ~fix: prevent double-tap-drag zoom gesture emitting a tap event~ ~~fix: prevent double-tap-drag zoom gesture emitting a tap event~~ Jan 15, 2024
@JaffaKetchup JaffaKetchup changed the title ~~fix: prevent double-tap-drag zoom gesture emitting a tap event~~ fix: prevent double-tap-drag zoom gesture emitting a tap event Jan 15, 2024
@androidseb
Copy link
Contributor Author

@josxha thanks for linking your PR here. This is a tricky problem for sure, I spent about an hour wrestling with the PositionedTapDetector2 class and almost got it to work, but couldn't get rid of the randomness in some edge cases.

I'll wait for your PR changes to land and I'll test those instead 👍

@JaffaKetchup
Copy link
Member

(Just wanted to say thanks @androidseb (even if it didn't exactly go to plan this time), and my apologies for not testing correctly, I completely misread/remembered the gesture these changes applied to.)

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.

3 participants