mapbox-navigation-android
mapbox-navigation-android copied to clipboard
Add some flexibility to NavigationLocationProvider
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.
Codecov Report
Merging #6420 (709748e) into main (8300e2f) will decrease coverage by
0.00%
. The diff coverage is64.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%) |
:arrow_down: |
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:
- Just make 2 interfaces. In this case: RawLocationObserver and EnhancedLocationObserver;
- 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.
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