grote / Transportr

Free Public Transport Assistant without Ads or Tracking
https://transportr.app
GNU General Public License v3.0
1.04k stars 188 forks source link

Not showing journey path on map for Nicaragua network #585

Closed ialokim closed 5 years ago

ialokim commented 5 years ago

Describe the bug Not sure since when this is happening, but Transportr apparently does not show the journey paths following the roads for Nicaragua anymore, although the data is provided by Navitia.

To Reproduce Steps to reproduce the behavior:

  1. Select Nicaragua as network provider
  2. Search for a trip from COTRAN Sur to Mercado Mayoreo
  3. Only see a path linking the two stops together instead of following the road.

Expected behavior The journey path should follow the road as provided by Navitia. I know this has worked some time before in Transportr, not sure when it stopped and what changed back then.

Screenshots As shown in Transportr: transportr_1 As provided by Navitia (see Navitia playground): transportr_2

Versions (please complete the following information):

grote commented 5 years ago

Very strange. You could try the first version when Nicaragua was added and see how it behaves there. If the line information is missing there as well, this might be a Navitia issue which is what I suspect, because IMHO the AbstractNavitiaProvider didn't change recently, did it?

ialokim commented 5 years ago

So with version 2.0.0 this still looks like it should:

transportr

grote commented 5 years ago

Interesting! Maybe related to the coordinates migration in PTE?

https://github.com/schildbach/public-transport-enabler/commits/master/enabler/src/de/schildbach/pte/AbstractNavitiaProvider.java

ialokim commented 5 years ago

It was still working with 2.0.4, so if it is was a change on PTE's side, it would have happened between Dec 9, 2018 and Jan 10, 2019.

ialokim commented 5 years ago

Now that is really strange: It still shows the paths as expected in 2.0.5-debug, but doesn't so in 2.0.5 from your testing repo?

Could there be some setting influencing this?

transportr_2

grote commented 5 years ago

No, there shouldn't be. If you build a release build from 2.0.5 source, does this change the picture?

ialokim commented 5 years ago

Yes, indeed. When building a release for 2.0.5, it behaves as the officially shipped version.

ialokim commented 5 years ago

And when adding

shrinkResources false
minifyEnabled false
multiDexEnabled true

to the release buildType it works again.

grote commented 5 years ago

Wow crazy s**t! Can you narrow it down to one of this option?

ialokim commented 5 years ago

Hum, was quite difficult... It works too with

//shrinkResources false
//minifyEnabled false
multiDexEnabled true

but it didn't only with minifyEnabled true. Anyways, it seems shrinkResources true requires minifyEnabled true which in turn requires multiDexEnabled true to build successfully.

ialokim commented 5 years ago

The release build for 2.0.0 still worked as expected.

ialokim commented 5 years ago

I have been checking it on another phone with the apks from F-Droid and it worked as expected until 2.0.3, but didn't so starting with 2.0.4. Probably you changed something in the proguard configuration between these versions?

ialokim commented 5 years ago

It is happening exactly after the PTE version switch from 4354c4a8 to c1c2b1e1 in d2b7d5e07eb11a28a35a01026144a92f552618e8.

Not sure if that has something to do with the problem, but there happened this change in the checksums file:

com.github.tony19:logback-android:1.1.1-12:logback-android-1.1.1-12.aar:3102228f0e408e3c003b34e96a604e9b9f59d314dcf8f03aa78d9d3648198932',
-        'com.gitlab.opentransitmap.public-transport-enabler:enabler:4354c4a8:enabler-4354c4a8.jar:1db5cfd805caad04c9728ea9456ac720ba1392a5b435e443e4f2dd65be70997b',
-        'com.gitlab.opentransitmap:public-transport-enabler:4354c4a8:public-transport-enabler-4354c4a8.jar:aab28b1a9798cb2c01a6ca2484949885a9653a068868d727e6d2f293cbbef32a',
+        'com.gitlab.opentransitmap:public-transport-enabler:c1c2b1e1:public-transport-enabler-c1c2b1e1.jar:fdcc25f2c8f6100e4e2bd7040130b427b893e29c0c2cf05c8c909a650ada269c',
         'com.google.android.apps.common.testing.accessibility.framework:accessibility-test-framework:2.0:accessibility-test-framework-2.0.jar:cdf16ef8f5b8023d003ce3cc1b0d51bda737762e2dab2fedf43d1c4292353f7f',

Perhaps PTE has been delivered in another fashion so that it didn't download two JARs anymore and perhaps that could have had some impact on the proguard configuration?

ialokim commented 5 years ago

Perhaps it could be linked to this change introducing a custom serialization for Trip.path?

ialokim commented 5 years ago

Indeed, it works now after adding

-keepclassmembers class * implements java.io.Serializable {
    private void writeObject(java.io.ObjectOutputStream);
    private void readObject(java.io.ObjectInputStream);
}

to the proguard rules. See https://www.guardsquare.com/en/products/proguard/manual/examples#serializable

grote commented 5 years ago

Wow, thanks a lot for this amazing detective work!

Crazy how throwing these methods out doesn't trigger any warning. Do you think we should restrict the proguard rule to the class in question or keep *?

ialokim commented 5 years ago

Hmmm I was wondering the same, but I think keeping * makes sense as these methods should never been excluded when they are overridden (as there is certainly a reason to implement them). Not sure though if keeping the rule like this or adding all methods proposed by proguard:

     private static final java.io.ObjectStreamField[] serialPersistentFields; 
     private void writeObject(java.io.ObjectOutputStream); 
     private void readObject(java.io.ObjectInputStream); 
     java.lang.Object writeReplace(); 
     java.lang.Object readResolve();

What's your opinion?

grote commented 5 years ago

Maybe they are overridden by a class that we don't need and can be purged?

grote commented 5 years ago

Reading the documentation properly, it seems we can just use the proguard ruled you propose.

ialokim commented 5 years ago

we can just use the proguard ruled you propose.

Okay, which one do you refer to? The first, minimal one or the second, complete one from the docs?

grote commented 5 years ago

If the minimal one fixes it, let's use this for now.