From 62395352b894f1b29bf376672002218800661aa3 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Thu, 16 Jul 2026 13:27:38 -0500 Subject: [PATCH] fix: remove MultiGeometry features from the map in removeFeature addFeature rendered MultiGeometry members by recursing with feature.copy(geometry = member), so the created map objects were keyed in the IdentityHashMap by those intermediate copies rather than by the caller's feature. removeFeature(feature) then found no entry and silently removed nothing: toggling a layer containing MultiPolygon/MultiLineString/ GeometryCollection features left their polygons, polylines, and markers stranded on the map, and re-adding the layer duplicated them. Render nested geometries through a private addGeometry(geometry, feature, mapObjects) recursion instead, accumulating every created map object into the original feature's entry so removeFeature works for any geometry. --- .../data/renderer/mapview/MapViewRenderer.kt | 39 +++++++---- .../data/renderer/MapViewRendererTest.kt | 68 +++++++++++++++++++ 2 files changed, 93 insertions(+), 14 deletions(-) diff --git a/data/src/main/java/com/google/maps/android/data/renderer/mapview/MapViewRenderer.kt b/data/src/main/java/com/google/maps/android/data/renderer/mapview/MapViewRenderer.kt index f599e7735..af8e91edd 100644 --- a/data/src/main/java/com/google/maps/android/data/renderer/mapview/MapViewRenderer.kt +++ b/data/src/main/java/com/google/maps/android/data/renderer/mapview/MapViewRenderer.kt @@ -29,6 +29,7 @@ import com.google.maps.android.data.renderer.IconProvider import com.google.maps.android.data.renderer.model.DataLayer import com.google.maps.android.data.renderer.model.DataScene import com.google.maps.android.data.renderer.model.Feature +import com.google.maps.android.data.renderer.model.Geometry import com.google.maps.android.data.renderer.model.GroundOverlay import com.google.maps.android.data.renderer.model.GroundOverlayStyle import com.google.maps.android.data.renderer.model.LineString @@ -109,9 +110,25 @@ class MapViewRenderer( override fun addFeature(feature: Feature) { val mapObjects = mutableListOf() - when (feature.geometry) { + addGeometry(feature.geometry, feature, mapObjects) + if (mapObjects.isNotEmpty()) { + renderedFeatures[feature] = mapObjects + } + } + + /** + * Renders a single [geometry] (recursing into [MultiGeometry] members) with [feature]'s style and + * properties, accumulating every created map object into [mapObjects] so nested geometries stay + * associated with the original [feature] and can be removed later via [removeFeature]. + */ + private fun addGeometry( + geometry: Geometry, + feature: Feature, + mapObjects: MutableList, + ) { + when (geometry) { is PointGeometry -> { - val point = feature.geometry.point + val point = geometry.point val style = feature.style as? PointStyle if (useAdvancedMarkers) { val markerOptions = createAdvancedMarkerOptions(point, style, feature.properties) @@ -159,28 +176,25 @@ class MapViewRenderer( } is LineString -> { - val lineString = feature.geometry val style = feature.style as? LineStyle - val polylineOptions = createPolylineOptions(lineString, style) + val polylineOptions = createPolylineOptions(geometry, style) mapObjects.add(map.addPolyline(polylineOptions)) } is Polygon -> { - val polygon = feature.geometry val style = feature.style as? PolygonStyle - val polygonOptions = createPolygonOptions(polygon, style) + val polygonOptions = createPolygonOptions(geometry, style) mapObjects.add(map.addPolygon(polygonOptions)) } is MultiGeometry -> { - feature.geometry.geometries.forEach { geometry -> - // Recursively add each geometry in the MultiGeometry - addFeature(feature.copy(geometry = geometry)) - } + // Recursively render each member into the same list so the rendered objects stay + // keyed by the original feature rather than by intermediate copies. + geometry.geometries.forEach { addGeometry(it, feature, mapObjects) } } is GroundOverlay -> { - val groundOverlay = feature.geometry + val groundOverlay = geometry val style = feature.style as? GroundOverlayStyle val options = createGroundOverlayOptions(groundOverlay, style) @@ -223,9 +237,6 @@ class MapViewRenderer( } } } - if (mapObjects.isNotEmpty()) { - renderedFeatures[feature] = mapObjects - } } override fun removeFeature(feature: Feature) { diff --git a/data/src/test/java/com/google/maps/android/data/renderer/MapViewRendererTest.kt b/data/src/test/java/com/google/maps/android/data/renderer/MapViewRendererTest.kt index d1ba1c6b5..3e9696019 100644 --- a/data/src/test/java/com/google/maps/android/data/renderer/MapViewRendererTest.kt +++ b/data/src/test/java/com/google/maps/android/data/renderer/MapViewRendererTest.kt @@ -22,8 +22,11 @@ import com.google.android.gms.maps.model.Marker import com.google.android.gms.maps.model.MarkerOptions import com.google.maps.android.data.renderer.mapview.MapViewRenderer import com.google.maps.android.data.renderer.model.Feature +import com.google.maps.android.data.renderer.model.LineString +import com.google.maps.android.data.renderer.model.MultiGeometry import com.google.maps.android.data.renderer.model.Point import com.google.maps.android.data.renderer.model.PointGeometry +import com.google.maps.android.data.renderer.model.Polygon import io.mockk.every import io.mockk.mockk import io.mockk.slot @@ -143,4 +146,69 @@ class MapViewRendererTest { assertEquals("Critical Right Turn", capturedOptions.title) assertEquals("Be careful here!", capturedOptions.snippet) } + + @Test + fun testRemoveFeature_multiGeometry_removesAllRenderedObjects() { + // Given + val mockMap = mockk(relaxed = true) + val mockIconProvider = mockk(relaxed = true) + + val addedPolygons = mutableListOf() + every { mockMap.addPolygon(any()) } answers { + mockk(relaxed = true).also { addedPolygons.add(it) } + } + + val renderer = MapViewRenderer(mockMap, mockIconProvider) + val feature = + Feature( + geometry = + MultiGeometry( + listOf( + Polygon(listOf(Point(0.0, 0.0), Point(0.0, 1.0), Point(1.0, 1.0))), + Polygon(listOf(Point(2.0, 2.0), Point(2.0, 3.0), Point(3.0, 3.0))), + ), + ), + ) + + // When + renderer.addFeature(feature) + renderer.removeFeature(feature) + + // Then: both rendered polygons are removed via the original feature reference. + assertEquals(2, addedPolygons.size) + addedPolygons.forEach { polygon -> verify(exactly = 1) { polygon.remove() } } + } + + @Test + fun testAddFeature_multiGeometry_addedTwice_doesNotLeakOnRemove() { + // Given + val mockMap = mockk(relaxed = true) + val mockIconProvider = mockk(relaxed = true) + + val addedPolylines = mutableListOf() + every { mockMap.addPolyline(any()) } answers { + mockk(relaxed = true).also { addedPolylines.add(it) } + } + + val renderer = MapViewRenderer(mockMap, mockIconProvider) + val feature = + Feature( + geometry = + MultiGeometry( + listOf( + LineString(listOf(Point(0.0, 0.0), Point(1.0, 1.0))), + ), + ), + ) + + // When: a hide/show cycle (remove + re-add) followed by a final remove. + renderer.addFeature(feature) + renderer.removeFeature(feature) + renderer.addFeature(feature) + renderer.removeFeature(feature) + + // Then: every polyline ever added has been removed — nothing is left on the map. + assertEquals(2, addedPolylines.size) + addedPolylines.forEach { polyline -> verify(exactly = 1) { polyline.remove() } } + } }