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

Pass click details to feature tapped #798

Merged
merged 3 commits into from
Dec 2, 2021

Conversation

felix-ht
Copy link
Collaborator

@felix-ht felix-ht commented Nov 30, 2021

Currently on feature tap swallows all map clicks that land on a feature. By also passing all the information to on featureTap that a user can still make the choice to treat a onFeatureTap as a normal click in his code.

In addition this is very helpful if the coordinates are needed, even if a feature has been tapped.

Note that this breaks the api of onFeatureTapped and users will have to add the new args to the function. This will be caught by the compiler tho and should be trivial to fix.

fixes #792

@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN November 30, 2021 19:47 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN November 30, 2021 19:47 Inactive
@felix-ht felix-ht requested a review from m0nac0 November 30, 2021 19:48
@ivan-ljubicic
Copy link

@felix-ht I think onMapClick should be called regardless of feature being tapped.

For example, we have layers which display different polygons (geojson data) for population number in every state/province/village etc. (covering the whole country for example). No interactions are needed for those polygons but we need to query rendered features on certain layers bellow. We would have to listen to onFeatureTapped although we don't need to know which feature was tapped, and we don't have to do anything with that feature, but we need to know point or coordinates where the user clicked. Because those polygons cover the whole country, onMapClick would never be called, which could be solved by calling onMapClick callback regardless of features tapped.

@felix-ht
Copy link
Collaborator Author

felix-ht commented Dec 1, 2021

@felix-ht I think onMapClick should be called regardless of feature being tapped.

For example, we have layers which display different polygons (geojson data) for population number in every state/province/village etc. (covering the whole country for example). No interactions are needed for those polygons but we need to query rendered features on certain layers bellow. We would have to listen to onFeatureTapped although we don't need to know which feature was tapped, and we don't have to do anything with that feature, but we need to know point or coordinates where the user clicked. Because those polygons cover the whole country, onMapClick would never be called, which could be solved by calling onMapClick callback regardless of features tapped.

@LjubiTech-Maxko
The big issues with always calling onMapClick is that it is impossible to know in the onMapClick callback, if the onFeatureTapped callback will also fire. This makes it very hard to do things that should only happen if a user only clicked the map and not a feature (because the feature tap might still happen).

Additionally I will add the ability to completely disable onFeatureTapped for new layers. So the developer has full control if he cares about feature taps or not. The reason why i chose to implement on featureTapped on the native side in the first place is to avoid doing a query for rendered features form dart, as this adds quite a bit of latency to clicks.

Copy link
Collaborator

@m0nac0 m0nac0 left a comment

Choose a reason for hiding this comment

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

LGTM!

Maybe already add a short note to the changelog how users can handle the breaking change?

@m0nac0
Copy link
Collaborator

m0nac0 commented Dec 1, 2021

Theoretically we could unify both click listeners and just have one click listener with a nullable parameter for the feature id.
But that would be an even more breaking change.

@felix-ht
Copy link
Collaborator Author

felix-ht commented Dec 2, 2021

Theoretically we could unify both click listeners and just have one click listener with a nullable parameter for the feature id. But that would be an even more breaking change.

I thought about that option as well - but that would be a breaking change for all users of onMapClicked (which are many more than for onFeatureTapped). In addition it would make it impossible to differentiate between a click on the map and a click on feature without an id (even tho is probably of very limited use). I will add the note to the changelog.

@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 2, 2021 13:30 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 2, 2021 13:30 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 2, 2021 13:41 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 2, 2021 13:41 Inactive
@felix-ht felix-ht merged commit 4af59c5 into master Dec 2, 2021
@meeximum
Copy link

hi, is there already a plan for releasing this feature?

@felix-ht
Copy link
Collaborator Author

felix-ht commented Dec 10, 2021

@meeximum i would like to wait until #795 is resolved from upstream

@m0nac0
Copy link
Collaborator

m0nac0 commented Dec 10, 2021

@meeximum If you already want to use this before the next release, you can update your dependency as shown here #695 (comment) (note that you will need to update the repository url).

m0nac0 added a commit to maplibre/flutter-maplibre-gl that referenced this pull request May 19, 2022
m0nac0 added a commit to maplibre/flutter-maplibre-gl that referenced this pull request May 19, 2022
* Cherry-pick upstream#798 (Pass click details to feature tapped)

https: //github.com/flutter-mapbox-gl/maps/pull/798
Co-Authored-By: Felix Horvat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onMapClick does not work
4 participants