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

Create a delegate for MapboxNavigation called requireMapboxNavigation #6233

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

kmadsen
Copy link
Contributor

@kmadsen kmadsen commented Aug 25, 2022

Description

There is some consistent boilerplate required to setup the MapboxNavigationApp. I've just spent a bit of time migrating :libnavui-androidauto: to use the MapboxNavigationApp #6141 and I've explored a few approaches with the examples repository mapbox/mapbox-navigation-android-examples#129. We also discussed some ambiguous scenarios where we want to access MapboxNavigation directly and it requires an unsafe call MapboxNavigationApp.current()!!. #6226 (comment). This delegate removes the need for !! with specific guidelines.

This is my favorite approach so far. Create a delegate that hooks up MapboxNavigation to your LifecycleOwner. I migrated a random example from the qa-test-app to show what it looks like, take a look at the FeedbackActivity. EDIT migrated another example InactiveRouteStylingActivity.

Screenshots or Gifs

Inside any LifecycleOwner you can use the requireMapboxNavigation delegate like this. And then you have access to mapboxNavigation as if you constructed it inside.

Default can be used when [MapboxNavigationApp] is setup elsewhere.

val mapboxNavigation by requireMapboxNavigation()

Initialize the [MapboxNavigationApp] when you are ready to use it

val mapboxNavigation by requireMapboxNavigation {
  MapboxNavigationApp.setup(..)
}

Initialize and setup subscriptions

val mapboxNavigation by requireMapboxNavigation(
  onInitialize = {
    MapboxNavigationApp.setup(
      NavigationOptions.Builder(this)
          .accessToken(accessToken)
          .build()
    )
  },
  onResumedObserver = object : MapboxNavigationObserver {
    override fun onAttached(mapboxNavigation: MapboxNavigation) {
      mapboxNavigation.registerLocationObserver(locationObserver)
      mapboxNavigation.registerRoutesObserver(routesObserver)
    }
    override fun onDetached(mapboxNavigation: MapboxNavigation) {
      mapboxNavigation.unregisterLocationObserver(locationObserver)
      mapboxNavigation.unregisterRoutesObserver(routesObserver)
    }
  }
)

Another example

private val mapboxNavigation by requireMapboxNavigation(
  onCreatedObserver = object : MapboxNavigationObserver {
    override fun onAttached(mapboxNavigation: MapboxNavigation) {
      binding.mapView.location.apply {
        setLocationProvider(navigationLocationProvider)
        enabled = true
      }
      mapboxNavigation.registerLocationObserver(locationObserver)
      mapboxNavigation.registerRoutesObserver(routesObserver)
    }

    override fun onDetached(mapboxNavigation: MapboxNavigation) {
      mapboxNavigation.unregisterLocationObserver(locationObserver)
      mapboxNavigation.unregisterRoutesObserver(routesObserver)
    }
   }
) {
  MapboxNavigationApp.setup(
    NavigationOptions.Builder(this)
        .accessToken(Utils.getMapboxAccessToken(this))
        .build()
  )
}

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #6233 (8dbc73a) into main (847f8a9) will increase coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #6233      +/-   ##
============================================
+ Coverage     68.81%   68.84%   +0.02%     
- Complexity     4346     4348       +2     
============================================
  Files           649      650       +1     
  Lines         26183    26223      +40     
  Branches       3067     3075       +8     
============================================
+ Hits          18019    18052      +33     
- Misses         6998     7001       +3     
- Partials       1166     1170       +4     
Impacted Files Coverage Δ
...vigation/core/lifecycle/RequireMapboxNavigation.kt 75.00% <75.00%> (ø)
...x/navigation/core/lifecycle/MapboxNavigationApp.kt 20.00% <0.00%> (+10.00%) ⬆️

@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch from 7d11345 to 0f6a748 Compare August 25, 2022 22:10

override fun onStop() {
super.onStop()
mapboxNavigation.unregisterRouteProgressObserver(replayProgressObserver)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug. If you background the app it will unsubscribe. The subscribe happens in onCreate so the subscription is lost.

@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch 2 times, most recently from e92feb7 to e8c6b00 Compare August 26, 2022 00:25
@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch 4 times, most recently from 20250d4 to 254b936 Compare August 30, 2022 17:07
@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch from 027af78 to 1d63ae9 Compare August 31, 2022 16:45
@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch 3 times, most recently from 64e6ff6 to 13785bf Compare September 1, 2022 19:41
mapboxNavigation.unregisterLocationObserver(locationObserver)
mapboxNavigation.stopTripSession()
}
private val mapboxNavigation by requireMapboxNavigation(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This never loads unless you reference it. The lazy load can be confusing.

Also, the onCreate callback from activity happens before the Lifecycle onCreate. That causes a crash if you try to reference this delegate without using Lifecycle callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it but was finding another race condition along the way.

mapboxNavigation.setRoutes(listOf(route))
binding.startNavigation.visibility = View.GONE
startSimulation(mapboxNavigation.getRoutes()[0]) <=== crashes because routes are not set yet

@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch from 8a8286f to aa87a7d Compare September 6, 2022 16:57
@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch from aa87a7d to 6234305 Compare September 6, 2022 18:22
@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch from 6234305 to b145c53 Compare September 6, 2022 18:23
@kmadsen kmadsen force-pushed the km-create-navigation-required-delegate branch from 0b3b832 to 8dbc73a Compare September 6, 2022 18:53
@kmadsen kmadsen merged commit e3e78f4 into main Sep 6, 2022
@kmadsen kmadsen deleted the km-create-navigation-required-delegate branch September 6, 2022 20:06
Guardiola31337 pushed a commit that referenced this pull request Dec 22, 2022
…ion` (#6233)

* Create a delegate for MapboxNavigation

* Make requireMapboxNavigation attach during initialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants