Support for complete strict concurrency and Swift 6 migration #456
Conversation
Generated by 🚫 Danger |
DemoApp/Sources/Components/Snapshot/LocalParticipantSnapshotViewModel.swift
Show resolved
Hide resolved
| @@ -173,10 +173,15 @@ final class LocalParticipantSnapshotViewModel: NSObject, AVCapturePhotoCaptureDe | |||
| } | |||
|
|
|||
| /// Provides the default value of the `LocalParticipantSnapshotViewModel` class. | |||
| #if swift(>=6.0) | |||
| struct LocalParticipantSnapshotViewModelKey: @preconcurrency InjectionKey { | |||
There was a problem hiding this comment.
I'm wondering how the environmenrkey on SwiftUI handles that? Is it assigned on MainActor?
There was a problem hiding this comment.
yes, this part is ugly, we have to find a better way
There was a problem hiding this comment.
Let's meet and dicsuccs
There was a problem hiding this comment.
maybe we can create a small package with the DI capabilities. Reuse it in both SwiftUI and Video, with the @preconcurrency import. That way, we can probably avoid this stuff.
| @@ -34,6 +32,8 @@ public class Call: @unchecked Sendable, WSEventsSubscriber { | |||
| /// Provides access to the speaker. | |||
| public let speaker: SpeakerManager | |||
|
|
|||
| @MainActor public internal(set) lazy var state = CallState() | |||
There was a problem hiding this comment.
What's the purpose of having it as lazy?
There was a problem hiding this comment.
the init is not on the main actor, so it was not possible to init the state like this anymore.
There was a problem hiding this comment.
Can we simply mark the init as @mainactor, instead?
There was a problem hiding this comment.
you mean the Call object? I don't think we should enforce that, calls can be created from any thread - only the state should be accessed from the main actor.
Sources/StreamVideoSwiftUI/Utils/PictureInPicture/StreamPictureInPictureVideoRenderer.swift
Outdated
Show resolved
Hide resolved
Sources/StreamVideoSwiftUI/Utils/PictureInPicture/StreamPictureInPictureVideoRenderer.swift
Outdated
Show resolved
Hide resolved
Sources/StreamVideoSwiftUI/Utils/PictureInPicture/StreamPictureInPictureVideoRenderer.swift
Outdated
Show resolved
Hide resolved
…wift into swift6-migration
.github/workflows/smoke-checks.yml
Outdated
| @@ -51,8 +51,8 @@ jobs: | |||
| run: bundle exec fastlane test device:"${{ env.IOS_SIMULATOR_DEVICE }}" | |||
| timeout-minutes: 40 | |||
| env: | |||
| XCODE_VERSION: "15.2" # the most stable pair of Xcode | |||
| IOS_SIMULATOR_DEVICE: "iPhone 15 Pro (17.2)" # and iOS | |||
| XCODE_VERSION: "15.4" # the most stable pair of Xcode | |||
There was a problem hiding this comment.
Temp to see if the CI passes - we might have a problem if nonisolated(unsafe) doesn't work on 15.2.
There was a problem hiding this comment.
What's the outcome on this one?
There was a problem hiding this comment.
we still have a problem here, need to dig into this
There was a problem hiding this comment.
I spent the whole day on this one - there's no good way. nonisolated(unsafe) was introduced in Swift 5.10. Tried with macros and property wrappers, but no luck. Let's discuss it if you have an idea.
There was a problem hiding this comment.
If there's no solution, we would have to drop 5.9.
| app?.endBackgroundTask(activeTask) | ||
| activeBackgroundTask = nil | ||
| executeOnMain { | ||
| self.app?.endBackgroundTask(activeTask) |
There was a problem hiding this comment.
Shall we change that to weak self?
.github/workflows/smoke-checks.yml
Outdated
| @@ -51,8 +51,8 @@ jobs: | |||
| run: bundle exec fastlane test device:"${{ env.IOS_SIMULATOR_DEVICE }}" | |||
| timeout-minutes: 40 | |||
| env: | |||
| XCODE_VERSION: "15.2" # the most stable pair of Xcode | |||
| IOS_SIMULATOR_DEVICE: "iPhone 15 Pro (17.2)" # and iOS | |||
| XCODE_VERSION: "15.4" # the most stable pair of Xcode | |||
There was a problem hiding this comment.
What's the outcome on this one?
| DispatchQueue.global(qos: .userInteractive).async { | ||
| self.captureSession?.startRunning() | ||
| } | ||
| self.captureSession?.startRunning() |
There was a problem hiding this comment.
Why did we remove the background async?
There was a problem hiding this comment.
good catch
| DispatchQueue.global(qos: .userInteractive).async { | ||
| self.captureSession?.stopRunning() | ||
| } | ||
| captureSession?.stopRunning() |
There was a problem hiding this comment.
Why did we remove the background async? Can we replace it instead with a simple task to the GlobalActor?
| let audioRecorder = self.audioRecorder | ||
| Task { | ||
| await audioRecorder.stopRecording() | ||
| } |
There was a problem hiding this comment.
Do you thin that may work?
| let audioRecorder = self.audioRecorder | |
| Task { | |
| await audioRecorder.stopRecording() | |
| } | |
| Task { @MainActor in | |
| await audioRecorder.stopRecording() | |
| } |
There was a problem hiding this comment.
same as here: #456 (comment).
| @@ -87,7 +88,10 @@ struct LobbyContentView: View { | |||
| HStack { | |||
| Spacer() | |||
| Button { | |||
| Task { await microphoneChecker.stopListening() } | |||
| let microphoneChecker = self.microphoneChecker | |||
| Task { | |||
There was a problem hiding this comment.
Can we dispatch to @mainactor and simplify the changeset?
There was a problem hiding this comment.
unfortunately not
| @@ -59,6 +59,7 @@ struct LobbyView_iOS13: View { | |||
| onCloseLobby: onCloseLobby | |||
| ) | |||
| .onReceive(callViewModel.$callSettings) { newValue in | |||
| let microphoneChecker = self.microphoneChecker | |||
| Task { | |||
There was a problem hiding this comment.
suggesstion: @mainactor maybe to simplify the changeset?
| trackSize = .init(width: Int(frame.width), height: Int(frame.height)) | ||
| nonisolated func renderFrame(_ frame: RTCVideoFrame?) { | ||
| nonisolated(unsafe) let unsafeFrame = frame | ||
| Task { @MainActor [weak self] in |
There was a problem hiding this comment.
I'm not sure that running the decoding part on MainActor is wise. What was the part that was complaining? Can we reduce the MainActor calling and move to the background for the transformation?
There was a problem hiding this comment.
there were some things, but I made them nonisolated.
| @@ -5,7 +5,6 @@ | |||
| @testable import StreamVideo | |||
| import XCTest | |||
|
|
|||
| @MainActor | |||
There was a problem hiding this comment.
Wasn't that sufficient?
There was a problem hiding this comment.
nope, since the base class is not a main actor.
SDK Size
|
|
🔗 Issue Links
Resolves https://stream-io.atlassian.net/browse/PBE-4992?atlOrigin=eyJpIjoiZTIyOWY1OWRkN2JlNDcxMWFjOGE1ZDdiMjRlNjQ0NzMiLCJwIjoiaiJ9.
🎯 Goal
Enable complete strict concurrency and make the migration to Swift 6 seamless.
The goal is to have no issues on both Xcodes. The checks and the language capabilities are not the same on both Xcodes, which means we need some #if language checks.
Current state:
Things yet to fix:
As soon as we're happy, we can merge with the Swift 5 version support - it anyway fixes some strict checking warnings (there's no need to wait for Xcode 16).
📝 Summary
We should discuss some of the fixes taken there.
🛠 Implementation
There are few types of issues, did similar things as described here: https://developer.apple.com/wwdc24/10169.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
imgimg🧪 Manual Testing Notes
We should test on both Xcodes. Touch as many features as we can, since there might be some wrong queue assertions we need to fix (that are causing crashes).
☑️ Contributor Checklist
🎁 Meme