Skip to content

Commit

Permalink
fix: Use recomposed map listener callbacks rather than only the initi…
Browse files Browse the repository at this point in the history 
…ally … (#478)

* fix: Use recomposed map listener callbacks rather than only the initially composed version
Fixes #466

* Minor formatting fix

* Label leaf composable as @GoogleMapComposable for proper compile-time diagnostics

* Clarify documentation

* Address spurious subcomposition recompositions by delaying state updates to after parent composition, not during parent composition

* Delay GoogleMap object access until composition apply phase (see #501)

* Revert "Address spurious subcomposition recompositions by delaying state updates to after parent composition, not during parent composition"

This reverts commit dff2b0a.
  • Loading branch information
@bubenheimer
bubenheimer committed Jan 31, 2024
1 parent 26ec87e commit d14daba
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 26 deletions.
10 changes: 6 additions & 4 deletions maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -126,17 +125,19 @@ public fun GoogleMap(
val currentContent by rememberUpdatedState(content)
LaunchedEffect(Unit) {
disposingComposition {
mapView.newComposition(parentComposition) {
mapView.newComposition(parentComposition, mapClickListeners) {
MapUpdater(
mergeDescendants = mergeDescendants,
contentDescription = currentContentDescription,
cameraPositionState = currentCameraPositionState,
clickListeners = mapClickListeners,
contentPadding = currentContentPadding,
locationSource = currentLocationSource,
mapProperties = currentMapProperties,
mapUiSettings = currentUiSettings,
)

MapClickListenerUpdater()

CompositionLocalProvider(
LocalCameraPositionState provides currentCameraPositionState,
) {
Expand All @@ -158,11 +159,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)
}
Expand Down
6 changes: 6 additions & 0 deletions maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>()
Expand Down
138 changes: 138 additions & 0 deletions maps-compose/src/main/java/com/google/maps/android/compose/MapClickListeners.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,21 @@
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
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
Expand Down Expand Up @@ -56,3 +68,129 @@ 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 map: GoogleMap,
private val setter: GoogleMap.(L?) -> Unit,
private val listener: L
) : MapNode {
override fun onAttached() = setListener(listener)
override fun onRemoved() = setListener(null)
override fun onCleared() = setListener(null)

private fun setListener(listenerOrNull: L?) = map.setter(listenerOrNull)
}

@Suppress("ComplexRedundantLet")
@Composable
internal fun MapClickListenerUpdater() {
// The mapClickListeners container object is not allowed to ever change
val mapClickListeners = (currentComposer.applier as MapApplier).mapClickListeners

with(mapClickListeners) {
::indoorStateChangeListener.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnIndoorStateChangeListener,
object : OnIndoorStateChangeListener {
override fun onIndoorBuildingFocused() =
callback().onIndoorBuildingFocused()

override fun onIndoorLevelActivated(building: IndoorBuilding) =
callback().onIndoorLevelActivated(building)
}
)
}

::onMapClick.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnMapClickListener,
OnMapClickListener { callback()?.invoke(it) }
)
}

::onMapLongClick.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnMapLongClickListener,
OnMapLongClickListener { callback()?.invoke(it) }
)
}

::onMapLoaded.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnMapLoadedCallback,
OnMapLoadedCallback { callback()?.invoke() }
)
}

::onMyLocationButtonClick.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnMyLocationButtonClickListener,
OnMyLocationButtonClickListener { callback()?.invoke() ?: false }
)
}

::onMyLocationClick.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnMyLocationClickListener,
OnMyLocationClickListener { callback()?.invoke(it) }
)
}

::onPOIClick.let { callback ->
MapClickListenerComposeNode(
callback,
GoogleMap::setOnPoiClickListener,
OnPoiClickListener { callback()?.invoke(it) }
)
}
}
}

/**
* Encapsulates the ComposeNode factory lambda as a recomposition optimization.
*
* @param L GoogleMap click listener type, e.g. [OnMapClickListener]
* @param callback a property reference to the callback lambda, i.e.
* invoking it returns the callback lambda
* @param setter a reference to a GoogleMap setter method, e.g. `setOnMapClickListener()`
* @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: GoogleMap.(L?) -> Unit,
listener: L
) {
val mapApplier = currentComposer.applier as MapApplier

MapClickListenerComposeNode(callback) { MapClickListenerNode(mapApplier.map, setter, listener) }
}

@Composable
@GoogleMapComposable
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) {}
}
22 changes: 0 additions & 22 deletions maps-compose/src/main/java/com/google/maps/android/compose/MapUpdater.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand All @@ -116,7 +97,6 @@ internal inline fun MapUpdater(
mergeDescendants: Boolean = false,
contentDescription: String?,
cameraPositionState: CameraPositionState,
clickListeners: MapClickListeners,
contentPadding: PaddingValues = NoPadding,
locationSource: LocationSource?,
mapProperties: MapProperties,
Expand All @@ -135,7 +115,6 @@ internal inline fun MapUpdater(
map = map,
contentDescription = contentDescription,
cameraPositionState = cameraPositionState,
clickListeners = clickListeners,
density = density,
layoutDirection = layoutDirection,
)
Expand Down Expand Up @@ -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 }
}
}

0 comments on commit d14daba

Please sign in to comment.