Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert core module to Kotlin #127

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

Convert core module to Kotlin #127

wants to merge 49 commits into from
+16,997 −26,010

Conversation

Fabi755
Copy link
Collaborator

@Fabi755 Fabi755 commented Nov 22, 2024 •
edited
Loading

This PR will convert the module libandroid-navigation to Kotlin.

I used Android Studio's converting tool and try my best to refactore the generated Kotlin code.

There are still classes that I didn't refactor, because the logic itself was already very complicated and ugly in Java. For this classes I think seperated follow up PRs make sense. Additional all Auto Models now data classes. Also I removed all unused classes from this module.

I tested the changes against the example app, and fix all occuring issues. But I suspect that I do not catched all.

I note following..

Breaking changes

org.maplibre.navigation.android.core.connectivity.* --> Removed. Not used internally. Make for me no sense to keep this.
org.maplibre.navigation.android.core.crashreporter.* --> Removed. Not used internally. Make for me no sense to keep this.
org.maplibre.navigation.android.core.FileUtils --> Removed. Only used by CrashReport

org.maplibre.navigation.android.navigation.v5.milestone.Milestone --> Remove Builder, use named arguments in constructor instead.
org.maplibre.navigation.android.navigation.v5.routeprogress.* --> Convert all @autovalue classes to Kotlin data classes. (Also Builder pattern is not used here anymore)
org.maplibre.navigation.android.navigation.v5.models.* -->
- Convert all @autovalue classes to Kotlin data classes. (Also Builder pattern is not used here anymore). Using KotlinX instead of GSON for JSON parsing.
- Convert annotation types to enums
- Using Kotlinx serialization instead of GSON for JSON parsing
- Remove classes MapLibreStreetsV8, DirectionsJsonObject, DirectionsAdapterFactory
org.maplibre.navigation.android.navigation.v5.models.utils.* --> Removed. Not used internally.
org.maplibre.navigation.android.navigation.v5.utils.abbreviation.* --> Removed. Not used internally. I think it is also not used by developers.
org.maplibre.navigation.android.navigation.v5.utils.TextUtils --> Removed. Can be replaced by Android's TextUtils or by Kotlin language functions.
org.maplibre.navigation.android.navigation.v5.utils.RouteUtils --> Now static (object) class.

org.maplibre.navigation.android.navigation.v5.navigation.NavigationLifecycleMonitor --> Removed. Not used internal.
org.maplibre.navigation.android.navigation.v5.navigation.NavigationMapRoute --> Deprecated/Removed. In UI package a newer version is available.
org.maplibre.navigation.android.navigation.v5.navigation.SdkVersionChecker --> Deprecated/Removed. Make no sense, use plain java/kotlin code.
org.maplibre.navigation.android.navigation.v5.navigation.NavigationTimeFormat --> Move to MapLibreNavigationOptions and migrate to enum class.

org.maplibre.navigation.android.navigation.v5.route.MapRouteProgressChangeListener --> Deprecated/Removed. In UI package a newer version is available.
org.maplibre.navigation.android.navigation.v5.route.OnRouteSelectionChangeListener --> Deprecated/Removed. In UI package a newer version is available.

org.maplibre.navigation.android.navigation.v5.location.MetricsLocation --> Removed. Seems not used.
org.maplibre.navigation.android.navigation.v5.location.replay.GpxParser --> Removed. Seems not used.
org.maplibre.navigation.android.navigation.v5.location.replay.ParseGpxTask --> Removed. Seems not used.
org.maplibre.navigation.android.navigation.v5.location.replay.ReplayJsonRouteDto --> Removed. Seems not used.
org.maplibre.navigation.android.navigation.v5.location.replay.ReplayJsonRouteLocationMapper --> Removed. Seems not used.
org.maplibre.navigation.android.navigation.v5.location.replay.ReplayLocationDto --> Removed. Seems not used.
org.maplibre.navigation.android.navigation.v5.location.replay.TimestampAdapter --> Removed. Seems not used.

org.maplibre.navigation.android.navigation.v5.navigation.camera.RouteInformation --> Remove Builder, use named arguments in constructor instead.
org.maplibre.navigation.android.navigation.v5.navigation.camera.Camera --> Remove function target, because it's not used anywhere.

org.maplibre.navigation.android.navigation.v5.models.RouteOptions --> Remove *list functions. Seems not used anywhere.

org.maplibre.navigation.android.navigation.v5.navigation.MapLibreNavigationOptions --> Remove isFromNavigationUi, because seems not used anywhere.
org.maplibre.navigation.android.navigation.v5.navigation.MapLibreNavigationOptions --> minimumDistanceBeforeRerouting is splitt up to two new options.

  1. offRouteMinimumDistanceMetersAfterReroute minimum meters after a reroute needs passed
  2. offRouteThresholdRadiusMeters a fixed threshold, that will cause a off-route state, when passed.

Refactor later

