Ensure AnnotationManager's mapLoaded happens first (#246)

Previously, AnnotationManager subscribed to .mapLoaded using MapboxCoreMaps directly; however, MapboxCoreMaps does not guarantee that observers will be notified in the order in which they subscribe. Among other things, this could result in annotations not being selected when they are tapped. This fix updates AnnotationManager to subscribe via BaseMapView.on which does guarantee notification ordering among its subscribers. Makes AnnotationSupportableMap internal Tests that AnnotationManager subscribes in init
mapbox · Apr 1, 2021 · c10c571 · c10c571
1 parent 07e9934
commit c10c571
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 64 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,15 @@
 
 Mapbox welcomes participation and contributions from everyone.
 
+### Breaking changes ⚠️
+
+- `AnnotationManager` no longer conforms to `Observer` and no longer has a `peer` ([#246](https://github.com/mapbox/mapbox-maps-ios/pull/246))
+- `AnnotationSupportableMap` is now internal ([#246](https://github.com/mapbox/mapbox-maps-ios/pull/246))
+
+### Bug fixes 🐞
+
+- Fixes an issue that could cause issues with annotations including causing them to not be selectable ([#246](https://github.com/mapbox/mapbox-maps-ios/pull/246))
+
 ## 10.0.0-beta.16 - March 29, 2021
 
 ### Breaking changes ⚠️

diff --git a/Sources/MapboxMaps/Annotations/AnnotationManager.swift b/Sources/MapboxMaps/Annotations/AnnotationManager.swift
@@ -17,9 +17,7 @@ import MapboxMapsFoundation
 
  All annotations added with this class belong to a single source and style layer.
  */
-public class AnnotationManager: Observer {
-
-    public var peer: MBXPeerWrapper?
+public class AnnotationManager {
 
     // MARK: - Public properties
 
@@ -130,7 +128,6 @@ public class AnnotationManager: Observer {
 
     deinit {
         self.tapGesture = nil
-        try! self.mapView?.observable?.unsubscribe(for: self, events: [MapEvents.mapLoaded])
     }
 
     /**
@@ -155,7 +152,15 @@ public class AnnotationManager: Observer {
         userInteractionEnabled = true
 
         configureTapGesture()
-        try! mapView.observable?.subscribe(for: self, events: [MapEvents.mapLoaded])
+        mapView.on(.mapLoaded) { [weak self] _ in
+            // Reset the annotation source and default layers.
+            guard let self = self else { return }
+            self.annotations = [:]
+            self.annotationSource = nil
+            self.symbolLayer = nil
+            self.lineLayer = nil
+            self.fillLayer = nil
+        }
     }
 
     internal func updateAnnotationOptions(with newOptions: AnnotationOptions) {
@@ -664,17 +669,4 @@ public class AnnotationManager: Observer {
         // Updating the annotation failed.
         case updateAnnotationFailed(Error?)
     }
-
-    public func notify(for event: MapboxCoreMaps.Event) {
-        guard event.type == MapEvents.mapLoaded else {
-            return
-        }
-
-        // Reset the annotation source and default layers.
-        annotations = [:]
-        annotationSource = nil
-        symbolLayer = nil
-        lineLayer = nil
-        fillLayer = nil
-    }
 }
diff --git a/Sources/MapboxMaps/Annotations/AnnotationSupportableMap.swift b/Sources/MapboxMaps/Annotations/AnnotationSupportableMap.swift
@@ -10,12 +10,12 @@ import MapboxMapsStyle
 import MapboxMapsFoundation
 #endif
 
-public protocol AnnotationSupportableMap: UIView {
-    var observable: Observable? { get }
+internal protocol AnnotationSupportableMap: UIView {
     func visibleFeatures(in rect: CGRect,
                          styleLayers: Set<String>?,
                          filter: Expression?,
                          completion: @escaping (Result<[Feature], BaseMapView.QueryRenderedFeaturesError>) -> Void)
+    func on(_ eventType: MapEvents.EventKind, handler: @escaping (MapboxCoreMaps.Event) -> Void)
 }
 
 extension BaseMapView: AnnotationSupportableMap {

diff --git a/Tests/MapboxMapsTests/Annotations/AnnotationInteractionDelegateTests.swift b/Tests/MapboxMapsTests/Annotations/AnnotationInteractionDelegateTests.swift
@@ -10,8 +10,8 @@ import CoreLocation
 //swiftlint:disable explicit_acl explicit_top_level_acl
 class AnnotationInteractionDelegateTests: XCTestCase {
 
-    var annotationSupportableMapMock: AnnotationSupportableMapMock!
-    var annotationSupportableStyleMock: AnnotationStyleDelegateMock!
+    var annotationSupportableMapMock: MockAnnotationSupportableMap!
+    var annotationSupportableStyleMock: MockAnnotationStyleDelegate!
     var defaultCoordinate: CLLocationCoordinate2D!
 
     var selectionExpectation: XCTestExpectation?
@@ -20,8 +20,8 @@ class AnnotationInteractionDelegateTests: XCTestCase {
     var deselectionDelegateWasCalled: Bool = false
 
     override func setUp() {
-        annotationSupportableMapMock = AnnotationSupportableMapMock()
-        annotationSupportableStyleMock = AnnotationStyleDelegateMock()
+        annotationSupportableMapMock = MockAnnotationSupportableMap()
+        annotationSupportableStyleMock = MockAnnotationStyleDelegate()
         selectionExpectation = expectation(description: "didSelectAnnotation was called")
         defaultCoordinate = CLLocationCoordinate2D(latitude: 0, longitude: 0)
     }

diff --git a/Tests/MapboxMapsTests/Annotations/AnnotationManagerTests.swift b/Tests/MapboxMapsTests/Annotations/AnnotationManagerTests.swift
@@ -2,38 +2,30 @@ import XCTest
 import CoreLocation
 import Turf
 
-#if canImport(MapboxMaps)
 @testable import MapboxMaps
-#else
-@testable import MapboxMapsAnnotations
-@testable import MapboxMapsFoundation
-#endif
 
 //swiftlint:disable explicit_acl explicit_top_level_acl
-class AnnotationManagerTests: XCTestCase {
+final class AnnotationManagerTests: XCTestCase {
 
-    var annotationSupportableMapMock: AnnotationSupportableMapMock!
-    var annotationSupportableStyleMock: AnnotationStyleDelegateMock!
+    var annotationSupportableMap: MockAnnotationSupportableMap!
+    var annotationSupportableStyle: MockAnnotationStyleDelegate!
     var annotationManager: AnnotationManager!
 
-    var defaultCoordinate: CLLocationCoordinate2D!
+    let defaultCoordinate = CLLocationCoordinate2D(latitude: 0, longitude: 0)
 
     override func setUp() {
         // Given
-        annotationSupportableMapMock = AnnotationSupportableMapMock()
-        annotationSupportableStyleMock = AnnotationStyleDelegateMock()
-        annotationManager = AnnotationManager(for: annotationSupportableMapMock,
-                                              with: annotationSupportableStyleMock,
+        annotationSupportableMap = MockAnnotationSupportableMap()
+        annotationSupportableStyle = MockAnnotationStyleDelegate()
+        annotationManager = AnnotationManager(for: annotationSupportableMap,
+                                              with: annotationSupportableStyle,
                                               options: AnnotationOptions())
-
-        defaultCoordinate = CLLocationCoordinate2D(latitude: 0, longitude: 0)
     }
 
     override func tearDown() {
-        annotationSupportableMapMock = nil
-        annotationSupportableStyleMock = nil
+        annotationSupportableMap = nil
+        annotationSupportableStyle = nil
         annotationManager = nil
-        defaultCoordinate = nil
     }
 
     // MARK: - Test adding point annotation
@@ -60,6 +52,8 @@ class AnnotationManagerTests: XCTestCase {
         XCTAssertNil(annotationManager.symbolLayer)
         XCTAssertNil(annotationManager.lineLayer)
         XCTAssertNil(annotationManager.fillLayer)
+        XCTAssertEqual(annotationSupportableMap.onStub.invocations.count, 1)
+        XCTAssertEqual(annotationSupportableMap.onStub.parameters.first?.eventType, .mapLoaded)
     }
 
     func testLayerIdentifiers() {

diff --git a/...tations/AnnotationStyleDelegateMock.swift → ...tations/MockAnnotationStyleDelegate.swift b/...tations/AnnotationStyleDelegateMock.swift → ...tations/MockAnnotationStyleDelegate.swift
@@ -1,15 +1,8 @@
 import UIKit
-
-#if canImport(MapboxMaps)
 @testable import MapboxMaps
-#else
-@testable import MapboxMapsAnnotations
-@testable import MapboxMapsStyle
-import MapboxCoreMaps
-#endif
 
 //swiftlint:disable explicit_acl explicit_top_level_acl
-class AnnotationStyleDelegateMock: AnnotationStyleDelegate {
+final class MockAnnotationStyleDelegate: AnnotationStyleDelegate {
     //swiftlint:disable function_parameter_count
     func setStyleImage(image: UIImage,
                        with identifier: String,

diff --git a/...ations/AnnotationSupportableMapMock.swift → ...ations/MockAnnotationSupportableMap.swift b/...ations/AnnotationSupportableMapMock.swift → ...ations/MockAnnotationSupportableMap.swift
@@ -1,22 +1,9 @@
 import UIKit
-import CoreLocation
 import Turf
-import MapboxCoreMaps
-
-#if canImport(MapboxMaps)
 @testable import MapboxMaps
-#else
-@testable import MapboxMapsAnnotations
-@testable import MapboxMapsFoundation
-import MapboxMapsStyle
-#endif
 
 //swiftlint:disable explicit_acl explicit_top_level_acl
-class AnnotationSupportableMapMock: UIView, AnnotationSupportableMap {
-
-    var observable: Observable? {
-        nil
-    }
+final class MockAnnotationSupportableMap: UIView, AnnotationSupportableMap {
 
     func visibleFeatures(in rect: CGRect,
                          styleLayers: Set<String>?,
@@ -27,4 +14,13 @@ class AnnotationSupportableMapMock: UIView, AnnotationSupportableMap {
         let feature = Feature(Point.init(coord))
         completion(.success([feature]))
     }
+
+    struct OnParameters {
+        var eventType: MapEvents.EventKind
+        var handler: (Event) -> Void
+    }
+    let onStub = Stub<OnParameters, Void>()
+    func on(_ eventType: MapEvents.EventKind, handler: @escaping (Event) -> Void) {
+        return onStub.call(with: OnParameters(eventType: eventType, handler: handler))
+    }
 }