Skip to content

Conversation

@agent-redd
Copy link
Contributor

@agent-redd agent-redd commented Jan 31, 2025

Hello everybody,
this is my first Pull Request, please give me Feedback if something is wrong.

I created an Issue #6113 earlier to discuss the changes. I am still waiting for more feedback on it.
This Pull Request should resolve the problem, that the precondition toilets=yes for the BabyChangingTable Quest could not be fulfilled by StreetComplete itself.

Edit: see comments, the filter was now changed several times

Fixes #6113

Summarized my new proposal is:

    ask for all restaurants and toilets
    ask for fuel stations or shops which have toilets
    ask for cafés and fast_food which have toilets or seating
Thanks for the Feedback. Remove Comments in the element filter, as the syntax does not allow comment.s Also, some parentheses are nowavoided because and has higher precedence than or. And the last condition is simplified by using a regex in the key.

Co-authored-by: Flo Edelmann <git@flo-edelmann.de>
Copy link
Member

@mnalis mnalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few points (some mentioned in the issue but this implementation is slightly different):

  • amenity=cafe is defined as "generally informal place with sit-down facilities selling beverages and light meals and/or snacks", so there should be not much point in checking if it has seating places (i.e. it would mostly be asked on all cafes; so it might be clearer if it joined restaurant as the amenity where quest is unconditionally asked on, as the result would be mostly the same).
  • the PR would of course ask the quest on much more places (which is kind of whole point I guess). Whether that would still fullfill quest guidelines ("💤 No spam" and "🕵️ Publicly accessible on foot") might be a consideration.

