[Android Auto] Start declaring apis with java interop #6292
Conversation
kmadsen
added
the
Android Auto
kmadsen
force-pushed
the
km-androidauto-java-interop
branch
2 times, most recently
from
be8dbbd to
49942ec
Compare
kmadsen
force-pushed
the
km-androidauto-java-interop
branch
from
f2b63c9 to
83db7ec
Compare
kmadsen
force-pushed
the
km-androidauto-java-interop
branch
from
83db7ec to
25a0c4a
Compare
Codecov Report
@@ Coverage Diff @@
## main #6292 +/- ##
============================================
- Coverage 68.89% 68.85% -0.04%
+ Complexity 4454 4453 -1
============================================
Files 668 668
Lines 26688 26690 +2
Branches 3147 3147
============================================
- Hits 18387 18378 -9
- Misses 7093 7104 +11
Partials 1208 1208
|
| @@ -6,6 +6,7 @@ Mapbox welcomes participation and contributions from everyone. | |||
| #### Features | |||
| #### Bug fixes and improvements | |||
| - Optimized calls to modify route arrow layer visibility. [#6308](https://github.com/mapbox/mapbox-navigation-android/pull/6308) | |||
| - Provide better java support for `MapboxNavigationApp`. [#6292](https://github.com/mapbox/mapbox-navigation-android/pull/6292) | |||
There was a problem hiding this comment.
should we automate checks if any of public objects' functions or public static functions don't have java interrupt @kmadsen ?
There was a problem hiding this comment.
Hmm I don't know I just lost some confidence in the @JvmStatic https://mapbox.atlassian.net/browse/NAVSDK-544
https://issuetracker.google.com/issues/158393309
java.lang.IncompatibleClassChangeError: The method 'com.mapbox.navigation.core.lifecycle.MapboxNavigationApp com.mapbox.navigation.core.lifecycle.MapboxNavigationApp.registerObserver(com.mapbox.navigation.core.lifecycle.MapboxNavigationObserver)' was expected to be of type virtual but instead was found to be of type static (declaration of 'com.mapbox.androidauto.MapboxCarApp' appears in /data/app/~~wWRVVkoksDjAWyl1zcYreA==/com.mapbox.navigation.qa_test_app-grYQDy8r9n-vE5Z1qLuR0Q==/base.apk!classes5.dex)
It changes the method signature so any changes will risk SEMVER
There was a problem hiding this comment.
2.8 is the first stable version for MapboxNavigationApp so this could be fine. But for other functions that have had previous stable versions - it will change the method signature.
There was a problem hiding this comment.
The crash looks like it should be fixed by #6316.
| * Java interoperability. Provides access to any registered observer instance. If multiple | ||
| * instances of the same class have been registered, the first observer will be returned. | ||
| * | ||
| * @throws IllegalStateException when the class has not been registered. |
There was a problem hiding this comment.
Don't we want to add @Throws annotation? It provides better java interoperability. Although in this case it doesn't matter since IllegalStateException is unchecked. However, I think it's a good pattern.
| } | ||
| } | ||
|
|
||
| interface Consumer<T> { |
There was a problem hiding this comment.
There is java.util.function.Consumer, why do you need this one?
There was a problem hiding this comment.
Didn't use it because it requires api 24
Description
I'm taking an idea from the maps repository. There is a CarJavaInterfaceChecker which helps verify interfaces with java.
We are declaring stable apis https://github.com/mapbox/navigation-sdks/issues/1904
There has also been a request for java mapbox/mapbox-navigation-android-examples#139.
So it is best we get started with defining apis that work will work with java.