Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

chgibb
Copy link
Contributor

@chgibb chgibb commented Jul 12, 2019

Description

This PR resolves the description given in the title. image_picker hangs on iOS simulator after being rejected for the camera being unavailable.

Related Issues

flutter/flutter #18095 describes very similar behaviour, except with respect to the gallery and video picker.
flutter/plugins #595 introduced the FLTImagePickerPlugin#imagePickerControllerDidCancel method, however it is never actually called anywhere in the code base. This PR simply makes use of it.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@chgibb chgibb requested a review from cyanglaz as a code owner July 12, 2019 16:00
@cyanglaz
Copy link
Contributor

I believe the imagePickerControllerDidCancel is a delegate method in the UIImagePickerControllerDelegate so it should be called by the system when the image picker is cancelled.
I feel like this issue can be fixed by bringing the below code outside of the camera authorization check.

 if ([UIImagePickerController isSourceTypeAvailable:UIImagePickerControllerSourceTypeCamera]) {
   _imagePickerController.sourceType = UIImagePickerControllerSourceTypeCamera;
   [_viewController presentViewController:_imagePickerController animated:YES completion:nil];
 } else {
   [[[UIAlertView alloc] initWithTitle:@"Error"
                               message:@"Camera not available."
                              delegate:nil
                     cancelButtonTitle:@"OK"
                     otherButtonTitles:nil] show];
 }

@cyanglaz cyanglaz self-assigned this Jul 12, 2019
@chgibb
Copy link
Contributor Author

chgibb commented Jul 12, 2019

A full search for the name returns no results except the declaration. As for factoring out the code you showed, what exactly do you propose? I am only passingly familiar with Objective-C.

@cyanglaz
Copy link
Contributor

@chgibb no worries. So OBJC has a delegation pattern. Basically our ImagePickerPlugin conform to the UIImagePickerControllerDelegate protocol. One of the method in the protocol is imagePickerControllerDidCancel. So the iOS framework is calling [UIImagePickerControllerDelegate imagePickerControllerDidCancel] somewhere. Delegate methods are not usually meant to be called manually.
That said, my hunch is that the issue is that we didn't check the camera availability before checking the camera authorization. We were checking the availability after authorization. See showCamera.
So ideally, if we move this logic that i posted outside and check the camera availability first, then check the authorization, we might be able to fix the issue.

@chgibb
Copy link
Contributor Author

chgibb commented Jul 12, 2019

@cyanglaz I believe that fundamentally, a result is not being returned in the event that the camera is not available. The else branch after the check for availability displays a dialog alerting the use that the camera is not available but does not return a result back to Flutter. Adding

self.result(nil);
self.result = nil;
_arguments = nil;

immediately following the "Camera not available" dialog also appears to resolve the issue. I realize that is simply the method body of the didCancel delegate, but it was my understanding that lines 122-124 already checked for camera availability.

@cyanglaz
Copy link
Contributor

Adding

self.result(nil);
self.result = nil;
_arguments = nil;

makes much more sense! Do you mind to update the PR with this change?

@chgibb
Copy link
Contributor Author

chgibb commented Jul 12, 2019

@cyanglaz absolutely! I will make the change on Monday and ping you again for review and approval. I appreciate the timely and kind discussion and feedback!

@chgibb
Copy link
Contributor Author

chgibb commented Jul 15, 2019

Made the change @cyanglaz .

@cyanglaz
Copy link
Contributor

@chadidi CI seems not happy about some formatting, do you mind reformat?

@cyanglaz cyanglaz merged commit a8d3347 into flutter:master Jul 15, 2019
mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants