Add some flexibility to NavigationLocationProvider

[kmadsen]

Description

I recently investigated the -jvm-default compiler flag but it would cause a SEMVER.

There is a nice alternative approach, and that is default java interfaces. Right now we are stuck with kotlin interfaces that cannot change because they will create a java SEMVER. If a future kotlin version supports java-interop with kotlin interfaces, we can always go back and use it.

Kotlin is experimenting with ideas for generating default methods, meanwhile java interfaces are simple and stable.

This pull request demonstrates the approach with the LocationObserver and allows us to delete unused functions.

I'm also proposing we upgrade NavigationLocationProvider so it can be integrated with MapboxNavigationApp architecture.

Links to other issues and investigations

• [Android Auto] Backwards compatible interfaces using using jvm default mapbox-maps-android#1674
• [Android Auto] Make interfaces backwards compatible in java mapbox-maps-android#1670
• Add jvm-default #6335 (this is why we can't use -jvm-defaul yet)
• https://blog.jetbrains.com/kotlin/2020/07/kotlin-1-4-m3-generating-default-methods-in-interfaces/
• https://youtrack.jetbrains.com/issue/KT-4779/Generate-default-methods-for-implementations-in-interfaces
• https://youtrack.jetbrains.com/issue/KT-54239/-Xjvm-defaultall-for-sdk-development

Why now?

I'm looking to delete the CarAppLocation in :ui-androidauto:, if we update NavigationLocationProvider I could delete that code. I'm also looking to delete the need for this.

I'm also showing the default java interface approach by example @mapbox/navigation-android . There are a few interfaces that can benefit from this.

[kmadsen]

Thinking about making this final.. Leaving it open provides more flexibility but it's not covering a real use case.

Once it is open it cannot be closed, so am debating making it final. Maybe someone would want to intercept these events 🤔

[codecov]

Codecov Report

Merging #6420 (709748e) into main (8300e2f) will decrease coverage by 0.00%.
The diff coverage is 64.28%.

@@             Coverage Diff              @@
##               main    #6420      +/-   ##
============================================
- Coverage     69.28%   69.27%   -0.01%     
- Complexity     4664     4667       +3     
============================================
  Files           698      699       +1     
  Lines         27499    27512      +13     
  Branches       3213     3214       +1     
============================================
+ Hits          19052    19060       +8     
- Misses         7201     7206       +5     
  Partials       1246     1246              

Impacted Files	Coverage Δ
...navigation/core/trip/session/LocationObserver.java	0.00% <0.00%> (ø)
...ion/ui/maps/location/NavigationLocationProvider.kt	87.09% <75.00%> (-3.10%)	⬇️

[LukasPaczos]

Thanks for investigating this problem and idea! It looks like the proposed solution of using Java interfaces is rather robust and we're not sacrificing a lot, only default arguments won't be available (we'd have to write manual overloads) but that's a rarely used feature in the interfaces in the first place.

[LukasPaczos]

What's the use case for opening it up?

[kmadsen]

It makes it so you can access the MapboxNavigation.LocationObserver events.

Take a look at the FeedbackActivity.kt as an example

     private val navigationLocationProvider = object : NavigationLocationProvider() {
        // Will be triggered when the `NavigationLocationProvider` has been attached to `mapboxNavigation`
        override fun onNewLocationMatcherResult(locationMatcherResult: LocationMatcherResult) {
            super.onNewLocationMatcherResult(locationMatcherResult)
            updateCamera(locationMatcherResult.enhancedLocation)
        }
    }

    // The same as calling "registerLocationObserver"
    navigationLocationProvider.onAttached(mapboxNavigation)
    // MapboxNavitationApp.registerObserver(navigationLocationProvider) <== alternative to the line above
    // mapboxNavigation.registerLocationObserver(navigationLocationProvider) <== Also works.. 

There is a potential to do something like this.. But it's not the intention considering it is a class and not an interface (multiple inheritance issues)

class CustomNavigationCamera : NavigationLocationProvider() {

[LukasPaczos]

By implementing the interface directly, we're exposing it as part of the NavigationLocationProvider surface, while it's not needed. The LocationObserver instance could be a member of the NavigationLocationProvider instead.

[kmadsen]

It makes it so you can capture onNewLocationMatcherResult events downstream.

LocationConsumer does not give access to LocationMatcherResult. If it is internal, we need a another way to surface the map matched locations. Do you think there should be another registry of LocationObserver observers?

[dzinad]

There is a nice alternative approach, and that is default java interfaces.

There are also abstract classes. I know that with interfaces you can implement many, while you can inherit only from one class but it can also be an alternative for some cases.
Another concern is that if we often want to skip some implementations, we may be doing something wrong. I see the following options here:

1. Just make 2 interfaces. In this case: RawLocationObserver and EnhancedLocationObserver;
2. Provide a default no-op implementation that can be extended:

open class DefaultLocationObserver : LocationObserver {
    override fun onRawLocation(...) {
        // do nothing
    }
    override fun onMatchedLocationResult(...) {
        // do nothing
    }    
}

I understand that option 1 is not applicable here because we don't want to break the API, but I'm talking about approaches for new interfaces.

[dzinad]

How is that related to the method?

[kmadsen]

This function is called by MapboxNavigationApp when the object is registered by MapboxNavigationApp. It allows you to share a single instance.

But point taken, this method comment should at least give details about what this function does.

[kmadsen]

Moved this back to draft because I'd like to test the -jvm-default=all compiler flag again. Kotlin folks responded and the issue I was seeing has been fixed KT-54239

So I'm going to revisit this #6335