[image_picker] fixes for iOS which doesn't present camera/albums with more complex navigation #2755

chris-rutkowski · 2020-05-13T02:25:58Z

Description

Existing implementation is incorrect especially when adding project as a module with more complex presentation structure.
If window's root view controller has presented modally any other view controller, it cannot present any other any more. Only the top most controller can present other.

With this change, the image picker/camera controller is presented on the top most view controller.

Related Issues

I found this issue raised by someone already:
flutter/flutter#34787

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

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.

…image or camera picker. Existing implementation is incorrect especially when adding project as a module with more complex presentation structure. If window's root view controller has presented modally any other view controller, it cannot present any other any more. Only the top most controller can present other. With this change, the image picker/camera controller is presented on the top most view controller.

googlebot · 2020-05-13T03:22:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-13T03:27:14Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-13T06:32:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-13T06:35:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Contains code from the following PRs: flutter#2761 flutter#2755

chris-rutkowski · 2020-05-19T08:12:48Z

@xster @mehmetf @cyanglaz @blasten I saw you merged some other PRs related to image picker recently, that's why I'm @-ing you. Is there anything more I can do here in this PR?

cyanglaz

Thanks for the PR! It looks good overall. I left some comments.

packages/image_picker/image_picker/pubspec.yaml

cyanglaz · 2020-05-19T16:55:40Z

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

 - (UIImagePickerController *)getImagePickerController {
  return _imagePickerController;
 }

