[google_maps_flutter] Add onPointOfInterestTap callback - #11872
[google_maps_flutter] Add onPointOfInterestTap callback#11872tenninebt wants to merge 2 commits into
Conversation
336fb13 to
e125948
Compare
There was a problem hiding this comment.
Code Review
This pull request adds support for tapping points of interest (POIs) on the map across Android, iOS, and Web platforms in the google_maps_flutter plugin. It introduces the PointOfInterestId type and PointOfInterestTapEvent in the platform interface, exposes the onPointOfInterestTap callback on the GoogleMap widget, and implements the platform-specific event handling and Pigeon messaging. The review feedback suggests adding a defensive null check for the PointOfInterest parameter in the Android onPoiClick handler to prevent a potential NullPointerException.
| public void onPoiClick(PointOfInterest pointOfInterest) { | ||
| if (pointOfInterest.placeId != null) { | ||
| flutterApi.onPointOfInterestTap( | ||
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Defensively checking pointOfInterest for null before accessing its properties is a good practice to prevent potential NullPointerExceptions, especially since the parameter is not annotated with @NonNull.
| public void onPoiClick(PointOfInterest pointOfInterest) { | |
| if (pointOfInterest.placeId != null) { | |
| flutterApi.onPointOfInterestTap( | |
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | |
| } | |
| } | |
| public void onPoiClick(PointOfInterest pointOfInterest) { | |
| if (pointOfInterest != null && pointOfInterest.placeId != null) { | |
| flutterApi.onPointOfInterestTap( | |
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | |
| } | |
| } |
There was a problem hiding this comment.
The placeId null guard is the important part, and it’s already there. The Maps SDK only calls onPoiClick with a non-null PointOfInterest when a POI is tapped, and the other click handlers in this class don’t null-check their parameters either. We already have a test for the realistic case (placeId == null), which matches iOS. An extra pointOfInterest != null check would be harmless but unnecessary.
|
Hi @stuartmorgan-g when you're ready to review ping me and I'll do it right away. I'd rather fix the conflicts onces and for all instead of doing it every commit on that part of the code ;). |
|
No need to resolve changelog/pubspec comments until the whole review process is essentially complete (per the FAQ) since the conflicts in those files don't affect the ability to review the changes. |
| - (void)didLongPressAtPosition:(FGMPlatformLatLng *)position; | ||
|
|
||
| /// Called when a point of interest is tapped. | ||
| - (void)didTapPointOfInterestWithPlaceId:(NSString *)placeId; |
There was a problem hiding this comment.
...WithPlaceIdentifier:
There was a problem hiding this comment.
Renamed. I also moved the declaration up so it sits in the same spot as the matching method in MapsCallbackApi.
| void onCircleTap(String circleId); | ||
|
|
||
| /// Called when a point of interest is tapped. | ||
| @ObjCSelector('didTapPointOfInterestWithPlaceId:') |
There was a problem hiding this comment.
...PlaceIdentifier:
There was a problem hiding this comment.
Done, it's didTapPointOfInterestWithPlaceIdentifier: now.
1a982ab to
2247d7a
Compare
Expose a place-ID-only POI tap stream across the federated plugin stack (Android, iOS, web) so apps can react when users tap built-in map POIs. Fixes flutter/flutter#60695
2247d7a to
1215581
Compare
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Looks good structurally; passing off to platform teams for in-depth platform-level reviews.
One question about the web implementation though.
|
|
||
| /// Fake implementation of FGMMapsCallbackApiProtocol that records the calls it receives. | ||
| class MockMapsCallbackApi: NSObject, FGMMapsCallbackApiProtocol { | ||
| var lastTappedPointOfInterestPlaceIdentifier: String? |
There was a problem hiding this comment.
Longer term this object may need to take callbacks instead of recording values, to allow for more flexible tests, but this is probably fine for now.
There was a problem hiding this comment.
Fair point — I'll keep that in mind if we need more flexible tests down the line. Leaving it as-is for now since it covers what we need.
| import 'dart:async'; | ||
| import 'dart:convert'; | ||
| import 'dart:js_interop'; | ||
| import 'dart:js_interop_unsafe'; |
There was a problem hiding this comment.
Why do we need this? Is there a property that needs to be added to google_maps? If so, that should be done upstream rather than worked around here.
There was a problem hiding this comment.
I agree. The placeId property should be added upstream. Or defined here using normal JS interop:
extension PlaceIdExtension on gmaps.MapMouseEventOrIconMouseEvent {
external String? placeId;
}
There was a problem hiding this comment.
You're right, we don't need unsafe interop here. I added a local placeId extension on MapMouseEventOrIconMouseEvent (same approach @mdebbar suggested) and read that directly in the click handler. Upstream would still be cleaner long term, but this gets us off dart:js_interop_unsafe for now.
mdebbar
left a comment
There was a problem hiding this comment.
Web changes look good to me but we should try to eliminate the use of dart:js_interop_unsafe.
| import 'dart:async'; | ||
| import 'dart:convert'; | ||
| import 'dart:js_interop'; | ||
| import 'dart:js_interop_unsafe'; |
There was a problem hiding this comment.
I agree. The placeId property should be added upstream. Or defined here using normal JS interop:
extension PlaceIdExtension on gmaps.MapMouseEventOrIconMouseEvent {
external String? placeId;
}
camsim99
left a comment
There was a problem hiding this comment.
Android implementation LGTM!
| public void onPoiClick(PointOfInterest pointOfInterest) { | ||
| if (pointOfInterest.placeId != null) { | ||
| flutterApi.onPointOfInterestTap( | ||
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | ||
| } | ||
| } |
|
Now that this has all the platform team sign-offs, please go ahead and make the first sub-PR with the platform interface changes. |
|
Opened the platform interface sub-PR as requested: #12752 |
c023adc to
62ef656
Compare
…safe MapMouseEventOrIconMouseEvent only exposes latLng upstream, so add a local external placeId getter and use that in the click handler instead of hasProperty + cast.
62ef656 to
954c841
Compare
Adds
GoogleMap.onPointOfInterestTap, a callback that fires when the user taps a built-in map point of interest. The callback receives aPointOfInterestIdcontaining the place ID only, matching maintainer feedback on #4052 and #10963.The change is wired through the federated plugin stack: platform_interface (type + event stream), Android (
OnPoiClickListener), iOS including sdk9/sdk10/shared (didTapPOIWithPlaceID), web (IconMouseEvent.placeIdon map click), and the app-facinggoogle_maps_flutterpackage. Tests cover Dart unit tests, Android Robolectric, iOS native, and web integration tests in the web example.Fixes flutter/flutter#60695
Pre-Review Checklist
[shared_preferences]///).