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.

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

Convert core module to Kotlin Mutliplatform #133

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

Conversation

Fabi755
Copy link
Collaborator

@Fabi755 Fabi755 commented Dec 17, 2024

This PR is migrating the core module to Kotlin Multiplatform.

Breaking changes

  • Instead of using android.location.Location we have now our own generic org.maplibre.navigation.core.location.Location.
  • Module libandroid-navigation renamed to maplibre-navigation-core (important for Jitpack reference)
  • Package org.maplibre.navigation.android.navigation.v5. has renamed to org.maplibre.navigation.core
  • You can still use MapLibreNavigation, but there it's now required to set a LocationEngine. Alternative using AndroidMapLibreNavigation / IOSMapLibreNavigation for a default implementation of this parameter.
  • ...

Some key facts

  • I moved out the UI related classes to the UI module, as it was wrong placed and using context and other Android platform related classes.
  • The logic was very very mixed with the NavigationService, Notification and Android specific threading. I don't see a reason to keep this, I switched the threading to coroutines, and the notification and foregrounds service are now optional components, that included in the UI module. The navigation core logic is now located in navigation.engine package. The notification is now located in the UI module in navigation package
  • As the module contains android, I renamed the module to maplibre-navigation-core
  • As the package contains android, I renamed the package from org.maplibre.navigation.android.navigation.v5. to org.maplibre.navigation.core
  • The only platform specific part is the location fetching. I solved this by an interface implementation.
  • The tests can only run on JVM(/Android) target, because we are using currently the mockk library

Platform specific logic

Two categories of platform specific parts are affected this module.

**1. the location logic.*+ I'm very happy with the solution around the LocationEngine and the platform specific logic.

2. the MapLibre library / GeoJson / Turf logic. I'm very unhappy with the geojson stuff. I now added a new module maplibre-navigation-geo that holds some partial (simplified) copies of the geojson and turf stuff. The original repository maplibre-java are used before by the Android client. But Java code can not included to iOS with KMP.

I see three solutions for this:

  1. Adding own module that gives interfaces and using internally the Android & iOS MapLibre-Native libraries.
  2. Convert maplibre-java also to a KMP project
  3. Keep the current solution and having our own GeoJson module.

All of this points having up- and downsides. I choosed now the 3. point, because it was the fastest. I would suggest to create a follow up issue and discuss this here.
To find a solution, we need also input from the maintainers of maplibre-java and needing also a look to the iOS side, and how we implement this core module.

Performance

I changed a the whole logic structure, that was necessary to remove all Android related things, and using plain Kotlin. I tried my best to keep it simple and efficient. But I think we all should do some performance tests with real navigations and real-life example usages.

@@ -118,7 +120,7 @@ private void cacheRouteOptions(RouteOptions routeOptions) {
private void cacheRouteDestination() {
boolean hasValidCoordinates = routeOptions != null && !routeOptions.getCoordinates().isEmpty();
if (hasValidCoordinates) {
List<Point> coordinates = routeOptions.getCoordinates();
List<Point> coordinates = toMapLibrePoints(routeOptions.getCoordinates());
Copy link
Collaborator Author

@Fabi755 Fabi755 Dec 19, 2024

Choose a reason for hiding this comment

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

This converting is very annoying and confusing for the developers. We now having three Point types

  • MapLibre (Android)
  • MapLibre Navigation (generic KMP)
  • Mapbox

But as mentioned, we need to decide how and where we want to have the generic GeoJson stuff. Changing this before this, make multiple effort and work.

@Fabi755
Copy link
Collaborator Author

Fabi755 commented Dec 19, 2024

If the package movements are to annoying for the review to you, I can also create a pre-PR that only moving stuff. So this changes are then not visible anymore in the changes.

Only an offer to you reviewers!

@Fabi755 Fabi755 changed the title DRAFT: Convert core module to Kotlin Mutliplatform. Convert core module to Kotlin Mutliplatform Dec 19, 2024

@Before
fun setUp() {
Dispatchers.setMain(mainThreadSurrogate)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider resetting the main dispatcher in a teardown to avoid polluting other test classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*/
open class GoogleLocationEngine(
context: Context,
private val looper: Looper
Copy link
Contributor

Choose a reason for hiding this comment

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

This looper was nullable beforehand, which was consistent with the underlying Google API: FusedLocationProviderClient

Since the null value doesn't cause any trouble and its effect is specified in Google's docs, I'd consider making it nullable again.

Suggested change
private val looper: Looper
private val looper: Looper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Fabi755 Fabi755 mentioned this pull request Jan 8, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants