Skip to content

Conversation

@AsimRoyChowdhury
Copy link

@AsimRoyChowdhury AsimRoyChowdhury commented Feb 4, 2026

This PR adds support for detecting taps on the base map's Points of Interest.

Issue: Fixes flutter/flutter#60695

Changes:

  • platform_interface: Added PointOfInterest type, MapPoiTapEvent, and onPoiTap stream definition.
  • android: Implemented OnPoiClickListener in GoogleMapController and wired the event to the method channel.
  • ios: Implemented GMSMapViewDelegate POI tap handling and forwarded events to Dart.
  • app-facing package: Exposed onPoiTap in the GoogleMap widget and GoogleMapController.
  • example: Added a new "Place POI" example page to demonstrate the feature.

Fixes: N/A - This is a new feature.

Pre-Review Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter].
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the onPoiTap callback to the GoogleMap widget, allowing developers to handle user taps on points of interest on the map. The feature is implemented across the platform interface, Android, and iOS, with corresponding updates to the example application and tests. The changes involve adding new event types, updating platform-specific listeners, and exposing the new callback in the app-facing widget. My review includes suggestions for improving code clarity, test robustness, and fixing a critical dependency issue.

flutter:
sdk: flutter
google_maps_flutter_platform_interface: ^2.13.0
google_maps_flutter_platform_interface: ^2.14.0

Choose a reason for hiding this comment

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

critical

The google_maps_flutter_platform_interface version should be ^2.15.0 to match the version in which onPoiTap support was added. The current version ^2.14.0 will cause dependency conflicts or build failures since this package uses types introduced in 2.15.0.

  google_maps_flutter_platform_interface: ^2.15.0

import 'package:google_maps_flutter_android/google_maps_flutter_android.dart';
import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart';

import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart';

Choose a reason for hiding this comment

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

medium

This import is redundant as google_maps_flutter_platform_interface is already imported on line 13. Please remove this duplicate line to improve code cleanliness.

Comment on lines 376 to 377
Log.e("POI_TEST", "Native POI Click Detected: " + poi.name);
Log.e("POI_TEST", "Native Tap! Sending to Channel ID: " + id);

Choose a reason for hiding this comment

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

medium

Using Log.e for debugging messages is not recommended as it implies an error has occurred and can clutter error logs. For debugging purposes, it's better to use Log.d or Log.v. These temporary debug logs should be removed before merging.

Comment on lines 322 to 333
public void onPoiClick_SendsMethodCall() {
PointOfInterest poi = new PointOfInterest(new LatLng(1.0, 2.0), "placeId", "name");

// controller is the instance of GoogleMapController in your test file
controller.onPoiClick(poi);

// Verify the messenger sent the message to the correct channel
verify(binaryMessenger).send(
eq("plugins.flutter.io/google_maps_0"),
any(ByteBuffer.class),
any(BinaryMessenger.BinaryReply.class));
}

Choose a reason for hiding this comment

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

medium

This test only verifies that a message is sent on the binary messenger, but it doesn't check the content of the message. A more robust test would be to use a mock MapsCallbackApi and verify that onPoiTap is called with the correct data. This ensures that the data is correctly constructed and passed to the Pigeon API.

Here's an example of how you could improve it:

  @Test
  public void onPoiClick_sendsCorrectData() {
    // Assuming 'controller' and 'flutterApi' are set up as in other tests.
    PointOfInterest poi = new PointOfInterest(new LatLng(1.0, 2.0), "placeId", "name");

    controller.onPoiClick(poi);

    ArgumentCaptor<Messages.PlatformPointOfInterest> poiCaptor = ArgumentCaptor.forClass(Messages.PlatformPointOfInterest.class);
    verify(flutterApi).onPoiTap(poiCaptor.capture(), any(Messages.VoidResult.class));

    Messages.PlatformPointOfInterest capturedPoi = poiCaptor.getValue();
    assertEquals("name", capturedPoi.getName());
    assertEquals("placeId", capturedPoi.getPlaceId());
    assertEquals(1.0, capturedPoi.getPosition().getLatitude(), 1e-6);
    assertEquals(2.0, capturedPoi.getPosition().getLongitude(), 1e-6);
  }

Comment on lines 18 to 19
/// Callback method for when a [PointOfInterest] is tapped.
typedef POIClickCallback = void Function(PointOfInterest poi);

Choose a reason for hiding this comment

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

medium

The POIClickCallback typedef appears to be unused in the codebase. To maintain code cleanliness, please remove it.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.

This needs to follow our documented process for changing federated plugins, so that CI works.

Please also be sure to review the feedback on the previous PR, especially this comment, and update accordingly. Even if this PR doesn't include web support (although it would be preferable if it did, especially since there is a previous PR to draw on), it does need to be designed in such a way as to not cause this problem for web later.

