Skip to content

feat(clustering): add incremental item diffing - #1037

Open
RotBolt wants to merge 2 commits into
googlemaps:mainfrom
RotBolt:feat/clustering-item-key
Open

feat(clustering): add incremental item diffing#1037
RotBolt wants to merge 2 commits into
googlemaps:mainfrom
RotBolt:feat/clustering-item-key

Conversation

@RotBolt

@RotBolt RotBolt commented Oct 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

  • Add an optional stable key to clustering; use item equals/hashCode when no key is supplied.
  • Diff additions, removals, and changed values, avoiding full refreshes when items are unchanged.
  • Add unit/instrumentation coverage and a 5,000-item frequently updated demo.

Fixes #504

Validation

  • :maps-compose-utils:testDebugUnitTest
  • :maps-app:connectedDebugAndroidTest for GoogleMapViewClusteringTests (6/6 passed on Motorola Edge 50 Neo)
  • :maps-compose-utils:lintDebug :maps-app:lintDebug
  • Manually tested the 5k keyed-diff demo on Motorola Edge 50 Neo with Maps tiles loaded; observed repeated 100-item updates and the algorithm retaining 5,000 items.

Manual verification

image

@RotBolt
RotBolt force-pushed the feat/clustering-item-key branch 2 times, most recently from 52a1912 to ba19eb9 Compare October 8, 2026 18:48
@RotBolt
RotBolt marked this pull request as ready for review October 8, 2026 18:54
@RotBolt

RotBolt commented Oct 8, 2026

Copy link
Copy Markdown
Author

@kikoso Please review this PR for adding clustering increamental item diff

@RotBolt
RotBolt force-pushed the feat/clustering-item-key branch from ba19eb9 to fc84cf6 Compare October 8, 2026 18:55
kikoso
kikoso reviewed Oct 12, 2026
View reviewed changes

@kikoso kikoso left a comment

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.

Thanks for the contribution, this is a nice improvement! Can you take a look at the comments, @RotBolt ? Thanks!

Comment thread maps-compose-utils/src/main/java/com/google/maps/android/compose/clustering/Clustering.kt
clusterContentRotation: Float = 0.0f,
clusterItemContentRotation: Float = 0.0f,
clusterRenderer: ClusterRenderer<T>? = null,
key: ((T) -> Any)? = null,

@kikoso kikoso Oct 12, 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.

Adding a parameter to an existing public function changes its JVM signature, so anything compiled against the previous version would break at runtime, even though source calls still compile thanks to the default value. Could we keep the old signatures as @Deprecated(level = DeprecationLevel.HIDDEN) overloads delegating to the new ones, or do I miss anything?

@RotBolt RotBolt Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Added hidden deprecated overloads for all four previous JVM signatures.

Comment thread maps-compose-utils/src/main/java/com/google/maps/android/compose/clustering/Clustering.kt
clusterContentRotation: Float = 0.0f,
clusterItemContentRotation: Float = 0.0f,
clusterItemDecoration: @Composable @GoogleMapComposable (T) -> Unit = {},
key: ((T) -> Any)? = null,

@kikoso kikoso Oct 12, 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.

nit: in the other overloads key goes before clusterItemDecoration, but here it comes after it. Should we keep the same order everywhere, for consistency?

@RotBolt RotBolt Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. key now comes before clusterItemDecoration in every overload.

Comment thread maps-compose-utils/src/main/java/com/google/maps/android/compose/clustering/ClusterItemsDiff.kt Outdated
items.forEach { item ->
val itemKey = key?.invoke(item) ?: item
if (key != null) {
require(itemKey !in keyedItems) {

@kikoso kikoso Oct 12, 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 will crash the app if the data contains a duplicated key, which can easily happen with backend data. Should we at least state in the KDoc of key that an IllegalArgumentException is thrown, or would it make sense to log and keep the last item instead? What do you think?

@RotBolt RotBolt Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed this to log a warning and keep the last item for duplicate keys. The behavior is documented in KDoc and covered by unit tests.

Comment thread maps-compose-utils/src/main/java/com/google/maps/android/compose/clustering/ClusterItemsDiff.kt
return keyedItems
}

internal fun <T, K> diffClusterItems(

@kikoso kikoso Oct 12, 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.

Since clusterItemsByKey and diffClusterItems are pure functions, could we add some plain JVM unit tests for them (empty lists, duplicate keys, unkeyed equality, add + remove + update in the same pass)? They would be much faster than the instrumented ones and cover more edge cases.

@RotBolt RotBolt Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Added JVM unit tests for empty collections, duplicate keys, unkeyed equality, and add/remove/update in one pass.

Comment thread ...app/src/main/java/com/google/maps/android/compose/markerexamples/MarkerClusteringActivity.kt Outdated
CustomUiClustering(
items = items,
)
LaunchedEffect(clusteringType) {

@kikoso kikoso Oct 12, 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.

Just a random idea: GoogleMapClustering is getting quite busy with the generator loop and the overlay. Would it make sense to move the keyed diff demo into its own composable (e.g. KeyedDiffClustering), like the other clustering types?

@RotBolt RotBolt Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Moved the keyed diff demo and its update loop and overlay into KeyedDiffClustering.

Comment thread ...app/src/main/java/com/google/maps/android/compose/markerexamples/MarkerClusteringActivity.kt Outdated
Comment on lines 27 to 30
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.horizontalScroll
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box

@kikoso kikoso Oct 12, 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.

nit: import ordering.

Suggested change
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.horizontalScroll
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.horizontalScroll
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column

@RotBolt RotBolt Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kikoso kikoso kikoso left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@dkhawk dkhawk

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clustering should support diff util

3 participants

@RotBolt @kikoso @dkhawk