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

Differenciate between roads and pedestrian streets #862

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

ENT8R
Copy link
Contributor

@ENT8R ENT8R commented Feb 12, 2018

While reviewing StreetComplete-related notes I saw many people irritated by the fact that StreetComplete can't differenciate between roads for cars and pedestrian streets.
This PR fixes this issue.

Some examples are:
https://www.openstreetmap.org/note/1296735
https://www.openstreetmap.org/note/1179245
https://www.openstreetmap.org/note/1174269
https://www.openstreetmap.org/note/1174268
https://www.openstreetmap.org/note/1174267
https://www.openstreetmap.org/note/1123039
https://www.openstreetmap.org/note/1290931
https://www.openstreetmap.org/note/1289562

@ENT8R
Copy link
Contributor Author

ENT8R commented Feb 12, 2018

I also added another answer for if the street is tagged with area=yes ("What is the name of this square?") as this was confusing to some users too...

@westnordost
Copy link
Member

That's a very good idea, nice!

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.

Ah wait, your last commit does not make sense. The quest explicitly excludes any areas.

@ENT8R
Copy link
Contributor Author

ENT8R commented Feb 12, 2018

The quest explicitly excludes any areas.

But what about this note: https://www.openstreetmap.org/note/1297034 which references https://www.openstreetmap.org/way/365260406 with area=yes...

@rugk
Copy link
Contributor

rugk commented Feb 12, 2018

and the last note is about the surface quest, not the name quest.

@rugk
Copy link
Contributor

rugk commented Feb 12, 2018

BTW, generally, just saw what I once wrote for that quest in the wiki as a question: "What is the name of this place?"
That can be asked for areas, streets or whatever… so that may be a way to make it more general.

@rugk
Copy link
Contributor

rugk commented Feb 12, 2018

Uh, wait, that's another quest… But if you are going into areas that is not really a road quest…

@matkoniecz
Copy link
Member

Maybe it was done using version not including 10f8beb ?

@ENT8R
Copy link
Contributor Author

ENT8R commented Feb 13, 2018

Oops... It was the surface quest... I will revert the last commit.

@ENT8R
Copy link
Contributor Author

ENT8R commented Feb 13, 2018

I removed the additional area question from the road name question but added it to the road surface question as it might make sense to include it there...

@@ -164,7 +164,10 @@ else if(noProperRoad == AddRoadNameForm.IS_LINK)
@Override public String getCommitMessage() { return "Determine road names and types"; }
@Override public int getIcon() { return R.drawable.ic_quest_street_name; }
@Override public int getTitle() { return R.string.quest_streetName_title; }
@Override public int getTitle(@NonNull Map<String,String> tags) { return getTitle(); }
@Override public int getTitle(@NonNull Map<String,String> tags) {
if (tags.get("highway").equals("pedestrian")) return R.string.quest_streetName_pedestrian_title;
Copy link
Member

Choose a reason for hiding this comment

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

possible nullpointer

Copy link
Member

Choose a reason for hiding this comment

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

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 thought that too...

Copy link
Member

Choose a reason for hiding this comment

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

Possibly, there won't be a problem with this now, but there may be in the future:

By contract (=by interface), it is only guaranteed that the tags parameter will never be null, it does not say anything about which tags must be present.

Note that the default implementation of getTitle in SimpleOverpassQuestType is:

@Override public final int getTitle()
{
	return getTitle(Collections.emptyMap());
}

So, asserting that every call to getTitle(@NonNull Map<String,String> tags) must carry the correct tags from the associated element with it, comes down to asserting that getTitle() will never be called on OsmQuestTypes. Ulteriorly "removing" methods from the interface this way is prone to error, as this is not obvious to anyone.

@westnordost
Copy link
Member

I'll just merge it and correct it afterwards.

@westnordost westnordost merged commit 6899ee1 into streetcomplete:master Feb 14, 2018
@ENT8R ENT8R deleted the pedestrian branch February 14, 2018 13: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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants