[Android Auto] Make interfaces backwards compatible in java #1670

kmadsen · 2022-09-14T17:57:01Z

Summary of changes

We would like to release a stable api for this AA extension library. In order to create a stable api we also prefer solutions that are flexible for handling future changes.

Making the gesture handler capable of adding the onClick function in the future without breaking SEMVER #1604

We don't have a solution at the moment, that allows downstream to add the onClick without an extension upgrade. It would be nice to have an implementation that allowed for extension, but the Jetpack car library is difficult in this regard. The library only allows for a single SurfaceCallback, if it allowed for multiple this would be easy. Here is a ticket where we asked Google to add this flexibility https://issuetracker.google.com/u/0/issues/200592099. There may be another solution here, but I'm only imaging something with reflection.

The other solution, is for Mapbox to release a beta channel. The AA beta channel could support for the AA beta apis.

For now, this pull request simply makes it so there is better java backwards compatibility in the mapbox extension.

User impact (optional)

Pull request checklist:

Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. If tests were not written, please explain why.
Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
Add example if relevant.
Document any changes to public APIs.
Run make update-api to update generated api files, if there's public API changes, otherwise the verify-api-* CI steps might fail.
Update CHANGELOG.md or use the label 'skip changelog', otherwise check changelog CI step will fail.
If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

...ndroidauto/src/main/java/com/mapbox/maps/extension/androidauto/MapboxCarMapGestureHandler.kt

kmadsen · 2022-09-15T22:40:07Z

android-auto-app/build.gradle.kts

@@ -32,6 +32,10 @@ android {
    }
  }

