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

implement ičo and vatin quests #669

Open
wants to merge 6 commits into
base: modified
Choose a base branch
from

Conversation

vfosnar
Copy link

@vfosnar vfosnar commented Sep 29, 2024

help wanted: you can work on this if you're interested in finishing this PR, see #669 (comment)

I used place_name quest as a template, I'm not sure if the element selection is okay and if it shouldn't be refactored in some way

@vfosnar
Copy link
Author

vfosnar commented Sep 29, 2024

relevant discussion in StreetComplete repo streetcomplete#5933

@vfosnar vfosnar changed the title implement ičo and vatin quest implement ičo and vatin quests Sep 29, 2024
fullElementSelectionDialog(context, prefs, questPrefix(prefs) + PREF_ELEMENTS, R.string.quest_settings_element_selection, NAME_PLACES)
}

private val NAME_PLACES = mapOf(

This comment was marked as outdated.

Copy link
Collaborator

@mnalis mnalis Jan 22, 2025

Choose a reason for hiding this comment

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

I think it should be made separate list, curated for the features that are actually likely to have ref:vatin / ref:ico signed. I mean, it seems very unlikely to me that landuse=cemetery or military=barracks and military=training_area would each have their own ref:vatin, for example!

Better start with smaller list to avoid a lot of places where it less likely that VATIN would be signed... It can always be extended later...

So, if the lists are same for both ref:vatin and ref:ico, I'd create a new list (e.g. var VATIN_PLACES = mapOf(....), put it in separate file (in same directory as the quests) and the import it in both quests; to avoid duplication and ease updating of the list in the future (if needed).

@Helium314
Copy link
Owner

The quests should be limited to countries where they (reasonably) apply, i.e. CZ and SK for IČO, and EU and some more for vatin (here it also depends how easy it's to find, be it from outside or on receipts).

Further, same as for contact quests, I do not want to push (nearly) unused *:signed=no tags.

(I'm also not happy about very country-specific quests of low importance, as I fear this will invite other people wanting to add such quests... While a few are fine (we also have the SC postbox cypher quest), this has potential to evolve into a huge list, making quest selection hard to use.)

@mnalis
Copy link
Collaborator

mnalis commented Jan 22, 2025

The quests should be limited to countries where they (reasonably) apply, i.e. CZ and SK for IČO, and EU and some more for vatin (here it also depends how easy it's to find, be it from outside or on receipts).

That one is easy @vfosnar; just search for enabledInCountries = NoCountriesExcept and use https://ent8r.github.io/blacklistr/ if needed to get country codes.

Further, same as for contact quests, I do not want to push (nearly) unused *:signed=no tags.

Yeah, I tend to agree. Even if *:signed prefix is defined, we should try not to spam it with newly invented values; especially in quests where it is not critical for using the data (like here).
So, I'd guess, just remove option to add *:signed=no, and have users hide the quest for themselves where unanswerable...

(I'm also not happy about very country-specific quests of low importance, as I fear this will invite other people wanting to add such quests... While a few are fine (we also have the SC postbox cypher quest), this has potential to evolve into a huge list, making quest selection hard to use.)

Agreed. ref:vatin at least is little wider so should be fine anyway, and I think we can accept the other one too for now. If more of such localized quests requests arise, we can set more firm inclusion criteria then... So far it does not seem like too problematic to me...

Re: [This branch has conflicts that must be resolved]

Also I note conflicts in app/src/main/res/values-cs/strings.xml - that file should be removed, non-English translations are handled separately from PR...

@Helium314
Copy link
Owner

and I think we can accept the other one too for now

Certainly.
But at the same time if a bunch of people is trying to add such quests I smell the "but you allowed this other quest" style of arguing we see evey time in SC with the power pole material quest.

Also I note conflicts in app/src/main/res/values-cs/strings.xml - that file should be removed, non-English translations are handled separately from PR...

Yes, SC strings should ideally not be touched (only strings_ee).

@vfosnar
Copy link
Author

vfosnar commented Feb 20, 2025

Sorry for leaving this PR dead, school got in the way and I lost motivation to continue developing. Implementing this quest is not my priority rn even though it shouldn't be hard to finish it. I'll leave the PR to others who wish to finish it atl for now.

@vfosnar vfosnar closed this Feb 20, 2025
@Helium314
Copy link
Owner

I'll leave the PR to others who wish to finish it atl for now.

If you agree, I would open the PR again and add a "help wanted" label. This way it's more visible, and I'd close it in case someone is interested in working on this.

@vfosnar vfosnar reopened this Feb 22, 2025
@Helium314 Helium314 added the help wanted Extra attention is needed label Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants