stile resurvey, fixes #3187 #3370

matkoniecz · 2021-10-11T18:54:01Z

resurvey stiles, fixes #3187

resurvey happens after 8 years, so only small pool of old stiles with stile=* becomes qualified
stiles without stile=* tag would get this quest anyway
note that in rare cases, where stile type is unchanged and material tag is added - then not strictly necessary stile:check_date will be added

when changing style type also step count should be removed

resurvey happens after 8 years, so only small pool of old stiles with stile=* becomes qualified stiles without stile=* tag would get this quest anyway note that in rare cases, where stile type is unchanged and material tag is added - then not strictly necessary stile:check_date will be added

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

in this case maybe deleting all of them would be a good idea, but it has history of being changes to whitelist anyway

peternewman

Generally LGTM, a few possible resurvey improvements.

This is however going to make #2754 harder again unfortunately. I suspect the ideal would be a generic update function which takes a list of tags to deleteIfExist if the main tag's value has changed, but I can do that in my PR if you'd rather.

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

matkoniecz · 2021-10-12T14:40:22Z

Spotted one problem - on marking stepover stile where material was marked already it is not OK to (1) delete it in general function (2) later add it

It results in:

java.lang.IllegalStateException: The key 'material' is already being modified.

in case where wooden stepover stile is replaced by stone stepover tile (or in reverse) - delete tags such as step count

extracts complex repeated part, so applyAnswerTo can be comprehended

matkoniecz · 2021-10-12T15:26:33Z

For now missing material is not triggering resurvey and I am still not really convinced that it should happen.

(BTW, I am curious how many people are actually using SC in areas with stiles anyway - and is time saved by implementing this quests greater than total time used to implement. research and discuss them :) )

peternewman

More minor suggestions

peternewman · 2021-10-12T15:30:52Z

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

+    private fun StringMapChangesBuilder.deleteIfExistList(keys: List<String>) {
+        keys.forEach { deleteIfExists(it) }
+    }


Should this not go in app/src/main/java/de/westnordost/streetcompletedata/osm/edits/update_tags/StringMapChangesBuilder.kt so it can be reused elsewhere?

Can we override like in C++ so they're both called deleteIfExists and you can pass in a List or a String?

@matkoniecz
please either put this function into app/src/main/java/de/westnordost/streetcompletedata/osm/edits/update_tags/StringMapChangesXt.kt or at the end of this file outside the class definition (your choice).

Can we override like in C++ so they're both called deleteIfExists and you can pass in a List or a String?

Yes, no problem. The naming of the function is all the same to me though.

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

Co-authored-by: Peter Newman <[email protected]>

peternewman · 2021-10-12T16:01:05Z

For now missing material is not triggering resurvey and I am still not really convinced that it should happen.

As in you don't think it's worth asking earlier for the material than during a traditional resurvey?

(BTW, I am curious how many people are actually using SC in areas with stiles anyway - and is time saved by implementing this quests greater than total time used to implement. research and discuss them :) )

I try and use SC whenever I go out walking (and wherever I walk). I very rarely bother with iD or other local editing in front of my PC (or anything else aside from SC). I've also just finished walking coast to coast across the UK (via Hadrian's Wall) although it seems I've only answered five stile quests so far (partly as some weren't tagged, hence #3372 ) and partly not enough data loaded (it's a very long way). However that's more than the number of CCTV quests I've done, which have been around for similar lengths of time and I spend far more time in the city...

Aside from the relative value of different types of quests/their data, then generally I'll go places with stiles less frequently, and the same place less so, than say commuting around parts of the city, so gathering that data then is more valuable than somewhere I pass daily for work and so could build up over a number of days.

The secret truth I probably don't want to tell myself is that for me personally I only really use OSM for car sat nav currently, so the connected graph of streets, road and house names/numbers and maybe speed limit and road surface is all I directly use... Although I'm seeing more websites switch to it recently.

westnordost · 2021-10-13T17:35:28Z

I haven't been following the conversation. @matkoniecz , please mark if this ready to review/merge by requesting a review when everything that you think should be resolved is resolved.

matkoniecz · 2021-10-13T18:04:42Z

Given that material is not really used anywhere despite being tagged quite frequently - I think that I will keep it as is.

westnordost

Given the added complexity in applyAnswerTo, I strongly suggest to write test cases to cover the different code paths here but I'll leave this up to you. In the end, this is your quest type.

It might be possible to reduce the complexity by following @peternewman 's advice.

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

westnordost · 2021-10-13T21:40:43Z

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

+    private fun StringMapChangesBuilder.deleteIfExistList(keys: List<String>) {
+        keys.forEach { deleteIfExists(it) }
+    }


