[google_maps_flutter] Add onPointOfInterestTap callback#11860
[google_maps_flutter] Add onPointOfInterestTap callback#11860tenninebt wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
github-actions
Bot
added
p: google_maps_flutter
platform-android
platform-ios
platform-web
labels
tenninebt
force-pushed
the
google-maps-on-poi-tap
branch
from
6fe3d32 to
ee382ab
Compare
|
@googlebot rescan |
tenninebt
force-pushed
the
google-maps-on-poi-tap
branch
3 times, most recently
from
ee382ab to
d3ae779
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for tapping points of interest (POI) on the map across the federated Google Maps packages, introducing PointOfInterestId and PointOfInterestTapEvent along with platform-specific implementations and tests. The code review feedback highlights several critical improvements: adding missing dart:js_interop imports in the web controller to prevent compilation errors, subscribing to the POI tap stream unconditionally in the main controller to support dynamic callback updates, and adding defensive null/nil checks in both the Android and iOS platform implementations to prevent potential crashes.
| if (event.hasProperty('placeId'.toJS).toDart) { | ||
| final String? placeId = (event as gmaps.IconMouseEvent).placeId; | ||
| if (placeId != null) { | ||
| _streamController.add( | ||
| PointOfInterestTapEvent(_mapId, PointOfInterestId(placeId)), | ||
| ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of hasProperty and toJS requires importing dart:js_interop and dart:js_interop_unsafe in this file. Please ensure these imports are added at the top of google_maps_controller.dart to prevent compilation errors on the web platform:
import 'dart:js_interop';
import 'dart:js_interop_unsafe';There was a problem hiding this comment.
google_maps_controller.dart is a part of google_maps_flutter_web.dart, which already imports dart:js_interop and dart:js_interop_unsafe (lines 9–10). Part files inherit library imports. hasProperty / toJS compile fine; web tests passed.
| if (_googleMapState.widget.onPointOfInterestTap != null) { | ||
| _streamSubscriptions.add( | ||
| GoogleMapsFlutterPlatform.instance | ||
| .onPointOfInterestTap(mapId: mapId) | ||
| .listen( | ||
| (PointOfInterestTapEvent e) => _googleMapState.onPointOfInterestTap(e.value), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unlike high-frequency camera events, tap events (such as onTap, onLongPress, onMarkerTap, etc.) are subscribed to unconditionally in GoogleMapController. Subscribing to onPointOfInterestTap conditionally introduces a bug where dynamically setting or updating the callback after the map is created will not work, as the subscription is only set up during initialization.
Please subscribe to onPointOfInterestTap unconditionally, matching the pattern of other tap events.
_streamSubscriptions.add(
GoogleMapsFlutterPlatform.instance
.onPointOfInterestTap(mapId: mapId)
.listen(
(PointOfInterestTapEvent e) => _googleMapState.onPointOfInterestTap(e.value),
),
);| @Override | ||
| public void onPoiClick(PointOfInterest pointOfInterest) { | ||
| flutterApi.onPointOfInterestTap( | ||
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | ||
| } |
There was a problem hiding this comment.
To prevent potential NullPointerException crashes, defensively check if pointOfInterest and pointOfInterest.placeId are non-null before invoking the Flutter API.
| @Override | |
| public void onPoiClick(PointOfInterest pointOfInterest) { | |
| flutterApi.onPointOfInterestTap( | |
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | |
| } | |
| @Override | |
| public void onPoiClick(PointOfInterest pointOfInterest) { | |
| if (pointOfInterest != null && pointOfInterest.placeId != null) { | |
| flutterApi.onPointOfInterestTap( | |
| pointOfInterest.placeId, (Result<Unit> result) -> Unit.INSTANCE); | |
| } | |
| } |
| - (void)mapView:(GMSMapView *)mapView | ||
| didTapPOIWithPlaceID:(NSString *)placeID | ||
| name:(NSString *)name | ||
| location:(CLLocationCoordinate2D)location { | ||
| [self.mapEventHandler didTapPointOfInterestWithPlaceId:placeID]; | ||
| } |
There was a problem hiding this comment.
If placeID is ever nil, sending it directly to the Pigeon channel will pass [NSNull null], which causes a crash or assertion failure on the Dart side because the Pigeon parameter is non-nullable.
Please defensively check that placeID is not nil before forwarding the event. Please apply this check across all iOS implementation packages (google_maps_flutter_ios, google_maps_flutter_ios_sdk9, google_maps_flutter_ios_sdk10, and google_maps_flutter_ios_shared_code).
- (void)mapView:(GMSMapView *)mapView
didTapPOIWithPlaceID:(NSString *)placeID
name:(NSString *)name
location:(CLLocationCoordinate2D)location {
if (placeID != nil) {
[self.mapEventHandler didTapPointOfInterestWithPlaceId:placeID];
}
}
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
tenninebt
force-pushed
the
google-maps-on-poi-tap
branch
from
d3ae779 to
35bfd3c
Compare
|
Unfortunately your patch does not currently meet our quality bar. While it's fine to use AI tools as part of developing a PR, we still require all PRs to follow our standard PR process. Please review our AI contribution guidelines and file a new PR whose description follows our template (including completing the checklist), and we'll be happy to take a look. Thanks! |
2 participants
Summary
Adds
onPointOfInterestTapto thegoogle_maps_flutterfederated plugin, forwarding place ID only via a newPointOfInterestIdtype (per maintainer feedback on #4052 / #10963).PointOfInterestId,PointOfInterestTapEvent,onPointOfInterestTap({required int mapId})stream, method-channel wiringOnPoiClickListener→ Pigeon → DartdidTapPOIWithPlaceIDdelegate → Pigeon → DartIconMouseEventhasplaceId; skips plain map tapGoogleMap.onPointOfInterestTapcallback + controller subscriptionOnPoiClickCallsFlutterApi; iOStestDidTapPOIForwardsPlaceId; web integration tests inexample/latestmap_click.dartThis is a combination PR for federated-plugin review. Path-based
dependency_overridesare included for CI/local testing per changing federated plugins and should be split/removed before landing individual package PRs.Fixes flutter/flutter#60695
Test plan
flutter_plugin_tools.dart analyze(7 packages)flutter_plugin_tools.dart dart-test(11 packages)flutter_plugin_tools.dart validateflutter_plugin_tools.dart license-checkOnPoiClickCallsFlutterApi, JDK 21)testDidTapPOIForwardsPlaceId, afterpod install)google_maps_flutter_iosrun_tests.dartsync validationMade with Cursor