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

Redesign the way MapViews are created to add support for MapLibre. #38

Merged
merged 10 commits into from
Apr 12, 2024

Conversation

alejandrocalles
Copy link
Contributor

@alejandrocalles alejandrocalles commented Apr 4, 2024

Will close #34. The objective is to abstract the way MapViews are provided in order to be able to choose freely between osmdroid and MapLibre view providers.
Ideally, support for markers will also be added for both osmdroid and MapLibre.

@alejandrocalles alejandrocalles added the enhancement New feature or request label Apr 4, 2024
@alejandrocalles alejandrocalles added this to the Milestone 1 milestone Apr 4, 2024
@alejandrocalles alejandrocalles self-assigned this Apr 4, 2024
@alejandrocalles alejandrocalles linked an issue Apr 6, 2024 that may be closed by this pull request
@alejandrocalles
Copy link
Contributor Author

alejandrocalles commented Apr 6, 2024

This should also close #40. Adding support for markers has been done for OsmdroidMapViewProvider. In the case of the MapLibreMapViewProvider, the marker features that are shown in most online tutorials/guides (and even in the official MapLibre walkthrough) seem to be deprecated. I'm struggling to find appropriate documentation for any other ways to display markers, if there are any.

@violoncelloCH
Copy link
Member

from a quick search how to draw on the map, I found this: https://maplibre.org/maplibre-native/docs/book/android/annotation-guide.html
is this what you mean seems deprecate? where dd you find that it'd be deprecated?

@alejandrocalles
Copy link
Contributor Author

Yes, that's exactly it. It's tagged as deprecated in the source code. Specifically the function map.addMarker and the class MarkerOptions. I vaguely remember seeing somewhere that there was another way to do it, but I haven't had much time to look into that.

@alejandrocalles alejandrocalles force-pushed the feature/map-composable branch 2 times, most recently from c24a0f9 to 712af94 Compare April 7, 2024 19:02
@alejandrocalles
Copy link
Contributor Author

alejandrocalles commented Apr 7, 2024

I did some more research, and the MapLibre project seems to be in a weird place right now. As you can see here, the whole annotations package has been deprecated.

If you click on any of the classes in the package, they suggest using the Mapbox annotation plugin, which hasn't been updated in 4 years (unless I'm misinterpreting something), and is in a repository that was archived last year.

I'll try to get it to work using the deprecated features first, and if that fails I'll try out the plugin.

I'm adding these links for future reference:

Copy link

@david-schroeter david-schroeter left a comment

Choose a reason for hiding this comment

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

Very good implementation, the integration of the Provider abstraction is very well implemented, nice job!

I just added a couple of comments, but in general I found that the code was of great quality! 👍🏻

Comment on lines 84 to 72
override fun update(view: MapView, events: List<Event>, callback: (Event) -> Unit) {
mapView.apply { drawAllMarkers(events, callback) }
}

Choose a reason for hiding this comment

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

Here, the view param is actually not used.

In this case view will be equal to the mapView variable, but what about using the view parameter directly, or if it isn't necessary simply remove the unused parameter from the function.

What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! In principle, the AndroidView is responsible for passing the View instance to the update function, so it's the parameter that should be used in this case.

In fact, the very existence of the mapView field is questionable, but I haven't found a better way to properly test my code...

* activity, or the [LocalContext][androidx.compose.ui.platform.LocalContext]`.current` if it is
* called from a composable.
*/
open class OsmdroidMapViewProvider(

Choose a reason for hiding this comment

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

Why specifically make the class open, should this class be further extended later ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not :) I'll remove it. Thanks again for catching this!

The open modifier is a remnant of an attempt I made to test this class. I wanted to access the mapView field without making it public, so I created a mock class that inherited from OsmdroidMapViewProvider. In the end I simply added getters for the relevant fields of the mapView.

mapViewFactory: (Context) -> MapView = { createMapView(it, TileSourceFactory.MAPNIK) },
update: (MapView) -> Unit = { updateMapView(it, LAUSANNE_GEO_POINT) }
context: Context,
events: MutableState<List<Event>>,

Choose a reason for hiding this comment

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

I saw that the events param is expected to be a MutableState<...>. I was wondering why not simply take a List<Event> here? So that outside of the MapDrawer, it is possible to instantiate the list as val myEvents by remember { mutableStateOf(emptyList<Event>()) }. And then simply pass the myEvents to the composable, it would prevent the need to call the .value when updating the list outside.

I haven't done the test myself, would it prevent the MapDrawer from recomposing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you''re right. I had my doubts about whether the AndroidView would be properly recomposed without the MutableState, but it seems to work just fine. Thanks!

@alejandrocalles alejandrocalles marked this pull request as ready for review April 12, 2024 02:15
Add the `IMapViewProvider` interface.
Modify `MapDrawer` to take an instance of `IMapViewProvider` as an
argument. The provider is in charge of defining the `factory` and
`update` functions.
Implement `OsmdroidMapViewProvider` to replicate the previous
functionality with the new `MapDrawer`.
Adapt the tests accordingly.
Now the `osmdroid` provider can receive a list of `Event`s as an argument and it
will draw a marker at each `Event.location`. The provider also accepts a
callback function that wil be called with the respective `Event` object
whenever a marker is clicked.
Add the `EchoAndroidView` composable. It is just a wrapper around an
`AndroidView` but it improves
readability.

Change the signature of the `update` method in `IMapViewProvider` to take advantage
of the way `AndroidView`s work (the `factory` function is only called
when the view is created, and the `update` function is called both when
the view is created and anytime the `AndroidView` is recomposed).

Adapt the `update` function and the constructor in `OsmdroidMapViewProvider` to comply with interface changes.

Improve the `MapDrawer` signature to to take a context instead of a
provider to facilitate usage.

Add documentation for relevant classes/composables.
Add the `toLatLng` method to get the `LatLng` representation of a
`Location`.
Remove the `context` as a parameter of the constructor. The
initialization of the `osmdroid` `Configuration` is now done in the
`factory` function. Adapt the `MapDrawer` signature to no longer take
the context as an argument.

Address the errors pointed out by David.
A new provider that uses the `MapLibre` library. Add support for
`MapLibre` markers using a deprecated API.

Add the necessary dependencies and resources.
Copy link

sonarcloud bot commented Apr 12, 2024

Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

Nice work, I tried the fonctionality on your branch and everything works fine. Map is correctly displayed and the pins of the events appears at the good location.

@alejandrocalles alejandrocalles merged commit e9e0923 into main Apr 12, 2024
2 checks passed
@alejandrocalles alejandrocalles deleted the feature/map-composable branch April 12, 2024 09:13
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
Status: Done in sprint 3
Development

Successfully merging this pull request may close these issues.

Add a way to display markers on a map. use Mapblibre SDK instead of osmdroid
4 participants