@matkoniecz
please either put this function into app/src/main/java/de/westnordost/streetcompletedata/osm/edits/update_tags/StringMapChangesXt.kt or at the end of this file outside the class definition (your choice).

Can we override like in C++ so they're both called deleteIfExists and you can pass in a List or a String?

Yes, no problem. The naming of the function is all the same to me though.

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt

Co-authored-by: Tobias Zwick <[email protected]>

It would crash completely without barrier tag It would not tag barrier=stile if it wold be unset and stile would be selected Co-authored-by: Tobias Zwick <[email protected]>

prevent claim that value was modified in cases where it was not actually changed

app/src/test/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileTypeTest.kt

Co-authored-by: Flo Edelmann <[email protected]>

confirm that toMutableList is avoidable

westnordost · 2021-10-14T16:49:45Z

...ain/java/de/westnordost/streetcomplete/data/osm/edits/update_tags/StringMapChangesBuilder.kt

+    fun deleteIfExistList(keys: List<String>) {
+        keys.forEach { deleteIfExists(it) }
+    }
+


As I wrote in my earlier review, I'd prefer if you added this function to StringMapChangesXt.kt, i.e. as an extension function.

peternewman

I think your function probably still needs a little more improvement and that these test cases should be added either way.

app/src/test/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileTypeTest.kt

Co-authored-by: Peter Newman <[email protected]>

This reverts commit d259ed0.

matkoniecz · 2021-10-14T17:45:13Z

@peternewman Thanks for noticing the problem!

westnordost · 2021-10-15T10:21:08Z

ok so err... should I review this again?

matkoniecz · 2021-10-15T10:52:46Z

There were no significant architectural changes, though a serious bug was fixed. Oh, and tests were added.

And I still need to handle #3370 (review)

westnordost · 2021-10-15T12:04:19Z

Don't mind that, I will do this after merging.

westnordost · 2021-10-15T12:51:20Z

Hm, after merge I found some more deficiencies and one bug. I'll create a PR.

peternewman · 2021-10-15T16:21:23Z

Given that material is not really used anywhere despite being tagged quite frequently - I think that I will keep it as is.

Vespucci uses it for stile related presets, although maybe just to tag them, I don't know if it displays the info too:
https://taginfo.openstreetmap.org/projects/vespucci#tags

peternewman · 2021-10-15T16:38:11Z

@peternewman Thanks for noticing the problem!

Well done for spotting the far bigger bug I missed with the wrong key!

matkoniecz added 2 commits October 11, 2021 19:17

problematic stile resurvey

24e33e6

when changing style type also step count should be removed

matkoniecz changed the title ~~No longer failed stile resurvey~~ No longer failed stile resurvey, fixes #3187 Oct 11, 2021

matkoniecz changed the title ~~No longer failed stile resurvey, fixes #3187~~ stile resurvey, fixes #3187 Oct 11, 2021

westnordost approved these changes Oct 11, 2021

View reviewed changes

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt Outdated Show resolved Hide resolved

matkoniecz force-pushed the no_longer_failed_stile_resurvey branch from 14c2292 to 6965331 Compare October 11, 2021 19:06

explain how tag list was obtained

6458090

in this case maybe deleting all of them would be a good idea, but it has history of being changes to whitelist anyway

matkoniecz added the hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event label Oct 11, 2021

peternewman reviewed Oct 12, 2021

View reviewed changes

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt Outdated Show resolved Hide resolved

app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt Outdated Show resolved Hide resolved

matkoniecz marked this pull request as draft October 12, 2021 14:38

matkoniecz added 3 commits October 12, 2021 16:41

better handle stepover stile resurvey

0d02cb6

in case where wooden stepover stile is replaced by stone stepover tile (or in reverse) - delete tags such as step count

handle restrictions requiring single tag modification

1609299

rewrite to reduce XXL function

3460acf

extracts complex repeated part, so applyAnswerTo can be comprehended

matkoniecz marked this pull request as ready for review October 12, 2021 15:26

peternewman reviewed Oct 12, 2021

View reviewed changes

matkoniecz and others added 2 commits October 12, 2021 17:40

fix English

5038ad9

Co-authored-by: Peter Newman <[email protected]>

pull what can be pulled directly from answer

1bb4bbd

This was referenced Oct 12, 2021

What type of Stile/Gate/Gap is here? #3372

Closed

Stile replacements and other edge cases #3188

Closed

matkoniecz requested a review from westnordost October 13, 2021 18:02

westnordost requested changes Oct 13, 2021

View reviewed changes

