Use recomposed map listener callbacks rather than only the initially …

…composed version

Fixes googlemaps#326

maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt

@@ -36,7 +36,6 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.platform.LocalInspectionMode
 import androidx.compose.ui.platform.LocalLifecycleOwner
-import androidx.compose.ui.semantics.semantics
 import androidx.compose.ui.viewinterop.AndroidView
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.LifecycleEventObserver
@@ -125,17 +124,19 @@ public fun GoogleMap(
     val currentContent by rememberUpdatedState(content)
     LaunchedEffect(Unit) {
         disposingComposition {
-            mapView.newComposition(parentComposition) {
+            mapView.newComposition(parentComposition, mapClickListeners) {
                 MapUpdater(
                     mergeDescendants = mergeDescendants,
                     contentDescription = contentDescription,
                     cameraPositionState = currentCameraPositionState,
-                    clickListeners = mapClickListeners,
                     contentPadding = currentContentPadding,
                     locationSource = currentLocationSource,
                     mapProperties = currentMapProperties,
                     mapUiSettings = currentUiSettings,
                 )
+
+                MapClickListenerUpdater()
+
                 CompositionLocalProvider(
                     LocalCameraPositionState provides cameraPositionState,
                 ) {
@@ -157,11 +158,12 @@ internal suspend inline fun disposingComposition(factory: () -> Composition) {
 
 private suspend inline fun MapView.newComposition(
     parent: CompositionContext,
+    mapClickListeners: MapClickListeners,
     noinline content: @Composable () -> Unit
 ): Composition {
     val map = awaitMap()
     return Composition(
-        MapApplier(map, this), parent
+        MapApplier(map, this, mapClickListeners), parent
     ).apply {
         setContent(content)
     }

maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt

@@ -31,9 +31,15 @@ internal interface MapNode {
 
 private object MapNodeRoot : MapNode
 
+// [mapClickListeners] must be a singleton for the [map] and is therefore stored here:
+// [GoogleMap.setOnIndoorStateChangeListener()] will not actually set a new non-null listener if
+// called more than once; if [mapClickListeners] were passed through the Compose function hierarchy
+// we would need to consider the case of it changing, which would require special treatment
+// for that particular listener; yet MapClickListeners never actually changes.
 internal class MapApplier(
     val map: GoogleMap,
     internal val mapView: MapView,
+    val mapClickListeners: MapClickListeners,
 ) : AbstractApplier<MapNode>(MapNodeRoot) {
 
     private val decorations = mutableListOf<MapNode>()

maps-compose/src/main/java/com/google/maps/android/compose/MapClickListeners.kt

@@ -15,9 +15,20 @@
 package com.google.maps.android.compose
 
 import android.location.Location
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.ComposeNode
+import androidx.compose.runtime.NonRestartableComposable
+import androidx.compose.runtime.currentComposer
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.setValue
+import com.google.android.gms.maps.GoogleMap.OnIndoorStateChangeListener
+import com.google.android.gms.maps.GoogleMap.OnMapClickListener
+import com.google.android.gms.maps.GoogleMap.OnMapLoadedCallback
+import com.google.android.gms.maps.GoogleMap.OnMapLongClickListener
+import com.google.android.gms.maps.GoogleMap.OnMyLocationButtonClickListener
+import com.google.android.gms.maps.GoogleMap.OnMyLocationClickListener
+import com.google.android.gms.maps.GoogleMap.OnPoiClickListener
 import com.google.android.gms.maps.model.IndoorBuilding
 import com.google.android.gms.maps.model.LatLng
 import com.google.android.gms.maps.model.PointOfInterest
@@ -56,3 +67,120 @@ internal class MapClickListeners {
     var onMyLocationClick: ((Location) -> Unit)? by mutableStateOf(null)
     var onPOIClick: ((PointOfInterest) -> Unit)? by mutableStateOf(null)
 }
+
+/**
+ * @param L GoogleMap click listener type, e.g. [OnMapClickListener]
+ */
+internal class MapClickListenerNode<L : Any>(
+    private val setter: (L?) -> Unit,
+    private val listener: L
+) : MapNode {
+    override fun onAttached() = setter(listener)
+    override fun onRemoved() = setter(null)
+    override fun onCleared() = setter(null)
+}
+
+@Suppress("ComplexRedundantLet")
+@Composable
+internal fun MapClickListenerUpdater() {
+    val mapApplier = currentComposer.applier as MapApplier
+
+    // The mapClickListeners container object is not allowed to ever change
+    with (mapApplier.mapClickListeners) {
+        with(mapApplier.map) {
+            ::indoorStateChangeListener.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnIndoorStateChangeListener,
+                    object : OnIndoorStateChangeListener {
+                        override fun onIndoorBuildingFocused() =
+                            callback().onIndoorBuildingFocused()
+
+                        override fun onIndoorLevelActivated(building: IndoorBuilding) =
+                            callback().onIndoorLevelActivated(building)
+                    }
+                )
+            }
+
+            ::onMapClick.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnMapClickListener,
+                    OnMapClickListener { callback()?.invoke(it) }
+                )
+            }
+
+            ::onMapLongClick.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnMapLongClickListener,
+                    OnMapLongClickListener { callback()?.invoke(it) }
+                )
+            }
+
+            ::onMapLoaded.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnMapLoadedCallback,
+                    OnMapLoadedCallback { callback()?.invoke() }
+                )
+            }
+
+            ::onMyLocationButtonClick.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnMyLocationButtonClickListener,
+                    OnMyLocationButtonClickListener { callback()?.invoke() ?: false }
+                )
+            }
+
+            ::onMyLocationClick.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnMyLocationClickListener,
+                    OnMyLocationClickListener { callback()?.invoke(it) }
+                )
+            }
+
+            ::onPOIClick.let { callback ->
+                MapClickListenerComposeNode(
+                    callback,
+                    ::setOnPoiClickListener,
+                    OnPoiClickListener { callback()?.invoke(it) }
+                )
+            }
+        }
+    }
+}
+
+/**
+ * Encapsulates the ComposeNode factory lambda as a recomposition optimization.
+ *
+ * @param L GoogleMap click listener type, e.g. [OnMapClickListener]
+ * @param listener must include a call to [callback()] inside the listener
+ * to use the most up-to-date recomposed version of the callback lambda;
+ * However, the resulting callback reference might actually be null due to races;
+ * the caller must guard against this case.
+ *
+ */
+@Composable
+@NonRestartableComposable
+private fun <L : Any> MapClickListenerComposeNode(
+    callback: () -> Any?,
+    setter: (L?) -> Unit,
+    listener: L
+) = MapClickListenerComposeNode(callback) { MapClickListenerNode(setter, listener) }
+
+@Composable
+private fun MapClickListenerComposeNode(
+    callback: () -> Any?,
+    factory: () -> MapClickListenerNode<*>
+) {
+    // Setting a GoogleMap listener may have side effects, so we unset it as needed.
+    // However, the listener is reset only when the corresponding callback lambda
+    // toggles between null and non-null. This is to avoid potential performance problems
+    // when callbacks recompose rapidly; setting GoogleMap listeners could potentially be
+    // expensive due to synchronization, etc. GoogleMap listeners are not designed with a
+    // use case of rapid recomposition in mind.
+    if (callback() != null) ComposeNode<MapClickListenerNode<*>, MapApplier>(factory) {}
+}

maps-compose/src/main/java/com/google/maps/android/compose/MapUpdater.kt

@@ -26,13 +26,11 @@ import androidx.compose.ui.unit.Density
 import androidx.compose.ui.unit.LayoutDirection
 import com.google.android.gms.maps.GoogleMap
 import com.google.android.gms.maps.LocationSource
-import com.google.android.gms.maps.model.IndoorBuilding
 
 internal class MapPropertiesNode(
     val map: GoogleMap,
     cameraPositionState: CameraPositionState,
     contentDescription: String?,
-    var clickListeners: MapClickListeners,
     var density: Density,
     var layoutDirection: LayoutDirection,
 ) : MapNode {
@@ -76,23 +74,6 @@ internal class MapPropertiesNode(
         map.setOnCameraMoveListener {
             cameraPositionState.rawPosition = map.cameraPosition
         }
-
-        map.setOnMapClickListener(clickListeners.onMapClick)
-        map.setOnMapLongClickListener(clickListeners.onMapLongClick)
-        map.setOnMapLoadedCallback(clickListeners.onMapLoaded)
-        map.setOnMyLocationButtonClickListener { clickListeners.onMyLocationButtonClick?.invoke() == true }
-        map.setOnMyLocationClickListener(clickListeners.onMyLocationClick)
-        map.setOnPoiClickListener(clickListeners.onPOIClick)
-
-        map.setOnIndoorStateChangeListener(object : GoogleMap.OnIndoorStateChangeListener {
-            override fun onIndoorBuildingFocused() {
-                clickListeners.indoorStateChangeListener.onIndoorBuildingFocused()
-            }
-
-            override fun onIndoorLevelActivated(building: IndoorBuilding) {
-                clickListeners.indoorStateChangeListener.onIndoorLevelActivated(building)
-            }
-        })
     }
 
     override fun onRemoved() {
@@ -116,7 +97,6 @@ internal inline fun MapUpdater(
     mergeDescendants: Boolean = false,
     contentDescription: String?,
     cameraPositionState: CameraPositionState,
-    clickListeners: MapClickListeners,
     contentPadding: PaddingValues = NoPadding,
     locationSource: LocationSource?,
     mapProperties: MapProperties,
@@ -135,7 +115,6 @@ internal inline fun MapUpdater(
                 map = map,
                 contentDescription = contentDescription,
                 cameraPositionState = cameraPositionState,
-                clickListeners = clickListeners,
                 density = density,
                 layoutDirection = layoutDirection,
             )
@@ -181,6 +160,5 @@ internal inline fun MapUpdater(
         set(mapUiSettings.zoomGesturesEnabled) { map.uiSettings.isZoomGesturesEnabled = it }
 
         update(cameraPositionState) { this.cameraPositionState = it }
-        update(clickListeners) { this.clickListeners = it }
     }
 }