feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts #436

philip-segerfast · 2023-10-13T11:34:17Z

This PR uses an AndroidView overload which allows re-using of the underlying view. This improves performance substantially when using GoogleMap in a Lazy layout, like LazyColumn.

This PR also incorporates @bubenheimer's changes from #522.

Fixes #285, #492 🦕

merge latest

This will make sure that the underlying MapView is re-used in a LazyColumn (and elsewhere) which will significantly improve scrolling performance in LazyColumn. If this overload isn't used the MapView will be destroyed every time it leaves composition. The MapView will still be destroyed when the parent node leaves the composition.

Issue: https://issuetracker.google.com/issues/267642562

google-cla · 2023-10-13T11:34:22Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

kikoso · 2023-10-27T15:14:45Z

Hi @philip-segerfast. As far as I read in the documentation, we need to provide a non-null value in the onReset method. This PR does not seem to include it.

philip-segerfast · 2023-10-27T16:28:45Z

Hi @kikoso, it is enough to make onReset non-null. You can verify by commenting that line out in my PR and you will see that the views are re-created when scrolling.
It still needs some more work though, mainly the GoogleMap state should be reset (for example animations). That's why I made this PR into a draft.
I'll try to make another commit or get back to you regarding this issue during the weekend.

Cheers

philip-segerfast · 2023-10-29T13:18:54Z

Upon further examination, it appears that executing a reset within onReset may not be necessary since dispositions already clear markers and halt animations, although the underlying GoogleMap's camera position state appears to be untouched.

Some other notes:

The issue concerning the zoom controls is unrelated to this PR, and appears to stem from a different source. You can reproduce on the main branch by putting a bunch of GoogleMap composables in a LazyColumn.
Additionally, version 1.6.0-alpha08 of androidx.compose.ui:ui harbors a bug leading to a crash, hence it is advisable to avoid using it.

The PR is quite ready except for the androidx.compose.ui:ui version being in alpha.

wangela · 2023-11-17T17:48:39Z

We'll wait until this feature moves out of alpha and doesn't cause crashes before we merge.

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

…View

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

philip-segerfast · 2024-04-24T16:22:17Z

Thanks for the review and everything in between, Uli @bubenheimer! It was an honor.
Don't hesitate to request more changes if there is anything.

bubenheimer · 2024-04-24T16:23:06Z

Looks great! LGT @kikoso

…view

ivannarino

LGTM

…view

el-qq · 2024-05-08T07:22:51Z

@dkhawk Would you look at the PR and include it in the plans please?

dkhawk · 2024-05-08T23:43:54Z

@dkhawk Would you look at the PR and include it in the plans please?

This is a great change! I spent some time with the demo activity and it works pretty well, except I can't scrolling the maps vertically. That might be nice to look into. Barring that we should be able to merge in this change into the 5.1.x release which should be soon.

bubenheimer · 2024-05-09T13:38:12Z

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

