Skip to content

Commit

Permalink
fix cycleway tests
Browse files Browse the repository at this point in the history 
the below are actually fixes for bugs, which however should not have surfaced in the actual app because it is not possible to set the cycleway situation for only one side of a road when the other side has already been defined in the UI:

1. be more cautious when determining that the road is (now) also a oneway for cyclists: It can only safely be said when both sides are defined
2. retain oneway:bicycle information when translating old style tags to new style tags
  • Loading branch information
@westnordost
westnordost committed Dec 19, 2023
1 parent 5e09168 commit 0276c31
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 67 deletions.
15 changes: 7 additions & 8 deletions app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/Cycleway.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ fun LeftAndRightCycleway.selectableOrNullValues(countryInfo: CountryInfo): LeftA
fun LeftAndRightCycleway.wasNoOnewayForCyclistsButNowItIs(tags: Map<String, String>, isLeftHandTraffic: Boolean): Boolean =
isOneway(tags)
&& isNotOnewayForCyclists(tags, isLeftHandTraffic)
&& isNotOnewayForCyclistsNow(tags, isLeftHandTraffic) == false
&& isNotOnewayForCyclistsNow(tags) == false

/** Returns whether this is now not a oneway for cyclists. It returns null if any side
* is null because it is possible that the undefined side has a track in the contra-flow direction
* (e.g. either a normal track on the left side or a dual-track on the right side) */
fun LeftAndRightCycleway.isNotOnewayForCyclistsNow(tags: Map<String, String>): Boolean? {

/** Returns whether this is now not a oneway for cyclists. It returns null if the contra-flow side
* is null, i.e. no change is being made for the contra-flow side */
fun LeftAndRightCycleway.isNotOnewayForCyclistsNow(tags: Map<String, String>, isLeftHandTraffic: Boolean): Boolean? {
val onewayDir = when {
isForwardOneway(tags) -> FORWARD
isReversedOneway(tags) -> BACKWARD
Expand All @@ -52,10 +54,7 @@ fun LeftAndRightCycleway.isNotOnewayForCyclistsNow(tags: Map<String, String>, is
// right side has contra-flow to oneway
if (rightDir != null && rightDir != onewayDir) return true

// if no cycleway is defined for the side that is usually contra to the oneway flow, we
// return that we do not know it
val contraFlowSide = if (isForwardOneway(tags) xor isLeftHandTraffic) left else right
if (contraFlowSide == null) return null
if (left == null || right == null) return null

return false
}
Expand Down
14 changes: 11 additions & 3 deletions app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayCreator.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import de.westnordost.streetcomplete.osm.cycleway.Direction.*
import de.westnordost.streetcomplete.osm.expandSides
import de.westnordost.streetcomplete.osm.hasCheckDateForKey
import de.westnordost.streetcomplete.osm.isInContraflowOfOneway
import de.westnordost.streetcomplete.osm.isNotOnewayForCyclists
import de.westnordost.streetcomplete.osm.isOneway
import de.westnordost.streetcomplete.osm.isReversedOneway
import de.westnordost.streetcomplete.osm.mergeSides
Expand All @@ -30,7 +31,7 @@ fun LeftAndRightCycleway.applyTo(tags: Tags, isLeftHandTraffic: Boolean) {
tags.expandSides("cycleway", "oneway", false)
tags.expandSides("cycleway", "segregated", false)

applyOnewayNotForCyclists(tags, isLeftHandTraffic)
applyOnewayNotForCyclists(tags)
left?.applyTo(tags, false, isLeftHandTraffic)
right?.applyTo(tags, true, isLeftHandTraffic)

Expand Down Expand Up @@ -64,6 +65,13 @@ private fun expandBareTags(tags: Tags, isLeftHandTraffic: Boolean) {
} else {
"both"
}
// an important part of expanding bare tags is to expand the cycleway=opposite_* tagging to
// oneway:bicyle=no because this information gets lost otherwise
val isNoOnewayForCyclists = isNotOnewayForCyclists(tags, isLeftHandTraffic)
if (isNoOnewayForCyclists && !tags.containsKey("oneway:bicycle")) {
tags["oneway:bicycle"] = "no"
}

if (!tags.containsKey("cycleway:$side")) {
tags["cycleway:$side"] = cycleway
.removePrefix("opposite_") // opposite_track -> track etc.
Expand All @@ -82,8 +90,8 @@ private fun Tags.expandBareTagIntoSide(key: String, suffix: String = "", side: S
remove("$key$post")
}

private fun LeftAndRightCycleway.applyOnewayNotForCyclists(tags: Tags, isLeftHandTraffic: Boolean) {
val isNotOnewayForCyclistsNow = isNotOnewayForCyclistsNow(tags, isLeftHandTraffic)
private fun LeftAndRightCycleway.applyOnewayNotForCyclists(tags: Tags) {
val isNotOnewayForCyclistsNow = isNotOnewayForCyclistsNow(tags)
if (!isOneway(tags) || isNotOnewayForCyclistsNow == false) {
if (tags["oneway:bicycle"] == "no") {
tags.remove("oneway:bicycle")
Expand Down
17 changes: 9 additions & 8 deletions app/src/test/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayCreatorKtTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,11 +623,12 @@ class CyclewayCreatorKtTest {
"cycleway:both" to "track",
"cycleway:right:oneway" to "no",
),
cycleway(NONE, null),
cycleway(NONE to FORWARD, TRACK to BOTH),
arrayOf(
StringMapEntryAdd("cycleway:left", "no"),
StringMapEntryAdd("cycleway:right", "track"),
StringMapEntryDelete("cycleway:both", "track"),
StringMapEntryModify("cycleway:right:oneway", "no", "no"),
StringMapEntryModify("oneway:bicycle", "no", "no"),
)
)
Expand All @@ -639,10 +640,11 @@ class CyclewayCreatorKtTest {
"cycleway:left" to "no",
"cycleway:right" to "track",
),
cycleway(NONE, null),
cycleway(NONE, TRACK),
arrayOf(
StringMapEntryModify("cycleway:left", "no", "no"),
StringMapEntryDelete("oneway:bicycle", "no"),
StringMapEntryModify("cycleway:right", "track", "track"),
)
)
}
Expand Down Expand Up @@ -713,7 +715,8 @@ class CyclewayCreatorKtTest {
arrayOf(
StringMapEntryDelete("cycleway", "opposite"),
StringMapEntryAdd("cycleway:right", "no"),
StringMapEntryAdd("cycleway:left", "track")
StringMapEntryAdd("cycleway:left", "track"),
StringMapEntryAdd("oneway:bicycle", "no")
)
)
verifyAnswer(
Expand All @@ -736,18 +739,16 @@ class CyclewayCreatorKtTest {
mapOf(
"oneway" to "yes",
"cycleway" to "opposite_lane",
"cycleway:lane" to "advisory",
"cycleway:oneway" to "yes"
"cycleway:lane" to "advisory"
),
cycleway(null, NONE),
arrayOf(
StringMapEntryDelete("cycleway", "opposite_lane"),
StringMapEntryDelete("cycleway:lane", "advisory"),
StringMapEntryDelete("cycleway:oneway", "yes"),
StringMapEntryAdd("cycleway:left", "lane"),
StringMapEntryAdd("cycleway:left:lane", "advisory"),
StringMapEntryAdd("cycleway:left:oneway", "yes"),
StringMapEntryAdd("cycleway:right", "no")
StringMapEntryAdd("cycleway:right", "no"),
StringMapEntryAdd("oneway:bicycle", "no")
)
)
}
Expand Down
93 changes: 45 additions & 48 deletions app/src/test/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayKtTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,28 +71,28 @@ class CyclewayKtTest {

assertNull(
LeftAndRightCycleway(null, null)
.isNotOnewayForCyclistsNow(noOnewayForCyclists, false)
.isNotOnewayForCyclistsNow(noOnewayForCyclists)
)
assertNull(
LeftAndRightCycleway(null, forwardTrack)
.isNotOnewayForCyclistsNow(noOnewayForCyclists, false)
.isNotOnewayForCyclistsNow(noOnewayForCyclists)
)
assertNull(
LeftAndRightCycleway(backwardTrack, null)
.isNotOnewayForCyclistsNow(noOnewayForCyclistsReverse, false)
.isNotOnewayForCyclistsNow(noOnewayForCyclistsReverse)
)

assertNull(
LeftAndRightCycleway(null, null)
.isNotOnewayForCyclistsNow(noOnewayForCyclists, true)
.isNotOnewayForCyclistsNow(noOnewayForCyclists)
)
assertNull(
LeftAndRightCycleway(forwardTrack, null)
.isNotOnewayForCyclistsNow(noOnewayForCyclists, true)
.isNotOnewayForCyclistsNow(noOnewayForCyclists)
)
assertNull(
LeftAndRightCycleway(null, backwardTrack)
.isNotOnewayForCyclistsNow(noOnewayForCyclistsReverse, true)
.isNotOnewayForCyclistsNow(noOnewayForCyclistsReverse)
)
}

Expand All @@ -101,7 +101,7 @@ class CyclewayKtTest {
LeftAndRightCycleway(
CyclewayAndDirection(NONE, BACKWARD),
CyclewayAndDirection(NONE, FORWARD)
).isNotOnewayForCyclistsNow(mapOf(), false)!!
).isNotOnewayForCyclistsNow(mapOf())!!
)
}

Expand All @@ -110,7 +110,7 @@ class CyclewayKtTest {
LeftAndRightCycleway(
CyclewayAndDirection(NONE, BACKWARD),
CyclewayAndDirection(NONE, FORWARD)
).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!!
).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"))!!
)
}

Expand All @@ -119,7 +119,7 @@ class CyclewayKtTest {
LeftAndRightCycleway(
CyclewayAndDirection(NONE, BACKWARD),
CyclewayAndDirection(NONE, FORWARD)
).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!!
).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"))!!
)
}