@stuartmorgan-g stuartmorgan-g changed the title feat: [google_maps_flutter] Add onPoiTap callback to GoogleMap [google_maps_flutter] Add onPoiTap callback to GoogleMap Feb 4, 2026
),
);

// 🔥 IMPORTANT: Wait for the map initialization (onMapCreated) to complete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please follow our standard commenting style. Sentences should be properly punctuated, and we don't use emoji in comments.

);

// 🔥 IMPORTANT: Wait for the map initialization (onMapCreated) to complete
await tester.pumpAndSettle();
Copy link
Collaborator

Choose a reason for hiding this comment

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

pumpAndSettle is for waiting for rendering to complete, not for arbitrary specific tasks. The way to wait for onMapCreated to complete is to have a future that completes when onMapCreated is called, and then wait for that. It is important that tests not be flaky because they are using delays as a proxy for waiting for specific conditions.

Copy link
Author

Choose a reason for hiding this comment

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

Please review the package again. I have done the following works:

  • Revamped the code as per the review of @stuartmorgan-g and @gemini_code_assist
  • Added Support for flutter web for onPoiTap Callback
  • Added Integration test for onPoiTap
  • Updated Version for the google_maps_flutter_web package

Choose a reason for hiding this comment

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

Thank you for addressing the feedback regarding the pumpAndSettle usage. The updated test now correctly waits for onMapCreated to complete using a Completer, which is a much more robust approach.

@AsimRoyChowdhury AsimRoyChowdhury force-pushed the feature/google-maps-poi-click branch from 4512a35 to 3079afc Compare February 7, 2026 09:17
@stuartmorgan-g stuartmorgan-g marked this pull request as draft February 7, 2026 22:25
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

I've left some initial feedback.

This is not the first time I've had to point out that previous feedback has not been addressed. Please ensure that you either address feedback, or ask for clarification if necessary, before requesting review again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generated files should never be patched or reconciled in merges, they should be regenerated from the source. These files should absolutely not be part of the PR, but the fact that they existed locally suggests that you haven't ensured up-to-date generated output.

final Set<ClusterManager> clusterManagers;

/// Callback for Point of Interests tap
final ArgumentCallback<PointOfInterest>? onPoiTap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be grouped with, and following the same comment style as, all the other callbacks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change should have corresponding native unit tests.


/// A [PointOfInterest] has been tapped.
Stream<MapPoiTapEvent> onPoiTap({required int mapId}) {
throw UnimplementedError('onPoiTap() has not been implemented.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should return an empty stream, not throw, to avoid breaking existing implementations.

final Set<ClusterManager> clusterManagers;

/// Callback for Point of Interests tap
final ArgumentCallback<PointOfInterest>? onPoiTap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

My previous comment has not been addressed. This is still returning an inflated POI rather than an ID.

@stuartmorgan-g stuartmorgan-g marked this pull request as ready for review February 12, 2026 15:29
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces onPoiTap support for the GoogleMap widget, enabling tap detection on Points of Interest across Android, iOS, and web. The changes are well-implemented, touching the platform interface, all three platform implementations, and adding a new example page with tests. My review includes a minor code simplification suggestion and points out several patch-related artifact files that should be removed to maintain repository cleanliness. Overall, this is a great feature addition.

@@ -0,0 +1,11 @@
--- packages/google_maps_flutter/google_maps_flutter_android/example/lib/example_google_map.dart

Choose a reason for hiding this comment

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

high

This file appears to be a patch reject file (.rej) that was likely added by accident. Please remove it from the pull request to keep the repository clean.

@@ -0,0 +1,1574 @@
--- packages/google_maps_flutter/google_maps_flutter_android/lib/src/messages.g.dart

Choose a reason for hiding this comment

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

high

This .rej file seems to be a patch artifact and should not be included in the pull request. Please remove it.

@@ -0,0 +1,901 @@
// Copyright 2013 The Flutter Authors

Choose a reason for hiding this comment

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

high

This .orig file seems to be an artifact from a merge or patch tool. It should be removed from the pull request.

@@ -0,0 +1,807 @@
--- packages/google_maps_flutter/google_maps_flutter_ios/ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/include/google_maps_flutter_ios/google_maps_flutter_pigeon_messages.g.h

Choose a reason for hiding this comment

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

high

This patch reject file (.rej) appears to be an unintended artifact. Please remove it from the commit.

@@ -0,0 +1,1496 @@
--- packages/google_maps_flutter/google_maps_flutter_ios/lib/src/messages.g.dart

Choose a reason for hiding this comment

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

high

This .rej file is likely a patch artifact and should be removed from the pull request.

Comment on lines 620 to 622
if (widget.onPoiTap != null) {
widget.onPoiTap!(poi);
}

Choose a reason for hiding this comment

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

medium

For conciseness, you can use the null-aware ?.call() operator here.

    widget.onPoiTap?.call(poi);

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.

[google_maps_flutter] Add callback for click on google poi's on the map

2 participants