From 2115e482d0c4458e928662ddee052153a51f3cc6 Mon Sep 17 00:00:00 2001 From: Uli Bubenheimer Date: Sat, 16 Dec 2023 15:05:21 -0500 Subject: [PATCH] fix: Use recomposed map listener callbacks rather than only the initially composed version Fixes #466 --- .../google/maps/android/compose/GoogleMap.kt | 10 +- .../google/maps/android/compose/MapApplier.kt | 6 + .../maps/android/compose/MapClickListeners.kt | 128 ++++++++++++++++++ .../google/maps/android/compose/MapUpdater.kt | 22 --- 4 files changed, 140 insertions(+), 26 deletions(-) diff --git a/maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt b/maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt index 2d292a2a0..8e89cb2a5 100644 --- a/maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt +++ b/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) } diff --git a/maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt b/maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt index 2c9ef6fbf..36e488b0b 100644 --- a/maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt +++ b/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(MapNodeRoot) { private val decorations = mutableListOf() diff --git a/maps-compose/src/main/java/com/google/maps/android/compose/MapClickListeners.kt b/maps-compose/src/main/java/com/google/maps/android/compose/MapClickListeners.kt index 15b5a124a..ac3514389 100644 --- a/maps-compose/src/main/java/com/google/maps/android/compose/MapClickListeners.kt +++ b/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( + 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 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, MapApplier>(factory) {} +} diff --git a/maps-compose/src/main/java/com/google/maps/android/compose/MapUpdater.kt b/maps-compose/src/main/java/com/google/maps/android/compose/MapUpdater.kt index f7ea02570..f27a000d3 100644 --- a/maps-compose/src/main/java/com/google/maps/android/compose/MapUpdater.kt +++ b/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 } } }