Expand All @@ -128,35 +128,36 @@ class CyclewayKtTest {
LeftAndRightCycleway(
CyclewayAndDirection(NONE, BACKWARD),
CyclewayAndDirection(TRACK, BOTH)
).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!!
).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"))!!
)
assertTrue(
LeftAndRightCycleway(
CyclewayAndDirection(TRACK, BOTH),
CyclewayAndDirection(NONE, FORWARD)
).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!!
).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"))!!
)
}

@Test fun `oneway is no oneway for cyclists when any cycleway goes in contra-flow direction`() {
val forwardTrack = CyclewayAndDirection(TRACK, FORWARD)
val backwardTrack = CyclewayAndDirection(TRACK, BACKWARD)
val none = CyclewayAndDirection(NONE, BOTH)

assertTrue(
LeftAndRightCycleway(backwardTrack, null)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!!
LeftAndRightCycleway(backwardTrack, none)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"))!!
)
assertTrue(
LeftAndRightCycleway(null, backwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!!
LeftAndRightCycleway(none, backwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"))!!
)
assertTrue(
LeftAndRightCycleway(forwardTrack, null)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!!
LeftAndRightCycleway(forwardTrack, none)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"))!!
)
assertTrue(
LeftAndRightCycleway(null, forwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!!
LeftAndRightCycleway(none, forwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"))!!
)
}

Expand All @@ -165,17 +166,15 @@ class CyclewayKtTest {
val separate = CyclewayAndDirection(SEPARATE, BOTH)
val none = CyclewayAndDirection(NONE, BOTH)

for (isLeftHandTraffic in listOf(false, true)) {
for (oneway in listOf(mapOf("oneway" to "yes"), mapOf("oneway" to "-1"))) {
assertFalse(
LeftAndRightCycleway(separate, none)
.isNotOnewayForCyclistsNow(oneway, isLeftHandTraffic)!!
)
assertFalse(
LeftAndRightCycleway(none, separate)
.isNotOnewayForCyclistsNow(oneway, isLeftHandTraffic)!!
)
}
for (oneway in listOf(mapOf("oneway" to "yes"), mapOf("oneway" to "-1"))) {
assertFalse(
LeftAndRightCycleway(separate, none)
.isNotOnewayForCyclistsNow(oneway)!!
)
assertFalse(
LeftAndRightCycleway(none, separate)
.isNotOnewayForCyclistsNow(oneway)!!
)
}
}

Expand All @@ -185,23 +184,21 @@ class CyclewayKtTest {
// direction does not matter for NONE, best way to test that is to set BOTH
val none = CyclewayAndDirection(NONE, BOTH)

for (isLeftHandTraffic in listOf(false, true)) {
assertFalse(
LeftAndRightCycleway(forwardTrack, none)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), isLeftHandTraffic)!!
)
assertFalse(
LeftAndRightCycleway(none, forwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), isLeftHandTraffic)!!
)
assertFalse(
LeftAndRightCycleway(backwardTrack, none)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), isLeftHandTraffic)!!
)
assertFalse(
LeftAndRightCycleway(none, backwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), isLeftHandTraffic)!!
)
}
assertFalse(
LeftAndRightCycleway(forwardTrack, none)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"))!!
)
assertFalse(
LeftAndRightCycleway(none, forwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"))!!
)
assertFalse(
LeftAndRightCycleway(backwardTrack, none)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"))!!
)
assertFalse(
LeftAndRightCycleway(none, backwardTrack)
.isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"))!!
)
}
}

0 comments on commit 0276c31

Please sign in to comment.