From af1ad7eff4a67f718faa35bdd8775fe42d0d0758 Mon Sep 17 00:00:00 2001 From: Tobias Zwick Date: Sat, 10 Dec 2022 18:41:21 +0100 Subject: [PATCH] harden parser: acknowledge other "yes"-like values such as "permissive" etc --- .../SeparateCyclewayCreator.kt | 33 +++++++++-- .../SeparateCyclewayParser.kt | 16 ++++-- app/src/main/res/values/strings.xml | 4 +- .../SeparateCyclewayCreatorKtTest.kt | 55 +++++++++++++++---- .../SeparateCyclewayParserKtTest.kt | 12 +++- 5 files changed, 95 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreator.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreator.kt index 625ec0ebca..4ee6c63864 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreator.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreator.kt @@ -16,8 +16,14 @@ fun SeparateCycleway.applyTo(tags: Tags) { when (this) { PATH -> { tags["highway"] = "path" - if (tags["foot"] == "no" || tags["foot"] == "designated") tags["foot"] = "yes" - if (tags["bicycle"] == "no" || tags["bicycle"] == "designated") tags["bicycle"] = "yes" + + // only re-tag to "yes" if defined and not some kind of "yes" value + if (tags.containsKey("foot") && tags["foot"] !in yesButNotDesignated) { + tags["foot"] = "yes" + } + if (tags.containsKey("bicycle") && tags["bicycle"] !in yesButNotDesignated) { + tags["bicycle"] = "yes" + } } NOT_ALLOWED, ALLOWED_ON_FOOTWAY, NON_DESIGNATED -> { // not a cycleway if not designated as one! @@ -26,9 +32,15 @@ fun SeparateCycleway.applyTo(tags: Tags) { } when (this) { - NOT_ALLOWED -> tags["bicycle"] = "no" - ALLOWED_ON_FOOTWAY -> tags["bicycle"] = "yes" - else -> if (tags["bicycle"] == "designated") tags.remove("bicycle") + 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" @@ -38,7 +50,8 @@ fun SeparateCycleway.applyTo(tags: Tags) { if (!isCycleway || tags.containsKey("bicycle")) { tags["bicycle"] = "designated" } - if (isCycleway || tags.containsKey("foot")) { + // do not retag highway=cycleway + foot=yes + if ((isCycleway || tags.containsKey("foot")) && tags["foot"] !in yesButNotDesignated) { tags["foot"] = "designated" } } @@ -88,3 +101,11 @@ fun SeparateCycleway.applyTo(tags: Tags) { tags.updateCheckDateForKey("bicycle") } } + +private val noCycling = setOf( + "no", "dismount" +) + +private val yesButNotDesignated = setOf( + "yes", "permissive", "private", "destination", "customers", "permit" +) diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParser.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParser.kt index f45471e3de..a85db02675 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParser.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParser.kt @@ -33,19 +33,27 @@ fun createSeparateCycleway(tags: Map): SeparateCycleway? { else -> null // only happens if highway=cycleway } - if (bicycle == "no") return NOT_ALLOWED + if (bicycle in noCycling) return NOT_ALLOWED - if (bicycle == "yes" && foot == "yes") return PATH + if (bicycle in yesButNotDesignated && foot in yesButNotDesignated) return PATH - if (bicycle == "yes" && foot == "designated") return ALLOWED_ON_FOOTWAY + if (bicycle in yesButNotDesignated && foot == "designated") return ALLOWED_ON_FOOTWAY if (bicycle != "designated") return NON_DESIGNATED val hasSidewalk = createSidewalkSides(tags)?.any { it == Sidewalk.YES } == true || tags["sidewalk"] == "yes" if (hasSidewalk) return EXCLUSIVE_WITH_SIDEWALK - if (foot !in listOf("yes", "designated")) return EXCLUSIVE + if (foot == "no" || foot == null) return EXCLUSIVE val segregated = tags["segregated"] == "yes" return if (segregated) SEGREGATED else NON_SEGREGATED } + +private val noCycling = setOf( + "no", "dismount" +) + +private val yesButNotDesignated = setOf( + "yes", "permissive", "private", "destination", "customers", "permit" +) diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index d11c1667b3..be676a8d70 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -1582,8 +1582,8 @@ Partially means that a wheelchair can enter and use the restroom, but no handrai Cyclists are allowed Path or trail - - Sidewalk or foot path + + Not designated for cyclists (cycling may still be allowed) Shared-use path diff --git a/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreatorKtTest.kt b/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreatorKtTest.kt index a5e6dc5c4c..6635e3c67c 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreatorKtTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayCreatorKtTest.kt @@ -19,6 +19,16 @@ class SeparateCyclewayCreatorKtTest { ) } + @Test fun `apply none does not change bicycle=dismount`() { + verifyAnswer( + mapOf("highway" to "cycleway", "bicycle" to "dismount"), + NOT_ALLOWED, + arrayOf( + StringMapEntryModify("highway", "cycleway", "path"), + ) + ) + } + @Test fun `apply none re-tags cycleway and adds foot=yes if foot was no before`() { verifyAnswer( mapOf("highway" to "cycleway", "foot" to "no"), @@ -92,6 +102,24 @@ class SeparateCyclewayCreatorKtTest { ALLOWED_ON_FOOTWAY, arrayOf(StringMapEntryAdd("bicycle", "yes")) ) + verifyAnswer( + mapOf("highway" to "footway", "bicycle" to "no"), + ALLOWED_ON_FOOTWAY, + arrayOf(StringMapEntryModify("bicycle", "no", "yes")) + ) + } + + @Test fun `apply allowed does not re-tag bicycle=permissive etc`() { + verifyAnswer( + mapOf("highway" to "footway", "bicycle" to "permissive"), + ALLOWED_ON_FOOTWAY, + arrayOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())) + ) + verifyAnswer( + mapOf("highway" to "footway", "bicycle" to "private"), + ALLOWED_ON_FOOTWAY, + arrayOf(StringMapEntryAdd("check_date:bicycle", nowAsCheckDateString())) + ) } @Test fun `apply allowed re-tags cycleway and adds foot=yes if foot was not yes before`() { @@ -263,7 +291,6 @@ class SeparateCyclewayCreatorKtTest { NON_SEGREGATED, arrayOf( StringMapEntryAdd("bicycle", "designated"), - StringMapEntryModify("foot", "yes", "designated"), StringMapEntryAdd("segregated", "no"), ) ) @@ -286,6 +313,23 @@ class SeparateCyclewayCreatorKtTest { ) } + @Test fun `apply non-segregated does not re-tag any yes-like value`() { + verifyAnswer( + mapOf("highway" to "cycleway", "foot" to "yes"), + NON_SEGREGATED, + arrayOf( + StringMapEntryAdd("segregated", "no"), + ) + ) + verifyAnswer( + mapOf("highway" to "cycleway", "foot" to "permissive"), + NON_SEGREGATED, + arrayOf( + StringMapEntryAdd("segregated", "no"), + ) + ) + } + @Test fun `apply non-segregated removes sidewalk tags`() { verifyAnswer( mapOf( @@ -336,15 +380,6 @@ class SeparateCyclewayCreatorKtTest { StringMapEntryAdd("segregated", "yes"), ) ) - verifyAnswer( - mapOf("highway" to "footway", "foot" to "yes"), - SEGREGATED, - arrayOf( - StringMapEntryAdd("bicycle", "designated"), - StringMapEntryModify("foot", "yes", "designated"), - StringMapEntryAdd("segregated", "yes"), - ) - ) verifyAnswer( mapOf("highway" to "cycleway"), SEGREGATED, diff --git a/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParserKtTest.kt b/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParserKtTest.kt index 14e82b706d..b6202160b2 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParserKtTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway_separate/SeparateCyclewayParserKtTest.kt @@ -15,24 +15,30 @@ class SeparateCyclewayParserKtTest { assertEquals(PATH, parse("highway" to "path")) assertEquals(PATH, parse("highway" to "cycleway", "foot" to "yes", "bicycle" to "yes")) assertEquals(PATH, parse("highway" to "footway", "foot" to "yes", "bicycle" to "yes")) + assertEquals(PATH, parse("highway" to "path", "foot" to "permissive", "bicycle" to "permissive")) + 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")) } @Test fun `parse not designated for cyclists`() { assertEquals(NON_DESIGNATED, parse("highway" to "footway")) - assertEquals(NON_DESIGNATED, parse("highway" to "path", "bicycle" to "permissive")) - assertEquals(NON_DESIGNATED, parse("highway" to "path", "bicycle" to "private")) assertEquals(NON_DESIGNATED, parse("highway" to "cycleway", "bicycle" to "yes")) - + assertEquals(NON_DESIGNATED, parse("highway" to "cycleway", "bicycle" to "yes", "foot" to "no")) } @Test fun `parse no bicyclists allowed on path`() { assertEquals(NOT_ALLOWED, parse("highway" to "path", "bicycle" to "no")) assertEquals(NOT_ALLOWED, parse("highway" to "footway", "bicycle" to "no")) assertEquals(NOT_ALLOWED, parse("highway" to "cycleway", "bicycle" to "no")) + assertEquals(NOT_ALLOWED, parse("highway" to "cycleway", "bicycle" to "dismount")) } @Test fun `parse bicyclists allowed on footway`() { assertEquals(ALLOWED_ON_FOOTWAY, parse("highway" to "footway", "bicycle" to "yes")) + assertEquals(ALLOWED_ON_FOOTWAY, parse("highway" to "footway", "bicycle" to "destination")) + assertEquals(ALLOWED_ON_FOOTWAY, parse("highway" to "path", "foot" to "designated", "bicycle" to "yes")) + assertEquals(ALLOWED_ON_FOOTWAY, parse("highway" to "path", "foot" to "designated", "bicycle" to "permissive")) } @Test fun `parse cyclists on non-segregated path`() {