maplibre / maplibre-navigation-android

Maplibre Navigation SDK for Android
MIT License
86 stars 44 forks source link

Copy navigation models to navigation core #84

Closed boldtrn closed 11 months ago

boldtrn commented 11 months ago

Fixes #37.

This is a bigger change and would result in releasing a new major version due to several breaking changes. This is the foundation to finally loosening this repository from the Mapbox API and make it easier for anyone to include third party routers. So we could consider looking at #64 afterwards.

I would like to ask everyone to give this a good test, if everything works out, we can merge this soon.

Fabi755 commented 11 months ago

The code changes are looking good for me. I also understand the idea of this changes. But what I don't understand is the module architecture.

The core module holding the logic and models, the ui module will holds UI components. But why we locating the Mapbox specific things also in the ui module. For example MapboxRouteFetcher. Or the DirectionsResponse in two variants, where the new copied one is located in core and the existing one also exist as dependency of ui.

boldtrn commented 11 months ago

But why we locating the Mapbox specific things also in the ui module.

My first goal was to remove all dependencies to Mapbox APIs out of the core. So you can use the basic navigation without Mapbox.

I think most serious apps will write their own UI as it is right now anyway, so apps can decide how they want to implement the route fetching and don't need to carry the unnecessary Mapbox weight in their apps.

The ideal setup IMHO would be the further modularize the UI that it's possible to have use some of the UI elements in your own app.

Then we could have a module for Mapbox routing, one for GraphHopper, one for Valhalla, one for...

Fabi755 commented 11 months ago

Okay, then we have the same thoughts. Thanks for explaining this!

boldtrn commented 11 months ago

Thanks for all the reviews. I would kindly ask everyone to give this a good test in your apps as well, if nothing odd comes up, we can release a new major version in 1-2 weeks or so.

Fabi755 commented 11 months ago

I already tested the changed code base in our app, by review process. I don't see any issues with our implementation. ✅