Fix annotation manager crashes (#1297)

* fixed crash caused be race condition in layer deletion * better layer ids for annotations * added overlap button to example
flutter-mapbox-gl · Apr 17, 2023 · 61b435e · 61b435e
1 parent 622146f
commit 61b435e
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 30 deletions.
diff --git a/android/src/main/java/com/mapbox/mapboxgl/MapboxMapController.java b/android/src/main/java/com/mapbox/mapboxgl/MapboxMapController.java
@@ -912,6 +912,7 @@ public void onError(@NonNull String message) {
 
           Expression filterExpression = parseFilter(filter);
 
+          removeLayer(layerId);
           addSymbolLayer(
               layerId,
               sourceId,
@@ -942,6 +943,7 @@ public void onError(@NonNull String message) {
 
           Expression filterExpression = parseFilter(filter);
 
+          removeLayer(layerId);
           addLineLayer(
               layerId,
               sourceId,
@@ -972,6 +974,7 @@ public void onError(@NonNull String message) {
 
           Expression filterExpression = parseFilter(filter);
 
+          removeLayer(layerId);
           addFillLayer(
               layerId,
               sourceId,
@@ -1003,6 +1006,7 @@ public void onError(@NonNull String message) {
 
           Expression filterExpression = parseFilter(filter);
 
+          removeLayer(layerId);
           addFillExtrusionLayer(
               layerId,
               sourceId,
@@ -1033,6 +1037,7 @@ public void onError(@NonNull String message) {
 
           Expression filterExpression = parseFilter(filter);
 
+          removeLayer(layerId);
           addCircleLayer(
               layerId,
               sourceId,
@@ -1057,6 +1062,8 @@ public void onError(@NonNull String message) {
           final Double maxzoom = call.argument("maxzoom");
           final PropertyValue[] properties =
               LayerPropertyConverter.interpretRasterLayerProperties(call.argument("properties"));
+
+          removeLayer(layerId);
           addRasterLayer(
               layerId,
               sourceId,
@@ -1281,8 +1288,7 @@ public void onFailure(@NonNull Exception exception) {
                 null);
           }
           String layerId = call.argument("layerId");
-          style.removeLayer(layerId);
-          interactiveFeatureLayerIds.remove(layerId);
+          removeLayer(layerId);
 
           result.success(null);
           break;
@@ -1958,6 +1964,13 @@ boolean onMove(MoveGestureDetector detector) {
     return true;
   }
 
+  void removeLayer(String layerId) {
+    if (style != null && layerId != null) {
+      style.removeLayer(layerId);
+      interactiveFeatureLayerIds.remove(layerId);
+    }
+  }
+
   void onMoveEnd(MoveGestureDetector detector) {
     PointF pointf = detector.getFocalPoint();
     invokeFeatureDrag(pointf, "end");

diff --git a/example/lib/click_annotations.dart b/example/lib/click_annotations.dart
@@ -28,6 +28,7 @@ class ClickAnnotationBody extends StatefulWidget {
 class ClickAnnotationBodyState extends State<ClickAnnotationBody> {
   ClickAnnotationBodyState();
   static const LatLng center = const LatLng(-33.88, 151.16);
+  bool overlapping = false;
 
   MapboxMapController? controller;
 
@@ -131,20 +132,37 @@ class ClickAnnotationBodyState extends State<ClickAnnotationBody> {
 
   @override
   Widget build(BuildContext context) {
-    return MapboxMap(
-      accessToken: MapsDemo.ACCESS_TOKEN,
-      annotationOrder: [
-        AnnotationType.fill,
-        AnnotationType.line,
-        AnnotationType.circle,
-        AnnotationType.symbol,
-      ],
-      onMapCreated: _onMapCreated,
-      onStyleLoadedCallback: _onStyleLoaded,
-      initialCameraPosition: const CameraPosition(
-        target: center,
-        zoom: 12.0,
+    return Scaffold(
+      body: MapboxMap(
+        accessToken: MapsDemo.ACCESS_TOKEN,
+        annotationOrder: [
+          AnnotationType.fill,
+          AnnotationType.line,
+          AnnotationType.circle,
+          AnnotationType.symbol,
+        ],
+        onMapCreated: _onMapCreated,
+        onStyleLoadedCallback: _onStyleLoaded,
+        initialCameraPosition: const CameraPosition(
+          target: center,
+          zoom: 12.0,
+        ),
       ),
+      floatingActionButton: ElevatedButton(
+          onPressed: () {
+            setState(() {
+              overlapping = !overlapping;
+            });
+            controller!.setSymbolIconAllowOverlap(overlapping);
+            controller!.setSymbolIconIgnorePlacement(overlapping);
+
+            controller!.setSymbolTextAllowOverlap(overlapping);
+            controller!.setSymbolTextIgnorePlacement(overlapping);
+          },
+          child: Padding(
+            padding: const EdgeInsets.all(8.0),
+            child: Text("Toggle overlapping"),
+          )),
     );
   }
 }
diff --git a/ios/Classes/MapboxMapController.swift b/ios/Classes/MapboxMapController.swift
@@ -354,6 +354,7 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let maxzoom = arguments["maxzoom"] as? Double
             let filter = arguments["filter"] as? String
 
+            removeLayer(layerId: layerId)
             let addResult = addSymbolLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -382,6 +383,7 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let maxzoom = arguments["maxzoom"] as? Double
             let filter = arguments["filter"] as? String
 
+            removeLayer(layerId: layerId)
             let addResult = addLineLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -410,6 +412,7 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let maxzoom = arguments["maxzoom"] as? Double
             let filter = arguments["filter"] as? String
 
+            removeLayer(layerId: layerId)
             let addResult = addFillLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -438,6 +441,7 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let maxzoom = arguments["maxzoom"] as? Double
             let filter = arguments["filter"] as? String
 
+            removeLayer(layerId: layerId)
             let addResult = addFillExtrusionLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -466,6 +470,7 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let maxzoom = arguments["maxzoom"] as? Double
             let filter = arguments["filter"] as? String
 
+            removeLayer(layerId: layerId)
             let addResult = addCircleLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -490,6 +495,8 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let belowLayerId = arguments["belowLayerId"] as? String
             let minzoom = arguments["minzoom"] as? Double
             let maxzoom = arguments["maxzoom"] as? Double
+
+            removeLayer(layerId: layerId)
             addHillshadeLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -508,6 +515,8 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
             let belowLayerId = arguments["belowLayerId"] as? String
             let minzoom = arguments["minzoom"] as? Double
             let maxzoom = arguments["maxzoom"] as? Double
+
+            removeLayer(layerId: layerId)
             addHeatmapLayer(
                 sourceId: sourceId,
                 layerId: layerId,
@@ -730,12 +739,7 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
         case "style#removeLayer":
             guard let arguments = methodCall.arguments as? [String: Any] else { return }
             guard let layerId = arguments["layerId"] as? String else { return }
-            guard let layer = mapView.style?.layer(withIdentifier: layerId) else {
-                result(nil)
-                return
-            }
-            interactiveFeatureLayerIds.remove(layerId)
-            mapView.style?.removeLayer(layer)
+            removeLayer(layerId: layerId)
             result(nil)
 
         case "style#setFilter":
@@ -897,6 +901,13 @@ class MapboxMapController: NSObject, FlutterPlatformView, MGLMapViewDelegate, Ma
         }
     }
 
+    private func removeLayer(layerId: String) {
+        if let layer = mapView.style?.layer(withIdentifier: layerId) {
+            mapView.style?.removeLayer(layer)
+            interactiveFeatureLayerIds.remove(layerId)
+        }
+    }
+
     private func loadIconImage(name: String) -> UIImage? {
         // Build up the full path of the asset.
         // First find the last '/' ans split the image name in the asset directory and the image file name.

diff --git a/lib/src/annotation_manager.dart b/lib/src/annotation_manager.dart
@@ -9,7 +9,11 @@ abstract class AnnotationManager<T extends Annotation> {
   final void Function(T)? onTap;
 
   /// base id of the manager. User [layerdIds] to get the actual ids.
-  final String id;
+  String get id => "${managerType}_$randomPostFix";
+
+  final String managerType;
+
+  final String randomPostFix;
 
   List<String> get layerIds =>
       [for (int i = 0; i < allLayerProperties.length; i++) _makeLayerId(i)];
@@ -29,9 +33,13 @@ abstract class AnnotationManager<T extends Annotation> {
 
   Set<T> get annotations => _idToAnnotation.values.toSet();
 
-  AnnotationManager(this.controller,
-      {this.onTap, this.selectLayer, required this.enableInteraction})
-      : id = getRandomString() {
+  AnnotationManager(
+    this.controller, {
+    required this.managerType,
+    this.onTap,
+    this.selectLayer,
+    required this.enableInteraction,
+  }) : randomPostFix = getRandomString() {
     for (var i = 0; i < allLayerProperties.length; i++) {
       final layerId = _makeLayerId(i);
       controller.addGeoJsonSource(layerId, buildFeatureCollection([]),
@@ -50,7 +58,6 @@ abstract class AnnotationManager<T extends Annotation> {
   Future<void> _rebuildLayers() async {
     for (var i = 0; i < allLayerProperties.length; i++) {
       final layerId = _makeLayerId(i);
-      await controller.removeLayer(layerId);
       await controller.addLayer(layerId, layerId, allLayerProperties[i]);
     }
   }
@@ -172,6 +179,7 @@ class LineManager extends AnnotationManager<Line> {
       {void Function(Line)? onTap, bool enableInteraction = true})
       : super(
           controller,
+          managerType: "line",
           onTap: onTap,
           enableInteraction: enableInteraction,
           selectLayer: (Line line) => line.options.linePattern == null ? 0 : 1,
@@ -201,6 +209,7 @@ class FillManager extends AnnotationManager<Fill> {
     bool enableInteraction = true,
   }) : super(
           controller,
+          managerType: "fill",
           onTap: onTap,
           enableInteraction: enableInteraction,
           selectLayer: (Fill fill) => fill.options.fillPattern == null ? 0 : 1,
@@ -228,6 +237,7 @@ class CircleManager extends AnnotationManager<Circle> {
     bool enableInteraction = true,
   }) : super(
           controller,
+          managerType: "circle",
           enableInteraction: enableInteraction,
           onTap: onTap,
         );
@@ -260,6 +270,7 @@ class SymbolManager extends AnnotationManager<Symbol> {
         _textIgnorePlacement = textIgnorePlacement,
         super(
           controller,
+          managerType: "symbol",
           enableInteraction: enableInteraction,
           onTap: onTap,
         );

diff --git a/lib/src/controller.dart b/lib/src/controller.dart
@@ -1241,7 +1241,7 @@ class MapboxMapController extends ChangeNotifier {
   /// Add a layer to the map with the given properties
   ///
   /// The returned [Future] completes after the change has been made on the
-  /// platform side.
+  /// platform side. If the layer already exists, the layer is updated.
   ///
   /// Setting [belowLayerId] adds the new layer below the given id.
   /// If [enableInteraction] is set the layer is considered for touch or drag

diff --git a/mapbox_gl_web/lib/src/mapbox_web_gl_platform.dart b/mapbox_gl_web/lib/src/mapbox_web_gl_platform.dart
@@ -645,7 +645,7 @@ class MapboxWebGlPlatform extends MapboxGlPlatform
       final styleJson = jsonDecode(styleString ?? '');
       final styleJsObject = jsUtil.jsify(styleJson);
       _map.setStyle(styleJsObject);
-    } catch(_) {
+    } catch (_) {
       _map.setStyle(styleString);
     }
     // catch style loaded for later style changes
@@ -692,8 +692,10 @@ class MapboxWebGlPlatform extends MapboxGlPlatform
 
   @override
   Future<void> removeLayer(String layerId) async {
-    _interactiveFeatureLayerIds.remove(layerId);
-    _map.removeLayer(layerId);
+    if (_map.getLayer(layerId) != null) {
+      _interactiveFeatureLayerIds.remove(layerId);
+      _map.removeLayer(layerId);
+    }
   }
 
   @override
@@ -884,6 +886,8 @@ class MapboxWebGlPlatform extends MapboxGlPlatform
     final paint = Map.fromEntries(
         properties.entries.where((entry) => !isLayoutProperty(entry.key)));
 
+    removeLayer(layerId);
+
     _map.addLayer({
       'id': layerId,
       'type': layerType,