-
Notifications
You must be signed in to change notification settings - Fork 3
MVP Push #5
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
base: master
Are you sure you want to change the base?
Conversation
app/build.gradle
Outdated
@@ -1,14 +1,16 @@ | |||
apply plugin: 'com.android.application' | |||
apply plugin: 'com.neenbedankt.android-apt' | |||
//apply plugin: 'com.neenbedankt.android-apt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete instead of comment, please!
OsmQuestAnswerListener, VisibleQuestListener, QuestsMapFragment.Listener, MapFragment.Listener | ||
{ | ||
@Inject CrashReportExceptionHandler crashReportExceptionHandler; | ||
Queue<Quest> questsToDo; @Inject CrashReportExceptionHandler crashReportExceptionHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate lines please!
@@ -1,5 +1,7 @@ | |||
package de.westnordost.streetcomplete.data.osm; | |||
|
|||
import android.util.Log; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to leave the logging so long as the debug level (Log.d?) is disabled in production!
import de.westnordost.streetcomplete.data.osmnotes.OsmNoteQuestType; | ||
import de.westnordost.streetcomplete.quests.baby_changing_table.AddBabyChangingTable; | ||
import de.westnordost.streetcomplete.quests.bike_parking_capacity.AddBikeParkingCapacity; | ||
import de.westnordost. streetcomplete.quests.bike_parking_capacity.AddBikeParkingCapacity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo
new AddTactilePavingBusStop(o) | ||
new AddBin(o) | ||
}; | ||
//new AddPassengerInformationDisplay(o), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comments and/or integrate above.
@@ -0,0 +1,64 @@ | |||
package de.westnordost.streetcomplete.quests; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to use only lit=yes/no for tags submitted to OSM (and probably just use a yes/no answer in the interface for now).
@Override public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) | ||
{ | ||
View view = super.onCreateView(inflater, container, savedInstanceState); | ||
setContentView(R.layout.sidewalk_lit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: check this layout and make sure we use yes/no rather than range.
@@ -0,0 +1,27 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this file!
@@ -0,0 +1,44 @@ | |||
package de.westnordost.streetcomplete.quests.crossing_markings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please disable for now - we will want to add as ways, hopefully under a different schema.
|
||
@Override protected String getTagFilters() | ||
{ | ||
//return "nodes, ways with "(("footway"="crossing") "and" ("kerb"="lowered"))"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
{ | ||
//return "nodes, ways with "(("footway"="crossing") "and" ("kerb"="lowered"))"; | ||
|
||
return "nodes, ways with (" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be nodes with (kerb=lowered and !tactile_paving)
, or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not displaying for some reason. nodes with kerb=lowered and !tactile_paving
might also work. Debug for alternative explanations as well - maybe not finding the icon, etc?
Also: just disable this quest for now so we can get a release out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also use a good icon!
String yesno = answer.getBoolean(YesNoQuestAnswerFragment.ANSWER) ? "yes" : "no"; | ||
changes.add("curb_ramps", yesno); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have an image? Would be handy for users to know what counts as 'tactile paving'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this isn't necessary to change before release - only if it's super super easy and fast.
|
||
@Override protected String getTagFilters() | ||
{ | ||
//return "nodes with (public_transport=platform or (highway=bus_stop and public_transport!=stop_position)) and !bench"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
// return "nodes with way[foot=yes][!lit]"; | ||
//return "ways with [highway="footway"] or way[foot="yes"] and !lit]"; | ||
|
||
return "nodes, ways with (" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ways with (highway=footway or foot=yes) and !lit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or similar)
|
||
public class AddSideWalkSurface extends SimpleOverpassQuestType | ||
{ | ||
// well, all roads have surfaces, what I mean is that not all ways with highway key are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment
|
||
@Override protected String getTagFilters() | ||
{ | ||
return "nodes, ways with (" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like ways with (highway=footway or foot=yes) and !surface
seems better, to me.
public class AddSideWalkSurfaceForm extends AbstractQuestFormAnswerFragment { | ||
public static final String SURFACE = "surface"; | ||
|
||
private final AddSideWalkSurfaceForm.Surface[] SURFACES = new AddSideWalkSurfaceForm.Surface[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing, make sure that the answer options make sense for footways, and that the images aren't of things that are obviously streets.
@@ -0,0 +1,30 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to remove references to this - we want to use lit=yes/no.
@@ -0,0 +1,15 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is titled busstop_shelter
but has references to tactilePavingSidewalkImage. This doesn't seem right!
@@ -0,0 +1,15 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - id of tactilePavingSidewalkImage
probably shouldn't be used for sidewalk_lit
app/src/main/res/values/strings.xml
Outdated
<string name="autosync_off">"Off"</string> | ||
<string name="quest_streetSurface_title">"What surface does this piece of road have?"</string> | ||
<string name="quest_streetSurface_name_title">"What surface does the road \"%s\" have here?"</string> | ||
<string name="quest_sideWalk_name_title">"What surface does the sidewalk \"%s\" have here?"</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made sense for streets, which often use the name
tag (e.g. NE 15th St), but sidewalks should almost never have a name
tag. If the quest module only shows this when a name
tag is present, this shouldn't hurt, though.
app/src/main/res/values/strings.xml
Outdated
<string name="quest_busStopLit_title">"Is this bus stop well lit?"</string> | ||
<string name="quest_busStopLit_title">"Is there lighting at this Bus stop?"</string> | ||
<string name="quest_busStopBench_title">"Does this bus stop have a bench?"</string> | ||
<string name="quest_SidewalkLit_title">"How many lights do you see from the place you’re standing?"</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to change this to a yes/no question (same as the street lit question, basically)
<string name="quest_passengerInformation_name_title">"Does the bus stop \"%s\" have an electronic information display?"</string> | ||
<string name="quest_busStopLit_name_title">"Is the bus stop \"%s\" well lit?"</string> | ||
<string name="quest_busStopLit_name_title">"Is there lighting at the \"%s\" Bus stop?"</string> | ||
<string name="quest_busStopLit_question2">"Are there one or more street lamps within 10 feet of this Bus stop?"</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ask a vaguer question, because the tag itself is also vaguer: is this bus stop lit?
app/src/main/res/values/strings.xml
Outdated
<string name="quest_address_house_name_label">House name:</string> | ||
<string name="quest_buildingLevels_roofLevelsLabel2">levels in the roof</string> | ||
<string name="quest_buildingLevels_levelsLabel2">regular levels (not counting the roof)</string> | ||
<string name="quest_range1">0</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the quest range question type is kept, these should be named more specifically. It's unlikely that most range questions will always fit into the categories of 0, 1-3, and 4-7, so I'd call it quest_range_lit_1 or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the previously suggested changes! Just a couple more and we're ready for release.
More things to double check:
- What are the changeset tags? We should have project=opensidewalks in the changeset tags (not to be confused with the tags set on the nodes and ways).
new AddPassengerInformationDisplay(o), | ||
new AddTactilePavingBusStop(o) | ||
new AddBusStopLit(o) | ||
//new AddBin(o) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-enable please!
import de.westnordost.streetcomplete.quests.bike_parking_cover.AddBikeParkingCover; | ||
import de.westnordost.streetcomplete.quests.building_levels.AddBuildingLevels; | ||
import de.westnordost.streetcomplete.quests.bus_stop_lit.AddBusStopLit; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused quests! e.g. baby_changing_table
{ | ||
//return "nodes, ways with "(("footway"="crossing") "and" ("kerb"="lowered"))"; | ||
|
||
return "nodes, ways with (" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not displaying for some reason. nodes with kerb=lowered and !tactile_paving
might also work. Debug for alternative explanations as well - maybe not finding the icon, etc?
Also: just disable this quest for now so we can get a release out there.
{ | ||
//return "nodes, ways with "(("footway"="crossing") "and" ("kerb"="lowered"))"; | ||
|
||
return "nodes, ways with (" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also use a good icon!
{ | ||
return "ways with (" + | ||
"((highway=footway) and (footway=sidewalk)" + | ||
" or way=foot" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be foot=yes
* Created by Neelam Purswani on 12-01-2018. | ||
*/ | ||
|
||
public class AddSideWalkSurface extends SimpleOverpassQuestType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swap to 'AddFootwaySurface' and remove (footway=sidewalk) so that this is a general-purpose footway surface annotator.
Also, we should explore all of the surface options to make sure we're (1) not missing anything and (2) not using any road-specific tags: https://taginfo.openstreetmap.org/keys/surface#values
app/src/main/res/values/strings.xml
Outdated
<string name="quest_busStopShelter_title">"Does this bus stop have a shelter?"</string> | ||
<string name="quest_bin_title">"Does this bus stop have a waste bin?"</string> | ||
<string name="quest_busStopLit_title">"Is this bus stop well lit?"</string> | ||
<string name="quest_busStopLit_title">"Is there lighting at this Bus stop?"</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add 'at night' for both of these
I'm curious what is the status of this project / pull request? Are there still plans to continue with a mobile app? Android only or are there iOS plans as well? |
No description provided.