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

Plan changes to Android annotation API #1189

Closed
fynngodau opened this issue May 30, 2023 · 9 comments · Fixed by #1255
Closed

Plan changes to Android annotation API #1189

fynngodau opened this issue May 30, 2023 · 9 comments · Fixed by #1255
Assignees
Labels
android 💰 bounty M Medium Bounty, USD 250

Comments

@fynngodau
Copy link
Collaborator

When starting work on #1154, I noticed it can't be implemented as easily as we were hoping, for the following reasons:

  • The code currently located in the Annotations repo has classes like Symbol, Fill and so on, that
    the older implementation in maplibre-native does not have.
  • Instead, it has the classes Marker, Polygon and more that imitate the Google Maps API and call some native code:
    jni::Local<jni::Array<jni::jlong>> NativeMapView::addMarkers(jni::JNIEnv& env,
    const jni::Array<jni::Object<Marker>>& jmarkers) {
    jni::NullCheck(env, &jmarkers);
    std::size_t len = jmarkers.Length(env);
    std::vector<jni::jlong> ids;
    ids.reserve(len);
    for (std::size_t i = 0; i < len; i++) {
    auto marker = jmarkers.Get(env, i);
    ids.push_back(map->addAnnotation(
    mbgl::SymbolAnnotation{Marker::getPosition(env, marker), Marker::getIconId(env, marker)}));
    }
    auto result = jni::Array<jni::jlong>::New(env, len);
    result.SetRegion<std::vector<jni::jlong>>(env, 0, ids);
    return result;
    }
  • This native code itself has classes like FillAnnotation, SymbolAnnotation and probably generates Geometry somewhere, which the annotations code also does, but not in C++ code.
  • Because the old API (deprecated and in maplibre-native) tries to immitate the Google Maps API whilst the new one (in annotations plugin) does not, there is no obvious path towards moving the code whilst un-deprecating the deprecated methods like we planned, without making the migration from the current API – which I imagine most users are using – unnecessarily complex.

Due to these considerations, we should make a plan for how we want the API to look like in the future. Some points that I think are important for this, regarding what aspects are nice about each of the APIs:

  • The old API had methods directly on the map object that allowed to addMarker, not bothering users with creating an object like SymbolAnnotations. I like that.
  • In total the two APIs are quite similar obviously (which is why we considered a merge in the first place); their user-facing renamings are pretty much a matter of taste – but it makes more sense to stick with the new terminology (Symbol, Fill) which is also used internally instead of trying to pretend to be Google Maps.
  • In certain places, the newer annotations API has the user wangle more with its internals. Here is one example of this: SymbolOptions.withIconImage takes a String that references the icon, and it is the user's responsibility to put their resource into the style, whereas Marker.setIcon takes an Icon object where it is easy to find out how to construct it. The latter is much preferrable and we should keep such an easy interface for the future.

Goals of this issue:

  • Create a concrete design proposal for changes to the API for annotations in Android.
  • The API should be a part of maplibre-native according to this design proposal.
  • It should aim to be harmonic and user-friendly, and create as few friction as necessary for existing users of the annotations plugin during migrations.
@JonasVautherin
Copy link
Collaborator

JonasVautherin commented Jun 16, 2023

I was asked my opinion on the two PRs above, so here it is 😊.

I don't exactly know the implementation details, but this does not seem like a super quick task (this issue is 2 weeks old, and there are discussions about it from at least a month ago). Since the plugins seem to have been unmaintained for a while (last released artifact is from 2021, CI is failing, ...), I guess it makes sense to give them some attention 👍.

Now personally, I have a couple concerns, which are:

  • If the plugins are merged "as-is" and stay unmaintained in the main repo, that's actually harder for us: it is easier for us to maintain a fork (and corresponding Maven artifact) of a single plugin than having to fork the main repo just for our small fixes.
  • If the plugins are actually re-designed/rewritten, then my concern is that we may lose features that we currently need, which would force us to either fork the whole project or go somewhere else (e.g. we can't use GoogleMaps because their markers are not powerful enough).

I guess my point is that as long as those changes don't prevent us from using MapLibre and we somehow see a way to get our fixes upstream, then that is fine for us 😇.

@louwers
Copy link
Collaborator

louwers commented Jun 16, 2023

@JonasVautherin MapLibre Native has a paid maintainer (me!) so I would not worry about not being able to upstream changes. Feel free to ping me on Slack at any time, I'll likely respond pretty quickly during office hours in Europe.

The reason why the plugins repo is stale is because bringing it to a good state again would require a lot of duplicate work that has been done for the main repo. As an example, we now have pretty good CI for this repo, including for example even running the UI tests for Android on AWS Device Farm.

If the plugins are actually re-designed/rewritten, then my concern is that we may lose features that we currently need

We will make sure no functionality is lost. This issue just exists to make a design proposal, you will be able to comment on the design when it is proposed at a later stage.

That said, would you want to help to make an official release of the plugins package?

@fynngodau
Copy link
Collaborator Author

@JonasVautherin I think the question this issue raises is: are there any specific design choices that have bothered you when working with the API for annotations in the past, something that could be made more user-friendly by designing it differently? Then we can consider it when writing the proposal in the first place.

@JonasVautherin
Copy link
Collaborator

I think the question this issue raises is: are there any specific design choices that have bothered you when working with the API for annotations in the past, something that could be made more user-friendly by designing it differently?

I think @RomanBapst can comment better on this, but to me it seems like it was pretty fine.

That said, would you want to help to make an official release of the plugins package?

Honestly I did not manage to build https://github.com/maplibre/maplibre-plugins-android, so instead I made the plugin we modify standalone (https://github.com/ramani-maps/maplibre-plugins-android/tree/main/plugin-annotation). So we can just go there and run ./gradlew build.

I could change it such that I can ./gradlew publish and it would push it to your Maven repo (I am used to Sonatype, not sure what you use). Either from your repo (if my PRs get in 😇) or directly from our fork, if you want.

Would that help? Or is it not what you were asking?

@RomanBapst
Copy link
Collaborator

@fynngodau

I think the question this issue raises is: are there any specific design choices that have bothered you when working with the API for annotations in the past, something that could be made more user-friendly by designing it differently? Then we can consider it when writing the proposal in the first place.

I have been working with the newer API (e.g. CircleManager) in the last couple of weeks and was quite happy with it. There are some really weird things like Text not being loaded on the map when you don't have an internet connection (I live somewhere in Tasania in the bush and that can happen :-p).
But mostly I found that it gives you great flexibility (after some small mods in the annotation repo) to be creative about what you draw on the map. I also really like that fact that all annotations types expose drag events which is really really important.

I would be really careful to reduce this flexibility, let me try to explain with an example. In the past I have been using google-maps-compose for an app where I wanted to create an interactive polygon where the user should have been able to drag the vertices of the polygon around. The lib offered a Marker element which supported dragging but only after a long press. Additionally, the Marker internally implemented behavior which would make the icon jump out from below your finger to make it easier to place it. That may make sense for a standard marker on the map but not for that use case. I did not find a way around this and had to abandon the lib for that project.
I think you can easily create convenience elements like Markers but that should not come at the cost of flexibility. I think the two things can easily co-exist.

@louwers louwers added the 💰 bounty M Medium Bounty, USD 250 label Jun 16, 2023
@fynngodau
Copy link
Collaborator Author

@RomanBapst @JonasVautherin Thanks for your input.

@louwers Feel free to assign the issue to me, so I can start working on it.

@fynngodau
Copy link
Collaborator Author

@RomanBapst @JonasVautherin My proposal is here: #1255

Feel free to comment in that PR 🙂

@louwers
Copy link
Collaborator

louwers commented Jul 2, 2023

This bounty can be paid out.

@fynngodau
Copy link
Collaborator Author

@louwers louwers unpinned this issue Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android 💰 bounty M Medium Bounty, USD 250
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants