-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[camera] Run iOS methods on UI thread by default #4140
[camera] Run iOS methods on UI thread by default #4140
Conversation
# Conflicts: # packages/camera/camera/CHANGELOG.md
266f96e
to
8bc557f
Compare
packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m
Outdated
Show resolved
Hide resolved
Co-authored-by: Maurits van Beusekom <[email protected]>
packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m
Outdated
Show resolved
Hide resolved
/** | ||
Called when result is successful. Sends "successWithData" to the notification center. | ||
*/ | ||
- (void)successWithData:(id)data { |
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.
Shouldn't there be a thread assertion in this code?
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.
What do you mean with the thread assertion? That we verify that it is not the main thread?
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 don't remember what I meant by having that comment in this location unfortunately; maybe I was forgetting the flow and thinking this was after the bounce back to the main thread.
However, the more general questions is still valid: the goal of this PR is to ensure that methods are getting callbacks on the main thread, so why isn't there any code in the test that calls into the camera handler with a result object controlled by the test, and actually asserts that the callback was on the main thread? Like this: https://github.com/flutter/plugins/blob/master/packages/local_auth/example/ios/RunnerTests/FLTLocalAuthPluginTests.m#L62-L64
It is not clear to me what in this test would deterministically fail without the fix from this PR.
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.
@stuartmorgan I have now added ThreadSafeFlutterResultTests that test that the result is always called on the main thread. These tests in CameraMethodChannelTests just verify that ThreadSafeFlutterResult is used, not how that is working.
packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m
Outdated
Show resolved
Hide resolved
packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m
Outdated
Show resolved
Hide resolved
…ading # Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/ios/Classes/CameraPlugin.m # packages/camera/camera/pubspec.yaml
9e3fb80
to
98fe08a
Compare
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 overall, mostly just nits around test fixtures.
@@ -23,6 +23,11 @@ - (void)setUp { | |||
self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:self.mockMessenger]; | |||
} | |||
|
|||
- (void)tearDown { | |||
self.mockMessenger = nil; | |||
self.cameraPlugin = nil; |
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 would actually much rather the ivars, the setup, and the teardown all be removed, and those two lines move into the test. Per the article I linked in a recent PR comment, stateless fixtures >> stateful fixtures, and in this case we're only saving two lines of boilerplate (assuming we eventually have more tests in this file; right now this is actually more code in order to theoretically reduce future duplication.)
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.
Done.
|
||
@interface CameraMethodChannelTests : XCTestCase | ||
@property(readonly, nonatomic) CameraPlugin *camera; | ||
@property(readonly, nonatomic) MockFLTThreadSafeFlutterResult *resultObject; |
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.
Per the comment in the other file: please remove these and make them local to the test. (Having the object under test be part of the fixture state is always a major red flag to me unless there's a very compelling reason.)
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.
Done.
packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m
Show resolved
Hide resolved
@end | ||
|
||
@implementation CameraPreviewPauseTests | ||
|
||
- (void)setUp { | ||
_camera = [[FLTCam alloc] init]; | ||
|
||
_resultObject = [[MockFLTThreadSafeFlutterResult alloc] init]; |
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.
This should be local (and while you're here, you could move _camera
into the tests. It's one line, and locality of setup of the object under test is absolutely worth one extra line of boilerplate.)
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.
Done.
* read the received result. | ||
*/ | ||
@interface MockFLTThreadSafeFlutterResult : FLTThreadSafeFlutterResult | ||
@property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation; |
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.
Can't this use the nonnull
@property
decoration?
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.
Yes this is possible, I used the Xcode "Fix" feature when the analyser warned me to specify a nonnull
attribute and it added the _Nonnull
declaration behind the pointer. It can be replaced with the nonnull
attribute in the @property
aswell (see update).
@@ -14,6 +15,7 @@ | |||
NS_DESIGNATED_INITIALIZER; | |||
- (instancetype)init NS_UNAVAILABLE; | |||
|
|||
- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result; |
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.
Needs a declaration 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.
Done.
@@ -14,6 +15,7 @@ | |||
NS_DESIGNATED_INITIALIZER; | |||
- (instancetype)init NS_UNAVAILABLE; | |||
|
|||
- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result; | |||
- (void)orientationChanged:(NSNotification *)notification; |
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.
Blank line between methods please.
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.
Done.
/** | ||
* Gets the original FlutterResult object wrapped by this FLTThreadSafeFlutterResult instance. | ||
*/ | ||
@property(readonly, nonatomic) FlutterResult _Nonnull flutterResult; |
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.
nonnull
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.
Done.
@property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation; | ||
@property(nonatomic, nullable) id receivedResult; | ||
|
||
- (instancetype _Nonnull)initWithExpectation:(XCTestExpectation *_Nonnull)expectation; |
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.
These should be nonnull
as well.
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.
Done.
* Initializes with a FlutterResult object. | ||
* @param result The FlutterResult object that the result will be given to. | ||
*/ | ||
- (nonnull id)initWithResult:(nonnull FlutterResult)result; |
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.
instancetype
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.
Fixed.
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.
@stuartmorgan I resolved all nits, would appreciate it if you could have another look.
|
||
@interface CameraMethodChannelTests : XCTestCase | ||
@property(readonly, nonatomic) CameraPlugin *camera; | ||
@property(readonly, nonatomic) MockFLTThreadSafeFlutterResult *resultObject; |
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.
Done.
packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m
Show resolved
Hide resolved
@@ -23,6 +23,11 @@ - (void)setUp { | |||
self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:self.mockMessenger]; | |||
} | |||
|
|||
- (void)tearDown { | |||
self.mockMessenger = nil; | |||
self.cameraPlugin = nil; |
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.
Done.
@end | ||
|
||
@implementation CameraPreviewPauseTests | ||
|
||
- (void)setUp { | ||
_camera = [[FLTCam alloc] init]; | ||
|
||
_resultObject = [[MockFLTThreadSafeFlutterResult alloc] init]; |
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.
Done.
* read the received result. | ||
*/ | ||
@interface MockFLTThreadSafeFlutterResult : FLTThreadSafeFlutterResult | ||
@property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation; |
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.
Yes this is possible, I used the Xcode "Fix" feature when the analyser warned me to specify a nonnull
attribute and it added the _Nonnull
declaration behind the pointer. It can be replaced with the nonnull
attribute in the @property
aswell (see update).
* Called when result is successful. | ||
* | ||
* Fulfills the expectation. | ||
*/ |
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.
Removed.
@@ -14,6 +15,7 @@ | |||
NS_DESIGNATED_INITIALIZER; | |||
- (instancetype)init NS_UNAVAILABLE; | |||
|
|||
- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result; |
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.
Done.
@@ -14,6 +15,7 @@ | |||
NS_DESIGNATED_INITIALIZER; | |||
- (instancetype)init NS_UNAVAILABLE; | |||
|
|||
- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result; | |||
- (void)orientationChanged:(NSNotification *)notification; |
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.
Done.
/** | ||
* Gets the original FlutterResult object wrapped by this FLTThreadSafeFlutterResult instance. | ||
*/ | ||
@property(readonly, nonatomic) FlutterResult _Nonnull flutterResult; |
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.
Done.
* Initializes with a FlutterResult object. | ||
* @param result The FlutterResult object that the result will be given to. | ||
*/ | ||
- (nonnull id)initWithResult:(nonnull FlutterResult)result; |
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.
Fixed.
7362ca7
to
9093887
Compare
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 some last nits. Thanks for getting this over the line!
XCTAssertNil(result); | ||
[resultExpectation fulfill]; | ||
}]; | ||
[self waitForExpectationsWithTimeout:2.0 handler:nil]; |
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.
Don't you still need this (and an expectation passed to the MockFLTThreadSafeFlutterResult
initialization) so that the assertions are guaranteed to run after the result callback?
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 don't think so, the [CameraPlugin pausePreviewWithResult:]
method that is tested has a very simple (synchronised) implementation and will not run its logic on a different queue (dispatching on different queue is done by the [CameraPlugin handleMethodCall]
method).
Also the result object that is passed in is a simple mock implementation of the FLTThreadSafeFlutterResult
class which simply echo's the value that is received by calling the [MockFLTThreadSafeFlutterResult sendSuccessWithData:]
method through the MockFLTThreadSafeFlutterResult.receivedResult
property.
So as far as I understand everything will run synchronously after each other.
XCTAssertNil(result); | ||
[resultExpectation fulfill]; | ||
}]; | ||
[self waitForExpectationsWithTimeout:2.0 handler:nil]; |
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.
Same.
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.
See explanation above.
* The expectation is fullfilled when a result is called allowing tests to await the result in an | ||
* asynchronous manner. | ||
*/ | ||
- (instancetype _Nonnull)initWithExpectation:(nonnull XCTestExpectation *)expectation; |
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.
nonnull instancetype
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.
Fixed.
/// Exposes the [CameraPlugin handleMethodCallAsync:result:] method for unit testing. | ||
/// | ||
/// This method should always be dispatched on a background queue to prevent deadlocks. | ||
|
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.
Remove the blank line here please.
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.
Done.
- (instancetype)init NS_UNAVAILABLE; | ||
|
||
/// Exposes the [CameraPlugin handleMethodCallAsync:result:] method for unit testing. |
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.
The comments on these methods should be normal declaration comments explaining what the method actually does. The "exposed for unit testing" part belongs as a comment on the category itself (where it already is, correctly).
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.
Improved the documentation as suggested.
19ca7bd
to
1d51203
Compare
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.
Changes LGTM (this didn't need another review round)
* master: [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418)
* master: [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter/pubspec.yaml
* master: (364 commits) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442) [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418) ... # Conflicts: # packages/google_maps_flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
…ideo_src_on_same_controller * commit '76e84c0679dbab7bfaaaa553b17bb0dbdb9a3c33': (537 commits) [video_player] Initialize player when size and duration become available (flutter#4438) [webview_flutter] Implement zoom enabled for ios and android (flutter#4417) Partial revert of "upgraded usage of BinaryMessenger (flutter#4451)" (flutter#4453) Implement Android WebView api with pigeon (Java portion) (flutter#4441) [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434) Implement Android WebView api with pigeon (Dart portion) (flutter#4435) upgraded usage of BinaryMessenger (flutter#4451) [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442) [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) ... # Conflicts: # packages/video_player/video_player/pubspec.yaml # packages/video_player/video_player_web/lib/video_player_web.dart # packages/video_player/video_player_web/pubspec.yaml
In flutter/flutter#25959 we found that not everything in the iOS camera implementation should be run on the UI thread, as that blocks the app UI. The solution was to run everything on a different thread.
In flutter/flutter#52578 we found that we should not run everything on a different thread, because for example sending the result should be done on the UI thread.
In PR #4007 it is decided that things should run on UI thread by default and exceptions are made for heavy operations.
In flutter/flutter#25959 the author stated that "it's recommended that AVCaptureSession operations be called on a dedicated serial DispatchQueue". That means that
initWithCameraName
,start
andstop
need to be run on a different thread, but I'm not sure if these are the only ones blocking the UI.Pre-launch Checklist
dart format
.)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.