[google_maps_flutter] Add onPoiTap callback to GoogleMap - #10963

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

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.

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.

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);
  }

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

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

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.

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

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.

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.

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

This change should have corresponding native unit tests.

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

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

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.

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

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

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

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

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

    widget.onPoiTap?.call(poi);

@stuartmorgan-g can you please see the code changes again. I have removed the name param from the onPoiTap. As you said that it sent inflated POI values in the web version. In the web version the name is not there in the native implementation which is why the inflated values were returned that's why I have removed the name param from all platform implementations and kept only the placeId and position.

As the unimplemented error was thrown which would have resulted in a breaking change so I have changed it to return an empty string instead of throwing.

I also applied the make-deps-path-based tooling but it was resulting in blank dependency overrides so I had to add the dependency overrides manually. Please help me in understanding this if I am using the tool incorrectly.

Above all thank you for your constant valuable feedback on the errors I was doing continuously, it would be great if you explain more about the errors if those happen again, and I have read the documentation regarding the federated plugin change and did all what I understood, please guide me if I have left something unaddressed.

I have removed the name param from the onPoiTap.

It appears that you have removed the name from the PointOfInterest class, which would make it impossible to have the method discussed in the linked comment thread that would query for POI information.

The issue is not that POI doesn't have a name on web, it's that the tap event callback doesn't return a POI on web. That can't be fixed by removing information from POI itself. It needs to be solved by having the tap handler receive something that is not the POI class.

@stuartmorgan-g Thank you for the clarification. That makes perfect sense regarding not removing data from the core PointOfInterest class.

Before I write the code and push another combination PR, I want to make sure my planned architecture aligns exactly with your expectations:

1. Restore PointOfInterest: I will revert the PointOfInterest class so it keeps its non-nullable name property, preserving it as a complete data model.
2. Create a dedicated Event/Details Class: I will create a separate class specifically for the tap callback payload (e.g., PointOfInterestTapDetails or MapPoiTapEvent).
3. Make name nullable in the Event: In this new event class, name will be String?.
4. Platform Implementations: Android and iOS will pass the actual POI name to this event. The Web implementation will safely pass null for the name since the Web SDK doesn't provide it.

Does this separation between the core POI class and the tap event payload resolve the issue?

My expectation is that you follow the architecture laid out in the comment I have referred you to several times, which is not what you just described. Before I spend more of my time on this review, I would like a clear, explicit answer to my previous question about how you are using AI here, and how much human involvement there is.

@stuartmorgan-g Thank you for your time and patience. I am trying to grasp on the architecture that you have referred in the comments. I have used AI to learn about the CI errors I was going through and therefore trying my best to contribute to the package.

I am trying to grasp on the architecture that you have referred in the comments.

A key point is this section of the comment:

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);)

An event payload class that has auto-hydrated POI data (such as a String? name) is not consistent with that.