Replay edits done during quests download (#5473)

MapDataWithEditsSource: call onUpdated to listeners again after the call onReplacedForBBox to listeners completed for updates that happened while the latter was being executed (fixes #4258)

app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/MapDataWithEditsSource.kt

@@ -19,6 +19,7 @@ import de.westnordost.streetcomplete.data.osm.mapdata.MapDataController
 import de.westnordost.streetcomplete.data.osm.mapdata.MapDataRepository
 import de.westnordost.streetcomplete.data.osm.mapdata.MapDataUpdates
 import de.westnordost.streetcomplete.data.osm.mapdata.MapDataWithGeometry
+import de.westnordost.streetcomplete.data.osm.mapdata.MapDataWithGeometryUpdates
 import de.westnordost.streetcomplete.data.osm.mapdata.MutableMapData
 import de.westnordost.streetcomplete.data.osm.mapdata.MutableMapDataWithGeometry
 import de.westnordost.streetcomplete.data.osm.mapdata.Node
@@ -61,6 +62,15 @@ class MapDataWithEditsSource internal constructor(
     private val updatedElements = HashMap<ElementKey, Element>()
     private val updatedGeometries = HashMap<ElementKey, ElementGeometry?>()
 
+    // onReplacedForBBox may not be called in parallel
+    private val onReplacedForBBoxLock = Any()
+
+    // access to isReplacingForBBox is atomic (didn't want to pull in kotlinx-atomicfu dependency just for this)
+    private val isReplacingForBBoxLock = Any()
+    private var isReplacingForBBox: Boolean = false
+
+    private val updatesWhileReplacingBBox = MapDataWithGeometryUpdates()
+
     private val mapDataListener = object : MapDataController.Listener {
 
         override fun onUpdated(updated: MutableMapDataWithGeometry, deleted: Collection<ElementKey>) {
@@ -136,12 +146,21 @@ class MapDataWithEditsSource internal constructor(
         }
 
         override fun onReplacedForBBox(bbox: BoundingBox, mapDataWithGeometry: MutableMapDataWithGeometry) {
-            synchronized(this) {
-                rebuildLocalChanges()
-                modifyBBoxMapData(bbox, mapDataWithGeometry)
-            }
+            synchronized(onReplacedForBBoxLock) {
+                synchronized(isReplacingForBBoxLock) { isReplacingForBBox = true }
+
+                synchronized(this) {
+                    rebuildLocalChanges()
+                    modifyBBoxMapData(bbox, mapDataWithGeometry)
+                }
+
+                callOnReplacedForBBox(bbox, mapDataWithGeometry)
 
-            callOnReplacedForBBox(bbox, mapDataWithGeometry)
+                synchronized(isReplacingForBBoxLock) { isReplacingForBBox = false }
+
+                callOnUpdated(updatesWhileReplacingBBox.updated, updatesWhileReplacingBBox.deleted)
+                updatesWhileReplacingBBox.clear()
+            }
         }
 
         override fun onCleared() {
@@ -515,9 +534,15 @@ class MapDataWithEditsSource internal constructor(
         listeners.remove(listener)
     }
 
-    private fun callOnUpdated(updated: MapDataWithGeometry = MutableMapDataWithGeometry(), deleted: Collection<ElementKey> = emptyList()) {
+    private fun callOnUpdated(updated: MapDataWithGeometry, deleted: Collection<ElementKey>) {
         if (updated.size == 0 && deleted.isEmpty()) return
         listeners.forEach { it.onUpdated(updated, deleted) }
+
+        synchronized(isReplacingForBBoxLock) {
+            if (isReplacingForBBox) {
+                updatesWhileReplacingBBox.add(updated, deleted)
+            }
+        }
     }
     private fun callOnReplacedForBBox(bbox: BoundingBox, mapDataWithGeometry: MapDataWithGeometry) {
         if (mapDataWithGeometry.size == 0) return

app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataWithGeometryUpdates.kt

@@ -0,0 +1,19 @@
+package de.westnordost.streetcomplete.data.osm.mapdata
+
+data class MapDataWithGeometryUpdates(
+    val updated: MutableMapDataWithGeometry = MutableMapDataWithGeometry(),
+    val deleted: MutableList<ElementKey> = ArrayList()
+) {
+    fun add(updated: MapDataWithGeometry, deleted: Collection<ElementKey>) {
+        this.deleted.removeAll(updated.map { it.key })
+        this.deleted.addAll(deleted)
+
+        this.updated.removeAll(deleted)
+        this.updated.putAll(updated)
+    }
+
+    fun clear() {
+        updated.clear()
+        deleted.clear()
+    }
+}

app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MutableMapDataWithGeometry.kt

@@ -62,6 +62,18 @@ class MutableMapDataWithGeometry() : MapDataWithGeometry {
         }
     }
 
+    fun putAll(other: MapDataWithGeometry) {
+        for (node in other.nodes) {
+            put(node, other.getNodeGeometry(node.id))
+        }
+        for (way in other.ways) {
+            put(way, other.getWayGeometry(way.id))
+        }
+        for (relation in other.relations) {
+            put(relation, other.getRelationGeometry(relation.id))
+        }
+    }
+
     fun putElement(element: Element) {
         val id = element.id
         when (element) {
@@ -79,6 +91,22 @@ class MutableMapDataWithGeometry() : MapDataWithGeometry {
         }
     }
 
+
+    fun removeAll(deleted: Collection<ElementKey>) {
+        for (key in deleted) {
+            remove(key.type, key.id)
+        }
+    }
+
+    fun clear() {
+        nodesById.clear()
+        waysById.clear()
+        relationsById.clear()
+        nodeGeometriesById.clear()
+        wayGeometriesById.clear()
+        relationGeometriesById.clear()
+    }
+
     override fun iterator(): Iterator<Element> {
         return (nodes.asSequence() + ways.asSequence() + relations.asSequence()).iterator()
     }

app/src/main/java/de/westnordost/streetcomplete/util/Listeners.kt

@@ -2,23 +2,20 @@ package de.westnordost.streetcomplete.util
 
 /** Lightweight wrapper around `HashSet` for storing listeners in a thread-safe way */
 class Listeners<T> {
-    private val listeners = HashSet<T>()
+    private var listeners: Set<T> = HashSet()
 
-    fun add(element: T): Boolean {
-        synchronized(this) {
-            return listeners.add(element)
-        }
+    fun add(element: T) {
+        synchronized(this) { listeners = listeners + element }
     }
 
-    fun remove(element: T): Boolean {
-        synchronized(this) {
-            return listeners.remove(element)
-        }
+    fun remove(element: T) {
+        synchronized(this) { listeners = listeners - element }
     }
 
     fun forEach(action: (T) -> Unit) {
-        synchronized(this) {
-            listeners.forEach(action)
-        }
+        val listeners = synchronized(this) { listeners }
+        // the executing of the action itself is not synchronized, only the access to the set,
+        // because it should be possible to call several Listeners::forEach at the same time
+        listeners.forEach(action)
     }
 }

app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MutableMapDataWithGeometryTest.kt

@@ -9,6 +9,7 @@ import de.westnordost.streetcomplete.testutils.rel
 import de.westnordost.streetcomplete.testutils.way
 import kotlin.test.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 
 class MutableMapDataWithGeometryTest {
@@ -65,4 +66,56 @@ class MutableMapDataWithGeometryTest {
         assertEquals(rel, m.getRelation(rel.id))
         assertEquals(geom, m.getRelationGeometry(rel.id))
     }
+
+    @Test fun clear() {
+        val m = MutableMapDataWithGeometry()
+        m.put(node(0), ElementPointGeometry(p()))
+        m.put(way(0), ElementPolylinesGeometry(listOf(), p()))
+        m.put(rel(0), ElementPolygonsGeometry(listOf(), p()))
+
+        assertEquals(3, m.size)
+
+        m.clear()
+        assertEquals(0, m.size)
+        assertEquals(0, m.nodes.size)
+        assertEquals(0, m.ways.size)
+        assertEquals(0, m.relations.size)
+        assertNull(m.getNodeGeometry(0))
+        assertNull(m.getWayGeometry(0))
+        assertNull(m.getRelationGeometry(0))
+    }
+
+    @Test fun putAll() {
+        val m = MutableMapDataWithGeometry()
+        m.put(node(0), ElementPointGeometry(p()))
+        m.put(way(0), ElementPolylinesGeometry(listOf(), p()))
+        m.put(rel(0), ElementPolygonsGeometry(listOf(), p()))
+
+        val n = MutableMapDataWithGeometry()
+        n.putAll(m)
+
+        assertEquals(3, n.size)
+        assertEquals(1, m.nodes.size)
+        assertEquals(1, m.ways.size)
+        assertEquals(1, m.relations.size)
+        assertNotNull(m.getNodeGeometry(0))
+        assertNotNull(m.getWayGeometry(0))
+        assertNotNull(m.getRelationGeometry(0))
+    }
+
+    @Test fun removeAll() {
+        val m = MutableMapDataWithGeometry()
+        m.put(node(0), ElementPointGeometry(p()))
+        m.put(node(1), ElementPointGeometry(p()))
+        m.put(way(0), ElementPolylinesGeometry(listOf(), p()))
+
+        m.removeAll(listOf(ElementKey(ElementType.NODE, 0), ElementKey(ElementType.WAY, 0)))
+
+        assertNull(m.getNode(0))
+        assertNull(m.getNodeGeometry(0))
+        assertNull(m.getWay(0))
+        assertNull(m.getWayGeometry(0))
+        assertNotNull(m.getNode(1))
+        assertNotNull(m.getNodeGeometry(1))
+    }
 }