-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[image_picker_ios] infinite await in picking methods when native picker is closed quickly #10460
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
base: main
Are you sure you want to change the base?
[image_picker_ios] infinite await in picking methods when native picker is closed quickly #10460
Conversation
…y - fixes #134715
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 addresses an issue where picking methods could await indefinitely if the native picker was closed quickly. The fix introduces a FLTImagePickerRemoveObserverView, a clever solution that uses the view lifecycle to detect when the picker is dismissed, covering edge cases not handled by standard delegate methods. A new isProcessingSelection flag prevents this new mechanism from interfering with normal selection flows. The changes are accompanied by a comprehensive set of new tests to validate the fix and prevent regressions. My review includes a few suggestions to improve documentation, code style, and one of the new tests for better robustness and maintainability.
| /** | ||
| * a callback function what the PickerViewController remove from window. | ||
| */ | ||
| typedef void (^FLTImagePickerRemoveCallback)(void); | ||
|
|
||
| /** | ||
| * Add the view to the PickerViewController's view, observing its window to observe the window of PickerViewController. | ||
| * This is to prevent PickerViewController from being removed from the screen without receiving callback information under other circumstances, | ||
| * such as being interactively dismissed before PickerViewController has fully popped up. | ||
| */ |
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 documentation for FLTImagePickerRemoveCallback and FLTImagePickerRemoveObserverView could be improved for clarity and grammar. Clear documentation is important for long-term maintainability.
/**
* A callback that is invoked when the picker view controller is removed from the window.
*/
typedef void (^FLTImagePickerRemoveCallback)(void);
/**
* A view that uses `didMoveToWindow` to observe when it has been removed from a window.
*
* This is added to the picker's view hierarchy to detect when the picker has been dismissed
* in cases where the standard delegate methods are not called (e.g., a quick interactive
* dismissal).
*/|
|
||
| @implementation FLTImagePickerRemoveObserverView | ||
|
|
||
| - (instancetype)initWithRemoveCallback:(FLTImagePickerRemoveCallback)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.
| FLTImagePickerRemoveObserverView *removeObserverView = | ||
| [[FLTImagePickerRemoveObserverView alloc]initWithRemoveCallback:^{ | ||
| __strong typeof(weakSelf) strongSelf = weakSelf; | ||
| // Add a small delay to ensure delegate methods have a chance to run first | ||
| // This prevents the observer from firing during normal selection flow | ||
| dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ | ||
| if(strongSelf && strongSelf.callContext == context && !strongSelf.isProcessingSelection) { |
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.
There are a couple of minor style issues here (missing spaces) that deviate from the project's coding style. Consistent formatting improves code readability.
FLTImagePickerRemoveObserverView *removeObserverView =
[[FLTImagePickerRemoveObserverView alloc] initWithRemoveCallback:^{
__strong typeof(weakSelf) strongSelf = weakSelf;
// Add a small delay to ensure delegate methods have a chance to run first
// This prevents the observer from firing during normal selection flow
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
if (strongSelf && strongSelf.callContext == context && !strongSelf.isProcessingSelection) {| - (void)testUIImagePickerImmediateCloseReturnsEmptyArray { | ||
| FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; | ||
|
|
||
| XCTestExpectation *resultExpectation = [self expectationWithDescription:@"result"]; | ||
|
|
||
| FLTImagePickerMethodCallContext *context = [[FLTImagePickerMethodCallContext alloc] | ||
| initWithResult:^void(NSArray<NSString *> *paths, FlutterError *error) { | ||
| if (paths == nil || paths.count == 0) { | ||
| XCTAssertNil(error); | ||
| [resultExpectation fulfill]; | ||
| } | ||
| }]; | ||
| context.includeImages = YES; | ||
| context.maxSize = [[FLTMaxSize alloc] init]; | ||
| context.maxItemCount = 1; | ||
| context.requestFullMetadata = NO; | ||
|
|
||
| plugin.callContext = context; | ||
|
|
||
| UIImagePickerController *controller = [[UIImagePickerController alloc] init]; | ||
| UIView *controllerView = controller.view; | ||
|
|
||
| UIView *observerView = [[UIView alloc] init]; | ||
| [controllerView addSubview:observerView]; | ||
|
|
||
| void (^removeCallback)(void) = ^{ | ||
| dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ | ||
| if (plugin && plugin.callContext == context && !plugin.isProcessingSelection) { | ||
| [plugin sendCallResultWithSavedPathList:nil]; | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| UIWindow *testWindow = [[UIWindow alloc] initWithFrame:CGRectMake(0, 0, 100, 100)]; | ||
| testWindow.hidden = NO; | ||
| [testWindow addSubview:controllerView]; | ||
|
|
||
| [testWindow setNeedsLayout]; | ||
| [testWindow layoutIfNeeded]; | ||
|
|
||
| [controllerView removeFromSuperview]; | ||
|
|
||
| removeCallback(); | ||
|
|
||
| [self waitForExpectationsWithTimeout:1.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.
This test manually creates a removeCallback that duplicates the implementation logic from bindRemoveObserver:. This makes the test brittle; if the implementation changes, this test would need to be updated manually and might not fail correctly. It also doesn't test the new FLTImagePickerRemoveObserverView class and its didMoveToWindow trigger.
A better approach would be to write this as an integration test that calls one of the pick... methods to set up the real observer, and then simulates the dismissal to trigger it. This would provide more robust testing of the actual code path.
- (void)testUIImagePickerImmediateCloseReturnsEmptyArray {
FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init];
UIImagePickerController *controller = [[UIImagePickerController alloc] init];
[plugin setImagePickerControllerOverrides:@[ controller ]];
// Mock camera access to avoid permission dialogs and device-specific logic.
id mockUIImagePicker = OCMClassMock([UIImagePickerController class]);
OCMStub(ClassMethod([mockUIImagePicker isSourceTypeAvailable:UIImagePickerControllerSourceTypeCamera]))
.andReturn(YES);
OCMStub(ClassMethod([mockUIImagePicker isCameraDeviceAvailable:UIImagePickerControllerCameraDeviceRear]))
.andReturn(YES);
id mockAVCaptureDevice = OCMClassMock([AVCaptureDevice class]);
OCMStub([mockAVCaptureDevice authorizationStatusForMediaType:AVMediaTypeVideo])
.andReturn(AVAuthorizationStatusAuthorized);
XCTestExpectation *resultExpectation = [self expectationWithDescription:@"result"];
FLTSourceSpecification *source = [FLTSourceSpecification makeWithType:FLTSourceTypeCamera
camera:FLTSourceCameraRear];
[plugin pickImageWithSource:source
maxSize:[[FLTMaxSize alloc] init]
quality:nil
fullMetadata:NO
completion:^(NSString *_Nullable result, FlutterError *_Nullable error) {
XCTAssertNil(result);
XCTAssertNil(error);
[resultExpectation fulfill];
}];
// The `pickImage` call will attach the observer. Now, simulate dismissal.
// This needs to happen on the next run loop to ensure the observer is attached.
dispatch_async(dispatch_get_main_queue(), ^{
UIWindow *testWindow = [[UIWindow alloc] init];
[testWindow addSubview:controller.view];
[controller.view removeFromSuperview];
});
[self waitForExpectationsWithTimeout:1.0 handler:nil];
}
fixes Issue #134715
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
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 under[^1].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 under[^1].///).