[camera] Improve the camera example with controllers initializations #5748
Conversation
|
/cc @hellohuanlin. I'm not sure how to test this so it's a draft, would a callback for initialization or an integer field as a counter sound good? |
|
I've also updated the comparison of how the controller flows before and after this PR. |
# Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/pubspec.yaml
|
Should I request a test exemption? @hellohuanlin |
| cameraController | ||
| .getMaxExposureOffset() | ||
| .then((double value) => _maxAvailableExposureOffset = value), | ||
| ], |
There was a problem hiding this comment.
is this just re-formatting?
|
@AlexV525 For this PR I would write a test such that the old implementation fails, and new implementation fixes the failure. |
I would consider reorganizing the example if a new test is required, some methods are ideal-written currently. |
|
@stuartmorgan Can you chime in here? Should we just land this without tests? Is there a good way to test this? |
There was a problem hiding this comment.
In general I don't think the examples have been directly tested (just used for integration tests), but I don't see why we shouldn't test them. The subtlety of the logic here seems like a good argument for adding tests.
E.g., a test with a mock controller could cover the potential dispose issue in my comment below.
| cameraController | ||
| .getMaxZoomLevel() | ||
| .then((double value) => _maxAvailableZoom = value), | ||
| cameraController | ||
| .getMinZoomLevel() | ||
| .then((double value) => _minAvailableZoom = value), | ||
| ]); | ||
| // `controller` needs to be set until it's fully initialized to avoid a |
There was a problem hiding this comment.
Is this supposed to say "needs to be unset"?
| @@ -651,39 +651,45 @@ class _CameraExampleHomeState extends State<CameraExampleHome> | |||
| imageFormatGroup: ImageFormatGroup.jpeg, | |||
| ); | |||
|
|
|||
| controller = cameraController; | |||
There was a problem hiding this comment.
With this change, wouldn't two calls to onNewCameraSelected in rapid succession have a race where a controller would be replaced without being disposed?
There was a problem hiding this comment.
I think it is not covered in this PR yet. Let me know if the example can be updated/refactored for better architecture or for test purposes.
There was a problem hiding this comment.
Sure, anything we can do to make the example better is a win. We should try to make sure that testability changes are minimally intrusive when possible so they don't distract from understanding of the example itself.
|
Thanks for clarify. I'll submit another PR when it's ready. Closing. |
Related to flutter/flutter#97199.
If we update the controller to the field immediately after it's created but not initialized, the initialization (onNewCameraSelected) will be called again since the first permission request will push the
AppLifecycleStatetoinactive.Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto 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.