+  kotlinOptions {
+    freeCompilerArgs = freeCompilerArgs + "-Xjvm-default=all"


Am testing this on a bunch of interfaces to see if we catch any issues on the navigation side

mapbox/mapbox-navigation-android#6335 (comment)
^ Here are some details that show that adding the compiler flag may force some projects to add the compiler option. That seems like an issue with the compiler flag 🤔

For this reason I'm preferring the java interface approach..

And by java interface approach you mean jvm-default=all instead of older annotation option?

Btw https://blog.jetbrains.com/kotlin/2020/07/kotlin-1-4-m3-generating-default-methods-in-interfaces/ found a blog post summarizing the differences

I mean, jvm-default=all works well if you are building an app but it has as different behavior when building an sdk.

specifically, when I added -Xjvm-default=all to the nav-sdk it broke a downstream build because it is not using -Xjvm-default=all that is using kotlin. So a new SDK like android-auto could require downstream to add the flag, but this unpredictability is too concerning IMO.

What I mean, is define the interface with MyInterface.java so that the sdk is not depending on a "kotlin to java" converter. It is instead relying on "kotlin's java interpreter" which is a bit more reliable.

Notice how this pull request requires the addition of the compiler flat in both build.gradle files

android-auto-app/build.gradle.kts extension-androidauto/build.gradle.kts

@yunikkk this pull request is updated with the approach I'm thinking we need.

I've moved the jvm-default flag approach over here #1674

LGTM, I'm fine with the approach.

BTW could you elaborate the issue you've faced adding with the -Xjvm-default=all flag? Were you able to fix using snapshot?

Yeah I've updated this branch to use the jvm-default issue https://github.com/mapbox/mapbox-maps-android/pull/1674/files#r975672871

That way anyone can pull that branch and reproduce the issue

#1670 (comment)

This comment describes the issue well. The -Xjvm-default compiler flag may do what we want in the future. But right now, it does not generate default interfaces in an SDK. The app developer has to include the flag in their app to make the app compile when it uses an sdk that is relying on the flag.

One big issue for us to not add it, is kotlin will fail to compile if the app does not include it - totally unexpected behavior in my opinion because I would only expect java to fail to compile.

But right now, it does not generate default interfaces in an SDK.

Unexpected indeed, thanks for the investigation

kmadsen force-pushed the km-make-gesture-handler-backwards-compatible branch 2 times, most recently from e443f68 to b1f9cf1 Compare September 14, 2022 18:14

kmadsen assigned ypenglyn and pengdev Sep 14, 2022

kmadsen marked this pull request as ready for review September 14, 2022 18:14

kmadsen requested review from a team as code owners September 14, 2022 18:14

kmadsen commented Sep 14, 2022

View reviewed changes

...ndroidauto/src/main/java/com/mapbox/maps/extension/androidauto/MapboxCarMapGestureHandler.kt Outdated Show resolved Hide resolved

kmadsen force-pushed the km-make-gesture-handler-backwards-compatible branch 4 times, most recently from a6bf196 to e2e3771 Compare September 15, 2022 20:02

kmadsen changed the title ~~Make MapboxCarMapGestureHandler backwards compatible~~ [Android Auto] Make interfaces backwards compatible in java Sep 15, 2022

kmadsen mentioned this pull request Sep 15, 2022

Add jvm-default mapbox/mapbox-navigation-android#6335

Closed

kmadsen commented Sep 15, 2022

View reviewed changes

kmadsen force-pushed the km-make-gesture-handler-backwards-compatible branch from e2e3771 to a308c2b Compare September 17, 2022 01:41

kmadsen mentioned this pull request Sep 17, 2022

[Android Auto] Backwards compatible interfaces using using jvm default #1674

Closed

9 tasks

kiryldz approved these changes Sep 19, 2022

View reviewed changes

yunikkk approved these changes Sep 19, 2022

View reviewed changes

kmadsen added 2 commits September 20, 2022 11:12

Make MapboxCarMapGestureHandler backwards compatible

155cdf1

use a java interface

651733f

kmadsen force-pushed the km-make-gesture-handler-backwards-compatible branch from a308c2b to 651733f Compare September 20, 2022 18:12

kmadsen merged commit fd2dd92 into main Sep 20, 2022

kmadsen deleted the km-make-gesture-handler-backwards-compatible branch September 20, 2022 18:41

This was referenced Sep 20, 2022

[Android Auto] Add stable area changed callback #1648

Merged

[Android Auto] Add changelog for 0.3.0 #1688

Merged

kmadsen mentioned this pull request Sep 30, 2022

Add some flexibility to NavigationLocationProvider mapbox/mapbox-navigation-android#6420

Draft

pengdev mentioned this pull request Nov 29, 2022

Introduce MapTelemetry.getUserTelemetryRequestState API - MAPSAND-645 #1877

Merged

9 tasks

mapbox-github-ci-writer-public-1 bot pushed a commit that referenced this pull request May 5, 2023

[port] Add active-anchor expression support (#1664) (#1670)

a8a336b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android Auto] Make interfaces backwards compatible in java #1670

[Android Auto] Make interfaces backwards compatible in java #1670

kmadsen commented Sep 14, 2022 •

edited

Loading

kmadsen Sep 15, 2022

kmadsen Sep 15, 2022 •

edited

Loading

yunikkk Sep 16, 2022

kmadsen Sep 16, 2022 •

edited

Loading

kmadsen Sep 16, 2022 •

edited

Loading

kmadsen Sep 17, 2022

yunikkk Sep 19, 2022

kmadsen Sep 20, 2022

kmadsen Sep 20, 2022 •

edited

Loading

yunikkk Sep 21, 2022

[Android Auto] Make interfaces backwards compatible in java #1670

[Android Auto] Make interfaces backwards compatible in java #1670

Conversation

kmadsen commented Sep 14, 2022 • edited Loading

Summary of changes

User impact (optional)

Pull request checklist:

kmadsen Sep 15, 2022

Choose a reason for hiding this comment

kmadsen Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

yunikkk Sep 16, 2022

Choose a reason for hiding this comment

kmadsen Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

kmadsen Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

kmadsen Sep 17, 2022

Choose a reason for hiding this comment

yunikkk Sep 19, 2022

Choose a reason for hiding this comment

kmadsen Sep 20, 2022

Choose a reason for hiding this comment

kmadsen Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

yunikkk Sep 21, 2022

Choose a reason for hiding this comment

kmadsen commented Sep 14, 2022 •

edited

Loading

kmadsen Sep 15, 2022 •

edited

Loading

kmadsen Sep 16, 2022 •

edited

Loading

kmadsen Sep 16, 2022 •

edited

Loading

kmadsen Sep 20, 2022 •

edited

Loading