fix selecting "not designated" option did not change anything for way…

…s previously tagged as paths (fixes #5596)

renamed NON_DESIGNATED to NON_DESIGNATED_ON_FOOTWAY because the both ALLOWED_ON_FOOTWAY and NON_DESIGNATED_ON_FOOTWAY shall now signify that there is a footway present (foot=designated). The difference to PATH is that nothing is =designated.

app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCycleway.kt

@@ -9,7 +9,7 @@ enum class SeparateCycleway {
     ALLOWED_ON_FOOTWAY,
     /** Unspecific value: This footway is not designated for cyclists. I.e. it is
      *  either NOT_ALLOWED or ALLOWED_ON_FOOTWAY (or not specified) */
-    NON_DESIGNATED,
+    NON_DESIGNATED_ON_FOOTWAY,
     /** Designated but not segregated from footway mapped on same way */
     NON_SEGREGATED,
     /** Designated and segregated from footway mapped on same way */

app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreator.kt

@@ -25,27 +25,25 @@ fun SeparateCycleway.applyTo(tags: Tags) {
                 tags["bicycle"] = "yes"
             }
         }
-        NOT_ALLOWED, ALLOWED_ON_FOOTWAY, NON_DESIGNATED -> {
-            // not a cycleway if not designated as one!
+        NOT_ALLOWED -> {
+            if (tags["bicycle"] !in noCycling) tags["bicycle"] = "no"
             if (isCycleway) {
                 tags["highway"] = if (tags["foot"] == "designated") "footway" else "path"
             }
-
-            when (this) {
-                NOT_ALLOWED -> {
-                    if (tags["bicycle"] !in noCycling) tags["bicycle"] = "no"
-                }
-                ALLOWED_ON_FOOTWAY -> {
-                    if (tags["bicycle"] !in yesButNotDesignated) tags["bicycle"] = "yes"
-                }
-                else -> {
-                    if (tags["bicycle"] == "designated") tags.remove("bicycle")
-                }
-            }
             if (tags["foot"] == "no") {
                 tags["foot"] = "yes"
             }
         }
+        ALLOWED_ON_FOOTWAY, NON_DESIGNATED_ON_FOOTWAY -> {
+            tags["highway"] = "footway"
+            if (tags.containsKey("foot")) tags["foot"] = "designated"
+
+            if (this == ALLOWED_ON_FOOTWAY) {
+                if (tags["bicycle"] !in yesButNotDesignated) tags["bicycle"] = "yes"
+            } else {
+                if (tags["bicycle"] == "designated") tags.remove("bicycle")
+            }
+        }
         NON_SEGREGATED, SEGREGATED -> {
             if (!isCycleway || tags.containsKey("bicycle")) {
                 tags["bicycle"] = "designated"
@@ -88,7 +86,7 @@ fun SeparateCycleway.applyTo(tags: Tags) {
 
     // tag segregated
     when (this) {
-        PATH, NOT_ALLOWED, ALLOWED_ON_FOOTWAY, NON_DESIGNATED, EXCLUSIVE, EXCLUSIVE_WITH_SIDEWALK -> {
+        PATH, NOT_ALLOWED, ALLOWED_ON_FOOTWAY, NON_DESIGNATED_ON_FOOTWAY, EXCLUSIVE, EXCLUSIVE_WITH_SIDEWALK -> {
             tags.remove("segregated")
         }
         NON_SEGREGATED -> {

app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayItem.kt

@@ -11,18 +11,18 @@ private val SeparateCycleway.titleResId: Int get() = when (this) {
     PATH ->               R.string.separate_cycleway_path
     NOT_ALLOWED ->        R.string.separate_cycleway_no
     ALLOWED_ON_FOOTWAY -> R.string.separate_cycleway_allowed
-    NON_DESIGNATED ->     R.string.separate_cycleway_no_or_allowed
+    NON_DESIGNATED_ON_FOOTWAY -> R.string.separate_cycleway_no_or_allowed
     NON_SEGREGATED ->     R.string.separate_cycleway_non_segregated
     SEGREGATED ->         R.string.separate_cycleway_segregated
     EXCLUSIVE ->          R.string.separate_cycleway_exclusive
-    EXCLUSIVE_WITH_SIDEWALK ->  R.string.separate_cycleway_with_sidewalk
+    EXCLUSIVE_WITH_SIDEWALK -> R.string.separate_cycleway_with_sidewalk
 }
 
 private fun SeparateCycleway.getIconResId(isLeftHandTraffic: Boolean): Int = when (this) {
     PATH ->               R.drawable.ic_separate_cycleway_path
     NOT_ALLOWED ->        R.drawable.ic_separate_cycleway_no
     ALLOWED_ON_FOOTWAY -> R.drawable.ic_separate_cycleway_allowed
-    NON_DESIGNATED ->     R.drawable.ic_separate_cycleway_no
+    NON_DESIGNATED_ON_FOOTWAY -> R.drawable.ic_separate_cycleway_no
     NON_SEGREGATED ->     R.drawable.ic_separate_cycleway_not_segregated
     SEGREGATED ->
         if (isLeftHandTraffic) {

app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParser.kt

@@ -39,9 +39,9 @@ fun parseSeparateCycleway(tags: Map<String, String>): SeparateCycleway? {
 
     if (bicycle in yesButNotDesignated && foot == "designated") return ALLOWED_ON_FOOTWAY
 
-    if (bicycle in yesButNotDesignated && foot in yesButNotDesignated) return PATH
+    if (bicycle in yesButNotDesignated && foot != "designated") return PATH
 
-    if (bicycle != "designated") return NON_DESIGNATED
+    if (bicycle != "designated") return NON_DESIGNATED_ON_FOOTWAY
 
     val hasSidewalk = parseSidewalkSides(tags)?.any { it == Sidewalk.YES } == true || tags["sidewalk"] == "yes"
     if (hasSidewalk) return EXCLUSIVE_WITH_SIDEWALK

app/src/main/java/de/westnordost/streetcomplete/overlays/cycleway/CyclewayOverlay.kt

@@ -72,7 +72,7 @@ private fun getSeparateCyclewayStyle(element: Element) =
 private fun SeparateCycleway?.getColor() = when (this) {
     SeparateCycleway.NOT_ALLOWED,
     SeparateCycleway.ALLOWED_ON_FOOTWAY,
-    SeparateCycleway.NON_DESIGNATED,
+    SeparateCycleway.NON_DESIGNATED_ON_FOOTWAY,
     SeparateCycleway.PATH ->
         Color.BLACK
 

app/src/main/java/de/westnordost/streetcomplete/overlays/cycleway/SeparateCyclewayForm.kt

@@ -20,7 +20,7 @@ import org.koin.android.ext.android.inject
 class SeparateCyclewayForm : AImageSelectOverlayForm<SeparateCycleway>() {
 
     override val items: List<DisplayItem<SeparateCycleway>> get() =
-        listOf(PATH, NON_DESIGNATED, NON_SEGREGATED, SEGREGATED, EXCLUSIVE_WITH_SIDEWALK, EXCLUSIVE).map {
+        listOf(PATH, NON_DESIGNATED_ON_FOOTWAY, NON_SEGREGATED, SEGREGATED, EXCLUSIVE_WITH_SIDEWALK, EXCLUSIVE).map {
             it.asItem(countryInfo.isLeftHandTraffic)
         }
 
@@ -74,7 +74,7 @@ class SeparateCyclewayForm : AImageSelectOverlayForm<SeparateCycleway>() {
             prerequisite for it being  displayed as a selectable option due to the reasons stated
             above.
          */
-        currentCycleway = if (cycleway == NOT_ALLOWED || cycleway == ALLOWED_ON_FOOTWAY) NON_DESIGNATED else cycleway
+        currentCycleway = if (cycleway == NOT_ALLOWED || cycleway == ALLOWED_ON_FOOTWAY) NON_DESIGNATED_ON_FOOTWAY else cycleway
         selectedItem = currentCycleway?.asItem(countryInfo.isLeftHandTraffic)
     }
 

app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreatorKtTest.kt

@@ -98,13 +98,19 @@ class SeparateCyclewayCreatorKtTest {
 
     @Test fun `apply allowed`() {
         assertEquals(
-            setOf(StringMapEntryAdd("bicycle", "yes")),
+            setOf(
+                StringMapEntryAdd("bicycle", "yes"),
+                StringMapEntryModify("highway", "footway", "footway")
+            ),
             ALLOWED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway"
             ))
         )
         assertEquals(
-            setOf(StringMapEntryModify("bicycle", "no", "yes")),
+            setOf(
+                StringMapEntryModify("bicycle", "no", "yes"),
+                StringMapEntryModify("highway", "footway", "footway")
+            ),
             ALLOWED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "bicycle" to "no"
@@ -114,14 +120,20 @@ class SeparateCyclewayCreatorKtTest {
 
     @Test fun `apply allowed does not re-tag bicycle=permissive etc`() {
         assertEquals(
-            setOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())),
+            setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
+                StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())
+            ),
             ALLOWED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "bicycle" to "permissive"
             ))
         )
         assertEquals(
-            setOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())),
+            setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
+                StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())
+            ),
             ALLOWED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "bicycle" to "private"
@@ -133,8 +145,8 @@ class SeparateCyclewayCreatorKtTest {
         assertEquals(
             setOf(
                 StringMapEntryAdd("bicycle", "yes"),
-                StringMapEntryModify("highway", "cycleway", "path"),
-                StringMapEntryModify("foot", "no", "yes")
+                StringMapEntryModify("highway", "cycleway", "footway"),
+                StringMapEntryModify("foot", "no", "designated")
             ),
             ALLOWED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "cycleway",
@@ -147,7 +159,8 @@ class SeparateCyclewayCreatorKtTest {
         assertEquals(
             setOf(
                 StringMapEntryAdd("bicycle", "yes"),
-                StringMapEntryModify("highway", "cycleway", "footway")
+                StringMapEntryModify("highway", "cycleway", "footway"),
+                StringMapEntryModify("foot", "designated", "designated"),
             ),
             ALLOWED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "cycleway",
@@ -159,6 +172,7 @@ class SeparateCyclewayCreatorKtTest {
     @Test fun `apply allowed removes sidewalk and segregated tags`() {
         assertEquals(
             setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
                 StringMapEntryAdd("bicycle", "yes"),
                 StringMapEntryDelete("segregated", "yes"),
                 StringMapEntryDelete("sidewalk", "both"),
@@ -171,6 +185,7 @@ class SeparateCyclewayCreatorKtTest {
         )
         assertEquals(
             setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
                 StringMapEntryAdd("bicycle", "yes"),
                 StringMapEntryDelete("segregated", "yes"),
                 StringMapEntryDelete("sidewalk:both", "yes"),
@@ -183,6 +198,7 @@ class SeparateCyclewayCreatorKtTest {
         )
         assertEquals(
             setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
                 StringMapEntryAdd("bicycle", "yes"),
                 StringMapEntryDelete("segregated", "yes"),
                 StringMapEntryDelete("sidewalk:left", "yes"),
@@ -199,29 +215,41 @@ class SeparateCyclewayCreatorKtTest {
 
     @Test fun `apply non-designated`() {
         assertEquals(
-            setOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())),
-            NON_DESIGNATED.appliedTo(mapOf("highway" to "footway"))
+            setOf(
+                StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString()),
+                StringMapEntryModify("highway", "footway", "footway"),
+            ),
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf("highway" to "footway"))
         )
     }
 
     @Test fun `apply non-designated does not change bicycle tag unless it is designated`() {
         assertEquals(
-            setOf(StringMapEntryDelete("bicycle", "designated")),
-            NON_DESIGNATED.appliedTo(mapOf(
+            setOf(
+                StringMapEntryDelete("bicycle", "designated"),
+                StringMapEntryModify("highway", "footway", "footway"),
+            ),
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "bicycle" to "designated"
             ))
         )
         assertEquals(
-            setOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())),
-            NON_DESIGNATED.appliedTo(mapOf(
+            setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
+                StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())
+            ),
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "bicycle" to "yes"
             ))
         )
         assertEquals(
-            setOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())),
-            NON_DESIGNATED.appliedTo(mapOf(
+            setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
+                StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())
+            ),
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "bicycle" to "no"
             ))
@@ -231,10 +259,10 @@ class SeparateCyclewayCreatorKtTest {
     @Test fun `apply non-designated re-tags cycleway and adds foot=yes if foot was not yes before`() {
         assertEquals(
             setOf(
-                StringMapEntryModify("highway", "cycleway", "path"),
-                StringMapEntryModify("foot", "no", "yes")
+                StringMapEntryModify("highway", "cycleway", "footway"),
+                StringMapEntryModify("foot", "no", "designated")
             ),
-            NON_DESIGNATED.appliedTo(mapOf(
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "cycleway",
                 "foot" to "no"
             ))
@@ -244,9 +272,10 @@ class SeparateCyclewayCreatorKtTest {
     @Test fun `apply non-designated re-tags cycleway to footway if foot is designated`() {
         assertEquals(
             setOf(
-                StringMapEntryModify("highway", "cycleway", "footway")
+                StringMapEntryModify("highway", "cycleway", "footway"),
+                StringMapEntryModify("foot", "designated", "designated"),
             ),
-            NON_DESIGNATED.appliedTo(mapOf(
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "cycleway",
                 "foot" to "designated"
             )),
@@ -256,22 +285,24 @@ class SeparateCyclewayCreatorKtTest {
     @Test fun `apply non-designated removes sidewalk and segregated tags`() {
         assertEquals(
             setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
                 StringMapEntryDelete("segregated", "yes"),
                 StringMapEntryDelete("sidewalk", "both"),
             ),
-            NON_DESIGNATED.appliedTo(mapOf(
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "segregated" to "yes",
                 "sidewalk" to "both",
             ))
         )
         assertEquals(
             setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
                 StringMapEntryDelete("segregated", "yes"),
                 StringMapEntryDelete("sidewalk:left", "yes"),
                 StringMapEntryDelete("sidewalk:right", "yes"),
             ),
-            NON_DESIGNATED.appliedTo(mapOf(
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "segregated" to "yes",
                 "sidewalk:left" to "yes",
@@ -280,10 +311,11 @@ class SeparateCyclewayCreatorKtTest {
         )
         assertEquals(
             setOf(
+                StringMapEntryModify("highway", "footway", "footway"),
                 StringMapEntryDelete("segregated", "yes"),
                 StringMapEntryDelete("sidewalk:both", "yes"),
             ),
-            NON_DESIGNATED.appliedTo(mapOf(
+            NON_DESIGNATED_ON_FOOTWAY.appliedTo(mapOf(
                 "highway" to "footway",
                 "segregated" to "yes",
                 "sidewalk:both" to "yes",

app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParserKtTest.kt

@@ -21,12 +21,12 @@ class SeparateCyclewayParserKtTest {
         assertEquals(PATH, parse("highway" to "path", "foot" to "customers", "bicycle" to "destination"))
         assertEquals(PATH, parse("highway" to "path", "bicycle" to "permissive"))
         assertEquals(PATH, parse("highway" to "path", "bicycle" to "private"))
+        assertEquals(PATH, parse("highway" to "cycleway", "bicycle" to "yes"))
+        assertEquals(PATH, parse("highway" to "cycleway", "bicycle" to "yes", "foot" to "no"))
     }
 
     @Test fun `parse not designated for cyclists`() {
-        assertEquals(NON_DESIGNATED, parse("highway" to "footway"))
-        assertEquals(NON_DESIGNATED, parse("highway" to "cycleway", "bicycle" to "yes"))
-        assertEquals(NON_DESIGNATED, parse("highway" to "cycleway", "bicycle" to "yes", "foot" to "no"))
+        assertEquals(NON_DESIGNATED_ON_FOOTWAY, parse("highway" to "footway"))
     }
 
     @Test fun `parse no bicyclists allowed on path`() {