-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[camera] Prevent crash when setting unsupported FocusMode #3992
Conversation
mvanbeusekom
left a comment
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.
Looks good in general, I have two nits that would be good to correct.
Co-authored-by: Maurits van Beusekom <maurits@vnbskm.nl>
mvanbeusekom
left a comment
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!
Is https://github.com/flutter/plugins/blob/master/packages/camera/camera/ios/Tests/CameraPluginTests.m not being compiled then? (We have an iOS unit test location mismatch that crept in at some point; I've raised it with jmagman to decide on a canonical location and standardize the repo.) |
Let me check with the cli tools in the plugin repo if it was actually running, but at least it was not visible in the xcode project which makes editing the file way harder. Edit: I just verified with the following branches: Both have this commit: Baseflow@fe2e132 And I ran the test doing: The before has the commit before this PR was merged and 'All XCTests have passed', the after has failing tests. PS: The PR body had to be updated, because the faulty test was fixed in b0858f6 |
* Check if chosen focus mode is supported * adding unit tests * Added tests for camera focus * Fail 1 test on purpose * formatting * Fix copyright notion * Fix test run * Add documentation and changelog * Update packages/camera/camera/CHANGELOG.md Co-authored-by: Maurits van Beusekom <maurits@vnbskm.nl> * Improve documentation Co-authored-by: Maurits van Beusekom <maurits@vnbskm.nl>
iOS supports 3 focus modes:
Flutter has
The second (AutoFocus) focuses once and then switches to Fixed.
Previously when setting the mode to 'Auto' the iOS code did check if ContinuousAutoFocus was supported and otherwise set AutoFocus. Setting locked always let iOS set mode to AutoFocus. However, not all devices support AutoFocus, for example the front camera of iPad (5th generation) does not. The FocusMode is always Locked.
My proposed solution is to just not set the FocusMode if it is not supported as that is the same behaviour already done when
ContinuousAutoFocusis not supported. I can also throw a PlatformException so the caller in the dart side knows that the mode is not supported. However, I don't know if you would also give a warning ifContinuousAutoFocusis not supported and you switch toAutoFocusinstead.I also need to added a test project as there was none yet, this test project contains a failing test to check the CI.
The change is basically this:
from:
to:
Fixes flutter/flutter#76498
Pre-launch Checklist
dart format. See plugin_tool format)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.