org.maplibre.navigation.android.navigation.v5.milestone.TriggerProperty --> Seems too complicated. Create a fresh new re-write.
org.maplibre.navigation.android.navigation.v5.location.replay.* --> Hard to refactor in clean Kotlin. Also seems too complicated. Create a fresh new re-write.
Add adapters or use DTOs instead of parsing directly to models that can be replaces by developers to use other API engines
org.maplibre.navigation.android.navigation.v5.utils.time.* --> Seems too complicated. Create a simpler version.
org.maplibre.navigation.android.navigation.v5.utils.RouteUtils --> Seems too complicated. Create simpler versions of util functions.

Rename engines to name with ending engine. And also choose a standardized naming for implementations.
- Camera --> CameraEngine
- Snap --> SnapEngine
- OffRoute --> OffRouteEngine
- FasterRoute --> FasterRouteEngine

Discuss

Can LegAnnotation used without distance field? If not, we should thinking about to make the distance list non-null

@Fabi755
Convert milestone logic to Kotlin
8904370
@Fabi755
Convert route progress models
8bc4fe0
@Fabi755
Convert tests to dirty Kotlin
b9aea76
@Fabi755
Make milestone logic work against tests
aa7c687
@Fabi755
Fix converted tests
c414bd4
@Fabi755
Convert models
1751b60
@Fabi755
Use Kotlin serialization for JSON parsing
7c8e514
@Fabi755
Remove abbreviation utils
810e4f4
@Fabi755
Remove unused text utils
67429eb
@Fabi755
Convert utils
0d1dea8
@Fabi755
Convert route snapping logic
330f931
@Fabi755
Convert most of the navigation logic
adcaa25
@Fabi755
Convert route logic
0ddc212
@Fabi755
Convert exception and instruction
870874d
@Fabi755
Convert off route logic
2bd5de7
@Fabi755
Replace Mockito by mockk
7f4b220
@Fabi755
Convert location logic
94fa153
@Fabi755
Convert notification classes
d91e014
@Fabi755
Remove core package
abafb63
@Fabi755
Convert route utils
cc486b8
@Fabi755
Convert camera logic
4212b30
@Fabi755
Remove unused and outdated classes
ae011c8
@Fabi755
Lint clean up
1028d0b
@Fabi755
Adjust UI module to new changes
72db91f
@Fabi755
Adjust example app to new changes
edf07ad
@Fabi755
Solve open todos
56cc90b
@Fabi755
Clean test code
1cec87e
@Fabi755
Solve open todos
b71ad54
@Fabi755
Initialize NavigationMapRoute after style has applied
8b45098
@Fabi755
Fix crashes
1b8cb83
boldtrn
boldtrn reviewed Nov 22, 2024
View reviewed changes
notes.txt Outdated Show resolved Hide resolved
sotomski
sotomski reviewed Nov 26, 2024
View reviewed changes
...d-navigation/src/main/java/org/maplibre/navigation/android/navigation/v5/utils/RouteUtils.kt Outdated
import org.maplibre.navigation.android.navigation.v5.navigation.NavigationConstants
import org.maplibre.navigation.android.navigation.v5.routeprogress.RouteProgress

object RouteUtils {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding the findCurrentBannerText function, please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! Seems unused for me, but if it used public I will add it again 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm, if it is unsued in the library, I am not sure if we should add it for third parties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? We will provide some tools that make sense to use with our library. But sure it's for every function complicated to decide, if this is a good helper, or a function that should implemented by the developer itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't care about this specific function, we can include it or not. In general my feeling is, that we should rather try to keep unused helper functions minimal, if the library or the UI module don't use thzem, are we sure anyone else uses it? If only a minority of implementing projects are using a function, I am not sure it should be part of the core.

I guess there are some helpers that make total sense though, so I am not saying we should remove everything necessarily :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this again for now ✅

@Fabi755 Fabi755 mentioned this pull request Nov 26, 2024
Open
6 tasks
sotomski
sotomski reviewed Nov 26, 2024
View reviewed changes
...java/org/maplibre/navigation/android/navigation/v5/navigation/MapLibreNavigationOptions.java

public abstract Builder isFromNavigationUi(boolean isFromNavigationUi);

public abstract Builder minimumDistanceBeforeRerouting(double distanceInMeters);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this prop lost in translation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter was not used in the named described way. So I decide to split this up in two new parameters:

org.maplibre.navigation.android.navigation.v5.navigation.MapLibreNavigationOptions --> minimumDistanceBeforeRerouting is split up to two new options.

  1. offRouteMinimumDistanceMetersAfterReroute minimum meters after a reroute needs passed
  2. offRouteThresholdRadiusMeters a fixed threshold, that will cause a off-route state, when passed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds this reasonable for you?

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Nov 28, 2024

As discussed, I added the Camera.target(...) function again.

@sotomski

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Nov 28, 2024

Round 2️⃣ Latest changes are ready now for a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boldtrn boldtrn boldtrn left review comments

@sotomski sotomski sotomski left review comments

@frankkienl frankkienl Awaiting requested review from frankkienl

At least 1 approving review is required to merge this pull request.

Assignees

@Fabi755 Fabi755

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Fabi755 @sotomski @boldtrn