+- (UIViewController *)viewController {
+  UIViewController *topController = [UIApplication sharedApplication].keyWindow.rootViewController;


this is great! By the way, while we are here, can we also make it work for navigation controller? (pushed viewcontrollers)

also tab bar controller if possible?

this solution will work in this case as well.
According to the documentation you shouldn't push UIImagePickerController, but present it modally instead.

So in the situation that FlutterViewController is pushed into an existing navigation controller, e.g.:

Window rootViewController someOtherViewController navigation controller (with some native view controller and FlutterVC) image picker controller not Window rootViewController someOtherViewController navigation controller (with some native view controller and FlutterVC, image picker controller)

I was worried about iPad which for a long time required popover presentation for albums, but since iOS 13 it presents modally as well without an issue.

what i meant was that if topViewController is a view controller that is pushed in the navigation stack.
And we want to present the image picker controller on top of that.

@cyanglaz the solution I made will work in virtually all cases, including these you are concerned about.

UINavigationController, UITabBarController they both manage internally some view controllers, but they themselves inherit from UIViewControllerr and either of them can be window's root view controller, or presented on some existing View Controller of any type.

The topViewController cannot be pushed in the navigation stack. In that case, UINavigationCoontroller is topViewController and some other push view controller is managed by UINavigationController. And with that arrangement, my solution will correctly display the Image Picker on top of the navigation controller.

Trust me - in my company we have already very complex navigation - some legacy Drawer Controller (in iOS), multiple Navigation controllers, TabBarControllers and Flutter V.C. is displayed on top of all of this. And my solution has been proven successful.

according to https://developer.apple.com/documentation/uikit/uiviewcontroller/1621407-presentedviewcontroller?language=objc the presentedViewController is only the top view controller when it is presented with presentViewController:animated:completion:.
What if the top view controller is presented by pushViewController? Does this still work?

@cyanglaz I am already repeating myself, but let me rephrase.

presentViewController is a method of UIViewController
UINavigationController, UITabBarController, MySweetViewController - they are all subclasses of UIViewController, therefore they can be presented on any other view controller, or some other view controller can be presented on top of them.

The top view controller presented by push view controller - such a thing doesn't exist.
You can have UINavigationController either as a root of the window, or presented on some other view controller. And Navigation Controller manages the stack of view controllers. These view controllers shouldn't present any other view controller (because they are managed by NC). Instead, you either push another VC in NC (horizontal), or present VC on top of NC (vertical).

Moreover - if UINavigationController is one of the children of UITabBarController, you shouldn't present any VC on top of that NC, you should present on top of UITabBarController instead.

Some diagram

example1: window view controller 1 view controller 2 image picker example2: window navigation controller (home, details) image picker

Ok. I'm interested in this case you presented

example2: window navigation controller (home, details) image picker

In this case, when you go through the code, what is the top view controller? Is it the navigation controller?
And the image picker is going to use the navigation controller as the "viewController", regardless which view controller is the on the top of the stack of the navigation controller? Am I understanding correctly?

exactly, image picker will be correctly presented on navigation controller. The contents of navigation controller doesn't matter at all.

chris-rutkowski · 2020-05-20T02:13:55Z

@cyanglaz as requested changed version to 0.6.6+5

chris-rutkowski · 2020-05-28T04:10:08Z

@cyanglaz any more feedback? I've already updated changelog a few times due to conflicts - other PRs being merged.

aboutlan · 2020-06-01T01:34:40Z

@cyanglaz I have the same problem.
I presented flutterViewController after I use present image_picker doesn't work, Xcode error warning: Attempt to present <UIImagePickerController: 0x7fbbb1856800> on <UINavigationController: 0x7fbbb201c000> whose view is not in the window hierarchy !
@chris-rutkowski PR solved my problem.

diegotori · 2020-06-09T15:39:03Z

@cyanglaz a gentle reminder for reviewing this PR, since this could be very useful to have.

@chris-rutkowski looks like your branch has conflicts that need to be resolved before this can merge. Thank you in advance for this PR.

cyanglaz

LGTM! Can we add tests for this?

window in normal circumstances is nil - in that case the key window will be found adds unit tests

chris-rutkowski · 2020-06-12T05:23:44Z

@cyanglaz added unit test and improved the solution a bit.

BTW I cannot find any guide to run iOS tests. I had to create a dummy project which wasn't very easy.

cyanglaz · 2020-06-12T15:58:50Z

@chris-rutkowski
try

cd ios
pod lib lint

cyanglaz · 2020-06-12T16:01:16Z

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.h

@@ -7,7 +7,7 @@
 @interface FLTImagePickerPlugin : NSObject <FlutterPlugin>

 // For testing only.


Should we remove this line now?

I don't think so? Both of these methods here are exposed only for testing and not meant to be used in normal circumstances.

… more complex navigation (flutter#2755)

googlebot added the cla: yes label May 13, 2020

chris-rutkowski mentioned this pull request May 13, 2020

image_picker:Attempt to present on whose view is not in the window hierarchy flutter/flutter#34787

Closed

googlebot added cla: no and removed cla: yes labels May 13, 2020

chris-rutkowski force-pushed the image_picker_dynamic_top_view_controller branch from d59a83b to 9f58745 Compare May 13, 2020 03:27

googlebot added cla: yes and removed cla: no labels May 13, 2020

chris-rutkowski added 2 commits May 13, 2020 10:28

style and unit tests

a324ff7

fixes format issues

d75a864

googlebot added cla: no and removed cla: yes labels May 13, 2020

chris-rutkowski force-pushed the image_picker_dynamic_top_view_controller branch from cc95b7d to d75a864 Compare May 13, 2020 06:35

googlebot added cla: yes and removed cla: no labels May 13, 2020

bumps version number and updates changelog

7c0ca0f

jlalvarez18 pushed a commit to jlalvarez18/plugins that referenced this pull request May 14, 2020

Temporary Fixes until PRs are Merged

bfc9fa6

Contains code from the following PRs: flutter#2761 flutter#2755

Merge branch 'master' into image_picker_dynamic_top_view_controller

fbeb12b

cyanglaz reviewed May 19, 2020

View reviewed changes

cyanglaz added the in review label May 19, 2020

Merge branch 'master' into image_picker_dynamic_top_view_controller

6262b8b

fixes the test after applying merge from different PRs

7025167

Merge branch 'master' into image_picker_dynamic_top_view_controller

f493fcd

cyanglaz reviewed Jun 12, 2020

View reviewed changes

exposes method in the interface for testability

653b1b1

window in normal circumstances is nil - in that case the key window will be found adds unit tests

adjust indentation

e88ed96

cyanglaz reviewed Jun 12, 2020

View reviewed changes

cyanglaz approved these changes Jun 15, 2020

View reviewed changes

cyanglaz merged commit ff53b70 into flutter:master Jun 15, 2020

chris-rutkowski deleted the image_picker_dynamic_top_view_controller branch June 17, 2020 06:30

chris-rutkowski restored the image_picker_dynamic_top_view_controller branch June 17, 2020 06:30

agent3bood pushed a commit to agent3bood/flutter-plugins that referenced this pull request Jul 10, 2020

[image_picker] fixes for iOS which doesn't present camera/albums with…

3c28fe4

… more complex navigation (flutter#2755)

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[image_picker] fixes for iOS which doesn't present camera/albums with…

bc36bcc

… more complex navigation (flutter#2755)

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[image_picker] fixes for iOS which doesn't present camera/albums with…

1edfad3

… more complex navigation (flutter#2755)

		@@ -7,7 +7,7 @@
		@interface FLTImagePickerPlugin : NSObject <FlutterPlugin>

		// For testing only.

[image_picker] fixes for iOS which doesn't present camera/albums with more complex navigation #2755

[image_picker] fixes for iOS which doesn't present camera/albums with more complex navigation #2755

Uh oh!

Conversation

chris-rutkowski commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

googlebot commented May 13, 2020

Uh oh!

googlebot commented May 13, 2020

Uh oh!

googlebot commented May 13, 2020

Uh oh!

googlebot commented May 13, 2020

Uh oh!

chris-rutkowski commented May 19, 2020

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-rutkowski May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-rutkowski Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-rutkowski commented May 20, 2020

Uh oh!

chris-rutkowski commented May 28, 2020

Uh oh!

aboutlan commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegotori commented Jun 9, 2020

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

chris-rutkowski commented Jun 12, 2020

Uh oh!

cyanglaz commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chris-rutkowski commented May 13, 2020 •

edited

Loading

chris-rutkowski May 29, 2020 •

edited

Loading

chris-rutkowski Jun 11, 2020 •

edited

Loading

aboutlan commented Jun 1, 2020 •

edited

Loading