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

[philip-segerfast]

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 🦕

[google-cla[bot]]

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]

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]

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]

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]

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

[bubenheimer]

@philip-segerfast could you verify the OnIndoorStateChangeListener behavior that I mentioned earlier? I suspect it will no longer work upon reuse, because we've lost the reference to the singleton listener. I'm not 100% sure, though, because we would request a fresh GoogleMap object upon reuse.

Not sure if there is a better way to store the listener than in MapView.tag. We would not store the MapClickListeners object, just the actual GoogleMap.OnIndoorStateChangeListener object:

https://github.com/googlemaps/android-maps-compose/blob/d14daba053a446676e37e847a06d4d405f1b0117/maps-compose/src/main/java/com/google/maps/android/compose/MapClickListeners.kt#L98-L104

If we have to use MapView.tag anyway, then you may want to refactor this PR a bit, to utilize that storage option better.

[philip-segerfast]

I have tested to save the entire MapClickListeners object to MapView.tag and it works well. I haven't managed to make this work properly with only storing OnIndoorStateChangeListener. You also added a comment to MapClickListenerUpdater that "The mapClickListeners container object is not allowed to ever change" which tells me that it makes sense to store this object in the tag.

Then if this node tree is reused by another composable then it just changes the state values of the MapClickListeners object.

Is there a particular reason why you didn't want to store the entire MapClickListeners singleton object?

[philip-segerfast]

Notes on implementation:

• Converted all ComposeNodes to ReusableComposeNodes
• Use ReusableComposition instead of Composition for GoogleMap composition
• Create/activate Composition when AndroidView is inflated from update instead of using a LaunchedEffect block.
• Composition is disposed from AndroidView's onRelease callback instead of when composable is disposed.
• The MapClickListeners and Composition objects are stored in the MapView's tag so that they are not lost after reuse.

[philip-segerfast]

~~There is a bug where if you scroll too fast the map lifecycle callbacks are not invoked properly and they break.~~

Edit: fixed

[bubenheimer]

@philip-segerfast can you split the changes regarding ComposeNode -> ReusableComposeNode, and Composition -> ReusableComposition into a separate PR, please? They can be independent of the AndroidView changes, and this PR is getting too big for me to confidently review.

I'm not convinced that those will help gain much performance, either; I'd think that 95% of the initial composition work has to be redone upon reuse, because it's all about slots (which are discarded upon reuse) and manipulating the GoogleMap object and other objects from the Play SDK.

The ReusableComposition holds on to the applier, which holds on to a lot of stuff. That makes this PR much riskier, in terms of leaks and reuse behaviors.

I think it also depends very much on your use case whether ReusableComposition makes sense, and how much of its node structure makes sense to reuse. I might just want to reuse the MapView/GoogleMap objects, but use different Map contents every time. Or maybe I want a specific type of half-populated Map on reuse where I can derive performance benefits from not redoing the entire subcomposition structure every time.

Might be best to divert this discussion to the issue itself or to another PR.

[bubenheimer]

I have tested to save the entire MapClickListeners object to MapView.tag and it works well. I haven't managed to make this work properly with only storing OnIndoorStateChangeListener. You also added a comment to MapClickListenerUpdater that "The mapClickListeners container object is not allowed to ever change" which tells me that it makes sense to store this object in the tag.

We don't want the mapClickListeners to be singleton for a given GoogleMap() composition, because it leads to funny Compose data lifetime & change logic. It's hard to see without digging all the way down to the innocuous looking object : IndoorStateChangeListener and how it is used exactly.

Then if this node tree is reused by another composable then it just changes the state values of the MapClickListeners object.

Is there a particular reason why you didn't want to store the entire MapClickListeners singleton object?

You can store MapClickListeners, but it does not buy you anything, and you still have to null out all the callbacks to prevent leaks.

Bit surprised there is no issue with IndoorStateChangeListener upon reuse.

Edit: I see it now - with ReusableComposition it will hold on to the corresponding MapClickListenerNode, which holds on to the GoogleMap listener. It may look different without ReusableComposition.

Edit: it looks easiest to pass in that OnIndoorStateChangeListener object via an additional Applier parameter.

[philip-segerfast]

Hi @bubenheimer. Sorry for the delay. I've been busy with other things the last few weeks. I'll start focusing more on this now again.

I will respond to your comments.

[bubenheimer]

Thanks @philip-segerfast I hope to find some time to review over the coming week

[philip-segerfast]

You're welcome, I still have a bunch of work left though. I converted this PR to a draft.

Edit: I've addressed the issues

[philip-segerfast]

I don't find any issues with the OnIndoorStateChangeListener. It seems to work fine to register/unregister the listener multiple times.

[bubenheimer]

I don't find any issues with the OnIndoorStateChangeListener. It seems to work fine to register/unregister the listener multiple times.

Thanks for checking that. I've confirmed in my own testing now that multiple registration of different GoogleMap.OnIndoorStateChangeListeners works fine at this time, with both legacy and latest renderer.

Perhaps it's just a matter of strangely worded documentation.

[philip-segerfast]

Sorry for the delay, @bubenheimer. Now finally I'm back working on this. Please tell me if you have any other thoughts.

[philip-segerfast]

The latest commit uses a custom approach for moving between lifecycle states, inspired by LifecycleRegistry.sync(). 99% of the code is in factory.

[philip-segerfast]

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]

Looks great! LGT @kikoso

[el-qq]

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

[dkhawk]

@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.

[philip-segerfast]

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?

[el-qq]

@dkhawk friendly reminder about this PR (current 6.0.0 :) )

Also, we often use GoogleMap in LazyColumn and have encountered some performance issues (like oom). Looking forward to these edits in the hope that they will improve performance

[philip-segerfast]

Surely! I'll also make sure to fix the conflicts soon so that it's ready to be merged again.

[GuillemRoca]

Great job! We will also benefit greatly if this PR gets merged soon so looking forward to it. 👏🏼

[dkhawk]

@philip-segerfast this is a great change. Thanks so much for your contribution.

[philip-segerfast]

Thanks, you're all welcome!

[googlemaps-bot]

:tada: This issue has been resolved in version 6.1.0 :tada:

The release is available on:

• v6.1.0
• GitHub release

Your semantic-release bot :package::rocket:

[el-qq]

Local tests show great results!

Thank you very much, @philip-segerfast