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 support for Nicaragua providers - v2 #389

Closed
wants to merge 1 commit into
base: master
from

Conversation

5 participants
@xamanu

xamanu commented Dec 5, 2017

Please add support and logos for providers in Nicaragua.

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch from 2fcf35d to bb6db53 Dec 5, 2017

@grote

This comment has been minimized.

Owner

grote commented Dec 5, 2017

this can now be submitted as a PR upstream.

Sorry, unfortunately it is not quite that easy as the commit contains unrelated changes as well and changes no longer necessary.

@xamanu

This comment has been minimized.

xamanu commented Dec 5, 2017

Sorry, my fault, I rebased.

@grote

This comment has been minimized.

Owner

grote commented Dec 5, 2017

The XML representation of the logo is huge (compared to the others). Couldn't the SVG be simplified a bit? Just the stripes and maybe a simple triangle in the middle? I would also suggest to remove all the groups from the SVG and join paths where possible, before doing the conversation to XML.

Furthermore, there's no Nicaragua provider in Transportr, yet. Maybe the logo should be submitted together with the provider?

You might also want to consider whether there should be one single Nicaragua Provider covering several cities or one provider per city. Changing this later is inconvenient for users as they need to re-select their provider and lose all saved data.

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch from bb6db53 to 5ebb87d Dec 5, 2017

@xamanu

This comment has been minimized.

xamanu commented Dec 5, 2017

The XML representation of the logo is huge (compared to the others). Couldn't the SVG be simplified a bit? Just the stripes and maybe a simple triangle in the middle? I would also suggest to remove all the groups from the SVG and join paths where possible, before doing the conversation to XML.

Yes, I simplified it.

Furthermore, there's no Nicaragua provider in Transportr, yet. Maybe the logo should be submitted together with the provider?

It's actually in the pipeline: schildbach/public-transport-enabler#181

You might also want to consider whether there should be one single Nicaragua Provider covering several cities or one provider per city. Changing this later is inconvenient for users as they need to re-select their provider and lose all saved data.

Currently we don't have enough coverage and some unresolved problems to provide a Nicaragua-wide GTFS.

@xamanu xamanu changed the title from Ni managua logo to Add logo for Managua, Nicaragua Dec 5, 2017

@xamanu

This comment has been minimized.

xamanu commented Dec 5, 2017

I know that @AltNico and @ialokim also suggested to have one for whole Nicaragua. I guess it is worth to talk together about it (here?):

Advantages of a country wide GTFS:

  • Easier for the user to select
  • Extensions and improvements to the GTFS are going to be included almost on the fly.

Disadvantages of a country wide GTFS:

  • More difficult to maintain.
  • On the current stage there is only the capital, and one middle size city and the bus connecting it. We have an unresolved problem on places (stops) with the same name in different cities.

I think it is not realistic to think, that if we go the path for one for the whole Nicaragua it will be soon anywhere close to complete. However it would probably help to work on it. I'm really not sure about which direction to take. What do you think (@grote, @AltNico, @ialokim)?

@xamanu xamanu changed the title from Add logo for Managua, Nicaragua to Add support for Managua, Nicaragua Dec 5, 2017

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch from 5ebb87d to 6cd64a1 Dec 5, 2017

@xamanu

This comment has been minimized.

xamanu commented Dec 5, 2017

I extended this pull request with the other parts that seem to be necessary in Transportr to support Managua.

android:pathData="M 32.835766 22.1992101 C 38.1324754817 22.1992101 42.4263069 26.4930415183 42.4263069 31.789751 C 42.4263069 37.0864604817 38.1324754817 41.3802919 32.835766 41.3802919 C 27.5390565183 41.3802919 23.2452251 37.0864604817 23.2452251 31.789751 C 23.2452251 26.4930415183 27.5390565183 22.1992101 32.835766 22.1992101 Z" />
<path
android:fillColor="#ffffff"
android:strokeWidth="1.2304405"

This comment has been minimized.

@grote

grote Dec 5, 2017

Owner

Way better smaller file! You could also remove these android:strokeWidth from the XML. They don't do anything on Android.

@@ -56,6 +56,7 @@
CANADA(R.string.np_region_canada, "🇨🇦"),
COSTA_RICA(R.string.np_region_costa_rica, "🇨🇷"),
AFRICA(R.string.np_region_africa, "🌍");
NICARAGUA(R.string.np_region_nicaragua, "🌍");

This comment has been minimized.

@grote

grote Dec 5, 2017

Owner

Here, you should add the flag of Nicaragua as a second parameter.

This comment has been minimized.

@xamanu

xamanu Dec 5, 2017

What is this second parameter? A blob? How can I modify it?

This comment has been minimized.

@ialokim

ialokim Dec 5, 2017

Contributor

It's the UTF8-Representation of the flag, see https://emojipedia.org/flag-for-nicaragua/

@@ -10,6 +10,7 @@
<change>Addresses will be saved</change>
<change>Round icon for new Android versions</change>
<change>Added New York, Ghana, Costa Rica</change>
<change>Added Managua</change>

This comment has been minimized.

@grote

grote Dec 5, 2017

Owner

Please add this to the list one line above, since this will only be shown to users when they upgrade from transportr 1 to 2.

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch 4 times, most recently from d65b33e to e8425ca Dec 5, 2017

@xamanu xamanu changed the title from Add support for Managua, Nicaragua to Add support for Nicaragua providers Dec 5, 2017

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch from e8425ca to 7026554 Dec 5, 2017

@xamanu

This comment has been minimized.

xamanu commented Dec 5, 2017

  • Use proper logo for Managua (based on rutas.mapanica.net).
  • Added also Esteli to this pull request.
@AltNico

AltNico approved these changes Dec 5, 2017

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch from 7026554 to 756b28d Dec 5, 2017

.build(),

new TransportNetworkBuilder()
.setId(NetworkId.ESTELI)

This comment has been minimized.

@grote

grote Dec 5, 2017

Owner

This PR will probably fail CI as these NetworkIds are not yet included in the version of PTE included here.


<group
android:translateX="-1.1721562"
android:translateY="-210.77028">

This comment has been minimized.

@grote

grote Dec 5, 2017

Owner

Could you maybe remove the group here (from the SVG and then export again). Also please use exactly 64dp for height and width.

<path
android:fillColor="#ffffff"
android:fillAlpha="0.98999999"
android:strokeAlpha="0.98999999"

This comment has been minimized.

@grote

grote Dec 5, 2017

Owner

These alphas can be removed. They don't do anything on Android.

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch from 756b28d to 720b5a5 Dec 5, 2017


new TransportNetworkBuilder()
.setId(NetworkId.MANAGUA)
.setName(R.string.np_name_managua)

This comment has been minimized.

@AltNico

AltNico Dec 5, 2017

Where do you defined this strings?

This comment has been minimized.

@xamanu

xamanu Dec 5, 2017

In the strings.xml (now added after a comment by @ialokim)

@ialokim

See my comment in line (that's why the travis ci fails).
Also it lacks all the strings in strings.xml (and translations):

  • np_region_nicaragua
  • np_name_managua
  • np_desc_managua
  • np_name_esteli
  • np_desc_esteli
@@ -56,6 +56,7 @@
CANADA(R.string.np_region_canada, "🇨🇦"),
COSTA_RICA(R.string.np_region_costa_rica, "🇨🇷"),
AFRICA(R.string.np_region_africa, "🌍");

This comment has been minimized.

@ialokim

ialokim Dec 5, 2017

Contributor

has to end with a comma here because the enum continues in the next line

This comment has been minimized.

@xamanu

xamanu Dec 5, 2017

Yes, indeed.

managuaProviderRef = new SoftReference<>(provider);
return provider;
} else if (networkId.equals(NetworkId.ESTELI)) {
if (esteliaProviderRef != null) {

This comment has been minimized.

@ialokim

ialokim Dec 5, 2017

Contributor

typo, did you mean esteliProviderRef?

This comment has been minimized.

@xamanu

xamanu Dec 5, 2017

Yes, thanks for noticing!

@xamanu xamanu force-pushed the mapanica:ni-managua-logo branch 3 times, most recently from baab572 to 45656d3 Dec 5, 2017

@xamanu

This comment has been minimized.

xamanu commented Dec 5, 2017

Thanks for your reviews, @grote, @AltNico and @ialokim! I fixed based on your comments.

@AltNico

This comment has been minimized.

AltNico commented Dec 7, 2017

So I was able to build the app by changing the pte dependency to nicaragua-providers--merged-into-otm. However, it does not find any information in Managua and Estelí. I think this is caused by pte looking for information at api.navitia.io instead of navitia.mapanica.net.

@grote

This comment has been minimized.

Owner

grote commented Dec 8, 2017

Yes, you should make sure your providers don't use the default API URL.

Before this can be merged, the upstream PTE MR needs to be ready (tests still missing) and then be merged into the staging PTE.

@ialokim ialokim force-pushed the mapanica:ni-managua-logo branch from 45656d3 to 9369c57 Dec 11, 2017

@xamanu

This comment has been minimized.

xamanu commented Feb 6, 2018

We created and integrated a PTE source where we can quickly integrate Managua: https://gitlab.com/opentransitmap/public-transport-enabler

@grote, can you please point me to the tests which are missing here? I guess there are other providers that implement such.

@grote

This comment has been minimized.

Owner

grote commented Feb 6, 2018

The live tests in your PTE pull requests are still missing. If it is in the PTE staging repo already anyway, it could already be integrated into transportr.

@xamanu xamanu changed the title from Add support for Nicaragua providers to Add support for Nicaragua providers - v2 Feb 9, 2018

@xamanu

This comment has been minimized.

xamanu commented Feb 9, 2018

Nicaragua has been included in navitia.io \o/ Now, I just included the respective PR into the staging PTE. This PR here needs to be modified to fit the single Nicaragua layer, as it was included in navitia.io

@xamanu

This comment has been minimized.

xamanu commented Feb 9, 2018

Closing in favour of its improved and working follower: #419

@xamanu xamanu closed this Feb 9, 2018

@ialokim ialokim deleted the mapanica:ni-managua-logo branch Mar 3, 2018

@rugk

This comment has been minimized.

rugk commented Aug 22, 2018

I know I am late, but as for simplifying SVGs you can/could just (have) use(d) https://jakearchibald.github.io/svgomg/.

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