[FEATURE] Improvement suggestion for map annotations.

[smallTrogdor]

Feature Description

I have recently started building a thin wrapper package for my company in a private repo on top of this library and I am really grateful for this plugin - thank you for all the work and the maintenance - good work!

Working a lot with this plugin, I came across the way annotations are managed as of today. After careful consideration, I decided to implement my own way (just using add / set geoJson source) instead of relying on the controller's addCircle, addFill, addX , removeCircle, removeX methods for the following reasons:

• I did not want to bloat my thin wrapper controller with a lot of methods that actually do almost the same under the hood (adding the features to a geojson source). My current implementation has just addAnnotation, removeAnnotation and updateAnnotation (with iterable implementation for array operations).
• I did not like the way the controller takes care of building the annotations, I found it more intuitive that users would create their annotations and pass in the whole object
• I wanted to take care of re-adding all the annotations after the map style changes without the users having to worry about that

Therefore I propose and wanted to discuss with you about:

• Simplifying annotation create, update and delete methods by only exposing a single method which takes a sealed class annotation from which there are the implementations of all the annotation types
• exposing an annotator with a callback and manipulating annotations through this interface, rather than the controller
• handling the way style switches remove all annotations from the map as of today (maybe added setStyle method on map controller #431 takes care of that anyway - haven't checked it out yet)

What do you think about that ? - I would be willing to invest my time into within the next couple weeks.

Describe alternatives you've considered

Using annotations the same way as today (which is in the end ok and has some bugs that one could also prioritize).

Additional context

No response

[josxha]

Thanks for creating this feature request @smallTrogdor.

Simplifying annotation create, update and delete methods by only exposing a single method which takes a sealed class annotation from which there are the implementations of all the annotation types

Sounds interesting. Although does that mean that we need to check the Annotation type in all these functions or would there be no separation between different annotations in the controller?

exposing an annotator with a callback and manipulating annotations through this interface, rather than the controller

What is the functionality of the annotator? Is it similar to what the AnnotationManager currently is?

handling the way style switches remove all annotations from the map as of today (maybe #431 takes care of that anyway - haven't checked it out yet)

I think it's currently not covered in #431 (now #433) but probably should be. 👍

I think this discussion is kind of similar to the discussion in #366 (comment) about how wide the API provided by maplibre_gl should be.

[smallTrogdor]

Although does that mean that we need to check the Annotation type in all these functions or would there be no separation between different annotations in the controller?

There is no differentiation between annotations for the user, however the annotation manager needs to differ those annotations using sealed classes and pattern matching to add the annotations to separate layers, but only uses a single GeoJSON source. I track the layers created separately, but the annotations in one LinkedHashMap.

What is the functionality of the annotator? Is it similar to what the AnnotationManager currently is?

Yes, very similar, however it is exposed to the user with a callback.

I think it's currently not covered in #433 but probably should be. 👍

Yes, the annotation manager should be able to react to style changes to re-add annotations in my opinion (maybe with a switch to disable this functionality). Before #433, I went an extra length and abstracted the style setting with a MapStyler, that I could listen to. Then I would expose only that styler to the user (which would just pass in the new style to the underlying map, as discussed in #431 as well).

Here is some excerpt from my code (maybe it helps):

  @override
  Future<void> addAnnotations(Iterable<MapAnnotation> annotations) async {
    final newAnnotations = annotations.where(_isNotKnown);
    if (newAnnotations.isEmpty) return;

    if (!_isGeoJsonSourceAdded) await _addGeoJsonSource();
    return _addAnnotationsToLayer(newAnnotations);
  }

// ...

  Future<void> _addAnnotationsToLayer(
    Iterable<MapAnnotation> annotations,
  ) async {
    await _addLayersIfNecessary(annotations);
    return _updateAll(annotations);
  }

  Future<void> _addLayersIfNecessary(
    Iterable<MapAnnotation> annotations,
  ) async {
    for (final a in annotations) {
      final layerId = _getAnnotationIdentifier(a);
      if (_addedLayers.keys.contains(layerId)) continue;
      final layerProperties = _getLayerProperties(a);

      await _applyLayerToStyle(layerId, layerProperties);
    }
  }

  Future<void> _applyLayerToStyle(
    String layerId,
    LayerProperties layerProperties,
  ) {
    return _controller
        .addLayer(_kSourceId, layerId, layerProperties)
        .catchError(_throwAnnotatorException(
            'Adding layer $layerId failed with exception: '))
        .then((_) => _addedLayers[layerId] = layerProperties);
  }

  _getAnnotationIdentifier(MapAnnotation annotation) => switch (annotation) { // the pattern matching
        MapSymbol() => _kSymbolIdentifier,
        MapCircle() => _kCircleIdentifier,
        // would need to be extended for all annotation types (line, fill)
      };

  LayerProperties _getLayerProperties(MapAnnotation annotation) =>
      switch (annotation) {
        MapSymbol() => MapSymbolLayer().makeLayerExpressions(),  // the maplibre_gl layer expressions
        MapCircle() => MapCircleLayer().makeLayerExpressions(),
      };

This is the method being called when the style changes:

  Future<void> onStyleChanged() async {
    if (!_isGeoJsonSourceAdded && _addedImages.isEmpty) return;
    await _addGeoJsonSource();
    await _reAddImages();
    await _reInsertLayers();
    return _updateAll();
  }

[smallTrogdor]

I forgot to mention: I filter the annotations in the layers with a custom property I set when adding ... to not confuse them from the same source ...

A first proposal could also be to deprecate the addCircle, addLine, etc methods in the controller and simply generalize them to a single addAnnotation(AnnotationType annotation) method ... would be a lot smaller of a change ...

What do you think @josxha ?

[josxha]

Sorry for my late reply @smallTrogdor.
I I think that's a great idea. I've seen that all methods come down call _setAll() in the end so there isn't really anything special about each method. What do you think @kuhnroyal?

[smallTrogdor]

@josxha Thanks for the update!

I have two questions:

• Shall I wait for feat: added set style method on controller #444 to make the changes when the styles changes?
• Shall I expose the annotationManager or do you prefer the addition to the controller (my later suggestion)?

Thanks @kuhnroyal for a feedback :)

Note: I will be afk until mid july and can implement it once back (will only take an evening or two in my estimation now)

[kuhnroyal]

I have not used annotations so far, so I don't currently have an opinion.
Can you look at what mapbox-gl does and see if/what we are missing?

[josxha]

The current implementation in maplibre_gl is about the same as in mapbox_gl. This feature would move us more apart from the mapbox_gl API, question is how much this matters to us. The development of mapbox_gl has mostly stopped in favor of the 1st party mapbox_maps_flutter package.

[kuhnroyal]

Still might make sense to check how this is implemented there.
Other than that I can't really provide feedback atm. - need to implement some markers/clustering next week in an app.
Maybe I have some useful feedback afterwards :)

[smallTrogdor]

Hi @kuhnroyal and @josxha,

any update on that ? - I am back to the keyboard now and could start working on the PR soon. I also saw, that #444 is kind of dead / not being worked on anymore...

In general the effort in this plugin from this spring has toned down a bit again, is there anything I can do help? It is really highly appreciated!

[josxha]

Hi @smallTrogdor, I haven't heared back from @kuhnroyal but I think your proposed changes are a good idea to clean up the controllers' API.
As #444 is currently stale you can ignore it for now. Thanks for your interest in contributing. (:

[smallTrogdor]

Alright, I will get to it at the end of this month 👍