Skip to content

Commit

Permalink
First round of PR review fixes re: concurrency
Browse files Browse the repository at this point in the history
  • Loading branch information
ianthetechie committed Mar 17, 2024
1 parent cb3af3f commit 3c084b3
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 68 deletions.
9 changes: 8 additions & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ let package = Package(
],
swiftSettings: [
.define("MOCKING", .when(configuration: .debug)),
.enableExperimentalFeature("StrictConcurrency"),
]
),
.target(
Expand All @@ -46,10 +47,16 @@ let package = Package(
.target(name: "InternalUtils"),
.product(name: "MapLibre", package: "maplibre-gl-native-distribution"),
.product(name: "MapLibreSwiftMacros", package: "maplibre-swift-macros"),
],
swiftSettings: [
.enableExperimentalFeature("StrictConcurrency"),
]
),
.target(
name: "InternalUtils"
name: "InternalUtils",
swiftSettings: [
.enableExperimentalFeature("StrictConcurrency"),
]
),

// MARK: Tests
Expand Down
1 change: 1 addition & 0 deletions Sources/MapLibreSwiftUI/Examples/Layers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import SwiftUI

// A collection of points with various
// attributes
@MainActor
let pointSource = ShapeSource(identifier: "points") {
// Uses the DSL to quickly construct point features inline
MLNPointFeature(coordinate: CLLocationCoordinate2D(latitude: 51.47778, longitude: -0.00139))
Expand Down
1 change: 1 addition & 0 deletions Sources/MapLibreSwiftUI/Examples/User Location.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import CoreLocation
import SwiftUI

@MainActor
private let locationManager = StaticLocationManager(initialLocation: CLLocation(
coordinate: switzerland,
altitude: 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ import Mockable

@Mockable
protocol MLNMapViewCameraUpdating: AnyObject {
var userTrackingMode: MLNUserTrackingMode { get set }
var minimumPitch: CGFloat { get set }
var maximumPitch: CGFloat { get set }
func setCenter(_ coordinate: CLLocationCoordinate2D,
zoomLevel: Double,
direction: CLLocationDirection,
animated: Bool)
func setZoomLevel(_ zoomLevel: Double, animated: Bool)
func setVisibleCoordinateBounds(
@MainActor var userTrackingMode: MLNUserTrackingMode { get set }
@MainActor var minimumPitch: CGFloat { get set }
@MainActor var maximumPitch: CGFloat { get set }
@MainActor func setCenter(_ coordinate: CLLocationCoordinate2D,
zoomLevel: Double,
direction: CLLocationDirection,
animated: Bool)
@MainActor func setZoomLevel(_ zoomLevel: Double, animated: Bool)
@MainActor func setVisibleCoordinateBounds(
_ bounds: MLNCoordinateBounds,
edgePadding: UIEdgeInsets,
animated: Bool,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ extension MapView {
/// - mapView: The MLNMapView that will host the gesture itself.
/// - context: The UIViewRepresentable context that will orchestrate the response sender
/// - gesture: The gesture definition.
func registerGesture(_ mapView: MLNMapView, _ context: Context, gesture: MapGesture) {
@MainActor func registerGesture(_ mapView: MLNMapView, _ context: Context, gesture: MapGesture) {
switch gesture.method {
case let .tap(numberOfTaps: numberOfTaps):
let gestureRecognizer = UITapGestureRecognizer(target: context.coordinator,
Expand Down Expand Up @@ -41,7 +41,7 @@ extension MapView {
/// - mapView: The MapView emitting the gesture. This is used to calculate the point and coordinate of the
/// gesture.
/// - sender: The UIGestureRecognizer
func processGesture(_ mapView: MLNMapView, _ sender: UIGestureRecognizer) {
@MainActor func processGesture(_ mapView: MLNMapView, _ sender: UIGestureRecognizer) {
guard let gesture = gestures.first(where: { $0.gestureRecognizer == sender }) else {
assertionFailure("\(sender) is not a registered UIGestureRecongizer on the MapView")
return
Expand All @@ -60,8 +60,8 @@ extension MapView {
/// - gesture: The gesture definition for this event.
/// - sender: The UIKit gesture emitting from the map view.
/// - Returns: The calculated context from the sending UIKit gesture
func processContextFromGesture(_ mapView: MLNMapView, gesture: MapGesture,
sender: UIGestureRecognizing) -> MapGestureContext
@MainActor func processContextFromGesture(_ mapView: MLNMapView, gesture: MapGesture,
sender: UIGestureRecognizing) -> MapGestureContext
{
// Build the context of the gesture's event.
let point: CGPoint = switch gesture.method {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import UIKit

@Mockable
protocol UIGestureRecognizing: AnyObject {
var state: UIGestureRecognizer.State { get }
func location(in view: UIView?) -> CGPoint
func location(ofTouch touchIndex: Int, in view: UIView?) -> CGPoint
@MainActor var state: UIGestureRecognizer.State { get }
@MainActor func location(in view: UIView?) -> CGPoint
@MainActor func location(ofTouch touchIndex: Int, in view: UIView?) -> CGPoint
}

extension UIGestureRecognizer: UIGestureRecognizing {
Expand Down
3 changes: 2 additions & 1 deletion Sources/MapLibreSwiftUI/MapView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public struct MapView: UIViewRepresentable {
context.coordinator.updateCamera(mapView: mapView,
camera: $camera.wrappedValue,
animated: false)
mapView.locationManager = mapView.locationManager

// Link the style loaded to the coordinator that emits the delegate event.
context.coordinator.onStyleLoaded = onStyleLoaded
Expand Down Expand Up @@ -88,7 +89,7 @@ public struct MapView: UIViewRepresentable {
animated: isStyleLoaded)
}

private func applyModifiers(_ mapView: MLNMapView, runUnsafe: Bool) {
@MainActor private func applyModifiers(_ mapView: MLNMapView, runUnsafe: Bool) {
mapView.contentInset = mapViewContentInset

mapView.logoView.isHidden = isLogoViewHidden
Expand Down
73 changes: 47 additions & 26 deletions Sources/MapLibreSwiftUI/MapViewCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ public class MapViewCoordinator: NSObject {
// every update cycle so we can avoid unnecessary updates
private var snapshotUserLayers: [StyleLayerDefinition] = []
private var snapshotCamera: MapViewCamera?

// Indicates whether we are currently in a push-down camera update cycle.
// This is necessary in order to ensure we don't keep trying to reset a state value which we were already processing
// an update for.
private var isUpdatingCamera = false

var onStyleLoaded: ((MLNStyle) -> Void)?
var onGesture: (MLNMapView, UIGestureRecognizer) -> Void

Expand Down Expand Up @@ -41,12 +47,13 @@ public class MapViewCoordinator: NSObject {
/// camera related MLNMapView functionality.
/// - camera: The new camera from the binding.
/// - animated: Whether to animate.
func updateCamera(mapView: MLNMapViewCameraUpdating, camera: MapViewCamera, animated: Bool) {
@MainActor func updateCamera(mapView: MLNMapViewCameraUpdating, camera: MapViewCamera, animated: Bool) {
guard camera != snapshotCamera else {
// No action - camera has not changed.
return
}

isUpdatingCamera = true
switch camera.state {
case let .centered(onCoordinate: coordinate, zoom: zoom, pitch: pitch, direction: direction):
mapView.userTrackingMode = .none
Expand Down Expand Up @@ -85,11 +92,12 @@ public class MapViewCoordinator: NSObject {
}

snapshotCamera = camera
isUpdatingCamera = false
}

// MARK: - Coordinator API - Styles + Layers

func updateStyleSource(_ source: MapStyleSource, mapView: MLNMapView) {
@MainActor func updateStyleSource(_ source: MapStyleSource, mapView: MLNMapView) {
switch (source, parent.styleSource) {
case let (.url(newURL), .url(oldURL)):
if newURL != oldURL {
Expand All @@ -98,7 +106,7 @@ public class MapViewCoordinator: NSObject {
}
}

func updateLayers(mapView: MLNMapView) {
@MainActor func updateLayers(mapView: MLNMapView) {
// TODO: Figure out how to selectively update layers when only specific props changed. New function in addition to makeMLNStyleLayer?

// TODO: Extract this out into a separate function or three...
Expand Down Expand Up @@ -195,35 +203,48 @@ extension MapViewCoordinator: MLNMapViewDelegate {

/// The MapView's region has changed with a specific reason.
public func mapView(_ mapView: MLNMapView, regionDidChangeWith reason: MLNCameraChangeReason, animated _: Bool) {
guard !isUpdatingCamera else {
return
}

// If any of these are a mismatch, we know the camera is no longer following a desired method, so we should
// detach and revert to a .centered camera. If any one of these is true, the desired camera state still
// matches the mapView's userTrackingMode
let isProgrammaticallyTracking: Bool = switch parent.camera.state {
case .centered(onCoordinate: _):
false
case .trackingUserLocation:
mapView.userTrackingMode == .follow
case .trackingUserLocationWithHeading:
mapView.userTrackingMode == .followWithHeading
case .trackingUserLocationWithCourse:
mapView.userTrackingMode == .followWithCourse
case .rect(boundingBox: _, edgePadding: _):
false
case .showcase(shapeCollection: _):
false
}
// NOTE: The use of assumeIsolated is just to make Swift strict concurrency checks happy.
// This invariant is upheld by the MLNMapView.
MainActor.assumeIsolated {

Check failure on line 215 in Sources/MapLibreSwiftUI/MapViewCoordinator.swift

View workflow job for this annotation

GitHub Actions / test (MapLibreSwiftUI-Package, platform=iOS Simulator,name=iPhone 15,OS=17.2)

'assumeIsolated(_:file:line:)' is only available in iOS 17.0 or newer

Check failure on line 215 in Sources/MapLibreSwiftUI/MapViewCoordinator.swift

View workflow job for this annotation

GitHub Actions / test (MapLibreSwiftUI-Package, platform=iOS Simulator,name=iPhone 15,OS=17.2)

'assumeIsolated(_:file:line:)' is only available in iOS 17.0 or newer
let userTrackingMode = mapView.userTrackingMode
let isProgrammaticallyTracking: Bool = switch parent.camera.state {
case .trackingUserLocation:
userTrackingMode == .follow
case .trackingUserLocationWithHeading:
userTrackingMode == .followWithHeading
case .trackingUserLocationWithCourse:
userTrackingMode == .followWithCourse
case .centered, .rect, .showcase:
false
}

if isProgrammaticallyTracking {
// Programmatic tracking is still active, we can ignore camera updates until we unset/fail this boolean
// check
return
}
guard !isProgrammaticallyTracking else {
// Programmatic tracking is still active, we can ignore camera updates until we unset/fail this boolean
// check
return
}

DispatchQueue.main.async {
// Publish the MLNMapView's "raw" camera state to the MapView camera binding.
self.parent.camera = .center(mapView.centerCoordinate,
zoom: mapView.zoomLevel,
reason: CameraChangeReason(reason))
// This path only executes when the map view diverges from the parent state, so this is a "matter of fact"
// state propagation.
let newCamera: MapViewCamera = .center(mapView.centerCoordinate,
zoom: mapView.zoomLevel,
// TODO: Pitch doesn't really describe current state
pitch: .freeWithinRange(
minimum: mapView.minimumPitch,
maximum: mapView.maximumPitch
),
direction: mapView.direction,
reason: CameraChangeReason(reason))
snapshotCamera = newCamera
self.parent.camera = newCamera
}
}
}
2 changes: 1 addition & 1 deletion Sources/MapLibreSwiftUI/Models/MapCamera/CameraPitch.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Foundation
import MapLibre

/// The current pitch state for the MapViewCamera
public enum CameraPitch: Hashable {
public enum CameraPitch: Hashable, Sendable {
/// The user is free to control pitch from it's default min to max.
case free

Expand Down
27 changes: 11 additions & 16 deletions Sources/MapLibreSwiftUI/StaticLocationManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,31 +11,20 @@ import MapLibre
/// You can provide a new location by setting the ``lastLocation`` property.
///
/// This class does not ever perform any authorization checks. That is the responsiblity of the caller.
public class StaticLocationManager: NSObject {
public var delegate: (any MLNLocationManagerDelegate)? = nil {
didSet {
DispatchQueue.main.async {
self.delegate?.locationManagerDidChangeAuthorization(self)
self.delegate?.locationManager(self, didUpdate: [self.lastLocation])
}
}
}
public final class StaticLocationManager: NSObject, @unchecked Sendable {
public var delegate: (any MLNLocationManagerDelegate)?

public var authorizationStatus: CLAuthorizationStatus = .authorizedAlways {
didSet {
DispatchQueue.main.async {
self.delegate?.locationManagerDidChangeAuthorization(self)
}
delegate?.locationManagerDidChangeAuthorization(self)
}
}

public var headingOrientation: CLDeviceOrientation = .portrait

public var lastLocation: CLLocation {
didSet {
DispatchQueue.main.async {
self.delegate?.locationManager(self, didUpdate: [self.lastLocation])
}
delegate?.locationManager(self, didUpdate: [lastLocation])
}
}

Expand All @@ -54,7 +43,13 @@ extension StaticLocationManager: MLNLocationManager {
}

public func startUpdatingLocation() {
// Do nothing
// This has to be async dispatched or else the map view will not update immediately if the camera is set to
// follow the user's location. This leads to some REALLY (unbearably) bad artifacts. We should find a better
// solution for this at some point. This is the reason for the @unchecked Sendable conformance by the way (so
// that we don't get a warning about using non-sendable self; it should be safe though).
DispatchQueue.main.async {
self.delegate?.locationManager(self, didUpdate: [self.lastLocation])
}
}

public func stopUpdatingLocation() {
Expand Down
4 changes: 2 additions & 2 deletions Tests/MapLibreSwiftUITests/MapView/MapViewGestureTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ final class MapViewGestureTests: XCTestCase {

// MARK: Gesture Processing

func testTapGesture() {
@MainActor func testTapGesture() {
let gesture = MapGesture(method: .tap(numberOfTaps: 2)) { _ in
// Do nothing
}
Expand All @@ -52,7 +52,7 @@ final class MapViewGestureTests: XCTestCase {
XCTAssertEqual(result.coordinate.longitude, -15, accuracy: 1)
}

func testLongPressGesture() {
@MainActor func testLongPressGesture() {
let gesture = MapGesture(method: .longPress(minimumDuration: 1)) { _ in
// Do nothing
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ final class MapViewCoordinatorCameraTests: XCTestCase {
}
}

func testUnchangedCamera() {
@MainActor func testUnchangedCamera() {
let camera: MapViewCamera = .default()

coordinator.updateCamera(mapView: maplibreMapView, camera: camera, animated: false)
Expand Down Expand Up @@ -50,7 +50,7 @@ final class MapViewCoordinatorCameraTests: XCTestCase {
.called(count: 0)
}

func testCenterCameraUpdate() {
@MainActor func testCenterCameraUpdate() {
let coordinate = CLLocationCoordinate2D(latitude: 12.3, longitude: 23.4)
let newCamera: MapViewCamera = .center(coordinate, zoom: 13)

Expand Down Expand Up @@ -80,7 +80,7 @@ final class MapViewCoordinatorCameraTests: XCTestCase {
.called(count: 0)
}

func testUserTrackingCameraUpdate() {
@MainActor func testUserTrackingCameraUpdate() {
let newCamera: MapViewCamera = .trackUserLocation()

coordinator.updateCamera(mapView: maplibreMapView, camera: newCamera, animated: false)
Expand Down Expand Up @@ -109,7 +109,7 @@ final class MapViewCoordinatorCameraTests: XCTestCase {
.called(count: 1)
}

func testUserTrackingWithCourseCameraUpdate() {
@MainActor func testUserTrackingWithCourseCameraUpdate() {
let newCamera: MapViewCamera = .trackUserLocationWithCourse()

coordinator.updateCamera(mapView: maplibreMapView, camera: newCamera, animated: false)
Expand Down Expand Up @@ -138,7 +138,7 @@ final class MapViewCoordinatorCameraTests: XCTestCase {
.called(count: 1)
}

func testUserTrackingWithHeadingUpdate() {
@MainActor func testUserTrackingWithHeadingUpdate() {
let newCamera: MapViewCamera = .trackUserLocationWithHeading()

coordinator.updateCamera(mapView: maplibreMapView, camera: newCamera, animated: false)
Expand Down

0 comments on commit 3c084b3

Please sign in to comment.