Quest for bus stop names (see #551) #986

PanierAvide · 2018-03-24T17:40:11Z

Based on place quest (re-used generic templates).

rugk · 2018-03-24T17:45:19Z

You can (automatically) let issues close when a PR is merged by using "fixes" instead of "see". (And don't putting it in the title, but in PR description. Then it is also linked by GitHub.)

rugk · 2018-03-24T17:48:14Z

app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_name/AddBusStopName.java

+	}
+
+	@Override public String getCommitMessage() { return "Determine bus stop names"; }
+	@Override public int getIcon() { return R.drawable.ic_quest_label; }


Hmm, would not a dedicated icon better? We have a bus and we have this one you chose here. I think one can simply combine them…

Well, we need other opinion on this, I was not sure about the one to choose. It seemed logical to have the same icon as we are asking a name, but also makes sense to show a bus icon.

westnordost · 2018-03-24T20:36:43Z

Actually, I made a special custom icon for this. Perhaps I did not import it from the quests.svg. It is the icon where you see a bus stop Am 24. März 2018 18:56:51 MEZ schrieb Adrien Pavie <[email protected]>:

…

PanierAvide commented on this pull request. > + } + + @OverRide public void applyAnswerTo(Bundle answer, StringMapChangesBuilder changes) + { + if(answer.getBoolean(AddBusStopNameForm.NO_NAME)) + { + changes.add("noname", "yes"); + return; + } + + String name = answer.getString(AddBusStopNameForm.NAME); + if(name != null) changes.add("name", name); + } + + @OverRide public String getCommitMessage() { return "Determine bus stop names"; } + @OverRide public int getIcon() { return R.drawable.ic_quest_label; } Well, we need other opinion on this, I was not sure about the one to choose. It seemed logical to have the same icon as we are asking a name, but also makes sense to show a bus icon. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #986 (comment)

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

rugk · 2018-03-24T21:40:30Z

app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_name/AddBusStopName.java

