Skip to content

Commit 6239535

Browse files
committed
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.
1 parent 0147954 commit 6239535

2 files changed

Lines changed: 93 additions & 14 deletions

File tree

data/src/main/java/com/google/maps/android/data/renderer/mapview/MapViewRenderer.kt

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import com.google.maps.android.data.renderer.IconProvider
2929
import com.google.maps.android.data.renderer.model.DataLayer
3030
import com.google.maps.android.data.renderer.model.DataScene
3131
import com.google.maps.android.data.renderer.model.Feature
32+
import com.google.maps.android.data.renderer.model.Geometry
3233
import com.google.maps.android.data.renderer.model.GroundOverlay
3334
import com.google.maps.android.data.renderer.model.GroundOverlayStyle
3435
import com.google.maps.android.data.renderer.model.LineString
@@ -109,9 +110,25 @@ class MapViewRenderer(
109110

110111
override fun addFeature(feature: Feature) {
111112
val mapObjects = mutableListOf<Any>()
112-
when (feature.geometry) {
113+
addGeometry(feature.geometry, feature, mapObjects)
114+
if (mapObjects.isNotEmpty()) {
115+
renderedFeatures[feature] = mapObjects
116+
}
117+
}
118+
119+
/**
120+
* Renders a single [geometry] (recursing into [MultiGeometry] members) with [feature]'s style and
121+
* properties, accumulating every created map object into [mapObjects] so nested geometries stay
122+
* associated with the original [feature] and can be removed later via [removeFeature].
123+
*/
124+
private fun addGeometry(
125+
geometry: Geometry,
126+
feature: Feature,
127+
mapObjects: MutableList<Any>,
128+
) {
129+
when (geometry) {
113130
is PointGeometry -> {
114-
val point = feature.geometry.point
131+
val point = geometry.point
115132
val style = feature.style as? PointStyle
116133
if (useAdvancedMarkers) {
117134
val markerOptions = createAdvancedMarkerOptions(point, style, feature.properties)
@@ -159,28 +176,25 @@ class MapViewRenderer(
159176
}
160177

161178
is LineString -> {
162-
val lineString = feature.geometry
163179
val style = feature.style as? LineStyle
164-
val polylineOptions = createPolylineOptions(lineString, style)
180+
val polylineOptions = createPolylineOptions(geometry, style)
165181
mapObjects.add(map.addPolyline(polylineOptions))
166182
}
167183

168184
is Polygon -> {
169-
val polygon = feature.geometry
170185
val style = feature.style as? PolygonStyle
171-
val polygonOptions = createPolygonOptions(polygon, style)
186+
val polygonOptions = createPolygonOptions(geometry, style)
172187
mapObjects.add(map.addPolygon(polygonOptions))
173188
}
174189

175190
is MultiGeometry -> {
176-
feature.geometry.geometries.forEach { geometry ->
177-
// Recursively add each geometry in the MultiGeometry
178-
addFeature(feature.copy(geometry = geometry))
179-
}
191+
// Recursively render each member into the same list so the rendered objects stay
192+
// keyed by the original feature rather than by intermediate copies.
193+
geometry.geometries.forEach { addGeometry(it, feature, mapObjects) }
180194
}
181195

182196
is GroundOverlay -> {
183-
val groundOverlay = feature.geometry
197+
val groundOverlay = geometry
184198
val style = feature.style as? GroundOverlayStyle
185199
val options = createGroundOverlayOptions(groundOverlay, style)
186200

@@ -223,9 +237,6 @@ class MapViewRenderer(
223237
}
224238
}
225239
}
226-
if (mapObjects.isNotEmpty()) {
227-
renderedFeatures[feature] = mapObjects
228-
}
229240
}
230241

231242
override fun removeFeature(feature: Feature) {

data/src/test/java/com/google/maps/android/data/renderer/MapViewRendererTest.kt

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@ import com.google.android.gms.maps.model.Marker
2222
import com.google.android.gms.maps.model.MarkerOptions
2323
import com.google.maps.android.data.renderer.mapview.MapViewRenderer
2424
import com.google.maps.android.data.renderer.model.Feature
25+
import com.google.maps.android.data.renderer.model.LineString
26+
import com.google.maps.android.data.renderer.model.MultiGeometry
2527
import com.google.maps.android.data.renderer.model.Point
2628
import com.google.maps.android.data.renderer.model.PointGeometry
29+
import com.google.maps.android.data.renderer.model.Polygon
2730
import io.mockk.every
2831
import io.mockk.mockk
2932
import io.mockk.slot
@@ -143,4 +146,69 @@ class MapViewRendererTest {
143146
assertEquals("Critical Right Turn", capturedOptions.title)
144147
assertEquals("Be careful here!", capturedOptions.snippet)
145148
}
149+
150+
@Test
151+
fun testRemoveFeature_multiGeometry_removesAllRenderedObjects() {
152+
// Given
153+
val mockMap = mockk<GoogleMap>(relaxed = true)
154+
val mockIconProvider = mockk<IconProvider>(relaxed = true)
155+
156+
val addedPolygons = mutableListOf<com.google.android.gms.maps.model.Polygon>()
157+
every { mockMap.addPolygon(any()) } answers {
158+
mockk<com.google.android.gms.maps.model.Polygon>(relaxed = true).also { addedPolygons.add(it) }
159+
}
160+
161+
val renderer = MapViewRenderer(mockMap, mockIconProvider)
162+
val feature =
163+
Feature(
164+
geometry =
165+
MultiGeometry(
166+
listOf(
167+
Polygon(listOf(Point(0.0, 0.0), Point(0.0, 1.0), Point(1.0, 1.0))),
168+
Polygon(listOf(Point(2.0, 2.0), Point(2.0, 3.0), Point(3.0, 3.0))),
169+
),
170+
),
171+
)
172+
173+
// When
174+
renderer.addFeature(feature)
175+
renderer.removeFeature(feature)
176+
177+
// Then: both rendered polygons are removed via the original feature reference.
178+
assertEquals(2, addedPolygons.size)
179+
addedPolygons.forEach { polygon -> verify(exactly = 1) { polygon.remove() } }
180+
}
181+
182+
@Test
183+
fun testAddFeature_multiGeometry_addedTwice_doesNotLeakOnRemove() {
184+
// Given
185+
val mockMap = mockk<GoogleMap>(relaxed = true)
186+
val mockIconProvider = mockk<IconProvider>(relaxed = true)
187+
188+
val addedPolylines = mutableListOf<com.google.android.gms.maps.model.Polyline>()
189+
every { mockMap.addPolyline(any()) } answers {
190+
mockk<com.google.android.gms.maps.model.Polyline>(relaxed = true).also { addedPolylines.add(it) }
191+
}
192+
193+
val renderer = MapViewRenderer(mockMap, mockIconProvider)
194+
val feature =
195+
Feature(
196+
geometry =
197+
MultiGeometry(
198+
listOf(
199+
LineString(listOf(Point(0.0, 0.0), Point(1.0, 1.0))),
200+
),
201+
),
202+
)
203+
204+
// When: a hide/show cycle (remove + re-add) followed by a final remove.
205+
renderer.addFeature(feature)
206+
renderer.removeFeature(feature)
207+
renderer.addFeature(feature)
208+
renderer.removeFeature(feature)
209+
210+
// Then: every polyline ever added has been removed — nothing is left on the map.
211+
assertEquals(2, addedPolylines.size)
212+
addedPolylines.forEach { polyline -> verify(exactly = 1) { polyline.remove() } }
213+
}
146214
}

0 commit comments

Comments
 (0)