fix: resolve 10 defects uncovered by Android Maps Testing Toolkit - #1795
Conversation
Code Coverage
Files
|
Fixes 10 production issues across `:data`, `:heatmaps`, `:clustering`,
`:library`, and `:ui` uncovered by deterministic JVM and visual testing
with Android Maps Testing Toolkit v1.1.0-rc01:
1. `:data` (`KmlLayer`): Cache `KmlGroundOverlay -> ModelFeature` in
`mGroundOverlayMap` so `removeLayerFromMap()` removes ground overlays
from `MapViewRenderer`'s `IdentityHashMap` instead of leaking them.
2. `:data` (`Style`, `MapViewRenderer`, `GeoJsonLayer`, `KmlLayer`):
Propagate `clickable` (defaulting to `true`) and `visible` to
`PolylineOptions` and `PolygonOptions` so GeoJSON and KML polygons
and polylines dispatch `OnFeatureClickListener` callbacks.
3. `:data` (`GeoJsonLayer`, `KmlLayer`, `MapViewRenderer`): Route
markers, polygons, polylines, and ground overlays through passed-in
`MarkerManager`, `PolygonManager`, `PolylineManager`, and
`GroundOverlayManager` collections when provided, and register click
listeners on those collections instead of clobbering global
`GoogleMap` click listeners.
4. `:data` (`KmlLayer`, `GeoJsonLayer`, `Style`, `MapViewRenderer`):
Aggregate placemarks and ground overlays from `<Document>` containers
in `hasPlacemarks()`, `getPlacemarks()`, `features`, and
`getGroundOverlays()`, and preserve styles on `MultiGeometry`
features via `CompositeStyle`.
5. `:data` (`GeoJsonLayer`, `Style`, `MapViewRenderer`): Propagate
`GeoJsonPointStyle` `title`, `snippet`, `isDraggable`, `isFlat`,
`isVisible`, `infoWindowAnchorU/V`, and custom `icon` (`BitmapDescriptor`)
to `MarkerOptions` and `AdvancedMarkerOptions`.
6. `:heatmaps` (`HeatmapTileProvider`): Clamp `zoom` when indexing
`maxIntensity` (preventing `ArrayIndexOutOfBoundsException` at zoom
level 22) and clamp `bucketX`/`bucketY` to `0 until gridDim` so points
on the inclusive upper boundary `maxX`/`maxY` do not throw
`ArrayIndexOutOfBoundsException`.
7. `:heatmaps` (`HeatmapTileProvider`): Check `&& wrappedPoints.isEmpty()`
before returning `TileProvider.NO_TILE` so cross-antimeridian points
render across the International Date Line, and enforce `MIN_RADIUS..MAX_RADIUS`
and `0.0..1.0` bounds validation in `setRadius` and `setOpacity`.
8. `:clustering` (`DefaultClusterRenderer`): Clear stale `marker.title`
and `marker.snippet` when a `ClusterItem`'s title/snippet is updated
to `null`, and update `marker.zIndex` in `onClusterItemUpdated` even
when `position` is unchanged.
9. `:library` (`MapObjectManager`): Invoke `setListenersOnUiThread()`
synchronously when constructed on the main thread instead of
unconditionally posting to the back of the main looper queue.
10. `:ui` (`AnimationUtil`, `IconGenerator`, `RotationLayout`): Snap
directly to `finalPosition` when `durationInMs <= 0L` (avoiding
`0 / 0.0f = NaN`), clamp `t` to `[0f, 1f]` so
`AccelerateDecelerateInterpolator` never rebounds on frame overshoot,
and use `degrees.mod(360)` so negative multi-turn rotations do not
throw `IllegalStateException`.
e7d9d13 to
7dbc806
Compare
7209983 to
d1cccca
Compare
| mRenderer = null | ||
| } else { | ||
| mRenderer = MapViewRenderer(map, UrlIconProvider()) | ||
| initializeRenderer(map) |
There was a problem hiding this comment.
What happens here if someone calls setMap() with a different map? The managers we stored in the constructor are still bound to the old GoogleMap, so it looks like the features (and the click listeners) would ended up in the old map. Before this PR setMap was ignoring the managers, so this case used to work. What if we drop the managers (or throw) when the map is not the same one?
There was a problem hiding this comment.
Good catch! Updated setMap() to remove existing features from the previous renderer first and drop the constructor-supplied managers/collections when map changes to a different GoogleMap instance.
| mPolylineManager, | ||
| mGroundOverlayManager, | ||
| ) | ||
| mFeatureClickListener?.let { setOnFeatureClickListener(it) } |
There was a problem hiding this comment.
Same question than in GeoJsonLayer.setMap(), what about the managers here when the map changes?
There was a problem hiding this comment.
Updated KmlLayer.setMap() with the same behavior to clean up the previous renderer and drop the managers/collections when switching to a different GoogleMap.
| polylineManager: PolylineManager? = null, | ||
| groundOverlayManager: GroundOverlayManager? = null, | ||
| ) : DataRenderer { | ||
| internal val markerCollection: MarkerManager.Collection? = markerManager?.newCollection() |
There was a problem hiding this comment.
Every time the renderer is recreated (e.g. setMap(null) + setMap(map)) we call newCollection() again, and there is no way to remove a collection from the manager. Could we reuse the collection instead of creating a new one each time?
There was a problem hiding this comment.
Good point! Lazily created the manager Collection instances once per layer and passed them into MapViewRenderer so setMap(null) + setMap(map) reuses the existing collections instead of calling newCollection() again.
| public fun hasPlacemarks(): Boolean = getAllPlacemarks().isNotEmpty() | ||
|
|
||
| public fun getPlacemarks(): Iterable<KmlPlacemark> = mPlacemarks | ||
| public fun getPlacemarks(): Iterable<KmlPlacemark> = getAllPlacemarks() |
There was a problem hiding this comment.
This changes what getPlacemarks() returns: before it was only the top-level placemarks, now it recurses into all the containers. What about apps that already walk getContainers() themselves (like KmlDemoActivity)? They would get the nested placemarks twice. What if we keep the old behavior here and add a new getAllPlacemarks() with KDoc? Same would apply for hasPlacemarks(), features and getGroundOverlays().
There was a problem hiding this comment.
Agreed! Restored hasPlacemarks(), getPlacemarks(), features, and getGroundOverlays() to return only top-level items, and exposed getAllPlacemarks() and getAllGroundOverlays() with KDoc for recursive traversal across nested <Document> and <Folder> containers.
| val anchorV: Float = 1.0f, | ||
| val scale: Float = 1.0f, | ||
| val zIndex: Float = 0.0f, | ||
| val title: String? = null, |
There was a problem hiding this comment.
PointStyle, LineStyle and PolygonStyle are public since v5.0.0, so adding constructor params is a binary break for the constructors and copy(). Fine for 6.0 I think, but should we add it to MIGRATION.md? The fix: title won't flag it otherwise.
There was a problem hiding this comment.
Good call — documented the expanded PointStyle, LineStyle, and PolygonStyle constructor signatures in MIGRATION.md under 6.0.0 changes.
| val draggable: Boolean = false, | ||
| val flat: Boolean = false, | ||
| val visible: Boolean = true, | ||
| val iconDescriptor: BitmapDescriptor? = null, |
There was a problem hiding this comment.
Is it intended to have a BitmapDescriptor here? GeoJsonMapper describes this model as platform-agnostic, and this couples it to the Maps SDK.
There was a problem hiding this comment.
Good catch! Removed BitmapDescriptor from PointStyle so com.google.maps.android.data.renderer.model stays completely platform-agnostic, and handled custom GeoJsonPointStyle BitmapDescriptors via an internal descriptor cache in MapViewRenderer.
| /** | ||
| * A composite style carrying point, line, and polygon styles for heterogeneous [MultiGeometry] features. | ||
| */ | ||
| data class CompositeStyle( |
There was a problem hiding this comment.
Adding a new subtype to the sealed Style will break any exhaustive when over Style in consumer code, could we mention it in MIGRATION.md too? Also, what about #1800 (explicitApi)? CompositeStyle, the new constructor properties and MapViewRenderer don't have public, so whichever lands second will need to fix it and regenerate data.api.
There was a problem hiding this comment.
Added CompositeStyle (and its impact on exhaustive when expressions over Style) to MIGRATION.md, added explicit public modifiers here, and will rebase #1800 and regenerate data.api right after.
| } else if (item.snippet != null && item.snippet != marker.title) { | ||
| marker.title = item.snippet | ||
| // Update marker text if the item text changed - same logic as adding marker in onBeforeClusterItemRendered() | ||
| val expectedTitle = item.title ?: item.snippet |
There was a problem hiding this comment.
What about subclasses that set their own title in onBeforeClusterItemRendered? When the item has no title and no snippet, this now sets marker.title = null and wipes it on the first update, before it was left untouched. Also, ClusterRendererMultipleItems and DefaultAdvancedMarkersClusterRenderer still have the old logic, should we update them too so the three renderers behaves the same?
There was a problem hiding this comment.
Great catch! Updated onClusterItemUpdated so that when both item.title and item.snippet are null, marker.title and marker.snippet are left untouched (preserving custom titles set by subclasses in onBeforeClusterItemRendered), while clearing marker.snippet when only title or snippet is non-null. Also applied the same title/snippet and zIndex update logic to ClusterRendererMultipleItems and DefaultAdvancedMarkersClusterRenderer.
…le, and cluster renderers
* feat: migrate android-maps-ktx into android-maps-utils (v6.0.0-rc01) (#1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage. * feat: port Maps SDK coroutine initialization extension from android-maps-ktx - Add canonical Context.awaitMapsSdkInitialized(preferredRenderer) suspending extension in com.google.maps.android. - Add deprecated backward-compatibility shim in com.google.maps.android.ktx. - Add canonical and shim unit test suites for MapsInitializer coroutine extensions. - Showcase awaitMapsSdkInitialized in KtxExtensionsDemoActivity and register demo in MainActivity. - Update README.md documentation with awaitMapsSdkInitialized usage example. * chore: release v6.0.0-rc01 * chore: release v6.0.0-rc02 * fix: resolve lint test SDK requirement and configure isolated test home for unit tests * chore: release v6.0.0-rc03 * chore: prepare v6.0.0 final release * build(deps): update dependencies for v6.0.0 release * refactor: address review comments on coroutines, inlining, and file structure * chore: release v6.0.0-rc04 * chore: align release-please version baseline with main for v6.0.0 release * fix: harden coroutine continuations, remove FQCNs/wildcard imports, and add adversarial tests * fix: address all remaining PR review comments from LoyalAbbas and kikoso * fix: add R8 consumer rules for optional location and Navigation SDK dependencies * fix(location): remove PASSIVE_PROVIDER fallback and guard missing providers * fix: resolve 10 defects uncovered by Android Maps Testing Toolkit (#1795) * fix: resolve 10 defects uncovered by Android Maps Testing Toolkit Fixes 10 production issues across `:data`, `:heatmaps`, `:clustering`, `:library`, and `:ui` uncovered by deterministic JVM and visual testing with Android Maps Testing Toolkit v1.1.0-rc01: 1. `:data` (`KmlLayer`): Cache `KmlGroundOverlay -> ModelFeature` in `mGroundOverlayMap` so `removeLayerFromMap()` removes ground overlays from `MapViewRenderer`'s `IdentityHashMap` instead of leaking them. 2. `:data` (`Style`, `MapViewRenderer`, `GeoJsonLayer`, `KmlLayer`): Propagate `clickable` (defaulting to `true`) and `visible` to `PolylineOptions` and `PolygonOptions` so GeoJSON and KML polygons and polylines dispatch `OnFeatureClickListener` callbacks. 3. `:data` (`GeoJsonLayer`, `KmlLayer`, `MapViewRenderer`): Route markers, polygons, polylines, and ground overlays through passed-in `MarkerManager`, `PolygonManager`, `PolylineManager`, and `GroundOverlayManager` collections when provided, and register click listeners on those collections instead of clobbering global `GoogleMap` click listeners. 4. `:data` (`KmlLayer`, `GeoJsonLayer`, `Style`, `MapViewRenderer`): Aggregate placemarks and ground overlays from `<Document>` containers in `hasPlacemarks()`, `getPlacemarks()`, `features`, and `getGroundOverlays()`, and preserve styles on `MultiGeometry` features via `CompositeStyle`. 5. `:data` (`GeoJsonLayer`, `Style`, `MapViewRenderer`): Propagate `GeoJsonPointStyle` `title`, `snippet`, `isDraggable`, `isFlat`, `isVisible`, `infoWindowAnchorU/V`, and custom `icon` (`BitmapDescriptor`) to `MarkerOptions` and `AdvancedMarkerOptions`. 6. `:heatmaps` (`HeatmapTileProvider`): Clamp `zoom` when indexing `maxIntensity` (preventing `ArrayIndexOutOfBoundsException` at zoom level 22) and clamp `bucketX`/`bucketY` to `0 until gridDim` so points on the inclusive upper boundary `maxX`/`maxY` do not throw `ArrayIndexOutOfBoundsException`. 7. `:heatmaps` (`HeatmapTileProvider`): Check `&& wrappedPoints.isEmpty()` before returning `TileProvider.NO_TILE` so cross-antimeridian points render across the International Date Line, and enforce `MIN_RADIUS..MAX_RADIUS` and `0.0..1.0` bounds validation in `setRadius` and `setOpacity`. 8. `:clustering` (`DefaultClusterRenderer`): Clear stale `marker.title` and `marker.snippet` when a `ClusterItem`'s title/snippet is updated to `null`, and update `marker.zIndex` in `onClusterItemUpdated` even when `position` is unchanged. 9. `:library` (`MapObjectManager`): Invoke `setListenersOnUiThread()` synchronously when constructed on the main thread instead of unconditionally posting to the back of the main looper queue. 10. `:ui` (`AnimationUtil`, `IconGenerator`, `RotationLayout`): Snap directly to `finalPosition` when `durationInMs <= 0L` (avoiding `0 / 0.0f = NaN`), clamp `t` to `[0f, 1f]` so `AccelerateDecelerateInterpolator` never rebounds on frame overshoot, and use `degrees.mod(360)` so negative multi-turn rotations do not throw `IllegalStateException`. * fix: address PR review comments on layer managers, KML accessors, Style, and cluster renderers * chore: enable Kotlin explicitApi() and Binary Compatibility Validator (.api dumps) (#1800) * chore: enable Kotlin explicitApi() and Binary Compatibility Validator (.api dumps) - Enable kotlin explicitApi() across published modules (:library, :clustering, :data, :heatmaps, :ui) - Add explicit public visibility and return types to all public Kotlin symbols across modules - Configure Binary Compatibility Validator (BCV) for AGP 9.3 via build-logic convention plugin and root aggregate tasks - Generate and commit baseline .api dumps for :library, :clustering, :data, :heatmaps, and :ui - Add apiCheck to the CI Pull Request test workflow Fixes #1794 * fix: restore public visibility on ResponseStreetView and update library.api
Summary
Fixes 10 production bugs across
:data,:heatmaps,:clustering,:library, and:uiuncovered during integration and adversarial verification with the Android Maps Testing Toolkit (v1.1.0-rc01), forked fromfeat/migrate-ktx-to-utils(#1716)::data(KmlLayer)KmlGroundOverlay -> ModelFeatureinmGroundOverlayMapsoremoveLayerFromMap()removes ground overlays fromMapViewRenderer'sIdentityHashMapinstead of leaking them onGoogleMap.:data(GeoJsonLayer,KmlLayer,MapViewRenderer,Style)clickable(defaulttrue) andvisibletoPolylineOptionsandPolygonOptionsso GeoJSON and KML polygons and polylines are clickable and dispatchOnFeatureClickListenercallbacks.:data(GeoJsonLayer,KmlLayer,MapViewRenderer)markerManager,polygonManager,polylineManager, andgroundOverlayManagertoMapViewRenderercollections and attach click listeners to those collections so layers coexist withClusterManager/MarkerManagerwithout clobbering globalGoogleMaplisteners.:data(KmlLayer,GeoJsonLayer,MapViewRenderer,Style)<Document>containers inhasPlacemarks(),getPlacemarks(),features, andgetGroundOverlays(), and preserve styles on<MultiGeometry>/GeoJsonMultiLineString/GeoJsonMultiPoint/GeoJsonGeometryCollectionviaCompositeStyle.:data(GeoJsonLayer,MapViewRenderer,Style)GeoJsonPointStyletitle,snippet,isDraggable,isFlat,isVisible,infoWindowAnchorU/V, and customicon(BitmapDescriptor) toMarkerOptionsandAdvancedMarkerOptions.:heatmaps(HeatmapTileProvider)zoomwhen indexingmaxIntensity(preventingArrayIndexOutOfBoundsExceptionat zoom 22) and clampbucketX/bucketYto0 until gridDimso points on the upper inclusive boundarymaxX/maxYdo not throwArrayIndexOutOfBoundsException.:heatmaps(HeatmapTileProvider)&& wrappedPoints.isEmpty()before returningTileProvider.NO_TILEso cross-antimeridian points render across the International Date Line, and enforceMIN_RADIUS..MAX_RADIUS(10..50) and0.0..1.0validation insetRadiusandsetOpacity.:clustering(DefaultClusterRenderer)marker.titleandmarker.snippetwhen aClusterItem's title/snippet is updated tonull, and updatemarker.zIndexinonClusterItemUpdatedeven whenpositionis unchanged.:library(MapObjectManager)setListenersOnUiThread()synchronously when constructed on the main thread instead of unconditionally posting to the back of the main looper queue.:ui(AnimationUtil,IconGenerator,RotationLayout)finalPositionwhendurationInMs <= 0L(preventing0 / 0.0f = NaN), clamptto[0f, 1f]soAccelerateDecelerateInterpolatornever rebounds backward on frame overshoot, and usedegrees.mod(360)so negative multi-turn rotations (<= -450°) do not throwIllegalStateException.Testing
MapObjectManagerTest,ClusterManagerTest,KmlLayerOnMapTest,GeoJsonLayerOnMapTest,HeatmapTileProviderTest,AnimationUtilTest, andIconGeneratorTest.:library,:clustering,:data,:heatmaps,:ui, and:lint-checks, as well as the full 5-module Android Maps Testing Toolkit (v1.1.0-rc01) test suite.