Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt Swift 6 concurrency norms #45

Merged
merged 5 commits into from
Aug 9, 2024
Merged

Adopt Swift 6 concurrency norms #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e86c15
Adopt Swift 6 concurrency norms
ianthetechie Aug 8, 2024
e9df3bb
Upgrade Xcode
ianthetechie Aug 8, 2024
7bff6a6
Don't double-run on push and PR
ianthetechie Aug 8, 2024
3771d02
Cleanup
ianthetechie Aug 8, 2024
839f92d
swiftformat
ianthetechie Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: Test

on:
push:
branches: [ "*" ]
branches: [ main ]
pull_request:
branches: [ main ]

Expand All @@ -29,7 +29,7 @@ jobs:
]
destination: [
# TODO: Add more destinations
'platform=iOS Simulator,name=iPhone 15,OS=17.2'
'platform=iOS Simulator,name=iPhone 15,OS=17.5'
]

steps:
Expand All @@ -38,7 +38,7 @@ jobs:

- uses: maxim-lobanov/setup-xcode@v1
with:
xcode-version: '15.2'
xcode-version: '15.4'

- name: Checkout maplibre-swiftui-dsl-playground
uses: actions/checkout@v4
Expand Down
3 changes: 2 additions & 1 deletion Sources/MapLibreSwiftUI/MLNMapViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ import UIKit

public protocol MapViewHostViewController: UIViewController {
associatedtype MapType: MLNMapView
var mapView: MapType { get }
@MainActor var mapView: MapType { get }
}

public final class MLNMapViewController: UIViewController, MapViewHostViewController {
@MainActor
public var mapView: MLNMapView {
view as! MLNMapView
}
Expand Down
12 changes: 4 additions & 8 deletions Sources/MapLibreSwiftUI/MapViewCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,18 +348,14 @@ public class MapViewCoordinator<T: MapViewHostViewController>: NSObject, MLNMapV

/// The MapView's region has changed with a specific reason.
public func mapView(_ mapView: MLNMapView, regionDidChangeWith reason: MLNCameraChangeReason, animated _: Bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, are we ever planning to call this on a non main actor thread? Cause if we're not we could mark the whole function as MainActor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we are not ever planning to call this off the main thread. However we can't currently mark this as MainActor due to the way it's currently implemented obj-c 😅 I can get the exact error when I'm back at my computer in a few hours.

If I can identify a fairly easy fix for that I'll open a PR upstream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's technically a warning...

Main actor-isolated instance method 'mapView(_:regionDidChangeWith:animated:)' cannot be used to satisfy nonisolated protocol requirement

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the warning makes sense. MapLibre will be calling our method here. MapLibre needs to promise us it only calls our implementation on the main thread. We know it does, or at least we think we know it does, but its not ensured by the compiler, and never trust humans 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so Holly Borla (core team member) explicitly endorses the approach taken here as the appropriate one for now in this forum post. SE-0423 will provide a way for us to explicitly mark the conformance as @preconcurrency, but hopefully before then, it's fixed ;)

I will look into possible solutions on the MapLibre side later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im fine with this solution. For me the ideal solution is adjusting the protocol and then fixing any warnings that might arise within maplibre. But I do realize that might be a really deep dive and therefore not reasonable within a normal time frame.

// FIXME: CI complains about MainActor.assumeIsolated being unavailable before iOS 17, despite building on iOS 17.2... This is an epic hack to fix it for now. I can only assume this is an issue with Xcode pre-15.3
// TODO: We could put this in regionIsChangingWith if we calculate significant change/debounce.
Task { @MainActor in
MainActor.assumeIsolated {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff was already on the main thread. We just couldn't prove to Swift that it was, and Xcode pre-15.3 was evidently broken (it took GitHub months to upgrade runners with the patches). This explicitly does the same thing and will let us know if for some reason MapLibre ever does something very evil, while ensuring that it's semantically clear that nothing is blocking / async dispatching here.

updateViewPort(mapView: mapView, reason: reason)
}

guard !suppressCameraUpdatePropagation else {
return
}
guard !suppressCameraUpdatePropagation else {
return
}

// FIXME: CI complains about MainActor.assumeIsolated being unavailable before iOS 17, despite building on iOS 17.2... This is an epic hack to fix it for now. I can only assume this is an issue with Xcode pre-15.3
Task { @MainActor in
updateParentCamera(mapView: mapView, reason: reason)
}
}
Expand Down
Loading