[google_maps_flutter_platform_interface] Add onPointOfInterestTap support - #12752
[google_maps_flutter_platform_interface] Add onPointOfInterestTap support#12752tenninebt wants to merge 1 commit 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. |
There was a problem hiding this comment.
Code Review
This pull request adds support for tapping points of interest (POIs) on the map by introducing PointOfInterestId, PointOfInterestTapEvent, and the onPointOfInterestTap stream. Feedback suggests enhancing the API by creating a PointOfInterest class that includes the POI's name and coordinates, rather than just its ID. Additionally, it is recommended to throw an UnimplementedError in the default implementation of onPointOfInterestTap to maintain consistency with other platform interface methods, and to update the corresponding tests accordingly.
| /// An event fired when a point of interest is tapped. | ||
| class PointOfInterestTapEvent extends MapEvent<PointOfInterestId> { | ||
| /// Build a PointOfInterestTap Event triggered from the map represented by `mapId`. | ||
| /// | ||
| /// The `value` of this event is a [PointOfInterestId] object that represents the | ||
| /// tapped point of interest. | ||
| PointOfInterestTapEvent(super.mapId, super.pointOfInterestId); | ||
| } |
There was a problem hiding this comment.
The native Google Maps SDKs on both Android (OnPoiClickListener) and iOS (mapView:didTapPOIWithPlaceID:name:location:) provide the name and location (LatLng) of the tapped point of interest, in addition to the place ID. \n\nBy only exposing PointOfInterestId (which only wraps the place ID string), developers will not be able to know the name or coordinates of the tapped POI without performing a separate, paid Google Places API lookup. \n\nTo make this API fully functional and useful, please consider defining a PointOfInterest class containing id, name, and latLng, and have PointOfInterestTapEvent wrap that instead of just the ID.
| Stream<PointOfInterestTapEvent> onPointOfInterestTap({required int mapId}) { | ||
| return const Stream<PointOfInterestTapEvent>.empty(); | ||
| } |
There was a problem hiding this comment.
To maintain consistency with all other stream methods in this class (such as onClusterTap, onGroundOverlayTap, onMarkerTap, etc.) and to adhere to the class's documented design principle (which states that default implementations of new methods should throw at runtime to signal unimplemented status), this method should throw an UnimplementedError instead of returning an empty stream.
| Stream<PointOfInterestTapEvent> onPointOfInterestTap({required int mapId}) { | |
| return const Stream<PointOfInterestTapEvent>.empty(); | |
| } | |
| Stream<PointOfInterestTapEvent> onPointOfInterestTap({required int mapId}) { | |
| throw UnimplementedError('onPointOfInterestTap() has not been implemented.'); | |
| } |
| test('onPointOfInterestTap() returns empty stream', () async { | ||
| final Stream<PointOfInterestTapEvent> stream = BuildViewGoogleMapsFlutterPlatform() | ||
| .onPointOfInterestTap(mapId: 0); | ||
| expect(await stream.isEmpty, isTrue); | ||
| }); |
There was a problem hiding this comment.
Update this test to expect an UnimplementedError to match the corrected default implementation of onPointOfInterestTap.
| test('onPointOfInterestTap() returns empty stream', () async { | |
| final Stream<PointOfInterestTapEvent> stream = BuildViewGoogleMapsFlutterPlatform() | |
| .onPointOfInterestTap(mapId: 0); | |
| expect(await stream.isEmpty, isTrue); | |
| }); | |
| test('onPointOfInterestTap() throws UnimplementedError', () { | |
| expect( | |
| () => BuildViewGoogleMapsFlutterPlatform().onPointOfInterestTap(mapId: 0), | |
| throwsUnimplementedError, | |
| ); | |
| }); |
…port Add PointOfInterestId, PointOfInterestTapEvent, and a default empty onPointOfInterestTap stream so implementations can opt in without a breaking platform-interface change.
4b55349 to
a0bb5d2
Compare
This is the platform interface portion of #11872
Adds
PointOfInterestId,PointOfInterestTapEvent, andGoogleMapsFlutterPlatform.onPointOfInterestTap. The default implementation returns an empty stream so existing platform implementations are not broken.Fixes flutter/flutter#60695 (platform interface only; app-facing and implementation packages land in follow-up PRs per the federated plugin contribution process).
Pre-Review Checklist
[shared_preferences]///).@stuartmorgan-g — this is the first sub-PR from #11872 as requested.