Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Camping site quests #4213

Merged
merged 35 commits into from
Aug 17, 2022
Merged

Camping site quests #4213

merged 35 commits into from
Aug 17, 2022

Conversation

mnalis
Copy link
Member

@mnalis mnalis commented Jul 14, 2022

This implements some (but not all) of requested quests for tourism=camp_site, thus it mostly fixes #4198:

  • drinking_water=* yes/no quest
  • power_supply=* yes/no quest
  • shower=* yes/no quest
  • 3-answer quest Who may camp here? ( Tents only / Caravan only / Both) for setting tents=* and caravans=* (with OtherAnswer available to set backcountry=yes)
  • create non-placeholder icons

Help needed with icons: This PR reuses icon for ic_quest_drinking_water (which might be OK as it is, or it might benefit from being separate icon, e.g. if all icons have tent+some_image), but has only placeholder icons for ic_quest_camp_power_supply, ic_quest_camp_shower and ic_quest_camp_type. Artistically inclined people are invited to fix those!

@matkoniecz
Copy link
Member

It is in the draft state - do you expect review of what is prepared?

Or is it request for help with some part (which one)?

@mnalis

This comment was marked as resolved.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@mnalis mnalis mentioned this pull request Jul 15, 2022
5 tasks
@mnalis
Copy link
Member Author

mnalis commented Jul 15, 2022

Code is tested and looks to work OK to me, so ready for review.
It needs replacing placeholder icons however, which needs someone artistically inclined (as noted in updated PR summary).

@mnalis mnalis marked this pull request as ready for review July 15, 2022 23:32
@mnalis mnalis requested a review from matkoniecz July 16, 2022 20:20
@mnalis

This comment was marked as outdated.

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Looks good :)
I have some quest filter formatting suggestions (to be consistent with existing quest filters) that I'll apply directly; please feel free to overwrite them again if they're not perfect.

I'm not approving, since real icons are still missing.

Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Some improvements/suggestions and comments.

mnalis and others added 3 commits July 31, 2022 11:34
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
Co-authored-by: Mateusz Konieczny <matkoniecz@gmail.com>
@westnordost
Copy link
Member

westnordost commented Aug 9, 2022

(Just ping me if this is ready for final review. I haven't been following this PR. As usual, icons are not required, I'll make them if they are missing.)

Copy link
Member

@matkoniecz matkoniecz left a comment

Choose a reason for hiding this comment

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

tested in emulator, I have this suggestion mentioned above - but not 100% sure is it useful

recently I was talking with friends and it turns out that in some regions even "is this camping site paid" maybe would be a viable quest

@mnalis
Copy link
Member Author

mnalis commented Aug 9, 2022

(Just ping me if this is ready for final review. I haven't been following this PR. As usual, icons are not required, I'll make them if they are missing.)

Thanks @westnordost, I've tried playing with icons but got stuck at transparency/fill issues, so I'd appreciate if you can take care of icons when you do final review!

Other than that, I think it's as finished as I could make it.
Two more things need decision, though:

  • Is "caravan" good enough US translation in order not to be confused with static caravan? Or should it be changed or some string translation hints be added (and where?) - see Camping site quests #4213 (comment)
  • should the camp_type quest be enabled or disabled by default? There have been arguments for both, so it's a matter of commenting or uncommenting the line - see Camping site quests #4213 (review)

@mnalis
Copy link
Member Author

mnalis commented Aug 9, 2022

recently I was talking with friends and it turns out that in some regions even "is this camping site paid" maybe would be a viable quest

