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

Add police type quest (Italy only) #2675

Merged
merged 18 commits into from Apr 22, 2021
Merged

Add police type quest (Italy only) #2675

merged 18 commits into from Apr 22, 2021

Conversation

naposm
Copy link
Contributor

@naposm naposm commented Mar 25, 2021

Good morning everyone!
I'm happy (and hope to) finally give a contribution to this amazing app!
I tried implementing the quest to add police operators in Italy (#2456).
It is my first time using Android Studio and I'm still a beginner with Git so I hope to not have messed up something. I apologize if I did!
I did a quick quest icon in Inkscape, but it's only for demo purposes, so if there's a better one you can absolutely change it!
I used #2583 as a template to understand how to move my first steps, so big thanks to @arrival-spring and contributors!

Thank you for your attention and have a nice day 🙂

P.S. I'm not adding closes #2456 because this PR only implements the quest for Italy

Added files for the quest PoliceType, useful for adding amenity:police operators in Italy
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.

Nice! I just did an initial review to save some time from @westnordost.

@naposm
Copy link
Contributor Author

naposm commented Mar 25, 2021

Thank you for taking time for reviewing, I'm fixing some of these right now 🙂

EDIT: I think to have fixed all of them

…type/PoliceType.kt

Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
@westnordost
Copy link
Member

Thanks for doing a review, @FloEdelmann ! Let me know when a second review is needed.

@@ -236,7 +237,8 @@ import javax.inject.Singleton
AddBenchStatusOnBusStop(),
AddBenchBackrest(),
AddTrafficSignalsButton(),
AddPostboxRoyalCypher()
AddPostboxRoyalCypher(),
AddPoliceType()
Copy link
Member

Choose a reason for hiding this comment

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

I'd move it a bit up in this list (which determines the quest priority), as the collected data seem more useful to me than those of some other quests. Maybe below AddBoardType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unsure of its importance, that's why I just put it there. But I trust you, I'll move it below AddBoardType 😉

res/graphics/police type graphics/carabinieri.svg Outdated Show resolved Hide resolved
@@ -412,7 +412,8 @@ object AchievementsModule {
"AddFerryAccessMotorVehicle", // 103
"AddInformationToTourism", // 137
"AddBoardType", // 188
"AddPostBoxRoyalCypher"
"AddPostBoxRoyalCypher",
"AddPoliceType"
Copy link
Member

Choose a reason for hiding this comment

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

Is it really rare? How many police stations without operator are there in Italy? Maybe the Overpass wizard @matkoniecz can help here 😉

I guess the same question can also be asked for AddPostBoxRoyalCypher, though.

Copy link
Contributor Author

@naposm naposm Mar 25, 2021

Choose a reason for hiding this comment

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

Yeah, very! In my area (Southern Italy) I think I'm the only one who adds operators to police (because usually it's only added in the name, a lot of new mappers don't know of this rule). But, if we could have a official response that would be fantastic!

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

@naposm's Overpass query from #2675 (comment) yields around 2800 quests in the whole country of Italy.

@westnordost Should this count towards the "rare" achievement?

AddPostBoxRoyalCyper is even worse though 😅
6890 quests in the London region alone: https://overpass-turbo.eu/s/15p9

grafik

Copy link
Contributor Author

@naposm naposm Mar 25, 2021

Choose a reason for hiding this comment

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

Mh, maybe it's not as rare as I thought 😢 That was just my plan to get the achievement faster 😈
Just kidding, if everyone agrees I can remove also AddPostBoxRoyalCyper. For now I will remove mine from the achievement then. I just did the same query in my area and it's quite popular there too, I'm quite surprised, indeed the only ones that appear to lack the operator tag are the ones in rural areas or in areas where people do not live (near shopping centers and wide streets - perfect for StreetComplete then since it's used mostly while walking).

Copy link
Contributor Author

@naposm naposm Mar 25, 2021

Choose a reason for hiding this comment

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

I don't know if it's me who hid your comment, I apologize if I did, that was not my intention!

Copy link
Member

Choose a reason for hiding this comment

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

No, it was me; since we have more accurate numbers now, I've hidden my comment as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, perfect!

class AddPoliceType : OsmFilterQuestType<PoliceType>() {

override val elementFilter = """
nodes, ways, relations with
Copy link
Member

Choose a reason for hiding this comment

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

Probably it's only nodes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ways and realations are there because I run a Overpass query* and found out that there are also a lot of areas and relations with amenity=police and no operator

* The query is very slow

Copy link
Member

Choose a reason for hiding this comment

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

Indeed: 1434 nodes, 1366 ways, 61 relations. Probably relations can be dropped then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will the amenities within those relations be mapped as well?

Copy link

@opk12 opk12 Mar 26, 2021

Choose a reason for hiding this comment

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

Data consumers expect relations not to have amenity=police, because the OSM wiki says "should not be used on relations" in the infobox at the right (the icon's mouse-hover tooltip text). The Italian amenity=police article also has the same provision. As a matter of fact, relations are only a thousand worldwide, which is usually not deemed sufficient to implement support.

Relations should be marked with a note (most likely, they will be re-tagged as nodes and/or areas, but this needs to be handled case by case, as always) or a discussion on lifting the wiki prescription (and on relation-specific conventions or tags to use in combination) should be posed in the worldwide or Italian tagging mailing lists

Copy link
Member

Choose a reason for hiding this comment

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

good catch

Data consumers expect relations not to have amenity=police, because the OSM wiki says "should not be used on relations" in the infobox at the right (the icon's mouse-hover tooltip text)

note that it excludes multipolygons, this is listed as areas in infobox, see https://wiki.openstreetmap.org/wiki/Template:Description#Parameters

I opened https://wiki.openstreetmap.org/wiki/Template_talk:Description#Misleading_tooltip_.28alt_text.29_for_relation

Copy link
Contributor Author

@naposm naposm Mar 26, 2021

Choose a reason for hiding this comment

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

Perfect, thanks for the information! I'll remove relations then! 🤗
I'll also discuss with the ML about editing relations with amenity=police 😁

Maybe it's not as rare as it seems :(
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.

This looks fine from my side now.

@westnordost Can you please have a look? Please also comment on the three remaining conversations:

override fun createForm() = AddPoliceTypeForm()

override fun applyAnswerTo(answer: PoliceType, changes: StringMapChangesBuilder) {
changes.add("name", answer.policeName);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about adding names? I am unfamiliar with Italy tagging, but in Poland police stations are often with own names such as name=Komisariat IV. See https://www.openstreetmap.org/way/201286959 https://www.openstreetmap.org/way/374532370 https://www.openstreetmap.org/way/167008548 that may be similar cases in Italy.

And additionally note that quest filter is not excluding things with name tag or operator:wikidata or operator:wikipedia

See say https://www.openstreetmap.org/way/24240779 - what will happen if user will solve quest in place where either of this tags is present already?

Copy link
Contributor Author

@naposm naposm Mar 26, 2021

Choose a reason for hiding this comment

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

Are you sure about adding names? I am unfamiliar with Italy tagging, but in Poland police stations are often with own names such as name=Komisariat IV. See https://www.openstreetmap.org/way/201286959 https://www.openstreetmap.org/way/374532370 https://www.openstreetmap.org/way/167008548 that may be similar cases in Italy.

I added the names only to avoid confusion between Polizia Stradale (road patrols), Polizia Ferroviaria (for trains) and Polizia di Stato (the main branch) which all have operator=Polizia di Stato. I agree, indeed I'd have never added them as me myself wouldn't add names with amenity=police. I've just asked on the Italian ML for this to see how other mappers add these. Anyway I think that the name tag doesn't get overridden with add() does it?

And additionally note that quest filter is not excluding things with name tag or operator:wikidata or operator:wikipedia

Yes, that was one thing I was thinking about, I didn't filter it because I want to add operator anyway (which I think is more important), the only reason there are these two tags is if it is a very important police station operated exclusively by a separated entity only for that station.
So I wouldn't like to override the tags but I'd still like to add the operator one, how do you think we can solve that? 🤔
I'd also love to make a query on Overpass to see how many cases are there and if it's worth the effort...

Copy link
Member

Choose a reason for hiding this comment

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

You asked on the Italian mailing list, so I'd say you should proceed whatever was the conclusion of that.
Note that it is also possible to only add the name if it does not exist yet:
if (changes.getPreviousValue(...) == null) changes.add(...)

@@ -161,6 +161,15 @@ parking lane graphics/
car4.svg
car5.svg

police type graphics/
carabinieri.svg modified from https://commons.wikimedia.org/wiki/File:Italian_traffic_signs_-_icona_carabinieri.svg, Gigillo83, License: CC-BY-SA
Copy link
Member

Choose a reason for hiding this comment

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

I would specify also that it is 3.0 version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I forgot to add it! 😅 Fixing this rn

@matkoniecz
Copy link
Member

Congratulations, looks nicely!

Have you tried solving some quests (without logging in) and looking at logs to see outcomes? With my own quests I often caught bugs this way.

Just quick check: is tagging used here a preferred or standard one used by Italian community? (StreetComplete is not supposed to introduce new tagging schemes on its own)

}

enum class PoliceOperator(val operatorName: String, val wikidata: String, val wikipedia: String) {
ARMA_DEI_CARABINIERI("Arma dei Carabinieri", "Q54852", "en:Carabinieri"),
Copy link

Choose a reason for hiding this comment

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

Hello. Is English Wikipedia linked for a specific reason? It has differences with respect to the Italian article, but I did not read through them, and cannot say which article is more comprehensive or "better".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!
I don't know why, but the ID presets (https://nsi.guide) uses the English Wikipedia, I'd love to change it to the Italian one as it is the one I use for the other operators, but I used the one I think is used more (since lots of people map with ID). Should I use the Italian Wikipedia? From a first impression I find the Italian one more complete. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

In Poland norm is to use Polish Wikipedia if possible.

Note that iD presets are not something free from mistakes - if Italian community prefers Italian links I can change it (I have commit rights to NSI repo, so I can edit directly and I will be able to find it).

And Italian police will be present in Italy, so it is not necessary to consult more widely.

Copy link

Choose a reason for hiding this comment

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

Yes, it would be much appreciated, thank you. It seems a cleanup commit from Dec 2020 replaced it:Arma dei Carabinieri with en:Carabinieri.

Choose a reason for hiding this comment

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

By all means, use the Italian one. For some reason I changed it when I was doing osmlab/name-suggestion-index#4752, and the Italian one is most certainly better. (Maybe when I was looking up Wikidata? I don't remember.) Sorry about that.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that only https://github.com/osmlab/name-suggestion-index/blob/main/data/operators/amenity/police.json#L21 needs change to it:

Is there any other change required? Such as adding Wikipedia linking where it is missing in NSI?

Choose a reason for hiding this comment

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

@matkoniecz The Italian page name is slightly different. Need to change to it:Arma dei Carabinieri to get to here. I thought (perhaps mistaken) that it didn't matter what Wikipedia language you use in the tag. Whatever consumer of the data could use the sister pages to show you the one in your language, if available. (That doesn't mean that consumers of the data do do that.)

Please feel free to add missing tags to NSI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will after this one will be merged, @dieterdreist suggested me in the list to remove the Wikipedia link and just use Wikidata, so I'll do a PR with all those changes, I'll also remove Corpo Forestale dello Stato which doesn't exist anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, so: delete wikipedia link, keep wikidata link.

@naposm naposm marked this pull request as draft March 26, 2021 12:17
@naposm
Copy link
Contributor Author

naposm commented Mar 26, 2021

I'm converting this pull request as draft waiting for responses on the Italian mailing list

@naposm
Copy link
Contributor Author

naposm commented Mar 26, 2021

Congratulations, looks nicely!

Have you tried solving some quests (without logging in) and looking at logs to see outcomes? With my own quests I often caught bugs this way.

Just quick check: is tagging used here a preferred or standard one used by Italian community? (StreetComplete is not supposed to introduce new tagging schemes on its own)

Thanks! And thanks for reviewing! 🤗
I did not look at the logs, I don't know how that works, but I used the Quest demo in the Settings and tried to go in places where the tags were needed to try, and the quest seems to work.
I used the scheme from the Italian wiki, but edited and created some (Polizia Ferroviaria and Polizia Stradale). The operators tag were added by looking at https://nsi.guide.

@matkoniecz
Copy link
Member

I used the scheme from the Italian wiki, but edited and created some (Polizia Ferroviaria and Polizia Stradale). The operators tag were added by looking at https://nsi.guide.

It would be good idea to confirm that adding it at a large scale is welcome - also, how name tag should be handled.

@naposm naposm marked this pull request as ready for review April 21, 2021 15:48
@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

Thank you very much @westnordost!
I'm just trying to fix all these problems. I'll commit soon 🙂

@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

P.S. Love the new icon, colors are much better!

@westnordost
Copy link
Member

Thanks! Mainly I just wanted to change the background color (blue = pedestrian, green ~= amenity) but I also noticed a problem with the shadow, the icon not being in the center and a little too dark police cap

@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

Thanks! Mainly I just wanted to change the background color (blue = pedestrian, green ~= amenity) but I also noticed a problem with the shadow, the icon not being in the center and a little too dark police cap

Yeah, I thought that blue was for pedestrian quests, the ones useful for pedestrians, that's why I (wrongly) used blue but it was a punch in the face 😆
Yours has those little details which make it better, glad you took a look and changed it!
Anyway, I just committed, let me know if there's any problem, Android Studio does not let me run the emulator so I had no chance to try it, I'll try to fix this problem, but I think it works as it did not change that much since my last test 🤔

@matkoniecz
Copy link
Member

matkoniecz commented Apr 21, 2021

Android Studio does not let me run the emulator

You can attach physical phone via USB and run app on it (I did it with my older computer that was incapable of running emulator).

It will require some setup (developer permission on phone may be needed etc)

@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

Android Studio does not let me run the emulator

You can attach physical phone via USB and run app on it (I did it with my older computer that was incapable of running emulator).

It will require some setup (developer permission on phone may be needed etc)

Yeah, I did try it last time, but I think my current USB cable does not support data transfer (I bought it when my previous USB cable broke last week at a cheap price in my local tech store, so that was expected 🙄). Since I also need to copy some photos, I'll buy one ASAP, but I'll need to go to a better shop due to Covid I'm a kinda blocked here at the moment 🤦‍‍♂️
Maybe for my next quest I'll just use this method and remove emulator completely if it keeps crashing...

@westnordost
Copy link
Member

By the way, my emulator also crashes all the time: I have to select "clean boot" every time.

@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

By the way, my emulator also crashes all the time: I have to select "clean boot" every time.

Mh, I will try doing this later and see if that's the problem, thanks!

@westnordost
Copy link
Member

Code looks good to me! You didn't test it though yet, did you?

@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

I did test both on the emulator and on my phone last time and it did work (tried using position spoofing to go in places where the quest should have appeared and it correctly did, also tried in debug mode in the app and the data was correctly sent, also the form was correct). I'll try cleaning the emulator and try on it since I can't use my phone. If it keeps crashing, could someone test it for me? I'd be very glad for it! Will let you know in a moment.

@westnordost
Copy link
Member

sure

@naposm
Copy link
Contributor Author

naposm commented Apr 21, 2021

After uninstalling and reinstalling Android Studio (which in Ubuntu is a pain to do apparently without getting errors 🤦‍♂️) I managed to make it work!
Here are a few screenshots of the quest in all its majesty:

CLICK
Dark theme:
CLICK

@dieterdreist
Copy link

dieterdreist commented Apr 21, 2021 via email

@westnordost
Copy link
Member

Ok, then this can be merged, right?

@dieterdreist
Copy link

dieterdreist commented Apr 22, 2021 via email

@westnordost westnordost merged commit 7bafe33 into streetcomplete:master Apr 22, 2021
@westnordost
Copy link
Member

Thanks for the great work, everyone! Merged now

@naposm
Copy link
Contributor Author

naposm commented Apr 22, 2021

Exactly, as @dieterdreist said that's the problem here in Italy, we have a lot of different police types, but most of them are operated by Polizia di Stato, indeed if @westnordost agrees I'd like to make another quest which asks for the specific type then, but I'd need to first propose a tag, as I said in the list I'd like to propose something like police:IT=* but it would first require a list, which I'm planning to do.
If I manage to do that I'll update this quest, but I think it will require some time and these tags will still be valid since they are the ones in the wiki (https://wiki.openstreetmap.org/wiki/IT:Tag:amenity%3Dpolice)

@naposm
Copy link
Contributor Author

naposm commented Apr 22, 2021

Thank you @westnordost and thank you very much to everyone for your help!

@dieterdreist
Copy link

dieterdreist commented Apr 22, 2021 via email

@naposm
Copy link
Contributor Author

naposm commented Apr 22, 2021

sent from a phone
On 22 Apr 2021, at 15:33, naposm @.*> wrote: police:IT=
I am aware this is probably not the best place for such a discussion, but my preference would be for something like police=IT

If you agree we could open another discussion in the list, this time only on this tag specifically and discuss with the community about how we could do it, I don't know if this is suitable for the tagging list too

@dieterdreist
Copy link

dieterdreist commented Apr 22, 2021 via email

@matkoniecz
Copy link
Member

@naposm

I have some questions about images that you used here and in some your other contributions.

https://github.com/streetcomplete/StreetComplete/commits/master/res/graphics/quest/police.svg 3717423
screen02

Is it your own work or have you used/based it on existing one?

the same for costiera logo:

screen03

do you remember on what you based or what you imported/copied?

Sorry for asking with such delay.

Also, the same for dbc19c8#diff-e35ba62cf1099fc5ce3af93f2eabf9bf3eacfdcedad35cb6ed7a69bff028e65a

screen04

@naposm
Copy link
Contributor Author

naposm commented Aug 18, 2022

Hi @matkoniecz I apologize for replying after so much time.

The quest icon was made by me, maybe I modified another icon, I can't remember, but the hat was made entirely by me. Trying to remember, I think I also made the user shape since they are two circles with a border.

The Guardia Costiera is just the official logo on a white background. The image of the logo was taken from Wikimedia Commons:
https://commons.wikimedia.org/wiki/File:Stemma_della_Guardia_Costiera_italiana.png

The fuel service quest icon was made by me by merging multiple quest icons together.

I hope I have answered your questions! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants