Skip to content

fixed wrong casing color for alternative route lines and added tests #3458

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
Merged
cafesilencio merged 1 commit into from
Aug 24, 2020
Merged

fixed wrong casing color for alternative route lines and added tests #3458

cafesilencio merged 1 commit into from
Aug 24, 2020

Conversation

@cafesilencio
Copy link

@cafesilencio cafesilencio commented Aug 18, 2020

Description

fixes #3446 by restoring the alternative route casing color

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Goal

Implementation

Screenshots or Gifs

Testing

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR
  • We might need to update / push api/current.txt files after running $> make core-update-api (Core) / $> make ui-update-api (UI) if there are changes / errors we're 🆗 with (e.g. AddedMethod changes are marked as errors but don't break SemVer) 🚀 If there are SemVer breaking changes add the SEMVER label. See Metalava docs

@cafesilencio cafesilencio requested a review from a team August 18, 2020 21:39
@LukasPaczos LukasPaczos added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Aug 19, 2020
Guardiola31337
Guardiola31337 reviewed Aug 19, 2020
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

This is crashing when running the test app 👀

08-19 10:25:24.039 21278-21278/com.mapbox.navigation.examples E/Mbgl-MapChangeReceiver: Exception in onDidFinishLoadingStyle
    java.lang.AbstractMethodError: abstract method "com.mapbox.mapboxsdk.style.expressions.Expression com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProvider.getCasingLineWidthExpression(float)"
        at com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProvider$DefaultImpls.initializeAlternativeRouteCasingLayer(MapboxRouteLayerProvider.kt:162)
        at com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProviderFactory$getLayerProvider$1.initializeAlternativeRouteCasingLayer(MapboxRouteLayerProviderFactory.kt:19)
        at com.mapbox.navigation.ui.route.MapRouteLine.initializeLayers(MapRouteLine.kt:614)
        at com.mapbox.navigation.ui.route.MapRouteLine.<init>(MapRouteLine.kt:394)
        at com.mapbox.navigation.ui.route.MapRouteLine.<init>(MapRouteLine.kt:105)
        at com.mapbox.navigation.ui.route.NavigationMapRoute.buildMapRouteLine(NavigationMapRoute.java:353)
        at com.mapbox.navigation.ui.route.NavigationMapRoute.<init>(NavigationMapRoute.java:114)
        at com.mapbox.navigation.ui.route.NavigationMapRoute.<init>(NavigationMapRoute.java:52)
        at com.mapbox.navigation.ui.route.NavigationMapRoute$Builder.build(NavigationMapRoute.java:599)
        at com.mapbox.navigation.ui.map.NavigationMapboxMap.initializeRoute(NavigationMapboxMap.java:901)
        at com.mapbox.navigation.ui.map.NavigationMapboxMap.<init>(NavigationMapboxMap.java:183)
        at com.mapbox.navigation.ui.map.NavigationMapboxMap.<init>(NavigationMapboxMap.java:140)
        at com.mapbox.navigation.examples.core.SimpleMapboxNavigationKt$onMapReady$2.onStyleLoaded(SimpleMapboxNavigationKt.kt:186)
        at com.mapbox.mapboxsdk.maps.MapboxMap.notifyStyleLoaded(MapboxMap.java:963)
        at com.mapbox.mapboxsdk.maps.MapboxMap.onFinishLoadingStyle(MapboxMap.java:225)
        at com.mapbox.mapboxsdk.maps.MapView$MapCallback.onDidFinishLoadingStyle(MapView.java:1344)
        at com.mapbox.mapboxsdk.maps.MapChangeReceiver.onDidFinishLoadingStyle(MapChangeReceiver.java:198)
        at com.mapbox.mapboxsdk.maps.NativeMapView.onDidFinishLoadingStyle(NativeMapView.java:1159)
        at android.os.MessageQueue.nativePollOnce(Native Method)
        at android.os.MessageQueue.next(MessageQueue.java:323)
        at android.os.Looper.loop(Looper.java:135)
        at android.app.ActivityThread.main(ActivityThread.java:5417)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err: java.lang.AbstractMethodError: abstract method "com.mapbox.mapboxsdk.style.expressions.Expression com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProvider.getCasingLineWidthExpression(float)"
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProvider$DefaultImpls.initializeAlternativeRouteCasingLayer(MapboxRouteLayerProvider.kt:162)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProviderFactory$getLayerProvider$1.initializeAlternativeRouteCasingLayer(MapboxRouteLayerProviderFactory.kt:19)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.MapRouteLine.initializeLayers(MapRouteLine.kt:614)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.MapRouteLine.<init>(MapRouteLine.kt:394)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.MapRouteLine.<init>(MapRouteLine.kt:105)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.NavigationMapRoute.buildMapRouteLine(NavigationMapRoute.java:353)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.NavigationMapRoute.<init>(NavigationMapRoute.java:114)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.NavigationMapRoute.<init>(NavigationMapRoute.java:52)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.route.NavigationMapRoute$Builder.build(NavigationMapRoute.java:599)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.map.NavigationMapboxMap.initializeRoute(NavigationMapboxMap.java:901)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.map.NavigationMapboxMap.<init>(NavigationMapboxMap.java:183)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.ui.map.NavigationMapboxMap.<init>(NavigationMapboxMap.java:140)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.navigation.examples.core.SimpleMapboxNavigationKt$onMapReady$2.onStyleLoaded(SimpleMapboxNavigationKt.kt:186)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.mapboxsdk.maps.MapboxMap.notifyStyleLoaded(MapboxMap.java:963)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.mapboxsdk.maps.MapboxMap.onFinishLoadingStyle(MapboxMap.java:225)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.mapboxsdk.maps.MapView$MapCallback.onDidFinishLoadingStyle(MapView.java:1344)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.mapboxsdk.maps.MapChangeReceiver.onDidFinishLoadingStyle(MapChangeReceiver.java:198)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.mapbox.mapboxsdk.maps.NativeMapView.onDidFinishLoadingStyle(NativeMapView.java:1159)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at android.os.MessageQueue.nativePollOnce(Native Method)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at android.os.MessageQueue.next(MessageQueue.java:323)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at android.os.Looper.loop(Looper.java:135)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at android.app.ActivityThread.main(ActivityThread.java:5417)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at java.lang.reflect.Method.invoke(Native Method)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
08-19 10:25:24.040 21278-21278/com.mapbox.navigation.examples W/System.err:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
    
    --------- beginning of crash
08-19 10:25:24.041 21278-21278/com.mapbox.navigation.examples A/libc: /usr/local/google/buildbot/src/android/ndk-release-r21/external/libcxx/../../external/libcxxabi/src/abort_message.cpp:72: abort_message: assertion "terminating with uncaught exception of type jni::PendingJavaException" failed
08-19 10:25:24.041 21278-21278/com.mapbox.navigation.examples A/libc: Fatal signal 6 (SIGABRT), code -6 in tid 21278 (gation.examples)
08-19 10:25:24.142 198-198/? A/DEBUG: *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
08-19 10:25:24.142 198-198/? A/DEBUG: Build fingerprint: 'google/hammerhead/hammerhead:6.0.1/M4B30Z/3437181:user/release-keys'
08-19 10:25:24.142 198-198/? A/DEBUG: Revision: '11'
08-19 10:25:24.143 198-198/? A/DEBUG: ABI: 'arm'
08-19 10:25:24.143 198-198/? A/DEBUG: pid: 21278, tid: 21278, name: gation.examples  >>> com.mapbox.navigation.examples <<<
08-19 10:25:24.143 198-198/? A/DEBUG: signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
08-19 10:25:24.184 198-198/? A/DEBUG: Abort message: '/usr/local/google/buildbot/src/android/ndk-release-r21/external/libcxx/../../external/libcxxabi/src/abort_message.cpp:72: abort_message: assertion "terminating with uncaught exception of type jni::PendingJavaException" failed'
08-19 10:25:24.184 198-198/? A/DEBUG:     r0 00000000  r1 0000531e  r2 00000006  r3 b6fbcb7c
08-19 10:25:24.184 198-198/? A/DEBUG:     r4 b6fbcb84  r5 b6fbcb34  r6 0000000b  r7 0000010c
08-19 10:25:24.185 198-198/? A/DEBUG:     r8 00000000  r9 00000000  sl bec4dcc0  fp b3b3e200
08-19 10:25:24.185 198-198/? A/DEBUG:     ip 00000006  sp bec4d6c0  lr b6d2bb61  pc b6d2df50  cpsr 400f0010
08-19 10:25:24.202 198-198/? A/DEBUG: backtrace:
08-19 10:25:24.202 198-198/? A/DEBUG:     #00 pc 00041f50  /system/lib/libc.so (tgkill+12)
08-19 10:25:24.202 198-198/? A/DEBUG:     #01 pc 0003fb5d  /system/lib/libc.so (pthread_kill+32)
08-19 10:25:24.202 198-198/? A/DEBUG:     #02 pc 0001c30f  /system/lib/libc.so (raise+10)
08-19 10:25:24.202 198-198/? A/DEBUG:     #03 pc 000194c1  /system/lib/libc.so (__libc_android_abort+34)
08-19 10:25:24.202 198-198/? A/DEBUG:     #04 pc 000174ac  /system/lib/libc.so (abort+4)
08-19 10:25:24.203 198-198/? A/DEBUG:     #05 pc 0001af23  /system/lib/libc.so (__libc_fatal+16)
08-19 10:25:24.203 198-198/? A/DEBUG:     #06 pc 00019549  /system/lib/libc.so (__assert2+20)
08-19 10:25:24.203 198-198/? A/DEBUG:     #07 pc 0027c3ab  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #08 pc 0027c4cb  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #09 pc 0027abfd  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #10 pc 0027a4ab  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #11 pc 0027a473  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so (__cxa_throw+74)
08-19 10:25:24.203 198-198/? A/DEBUG:     #12 pc 00053bed  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #13 pc 0006c559  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #14 pc 000c289d  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #15 pc 0014cda7  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #16 pc 0014c99f  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #17 pc 0014e871  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.203 198-198/? A/DEBUG:     #18 pc 00195339  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.204 198-198/? A/DEBUG:     #19 pc 000ae3c7  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.204 198-198/? A/DEBUG:     #20 pc 0019024f  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.204 198-198/? A/DEBUG:     #21 pc 0018fcb5  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.204 198-198/? A/DEBUG:     #22 pc 0018fd43  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.204 198-198/? A/DEBUG:     #23 pc 0018f21f  /data/app/com.mapbox.navigation.examples-1/lib/arm/libmapbox-gl.so
08-19 10:25:24.204 198-198/? A/DEBUG:     #24 pc 00012e93  /system/lib/libutils.so (_ZN7android6Looper9pollInnerEi+530)
08-19 10:25:24.204 198-198/? A/DEBUG:     #25 pc 00012f63  /system/lib/libutils.so (_ZN7android6Looper8pollOnceEiPiS1_PPv+130)
08-19 10:25:24.204 198-198/? A/DEBUG:     #26 pc 00081d05  /system/lib/libandroid_runtime.so (_ZN7android18NativeMessageQueue8pollOnceEP7_JNIEnvP8_jobjecti+22)
08-19 10:25:24.204 198-198/? A/DEBUG:     #27 pc 72e6d56d  /data/dalvik-cache/arm/system@framework@boot.oat (offset 0x1ed6000)

@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Aug 19, 2020

Crash is easily reproducible, it happens at startup of any example in the test app.

@cafesilencio cafesilencio force-pushed the sb-3446-alternative-route-casing-color branch from 3f7fdd6 to 667b88e Compare August 19, 2020 21:36
@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Aug 19, 2020

Crash is easily reproducible, it happens at startup of any example in the test app.

After latest push rebasing from master I can't reproduce anymore.

One thing that I noticed though is that interestingly the casing is working for alternatives but not for the primary route (refs. #3472) 👀

casing_not_visible_in_primary_but_visible_in_alternatives

After discussing with @cafesilencio confirming that ☝️ is an issue with the gradient. As soon as you enable gradient the casing is not visible (for alternatives there's no gradient by default) so #3472 is still valid as a workaround until we get a fix from Maps.

@cafesilencio cafesilencio force-pushed the sb-3446-alternative-route-casing-color branch 2 times, most recently from 5f55d67 to 91c1132 Compare August 19, 2020 22:27
Guardiola31337
Guardiola31337 reviewed Aug 19, 2020
View reviewed changes
...igation-ui/src/main/java/com/mapbox/navigation/ui/internal/route/MapboxRouteLayerProvider.kt
fun getRouteLineColorExpressions(defaultColor: Int): List<Expression> {
fun getRouteLineColorExpressions(
defaultColor: Int,
routeColorProvider: KProperty1<RouteStyleDescriptor, Int>
Copy link
Contributor

@Guardiola31337 Guardiola31337 Aug 19, 2020

Choose a reason for hiding this comment

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

Do we really need KProperty1? reflect in general is scary. Also I'm wondering if that may introduce problems with Java compatibility as it's a Kotlin type 👀 at the ⚠️ https://kotlinlang.org/docs/reference/reflection.html#reflection

Copy link
Author

@cafesilencio cafesilencio Aug 19, 2020

Choose a reason for hiding this comment

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

This saves from writing a lot of duplicate code in this case and the class is internal now so nobody external to the team will be accessing it.

Copy link
Contributor

@Guardiola31337 Guardiola31337 Aug 19, 2020

Choose a reason for hiding this comment

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

Ohh but we are as most of the UI codebase is in Java 😬

Copy link
Contributor

@Guardiola31337 Guardiola31337 Aug 19, 2020

Choose a reason for hiding this comment

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

Only wondering about the implications of depending on reflect.

Copy link
Author

@cafesilencio cafesilencio Aug 21, 2020

Choose a reason for hiding this comment

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

KProperty1 is just an interface so a parameter like RouteStyleDescriptor::lineColorResourceId can be referred to. When you call ::myFunction() you're using a KFunction. We do this all over the code base.

...avigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapboxRouteLayerProviderFactory.kt Outdated
package com.mapbox.navigation.ui.route

import com.mapbox.navigation.ui.route.RouteStyleDescriptor
import com.mapbox.navigation.ui.internal.route.MapboxRouteLayerProvider
Copy link
Contributor

@Guardiola31337 Guardiola31337 Aug 19, 2020

Choose a reason for hiding this comment

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

Can MapboxRouteLayerProvider be move out of the internal package and have the internal Kotlin modifier too?

@cafesilencio cafesilencio force-pushed the sb-3446-alternative-route-casing-color branch from 91c1132 to f081a37 Compare August 19, 2020 22:29
Guardiola31337
Guardiola31337 reviewed Aug 19, 2020
View reviewed changes
...src/test/java/com/mapbox/navigation/ui/internal/route/MapboxRouteLayerProviderFactoryTest.kt Outdated
@@ -1,5 +1,6 @@
package com.mapbox.navigation.ui.internal.route

import com.mapbox.navigation.ui.route.MapboxRouteLayerProviderFactory
Copy link
Contributor

@Guardiola31337 Guardiola31337 Aug 19, 2020

Choose a reason for hiding this comment

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

Can MapboxRouteLayerProviderFactoryTest test be move out of the internal package too?

...ion-ui/src/test/java/com/mapbox/navigation/ui/internal/route/MapboxRouteLayerProviderTest.kt Outdated
@@ -1,5 +1,6 @@
package com.mapbox.navigation.ui.internal.route

import com.mapbox.navigation.ui.route.MapboxRouteLayerProvider
Copy link
Contributor

@Guardiola31337 Guardiola31337 Aug 19, 2020

Choose a reason for hiding this comment

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

Same as above re: move the test out of the internal package

Copy link
Author

@cafesilencio cafesilencio Aug 19, 2020

Choose a reason for hiding this comment

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

done

@cafesilencio cafesilencio force-pushed the sb-3446-alternative-route-casing-color branch from f081a37 to 467bc19 Compare August 19, 2020 22:34
@cafesilencio cafesilencio merged commit 6594a90 into master Aug 24, 2020
@cafesilencio cafesilencio deleted the sb-3446-alternative-route-casing-color branch August 24, 2020 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Alternative routes casing color regression

3 participants

@cafesilencio @Guardiola31337 @LukasPaczos