redo StreetParkingCreator (fixes #4634)

streetcomplete · Nov 17, 2022 · 6742ab0 · 6742ab0
1 parent e345e4b
commit 6742ab0
Show file tree

Hide file tree

Showing 6 changed files with 125 additions and 65 deletions.
diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/Tags.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/Tags.kt
@@ -4,23 +4,25 @@ import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapChanges
 
 typealias Tags = StringMapChangesBuilder
 
-fun Tags.expandSides(key: String, includeBareTag: Boolean) {
-    val both = get("$key:both") ?: (if (includeBareTag) get(key) else null)
+fun Tags.expandSides(key: String, postfix: String? = null, includeBareTag: Boolean) {
+    val post = if (postfix != null) ":$postfix" else ""
+    val both = get("$key:both$post") ?: (if (includeBareTag) get("$key$post") else null)
     if (both != null) {
         // *:left/right is seen as more specific/correct in case the two contradict each other
-        if (!containsKey("$key:left")) set("$key:left", both)
-        if (!containsKey("$key:right")) set("$key:right", both)
+        if (!containsKey("$key:left$post")) set("$key:left$post", both)
+        if (!containsKey("$key:right$post")) set("$key:right$post", both)
     }
-    remove("$key:both")
-    if (includeBareTag) remove(key)
+    remove("$key:both$post")
+    if (includeBareTag) remove("$key$post")
 }
 
-fun Tags.mergeSides(key: String) {
-    val left = get("$key:left")
-    val right = get("$key:right")
+fun Tags.mergeSides(key: String, postfix: String? = null) {
+    val post = if (postfix != null) ":$postfix" else ""
+    val left = get("$key:left$post")
+    val right = get("$key:right$post")
     if (left != null && left == right) {
-        set("$key:both", left)
-        remove("$key:left")
-        remove("$key:right")
+        set("$key:both$post", left)
+        remove("$key:left$post")
+        remove("$key:right$post")
     }
 }
diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/sidewalk/SidewalkCreator.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/sidewalk/SidewalkCreator.kt
@@ -19,7 +19,7 @@ fun LeftAndRightSidewalk.applyTo(tags: Tags) {
        - sidewalk:right=yes
        First separating the values and then later conflating them again, if possible, solves this.
     */
-    tags.expandSides("sidewalk", false)
+    tags.expandSides("sidewalk", includeBareTag = false)
     tags.separateConflatedSidewalk()
 
     if (left != null)  tags["sidewalk:left"] = left.osmValue

diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingCreator.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingCreator.kt
@@ -1,66 +1,62 @@
 package de.westnordost.streetcomplete.osm.street_parking
 
 import de.westnordost.streetcomplete.osm.Tags
+import de.westnordost.streetcomplete.osm.expandSides
 import de.westnordost.streetcomplete.osm.hasCheckDateForKey
+import de.westnordost.streetcomplete.osm.mergeSides
 import de.westnordost.streetcomplete.osm.updateCheckDateForKey
 
 fun LeftAndRightStreetParking.applyTo(tags: Tags) {
-    val currentParking = createStreetParkingSides(tags)
-
-    // first clear previous
-    val keyToRemove = Regex("parking:lane:(both|left|right)(:(parallel|diagonal|perpendicular))?")
-    for (key in tags.keys) {
-        if (key.matches(keyToRemove)) tags.remove(key)
-    }
-
-    val r = right ?: currentParking?.right
-    val l = left ?: currentParking?.left
-
-    // parking:lane:<left/right/both>
-    val laneRight = r?.toOsmLaneValue()
-    val laneLeft =  l?.toOsmLaneValue()
-
-    if (laneLeft == laneRight) {
-        if (laneLeft != null) tags["parking:lane:both"] = laneLeft
-    } else {
-        if (laneLeft != null) tags["parking:lane:left"] = laneLeft
-        if (laneRight != null) tags["parking:lane:right"] = laneRight
-    }
-
-    // parking:lane:<left/right/both>:<parallel/diagonal/perpendicular>
-    val positionRight = (r as? StreetParkingPositionAndOrientation)?.position?.toOsmValue()
-    val positionLeft = (l as? StreetParkingPositionAndOrientation)?.position?.toOsmValue()
-
-    if (laneLeft == laneRight && positionLeft == positionRight) {
-        if (positionLeft != null) tags["parking:lane:both:$laneLeft"] = positionLeft
-    } else {
-        if (positionLeft != null) tags["parking:lane:left:$laneLeft"] = positionLeft
-        if (positionRight != null) tags["parking:lane:right:$laneRight"] = positionRight
-    }
+    if (left == null && right == null) return
+    /* for being able to modify only one side (e.g. `left` is null while `right` is not null),
+       the sides conflated in :both keys need to be separated first. E.g. parking:lane=no
+       when left is made separate should become
+       - parking:lane:right=no
+       - parking:lane:left=separate
+       First separating the values and then later conflating them again, if possible, solves this.
+     */
+    tags.expandSides("parking:lane", null, true)
+    ParkingOrientation.values().forEach { tags.expandSides("parking:lane", it.osmValue, true) }
+
+    // parking:lane:<left/right>
+    left?.applyTo(tags, "left")
+    right?.applyTo(tags, "right")
+
+    tags.mergeSides("parking:lane")
+    ParkingOrientation.values().forEach { tags.mergeSides("parking:lane", it.osmValue) }
 
     if (!tags.hasChanges || tags.hasCheckDateForKey("parking:lane")) {
         tags.updateCheckDateForKey("parking:lane")
     }
 }
 
+private fun StreetParking.applyTo(tags: Tags, side: String) {
+    tags["parking:lane:$side"] = osmLaneValue
+    // clear previous orientation(s)
+    ParkingOrientation.values().forEach { tags.remove("parking:lane:$side:${it.osmValue}") }
+    if (this is StreetParkingPositionAndOrientation) {
+        tags["parking:lane:$side:$osmLaneValue"] = position.osmValue
+    }
+}
+
 /** get the OSM value for the parking:lane key */
-private fun StreetParking.toOsmLaneValue(): String = when (this) {
-    is StreetParkingPositionAndOrientation -> orientation.toOsmValue()
+private val StreetParking.osmLaneValue get() = when (this) {
+    is StreetParkingPositionAndOrientation -> orientation.osmValue
     NoStreetParking -> "no"
     StreetParkingSeparate -> "separate"
     UnknownStreetParking, IncompleteStreetParking -> throw IllegalArgumentException("Attempting to tag invalid parking lane")
 }
 
-private fun ParkingPosition.toOsmValue() = when (this) {
-    ParkingPosition.ON_STREET -> "on_street"
-    ParkingPosition.HALF_ON_KERB -> "half_on_kerb"
-    ParkingPosition.ON_KERB -> "on_kerb"
-    ParkingPosition.STREET_SIDE -> "street_side"
+private val ParkingPosition.osmValue get() = when (this) {
+    ParkingPosition.ON_STREET ->         "on_street"
+    ParkingPosition.HALF_ON_KERB ->      "half_on_kerb"
+    ParkingPosition.ON_KERB ->           "on_kerb"
+    ParkingPosition.STREET_SIDE ->       "street_side"
     ParkingPosition.PAINTED_AREA_ONLY -> "painted_area_only"
 }
 
-private fun ParkingOrientation.toOsmValue() = when (this) {
-    ParkingOrientation.PARALLEL -> "parallel"
-    ParkingOrientation.DIAGONAL -> "diagonal"
+private val ParkingOrientation.osmValue get() = when (this) {
+    ParkingOrientation.PARALLEL ->      "parallel"
+    ParkingOrientation.DIAGONAL ->      "diagonal"
     ParkingOrientation.PERPENDICULAR -> "perpendicular"
 }
diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingParser.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingParser.kt
@@ -67,7 +67,6 @@ private fun String.toParkingPosition() = when (this) {
 private fun expandRelevantSidesTags(tags: Map<String, String>): Map<String, String> {
     val result = tags.toMutableMap()
     expandSidesTag("parking:lane", "", result)
-    expandSidesTag("parking:condition", "", result)
     expandSidesTag("parking:lane", "parallel", result)
     expandSidesTag("parking:lane", "diagonal", result)
     expandSidesTag("parking:lane", "perpendicular", result)

diff --git a/app/src/test/java/de/westnordost/streetcomplete/osm/TagsKtTest.kt b/app/src/test/java/de/westnordost/streetcomplete/osm/TagsKtTest.kt
@@ -7,7 +7,7 @@ class TagsKtTest {
 
     @Test fun `expand sides`() {
         val tags = Tags(mapOf("a:both" to "blub", "a" to "norg"))
-        tags.expandSides("a", false)
+        tags.expandSides("a", includeBareTag = false)
         assertEquals(
             mapOf("a:left" to "blub", "a:right" to "blub", "a" to "norg"),
             tags.toMap()
@@ -16,7 +16,7 @@ class TagsKtTest {
 
     @Test fun `expand sides with bare tag`() {
         val tags = Tags(mapOf("a" to "blub"))
-        tags.expandSides("a", true)
+        tags.expandSides("a", includeBareTag = true)
         assertEquals(
             mapOf("a:left" to "blub", "a:right" to "blub"),
             tags.toMap()
@@ -25,7 +25,7 @@ class TagsKtTest {
 
     @Test fun `when expanding sides, explicit tag takes precedence over bare tag`() {
         val tags = Tags(mapOf("a" to "blub", "a:both" to "burg"))
-        tags.expandSides("a", true)
+        tags.expandSides("a", includeBareTag = true)
         assertEquals(
             mapOf("a:left" to "burg", "a:right" to "burg"),
             tags.toMap()
@@ -34,7 +34,7 @@ class TagsKtTest {
 
     @Test fun `when expanding sides, does not overwrite left and right tags`() {
         val tags = Tags(mapOf("a:both" to "gah", "a:left" to "le", "a:right" to "ri"))
-        tags.expandSides("a", true)
+        tags.expandSides("a", includeBareTag = true)
         assertEquals(
             mapOf("a:left" to "le", "a:right" to "ri"),
             tags.toMap()
@@ -43,13 +43,29 @@ class TagsKtTest {
 
     @Test fun `don't expand unrelated tags sides`() {
         val tags = Tags(mapOf("abc:both" to "blub"))
-        tags.expandSides("a", false)
+        tags.expandSides("a", includeBareTag = false)
         assertEquals(
             mapOf("abc:both" to "blub"),
             tags.toMap()
         )
     }
 
+    @Test fun `expand sides with postfix`() {
+        val tags = Tags(mapOf("a:both:c" to "blub", "a:c" to "blarg"))
+        tags.expandSides("a", "c", includeBareTag = false)
+        assertEquals(
+            mapOf("a:left:c" to "blub", "a:right:c" to "blub", "a:c" to "blarg"),
+            tags.toMap()
+        )
+
+        val tags2 = Tags(mapOf("a:c" to "blarg"))
+        tags2.expandSides("a", "c", includeBareTag = true)
+        assertEquals(
+            mapOf("a:left:c" to "blarg", "a:right:c" to "blarg"),
+            tags2.toMap()
+        )
+    }
+
     @Test fun `merge sides`() {
         val tags = Tags(mapOf("a:left" to "blub", "a:right" to "blub"))
         tags.mergeSides("a")

diff --git a/.../test/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingCreatorKtTest.kt b/.../test/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingCreatorKtTest.kt
@@ -11,6 +11,14 @@ import org.junit.Test
 
 class StreetParkingCreatorKtTest {
 
+    @Test fun `apply nothing applies nothing`() {
+        verifyAnswer(
+            mapOf(),
+            LeftAndRightStreetParking(null, null),
+            arrayOf()
+        )
+    }
+
     @Test fun `apply no parking`() {
         verifyAnswer(
             mapOf(),
@@ -127,14 +135,53 @@ class StreetParkingCreatorKtTest {
         )
     }
 
-    @Test fun `changing one side replaces both-tag`() {
+    @Test fun `apply for one side does not touch the other side`() {
+        verifyAnswer(
+            mapOf("parking:lane:left" to "separate"),
+            LeftAndRightStreetParking(null, NoStreetParking),
+            arrayOf(
+                StringMapEntryAdd("parking:lane:right", "no")
+            )
+        )
+        verifyAnswer(
+            mapOf("parking:lane:right" to "no"),
+            LeftAndRightStreetParking(StreetParkingSeparate, null),
+            arrayOf(
+                StringMapEntryAdd("parking:lane:left", "separate")
+            )
+        )
+    }
+
+    @Test fun `apply for one side does not touch the other side even if it is invalid`() {
+        verifyAnswer(
+            mapOf("parking:lane:left" to "narrow"),
+            LeftAndRightStreetParking(null, NoStreetParking),
+            arrayOf(
+                StringMapEntryAdd("parking:lane:right", "no")
+            )
+        )
+        verifyAnswer(
+            mapOf("parking:lane:right" to "narrow"),
+            LeftAndRightStreetParking(StreetParkingSeparate, null),
+            arrayOf(
+                StringMapEntryAdd("parking:lane:left", "separate")
+            )
+        )
+    }
+
+    @Test fun `apply for one side does not change values for the other side even if it was defined for both sides before and invalid`() {
         verifyAnswer(
-            mapOf("parking:lane:both" to "separate"),
+            mapOf(
+                "parking:lane" to "hexagonal",
+                "parking:lane:diagonal" to "on_kerb",
+            ),
             LeftAndRightStreetParking(null, NoStreetParking),
             arrayOf(
-                StringMapEntryAdd("parking:lane:left", "separate"),
                 StringMapEntryAdd("parking:lane:right", "no"),
-                StringMapEntryDelete("parking:lane:both", "separate"),
+                StringMapEntryDelete("parking:lane", "hexagonal"),
+                StringMapEntryDelete("parking:lane:diagonal", "on_kerb"),
+                StringMapEntryAdd("parking:lane:left", "hexagonal"),
+                StringMapEntryAdd("parking:lane:left:diagonal", "on_kerb"),
             )
         )
     }