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.

Sign up for GitHub

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

Jump to bottom

Tactile pavings quest #303

Merged
merged 3 commits into from Jun 7, 2017
Merged

Tactile pavings quest #303

merged 3 commits into from Jun 7, 2017

Conversation

PanierAvide
Copy link
Contributor

See #238

westnordost
westnordost reviewed Jun 7, 2017
View reviewed changes
.../main/java/de/westnordost/streetcomplete/quests/tactile_paving/TactilePavingBusStopForm.java Outdated
setTitle(R.string.quest_tactilePaving_title_bus);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The reason why the indentation looks odd here everywhere is because you use 4 spaces for indentation but the project wide setting is 1 tab.

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 the project was using spaces instead of tabs (Android Studio said so). I will correct this.

Copy link
Member

Choose a reason for hiding this comment

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

That's odd, I am pretty sure it is tabs.

westnordost
westnordost reviewed Jun 7, 2017
View reviewed changes
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.

Looks good!

Two quests for the price of one! :-)

Though, it feels still like way too much code for such a simple quest. Perhaps I will remove the "quest priority" and make the order in whcih they are defined in the quest list the priority. For version 2.

Is it ready for merge from your side?

app/src/main/res/values/strings.xml Outdated
@@ -266,4 +266,7 @@ The info you enter is then directly added to the OpenStreetMap in your name, wit
<string name="quest_leave_new_note_in_response_to">"In reply to \"%1$s\":
%2$s"</string>
<string name="quest_toiletsFee_title">"Do these toilets require a fee?"</string>
<string name="quest_tactilePaving_title_bus">"Does this bus stop has tactile pavings?"</string>
Copy link
Member

Choose a reason for hiding this comment

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

*have

app/src/main/res/values/strings.xml Outdated
@@ -266,4 +266,7 @@ The info you enter is then directly added to the OpenStreetMap in your name, wit
<string name="quest_leave_new_note_in_response_to">"In reply to \"%1$s\":
%2$s"</string>
<string name="quest_toiletsFee_title">"Do these toilets require a fee?"</string>
<string name="quest_tactilePaving_title_bus">"Does this bus stop has tactile pavings?"</string>
<string name="quest_tactilePaving_title_crosswalk">"Does this crosswalk has tactile pavings?"</string>
Copy link
Member

Choose a reason for hiding this comment

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

*have

app/src/main/res/values/strings.xml Outdated
@@ -266,4 +266,7 @@ The info you enter is then directly added to the OpenStreetMap in your name, wit
<string name="quest_leave_new_note_in_response_to">"In reply to \"%1$s\":
%2$s"</string>
<string name="quest_toiletsFee_title">"Do these toilets require a fee?"</string>
<string name="quest_tactilePaving_title_bus">"Does this bus stop has tactile pavings?"</string>
<string name="quest_tactilePaving_title_crosswalk">"Does this crosswalk has tactile pavings?"</string>
<string name="quest_tactilePaving_title_name_bus">"Does the bus stop \"%s\" has tactile pavings?"</string>
Copy link
Member

Choose a reason for hiding this comment

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

same

@PanierAvide
Copy link
Contributor Author

Applied fixes to every comment made, so for me it's ready to merge :-)
I agree that the quest priority might be defined by declaration order, or maybe all quests should be equal in rights.

@westnordost westnordost merged commit 545f7f9 into streetcomplete:master Jun 7, 2017
@Binnette
Copy link
Contributor

Hi, I test those quests today. And they are working very fine... But I make some mistakes on bus stop. Cause quests "bus stop shelter" and "bus stop tactile paving" share the same icon.

We have to use 2 differents icon, here are some ideas :
First line for bus shelter and second for tactile paving.
bus

@westnordost
Copy link
Member

westnordost commented Jun 10, 2017 •
edited

I'd simply add an icon like http://www.marland.eu/uploads/tx_commerce/870260.png for any tactile pavings quest.

@matkoniecz matkoniecz mentioned this pull request Jun 16, 2017
Closed
@matkoniecz
Copy link
Member

@PanierAvide It would be probably be a good idea to mention source of https://github.com/PanierAvide/StreetComplete/blob/de4ed993279643878d63584f727b82560c1db138/app/src/main/res/drawable-hdpi/tactile_paving_illustration.jpg in https://github.com/westnordost/StreetComplete/blob/master/app/src/main/res/authors.txt file (I just discovered it during attempting to make quest that involves images).

@PanierAvide
Copy link
Contributor Author

The image was made from these three pictures:

@matkoniecz
Copy link
Member

BTW, have you found somewhere whatever CC BY SA 2.0 is compatible with GPLv3? In long search I found that CC BY SA 4.0 is one way compatible, but nothing clear about earlier versions (https://creativecommons.org/2015/10/08/cc-by-sa-4-0-now-one-way-compatible-with-gplv3/). As result during search for images I ignored CC BY SA with version earlier than 4.0 (what means ignoring most available).

@PanierAvide
Copy link
Contributor Author

I haven't looked for this. However, I know the person who took these pictures, if necessary I can ask him if he allows us to use them as CC By 4.0 (he will agree for sure, as he proposed to use these pictures for the quest).

matkoniecz added a commit to matkoniecz/Zazolc that referenced this pull request Jun 16, 2017
@matkoniecz
give sources for tactile_paving_illustration.jpg
3ec94e9 
based on streetcomplete#303 (comment)
comment by author of commit that added this file
@matkoniecz
Copy link
Member

@PanierAvide That would be great (copyright issues are unfunny, the worst case is ignoring copyright issues at start and later changing opinion and discovering that it is too late to do that).

@PanierAvide
Copy link
Contributor Author

I understand. I asked him by email to come comment this issue in order to give his agreement.

@westnordost
Copy link
Member

westnordost commented Jun 16, 2017 via email

@westnordost
Copy link
Member

westnordost commented Jun 16, 2017 via email

@matkoniecz
Copy link
Member

Cc by sa -> GPL is no problem

Why? From what I understand it is not completely clear, for example cc-by-sa 4.0 compatibility was not presented as something obvious (https://creativecommons.org/2015/10/08/cc-by-sa-4-0-now-one-way-compatible-with-gplv3/ - "CC BY-SA 4.0 one-way compatibility with GPLv3 is a huge win. It took many years to achieve.")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westnordost westnordost westnordost left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PanierAvide @Binnette @westnordost @matkoniecz