Skip to content

feat(clustering,maps-compose): remove deprecated rememberSaveable key and optimize loops #983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dkhawk wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(clustering,maps-compose): remove deprecated rememberSaveable key and optimize loops #983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7311570
feat(clustering,maps-compose): remove deprecated rememberSaveable key…
dkhawk Aug 26, 2026
688b41f
Merge branch 'main' into chore/lint-fixes
dkhawk Sep 1, 2026
3eabff1
Merge branch 'main' into chore/lint-fixes
kikoso Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions ...compose-utils/src/main/java/com/google/maps/android/compose/clustering/ClusterRenderer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ internal class ComposeUiClusterRenderer<T : ClusterItem>(
clusterItemContentZIndexState.value,
)
}.collect {
keysToViews.forEach { (key, viewInfo) ->
for ((key, viewInfo) in keysToViews) {
when (key) {
is ViewKey.Cluster -> {
getMarker(key.cluster)?.apply {
Expand Down Expand Up @@ -158,15 +158,15 @@ internal class ComposeUiClusterRenderer<T : ClusterItem>(

val keys = clusters.flatMap { it.computeViewKeys() }

@kikoso kikoso Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the "optimize loops" in the title could actually land.

keys is a List, so key !in keys on line 164 is a linear scan run once per entry in keysToViews, making onClustersChanged O(n*m) on every cluster change. computeViewKeys() already returns a Set, so:

Suggested change
val keys = clusters.flatMap { it.computeViewKeys() }
val keys = clusters.flatMapTo(mutableSetOf()) { it.computeViewKeys() }

That makes both membership checks O(1) and dedupes safely (the for (key in keys) loop below adds into a map, so duplicates were never wanted). With a few hundred markers this is the difference that's actually measurable, unlike the forEach to for swaps.


with(keysToViews.iterator()) {
forEach { (key, viewInfo) ->
if (key !in keys) {
remove()
viewInfo.onRemove()
}
val iterator = keysToViews.iterator()

@kikoso kikoso Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hunk is a genuine improvement. The old with(keysToViews.iterator()) { forEach { ... remove() } } was fragile, relying on the interplay between Iterator.forEach and MutableIterator.remove. The explicit while (hasNext()) is clearer and obviously correct. 👍

I'd just reframe it in the PR description though: the forEach to for swaps elsewhere (lines 127 and 169) are readability only. Iterable.forEach and Map.forEach are inline, so the bytecode is effectively identical and there's no perf win to claim there.

while (iterator.hasNext()) {
val (key, viewInfo) = iterator.next()
if (key !in keys) {
iterator.remove()
viewInfo.onRemove()
}
}
keys.forEach { key ->
for (key in keys) {
if (key !in keysToViews.keys) {
createAndAddView(key)
}
Expand Down
2 changes: 0 additions & 2 deletions maps-compose/src/main/java/com/google/maps/android/compose/CameraMoveStartedReason.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
package com.google.maps.android.compose

import androidx.compose.runtime.Immutable
import com.google.maps.android.compose.CameraMoveStartedReason.Companion.fromInt
import com.google.maps.android.compose.CameraMoveStartedReason.NO_MOVEMENT_YET
import com.google.maps.android.compose.CameraMoveStartedReason.UNKNOWN

@kikoso kikoso Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anchoring here since the two removed lines only exist on the left side of the diff.

The removed fromInt and NO_MOVEMENT_YET imports aren't unused: both are referenced from the enum's KDoc just below, in "[NO_MOVEMENT_YET] is used as the initial state..." and "...an unsupported integer value is provided to [fromInt]". Compilation is fine either way, but could you check the Dokka output still resolves both links after this?

It's also inconsistent: this UNKNOWN import is kept, and it's equally KDoc-only. Its one code use, ?: return UNKNOWN inside the companion, resolves as an enum member without any import. So by the same criterion it would go too. I'd drop all three or keep all three.


/**
Expand Down
2 changes: 1 addition & 1 deletion maps-compose/src/main/java/com/google/maps/android/compose/CameraPositionState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public inline fun rememberCameraPositionState(
public inline fun rememberCameraPositionState(
key: String? = null,
crossinline init: CameraPositionState.() -> Unit = {}
): CameraPositionState = rememberSaveable(key = key, saver = CameraPositionState.Saver) {
): CameraPositionState = rememberSaveable(saver = CameraPositionState.Saver) {

@kikoso kikoso Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inside the @Deprecated overload, so after this change key stays in the public signature and does nothing. Callers still compile, but their saved-state key flips from their string to currentCompositeKeyHashCode. Three consequences, none of which the compiler surfaces:

  • State persisted under the old custom key isn't found after upgrade, so the camera resets once.
  • Anyone sharing one key across two call sites silently loses that sharing.
  • Anyone using key to distinguish iterations of the same call site (loop, pager) now collides, since positional scoping yields the same hash per iteration. The correct migration is wrapping in the key(...) composable, which nothing in our KDoc currently says.

I grepped every rememberSaveable call site in the repo: the non-deprecated path at line 54 is already keyless, and all three remaining keyed sites are inside @Deprecated declarations. So this change buys nothing on the supported surface, and only alters APIs already on their way out.

Suggestion: leave the body as it was and silence the warning instead, then delete the overload wholesale at the next major, where removing key is an honest feat!: with a migration note.

@Suppress("DEPRECATION")
@Composable
public inline fun rememberCameraPositionState(
    key: String? = null,
    crossinline init: CameraPositionState.() -> Unit = {}
): CameraPositionState = rememberSaveable(key = key, saver = CameraPositionState.Saver) {

If you'd rather keep it as written, that's defensible given the function is already deprecated, but it needs a release-note line pointing loop and pager users at key(...).

CameraPositionState().apply(init)
}

Expand Down
4 changes: 2 additions & 2 deletions maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public class MarkerState private constructor(position: LatLng) {
"so it will be changed or removed.",
replaceWith = ReplaceWith(
expression = """
val markerState = rememberSaveable(key = key, saver = MarkerState.Saver) {
val markerState = rememberSaveable(saver = MarkerState.Saver) {

@kikoso kikoso Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, but while we're in here: this ReplaceWith can't actually be applied by the IDE. It's a multi-line statement (val markerState = ...) rather than an expression, and it carries no imports for rememberSaveable / Saver. It also contradicts the message right above it, which says to use rememberUpdatedMarkerState.

Either ReplaceWith("rememberUpdatedMarkerState(position)") or dropping replaceWith entirely would be more honest than what's there today.

MarkerState(position)
}
"""
Expand All @@ -204,7 +204,7 @@ public class MarkerState private constructor(position: LatLng) {
public fun rememberMarkerState(
key: String? = null,
position: LatLng = LatLng(0.0, 0.0)
): MarkerState = rememberSaveable(key = key, saver = MarkerState.Saver) {
): MarkerState = rememberSaveable(saver = MarkerState.Saver) {

@kikoso kikoso Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as CameraPositionState.kt:75: key survives in the signature as a no-op.

One extra wrinkle here. The deprecation message on line 193 only talks about rememberUpdatedMarkerState and never mentions key, so a caller passing key gets no hint at all that its meaning changed. If we keep this change rather than suppressing, the message needs a sentence about it.

MarkerState(position)
}

Expand Down
Loading