+    fun CoroutineScope.launchSubcomposition(mapView: MapView): Job {
+        val mapCompositionContent: @Composable () -> Unit = {
+            MapUpdater(
+                mergeDescendants = mergeDescendants,
+                contentDescription = currentContentDescription,
+                cameraPositionState = currentCameraPositionState,
+                contentPadding = currentContentPadding,
+                locationSource = currentLocationSource,
+                mapProperties = currentMapProperties,
+                mapUiSettings = currentUiSettings,
+            )


It would be useful to move launchSubcomposition() to the top level, to help avoid direct references to GoogleMap() parameters from the long-running lambda closure. There have been subtle bugs in this project caused by such references, they're easy to miss.

This should incorporate the changes from recently approved #522, grouping together all MapUpdater() parameters.

Alright 👍

I've incorporated the changes from #522. Please tell me if there's anything you'd like to change.

Looks great, thank you! The MapUpdater() changes are excellent.

bubenheimer · 2024-05-09T13:44:07Z

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

+        return launch(start = CoroutineStart.UNDISPATCHED) {
+            val map = mapView.awaitMap()
+            val composition = Composition(
+                applier = MapApplier(map, mapView, mapClickListeners),


mapClickListeners should be moved out of MapApplier, and passed as a parameter to MapClickListenerUpdater(). @philip-segerfast determined earlier in the review process that MapClickListeners does not actually have to be a singleton. (I verified the finding.)

It's a trivial change that will also prepare MapApplier for potential future use in ReusableComposition.

I've implemented this. Please tell me what you think.

@philip-segerfast I've realized that the MapClickListeners change that I proposed is not valid, sadly. It will work in this case, because MapClickListeners actually is a singleton within the scope of a non-reused composition anyway. However, from the perspective of MapClickListenerUpdater(MapClickListeners) as a standalone Composable function it is not correct, as passing a different MapClickListener on recomposition will not do the right thing. The initial listener is too baked into the lambdas there.

Let's undo this part of the commit and leave it for the future, it does not look trivial. My sincere apologies!

No worries. This has been reverted.

philip-segerfast · 2024-05-09T15:10:53Z

Hi, @dkhawk! I made the maps pannable in the demo if you first pan them horizontally so that you still can easily scroll the list. Is this enough for you or would you like the maps to completely intercept the touch events from the LazyColumn like in MapInColumnActivity?

…operty from MapApplier [see desc.] - Wrap MapUpdater parameters inside a MapUpdaterState data class - Remove MapClickListeners property from MapApplier - Pass MapClickListeners directly to MapClickListenerUpdater

bubenheimer · 2024-05-10T10:44:22Z

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

+    mapUpdaterState: MapUpdaterState,
+    parentComposition: CompositionContext,
+    clickListeners: MapClickListeners,
+    content: (@Composable @GoogleMapComposable () -> Unit)?,
+    mapView: MapView


nit: the parameter order is non-standard. The content lambda should be the last parameter.

Also, content is now a regular non-nullable lambda after the latest changes from main. This will also slightly simplify its usage below.

Good point. fixed.

bubenheimer · 2024-05-10T11:04:15Z

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

-        event.targetState
-        when (event) {
-            Lifecycle.Event.ON_CREATE -> {
-                // Skip calling mapView.onCreate if the lifecycle did not go through onDestroy - in
-                // this case the GoogleMap composable also doesn't leave the composition. So,
-                // recreating the map does not restore state properly which must be avoided.
-                if (previousState.value != Lifecycle.Event.ON_STOP) {
-                    this.onCreate(Bundle())
+                CompositionLocalProvider(
+                    LocalCameraPositionState provides currentCameraPositionState
+                ) {
+                    content?.invoke()


currentCameraPositionState is not valid here. After the content type change, this whole expression can change to simply:

CompositionLocalProvider( LocalCameraPositionState provides mapUpdaterState.cameraPositionState, content )

…sitionState`

philip-segerfast added 5 commits October 5, 2023 16:04

Merge pull request #1 from googlemaps/main

1421dc7

merge latest

Update androidx.compose.ui:ui library to fix AndroidView bug

284f93a

Issue: https://issuetracker.google.com/issues/267642562

Add MapsInLazyColumnActivity

6968d1e

Hoist list items state

edc75c4

blunderbuss-gcf bot assigned wangela Oct 13, 2023

philip-segerfast changed the title ~~Android view overload re use map view~~ feat: Android view overload re use map view Oct 13, 2023

philip-segerfast changed the title ~~feat: Android view overload re use map view~~ feat: Use AndroidView overload which re-uses MapView to improve performance Oct 13, 2023

philip-segerfast changed the title ~~feat: Use AndroidView overload which re-uses MapView to improve performance~~ feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts Oct 13, 2023

philip-segerfast marked this pull request as draft October 24, 2023 18:35

philip-segerfast added 6 commits October 28, 2023 22:54

Bump compose-ui dependency

7c62ce7

Update libs.versions.toml

c1f13b0

Show list of countries in MapsInLazyColumnActivity

6b85dab

Cleanup

9c4dd29

Merge branch 'main' into android-view-overload-re-use-map-view

5a4073b

Merge branch 'main' into android-view-overload-re-use-map-view

32e3230

philip-segerfast marked this pull request as ready for review October 29, 2023 13:19

wangela added the status: blocked Resolving the issue is dependent on other work. label Nov 17, 2023

Merge branch 'googlemaps:main' into main

7de5d02

bubenheimer suggested changes Jan 29, 2024

View reviewed changes

maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt Outdated Show resolved Hide resolved

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

philip-segerfast added 5 commits February 1, 2024 17:16

Remove compose-ui dependency version override

2f5f0b6

Merge branch 'googlemaps:main' into main

e012317

Update libs.versions.toml

f38a3a1

Add parameter to GoogleMap to allow user to re-use the underlying Map…

44c524f

…View

Merge branch 'main' into android-view-overload-re-use-map-view

17fbec4

philip-segerfast added 5 commits April 24, 2024 16:39

Move lifecycleOwner inside object : View.OnAttachStateChangeListener

c562f3b

Fix comment

3bbb774

Convert registerComponentCallbacks to a top-level class

bbc6ac1

Inline componentCallbacks class

4af88f9

Store reference to Lifecycle instead of LifecycleOwner

348658f

bubenheimer suggested changes Apr 24, 2024

View reviewed changes

maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt Outdated Show resolved Hide resolved

Update comment

a4cdd8b

bubenheimer approved these changes Apr 24, 2024

View reviewed changes

philip-segerfast added 2 commits April 24, 2024 18:29

Fix typo

b26f55d

Merge branch 'googlemaps:main' into android-view-overload-re-use-map-…

2e86585

…view

ivannarino approved these changes Apr 30, 2024

View reviewed changes

kikoso removed the status: blocked Resolving the issue is dependent on other work. label May 1, 2024

philip-segerfast added 2 commits May 3, 2024 09:38

Merge branch 'googlemaps:main' into android-view-overload-re-use-map-…

748df47

…view

Remove unused TAG constant

653c857

bubenheimer approved these changes May 7, 2024

View reviewed changes

Demo - Set buildingFocused state on initial composition

0a3a0e5

bubenheimer reviewed May 9, 2024

View reviewed changes

Make maps in MapsInLazyColumnActivity pannable

99810a7

philip-segerfast added 3 commits May 9, 2024 17:14

Merge branch 'main' into android-view-overload-re-use-map-view

14ea4c7

Merge branch 'main' into android-view-overload-re-use-map-view

e519915

bubenheimer reviewed May 10, 2024

View reviewed changes

bubenheimer suggested changes May 10, 2024

View reviewed changes

philip-segerfast added 2 commits May 10, 2024 13:31

Revert MapClickListeners change + fix parameter order convention

cff3b49

Use mapUpdaterState.cameraPositionState instead of `currentCameraPo…

a917ed0

…sitionState`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts #436

feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts #436

philip-segerfast commented Oct 13, 2023 •

edited

google-cla bot commented Oct 13, 2023

kikoso commented Oct 27, 2023

philip-segerfast commented Oct 27, 2023 •

edited

philip-segerfast commented Oct 29, 2023

wangela commented Nov 17, 2023

philip-segerfast commented Apr 24, 2024 •

edited

bubenheimer commented Apr 24, 2024

ivannarino left a comment

el-qq commented May 8, 2024

dkhawk commented May 8, 2024

bubenheimer May 9, 2024

philip-segerfast May 9, 2024

philip-segerfast May 9, 2024

bubenheimer May 9, 2024

bubenheimer May 9, 2024

philip-segerfast May 9, 2024

bubenheimer May 9, 2024

philip-segerfast May 10, 2024

philip-segerfast commented May 9, 2024

bubenheimer May 10, 2024

bubenheimer May 10, 2024

philip-segerfast May 10, 2024

bubenheimer May 10, 2024

philip-segerfast May 10, 2024

feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts #436

Are you sure you want to change the base?

feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts #436

Conversation

philip-segerfast commented Oct 13, 2023 • edited

google-cla bot commented Oct 13, 2023

kikoso commented Oct 27, 2023

philip-segerfast commented Oct 27, 2023 • edited

philip-segerfast commented Oct 29, 2023

wangela commented Nov 17, 2023

philip-segerfast commented Apr 24, 2024 • edited

bubenheimer commented Apr 24, 2024

ivannarino left a comment

Choose a reason for hiding this comment

el-qq commented May 8, 2024

dkhawk commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-segerfast commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-segerfast commented Oct 13, 2023 •

edited

philip-segerfast commented Oct 27, 2023 •

edited

philip-segerfast commented Apr 24, 2024 •

edited