Cleanup in MapContentVisitor (#2001)

* Cleanup in MapContentVisitor * Optimize annotation group updates * Return stable annotations order * Fix tests
mapbox · Jan 26, 2024 · 202ea7f · 202ea7f
1 parent d83148d
commit 202ea7f
Show file tree

Hide file tree

Showing 17 changed files with 89 additions and 66 deletions.
diff --git a/Sources/MapboxMaps/Foundation/CollectionDiff.swift b/Sources/MapboxMaps/Foundation/CollectionDiff.swift
@@ -9,6 +9,10 @@ struct CollectionDiff<T, ID> {
 extension CollectionDiff: Equatable where T: Equatable, ID: Equatable {}
 
 extension RandomAccessCollection {
+    func diff<ID>(from old: Self, id keyPath: KeyPath<Element, ID>) -> CollectionDiff<Element, ID>
+    where ID: Equatable, Element: Equatable {
+        diff(from: old, id: { $0[keyPath: keyPath] })
+    }
     /// 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`.

diff --git a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/CircleAnnotationGroup.swift b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/CircleAnnotationGroup.swift
@@ -85,7 +85,7 @@ public struct CircleAnnotationGroup<Data: RandomAccessCollection, ID: Hashable>:
 
     func _visit(_ visitor: MapContentVisitor) {
         let group = AnnotationGroup(
-            prefixId: visitor.id,
+            positionalId: visitor.positionalId,
             layerId: layerId,
             layerPosition: layerPosition,
             store: store,

diff --git a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PointAnnotationGroup.swift b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PointAnnotationGroup.swift
@@ -83,7 +83,7 @@ public struct PointAnnotationGroup<Data: RandomAccessCollection, ID: Hashable>:
 
     func _visit(_ visitor: MapContentVisitor) {
         let group = AnnotationGroup(
-            prefixId: visitor.id,
+            positionalId: visitor.positionalId,
             layerId: layerId,
             layerPosition: layerPosition,
             store: store,

diff --git a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolygonAnnotationGroup.swift b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolygonAnnotationGroup.swift
@@ -81,7 +81,7 @@ public struct PolygonAnnotationGroup<Data: RandomAccessCollection, ID: Hashable>
 
     func _visit(_ visitor: MapContentVisitor) {
         let group = AnnotationGroup(
-            prefixId: visitor.id,
+            positionalId: visitor.positionalId,
             layerId: layerId,
             layerPosition: layerPosition,
             store: store,

diff --git a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolylineAnnotationGroup.swift b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolylineAnnotationGroup.swift
@@ -84,7 +84,7 @@ public struct PolylineAnnotationGroup<Data: RandomAccessCollection, ID: Hashable
 
     func _visit(_ visitor: MapContentVisitor) {
         let group = AnnotationGroup(
-            prefixId: visitor.id,
+            positionalId: visitor.positionalId,
             layerId: layerId,
             layerPosition: layerPosition,
             store: store,

diff --git a/Sources/MapboxMaps/SwiftUI/Annotations/LayerAnnotationCoordinator.swift b/Sources/MapboxMaps/SwiftUI/Annotations/LayerAnnotationCoordinator.swift
@@ -1,35 +1,36 @@
 @available(iOS 13.0, *)
-class LayerAnnotationCoordinator {
+final class LayerAnnotationCoordinator {
 
     private struct DisplayedAnnotationGroup {
-        var id: AnyHashable
+        var positionalId: AnyHashable
         var stringId: String
         // Maps stable id to string ID for every annotation
         var idsMap: [AnyHashable: String] = [:]
     }
 
     private let annotationOrchestrator: AnnotationOrchestrator
-    private var annotations = [DisplayedAnnotationGroup]()
+    private var displayedAnnotationGroups = [DisplayedAnnotationGroup]()
 
     init(annotationOrchestrator: AnnotationOrchestrator) {
         self.annotationOrchestrator = annotationOrchestrator
     }
 
-    func update(annotations newAnnotations: [(AnyHashable, AnnotationGroup)]) {
-        let displayedIds = annotations.map(\.id)
-        let newIds = newAnnotations.map(\.0)
+    func update(annotations newAnnotations: [AnnotationGroup]) {
+        let displayedIds = displayedAnnotationGroups.map(\.positionalId)
+        let newIds = newAnnotations.map(\.positionalId)
 
-        let diff = newIds.diff(from: displayedIds, id: { $0 })
+        let diff = newIds.diff(from: displayedIds, id: \.self)
 
-        var oldIdTable = Dictionary(uniqueKeysWithValues: annotations.map { ($0.id, $0) })
+        var oldIdTable = Dictionary(uniqueKeysWithValues: displayedAnnotationGroups.map { ($0.positionalId, $0) })
         for removeId in diff.remove {
             guard let stringId = oldIdTable.removeValue(forKey: removeId)?.stringId else { continue }
             annotationOrchestrator.removeAnnotationManager(withId: stringId)
         }
 
-        self.annotations = newAnnotations.map { id, group in
+        self.displayedAnnotationGroups = newAnnotations.map { group in
             let layerId = group.layerId ?? String(UUID().uuidString.prefix(5))
-            var displayedGroup = oldIdTable[id] ?? DisplayedAnnotationGroup(id: id, stringId: layerId)
+            let positionalId = group.positionalId
+            var displayedGroup = oldIdTable[positionalId] ?? DisplayedAnnotationGroup(positionalId: positionalId, stringId: layerId)
             group.update(self.annotationOrchestrator, displayedGroup.stringId, &displayedGroup.idsMap)
             return displayedGroup
         }

diff --git a/Sources/MapboxMaps/SwiftUI/Builders/AnnotationGroup.swift b/Sources/MapboxMaps/SwiftUI/Builders/AnnotationGroup.swift
@@ -12,22 +12,29 @@ protocol MapContentAnnotationManager: AnyObject {
 
 /// Type erasure wrapper for annotation groups.
 struct AnnotationGroup {
+    var positionalId: AnyHashable
     var layerId: String?
     var update: (AnnotationOrchestrator, String, inout [AnyHashable: String]) -> Void
 
-    init(layerId: String? = nil, update: @escaping (AnnotationOrchestrator, String, inout [AnyHashable: String]) -> Void) {
+    init(
+        positionalId: AnyHashable,
+        layerId: String? = nil,
+        update: @escaping (AnnotationOrchestrator, String, inout [AnyHashable: String]) -> Void
+    ) {
+        self.positionalId = positionalId
         self.layerId = layerId
         self.update = update
     }
 
     init<M: MapContentAnnotationManager, Data: RandomAccessCollection, ID: Hashable>(
-        prefixId: [AnyHashable],
+        positionalId: AnyHashable,
         layerId: String?,
         layerPosition: LayerPosition?,
         store: ForEvery<M.AnnotationType, Data, ID>,
         make: @escaping (AnnotationOrchestrator, String, LayerPosition?) -> M,
         updateProperties: @escaping (M) -> Void
     ) {
+        self.positionalId = positionalId
         self.layerId = layerId
         // For some reason, the data in the store corrupts under tests in release mode when captured
         // by `update` closure. Copying the data fixes the issue.
@@ -40,10 +47,10 @@ struct AnnotationGroup {
             updateProperties(manager)
 
             var annotations = [M.AnnotationType]()
-            data.forEach { id, annotation in
+            data.forEach { elementId, annotation in
                 var annotation = annotation
-                let stringId = annotationsIdMap[id] ?? annotation.id
-                annotationsIdMap[id] = stringId
+                let stringId = annotationsIdMap[elementId] ?? annotation.id
+                annotationsIdMap[elementId] = stringId
                 annotation.id = stringId
                 annotation.isDraggable = false
                 annotation.isSelected = false

diff --git a/Sources/MapboxMaps/SwiftUI/Builders/MapContentVisitor.swift b/Sources/MapboxMaps/SwiftUI/Builders/MapContentVisitor.swift
@@ -1,35 +1,40 @@
 import SwiftUI
 
 protocol MapContentVisitor: AnyObject {
-    var id: [AnyHashable] { get }
-    var locationOptions: LocationOptions { get set }
+    var positionalId: [AnyHashable] { get }
+
     @available(iOS 13.0, *)
     func add(viewAnnotation: MapViewAnnotation)
     func add(annotationGroup: AnnotationGroup)
+    func add(locationOptions: LocationOptions)
+
     func visit(id: AnyHashable, content: MapContent)
 }
 
 @available(iOS 13.0, *)
 final class DefaultMapContentVisitor: MapContentVisitor {
-    var locationOptions: LocationOptions = LocationOptions()
-
-    private(set) var id: [AnyHashable] = []
+    private(set) var locationOptions: LocationOptions = LocationOptions()
     private(set) var visitedViewAnnotations: [AnyHashable: MapViewAnnotation] = [:]
+    private(set) var annotationGroups: [AnnotationGroup] = []
 
-    private(set) var annotationGroups: [(AnyHashable, AnnotationGroup)] = []
+    private(set) var positionalId: [AnyHashable] = []
 
     @available(iOS 13.0, *)
     func add(viewAnnotation: MapViewAnnotation) {
-        visitedViewAnnotations[id] = viewAnnotation
+        visitedViewAnnotations[positionalId] = viewAnnotation
     }
 
     func add(annotationGroup: AnnotationGroup) {
-        annotationGroups.append((id, annotationGroup))
+        annotationGroups.append(annotationGroup)
+    }
+
+    func add(locationOptions options: LocationOptions) {
+        locationOptions = options
     }
 
     func visit(id: AnyHashable, content: MapContent) {
-        self.id.append(id)
+        positionalId.append(id)
         content.visit(self)
-        self.id.removeLast()
+        positionalId.removeLast()
     }
 }
diff --git a/Sources/MapboxMaps/SwiftUI/Builders/Puck2D.swift b/Sources/MapboxMaps/SwiftUI/Builders/Puck2D.swift
@@ -110,9 +110,10 @@ public struct Puck2D: PrimitiveMapContent {
     }
 
     func _visit(_ visitor: MapContentVisitor) {
-        visitor.locationOptions = LocationOptions(
+        visitor.add(locationOptions: LocationOptions(
             puckType: .puck2D(configuration),
             puckBearing: bearing ?? .heading,
-            puckBearingEnabled: bearing != nil)
+            puckBearingEnabled: bearing != nil
+        ))
     }
 }
diff --git a/Sources/MapboxMaps/SwiftUI/Builders/Puck3D.swift b/Sources/MapboxMaps/SwiftUI/Builders/Puck3D.swift
@@ -149,9 +149,10 @@ public struct Puck3D: PrimitiveMapContent {
     }
 
     func _visit(_ visitor: MapContentVisitor) {
-        visitor.locationOptions = LocationOptions(
+        visitor.add(locationOptions: LocationOptions(
             puckType: .puck3D(configuration),
             puckBearing: bearing ?? .heading,
-            puckBearingEnabled: bearing != nil)
+            puckBearingEnabled: bearing != nil
+        ))
     }
 }
diff --git a/Tests/MapboxMapsTests/SwiftUI/AnnotationGroupTests.swift b/Tests/MapboxMapsTests/SwiftUI/AnnotationGroupTests.swift
@@ -35,7 +35,7 @@ final class AnnotationGroupTests: XCTestCase {
             DummyAnnotation(property: "foo")
         }
         var group = AnnotationGroup(
-            prefixId: [],
+            positionalId: 0,
             layerId: "layer-id",
             layerPosition: layerPos,
             store: store,
@@ -71,7 +71,7 @@ final class AnnotationGroupTests: XCTestCase {
             DummyAnnotation(property: "bar")
         }
         group = AnnotationGroup(
-            prefixId: [],
+            positionalId: 0,
             layerId: "layer-id",
             layerPosition: layerPos,
             store: store2,

diff --git a/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/CircleAnnotationGroupTests.swift b/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/CircleAnnotationGroupTests.swift
@@ -40,9 +40,9 @@ final class CircleAnnotationGroupTests: XCTestCase {
         // When
         visitor.visit(id: "any-id", content: group)
         let addedGroup = try XCTUnwrap(visitor.annotationGroups.first)
-        XCTAssertEqual(addedGroup.0, ["any-id"])
-        XCTAssertEqual(addedGroup.1.layerId, layerId)
-        addedGroup.1.update(orchestrator, layerId, &annotationIds)
+        XCTAssertEqual(addedGroup.positionalId, ["any-id"])
+        XCTAssertEqual(addedGroup.layerId, layerId)
+        addedGroup.update(orchestrator, layerId, &annotationIds)
 
         // Then
         let stubbed = orchestratorImpl.makeCircleAnnotationManagerStub.invocations[0]

diff --git a/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/PointAnnotationGroupTests.swift b/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/PointAnnotationGroupTests.swift
@@ -42,9 +42,9 @@ final class PointAnnotationGroupTests: XCTestCase {
         // When
         visitor.visit(id: "any-id", content: group)
         let addedGroup = try XCTUnwrap(visitor.annotationGroups.first)
-        XCTAssertEqual(addedGroup.0, ["any-id"])
-        XCTAssertEqual(addedGroup.1.layerId, layerId)
-        addedGroup.1.update(orchestrator, layerId, &annotationIds)
+        XCTAssertEqual(addedGroup.positionalId, ["any-id"])
+        XCTAssertEqual(addedGroup.layerId, layerId)
+        addedGroup.update(orchestrator, layerId, &annotationIds)
 
         // Then
         let stubbed = orchestratorImpl.makePointAnnotationManagerStub.invocations[0]

diff --git a/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/PolygonAnnotationGroupTests.swift b/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/PolygonAnnotationGroupTests.swift
@@ -40,9 +40,9 @@ final class PolygonAnnotationGroupTests: XCTestCase {
         // When
         visitor.visit(id: "any-id", content: group)
         let addedGroup = try XCTUnwrap(visitor.annotationGroups.first)
-        XCTAssertEqual(addedGroup.0, ["any-id"])
-        XCTAssertEqual(addedGroup.1.layerId, layerId)
-        addedGroup.1.update(orchestrator, layerId, &annotationIds)
+        XCTAssertEqual(addedGroup.positionalId, ["any-id"])
+        XCTAssertEqual(addedGroup.layerId, layerId)
+        addedGroup.update(orchestrator, layerId, &annotationIds)
 
         // Then
         let stubbed = orchestratorImpl.makePolygonAnnotationManagerStub.invocations[0]

diff --git a/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/PolylineAnnotationGroupTests.swift b/Tests/MapboxMapsTests/SwiftUI/Annotations/Generated/PolylineAnnotationGroupTests.swift
@@ -40,9 +40,9 @@ final class PolylineAnnotationGroupTests: XCTestCase {
         // When
         visitor.visit(id: "any-id", content: group)
         let addedGroup = try XCTUnwrap(visitor.annotationGroups.first)
-        XCTAssertEqual(addedGroup.0, ["any-id"])
-        XCTAssertEqual(addedGroup.1.layerId, layerId)
-        addedGroup.1.update(orchestrator, layerId, &annotationIds)
+        XCTAssertEqual(addedGroup.positionalId, ["any-id"])
+        XCTAssertEqual(addedGroup.layerId, layerId)
+        addedGroup.update(orchestrator, layerId, &annotationIds)
 
         // Then
         let stubbed = orchestratorImpl.makePolylineAnnotationManagerStub.invocations[0]

diff --git a/Tests/MapboxMapsTests/SwiftUI/LayerAnnotationCoordinatorTests.swift b/Tests/MapboxMapsTests/SwiftUI/LayerAnnotationCoordinatorTests.swift
@@ -10,19 +10,23 @@ final class LayerAnnotationCoordinatorTests: XCTestCase {
         let sut = LayerAnnotationCoordinator(annotationOrchestrator: annotationOrchestrator)
 
         var group1Id: String?
-        let annotationGroup1 = AnnotationGroup(layerId: "layer", update: { orchestrator, groupId, _ in
-            XCTAssertIdentical(orchestrator, annotationOrchestrator)
-            group1Id = groupId
-        })
+        let annotationGroup1 = AnnotationGroup(
+            positionalId: 0,
+            layerId: "layer",
+            update: { orchestrator, groupId, _ in
+                XCTAssertIdentical(orchestrator, annotationOrchestrator)
+                group1Id = groupId
+            }
+        )
 
-        sut.update(annotations: [(0, annotationGroup1)])
+        sut.update(annotations: [annotationGroup1])
         XCTAssertNotNil(group1Id)
 
-        let annotationGroup2 =  AnnotationGroup(layerId: "layer") { orchestrator, groupId, _ in
+        let annotationGroup2 =  AnnotationGroup(positionalId: 0, layerId: "layer") { orchestrator, groupId, _ in
             XCTAssertIdentical(orchestrator, annotationOrchestrator)
             XCTAssertEqual(groupId, group1Id, "Update with previously registered annotation group")
         }
-        sut.update(annotations: [(0, annotationGroup2)])
+        sut.update(annotations: [annotationGroup2])
 
         sut.update(annotations: [])
         XCTAssertEqual(mockAnnotationOrchestratorImpl.removeAnnotationManagerStub.invocations.last?.parameters, group1Id)

diff --git a/Tests/MapboxMapsTests/SwiftUI/MapContentBuilderTests.swift b/Tests/MapboxMapsTests/SwiftUI/MapContentBuilderTests.swift
@@ -9,12 +9,12 @@ final class MapContentBuilderTests: XCTestCase {
 
         @MapContentBuilder func content() -> MapContent {
             DummyMapContent {
-                XCTAssertEqual($0.id.last, 0)
-                visitorIds += $0.id
+                XCTAssertEqual($0.positionalId.last, 0)
+                visitorIds += $0.positionalId
             }
             DummyMapContent {
-                XCTAssertEqual($0.id.last, 1)
-                visitorIds += $0.id
+                XCTAssertEqual($0.positionalId.last, 1)
+                visitorIds += $0.positionalId
             }
         }
 
@@ -24,7 +24,7 @@ final class MapContentBuilderTests: XCTestCase {
 
         XCTAssertEqual(composite.children.count, 2)
         XCTAssertEqual(visitorIds, [0, 1])
-        XCTAssertTrue(visitor.id.isEmpty)
+        XCTAssertTrue(visitor.positionalId.isEmpty)
     }
 
     func testOptionalMapContent() throws {
@@ -35,8 +35,8 @@ final class MapContentBuilderTests: XCTestCase {
             // This will form a nested CompositeMapContent, with one child if condition is true, empty otherwise.
             if condition {
                 DummyMapContent {
-                    XCTAssertEqual($0.id.last, 0)
-                    visitorIds += $0.id
+                    XCTAssertEqual($0.positionalId.last, 0)
+                    visitorIds += $0.positionalId
                 }
             }
         }
@@ -49,7 +49,7 @@ final class MapContentBuilderTests: XCTestCase {
 
         composite.visit(visitor)
         XCTAssertTrue(visitorIds.isEmpty)
-        XCTAssertTrue(visitor.id.isEmpty)
+        XCTAssertTrue(visitor.positionalId.isEmpty)
 
         condition = true
         composite = try XCTUnwrap(content() as? CompositeMapContent)
@@ -59,7 +59,7 @@ final class MapContentBuilderTests: XCTestCase {
 
         composite.visit(visitor)
         XCTAssertEqual(visitorIds, [0, 0])
-        XCTAssertTrue(visitor.id.isEmpty)
+        XCTAssertTrue(visitor.positionalId.isEmpty)
     }
 
     func testConditionalMapContent() throws {
@@ -69,11 +69,11 @@ final class MapContentBuilderTests: XCTestCase {
         @MapContentBuilder func content() -> MapContent {
             if condition {
                 DummyMapContent {
-                    visitorIds += $0.id
+                    visitorIds += $0.positionalId
                 }
             } else {
                 DummyMapContent {
-                    visitorIds += $0.id
+                    visitorIds += $0.positionalId
                 }
             }
         }
@@ -102,7 +102,7 @@ final class MapContentBuilderTests: XCTestCase {
         @MapContentBuilder func content() -> MapContent {
             ForEvery(data, id: \.self) { _ in
                 DummyMapContent {
-                    visitorIds += $0.id
+                    visitorIds += $0.positionalId
                 }
             }
         }