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 "is construction finished" quests for highway=construction and building=construction #920

Merged
merged 3 commits into from
Apr 9, 2018

Conversation

matkoniecz
Copy link
Member

@matkoniecz matkoniecz commented Feb 26, 2018

fixes #685

I attempted to test quests quite thoroughly, but I would welcome somebody looking through code and/or trying to break it as it turned out to be more complex that I expected.

To handle:

:)

done:


unit tests TODOs:

@westnordost
Copy link
Member

Squash changes is something I can do when merging. I just added two icons for you.

@rugk
Copy link
Contributor

rugk commented Feb 26, 2018

consider railway=construction quest

If you use a common class, the maintenance "costs" should be low.
Also, I'd say, it is only less "popular", because there are more streets than railways in general. Apart from that, it should be quite the same as popular as the street version, relatively to the amount of streets/railways in general.

@matkoniecz
Copy link
Member Author

Thanks for icons, I will add them tomorrow. I think that I will add railway quest as a separate PR, it should be possible by adding new class without reworking general one (can you make icon for railway construction if you want it?).

@westnordost
Copy link
Member

So, is this ready for review?

@westnordost
Copy link
Member

Regarding railway, are you sure there is a considerate amount of these?

@matkoniecz
Copy link
Member Author

Yes, as this code goes only icon remains to be fixed.

I am not entirely happy about how complex it is, but it is result of handling complex tagging and I see no way to further simplify code (though I probably missed something to make code better, I am curious how it may be improved).

@matkoniecz
Copy link
Member Author

Regarding railway, are you sure there is a considerate amount of these?

From looking at taginfo - railway=construction is quite rare and in addition railway routing graph is far less important that road routing graph.

That is why I am not sure that railway version is worth adding.

@matkoniecz
Copy link
Member Author

@matkoniecz
Copy link
Member Author

matkoniecz commented Feb 27, 2018

PR was rebased and updated to include new icons (thanks for them!).

Code is ready to review, I run out of ideas how to improve it.

@rugk
Copy link
Contributor

rugk commented Feb 27, 2018

railway=construction is about 10k vs about 100k highway=construction

Well… that is obvious even without looking at OSM. Of course, there are more streets than railways, so what is the point?

@matkoniecz matkoniecz changed the title Add "is contruction finished" quests for highway=construction and building=construction Add "is contruction finished" quests for highway=construction and building=construction [waiting for review but not mergeable] Feb 28, 2018
@matkoniecz matkoniecz changed the title Add "is contruction finished" quests for highway=construction and building=construction [waiting for review but not mergeable] Add "is contruction finished" quests for highway=construction and building=construction [waiting for review, mergeable after squashing] Feb 28, 2018
@westnordost westnordost added this to the v5 milestone Mar 1, 2018
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.

I am impressed! This quest is pretty smart and only possible in this form with this complex overpass query you crafted. All in all, it is in a pretty good shape.

However, there are some open questions. See below.

@@ -27,6 +27,16 @@ public void delete(@NonNull String key)
}
checkDuplicate(key);
changes.put(key, new StringMapEntryDelete(key, valueBefore));
deleteIfPresent("source:" + key);
Copy link
Member

Choose a reason for hiding this comment

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

This may look like a reasonable thing to do, however, I fear that this will open a can of worms: Source is not the only subtag there is. In fact, there are countless subtags. I.e. if I delete highway=service, then service=driveway should also be deleted, if it exists. etc. etc.
Should the app have the entitlement to at this level do this kind of automation, use this kind of intelligence? I think perhaps this should be in the responsibility of each and every individual quest that deletes things.

Another comment: StringMapChanges class (and the Builder) is currently oblivious of any OpenStreetMap data. This line would make it exclusively tailored for OSM which is not reflected in the name of that class.

Copy link
Member Author

@matkoniecz matkoniecz Mar 3, 2018

Choose a reason for hiding this comment

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

I think perhaps this should be in the responsibility of each and every individual quest that deletes things.

I moved it here as deleteIfPresent("source:" + YET_ANOTHER_KEY) after every single deletion looked ugly, but I failed to consider this. I will move it back. It will make this quest code uglier, but I am no longer sure is it overall improving situation.

EDIT: moved

deleteIfPresent("source:" + key);
}

