Regression: recomposed map listeners ignored

[bubenheimer]

A regression was introduced in 1.4.21, specifically #320, where only the initial compositions of all the GoogleMap listener lambdas are used; recomposed listener lambdas are ignored. This can cause pervasive malfunctions of listener functionality and is a likely cause for some of the issues reported in the last months.

The diff screenshot shows the culprit - the actual Map listener used to call the latest version of the listener lambda, whereas now the actual Map listener is directly set to the initial version of the listener lambda.

I don't know how to trivially fix it without undoing this part of the PR, which would undo the fix for #38. @romainpiel, as the original author of the PR, are you able to devise a way to get recomposed listeners working again?

I am seeing this issue as of maps-compose version 4.3.0, but it has been around since 1.4.21.

[kikoso]

Hi @bubenheimer ,

I think a potential fix for this could be to switch from remember to rememberUpdatedState when adding the listeners. We currently have the following:

    val mapClickListeners = remember { MapClickListeners() }.also {
        it.indoorStateChangeListener = indoorStateChangeListener
        it.onMapClick = onMapClick
        it.onMapLongClick = onMapLongClick
        it.onMapLoaded = onMapLoaded
        it.onMyLocationButtonClick = onMyLocationButtonClick
        it.onMyLocationClick = onMyLocationClick
        it.onPOIClick = onPOIClick
    }

As per the documentation from remember, when using remember every consecutive calls to recomposition will only return same value that was computed initially during the first call made to remember.

On the other hand, with rememberUpdatedState and by contrast, the object is updated by every recomposition. This seems to be however something that has been happening previous to the introduction of #320.

[bubenheimer]

Thanks @kikoso, let me try to understand what you have in mind and update in a bit. I initially misunderstood what you meant.

I did verify that prior to #320, the behavior seen was the standard Compose callback behavior, GoogleMap() was always using the latest version of recomposed listener callback lambdas.

[bubenheimer]

@kikoso I've looked at it again and I don't think it's as easy as using rememberUpdatedState() somewhere. Prior to #320 the permanently set map listener had the benefit of an additional indirection that would use whatever the latest version was. However, #38 requires at least some of the listeners to not be set by default. On recomposition, callbacks may switch between null and non-null. This means that a map listener may need to be set again upon recomposition. The current code that sets the map listeners in MapPropertiesNode.onAttached() is only run once, when the GoogleMap ComposeNode is first added.

The solution is to set any changed map listeners upon recomposition. I don't know Compose and GoogleMap internals well enough to suggest a good way to do this.

[bubenheimer]

@kikoso @wangela do you have access to the Maps SDK team? #38 describes undocumented side effects of the Maps SDK. If it were possible for the Maps SDK team to augment the API contract/documentation with more detailed and complete effects of setting or not setting each of the listeners then the Maps Compose API could make more aggressive, performant implementation choices.

I think that right now, without this additional detailed information, the best course of action in terms of implementing a fix here is to check all Map listener callbacks for changes on recomposition (via ComposeNode update lambda), rather than permanently setting some listeners for better performance (as with the pre-#320 behavior).

The concern is that any change (intentional or unintentional) in Maps SDK behavior about side effects of setting or not setting a listener could be amplified if Maps Compose has that listener permanently set for performance reasons.

On the other hand, the performance impact of checking all listener callback lambdas for changes on recomposition may not be very significant, so perhaps it's a mute point.

[kikoso]

Hi @bubenheimer ,

We are checking this, as soon as we have an answer we will update this issue, and eventually propose a solution. Thanks for the patience.

[bubenheimer]

Thanks for the update, @kikoso.

I've been thinking about possible efficient approaches some more. I wonder if emitting additional ComposeNodes might be a good solution; one ComposeNode per GoogleMap listener. The rationale being that there should be a hard distinction between a map listener present (non-null) or not present, and that could perhaps be modeled well by a dedicated ComposeNode:

• When a GoogleMap() callback is null then the map listener needs to be null.

• When a callback is non-null, then the map listener needs to be present. The actual shape of the listener, when it is present, is secondary in a sense, and should likely be the original callback indirection lambda that Chris envisioned: setting or changing a map listener could be a slightly expensive operation (e.g. due to synchronization), and that would commonly be hidden as an implementation detail; in a scenario of rapidly recomposing callback lambdas it may not be a good idea to change the map listener on every recomposition.

I would postulate that a listener callback does not commonly recompose between null and non-null states in a rapid fashion, usually just different (non-null) lambdas, so we can optimize for that behavior.

We also want to minimize unnecessary recompositions of other GoogleMap() implementation parts when a listener callback recomposes, and preferably even minimize the number of equality checks and object creations + field reassignments on recomposition, as there are quite a lot of top-level listener callbacks to deal with.

This approach might eclipse the need for additional information from Maps SDK.

I've never worked at the ComposeNode level, though.

[bubenheimer]

I'm working on a PR

[googlemaps-bot]

🎉 This issue has been resolved in version 4.3.3 🎉

The release is available on:

• v4.3.3
• GitHub release

Your semantic-release bot 📦🚀