ably / ably-asset-tracking-android

Android client SDKs for the Ably Asset Tracking service.
Apache License 2.0
13 stars 6 forks source link

CoreLocationAnimator is not compatible with Jetpack Compose Maps #681

Closed ikurek closed 2 years ago

ikurek commented 2 years ago

CoreLocationAnimator exposes a Flow of camera updates for Google Maps. This is intended to be used with googleMap.animateCamera() to achieve a smooth camera animation, and it works okay with the classic approach to Android UI (with XML maps).

Jetpack Compose Maps provides two methods to interact with the map camera - animate() and move(). While the move() method can be used with CoreLocationAnimator, the animate() method can't. The definition of animate() looks like this:

@UiThread
suspend fun animate(update: CameraUpdate, durationMs: Int = MAX_VALUE)

so the function is suspending, and according to the documentation, will throw CancellationException if the animation does not fully complete. However it's not possible to call a suspending function from inside the Composable, so a structure like this is NOT possible:

    GoogleMap(
        modifier = Modifier.fillMaxSize(),
        cameraPositionState = cameraPositionState
    ) {
            cameraPositionState.animate(     // <-- Can't be done because animate is suspending
                CameraUpdateFactory.newLatLngZoom(
                    mapState.cameraLatLng()!!,
                    zoomLevel
                )
            Circle(
                center = mapState.locationLatLng()!!,
                radius = mapState.location.accuracy.toDouble(),
                strokeColor = Color.Blue,
                fillColor = Color.Blue,
            )
        }
    }

It is possible to call animate() from outside of the composable, but then it's required to perform the animation in try ... catch ... block to catch CancellationException, and a lot of Coroutines has to be launched to perform a smooth animation.

It may be a good idea to rethink the UI animation approach for Compose, and maybe think about implementing an alternative solution.

┆Issue is synchronized with this Jira Task by Unito

KacperKluka commented 2 years ago

While it is true that the current camera updates implementation does not support Jetpack Compose Maps out of the box, as you said it is still possible to make it work by wrapping the code that throws an exception in a try / catch block. Even if you have to launch a lot of coroutines it should not affect the performance as they are light-weight.

We've not intended to make the LocationAnimator support any specific map provider, we've tried to make it as generic as possible so then users can adjust their implementation to the map. Therefore, I'll change the label from bug to enhancement but most likely it will be resolved by https://github.com/ably/ably-asset-tracking-android/issues/683. :wink:

KacperKluka commented 2 years ago

Because changing location animator camera updates behaviour introduces some UX issues described here, we won't change it and therefore this issue will be closed as well :wink: