From 202ea7f82b1c338fd4f9b5addc3dfff802e5d875 Mon Sep 17 00:00:00 2001 From: Aleksei Sapitskii <45671572+aleksproger@users.noreply.github.com> Date: Fri, 26 Jan 2024 14:37:12 +0200 Subject: [PATCH] Cleanup in MapContentVisitor (#2001) * Cleanup in MapContentVisitor * Optimize annotation group updates * Return stable annotations order * Fix tests --- .../Foundation/CollectionDiff.swift | 4 +++ .../Generated/CircleAnnotationGroup.swift | 2 +- .../Generated/PointAnnotationGroup.swift | 2 +- .../Generated/PolygonAnnotationGroup.swift | 2 +- .../Generated/PolylineAnnotationGroup.swift | 2 +- .../LayerAnnotationCoordinator.swift | 21 ++++++++-------- .../SwiftUI/Builders/AnnotationGroup.swift | 17 +++++++++---- .../SwiftUI/Builders/MapContentVisitor.swift | 25 +++++++++++-------- .../MapboxMaps/SwiftUI/Builders/Puck2D.swift | 5 ++-- .../MapboxMaps/SwiftUI/Builders/Puck3D.swift | 5 ++-- .../SwiftUI/AnnotationGroupTests.swift | 4 +-- .../CircleAnnotationGroupTests.swift | 6 ++--- .../Generated/PointAnnotationGroupTests.swift | 6 ++--- .../PolygonAnnotationGroupTests.swift | 6 ++--- .../PolylineAnnotationGroupTests.swift | 6 ++--- .../LayerAnnotationCoordinatorTests.swift | 18 +++++++------ .../SwiftUI/MapContentBuilderTests.swift | 24 +++++++++--------- 17 files changed, 89 insertions(+), 66 deletions(-) diff --git a/Sources/MapboxMaps/Foundation/CollectionDiff.swift b/Sources/MapboxMaps/Foundation/CollectionDiff.swift index bafdf84cb418..200cded33c5e 100644 --- a/Sources/MapboxMaps/Foundation/CollectionDiff.swift +++ b/Sources/MapboxMaps/Foundation/CollectionDiff.swift @@ -9,6 +9,10 @@ struct CollectionDiff { extension CollectionDiff: Equatable where T: Equatable, ID: Equatable {} extension RandomAccessCollection { + func diff(from old: Self, id keyPath: KeyPath) -> CollectionDiff + 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 index cffe4274a37a..6e49fed281ff 100644 --- a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/CircleAnnotationGroup.swift +++ b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/CircleAnnotationGroup.swift @@ -85,7 +85,7 @@ public struct CircleAnnotationGroup: 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 index c572a3c33cdd..7c8c301f80a4 100644 --- a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PointAnnotationGroup.swift +++ b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PointAnnotationGroup.swift @@ -83,7 +83,7 @@ public struct PointAnnotationGroup: 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 index 056ce69c1e55..375c23486ea7 100644 --- a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolygonAnnotationGroup.swift +++ b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolygonAnnotationGroup.swift @@ -81,7 +81,7 @@ public struct PolygonAnnotationGroup 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 index 80a6e97625d3..e7815879869d 100644 --- a/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolylineAnnotationGroup.swift +++ b/Sources/MapboxMaps/SwiftUI/Annotations/Generated/PolylineAnnotationGroup.swift @@ -84,7 +84,7 @@ public struct PolylineAnnotationGroup 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( - prefixId: [AnyHashable], + positionalId: AnyHashable, layerId: String?, layerPosition: LayerPosition?, store: ForEvery, 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 index 54b756e18ca3..13f439a9db9c 100644 --- 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 index a8c689455a02..52c512c5bc8c 100644 --- 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 index 65dff23937af..ddf95fac3c55 100644 --- 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 index 3445170164fe..1de18279bca2 100644 --- 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 index 932422742c34..efbe4e82d4fa 100644 --- 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 index dfe3516a19a5..2e2042b88af1 100644 --- 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 index 236d95741f5a..03efd377220d 100644 --- 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 index 87f405c07f65..bad289fcb6cf 100644 --- 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 index 111d3a82cbcb..8bb19c18ab4a 100644 --- 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 index fba48ddd3a04..6c2a15278390 100644 --- 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 } } }