Repository navigation
Conversation
… handler interface This commit addresses issue #60695, which mentions the lack of an on point of interest event listener. The google_maps_flutter_platform_interface package is being released first to enable other Google Maps platform-specific packages to implement the onPoiClick feature. See: flutter/flutter#60695
…for point of interest event handler This commit adds the Android implementation for the point of interest event handler in the google_maps_flutter_platform_interface package. It addresses issue #60695, which raised the need for this functionality. By including the Android implementation, other Google Maps platform-specific packages can now utilize the onPoiClick feature on Android devices. Issue link: flutter/flutter#60695
…vent handler in ios This commit adds the necessary code to the `google_maps_flutter_ios` package to handle point of interest (POI) events. It includes the implementation for registering the POI click callback and handling the corresponding events on iOS. This functionality is in line with the recent addition to the `google_maps_flutter_platform_interface` package, allowing users to interact with points of interest on the map. The changes are currently untested and require further validation on macOS. Issue: #60695
…o example This commit fixes a compilation issue in the `google_maps_flutter_ios` package caused by my previous commit. The issue has been resolved, and the package now builds successfully on iOS. Additionally, iOS testing support has been added to the example app in google_maps_flutter to ensure proper functionality. Issue: #60695
This commit updates the versions of the `google_maps_flutter` packages. The new versions include the relevant code for point of interest clicked event handling for Android and iOS. The updated packages are as follows: - `google_maps_flutter`: 2.2.7 - `google_maps_flutter_platform_interface`: 2.2.7 - `google_maps_flutter_android`: 2.4.15 - `google_maps_flutter_ios`: 2.2.3 Issue: flutter/flutter#60695
This commit updates the dependencies of the `google_maps_flutter` packages to point to the new package versions mentioned in the previous commit. By updating the dependencies, the packages now utilize the latest versions that include the implementation of the point of interest event functionality. Updated dependencies: - `google_maps_flutter`: 2.2.7 - `google_maps_flutter_platform_interface`: 2.2.7 - `google_maps_flutter_android`: 2.4.15 - `google_maps_flutter_ios`: 2.2.3 Issue: flutter/flutter#60695
github-actions
Bot
added
p: google_maps_flutter
platform-android
platform-ios
platform-web
labels
|
It looks like the federation safety checks are failing. https://github.com/flutter/packages/pull/4052/checks?check_run_id=13646626910 |
|
Looks like I need to make a separate PR for google_maps_flutter_platform_interface. Thank you for pointing that out. |
…rest event interface Issue: flutter/flutter#60695
…t of Interest event interface Issue: flutter/flutter#60695
…t of Interest event interface Issue: flutter/flutter#60695
|
@reidbaker I have added the pull request for the only the google_maps_flutter_platform_interface (#4063). |
|
Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins. The sub-PRs should only be made once the overall PR has been reviewed. You just need to revert the constraint version bumps for now so that version solving actually works and the CI can run. |
|
Hi @stuartmorgan thank you for the detailed review. I have address every point besides this one: #4052 (comment). Because I am not sure about the preferred implementation to take. |
|
It looks like there are some pretty simple formatting changes that are required. |
reidbaker
left a comment
reidbaker
left a comment
There was a problem hiding this comment.
The android implementation code looks ok to me but I dont see android tests. Am I missing something?
|
This will definitely need integration test and/or unit tests that cover the implementations on all platforms. I'm not sure how I missed that in the initial review. |
There was a problem hiding this comment.
@stuartmorgan About the nullable "name" on the web:
As opposed to iOS' didTapPOIWithPlaceID:name:location: and Android's onPoiClick's PointOfInterest object, retrieving the "name" of a POI from its placeId on the web seems to always have an additional cost. Pricing. On the web, the POI tap event only gets latLng and placeId for free. Tinker here.
If we make the plugin always return a POI with "name", we'd be forcing the web implementation to always perform an additional (metered) request to the Places API to fetch the "name" of the clicked POI (and throw away a bunch of additional information that could be retrieved by the same price). This is not super user-friendly :P
Rethinking my String? name suggestion, I agree this doesn't match what we do in the plugin very well. The issue I see now is that this PR is mixing the PointOfInterest (hydrated object, costs money, can have more info than "name") with the PointOfInterestId (what Maps gives us across all platforms).
Considering the plugin as a whole, I think this PR should propagate the tap events on POIs by their PointOfInterestId, rather than the hydrated version, just like it does onCircleTap(CircleId circleId).
I think the plugin should NOT auto-hydrate any POI Data, unless the programmer explicitly requests it to, via a separate method (something like: Future<Place> getPlaceData(PlaceId id);)
Hydrating a PointOfInterest later lets the user pick what data they want to retrieve (data is priced in 3 tiers, so we could have tiered factories to construct a POI from its placeId), and not drop any data to the floor. (Similarly we need to find a way to keep the free information that is offered by Android and iOS, so it can be used later!)
In addition to the "please do not force web to hydrate PointOfInterests", I'd suggest to rename the new data structures to Place and PlaceId. That's the name used in the Maps SDK; after all a POI is just a Place that has some special "prominence" that gets it automatically rendered on the map, and make the Place object support as much data as possible coming from the Places API, not just the "name". This should enable the plugin to evolve and maybe provide other functionality of the Places API.
See available data for a Place.
Apologies for not having written my reasoning more thoroughly before!
There was a problem hiding this comment.
(On dart we may be able to know if a Stream has listeners or not, so we turn on/off the hydration as needed, but I think this has further implications than that)
|
@bndos Are you planning on adding tests, per the review comments above? |
|
Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks! |
Adds `GoogleMap.onPointOfInterestTap`, a callback that fires when the user taps a built-in map point of interest. The callback receives a `PointOfInterestId` containing 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.placeId` on map click), and the app-facing `google_maps_flutter` package. Tests cover Dart unit tests, Android Robolectric, iOS native, and web integration tests in the web example. Fixes flutter/flutter#60695 ## Pre-Review Checklist
Issue #60695 mentions the lack of a point of interest (POI) click handler in the
google_maps_flutterpackage. This pull request adds support for the point of interest (POI) event handler in thegoogle_maps_flutterpackage. The POI event handler allows users to interact with points of interest on the map by detecting when a POI is clicked.The changes in this pull request include:
Updated dependencies to the latest versions that include the POI event handler functionality:
google_maps_flutter: 2.2.9google_maps_flutter_platform_interface: 2.2.8google_maps_flutter_android: 2.4.16google_maps_flutter_ios: 2.2.4Implemented the POI event handler for Android and iOS:
google_maps_flutter_platform_interface: Added the interface methods for the POI event handler to be implemented in platform-specific code.google_maps_flutter_android: Implemented the POI event handler for Android devices.google_maps_flutter_ios: Implemented the POI event handler for iOS devices.Additionally, the
google_maps_flutterpackage now includes an example in theexampledirectory (map_click.dart) that demonstrates the usage of the POI event handler.Fixes issue: flutter/flutter#60695