refactor(map): adopt maps-compose 8.4.0 stock clustering, drop custom renderer workaround - #6301
Merged
Merged
Conversation
… renderer workaround maps-compose 8.4.0 fixes the two issues that forced NodeClusterMarkers onto a custom DefaultClusterRenderer with pre-baked bitmaps: ComposeUiClusterRenderer now sets ViewTreeLifecycleOwner/ViewTreeSavedStateRegistryOwner on its off-screen render views and guards callbacks with scope.isActive (upstream googlemaps/android-maps-compose#930 — the root cause of our former top Crashlytics FATAL), and custom Compose info windows no longer render blank on compose-ui 1.10+ (#931). - NodeClusterMarkers: rewritten to the stock Clustering() API (167 -> 88 lines). Chips compose via clusterItemContent, per-item z-order via ClusteringMarkerProperties, precision circles via clusterItemDecoration, minClusterSize via the onClusterManager hook. Native info windows and click navigation unchanged. - The bump pulls android-maps-utils 5.0.0 (source-breaking Kotlin conversion): NodeClusterItem moves to property overrides; MapView adapts isLayerOnMap dispatch, Feature.getGeometry()/getGeometryType(), and GeoJson style setter functions. Verified on emulator (google debug, 200-node replay): chips, 10+ cluster bubble, info windows, info-window navigation, and 0 crashes under repeated fast back-navigation during clustering. Supersedes #6298. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 task
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMap layer registration and GeoJSON styling now use explicit layer checks and setter APIs. Cluster markers use composable content and decorations, with cluster sizing configured through the cluster manager. The maps-compose version was upgraded to 8.4.0. ChangesMap rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Clustering
participant NodeClusterItem
participant PulsingNodeChip
participant DefaultClusterRenderer
participant Circle
Clustering->>NodeClusterItem: Read position and zIndex properties
Clustering->>PulsingNodeChip: Render unclustered node marker
Clustering->>Circle: Draw precision circle when enabled
Clustering->>DefaultClusterRenderer: Apply minimum cluster size
DefaultClusterRenderer->>DefaultClusterRenderer: Re-cluster items
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
androidApp/src/google/kotlin/org/meshtastic/app/map/model/NodeClusterItem.kt (1)
51-66: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winExtract
precisionMapto avoid repeated allocations.
getPrecisionMeters()is called for every node during map clustering, which happens frequently on the UI thread. Re-instantiating a map of 10 items on every invocation causes unnecessary memory allocation and garbage collection pressure. MoveprecisionMapto acompanion objectso it is initialized only once.⚡ Proposed fix
- fun getPrecisionMeters(): Double? { - val precisionMap = - mapOf( - 10 to 23345.484932, - 11 to 11672.7369, - 12 to 5836.36288, - 13 to 2918.175876, - 14 to 1459.0823719999053, - 15 to 729.53562, - 16 to 364.7622, - 17 to 182.375556, - 18 to 91.182212, - 19 to 45.58554, - ) - return precisionMap[this.node.position.precision_bits] - } + companion object { + private val PRECISION_MAP = mapOf( + 10 to 23345.484932, + 11 to 11672.7369, + 12 to 5836.36288, + 13 to 2918.175876, + 14 to 1459.0823719999053, + 15 to 729.53562, + 16 to 364.7622, + 17 to 182.375556, + 18 to 91.182212, + 19 to 45.58554, + ) + } + + fun getPrecisionMeters(): Double? = PRECISION_MAP[this.node.position.precision_bits]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/google/kotlin/org/meshtastic/app/map/model/NodeClusterItem.kt` around lines 51 - 66, Move the precisionMap definition out of getPrecisionMeters() and into the NodeClusterItem companion object so the map is initialized once and reused across calls. Update getPrecisionMeters() to look up node.position.precision_bits from that shared map, preserving the existing nullable result and precision values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.kt`:
- Around line 1378-1386: Update MapView.kt sites 1351-1358, 1378-1386, and 1395
to use property access for the maps-utils APIs: replace isLayerOnMap(),
feature.getGeometry()?.getGeometryType(), setStrokeColor(...),
setStrokeWidth(...), and setWidth(...) with their corresponding properties,
preserving the existing values and behavior.
In `@gradle/libs.versions.toml`:
- Line 65: Update the maps-compose version declaration to a published,
resolvable release, using 8.3.0 or another confirmed published version instead
of 8.4.0.
---
Outside diff comments:
In
`@androidApp/src/google/kotlin/org/meshtastic/app/map/model/NodeClusterItem.kt`:
- Around line 51-66: Move the precisionMap definition out of
getPrecisionMeters() and into the NodeClusterItem companion object so the map is
initialized once and reused across calls. Update getPrecisionMeters() to look up
node.position.precision_bits from that shared map, preserving the existing
nullable result and precision values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72033d8f-c3b4-4271-bf79-13b147cb4302
📒 Files selected for processing (4)
androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/component/NodeClusterMarkers.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/model/NodeClusterItem.ktgradle/libs.versions.toml
Review follow-up: getPrecisionMeters() runs per unclustered item on every cluster pass; allocate the bits->meters map once instead of per call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bumps maps-compose 8.3.1 → 8.4.0 and retires the custom cluster-renderer workaround that upstream has now fixed. Supersedes #6298, which cannot land on its own: 8.4.0 transitively bumps
android-maps-utilsto 5.0.0, a source-breaking Kotlin-conversion major, so the version bump must ship together with the call-site adaptations in this PR.Why now: 8.4.0 carries two fixes we want on the google flavor:
MarkerInfoWindowComposablefor track points), because compose-ui 1.10 added anisShownguard that makes detached views draw nothing.ComposeUiClusterRenderernow setsViewTreeSavedStateRegistryOwner/ViewTreeLifecycleOwneron its off-screen render views and guards render callbacks withscope.isActive— the root cause of our former top Crashlytics FATAL (Composed into the View which doesn't propagate ViewTreeLifecycleOwner!), whichNodeClusterMarkersworked around with a custom renderer and pre-baked bitmaps.Changes
🛠️ Refactoring & Architecture
NodeClusterMarkers.ktrewritten to the stockClustering(...)API (167 → 88 lines): the customDefaultClusterRenderersubclass, per-nodeBitmapDescriptorbaking, and theMapEffect/SideEffect/re-cluster wiring are all deleted. Node chips compose viaclusterItemContent(safe as of upstream [Bug] Trying to change fixed position coords fails #930), per-item z-order flows through the newClusteringMarkerPropertieshelper, precision circles draw via the newclusterItemDecorationslot, andminClusterSize = 10is applied through the newonClusterManagerhook. Native info windows (title/snippet) and click navigation are unchanged. Side benefit: the 1-second "just heard" pulse on node chips now actually animates — the bake path froze it at frame 0.🧹 Chores (android-maps-utils 5.0.0 source-compat)
gradle/libs.versions.toml: maps-compose 8.3.1 → 8.4.0 (pulls maps-ktx 6.2.0 / android-maps-utils 5.0.0).NodeClusterItem:ClusterItemis a Kotlin interface with properties in utils 5.0.0 — getter overrides converted tooverride val position/title/snippet/zIndex.MapView.kt:isLayerOnMapmoved off theLayerbase class (now dispatched per concreteGeoJsonLayer/KmlLayer);Feature.geometrybecame private (getGeometry()/getGeometryType()); GeoJson style setters became explicit functions (setStrokeColor/setStrokeWidth/setWidth).com.google.maps.android.dataKML/GeoJSON layer stack in favor of a new platform-agnostic data layer, and the deprecated bridge classes have known rendering quirks in 5.0.0 (e.g. invisible point markers, MultiPolygon style drops). This PR keeps the minimal source-compat fixes so the bump can land; a full migration of the custom map-layer overlay onto the new data-layer pipeline is already in progress on a separate branch and will supersede thesafeAddLayerToMap/applySimpleStyleSpeccode touched here.Testing Performed
spotlessCheck,detekt,assembleDebug,test,allTestsall green locally.precision_bits); the gating logic is condition-identical to the previous implementation.Summary by CodeRabbit