matkoniecz and others added 4 commits October 14, 2021 13:32

Nicer Kotlin

f2b9772

Co-authored-by: Tobias Zwick <[email protected]>

change name as requested

921d1b2

switch function to companion object

1924b89

Prevent problems on barrier!=stile stile=*

c18c873

It would crash completely without barrier tag It would not tag barrier=stile if it wold be unset and stile would be selected Co-authored-by: Tobias Zwick <[email protected]>

matkoniecz added 5 commits October 14, 2021 15:01

prevent incorrect modification claims

1659647

prevent claim that value was modified in cases where it was not actually changed

add tests

6a1e46c

move function around

a644cfe

whitespace change

99f70c9

better specify that only tags in list will be removed

c6f4756

FloEdelmann reviewed Oct 14, 2021

View reviewed changes

app/src/test/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileTypeTest.kt Outdated Show resolved Hide resolved

matkoniecz and others added 2 commits October 14, 2021 15:09

avoid setting extra tags if not needed

6edfe7a

Fix English

d2faba2

Co-authored-by: Flo Edelmann <[email protected]>

matkoniecz requested a review from westnordost October 14, 2021 13:10

one more test case with just check date

0556b7f

confirm that toMutableList is avoidable

westnordost approved these changes Oct 14, 2021

View reviewed changes

peternewman suggested changes Oct 14, 2021

View reviewed changes

app/src/test/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileTypeTest.kt Show resolved Hide resolved

matkoniecz and others added 6 commits October 14, 2021 19:21

Test for adding stile=* tag correctly

92b4d1d

Co-authored-by: Peter Newman <[email protected]>

remove duplicated test

d259ed0

fix horrific mistake

8680110

stop damaging existing data

eb6e8e0

Revert "remove duplicated test"

cd08185

This reverts commit d259ed0.

tweak tests

281cca9

matkoniecz force-pushed the no_longer_failed_stile_resurvey branch from 3f0e575 to 281cca9 Compare October 14, 2021 17:44

westnordost merged commit ea420dc into streetcomplete:master Oct 15, 2021

peternewman mentioned this pull request Oct 15, 2021

Fix stile resurvey #3384

Merged

matkoniecz deleted the no_longer_failed_stile_resurvey branch October 15, 2021 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stile resurvey, fixes #3187 #3370

stile resurvey, fixes #3187 #3370

matkoniecz commented Oct 11, 2021

peternewman left a comment

matkoniecz commented Oct 12, 2021

matkoniecz commented Oct 12, 2021 •

edited

Loading

peternewman left a comment

peternewman Oct 12, 2021

westnordost Oct 13, 2021

peternewman commented Oct 12, 2021

westnordost commented Oct 13, 2021

matkoniecz commented Oct 13, 2021

westnordost left a comment •

edited

Loading

westnordost Oct 13, 2021

westnordost Oct 14, 2021

peternewman left a comment

matkoniecz commented Oct 14, 2021

westnordost commented Oct 15, 2021

matkoniecz commented Oct 15, 2021

westnordost commented Oct 15, 2021 •

edited

Loading

westnordost commented Oct 15, 2021

peternewman commented Oct 15, 2021

peternewman commented Oct 15, 2021

stile resurvey, fixes #3187 #3370

stile resurvey, fixes #3187 #3370

Conversation

matkoniecz commented Oct 11, 2021

peternewman left a comment

Choose a reason for hiding this comment

matkoniecz commented Oct 12, 2021

matkoniecz commented Oct 12, 2021 • edited Loading

peternewman left a comment

Choose a reason for hiding this comment

peternewman Oct 12, 2021

Choose a reason for hiding this comment

westnordost Oct 13, 2021

Choose a reason for hiding this comment

peternewman commented Oct 12, 2021

westnordost commented Oct 13, 2021

matkoniecz commented Oct 13, 2021

westnordost left a comment • edited Loading

Choose a reason for hiding this comment

westnordost Oct 13, 2021

Choose a reason for hiding this comment

westnordost Oct 14, 2021

Choose a reason for hiding this comment

peternewman left a comment

Choose a reason for hiding this comment

matkoniecz commented Oct 14, 2021

westnordost commented Oct 15, 2021

matkoniecz commented Oct 15, 2021

westnordost commented Oct 15, 2021 • edited Loading

westnordost commented Oct 15, 2021

peternewman commented Oct 15, 2021

peternewman commented Oct 15, 2021

matkoniecz commented Oct 12, 2021 •

edited

Loading

westnordost left a comment •

edited

Loading

westnordost commented Oct 15, 2021 •

edited

Loading