Skip to content
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

NN-843 #7349

Merged
merged 8 commits into from
Jul 7, 2023
Merged

NN-843 #7349

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/unreleased/issues/changes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- In case an app provides custom `Router` implementation to Nav SDK, the Nav SDK doesn't trigger `NavigationRouteAlternativesObserver` with online alternatives when current route is offline.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it really so? I thought it was more like "it's triggered, but much later". The alternatives mechanism is still working, right? we just won't trigger an alternative request as soon as the network is back.

Copy link
Contributor

Choose a reason for hiding this comment

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

@averkhaturau, can you please clarify consequences of using custom router for offline-online switch?

Choose a reason for hiding this comment

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

NN knows which router is online only if it creates the routers itself. Providing a custom router makes this information unavailable, we don't know how to create an online router anymore. That's why the back online feature will not be available with a custom router

2 changes: 1 addition & 1 deletion gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ ext {
// version which we should use in this build
def mapboxNavigatorVersion = System.getenv("FORCE_MAPBOX_NAVIGATION_NATIVE_VERSION")
if (mapboxNavigatorVersion == null || mapboxNavigatorVersion == '') {
mapboxNavigatorVersion = '142.0.0'
mapboxNavigatorVersion = '143.0.0'
}
println("Navigation Native version: " + mapboxNavigatorVersion)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import kotlinx.coroutines.flow.first
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Ignore
import org.junit.Rule
import org.junit.Test

Expand Down Expand Up @@ -67,7 +66,6 @@ class EvOfflineTest : BaseCoreNoCleanUpTest() {
}

@Test
@Ignore("NN-843")
fun startNavigationOfflineThenSwitchToOnlineRouteWhenInternetAppears() = sdkTest(
timeout = INCREASED_TIMEOUT_BECAUSE_OF_REAL_ROUTING_TILES_USAGE
) {
Expand Down Expand Up @@ -178,6 +176,7 @@ class EvOfflineTest : BaseCoreNoCleanUpTest() {
navigation.registerRouteAlternativesObserver(
SimpleAlternativesObserverFromDocumentation(navigation)
)

navigation.startTripSession()
stayOnPosition(
evBerlinTestRoute.origin.latitude(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,11 @@ class MapboxNavigation @VisibleForTesting internal constructor(
),
navigationOptions.accessToken ?: "",
if (moduleRouter.isInternalImplementation()) {
nativeRouter
// We pass null to let NN know that default router is used and it can rely
// on implementation details in some cases like offline-online switch.
// Meanwhile platform SDK uses its own instance of native router for
// route requests
null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also pass nativeRouter in recreate method. It should also be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice one! fixing in a followup PR

Copy link
Contributor

Choose a reason for hiding this comment

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

} else {
RouterInterfaceAdapter(moduleRouter, ::getNavigationRoutes)
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ internal object NavigationComponentProvider {
historyRecorderComposite: HistoryRecorderHandle?,
tilesConfig: TilesConfig,
accessToken: String,
router: RouterInterface,
router: RouterInterface?,
): MapboxNativeNavigator = MapboxNativeNavigatorImpl.create(
config,
historyRecorderComposite,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import com.mapbox.navigation.base.route.RouterCallback
import com.mapbox.navigation.base.route.RouterFailure
import com.mapbox.navigation.base.route.RouterOrigin.Offboard
import com.mapbox.navigation.base.route.RouterOrigin.Onboard
import com.mapbox.navigation.navigator.internal.MapboxNativeNavigator
import com.mapbox.navigation.navigator.internal.mapToRoutingMode
import com.mapbox.navigation.route.internal.util.ACCESS_TOKEN_QUERY_PARAM
import com.mapbox.navigation.route.internal.util.TestRouteFixtures
Expand Down Expand Up @@ -74,7 +73,6 @@ class RouterWrapperTests {
var coroutineRule = MainCoroutineRule()

private lateinit var routerWrapper: RouterWrapper
private val mapboxNativeNavigator: MapboxNativeNavigator = mockk(relaxed = true)
private val router: RouterInterface = mockk(relaxed = true)
private val accessToken = "pk.123"
private val route: DirectionsRoute = mockk(relaxed = true)
Expand Down Expand Up @@ -145,7 +143,6 @@ class RouterWrapperTests {
every { ThreadController.IODispatcher } returns coroutineRule.testDispatcher
every { ThreadController.DefaultDispatcher } returns coroutineRule.testDispatcher

every { mapboxNativeNavigator.router } returns router
every { router.getRoute(any(), any(), capture(getRouteSlot)) } returns 0L
every { router.getRouteRefresh(any(), capture(refreshRouteSlot)) } returns 0L

Expand All @@ -155,7 +152,7 @@ class RouterWrapperTests {

routerWrapper = RouterWrapper(
accessToken,
mapboxNativeNavigator.router,
router,
ThreadController(),
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ interface MapboxNativeNavigator {
historyRecorderComposite: HistoryRecorderHandle?,
tilesConfig: TilesConfig,
accessToken: String,
router: RouterInterface,
router: RouterInterface?,
): MapboxNativeNavigator

/**
Expand Down Expand Up @@ -192,6 +192,4 @@ interface MapboxNativeNavigator {
val roadObjectMatcher: RoadObjectMatcher?

val experimental: Experimental

val router: RouterInterface
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ object MapboxNativeNavigatorImpl : MapboxNativeNavigator {
override var roadObjectsStore: RoadObjectsStore? = null
override lateinit var experimental: Experimental
override lateinit var cache: CacheHandle
override lateinit var router: RouterInterface
override lateinit var routeAlternativesController: RouteAlternativesControllerInterface
private val nativeNavigatorRecreationObservers =
CopyOnWriteArraySet<NativeNavigatorRecreationObserver>()
Expand All @@ -83,7 +82,7 @@ object MapboxNativeNavigatorImpl : MapboxNativeNavigator {
historyRecorderComposite: HistoryRecorderHandle?,
tilesConfig: TilesConfig,
accessToken: String,
router: RouterInterface,
router: RouterInterface?,
): MapboxNativeNavigator {
navigator?.shutdown()

Expand All @@ -99,7 +98,6 @@ object MapboxNativeNavigatorImpl : MapboxNativeNavigator {
roadObjectsStore = nativeComponents.navigator.roadObjectStore()
experimental = nativeComponents.navigator.experimental
cache = nativeComponents.cache
this.router = nativeComponents.router
routeAlternativesController = nativeComponents.routeAlternativesController
this.accessToken = accessToken
return this
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ object NavigatorLoader {
config: ConfigHandle,
historyRecorderComposite: HistoryRecorderHandle?,
tilesConfig: TilesConfig,
router: RouterInterface,
router: RouterInterface?,
): NativeComponents {
val cache = CacheFactory.build(tilesConfig, config, historyRecorderComposite)
val navigator = Navigator(
Expand All @@ -69,7 +69,6 @@ object NavigatorLoader {
graphAccessor,
cache,
roadObjectMatcher,
router,
navigator.routeAlternativesController,
)
}
Expand Down Expand Up @@ -149,7 +148,6 @@ object NavigatorLoader {
val graphAccessor: GraphAccessor,
val cache: CacheHandle,
val roadObjectMatcher: RoadObjectMatcher,
val router: RouterInterface,
val routeAlternativesController: RouteAlternativesControllerInterface,
)
}