(also I've edited original text of this PR by adding "magic text" so issue would close automatically if this PR is merged)

@mnalis
Copy link
Member

mnalis commented Feb 1, 2025

Warning: This code is not yet linted or tested, yet! (I currently do not have an Android Studio setup yet.)

On that subject - if you prefer, you can test if your PR compiles (and how well does resulting .apk work) by few simple clicks in GitHub actions web interface of your fork (without installing Android Studio) - see https://github.com/streetcomplete/StreetComplete/blob/master/CONTRIBUTING_A_NEW_QUEST.md#alternative-to-android-studio for quick instructions.

Such debug .apks can be installed alongside your regular StreetComplete, without affecting it.

agent-redd and others added 3 commits February 1, 2025 12:39
…Table.kt

Co-authored-by: Matija Nalis <mnalis-github@voyager.hr>
amenity=cafe is defined as "generally informal place with sit-down facilities selling beverages and light meals and/or snacks", so there should be not much point in checking if it has seating places
@agent-redd
Copy link
Contributor Author

* `amenity=cafe` is defined as _"generally informal place with sit-down facilities selling beverages and light meals and/or snacks"_, so there should be not much point in checking if it has seating places (i.e. it would mostly be asked on all cafes; so it might be clearer if it joined `restaurant`  as the amenity where quest is unconditionally asked on, as the result would be mostly the same).

Thanks. I moved the cafe into first line without preconditions to join the restaurants.

* the PR would of course ask the quest on much more places (which is kind of whole point I guess). Whether that would still fullfill [quest guidelines](https://github.com/streetcomplete/StreetComplete/blob/master/QUEST_GUIDELINES.md) (_"💤 No spam"_ and _"🕵️ Publicly accessible on foot"_) might be a consideration.

Thanks for that feedback. I reverted the shops to mall and department_store to be less spammy. My biggest intention is to ask for restaruants (also fast_food) and cafés.

Regarding the "🕵️ Publicly accessible on foot". This quest will probably never fulfill this requirement and that is the reason why this quest is deactivated by default. Perhaps there needs to be bigger discussion how indoor survey quests are handled in StreetComplete. This would also concern other deactivated quests like wheelchair_access, toilets_fee, air_conditioning, internet_access, accepts_card and accepts_cash.

@agent-redd
Copy link
Contributor Author

agent-redd commented Feb 1, 2025

Thanks to @mnalis for the hint of Github Actions to build a debug APK.

I built this change with Github Actions to test the changes:
GitHub Run: https://github.com/agent-redd/StreetComplete/actions/runs/13088445391
Debug APK: https://github.com/agent-redd/StreetComplete/actions/runs/13088445391/artifacts/2521143573

I updated also the initial comment of this Pull Request to match the changes

EDIT: wrong links! Do not use theses

@mnalis
Copy link
Member

mnalis commented Feb 1, 2025

Regarding the "🕵️ Publicly accessible on foot". This quest will probably never fulfill this requirement and that is the reason why this quest is deactivated by default.

Yeah, the guidelines have some wiggle room; and for deactivated-by-default quests it is often because one has to enter the place. As you note, this one is already deactivated by default, so that looks fine to me.

https://github.com/agent-redd/StreetComplete/actions/runs/13088445391

So, have you installed that debug .apk and tried how it works? Does the selection of the quests work as you expected? Does it look too spammy or still too restricted to you? Does it select strange things, or fails to select things you think it should select?

Because unless I'm wrong that compiled 8e6fea3 is just vanilla StreetComplete master, and not a branch with your changes (patch-1). I.e. you've probably missed this part of the instructions:

make sure you select the correct branch with your changes when clicking Run workflow,

@agent-redd
Copy link
Contributor Author

So, have you installed that debug .apk and tried how it works? Does the selection of the quests work as you expected? Does it look too spammy or still too restricted to you? Does it select strange things, or fails to select things you think it should select?

Because unless I'm wrong that compiled 8e6fea3 is just vanilla StreetComplete master, and not a branch with your changes (patch-1). I.e. you've probably missed this part of the instructions:

make sure you select the correct branch with your changes when clicking Run workflow,

thanks for pointing that out. I ran the workflow from branch patch-1, but then the workflow ran again from master unintendenly.

Here is the correct link from first workflow from branch patch-1:

https://github.com/agent-redd/StreetComplete/actions/runs/13088443571

I will now download again and test it thoroughly.

@agent-redd
Copy link
Contributor Author

So unfortunately with the build from my branch the download of data fails. I assume a problem in the filter syntax:

Screenshot_2025-02-01-16-22-33-846_de westnordost streetcomplete debug

@agent-redd
Copy link
Contributor Author

agent-redd commented Feb 1, 2025

I dont understand the ~toilets yet in line 18:

or amenity = fast_food and ~toilets|indoor_seating|outdoor_seating = yes

Could this be a syntax error?

@FloEdelmann
Copy link
Member

–toilets|indoor_seating|outdoor_seating ~ yes would mean any tag whose key matches the regex toilets|indoor_seating|outdoor_seating and whose value matches the regex yes (see the cheatsheet).

But putting an = after a regex key might actually be a syntax error, at least it is not documented. So you might have to change:

- –toilets|indoor_seating|outdoor_seating = yes
+ –toilets|indoor_seating|outdoor_seating ~ yes

@mnalis
Copy link
Member

mnalis commented Feb 1, 2025

But putting an = after a regex key might actually be a syntax error

Probably. Or just expand it, as not much is gained by that key regex and simple value... e.g.

- or amenity = fast_food and ~toilets|indoor_seating|outdoor_seating = yes
+ or amenity = fast_food and (toilets = yes or indoor_seating = yes or outdoor_seating = yes)

I will now download again and test it thoroughly.

If you click to report crash by composing mail, it should open in your mail client (well at least it does in my K-9) so you can preview it (and abort sending).
Also, when things don't work, sometimes looking at About / Show logs might be helpful (not always though).

@agent-redd
Copy link
Contributor Author

Also, when things don't work, sometimes looking at About / Show logs might be helpful (not always though).

Indeed, it shows some hintful message:

2025-02-01T16:25:34.172: E [Download] Unable to download de.westnordost.streetcomplete.data.elementfilter.ParseException: At position 237: Unexpected operator '=': The key prefix operator '~' must be used together with the binary operator '~' or '!~'

@agent-redd
Copy link
Contributor Author

agent-redd commented Feb 1, 2025

But putting an = after a regex key might actually be a syntax error

Probably. Or just expand it, as not much is gained by that key regex and simple value... e.g.

- or amenity = fast_food and ~toilets|indoor_seating|outdoor_seating = yes
+ or amenity = fast_food and (toilets = yes or indoor_seating = yes or outdoor_seating = yes)

Thanks. I took over your suggestion in a testing branch, rebuilt a dev version and testet my use cases before i merged it back into this branch:

  1. all restaurants and cafes are now asked
  2. fast_food is not asked until I activate the seating quest and add indoor/outdoor/both seating

I assume this Pull Request is now technically ready to merge.

Co-authored-by: Matija Nalis <mnalis-github@voyager.hr>
@agent-redd
Copy link
Contributor Author

agent-redd commented Feb 2, 2025

I applied the proposal from @mnalis to revert the unintended change regarding fuel stations.

I then rebuilt a dev version with Github Actions: https://github.com/agent-redd/StreetComplete/actions/runs/13097378940

And then tested that its only asking for fuel stations which have toilets=yes. And additinally tested that its asking in all cafes ans restarants, and for fast-food only if toilets or seating is tagged.

Thanks everybody for the helpful feedback! I hope that this Pull Request is now ready to merge.

@westnordost
Copy link
Member

westnordost commented Feb 2, 2025

Without having followed the massive discussion here, I think @agent-redd 's proposal makes sense. The toilets quest is not asked for every restaurant etc. because the assumption is that many/most do have toilets, so asking for it specifically would be spammy. The baby changing table quest is already disabled by default (because the mapper would need to go inside and check/ask), I don't think it is necessary to require toilets=yes being set, as that is what can already be assumed.

Regarding the PR in its current state:

  1. What if if specifically toilets=no is set? Should it still be asked? I'd say that "no"?
  2. The general assumption that there is a toilet is I'd say even more true for malls and department stores, so IMO it's fine to ask for baby changing tables always, like for restaurant etc.
  3. TBH I tend towards also asking for fast food.

I understand why in the current state of the PR, 2 and 3 were made the way they are now: Because the toilets quests is asked for malls and department stores plus there's also a (deactivated by default) seating quest. But IMO that's not necessary. It is enough to check that they are not tagged like toilets=no (, indoor_seating=no, ...)

(We don't want to coerce users who are interested in mapping baby changing tables to also enable and answer the toilets and seating quests - without telling them, to boot. These quests should just be sorted further up in the quest hierarchy, so when these are activated, they are asked first.)

@agent-redd
Copy link
Contributor Author

agent-redd commented Feb 2, 2025

Thank you @westnordost for your helpful feedback. Your suggestion to always ask unless toilets=no would simplify the filter and be more intuitiv. The seating precondition could be eliminated again.

I will rework and test my Pull Request and give an update then.

Edit: fix typos

@agent-redd
Copy link
Contributor Author

agent-redd commented Feb 2, 2025

I now simplified the Pull Request to the logic as @westnordost suggested. Built a dev apk with Github Actions and tested it against some amenity=fast_food without changing_table=* POIs

A) toilets=no: https://www.openstreetmap.org/node/438049614
==> Quest does not appear
B) toilets=yes:https://www.openstreetmap.org/node/2866502289
==> Quest appears
C) without toilets=*: https://www.openstreetmap.org/node/12165822426
==> Quest appears

Here is the dev build if anybody wants to do further tests:
https://github.com/agent-redd/StreetComplete/actions/runs/13102320869

I guess if this Pull Request gets merged the commit history should be squashed to hide the back and forth. Not sure if Github can do this, or if I should prepare this by creating a new cleaner branch?

Edit: wording

@westnordost westnordost merged commit e377a41 into streetcomplete:master Feb 3, 2025
@agent-redd agent-redd deleted the patch-1 branch February 10, 2025 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants