-
Notifications
You must be signed in to change notification settings - Fork 319
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
Follow up from #2452 #2458
Follow up from #2452 #2458
Conversation
… includes the fix
d619a1f
to
5068289
Compare
We're in Active Guidance though 🤔 |
@@ -101,7 +101,11 @@ object MapboxNativeNavigatorImpl : MapboxNativeNavigator { | |||
|
|||
// Offline | |||
|
|||
override fun configureRouter(routerParams: RouterParams, httpClient: HttpInterface): Long = | |||
override fun cacheLastRoute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we expose a NavigationOptions
boolean flag that determines whether we call this method or not after we set the route? We can set it up in a following PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you mean not exposing this method at all and add a flag to setRoute
(via NavigationOptions
) and control if cacheLastRoute
should be called or not? If so, yeah that's an option, we can also control that upstream in MapboxTripSession
/ MapboxNavigation
(MapboxTripSession
doesn't accept NavigationOptions
but I believe it's going to be needed at some point). Is that what you were thinking? In any case, we can follow up in a separate PR as you mentioned and not block this one 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of those works, we can either pass the options or call from upstream. Let's decide in the follow up 👍
…erpolation-SNAPSHOT-2
@@ -101,7 +101,11 @@ object MapboxNativeNavigatorImpl : MapboxNativeNavigator { | |||
|
|||
// Offline | |||
|
|||
override fun configureRouter(routerParams: RouterParams, httpClient: HttpInterface): Long = | |||
override fun cacheLastRoute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of those works, we can either pass the options or call from upstream. Let's decide in the follow up 👍
* make configure router http interface parameter nullable * add nn uncertain route state * expose nn cache last route api in mapbox navigator * remove lat long workaround not needed anymore as nn master-SNAPSHOT-0 includes the fix * bump mapbox-navigation-native version to ms-bearing-from-shape-on-interpolation-SNAPSHOT-2
…2452) * expose list of key points leading up to the enhanced location update * fix SimpleMapboxNavigation example * use env or project variables for bintray credentials * updated licenses * fix http client tests * bump mapboxMapSdk version to 9.0.0-20200207.215725-27 (mapbox/mapbox-gl-native-android#166) which includes fix for mapbox-gl-native mapbox/mapbox-gl-native#16180 crash * Follow up from #2452 (#2458) * make configure router http interface parameter nullable * add nn uncertain route state * expose nn cache last route api in mapbox navigator * remove lat long workaround not needed anymore as nn master-SNAPSHOT-0 includes the fix * bump mapbox-navigation-native version to ms-bearing-from-shape-on-interpolation-SNAPSHOT-2 * bumped maps sdk to the latest 9.1.0-SNAPSHOT * update MapboxTripSession tests * update SimpleMapboxNavigation example log level * remove explicit Bintray env variables declaration on CI Co-authored-by: Pablo Guardiola <guardiola31337@gmail.com>
Follow up from #2452
Addressing 👇
One thing that I noticed though is that we're still cutting corners 🤔
Will keep 👀
cc @mskurydin @zugaldia @andrewychen @Aurora-Boreal @LukasPaczos @SiarheiFedartsou