-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[camera] Fix ImageStream ImageFormatGroup is Ignored in iOS #4519
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
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.
Thanks for the contribution!
This change looks like it makes sense, but I'm having a hard time reconciling the change with the description of the issue (and the test). It looks like it wasn't being ignored, it was being tracked statically rather than per-instance—which I would expect to result in issues only if there were multiple instances of the camera. Am I missing something important about the old implementation?
@@ -371,6 +370,8 @@ - (instancetype)initWithCameraName:(NSString *)cameraName | |||
_focusMode = FocusModeAuto; | |||
_lockedCaptureOrientation = UIDeviceOrientationUnknown; | |||
_deviceOrientation = orientation; | |||
// Format used for video and image streaming. |
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.
Comments about the purpose of a variable should be on the declaration (in this case, the property) not the assignment.
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.
I fixed it.
Thank you for the review. |
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.
Thanks for the explanation, LGTM!
@cyanglaz for secondary review.
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.
LGTM with a small nit pick. The version will also need to be bumped one more time.
Co-authored-by: Maurice Parrish <[email protected]>
@bparrishMines I resolved the conflicts and applied your suggestion to the CHANGELOG |
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.
LGTM
It looks like setting the default email for an organization is still an issue: todogroup/gh-issues#32. I'll fix the CLA manually. |
Fixes flutter/flutter#93842
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.