public void deleteIfPresent(@NonNull String key)
Copy link
Member

Choose a reason for hiding this comment

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

*IfExists`?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed

import java.text.SimpleDateFormat;
import java.util.Calendar;

public class DateHandler {
Copy link
Member

Choose a reason for hiding this comment

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

Handler seems to be an odd name for this. I call these classes which only contain utility functions usually *Utils.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to DateUtil to match QuestUtil

return new YesNoQuestAnswerFragment();
}

public static String getQueryPart(String key, String nameOfGeneratedGroup, int rewievIntervalInDays){
Copy link
Member

Choose a reason for hiding this comment

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

Why is this public?

Copy link
Member

Choose a reason for hiding this comment

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

Also, typo in review

Copy link
Member Author

@matkoniecz matkoniecz Mar 3, 2018

Choose a reason for hiding this comment

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

No idea why it is public, thanks for catching typo. EDIT: handled

" - .known_opening_date_in_future)" +
" - .recently_edited" +
") -> " + nameOfGeneratedGroup + ";";
}
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty complex, wow! Did you test that this works? Do you think that it could somehow be tested in a unit test?

Copy link
Member

Choose a reason for hiding this comment

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

This query could probably be simplified? Perhaps i.e. like this(?):
way[highway=construction](!newer: my-cutoff-date)(if:!is_date(t['opening_date']) || date(t['opening_date'])<date('" + currentDate + "'))

Copy link
Member Author

Choose a reason for hiding this comment

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

"Did you test that this works?", obviously yes, before testing it was full of bugs of various kinds (from invalid queries to not handling unexpected tagging - it even ended with a new tiny feature for overpass turbo :) ).

I was not even thinking about setting up unit test, but it may be a good idea - with query complexity it will be useful as documentation. Main problem is that unit tests rely on OSM data and this kind of data will be especially fragile. But maybe for unit tests we should query historical data that will not change? I just though about it, but it seems that it should solve problem of fragile tests, at cost of even greater complexity.

It is likely that it can be simplified, but I will try to set up unit test before that.

Copy link
Member

Choose a reason for hiding this comment

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

Cool! Well, I was just wondering about the (unit) tests. If to enable testing at all results in (much) more complex code, one should think twice if it is worth it, in my opinion.
You are right of course that this construction data is especially fragile.... perhaps we could use the Airport Berlin as a relatively stable construction site, haha (construction began in 2006, not ready until at least 2020)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, much more complex code is not worth it - but hopefully it can be done without it.

Copy link
Member Author

Choose a reason for hiding this comment

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

!newer - unfortunately neither older or !newer is an allowed Overpass Turbo syntax. I used other ideas to simplify the query, thanks!

See tyrasd/overpass-turbo#247 for a related feature request

") -> .invalid_construction_type;" +
"(" +
" way[highway=construction][fixme];" +
") -> .with_fixme;" +
Copy link
Member

Choose a reason for hiding this comment

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

What are the "(" .... ")" for for the .invalid_construction_type; and with_fixme query?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why are fixmes filtered out? (And why then only here, not in the other?)

Copy link
Member Author

@matkoniecz matkoniecz Mar 3, 2018

Choose a reason for hiding this comment

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

What are the "(" .... ")" for for the

I thought that it is a necessary syntax, but I may be wrong. I will try to eliminate it.

Also, why are fixmes filtered out? (And why then only here, not in the other?)

On encountering access tag on the road quest will add fixme and make no further edits. So fixme for roads must be filtered out to avoid repeating quest over and over again and to avoid overwritting existing fixmes. Buildings are not affected by access mess, so it is not necessary to leave fixmes, so it is not necessary to filter them out.

Copy link
Member

@westnordost westnordost Mar 4, 2018

Choose a reason for hiding this comment

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

Ah, I see, so that is where that comes from. Well, so on that access topic, if no feasible automatism can be found, perhaps the user could also be asked about it, since he is already there. So, if the access tag is not clear from context, the app could ask the surveyor about it after he answered "yes, the construction is finished!".

Copy link
Member Author

@matkoniecz matkoniecz Mar 4, 2018

Choose a reason for hiding this comment

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

So, if the access tag is not clear from context, the app could ask the surveyor about it after he answered "yes, the construction is finished!

It can be done as a separate issue but given very long list of access tags we run into tag translation issue. I can add it but it would add new translation string for every support access tag (currently list has 19 values).

Copy link
Member Author

Choose a reason for hiding this comment

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

And it is also trickier than I initially though - what about situation where proper solution is not to accept all access tags or remove all of them, but to remove part of them?

}
}
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Very thoughtful!
However, why only include the "most popular" road access tags (why not any, or at least, any documented one)?
And why not remove the access tag?

Copy link
Member Author

@matkoniecz matkoniecz Mar 3, 2018

Choose a reason for hiding this comment

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

why not any, or at least, any documented one

I though that including all defined at wiki would require either update treadmill (as people invent new ones and add them at https://wiki.openstreetmap.org/wiki/Key:access#Land-based_transportation ) or it would be weird "all common ones and all defined at February 2018".

But I may update list to include all listed at https://wiki.openstreetmap.org/wiki/Key:access#Land-based_transportation - should I do it?

And why not remove the access tag?

https://www.openstreetmap.org/way/83230134 has access tags of way that will be build, https://www.openstreetmap.org/way/540501408/history has something that I am unable to interpret and maybe should be kept maybe deleted https://www.openstreetmap.org/way/88702995 has tags that refer to what will be constructed - for more cases like this see http://overpass-turbo.eu/s/wGm

overall, in this case I prefer to avoid making edit that in 96% will be correct, in 3% will be highly confusing and in 1% will break something and irritate normal mappers. % are obviously based on biased sample and guessing. But even if silent damage, without mistake by mapper, would happen in 0.1% of cases I would still avoid edit like this.

Copy link
Member

Choose a reason for hiding this comment

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

I though that including all defined at wiki would require either update treadmill

I mean, why not access=*?

And why not remove the access tag?

Wouldn't then a simpler logic be feasible as well? If access=no|construction_vehicles or similar -> remove access. Anything else, don't change anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

See for example https://www.openstreetmap.org/way/83230134 where access=no should stay.

I mean, why not access=*?

Just access key? And ignore other access keys? Note that it is quite common to set value also in other keys - foot is the most popular but there are also more complex cases.

Copy link
Member

Choose a reason for hiding this comment

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

Just access key? And ignore other access keys?

Ah, I understand.

return OverpassQLUtil.getGlobalOverpassBBox(bbox) +
getQueryPart("highway", groupName, 14) +
"(" +
"way[construction]" +
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this line superfluous?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why are "invalid road construction types" filtered out?

Copy link
Member Author

@matkoniecz matkoniecz Mar 3, 2018

Choose a reason for hiding this comment

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

I had no good idea what to do with cases like highway=construction, construction=service_parking. Throw away construction value? Tag with invalid highway value? And anyway it would further increase complexity.

Copy link
Member

Choose a reason for hiding this comment

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

How about not excluding it, but treating these kind of invalid values in a way that then the quest tags the fallback highway=road? I mean, in the wiki, it is pretty clearly defined that the value of the construction tag should be the value of the highway tag, right? So, if it is invalid, then it can be ignored. But what shouldn't be ignored is the fact that the construction site exists at all.

Where does service_parking come from? Was that just some guy or is this used by several people?

Copy link
Member

@westnordost westnordost Mar 4, 2018

Choose a reason for hiding this comment

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

Or, another option would be to tag highway=service_parking in that case. Why should the app fix tagging anomalies automatically that were introduced manually by humans?

Copy link
Member Author

@matkoniecz matkoniecz Mar 4, 2018

Choose a reason for hiding this comment

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

OK, so such edit would not introduce a mistake - it may even make it more visible.

And useful data (that construction is completed) is still added.

And it will simplify code.

I think that keeping it makes no sense (it remains from original version of code).

EDIT: Changed

changes.deleteIfPresent(OsmTaggings.SURVEY_MARK_KEY);
} else {
changes.addOrModify(OsmTaggings.SURVEY_MARK_KEY, DateHandler.getCurrentDateString());
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would make sense if these lines (53-58) would move to the super class?

Copy link
Member Author

Choose a reason for hiding this comment

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

These were initially in superclass with other code that I simplified and I moved them here as I though that calling function makes less clear what happens.

I may move them back again (I though about it and was unsure what is better, I am fine with both solutions).

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved back to superclass.

groupName + " - .invalid_construction_type)" +
"- .with_fixme" +
") -> " + resultsName + ";" +
resultsName + " out meta geom;";
Copy link
Member

Choose a reason for hiding this comment

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

If the getQueryPart function would accept an additional parameter named something like additionalFilters, this would simplify this method somewhat

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code was already simplified by implementing other requests.

I am not convinced that this change would improve it further - it would just move complexity to getQueryPart function that would need to handle additionalFilters.

It would be even worse, as additionalFilters may be empty.

@matkoniecz
Copy link
Member Author

I am impressed! This quest is pretty smart and only possible in this form with this complex overpass query you crafted. All in all, it is in a pretty good shape.

Thanks!

However, there are some open questions. See below.

For start - thanks for a review!

Simpler changes are committed and pushed - other are added to as TODOs at #920 (comment) (hopefully nothing is missed).

Currently my main question is whatever I should add all land access tags listed at https://wiki.openstreetmap.org/wiki/Key:access#Land-based_transportation or keep list as it is current?

I plan on handling remaining TODOs and testing code again - hopefully I will be able to do it soon.

@matkoniecz matkoniecz changed the title Add "is contruction finished" quests for highway=construction and building=construction [waiting for review, mergeable after squashing] Add "is contruction finished" quests for highway=construction and building=construction Mar 3, 2018
@westnordost
Copy link
Member

Currently my main question is whatever I should add all land access tags listed at https://wiki.openstreetmap.org/wiki/Key:access#Land-based_transportation or keep list as it is current?

Well, my remark was based on a misunderstanding. I thought you check for access = any of these accesses instead of any of these accesses = *.

matkoniecz added a commit to matkoniecz/Zazolc that referenced this pull request Mar 7, 2018
currently live data is used, as result tests may be broken by OSM edits

note new test_unspecified_building_type_is_excluded - this test fails with the curent OSM data, but it is working for specified point of time

this increases complexity of code, but it should be preferable to tests failing because OSM data changed

I plan on using the same method for unit tests for streetcomplete#920
matkoniecz added a commit to matkoniecz/Zazolc that referenced this pull request Mar 7, 2018
currently live data is used, as result tests may be broken by OSM edits

note new test_unspecified_building_type_is_excluded - this test fails with the curent OSM data, but it is working for specified point of time

this increases complexity of code, but it should be preferable to tests failing because OSM data changed

I plan on using the same method for unit tests for streetcomplete#920
matkoniecz added a commit to matkoniecz/Zazolc that referenced this pull request Mar 7, 2018
currently live data is used, as result tests may be broken by OSM edits

note new test_unspecified_building_type_is_excluded - this test fails with the curent OSM data, but it is working for specified point of time

this increases complexity of code, but it should be preferable to tests failing because OSM data changed

I plan on using the same method for unit tests for streetcomplete#920
return overpassServer.getAndHandleQuota(getOverpassQuery(bbox), handler);
}

/** @return overpass query string to get streets marked as under construction but excluding ones
Copy link
Contributor

Choose a reason for hiding this comment

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

buildings, not streets

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@matkoniecz
Copy link
Member Author

matkoniecz commented Mar 8, 2018

Currently I am waiting for #955 #967 to check whatever my approach to unit tests using overpass data will be accepted.

If idea from that PR is accepted I will implement similar unit tests for this PR.

If not - I will need to invent some new method for stable unit tests or to not implement this.

@westnordost
Copy link
Member

westnordost commented Mar 8, 2018 via email

@goldfndr
Copy link
Contributor

goldfndr commented Mar 8, 2018

The PR has 14 and 180 hardcoded in the getQueryPart calls. Should those perhaps be in ApplicationConstants.java (or one of the new files) instead of magically appearing?

@matkoniecz
Copy link
Member Author

matkoniecz commented Mar 8, 2018

I am convinced that separating constant definitions from only place where the constants are used is making code less readable.

Note also how it looks in Android Studio (and other usable Java IDEs)

selection_018

@matkoniecz matkoniecz changed the title Add "is contruction finished" quests for highway=construction and building=construction Add "is construction finished" quests for highway=construction and building=construction Mar 12, 2018
@matkoniecz
Copy link
Member Author

Two comments above were mistakenly put by github here, I replied to them in their review threads.

@matkoniecz matkoniecz changed the title Add "is construction finished" quests for highway=construction and building=construction [waiting for a second review] Add "is construction finished" quests for highway=construction and building=construction Mar 21, 2018
@westnordost
Copy link
Member

Ok, so the only open issue is #920 (comment) , then it can be merged.

@westnordost westnordost removed this from the v5 milestone Apr 7, 2018
@matkoniecz matkoniecz force-pushed the construction_PR branch 2 times, most recently from 22b7ed7 to bd8cddf Compare April 9, 2018 11:40
checks highway=construction
checks building=construction

fixes streetcomplete#685
@matkoniecz matkoniecz force-pushed the construction_PR branch 2 times, most recently from a32e670 to bbef05e Compare April 9, 2018 13:10
@matkoniecz
Copy link
Member Author

So at this moment I am again testing after making requested changes.

DateUtil function getOffsetDateStringFromDate was modyfying passed object
now it is fixed
@matkoniecz
Copy link
Member Author

matkoniecz commented Apr 9, 2018

And now, after testing it is ready for a review!

Note to self: after merging - push PR for #829

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.

👍

@westnordost
Copy link
Member

Found nothing to complain about anymore. Great job! I trust you tested it, so I will merge it right away. It can go into beta2 that I will release tomorrow or latest on Wednesday.

@westnordost westnordost merged commit 9a09d95 into streetcomplete:master Apr 9, 2018
@matkoniecz matkoniecz deleted the construction_PR branch April 9, 2018 22:19
@matkoniecz
Copy link
Member Author

And thanks for reviews!

I was thinking about topic for the next PR - I thought about #527, #1009, #213 and #1010. Do you have any preferences of what should be done next? Currently I am planning to go with #1010 (should make further development easier, is in some form finishing this PR).

@rugk
Copy link
Contributor

rugk commented Apr 10, 2018

@matkoniecz BTW, can you confirm that this overpass query for this quest is still up-to-date? I use it in the wiki.

@RubenKelevra
Copy link
Contributor

The icon looks a bit odd... never seen this type of mark on a construction zone in Germany 🤔

They are white with a white/red reflector here, not yellow black.

Maybe a red/white pylon is a bit more internationally recognizeable? 🤔

@rugk
Copy link
Contributor

rugk commented Apr 12, 2018

Indeed the colors are more typical for the US, I think. But on the other hand, they are also well-known in Europe (I think), because of websites or so, using this to show "work in progress" or similar things. 😄

@ENT8R
Copy link
Contributor

ENT8R commented Apr 12, 2018

IMO the icon must not reflect 100% the situation how a sign looks like in every single country because then we end up having 30 icons all meaning the same but having only different colours. Which is way to much effort 😄

@RubenKelevra
Copy link
Contributor

Well, the black/yellow thing doesn't fit anything familiar and I clicked on it because I was curious what that means.

I think a pylon in red/white might be more well known in the most countries. They are even used in the USA, but selecting a barricade which is not used anywhere in Europe is just like asking everybody to insert the maxspeed in USA signs - which we don't do. ;)

@ENT8R
Copy link
Contributor

ENT8R commented Apr 12, 2018

Or maybe something like that:

@rugk
Copy link
Contributor

rugk commented Apr 12, 2018

Let's not integrate VLC into this app. 😉

@RubenKelevra
Copy link
Contributor

@ENT8R I like that, but it really looks like the VLC icon ;)

Maybe we can just draw a different style of pylon from a photo? 🤔

@rugk
Copy link
Contributor

rugk commented Apr 13, 2018

Maybe we can just draw a different style of pylon from a photo?

The result will be the same. You can't get around VLC when drawing a pylon, unless you swap colors somehow or so, but then it looks weird. 😉

@RubenKelevra
Copy link
Contributor

@rugk what do you think about this one?

roadpado75_01d_600x600 svg

@matkoniecz
Copy link
Member Author

In my opinion current icons are recognizable as construction-related what makes them good enough. I see no necessity of matching them to the most common construction barrier in one specific country.

Also, if that would be real problem it would be much better to discuss it in a separate issue - this is a completed, merged pull request - and I prefer to not put it on ignore list because there may be an useful question in future (why I did X?).

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.

Request: is this feature still under construction?
6 participants