In my limited experience of near-to-Croatia parts of Europe it does seem spammy (see #4198 (comment)), but I guess it might differ worldwide. If camp fee quest would be useful in some countries (and backcountry answer as currently implemented is insufficient), I think another issue/PR should be opened, as this one if overstuffed already (probably would need some research where it would not be spammy).

@peternewman
Copy link
Collaborator

(and backcountry answer as currently implemented is insufficient), I think another issue/PR should be opened, as this one if overstuffed already

I strongly feel this PR shouldn't be merged with the backcountry answer in its current state; leaving a note when required occasionally would be better than the current other answer which can result in incorrect tagging. Looking at taginfo, more than a quarter of backcountry=yes are also tagged with tents=yes, so setting the opposite for one when we tag the other seems crazy to me:
https://taginfo.openstreetmap.org/tags/backcountry=yes#combinations

@mnalis
Copy link
Member Author

mnalis commented Aug 10, 2022

I strongly feel this PR shouldn't be merged with the backcountry answer in its current state; leaving a note when required occasionally would be better than the current other answer which can result in incorrect tagging. Looking at taginfo, more than a quarter of backcountry=yes are also tagged with tents=yes, so setting the opposite for one when we tag the other seems crazy to me: https://taginfo.openstreetmap.org/tags/backcountry=yes#combinations

@peternewman I'm not sure of which incorrect tagging are you talking about, can you provide an example of what would be marked incorrectly in which case?
If user chooses Backcountry camping (no facilities) answer, it will be marked only with backcountry=yes tag (and no other quests about facilities will be asked).

I understand that it might be argued that some additional tags might be asked for backcountry camps too; but surely there is no incorrect tagging if tourism=camp_site gets tagged only with additional backcountry=yes?

Or did I misunderstand your point? You may try this debug .apk if you wish to quickly check what the quest actually tags on which answer and/or wish to provide a screenshot of the problem.

Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Some suggestions to clarify the backcountry behaviour

@peternewman
Copy link
Collaborator

@peternewman I'm not sure of which incorrect tagging are you talking about, can you provide an example of what would be marked incorrectly in which case? If user chooses Backcountry camping (no facilities) answer, it will be marked only with backcountry=yes tag

Apologies, I'd got caught out by your BACKCOUNTRY(false, false),, not checked the other code and assumed it was going to tag tents=no too. I've added some code suggestions for how to improve the clarity of that a lot (IMHO), including the all-important comment, and a fairly simple extension for how to potentially make it even better and remove the gotcha false values entirely.

(and no other quests about facilities will be asked).

This bit is currently untrue looking at the quest filters, you'll still be asked about water, showers and power even if its tagged as backcountry (where they are less likely and its possibly spammy).

mnalis and others added 3 commits August 10, 2022 23:48
…/CampType.kt

Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
…/AddCampType.kt


cleaner BACKCOUNTRY matching

Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
…/AddCampType.kt

Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
@mnalis
Copy link
Member Author

mnalis commented Aug 10, 2022

Apologies, I'd got caught out by your BACKCOUNTRY(false, false),, not checked the other code and assumed it was going to tag tents=no too. I've added some code suggestions for how to improve the clarity of that a lot (IMHO), including the all-important comment, and a fairly simple extension for how to potentially make it even better and remove the gotcha false values entirely.

Thanks, I've added you code-readability improvements!

This bit is currently untrue looking at the quest filters, you'll still be asked about water, showers and power even if its tagged as backcountry (where they are less likely and its possibly spammy).

Ah yes. I've had a mind about disabling quests for them, but after more careful reading of https://wiki.openstreetmap.org/wiki/Key:backcountry I've decided to still ask for them, as it seems at least some of them are still possible (just less likely, i.e. assumed no by default). And as backcountry=yes is only a small subset of camps (according to taginfo) and camps are relatively rare themselves, I do not think continuing to ask for them would be too spammy. But if there is consensus to disable some of them for backcountry=yes, I'm not against.

@westnordost westnordost self-assigned this Aug 17, 2022
Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Nice, clean implementation. I have nothing else to add other than the tweaks on the wordings. I will do the icons after merging.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@westnordost westnordost merged commit 3f8e65e into streetcomplete:master Aug 17, 2022
@matkoniecz
Copy link
Member

From not-a-native-speaker: I would have no idea what "RV" is, even with context.

@westnordost
Copy link
Member

Okay

@riQQ
Copy link
Collaborator

riQQ commented Aug 17, 2022

Also as a non-native speaker: RV is clear for me.

@westnordost
Copy link
Member

🤷 I changed it to "motorhomes, travel trailers, ..." or something

@mnalis mnalis deleted the camping-quests branch August 23, 2022 21:44
@mnalis mnalis added the new quest accepted new quest proposal (if marked as blocked, it may require upstream work first) label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new quest accepted new quest proposal (if marked as blocked, it may require upstream work first)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quests for Amenities at Campsites
6 participants