+	@Override protected String getTagFilters()
+	{
+		return " nodes, ways with !name and noname != yes" +
+			   " and ( highway = bus_stop or public_transport = platform )";


The tactile paving quests filters that to only ask when it is not the stop position (i.e. (highway=bus_stop and public_transport!=stop_position)). So maybe you should do it here, too?

rugk · 2018-03-24T21:42:34Z

Ah okay, this one:

~~In the issue, I said:~~

~~Of course, this also applies to trams.~~

So can we extent that to trams, too? Could be problematic with the icon then, but mhh… (Okay, maybe not. The bus is a sign for "public transport" in StreetComplete anyway, as the other quests for public transport also ask about tram stop and do not have a different icon.)

Uhg, forget it, it actually already applies to that as it seems.

PanierAvide · 2018-03-24T22:02:14Z

Can't find anymore the way to define an icon from SVG into drawable for Android. Can you provide some pointers about that ?

PanierAvide · 2018-03-25T06:18:56Z

Found a way to do so (using online converter) so changed the icon. Is there a documentation in the project for this conversion process ?

ENT8R · 2018-03-25T06:40:29Z

I also had some trouble doing this and this is what helped me: #825 (comment)

westnordost · 2018-03-25T07:00:52Z

Found a way to do so (using online converter) so changed the icon. Is there a documentation in the project for this conversion process ?

Put the icon apart from the main quest icons sheet into a separate 128x128 svg. In Inkscape select all and ungroup everything until nothing is grouped anymore to workaround a shortcoming/bug of the Android Studio drawable importer. Then, save it.
In Android Studio, New -> Vector Asset -> Asset type: local file

westnordost · 2018-03-25T07:05:29Z

app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_name/AddBusStopName.java

+	@Override protected String getTagFilters()
+	{
+		return " nodes, ways with !name and noname != yes" +
+			   " and (public_transport=platform or (highway=bus_stop and public_transport!=stop_position))";


This would include any kind of public transport platform (i.e. also train stations and metro stops).

I think it would make sense if the query is the same as for AddBusStopShelter:
https://github.com/westnordost/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_shelter/AddBusStopShelter.java#L24-L26

As tram was named in the original issue, it was making sense to include them as well. So is it better to only show bus stops ?

Well answer is below.

westnordost · 2018-03-25T07:08:33Z

app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_name/AddBusStopNameForm.java

+	{
+		return !nameInput.getText().toString().trim().isEmpty();
+	}
+}


Isn't this exactly the same as the AddPlaceNameForm? Couldn't that be used instead? (And rename to AddNameForm perhaps)

westnordost · 2018-03-25T07:09:41Z

app/src/main/res/drawable/ic_quest_bus_stop_name.xml

+														android:strokeMiterLimit="4" />
+													</group>
+												</group>
+											</vector>


This will likely have to be reimported using the method I described because I made the experience that Android does not handle groups well.

westnordost · 2018-03-25T07:12:13Z

app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_name/AddBusStopName.java

+	@Override public int getIcon() { return R.drawable.ic_quest_bus_stop_name; }
+	@Override public int getTitle(@NonNull Map<String, String> tags)
+	{
+		return R.string.quest_busStopName_title;


Right now this includes any kind of public transport platform. It would be odd if the quest asked that question for a tram, railway or metro stop. The question should be different for every different type of included public transport platform. I suggest to only include bus and tram stops and then have two strings. See
https://github.com/westnordost/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/bus_stop_shelter/AddBusStopShelter.java#L24-L26

rugk · 2018-03-25T07:15:07Z

Basically, I've already done 1. 😆 (You may just use the unoptimized version.)
For a "how to" for splitting the SVG file out of the main SVG, see here.

westnordost · 2018-03-25T07:16:36Z

Oh, by the way, I think there is room for a major improvement here.

In countries where it is common that street names have a name in two languages, I would expect bus stops to also have two languages. The street quest offers for these cases to input the name of the street in each language, perhaps it would make sense to use the same logic for this quest.

PanierAvide · 2018-03-25T10:04:41Z

I started this refactoring, but having an issue when opening input for bus stop name. Will work on this later (or if someone have time to check...).

HolgerJeromin · 2018-03-25T12:01:28Z

Perhaps stops on the other directions have a name. Should / could we provide names from near by stops?

matkoniecz · 2018-03-25T17:13:30Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopName.java

+	@Override protected String getTagFilters()
+	{
+		return "nodes with" +
+			" ((public_transport=platform and (bus=yes or trolleybus=yes or tram=yes))" +


It should be renamed from "AddBusStopName.java" or trams should not be accepted here (the first solution is probably preferable).

Note that also, question, commit description and icon may require changes.

Icon does not, in the other quests the bus also stands for any public transport. (see the crossed part in #986 (comment), where I already mentioned that.)

Also still see #986 (comment) BTW, you do not exclude the stop position.

Does anyone care?

Ah, it's actually below this line, sorry for the noise. Just did not see this was already resolved.

westnordost · 2018-03-27T10:12:30Z

I started this refactoring, but having an issue when opening input for bus stop name. Will work on this later (or if someone have time to check...).

Hui, cool. Just so you know, I do not require the multi-language thing to be implemented for this PR to be merged, it is just an obvious and reasonable improvement.

PanierAvide · 2018-03-28T16:43:32Z

Well, now it works with multi-language support :-) Tried to make code as common as possible between street name and bus stop name quests (introduced LocalizedName class).

westnordost

Wow, that looks a lot. The refactor is very good. It's good to have a common abstract super class rather than having one concrete inherit from the other.

I left some comments, not sure if changes are necessary or I understood somethign wrong.

westnordost · 2018-03-28T18:41:18Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopName.java

@@ -0,0 +1,88 @@
+package de.westnordost.streetcomplete.quests.localized_name;


Is the name of the package deliberate? Just wondering.

Yes, but can be changed if needed.

westnordost · 2018-03-28T18:43:29Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopName.java

+		}
+
+		String[] names = answer.getStringArray(AddRoadNameForm.NAMES);
+		String[] languages = answer.getStringArray(AddRoadNameForm.LANGUAGE_CODES);


Shouldn't it be AddLocalizedNameForm.NAMES etc?

westnordost · 2018-03-28T18:46:38Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopName.java

+			}
+		}
+		return result;
+	}


I take the last 40+ lines are copied 1:1 from the road name quest? Perhaps this should be outsourced here. Abstraction is not a reasonable option here, but perhaps simply put it into the AddLocalizedNameForm class as a static helper method to access the answer-bundle?

westnordost · 2018-03-28T18:49:28Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopNameForm.java

+	{
+		// either the user added a language or typed something for the street name
+		return adapter.getData().size() > 1 || !adapter.getData().get(0).name.trim().isEmpty();
+	}


I think this does not need to be overriden. It is already defined in the parent class.

westnordost · 2018-03-28T18:54:34Z

...c/main/java/de/westnordost/streetcomplete/quests/localized_name/AddLocalizedNameAdapter.java

-							  List<Map<String, String>> roadNameSuggestions, Button addLanguageButton)
+	public AddLocalizedNameAdapter(ArrayList<LocalizedName> data, Context context, List<String> languages,
+								   AbbreviationsByLocale abbreviationsByLocale,
+								   List<Map<String, String>> localizedNameSuggestions, Button addLanguageButton)


Adding the @Nullable annotations to parameters (and fields) that may explicitly be null will make intelligent IDEs (like Android Studio) be able show an error if you forgot the null-check somewhere. Would you add this annotation? Also good for documenation.

westnordost · 2018-03-28T18:56:33Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddRoadNameForm.java


 		return view;
 	}

-	private void addOtherAnswers()
+	protected void addOtherAnswers()


