-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[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
Open
raju-muliyashiya
wants to merge
7
commits into
flutter:main
Choose a base branch
from
raju-muliyashiya:fix-detact-immediate-close-of-picker
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+290
−6
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d9e5785
infinite await in picking methods when native picker is closed quickl…
raju-muliyashiya ca44409
Version bump
raju-muliyashiya 1449a47
Code format
raju-muliyashiya 484408e
Address gemini comments
raju-muliyashiya fc79773
code format
raju-muliyashiya bacbdad
code format
raju-muliyashiya 67d7992
determine the state without timer
raju-muliyashiya File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -725,4 +725,218 @@ - (void)testPickVideoSetsCurrentRepresentationMode API_AVAILABLE(ios(14)) { | |||||
| OCMVerifyAll(mockPickerViewController); | ||||||
| } | ||||||
|
|
||||||
| #pragma mark - Test immediate picker close detection | ||||||
|
|
||||||
| - (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] initWithFrame:CGRectMake(0, 0, 100, 100)]; | ||||||
| testWindow.hidden = NO; | ||||||
| [testWindow addSubview:controller.view]; | ||||||
|
|
||||||
| [testWindow setNeedsLayout]; | ||||||
| [testWindow layoutIfNeeded]; | ||||||
|
|
||||||
| // Simulate the picker being removed from the window hierarchy | ||||||
| [controller.view removeFromSuperview]; | ||||||
| }); | ||||||
|
|
||||||
| [self waitForExpectationsWithTimeout:1.0 handler:nil]; | ||||||
| } | ||||||
|
|
||||||
| - (void)testPHPickerImmediateCloseReturnsEmptyArray API_AVAILABLE(ios(14)) { | ||||||
| id photoLibrary = OCMClassMock([PHPhotoLibrary class]); | ||||||
| OCMStub(ClassMethod([photoLibrary authorizationStatus])) | ||||||
| .andReturn(PHAuthorizationStatusAuthorized); | ||||||
|
|
||||||
| FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; | ||||||
|
|
||||||
| XCTestExpectation *resultExpectation = [self expectationWithDescription:@"result"]; | ||||||
|
|
||||||
| [plugin pickMultiImageWithMaxSize:[[FLTMaxSize alloc] init] | ||||||
| quality:nil | ||||||
| fullMetadata:NO | ||||||
| limit:nil | ||||||
| completion:^(NSArray<NSString *> *_Nullable result, | ||||||
| FlutterError *_Nullable error) { | ||||||
| XCTAssertNotNil(result); | ||||||
| XCTAssertEqual(result.count, 0); | ||||||
| XCTAssertNil(error); | ||||||
| [resultExpectation fulfill]; | ||||||
| }]; | ||||||
|
|
||||||
| id mockPresentationController = OCMClassMock([UIPresentationController class]); | ||||||
| [plugin presentationControllerDidDismiss:mockPresentationController]; | ||||||
|
|
||||||
| [self waitForExpectationsWithTimeout:1.0 handler:nil]; | ||||||
| } | ||||||
|
|
||||||
| - (void)testObserverDoesNotInterfereWhenProcessingSelection API_AVAILABLE(ios(14)) { | ||||||
| id photoLibrary = OCMClassMock([PHPhotoLibrary class]); | ||||||
| OCMStub(ClassMethod([photoLibrary authorizationStatus])) | ||||||
| .andReturn(PHAuthorizationStatusAuthorized); | ||||||
|
|
||||||
| FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; | ||||||
|
|
||||||
| XCTestExpectation *resultExpectation = [self expectationWithDescription:@"result"]; | ||||||
| __block BOOL emptyResultReceived = NO; | ||||||
|
|
||||||
| [plugin pickMultiImageWithMaxSize:[[FLTMaxSize alloc] init] | ||||||
| quality:nil | ||||||
| fullMetadata:NO | ||||||
| limit:nil | ||||||
| completion:^(NSArray<NSString *> *_Nullable result, | ||||||
| FlutterError *_Nullable error) { | ||||||
| if (result != nil && result.count > 0) { | ||||||
| emptyResultReceived = NO; | ||||||
| [resultExpectation fulfill]; | ||||||
| } else if (result != nil && result.count == 0) { | ||||||
| emptyResultReceived = YES; | ||||||
| } | ||||||
| }]; | ||||||
|
|
||||||
| NSURL *tiffURL = [[NSBundle bundleForClass:[self class]] URLForResource:@"tiffImage" | ||||||
| withExtension:@"tiff"]; | ||||||
| NSItemProvider *tiffItemProvider = [[NSItemProvider alloc] initWithContentsOfURL:tiffURL]; | ||||||
| PHPickerResult *tiffResult = OCMClassMock([PHPickerResult class]); | ||||||
| OCMStub([tiffResult itemProvider]).andReturn(tiffItemProvider); | ||||||
|
|
||||||
| id mockPickerViewController = OCMClassMock([PHPickerViewController class]); | ||||||
|
|
||||||
| [plugin picker:mockPickerViewController didFinishPicking:@[ tiffResult ]]; | ||||||
|
|
||||||
| dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.2 * NSEC_PER_SEC)), | ||||||
| dispatch_get_main_queue(), ^{ | ||||||
| if (!resultExpectation.inverted) { | ||||||
| XCTAssertFalse(emptyResultReceived, | ||||||
| @"Observer should not fire when processing selection"); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| [self waitForExpectationsWithTimeout:5.0 handler:nil]; | ||||||
| } | ||||||
|
|
||||||
| - (void)testObserverRespectsContextClearing { | ||||||
| id photoLibrary = OCMClassMock([PHPhotoLibrary class]); | ||||||
| OCMStub(ClassMethod([photoLibrary authorizationStatus])) | ||||||
| .andReturn(PHAuthorizationStatusAuthorized); | ||||||
|
|
||||||
| FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; | ||||||
| UIImagePickerController *controller = [[UIImagePickerController alloc] init]; | ||||||
| [plugin setImagePickerControllerOverrides:@[ controller ]]; | ||||||
|
|
||||||
| XCTestExpectation *resultExpectation = [self expectationWithDescription:@"result"]; | ||||||
| __block NSInteger completionCallCount = 0; | ||||||
|
|
||||||
| [plugin pickImageWithSource:[FLTSourceSpecification makeWithType:FLTSourceTypeGallery | ||||||
| camera:FLTSourceCameraRear] | ||||||
| maxSize:[[FLTMaxSize alloc] init] | ||||||
| quality:nil | ||||||
| fullMetadata:NO | ||||||
| completion:^(NSString *_Nullable result, FlutterError *_Nullable error) { | ||||||
| completionCallCount++; | ||||||
| [resultExpectation fulfill]; | ||||||
| }]; | ||||||
|
|
||||||
| XCTAssertNotNil(plugin.callContext, @"Context should be set after pickImage call"); | ||||||
|
|
||||||
| plugin.callContext = nil; | ||||||
|
|
||||||
| UIView *controllerView = controller.view; | ||||||
| if (controllerView) { | ||||||
| UIWindow *testWindow = [[UIWindow alloc] init]; | ||||||
| [testWindow addSubview:controllerView]; | ||||||
| [controllerView removeFromSuperview]; | ||||||
| } | ||||||
|
|
||||||
| dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.2 * NSEC_PER_SEC)), | ||||||
| dispatch_get_main_queue(), ^{ | ||||||
| XCTAssertLessThanOrEqual(completionCallCount, 1, | ||||||
| @"Observer should not fire after context is cleared"); | ||||||
| if (completionCallCount == 0) { | ||||||
| [resultExpectation fulfill]; | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| [self waitForExpectationsWithTimeout:1.0 handler:nil]; | ||||||
| } | ||||||
|
|
||||||
| - (void)testObserverDelayAllowsDelegateMethodsToRunFirst { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
| id photoLibrary = OCMClassMock([PHPhotoLibrary class]); | ||||||
| OCMStub(ClassMethod([photoLibrary authorizationStatus])) | ||||||
| .andReturn(PHAuthorizationStatusAuthorized); | ||||||
|
|
||||||
| FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; | ||||||
| UIImagePickerController *controller = [[UIImagePickerController alloc] init]; | ||||||
| [plugin setImagePickerControllerOverrides:@[ controller ]]; | ||||||
|
|
||||||
| XCTestExpectation *resultExpectation = [self expectationWithDescription:@"result"]; | ||||||
| __block NSInteger callCount = 0; | ||||||
|
|
||||||
| [plugin pickImageWithSource:[FLTSourceSpecification makeWithType:FLTSourceTypeGallery | ||||||
| camera:FLTSourceCameraRear] | ||||||
| maxSize:[[FLTMaxSize alloc] init] | ||||||
| quality:nil | ||||||
| fullMetadata:NO | ||||||
| completion:^(NSString *_Nullable result, FlutterError *_Nullable error) { | ||||||
| callCount++; | ||||||
| if (callCount == 1) { | ||||||
| XCTAssertNil(result); | ||||||
| XCTAssertNil(error); | ||||||
|
|
||||||
| UIView *controllerView = controller.view; | ||||||
| if (controllerView) { | ||||||
| UIWindow *testWindow = [[UIWindow alloc] init]; | ||||||
| [testWindow addSubview:controllerView]; | ||||||
| [controllerView removeFromSuperview]; | ||||||
| } | ||||||
|
|
||||||
| dispatch_after( | ||||||
| dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.2 * NSEC_PER_SEC)), | ||||||
| dispatch_get_main_queue(), ^{ | ||||||
| XCTAssertEqual( | ||||||
| callCount, 1, | ||||||
| @"Observer should not fire after context cleared by cancel"); | ||||||
| [resultExpectation fulfill]; | ||||||
| }); | ||||||
| } | ||||||
| }]; | ||||||
|
|
||||||
| [plugin imagePickerControllerDidCancel:controller]; | ||||||
|
|
||||||
| [self waitForExpectationsWithTimeout:1.0 handler:nil]; | ||||||
| } | ||||||
|
|
||||||
| @end | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,48 @@ - (instancetype)initWithResult:(nonnull FlutterResultAdapter)result { | |
| } | ||
| @end | ||
|
|
||
| /** | ||
| * A callback function that is invoked when the PickerViewController is removed from the window. | ||
| * This callback is used to handle cleanup operations and notify the plugin when the picker | ||
| * interface has been dismissed, either through user interaction or programmatically. | ||
| */ | ||
| typedef void (^FLTImagePickerRemoveCallback)(void); | ||
|
|
||
| /** | ||
| * A UIView subclass that monitors the removal of a PickerViewController from the window hierarchy. | ||
| * | ||
| * This observer view is added to the PickerViewController's view and monitors changes to its window | ||
| * property. When the PickerViewController is removed from the screen (either through user dismissal | ||
| * or programmatic dismissal), this view detects the change and triggers the associated callback. | ||
| * | ||
| * This mechanism ensures that the plugin receives notification when the picker is dismissed under | ||
| * various circumstances, including interactive dismissal gestures that occur before the | ||
| * PickerViewController has fully appeared on screen. | ||
| */ | ||
| @interface FLTImagePickerRemoveObserverView : UIView | ||
|
|
||
| @property(nonatomic, copy, nonnull) FLTImagePickerRemoveCallback removeCallback; | ||
|
|
||
| - (instancetype)initWithRemoveCallback:(FLTImagePickerRemoveCallback)callback; | ||
|
|
||
| @end | ||
|
|
||
| @implementation FLTImagePickerRemoveObserverView | ||
|
|
||
| - (instancetype)initWithRemoveCallback:(FLTImagePickerRemoveCallback)callback { | ||
| if (self = [super init]) { | ||
| self.removeCallback = callback; | ||
| } | ||
| return self; | ||
| } | ||
| - (void)didMoveToWindow { | ||
| if (!self.window) { | ||
| [self removeFromSuperview]; | ||
| [[NSOperationQueue mainQueue] addOperationWithBlock:self.removeCallback]; | ||
| } | ||
| } | ||
| @end | ||
|
|
||
| #pragma mark - | ||
|
|
||
| @interface FLTImagePickerPlugin () | ||
|
|
@@ -115,6 +157,7 @@ - (void)launchPHPickerWithContext:(nonnull FLTImagePickerMethodCallContext *)con | |
| pickerViewController.presentationController.delegate = self; | ||
| self.callContext = context; | ||
|
|
||
| [self bindRemoveObserver:pickerViewController context:context]; | ||
| [self showPhotoLibraryWithPHPicker:pickerViewController]; | ||
| } | ||
|
|
||
|
|
@@ -138,6 +181,7 @@ - (void)launchUIImagePickerWithSource:(nonnull FLTSourceSpecification *)source | |
|
|
||
| self.callContext = context; | ||
|
|
||
| [self bindRemoveObserver:imagePickerController context:context]; | ||
| switch (source.type) { | ||
| case FLTSourceTypeCamera: | ||
| [self checkCameraAuthorizationWithImagePicker:imagePickerController | ||
|
|
@@ -158,6 +202,19 @@ - (void)launchUIImagePickerWithSource:(nonnull FLTSourceSpecification *)source | |
| } | ||
| } | ||
|
|
||
| - (void)bindRemoveObserver:(nonnull UIViewController *)controller | ||
| context:(nonnull FLTImagePickerMethodCallContext *)context { | ||
| __weak typeof(self) weakSelf = self; | ||
| FLTImagePickerRemoveObserverView *removeObserverView = | ||
| [[FLTImagePickerRemoveObserverView alloc] initWithRemoveCallback:^{ | ||
| __strong typeof(weakSelf) strongSelf = weakSelf; | ||
| if (strongSelf && strongSelf.callContext == context && !strongSelf.isProcessingSelection) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does |
||
| [strongSelf sendCallResultWithSavedPathList:nil]; | ||
| } | ||
| }]; | ||
| [controller.view addSubview:removeObserverView]; | ||
| } | ||
|
|
||
| #pragma mark - FLTImagePickerApi | ||
|
|
||
| - (void)pickImageWithSource:(nonnull FLTSourceSpecification *)source | ||
|
|
@@ -464,6 +521,8 @@ - (NSNumber *)getDesiredImageQuality:(NSNumber *)imageQuality { | |
| #pragma mark - UIAdaptivePresentationControllerDelegate | ||
|
|
||
| - (void)presentationControllerDidDismiss:(UIPresentationController *)presentationController { | ||
| self.isProcessingSelection = YES; | ||
|
|
||
| [self sendCallResultWithSavedPathList:nil]; | ||
| } | ||
|
|
||
|
|
@@ -476,6 +535,7 @@ - (void)picker:(PHPickerViewController *)picker | |
| [self sendCallResultWithSavedPathList:nil]; | ||
| return; | ||
| } | ||
| self.isProcessingSelection = YES; | ||
| __block NSOperationQueue *saveQueue = [[NSOperationQueue alloc] init]; | ||
| saveQueue.name = @"Flutter Save Image Queue"; | ||
| saveQueue.qualityOfService = NSQualityOfServiceUserInitiated; | ||
|
|
@@ -489,15 +549,13 @@ - (void)picker:(PHPickerViewController *)picker | |
| NSMutableArray *pathList = [[NSMutableArray alloc] initWithCapacity:results.count]; | ||
| __block FlutterError *saveError = nil; | ||
| __weak typeof(self) weakSelf = self; | ||
| // This operation will be executed on the main queue after | ||
| // all selected files have been saved. | ||
| NSBlockOperation *sendListOperation = [NSBlockOperation blockOperationWithBlock:^{ | ||
| if (saveError != nil) { | ||
| [weakSelf sendCallResultWithError:saveError]; | ||
| } else { | ||
| [weakSelf sendCallResultWithSavedPathList:pathList]; | ||
| } | ||
| // Retain queue until here. | ||
| weakSelf.isProcessingSelection = NO; | ||
| saveQueue = nil; | ||
| }]; | ||
|
|
||
|
|
@@ -522,22 +580,23 @@ - (void)picker:(PHPickerViewController *)picker | |
| [saveQueue addOperation:saveOperation]; | ||
| }]; | ||
|
|
||
| // Schedule the final Flutter callback on the main queue | ||
| // to be run after all images have been saved. | ||
| [NSOperationQueue.mainQueue addOperation:sendListOperation]; | ||
| } | ||
|
|
||
| #pragma mark - UIImagePickerControllerDelegate | ||
|
|
||
| - (void)imagePickerController:(UIImagePickerController *)picker | ||
| didFinishPickingMediaWithInfo:(NSDictionary<NSString *, id> *)info { | ||
| self.isProcessingSelection = YES; | ||
|
|
||
| NSURL *videoURL = info[UIImagePickerControllerMediaURL]; | ||
| [picker dismissViewControllerAnimated:YES completion:nil]; | ||
| // The method dismissViewControllerAnimated does not immediately prevent | ||
| // further didFinishPickingMediaWithInfo invocations. A nil check is necessary | ||
| // to prevent below code to be unwantly executed multiple times and cause a | ||
| // crash. | ||
| if (!self.callContext) { | ||
| self.isProcessingSelection = NO; | ||
| return; | ||
| } | ||
| if (videoURL != nil) { | ||
|
|
@@ -618,6 +677,8 @@ - (void)imagePickerController:(UIImagePickerController *)picker | |
| } | ||
|
|
||
| - (void)imagePickerControllerDidCancel:(UIImagePickerController *)picker { | ||
| self.isProcessingSelection = YES; | ||
|
|
||
| [picker dismissViewControllerAnimated:YES completion:nil]; | ||
| [self sendCallResultWithSavedPathList:nil]; | ||
| } | ||
|
|
@@ -660,6 +721,7 @@ - (void)sendCallResultWithSavedPathList:(nullable NSArray *)pathList { | |
| self.callContext.result(pathList ?: [NSArray array], nil); | ||
| } | ||
| self.callContext = nil; | ||
| self.isProcessingSelection = NO; | ||
| } | ||
|
|
||
| /// Sends the given error via `callContext.result` as the result of the original platform channel | ||
|
|
@@ -672,6 +734,7 @@ - (void)sendCallResultWithError:(FlutterError *)error { | |
| } | ||
| self.callContext.result(nil, error); | ||
| self.callContext = nil; | ||
| self.isProcessingSelection = NO; | ||
| } | ||
|
|
||
| @end | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
update changelog and pubspec version
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.
as current is 0.8.13+3
0.8.14 should be next right?