fix: Use recomposed map listener callbacks rather than only the initially …

[bubenheimer]

…composed version

Fixes #466

This solution is optimized for the common case of recomposition updating a non-null GoogleMap listener callback to another non-null callback, and has negligible overhead, supporting rapid recomposition.

The other, less common case, toggling between null and non-null callbacks, calls the appropriate GoogleMap SDK listener setter in the composition apply phase; any overhead from calling the setter is passed through.

Thank you for opening a Pull Request!

---

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

[bubenheimer]

@kikoso would you have time for a review? Happy to explain changes. Would like to get it merged soon to not let the issue fester longer. Have a few other PRs waiting on this one.

[kikoso]

Hi @bubenheimer !

Thanks for putting this PR together. Since there are a few that could depend on the output of this one, let's focus on getting this one merged first.

The internal behavior of Google Maps follows this approach (using a POI and OnPoiClickListener as a reference):

• If a POI is clicked but if there is no OnPoiClickListener set, the OnMapClickListener will be invoked instead (given that it has been previously set). If none of those listeners has been set, the click is still “captured” internally by the map, and triggers any default behavior (for instance, hiding the toolbar if it's displayed).

• For markers, if a marker is clicked but does not have an OnMarkerClickListener set, again the click is still “captured” by the map to trigger default behavior (camera moves to the marker, info window appears and toolbar displayed if enabled).

As a rule of thumb, the map invokes any user-provided listeners first, and then afterwards processes internally the event to perform any other actions.

From your approach: the fact that it is not a breaking change, and that the logic of the MapUpdater stays relatively unchanged is good.

The PR seems to honor that A) Recomposed callbacks are working, and B) They default to the Google Map listener if they are null.

The PR looks good. Let me check if there are no further issues and get back!.

[kikoso]

Hi @bubenheimer,

This looks good to be merged, will do it soon.

[bubenheimer]

Bump

[bubenheimer]

I've reverted the earlier commit that moved the initial parts of updating the listener callbacks into a SideEffect. The SideEffect approach delays the listener updates to a future recomposition cycle, which I have to come to consider too slow.

Without SideEffect there is the issue of spurious recompositions that I believe are fast and do essentially nothing. However, the listeners are updated more quickly. I have not seen issues in terms of affected code behaving incorrectly because of the spurious recompositions.

The spurious recompositions are not specific to this PR, they are prone to happen with the current GoogleMap() subcomposition approach in general: #480

[googlemaps-bot]

🎉 This PR is included in version 4.3.3 🎉

The release is available on:

• v4.3.3
• GitHub release

Your semantic-release bot 📦🚀