googlemaps / android-maps-compose

Jetpack Compose composables for the Maps SDK for Android
https://developers.google.com/maps/documentation/android-sdk/maps-compose
Apache License 2.0
1.16k stars 139 forks source link

Setting CameraPositionState.position is not stateful #523

Open bubenheimer opened 9 months ago

bubenheimer commented 9 months ago

android-maps-compose 4.3.3

Setting CameraPositionState.position is not stateful. (Setting the property calls through to the GoogleMap SDK and immediately changes the camera position on the map.) This is an invalid Compose architectural decision:

  1. Reading this property is stateful: it reads from a state.
  2. The class has State in its name, and position is its primary public property, so setting it must be stateful to not completely defy user expectations.
  3. Not even the KDoc mentions setting not being stateful.
  4. By contrast, setting MarkerState.position is stateful. The behavior of CameraPositionState is different when it should be the same.

The general impact is that setting it via snapshot state does not work correctly. This is a fundamental violation of normal Compose behavior.

For example, a user might attempt something like this to control camera position, which mimics the behavior of rememberUpdatedState(), but for CameraPositionState instead of MutableState:

@Composable
fun MapWithCamera(cameraPosition: CameraPosition) {
    val cameraState = rememberCameraPositionState(position = cameraPosition)
        .also { it.position = cameraPosition }

    GoogleMap(
        cameraPositionState = cameraState
    )
}

This approach is normally sound, but not valid here, because setting CameraStatePosition.position is a side effect (irreversible), instead of setting snapshot state. For example, if the composition is cancelled, the new camera position remains set.

CameraPositionState.move() can only be called from the main thread, so disallowing setter access in favor of this method is not a great workaround.

https://github.com/googlemaps/android-maps-compose/blob/f857fc48bf1a05191d363cd8fdb19692b284fc30/maps-compose/src/main/java/com/google/maps/android/compose/CameraPositionState.kt#L97-L111

bubenheimer commented 9 months ago

Another consequence: updating padding and camera position together is not possible - camera will always be set instantly, then eventually padding when applying composition. This means that the camera will not be centered within the updated padding.

If CameraPositionState.position was backed by MutableState, then the API could update padding and camera in the appropriate order.

This is related to #142

CaolanCode commented 7 months ago

This issue remains unresolved.