Remove experimental from MapboxNavigationApp

[kmadsen]

Description

In order to create stable apis for Android Auto and DropInUi, the MapboxNavigationApp framework needs to also be stable. We have also reached a point where it would cause many issues downstream if changes are not backwards compatible.

This also deprecates MapboxNavigationProvider. Adopting MapboxNavigationApp will not break any references to MapboxNavigationProvider because it is used by MapboxNavigationOwner.

cc: @mapbox/navigation-android

[codecov]

Codecov Report

Merging #6143 (0b6453c) into main (253b03a) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #6143      +/-   ##
============================================
- Coverage     68.90%   68.89%   -0.01%     
  Complexity     4255     4255              
============================================
  Files           639      639              
  Lines         25646    25643       -3     
  Branches       3003     3003              
============================================
- Hits          17671    17668       -3     
  Misses         6828     6828              
  Partials       1147     1147              

Impacted Files	Coverage Δ
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	67.66% <ø> (ø)
...mapbox/navigation/core/MapboxNavigationProvider.kt	45.45% <ø> (ø)
.../navigation/core/lifecycle/CarAppLifecycleOwner.kt	83.69% <ø> (-0.18%)	⬇️
...x/navigation/core/lifecycle/MapboxNavigationApp.kt	0.00% <ø> (ø)
...tion/core/lifecycle/MapboxNavigationAppDelegate.kt	91.66% <ø> (-0.23%)	⬇️
...navigation/core/lifecycle/MapboxNavigationOwner.kt	89.47% <ø> (-0.27%)	⬇️

[abhishek1508]

Nice!! Glad to see we are moving towards stability.

[dzinad]

Is there a ticket to update all of our examples?

[Zayankovsky]

Should we somehow discourage users from instantiating MapboxNavigation via its constructor?

[LukasPaczos]

🚢

Should we somehow discourage users from instantiating MapboxNavigation via its constructor?

A small problem I see with the MapboxNavigationApp is that it's somewhat inherently async. If there's a use-case that would benefit from synchronously getting access to MapboxNavigation, it might be inconvenient to use. Unless MapboxNavigationApp#current guarantees a synchronous access after MapboxNavigationApp#setup? I haven't analyzed the order of lifecycle observer calls to confirm that. If we can provide a good synchronous access, then we should be in a position to deprecate the primary MapboxNavigation constructor.

[tomaszrybakiewicz]

Additionally to deprecating, maybe we should update all MapboxNavigaitonProvider methods to call MapboxNavigationApp? WDYT?

[kmadsen]

Thanks for comments! Responses are below the comment links.

#6143 (comment) Is there a ticket to update all of our examples?

Not yet, we need a v2.8.0 migration guide. MapboxNavigationApp will be stable as of the v2.8.0 release train, which is currently in alpha. Many apis that make the migration easier are experimental or are built to support deprecated APIs. Declaring stability will help resolve opinions on the details of the migration.

#6143 (comment) Should we somehow discourage users from instantiating MapboxNavigation via its constructor?

This topic has come up before, we are discouraging it. it would also be a SEMVER to prohibit it strictly. I think we are being careful to decide if the public constructor should deprecated. It does add support for a simple synchronous construction as lukas mentioned.

#6143 (comment) MapboxNavigationApp is that it's somewhat inherently async. If there's a use-case that would benefit from synchronously getting access to MapboxNavigation, it might be inconvenient to use.

Adding to this. We are discouraging synchronous creation of MapboxNavigation because it has a lifecycle. But, it is still possible. You can create MapboxNavigation with MapboxNavigationApp synchronously using this specific call order.

mapboxNavigation = MapboxNavigationApp.setup(navigationOptions).attach(lifecycleOwner).current()!!

lifecycleOwner must be in the CREATED state or else current()!! will throw a NPE. This means you can do this in Activity.onStart but you cannot do it in Activity.init or Activity.onCreate because the lifecycle owner is still in the INITIALIZED state. This encourages the use of MapboxNavigationApp.registerObserver.

#6143 (comment) Additionally to deprecating, maybe we should update all MapboxNavigaitonProvider methods to call MapboxNavigationApp? WDYT?

We need to support MapboxNavigationProvider functionality for all the 2.x releases. Maybe there is another solution to maintain backwards compatibility, but I found a pretty straightforward solution awhile ago and haven't found a reason to update it. MapboxNavigationApp relies on MapboxNavigationProvider so that we can maintain backwards compatibility.

Any changes to MapboxNavigationProvider will risk a regression so I think it's best to leave it alone.

[kmadsen]

This had some pretty bad consequences. Considering onDestroy will remove all registered listener

When using MapboxNavigationApp, this recreation can be done and listeners can be re-registered in MapboxNavigationObserver.onAttached

[VysotskiVadim]

@kmadsen , do you suggest using MapboxNavigationApp in the examples?

[abhishek1508]

@kmadsen , do you suggest using MapboxNavigationApp in the examples?

This is what we are aiming for. There would be a separate ticket that would change the instantiation of mapboxNavigation using MapboxNavigationApp in all the existing examples.

[kmadsen]

We are not entirely ready to migrate all examples (the release is still in alpha). We could migrate the examples now, but there are many helpful extensions we should consider that will makes migration simpler.

Here is a tracker for it though mapbox/mapbox-navigation-android-examples#128