-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[camera_avfoundation] Wrappers swift migration - part 6 #10752
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.
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
[camera_avfoundation] Wrappers swift migration - part 6 #10752
Conversation
packages/camera/camera_avfoundation/example/ios/RunnerTests/Mocks/MockAssetWriter.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request continues the migration of the camera_avfoundation plugin from Objective-C to Swift, focusing on several wrapper and utility classes like FLTSavePhotoDelegate, FLTWritableData, FLTImageStreamHandler, CameraProperties, and QueueUtils. The migration is well-executed, adhering to Swift conventions by removing the 'FLT' prefixes and adopting modern Swift patterns.
My review includes a few suggestions:
- A potential bug in
SavePhotoDelegatewhere anilphoto data case is not handled, leading to a false success report. - A correction for misleading documentation in
SavePhotoDelegate.swift. - A recommendation to use
preconditionFailureinstead ofassertionFailureinCameraProperties.swiftfor a known invalid input to ensure robustness in release builds.
...era_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/SavePhotoDelegate.swift
Show resolved
Hide resolved
...mera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraProperties.swift
Show resolved
Hide resolved
...era_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/SavePhotoDelegate.swift
Outdated
Show resolved
Hide resolved
packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraPropertiesTests.swift
Show resolved
Hide resolved
packages/camera/camera_avfoundation/example/ios/RunnerTests/Mocks/MockWritableData.swift
Outdated
Show resolved
Hide resolved
packages/camera/camera_avfoundation/example/ios/RunnerTests/StreamingTests.swift
Show resolved
Hide resolved
...era_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/SavePhotoDelegate.swift
Show resolved
Hide resolved
...vfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/ThreadSafeEventChannel.swift
Show resolved
Hide resolved
34ecb9b to
8d3b1ab
Compare
|
@hellohuanlin You can take a look again (I've resolved the nit and responded to the rest of the comments) |
...vfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/ThreadSafeEventChannel.swift
Show resolved
Hide resolved
Migrates camera wrappers as part of flutter/flutter#119109 This (last wrappers related 🎉) PR migrates wrappers to Swift: * `FLTSavePhotoDelegate` * `FLTWritableData` * `FLTImageStreamHandler` * `CameraProperties` * `QueueUtils` In line with Swift conventions, the `FLT` prefixes are removed. ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. (cherry picked from commit 2dc24c5)
Migrates camera wrappers as part of flutter/flutter#119109
This (last wrappers related 🎉) PR migrates wrappers to Swift:
FLTSavePhotoDelegateFLTWritableDataFLTImageStreamHandlerCameraPropertiesQueueUtilsIn line with Swift conventions, the
FLTprefixes are removed.Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3