I think this is not necessary as you did the refactor properly by using a common super class.

westnordost · 2018-03-28T18:58:39Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddLocalizedNameForm.java

+import de.westnordost.streetcomplete.util.Serializer;
+import de.westnordost.streetcomplete.view.dialogs.AlertDialogBuilder;
+
+public abstract class AddLocalizedNameForm extends AbstractQuestFormAnswerFragment


westnordost · 2018-03-28T19:04:34Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopNameForm.java

+		bundle.putStringArray(NAMES, names);
+		bundle.putStringArray(LANGUAGE_CODES, languageCodes);
+		applyFormAnswer(bundle);
+	}


Isn't this already defined in the super class?

westnordost · 2018-03-28T19:04:53Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopNameForm.java

+			new LinearLayoutManager(getActivity(), LinearLayoutManager.VERTICAL, false));
+		recyclerView.setAdapter(adapter);
+		recyclerView.setNestedScrollingEnabled(false);
+	}


Isn't this already defined in the super class?

PanierAvide · 2018-03-29T06:27:59Z

I have done the requested refactoring, making subclasses lighter :-)

westnordost · 2018-03-29T19:29:43Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopName.java


-		HashMap<String,String> stopNameByLanguage = toStopNameByLanguage(names, languages);
+		HashMap<String,String> stopNameByLanguage = AddLocalizedNameForm.toNameByLanguage(names, languages);


Actually, that method could accept a bundle instead. This way, AddLocalizedNameForm.LANGUAGE_CODES etc. wouldn't even need to be public :-o

westnordost · 2018-03-29T19:31:02Z

app/src/main/java/de/westnordost/streetcomplete/quests/localized_name/AddBusStopNameForm.java

@@ -23,7 +23,7 @@ public View onCreateView(LayoutInflater inflater, ViewGroup container,

 		View contentView = setContentView(R.layout.quest_localizedname);

-		addOtherAnswers();
+		addOtherAnswers(this::confirmNoName);


Hm, I find this a bit odd/too much. If it is not the same action, it might just as well be defined in AddBusStopNameForm and not be "injected" into the super class.

PanierAvide · 2018-03-30T08:50:16Z

Refactoring of those two points also done ;-)

westnordost · 2018-03-30T10:47:33Z

Great! :-)

rugk · 2018-03-30T18:21:43Z

This still does not exclude the stop position. AFAIK it is not very useful to set the name of the stop position, is not it?

PanierAvide · 2018-03-31T08:00:53Z

Well, it depends if the stop position is part of a stop area relation. If it is not, it is useful to set the name as well.

Quest for bus stop names (see #551)

f892c95

rugk reviewed Mar 24, 2018

View reviewed changes

PanierAvide added 2 commits March 24, 2018 23:04

Bus stop name: filter out stop_positions

52b6fb5

Bus stop name quest: using proper icon

45a681e

westnordost requested changes Mar 25, 2018

View reviewed changes

WIP refactoring bus stop name + street name quests

b2a06d3

matkoniecz requested changes Mar 25, 2018

View reviewed changes

Missing bus stop name : fixed quest

053807f

westnordost reviewed Mar 28, 2018

View reviewed changes

Localized name quests : more merging of common methods

f55c083

westnordost reviewed Mar 29, 2018

View reviewed changes

Localized name quest: refactoring of some methods

b3a0a68

westnordost approved these changes Mar 30, 2018

View reviewed changes

westnordost merged commit 60f3831 into streetcomplete:master Mar 30, 2018

		@@ -0,0 +1,88 @@
		package de.westnordost.streetcomplete.quests.localized_name;


		HashMap<String,String> stopNameByLanguage = toStopNameByLanguage(names, languages);
		HashMap<String,String> stopNameByLanguage = AddLocalizedNameForm.toNameByLanguage(names, languages);

Quest for bus stop names (see #551) #986

Quest for bus stop names (see #551) #986

Conversation

PanierAvide commented Mar 24, 2018

rugk commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost commented Mar 24, 2018 via email

Choose a reason for hiding this comment

rugk commented Mar 24, 2018

PanierAvide commented Mar 24, 2018

PanierAvide commented Mar 25, 2018

ENT8R commented Mar 25, 2018

westnordost commented Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk commented Mar 25, 2018 • edited Loading

westnordost commented Mar 25, 2018

PanierAvide commented Mar 25, 2018

HolgerJeromin commented Mar 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost commented Mar 27, 2018

PanierAvide commented Mar 28, 2018

westnordost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanierAvide commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanierAvide commented Mar 30, 2018

westnordost commented Mar 30, 2018

rugk commented Mar 30, 2018

PanierAvide commented Mar 31, 2018

westnordost commented Mar 25, 2018 •

edited

Loading

rugk commented Mar 25, 2018 •

edited

Loading

westnordost Mar 28, 2018 •

edited

Loading