[image_picker] add requestFullMetadata for iOS (optional permissions) - iOS changes #5713

PiotrMitkowski · 2022-05-13T09:36:14Z

Description

iOS changes for #5915

Related issues

flutter/flutter#65995

Pre-launch Checklist

PiotrMitkowski · 2022-05-13T09:37:35Z

@stuartmorgan @cyanglaz I moved iOS specific changes to a separate branch, as suggested.

cyanglaz · 2022-05-13T16:56:29Z

packages/image_picker/image_picker_ios/example/pubspec.yaml

@@ -16,7 +16,7 @@ dependencies:
    # The example app is bundled with the plugin so we use a path dependency on
    # the parent directory to use the current plugin's version.
    path: ../
-  image_picker_platform_interface: ^2.3.0
+  image_picker_platform_interface: ^2.5.0


2.4.0 was the broken version that I reverted in 2.4.1

packages/image_picker/image_picker_ios/lib/image_picker_ios.dart

stuartmorgan

Looks good with some minor nits

stuartmorgan · 2022-05-13T17:30:36Z

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

@@ -119,7 +119,12 @@ - (void)launchPHPickerWithContext:(nonnull FLTImagePickerMethodCallContext *)con
  _pickerViewController.presentationController.delegate = self;
  self.callContext = context;

-  [self checkPhotoAuthorizationForAccessLevel];
+  BOOL requestFullMetadata = context.requestFullMetadata;


Nit: there's no value to this local variable given how short and readable the right hand side is; you can just inline context.requestFullMetadata into the if condition.

stuartmorgan · 2022-05-13T17:31:09Z

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

@@ -129,14 +134,24 @@ - (void)launchUIImagePickerWithSource:(nonnull FLTSourceSpecification *)source
  imagePickerController.delegate = self;
  imagePickerController.mediaTypes = @[ (NSString *)kUTTypeImage ];
  self.callContext = context;
+  BOOL requestFullMetadata = context.requestFullMetadata;


Same here. Especially since this is only used once, and conditionally, so doesn't make sense to define at such a high level.

stuartmorgan · 2022-05-13T17:31:51Z

packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m

@@ -177,6 +179,27 @@ - (void)testPickMultiImageShouldUseUIImagePickerControllerOnPreiOS14 {
            [mockUIImagePicker setSourceType:UIImagePickerControllerSourceTypePhotoLibrary]);
 }

+- (void)testPickImageWithoutFullMetadataPreiOS14 {


Why is this test pre-14?

I realized that it was a wrong test case, not fully covering the new flag's use case and vulnerable to false positives. I've changed it so it now verifies that there was no check for gallery authorization status.

stuartmorgan · 2022-05-13T17:32:27Z

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

@@ -227,14 +244,19 @@ - (void)pickVideoWithSource:(nonnull FLTSourceSpecification *)source
  }

  self.callContext = context;
+  BOOL requestFullMetadata = context.requestFullMetadata;


Same here; just inline the expression.

stuartmorgan · 2022-05-13T17:32:46Z

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

@@ -553,8 +575,13 @@ - (void)imagePickerController:(UIImagePickerController *)picker
    NSNumber *maxHeight = self.callContext.maxSize.height;
    NSNumber *imageQuality = self.callContext.imageQuality;
    NSNumber *desiredImageQuality = [self getDesiredImageQuality:imageQuality];
+    BOOL requestFullMetadata = _callContext.requestFullMetadata;


stuartmorgan · 2022-05-13T17:33:24Z

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

-    PHAsset *originalAsset = [FLTImagePickerPhotoAssetUtil getAssetFromImagePickerInfo:info];
+    PHAsset *originalAsset;
+    if (requestFullMetadata) {
+      // Full metadata are available only in PHAsset, which requires gallery permission


Nit: missing period.

…ermissions-ios-changes � Conflicts: � packages/image_picker/image_picker_ios/CHANGELOG.md � packages/image_picker/image_picker_ios/pubspec.yaml

jmagman · 2022-05-31T22:44:03Z

Friendly bump, looks like the nits were addressed.

stuartmorgan

Sorry, one more issue I missed last time.

stuartmorgan · 2022-06-01T00:10:14Z

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

@@ -234,7 +249,11 @@ - (void)pickVideoWithSource:(nonnull FLTSourceSpecification *)source
                                             camera:[self cameraDeviceForSource:source]];
      break;
    case FLTSourceTypeGallery:
-      [self checkPhotoAuthorizationWithImagePicker:imagePickerController];
+      if (context.requestFullMetadata) {


Doesn't this need the same @available check and fallback?

I just realized that this change for picking a video can't be used with the current platform interface. It doesn't allow passing requestFullMetadata for both multi-image and videos. I can either improve the interface or remove this change for video picking (depending on what you think is better).

I don't see any reason we wouldn't want support for this in the multi-image case, so updating that interface makes sense.

Looking at the result callback, is this change even relevant for video? It looks like the parameter is only used in a non-video codepath.

Okay, I'll update the interface and create a PR soon. I'll also remove usage of requestFullMetadata for the video path, since it it isn't used, as you've noticed.

Looks like this is still missing the iOS 11 @available check.

I've removed usage of requestFullMetadata from here since it isn't supported in the video picking.

cyanglaz

LGTM when Stuart's comments are addressed.

cyanglaz · 2022-06-30T15:55:21Z

@PiotrMitkowski Friendly ping :) Are you planning on continuing this PR?

PiotrMitkowski · 2022-06-30T18:29:53Z

@cyanglaz Sure, I got stuck on waiting for review of this one: #5914, which blocks further work on adding requestFullMetadata flag. I wanted to avoid too much pinging, but I guess it's a good time to post it on Discord :)

cyanglaz · 2022-06-30T18:32:05Z

I will take a look at #5914

…ermissions-ios-changes � Conflicts: � packages/image_picker/image_picker_ios/CHANGELOG.md � packages/image_picker/image_picker_ios/pubspec.yaml � packages/image_picker/image_picker_ios/test/test_api.dart

…ermissions-ios-changes

PiotrMitkowski · 2022-08-05T09:06:03Z

@cyanglaz I've fixed the code based on your latest remarks. Right now, I can't see an option to request your another review, so I'm pinging through this comment :)

cyanglaz

LGTM
cc @stuartmorgan

stuartmorgan

A couple of small things, but otherwise looks good.

stuartmorgan · 2022-08-22T20:30:08Z

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

@@ -234,7 +249,11 @@ - (void)pickVideoWithSource:(nonnull FLTSourceSpecification *)source
                                             camera:[self cameraDeviceForSource:source]];
      break;
    case FLTSourceTypeGallery:
-      [self checkPhotoAuthorizationWithImagePicker:imagePickerController];
+      if (context.requestFullMetadata) {


Looks like this is still missing the iOS 11 @available check.

stuartmorgan · 2022-08-22T20:31:11Z

packages/image_picker/image_picker_ios/lib/image_picker_ios.dart

 import 'package:image_picker_platform_interface/image_picker_platform_interface.dart';

-import 'src/messages.g.dart';


Why did you change this to a package-style import within the same package?

I was checking if it could fix an issue with imports related to Pigeon, but then I've found a GitHub issue about it and forgot to revert this change. I'll do it now.

…ermissions-ios-changes

stuartmorgan

LGTM!

auto-submit · 2022-08-31T13:28:27Z

The status or check suite dart_unit_tests CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.

…rmissions) - iOS changes (flutter/plugins#5713)

idish · 2022-09-13T20:32:33Z

Thanks for the great work!
Is there any schedule for the 0.8.6 version to be published? Really looking forward to it.

jmagman · 2022-09-13T21:00:32Z

It's already available, note it's image_picker_ios https://pub.dev/packages/image_picker_ios/changelog#086

stuartmorgan · 2022-09-14T00:18:03Z

The PR you'll want to follow to know when you can use this feature as a plugin client is #5915

idish · 2022-09-14T11:01:31Z

Just tried to implement that in my app and seems as it crashes due to permission issue after picking photos.
I think it worth noting that here as well. See my comment:
#5915 (comment)

… - iOS changes (flutter#5713)

PiotrMitkowski added 2 commits May 13, 2022 11:27

Moved iOS specific changes to a separate branch

c705774

Fixed line formatting

bf73c73

PiotrMitkowski requested a review from cyanglaz as a code owner May 13, 2022 09:36

PiotrMitkowski changed the title ~~[image_picker] add requestFullMetadata for iOS (optional permissions) - platform interface~~ [image_picker] add requestFullMetadata for iOS (optional permissions) - iOS changes May 13, 2022

github-actions bot added p: image_picker platform-ios labels May 13, 2022

cyanglaz suggested changes May 13, 2022

View reviewed changes

stuartmorgan suggested changes May 13, 2022

View reviewed changes

PiotrMitkowski added 3 commits May 25, 2022 13:02

Merge branch 'main' of github.com:flutter/plugins into ios-optional-p…

d48e642

…ermissions-ios-changes � Conflicts: � packages/image_picker/image_picker_ios/CHANGELOG.md � packages/image_picker/image_picker_ios/pubspec.yaml

PR remarks

3b3435f

PR remarks

420a4c4

PiotrMitkowski requested review from cyanglaz and stuartmorgan May 25, 2022 12:32

stuartmorgan suggested changes Jun 1, 2022

View reviewed changes

cyanglaz approved these changes Jun 1, 2022

View reviewed changes

PiotrMitkowski marked this pull request as draft June 7, 2022 15:30

cyanglaz mentioned this pull request Jun 30, 2022

[image_picker] Permission is not required for image picking from photo library to use PHPickerViewController or UIImagePickerController flutter/flutter#106799

Closed

PiotrMitkowski added 8 commits July 26, 2022 16:28

Merge branch 'main' of github.com:flutter/plugins into ios-optional-p…

dff7042

…ermissions-ios-changes

Updated iOS package with new interface method implementation

b9c4cef

Implemented native part of picking multi images without full metadata

b3d81e3

Merge branch 'main' of github.com:flutter/plugins into ios-optional-p…

d95b117

…ermissions-ios-changes

Updated versions of the platform interface in iOS implementation

4a41df1

Fixed import for generated messages file

932ae7c

Restored accidentally removed comments from generated test API file

2424828

cyanglaz self-requested a review August 3, 2022 20:47

PiotrMitkowski added 2 commits August 5, 2022 09:33

Merge branch 'main' of github.com:flutter/plugins into ios-optional-p…

5685468

…ermissions-ios-changes

PR remarks

2464c13

cyanglaz approved these changes Aug 5, 2022

View reviewed changes

stuartmorgan suggested changes Aug 22, 2022

View reviewed changes

PiotrMitkowski added 2 commits August 25, 2022 16:40

Merge branch 'main' of github.com:flutter/plugins into ios-optional-p…

9252583

…ermissions-ios-changes

PR remarks

09a264a

PiotrMitkowski requested review from cyanglaz and stuartmorgan and removed request for cyanglaz August 25, 2022 15:59

PiotrMitkowski mentioned this pull request Aug 31, 2022

[image_picker] add requestFullMetadata for iOS (optional permissions) #5915

Merged

11 tasks

Merge branch 'main' into ios-optional-permissions-ios-changes

6aaa4b0

stuartmorgan approved these changes Aug 31, 2022

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2022

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2022

auto-submit bot merged commit ef20791 into flutter:main Aug 31, 2022

PiotrMitkowski deleted the ios-optional-permissions-ios-changes branch August 31, 2022 13:58

engine-flutter-autoroll mentioned this pull request Aug 31, 2022

Roll Plugins from c08e447f989c to ef2079182960 (7 revisions) flutter/flutter#110674

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2022

ef2079182 [image_picker] add requestFullMetadata for iOS (optional pe…

1ba597f

…rmissions) - iOS changes (flutter/plugins#5713)

engine-flutter-autoroll mentioned this pull request Aug 31, 2022

Roll Plugins from c08e447f989c to c6b2f8a84439 (8 revisions) flutter/flutter#110677

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2022

ef2079182 [image_picker] add requestFullMetadata for iOS (optional pe…

009c4b8

…rmissions) - iOS changes (flutter/plugins#5713)

adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022

[image_picker] add requestFullMetadata for iOS (optional permissions)…

702cc49

… - iOS changes (flutter#5713)

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[image_picker] add requestFullMetadata for iOS (optional permissions)…

bf2a2f1

… - iOS changes (flutter#5713)

		import 'package:image_picker_platform_interface/image_picker_platform_interface.dart';

		import 'src/messages.g.dart';

[image_picker] add requestFullMetadata for iOS (optional permissions) - iOS changes #5713

[image_picker] add requestFullMetadata for iOS (optional permissions) - iOS changes #5713

Conversation

PiotrMitkowski commented May 13, 2022 • edited Loading

Description

Related issues

Pre-launch Checklist

PiotrMitkowski commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman commented May 31, 2022

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrMitkowski Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment • edited Loading

Choose a reason for hiding this comment

cyanglaz commented Jun 30, 2022

PiotrMitkowski commented Jun 30, 2022

cyanglaz commented Jun 30, 2022

PiotrMitkowski commented Aug 5, 2022

cyanglaz left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

auto-submit bot commented Aug 31, 2022

idish commented Sep 13, 2022

jmagman commented Sep 13, 2022

stuartmorgan commented Sep 14, 2022

idish commented Sep 14, 2022

PiotrMitkowski commented May 13, 2022 •

edited

Loading

PiotrMitkowski Jun 6, 2022 •

edited

Loading

cyanglaz left a comment •

edited

Loading