mapbox-navigation-android icon indicating copy to clipboard operation
mapbox-navigation-android copied to clipboard

Add some flexibility to NavigationLocationProvider

Open kmadsen opened this issue 2 years ago • 3 comments

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

  • https://github.com/mapbox/mapbox-maps-android/pull/1674
  • https://github.com/mapbox/mapbox-maps-android/pull/1670
  • https://github.com/mapbox/mapbox-navigation-android/pull/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 avatar Sep 30 '22 00:09 kmadsen

Codecov Report

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

Impacted file tree graph

@@             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%) :arrow_down:

codecov[bot] avatar Sep 30 '22 01:09 codecov[bot]

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 avatar Oct 03 '22 19:10 dzinad

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 https://github.com/mapbox/mapbox-navigation-android/pull/6335

kmadsen avatar Oct 05 '22 21:10 kmadsen