MAPSIOS-1249: Remove duplicated annotations, fix crash (#1981)

mapbox · Jan 17, 2024 · 1b7dfcc · 1b7dfcc
1 parent ba7bdc3
commit 1b7dfcc
Show file tree

Hide file tree

Showing 18 changed files with 204 additions and 67 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ Mapbox welcomes participation and contributions from everyone.
 * Make padding optional in `MapboxMap.camera(for:padding:bearing:pitch:maxZoom:offset:)` and `MapboxMap.camera(for:padding:bearing:pitch:)`.
 * Build XCFramework with `SWIFT_SERIALIZE_DEBUGGING_OPTIONS=NO` flag to avoid serialized search paths in Swift modules.
 * Fixed a bug where the attribution dialog does not appear when there is a presented view controller.
+* Fixed a crash that occurs when annotations have duplicate identifiers.
 
 ## 11.1.0-rc.1 - 04 January, 2024
 

diff --git a/Sources/MapboxMaps/Annotations/Annotation.swift b/Sources/MapboxMaps/Annotations/Annotation.swift
@@ -0,0 +1,38 @@
+/// A top-level interface for annotations.
+public protocol Annotation {
+
+    /// The unique identifier of the annotation.
+    var id: String { get }
+
+    /// The geometry that is backing this annotation.
+    var geometry: Geometry { get }
+
+    /// Properties associated with the annotation.
+    @available(*, deprecated, message: "Will be deleted in future, for Mapbox-provided annotations see customData instead.")
+    var userInfo: [String: Any]? { get set }
+}
+
+extension Array where Element: Annotation {
+    /// Deduplicates annotations.
+    mutating func removeDuplicates() {
+        let duplicates = self.removeDuplicates(by: \.id)
+        if !duplicates.isEmpty {
+            let ids = duplicates.lazy.map(\.id).joined(separator: ", ")
+            Log.error(forMessage: "Duplicated annotations: \(ids)", category: "Annotations")
+        }
+    }
+}
+
+extension StyleProtocol {
+    func apply<T: Annotation>(annotationsDiff diff: CollectionDiff<T, String>, sourceId: String, feature: (T) -> Feature) {
+        if !diff.remove.isEmpty {
+            removeGeoJSONSourceFeatures(forSourceId: sourceId, featureIds: diff.remove, dataId: nil)
+        }
+        if !diff.update.isEmpty {
+            updateGeoJSONSourceFeatures(forSourceId: sourceId, features: diff.update.map(feature), dataId: nil)
+        }
+        if !diff.add.isEmpty {
+            addGeoJSONSourceFeatures(forSourceId: sourceId, features: diff.add.map(feature), dataId: nil)
+        }
+    }
+}
diff --git a/Sources/MapboxMaps/Annotations/AnnotationOrchestrator.swift b/Sources/MapboxMaps/Annotations/AnnotationOrchestrator.swift
@@ -1,19 +1,5 @@
 import UIKit
 
-/// A top-level interface for annotations.
-public protocol Annotation {
-
-    /// The unique identifier of the annotation.
-    var id: String { get }
-
-    /// The geometry that is backing this annotation.
-    var geometry: Geometry { get }
-
-    /// Properties associated with the annotation.
-    @available(*, deprecated, message: "Will be deleted in future, for Mapbox-provided annotations see customData instead.")
-    var userInfo: [String: Any]? { get set }
-}
-
 public protocol AnnotationManager: AnyObject {
 
     /// The id of this annotation manager.

diff --git a/Sources/MapboxMaps/Annotations/Generated/CircleAnnotationManager.swift b/Sources/MapboxMaps/Annotations/Generated/CircleAnnotationManager.swift
@@ -14,10 +14,13 @@ public class CircleAnnotationManager: AnnotationManagerInternal {
     public let id: String
 
     /// The collection of ``CircleAnnotation`` being managed.
+    ///
+    /// Each annotation must have a unique identifier. Duplicate IDs will cause only the first annotation to be displayed, while the rest will be ignored.
     public var annotations: [CircleAnnotation] {
         get { mainAnnotations + draggedAnnotations }
         set {
             mainAnnotations = newValue
+            mainAnnotations.removeDuplicates()
             draggedAnnotations.removeAll(keepingCapacity: true)
             draggedAnnotationIndex = nil
         }

diff --git a/Sources/MapboxMaps/Annotations/Generated/PointAnnotationManager.swift b/Sources/MapboxMaps/Annotations/Generated/PointAnnotationManager.swift
@@ -14,10 +14,13 @@ public class PointAnnotationManager: AnnotationManagerInternal {
     public let id: String
 
     /// The collection of ``PointAnnotation`` being managed.
+    ///
+    /// Each annotation must have a unique identifier. Duplicate IDs will cause only the first annotation to be displayed, while the rest will be ignored.
     public var annotations: [PointAnnotation] {
         get { mainAnnotations + draggedAnnotations }
         set {
             mainAnnotations = newValue
+            mainAnnotations.removeDuplicates()
             draggedAnnotations.removeAll(keepingCapacity: true)
             draggedAnnotationIndex = nil
         }

diff --git a/Sources/MapboxMaps/Annotations/Generated/PolygonAnnotationManager.swift b/Sources/MapboxMaps/Annotations/Generated/PolygonAnnotationManager.swift
@@ -14,10 +14,13 @@ public class PolygonAnnotationManager: AnnotationManagerInternal {
     public let id: String
 
     /// The collection of ``PolygonAnnotation`` being managed.
+    ///
+    /// Each annotation must have a unique identifier. Duplicate IDs will cause only the first annotation to be displayed, while the rest will be ignored.
     public var annotations: [PolygonAnnotation] {
         get { mainAnnotations + draggedAnnotations }
         set {
             mainAnnotations = newValue
+            mainAnnotations.removeDuplicates()
             draggedAnnotations.removeAll(keepingCapacity: true)
             draggedAnnotationIndex = nil
         }

diff --git a/Sources/MapboxMaps/Annotations/Generated/PolylineAnnotationManager.swift b/Sources/MapboxMaps/Annotations/Generated/PolylineAnnotationManager.swift
@@ -14,10 +14,13 @@ public class PolylineAnnotationManager: AnnotationManagerInternal {
     public let id: String
 
     /// The collection of ``PolylineAnnotation`` being managed.
+    ///
+    /// Each annotation must have a unique identifier. Duplicate IDs will cause only the first annotation to be displayed, while the rest will be ignored.
     public var annotations: [PolylineAnnotation] {
         get { mainAnnotations + draggedAnnotations }
         set {
             mainAnnotations = newValue
+            mainAnnotations.removeDuplicates()
             draggedAnnotations.removeAll(keepingCapacity: true)
             draggedAnnotationIndex = nil
         }

diff --git a/Sources/MapboxMaps/Annotations/StyleManager+Extensions.swift b/Sources/MapboxMaps/Annotations/StyleManager+Extensions.swift
diff --git a/Sources/MapboxMaps/Foundation/CollectionDiff.swift b/Sources/MapboxMaps/Foundation/CollectionDiff.swift
@@ -1,65 +1,50 @@
-struct CollectionDiff<T> {
-    var remove = [T]()
+struct CollectionDiff<T, ID> {
+    var remove = [ID]()
     var update = [T]()
     var add = [T]()
 
     var isEmpty: Bool { remove.isEmpty && update.isEmpty && add.isEmpty }
 }
 
-extension CollectionDiff: Equatable where T: Equatable {}
+extension CollectionDiff: Equatable where T: Equatable, ID: Equatable {}
 
 extension RandomAccessCollection {
     /// Returns operations needed to perform in order to get `self` from `old` collection.
     /// Treats insertion in the middle as removing all the following elements and re-adding them.
     /// Updates element if its `id` and position are the same, but `old != new`.
     ///
     /// - Complexity: O(n + m), where *n* is length of `self` and *m* is length of `old`.
-    func diff<ID>(from old: Self, id: (Element) -> ID) -> CollectionDiff<Element>
-    where ID: Hashable, Element: Equatable {
-        var result = CollectionDiff<Element>()
-
-        let oldIdsMap = Dictionary(uniqueKeysWithValues: zip(old, old.indices).map { (id($0), $1) })
+    func diff<ID>(from old: Self, id getId: (Element) -> ID) -> CollectionDiff<Element, ID>
+    where ID: Equatable, Element: Equatable {
+        var result = CollectionDiff<Element, ID>()
 
         var oldIt = old.startIndex
         var it = startIndex
 
-        while oldIt != old.endIndex && it != self.endIndex {
+        while oldIt != old.endIndex && it != endIndex {
             let element = self[it]
-            let oldElement: Element = old[oldIt]
-            let newId = id(element)
-            let oldId = id(oldElement)
+            let id = getId(element)
+
+            let oldElement = old[oldIt]
+            let oldId = getId(oldElement)
 
-            if newId == oldId {
+            if id == oldId {
+                // The elements are the same, update them if changed.
                 if oldElement != element {
                     result.update.append(element)
                 }
-                self.formIndex(&it, offsetBy: 1)
+                formIndex(&it, offsetBy: 1)
                 old.formIndex(&oldIt, offsetBy: 1)
-                continue
-            }
 
-            if let foundOldIt = oldIdsMap[newId], foundOldIt > oldIt {
-                while oldIt != foundOldIt {
-                    result.remove.append(old[oldIt])
-                    old.formIndex(&oldIt, offsetBy: 1)
-                }
-                oldIt = foundOldIt
                 continue
             }
 
-            break
-        }
-
-        while it != self.endIndex {
-            result.add.append(self[it])
-            self.formIndex(&it, offsetBy: 1)
-        }
-
-        while oldIt != old.endIndex {
-            result.remove.append(old[oldIt])
+            result.remove.append(oldId)
             old.formIndex(&oldIt, offsetBy: 1)
         }
 
+        result.add.append(contentsOf: self[it...])
+        result.remove.append(contentsOf: old[oldIt...].lazy.map(getId))
         return result
     }
 }
diff --git a/Sources/MapboxMaps/Foundation/Extensions/Array+Extensions.swift b/Sources/MapboxMaps/Foundation/Extensions/Array+Extensions.swift
@@ -0,0 +1,18 @@
+extension Array {
+    /// Removes elements from `self` whose `key` is duplicated with other elements.
+    ///
+    /// - Returns: A list of duplicated keys.
+    mutating func removeDuplicates<Key: Hashable>(by key: (Element) -> Key) -> [Element] {
+        var keys = Set<Key>()
+        var duplicates = [Element]()
+        self.removeAll { el in
+            let k = key(el)
+            let isUnique = keys.insert(k).inserted
+            if !isUnique {
+                duplicates.append(el)
+            }
+            return !isUnique
+        }
+        return duplicates
+    }
+}
diff --git a/Sources/MapboxMaps/Style/RasterArraySource+DataLayer.swift b/Sources/MapboxMaps/Style/RasterArraySource+DataLayer.swift
@@ -17,10 +17,5 @@ extension RasterArraySource {
         @_documentation(visibility: public)
 #endif
         public let bands: [String]
-
-        internal init(layerId: String, bands: [String]) {
-            self.layerId = layerId
-            self.bands = bands
-        }
     }
 }
diff --git a/Sources/MapboxMaps/SwiftUI/Util/Util.swift b/Sources/MapboxMaps/SwiftUI/Util/Util.swift
@@ -12,7 +12,7 @@ func wrapAssignError(_ body: () throws -> Void) {
     do {
         try body()
     } catch {
-        Log.error(forMessage: "Failed to assign property, error: \(error)", category: "swiftui")
+        Log.error(forMessage: "Failed to assign property, error: \(error)", category: "SwiftUI")
     }
 }
 

diff --git a/Tests/MapboxMapsTests/Annotations/Generated/CircleAnnotationManagerTests.swift b/Tests/MapboxMapsTests/Annotations/Generated/CircleAnnotationManagerTests.swift
@@ -747,6 +747,18 @@ final class CircleAnnotationManagerTests: XCTestCase, AnnotationInteractionDeleg
         $displayLink.send()
         XCTAssertEqual(style.updateGeoJSONSourceStub.invocations.count, 0)
     }
+
+    func testRemovingDuplicatedAnnotations() {
+      let annotation1 = CircleAnnotation(id: "A", point: .init(.init(latitude: 1, longitude: 1)), isSelected: false, isDraggable: false)
+      let annotation2 = CircleAnnotation(id: "B", point: .init(.init(latitude: 2, longitude: 2)), isSelected: false, isDraggable: false)
+      let annotation3 = CircleAnnotation(id: "A", point: .init(.init(latitude: 3, longitude: 3)), isSelected: false, isDraggable: false)
+      manager.annotations = [annotation1, annotation2, annotation3]
+
+      XCTAssertEqual(manager.annotations, [
+          annotation1,
+          annotation2
+      ])
+  }
 }
 
 // End of generated file
diff --git a/Tests/MapboxMapsTests/Annotations/Generated/PointAnnotationManagerTests.swift b/Tests/MapboxMapsTests/Annotations/Generated/PointAnnotationManagerTests.swift
@@ -3339,6 +3339,18 @@ final class PointAnnotationManagerTests: XCTestCase, AnnotationInteractionDelega
         $displayLink.send()
         XCTAssertEqual(style.updateGeoJSONSourceStub.invocations.count, 0)
     }
+
+    func testRemovingDuplicatedAnnotations() {
+      let annotation1 = PointAnnotation(id: "A", point: .init(.init(latitude: 1, longitude: 1)), isSelected: false, isDraggable: false)
+      let annotation2 = PointAnnotation(id: "B", point: .init(.init(latitude: 2, longitude: 2)), isSelected: false, isDraggable: false)
+      let annotation3 = PointAnnotation(id: "A", point: .init(.init(latitude: 3, longitude: 3)), isSelected: false, isDraggable: false)
+      manager.annotations = [annotation1, annotation2, annotation3]
+
+      XCTAssertEqual(manager.annotations, [
+          annotation1,
+          annotation2
+      ])
+  }
 }
 
 private extension PointAnnotation {

diff --git a/Tests/MapboxMapsTests/Annotations/Generated/PolygonAnnotationManagerTests.swift b/Tests/MapboxMapsTests/Annotations/Generated/PolygonAnnotationManagerTests.swift
@@ -760,6 +760,39 @@ final class PolygonAnnotationManagerTests: XCTestCase, AnnotationInteractionDele
         $displayLink.send()
         XCTAssertEqual(style.updateGeoJSONSourceStub.invocations.count, 0)
     }
+
+    func testRemovingDuplicatedAnnotations() {
+      let polygonCoords1 = [
+                CLLocationCoordinate2DMake(25.51713945052515, -88.857177734375),
+                CLLocationCoordinate2DMake(25.51713945052515, -86.967529296875),
+                CLLocationCoordinate2DMake(27.244156283890756, -86.967529296875),
+                CLLocationCoordinate2DMake(27.244156283890756, -88.857177734375),
+                CLLocationCoordinate2DMake(25.51713945052515, -88.857177734375)
+            ]
+            let annotation1 = PolygonAnnotation(id: "A", polygon: .init(outerRing: .init(coordinates: polygonCoords1)), isSelected: false, isDraggable: false)
+      let polygonCoords2 = [
+                CLLocationCoordinate2DMake(26.51713945052515, -87.857177734375),
+                CLLocationCoordinate2DMake(26.51713945052515, -85.967529296875),
+                CLLocationCoordinate2DMake(28.244156283890756, -85.967529296875),
+                CLLocationCoordinate2DMake(28.244156283890756, -87.857177734375),
+                CLLocationCoordinate2DMake(26.51713945052515, -87.857177734375)
+            ]
+            let annotation2 = PolygonAnnotation(id: "B", polygon: .init(outerRing: .init(coordinates: polygonCoords2)), isSelected: false, isDraggable: false)
+      let polygonCoords3 = [
+                CLLocationCoordinate2DMake(27.51713945052515, -86.857177734375),
+                CLLocationCoordinate2DMake(27.51713945052515, -84.967529296875),
+                CLLocationCoordinate2DMake(29.244156283890756, -84.967529296875),
+                CLLocationCoordinate2DMake(29.244156283890756, -86.857177734375),
+                CLLocationCoordinate2DMake(27.51713945052515, -86.857177734375)
+            ]
+            let annotation3 = PolygonAnnotation(id: "A", polygon: .init(outerRing: .init(coordinates: polygonCoords3)), isSelected: false, isDraggable: false)
+      manager.annotations = [annotation1, annotation2, annotation3]
+
+      XCTAssertEqual(manager.annotations, [
+          annotation1,
+          annotation2
+      ])
+  }
 }
 
 // End of generated file
diff --git a/Tests/MapboxMapsTests/Annotations/Generated/PolylineAnnotationManagerTests.swift b/Tests/MapboxMapsTests/Annotations/Generated/PolylineAnnotationManagerTests.swift
@@ -1073,6 +1073,21 @@ final class PolylineAnnotationManagerTests: XCTestCase, AnnotationInteractionDel
         $displayLink.send()
         XCTAssertEqual(style.updateGeoJSONSourceStub.invocations.count, 0)
     }
+
+    func testRemovingDuplicatedAnnotations() {
+      let lineCoordinates1 = [ CLLocationCoordinate2DMake(1, 1), CLLocationCoordinate2DMake(11, 11) ]
+            let annotation1 = PolylineAnnotation(id: "A", lineString: .init(lineCoordinates1), isSelected: false, isDraggable: false)
+      let lineCoordinates2 = [ CLLocationCoordinate2DMake(2, 2), CLLocationCoordinate2DMake(12, 12) ]
+            let annotation2 = PolylineAnnotation(id: "B", lineString: .init(lineCoordinates2), isSelected: false, isDraggable: false)
+      let lineCoordinates3 = [ CLLocationCoordinate2DMake(3, 3), CLLocationCoordinate2DMake(13, 13) ]
+            let annotation3 = PolylineAnnotation(id: "A", lineString: .init(lineCoordinates3), isSelected: false, isDraggable: false)
+      manager.annotations = [annotation1, annotation2, annotation3]
+
+      XCTAssertEqual(manager.annotations, [
+          annotation1,
+          annotation2
+      ])
+  }
 }
 
 // End of generated file
diff --git a/Tests/MapboxMapsTests/Foundation/CollectionDiffTests.swift b/Tests/MapboxMapsTests/Foundation/CollectionDiffTests.swift
@@ -55,7 +55,7 @@ final class CollectionDiffTests: XCTestCase {
         XCTAssertEqual(
             result,
             CollectionDiff(
-                remove: [Element(id: 1, value: "1")],
+                remove: [1],
                 update: [Element(id: 2, value: "Y")],
                 add: [Element(id: 3, value: "3")]
             )
@@ -77,7 +77,7 @@ final class CollectionDiffTests: XCTestCase {
         XCTAssertEqual(
             result,
             CollectionDiff(
-                remove: [Element(id: 3, value: "3")],
+                remove: [3],
                 update: [Element(id: 1, value: "X")],
                 add: [
                     Element(id: 2, value: "2"),