Adopt Swift 6 concurrency norms #45
Conversation
|
Would love a review from @Patrick-Kladek and/or @hactar too. |
| // TODO: We could put this in regionIsChangingWith if we calculate significant change/debounce. | ||
| Task { @MainActor in | ||
| MainActor.assumeIsolated { |
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess it's technically a warning...
Main actor-isolated instance method 'mapView(_:regionDidChangeWith:animated:)' cannot be used to satisfy nonisolated protocol requirement
There was a problem hiding this comment.
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 😊
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What the title says. This should not result in any functional or "real" changes.