[image_picker_ios] Pass through error message from image saving #6858

jmagman · 2022-12-17T01:27:16Z

Previously, if there was an error loading the image, the result callback would never be called, and the callContext would not be nil'd out. When the next request happened, the user would get a confusing multiple_request error because the last state wasn't cleared out.

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

Lines 273 to 277 in 2cbb314

    
           if (self.callContext) { 
        
             [self sendCallResultWithError:[FlutterError errorWithCode:@"multiple_request" 
        
                                                               message:@"Cancelled by a second request" 
        
                                                               details:nil]]; 
        
             self.callContext = nil;

Now, when there is an error loading an image, pipe an error back to the flutter result instead of silently failing and never calling the result callback. This also nils out the callContext so the next request has a chance of succeeding.

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

Lines 683 to 684 in 2cbb314

    
           self.callContext.result(nil, error); 
        
           self.callContext = nil;

As before, save all the picked images on a background thread, and when complete call the callback with either all the saved images, or with the image error. Use the NSOperationQueue API instead of GCD to take advantage of dependency operations (the final main callback is dependent on the image save operations completing).

If any of the picked images fail to load, return an error back to the dart side. Ideally there would be a mapping between successfully picked images and the images that failed to load, but I believe that would require an API change like flutter/flutter#95268.

Also

Minor __bridge cleanup
Use XCTest expectations instead of semaphores.

Fixes flutter/flutter#98569
Fixes flutter/flutter#82602
Will make flutter/flutter#49780 actually show the underlying error.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman · 2022-12-17T01:34:30Z

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

-@interface FLTImagePickerPlugin : NSObject <FlutterPlugin>
-
-// For testing only.
- (UIViewController *)viewControllerWithWindow:(UIWindow *)window;


Moved to existing FLTImagePickerPlugin_Test.h

jmagman · 2022-12-17T01:35:09Z

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

@@ -29,10 +29,7 @@ - (instancetype)initWithResult:(nonnull FlutterResultAdapter)result {

 #pragma mark -

-@interface FLTImagePickerPlugin () <UINavigationControllerDelegate,


Also moved to FLTImagePickerPlugin_Test.h so the tests can directly call the PHPickerViewControllerDelegate methods.

jmagman · 2022-12-17T01:38:25Z

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

@@ -102,10 +101,18 @@ - (void)start {
                                    UIImage *image = [[UIImage alloc] initWithData:data];
                                    [self processImage:image];
                                  } else {
-                                    os_log_error(OS_LOG_DEFAULT, "Could not process image: %@",
-                                                 error);
+                                    FlutterError *flutterError =


This is the fix--when the image fails to load, send back an error.

jmagman · 2022-12-17T01:38:41Z

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

                                  }
                                }];
+    } else {
+      FlutterError *flutterError = [FlutterError errorWithCode:@"invalid_source"


And if it's not an image, also send back an error.

jmagman · 2022-12-17T01:40:26Z

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

+    [sendListOperation addDependency:saveOperation];
+    [backgroundQueue addOperation:saveOperation];


Use NSOperationQueue so the main thread callback operation can be scheduled ASAP, but won't run until all the dependent save operations have completed.

jmagman · 2022-12-17T01:50:29Z

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

-    dispatch_async(dispatch_get_main_queue(), ^{
+  __block NSOperationQueue *saveQueue = [[NSOperationQueue alloc] init];
+  saveQueue.name = @"Flutter Save Image Queue";
+  saveQueue.qualityOfService = NSQualityOfServiceUserInitiated;


This queue should not be background QoS, that's for app maintenance like clearing caches and can take awhile to schedule, or sometimes not scheduled at all until the OS is idle. But this image save is in reaction to the user tapping on image, who is waiting for the app to continue.
https://developer.apple.com/documentation/dispatch/dispatchqos/qosclass

jmagman · 2022-12-17T02:11:27Z

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

+    // NSNull means it hasn't saved yet.
+    pathList[index] = [NSNull null];


This isn't really needed anymore, either all the images will save and this will be densely populated, or it will not be returned at all and only the error will be returned.
However if flutter/flutter#95268 is implemented then this should return all the saved images, and for the failing ones some object with the error attached.

vashworth

LGTM

hellohuanlin · 2022-12-19T18:31:24Z

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

@@ -269,37 +271,130 @@ - (void)testViewController {
 - (void)testPluginMultiImagePathHasNullItem {
  FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init];

-  dispatch_semaphore_t resultSemaphore = dispatch_semaphore_create(0);


hellohuanlin · 2022-12-19T18:39:44Z

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

@@ -120,7 +120,7 @@ + (GIFInfo *)scaledGIFImage:(NSData *)data
  options[(NSString *)kCGImageSourceTypeIdentifierHint] = (NSString *)kUTTypeGIF;

  CGImageSourceRef imageSource =
-      CGImageSourceCreateWithData((CFDataRef)data, (CFDictionaryRef)options);
+      CGImageSourceCreateWithData((__bridge CFDataRef)data, (CFDictionaryRef)options);


(__bridge CFDictionaryRef)?

Oh yeah missed that.

hellohuanlin · 2022-12-19T18:57:09Z

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

+  NSNumber *imageQuality = currentCallContext.imageQuality;
+  NSNumber *desiredImageQuality = [self getDesiredImageQuality:imageQuality];
+  BOOL requestFullMetadata = currentCallContext.requestFullMetadata;
+  NSMutableArray *pathList = [[NSMutableArray alloc] initWithCapacity:results.count];


do we need to populate the array like before?

See my comment below: https://github.com/flutter/plugins/pull/6858/files#r1051290080

Hmmm, maybe im missing something. I was saying if pathList is not populated (i.e. size=0, capacity=count), then will pathList[0] = foo throw index out of bounds?

You're right, good catch. I'm going to make this a NSPointerArray.

Actually just populating the array with NSNull as was done before is probably safer. A NSPointerArray won't have the out-of-bounds problem, since it "populates" up to count with nil, and will compact those out during allObjects. So if there was a bug where all indices weren't set to NSNull then we would get wacky index values and the array length would be incorrect.

Yes what I have here is correct, the first thing it's doing is populating the array with NSNull, in index order:

NSMutableArray *pathList = [[NSMutableArray alloc] initWithCapacity:results.count]; ... [results enumerateObjectsUsingBlock:^(PHPickerResult *result, NSUInteger index, BOOL *stop) { // NSNull means it hasn't saved yet. pathList[index] = [NSNull null]; ...

This is exactly the same as the previous logic, it just replaced a for loop with fast enumeration over the results.

NSMutableArray *mutableArray = [[NSMutableArray alloc] initWithCapacity:size]; for (int i = 0; i < size; [mutableArray addObject:[NSNull null]], i++) ;

Hmmm, the previous logic is using addObject, which increases the size though. iirc I think pathList[index] = [NSNull null] will simply crash.

I just did an experiment. Very interesting/strange behavior of NSMutableArray:

Experiment 1: This works:

NSMutableArray *arr = [NSMutableArray arrayWithCapacity:100]; arr[0] = @"foo0"; arr[1] = @"foo1"; arr[2] = @"bar2"; NSLog(@"print out: %@", arr);

Experiment 2: This crashes:

NSMutableArray *arr = [NSMutableArray arrayWithCapacity:100]; arr[0] = @"foo0"; arr[2] = @"bar2"; NSLog(@"print out: %@", arr);

Experiment 3: This also crashes:

NSMutableArray *arr = [NSMutableArray arrayWithCapacity:100]; arr[1] = @"foo1"; arr[2] = @"bar2"; NSLog(@"print out: %@", arr);

Looks like as long as you add from index 0, one by one, NSMutableArray will just auto increment the size one by one. I wasn't able to find this behavior documented anywhere. However, the doc of setObject API explicitly says it should crash:

If index is beyond the end of the array (that is, if index is greater than or equal to the value returned by count), an NSRangeException is raised.

The above contradicts with actual behavior of Experiment 1, so it feels like a bug in NSMutableArray. I recommend just do addObject instead of setObject API.

The code is doing experiment 1 because enumerateObjectsUsingBlock: starts at 0 and goes through the whole array (unless *stop = YES is set).

https://developer.apple.com/documentation/foundation/nsarray/1415846-enumerateobjectsusingblock

Executes a given block using each object in the array, starting with the first object and continuing through the array to the last object.

I can swap to addObject: though since it is incrementing by one every time.

I can swap to addObject: though since it is incrementing by one every time.

Yep, I think experiment 1 is a bug in NSMutableArray that's supposed to crash, since it contradicts with the doc.

hellohuanlin · 2022-12-19T19:02:51Z

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

+
+  // This operation will be executed on the main queue after
+  // all selected files have been saved.
+  NSBlockOperation *sendListOperation = [NSBlockOperation blockOperationWithBlock:^{


hellohuanlin · 2022-12-19T19:05:36Z

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

+  // This operation will be executed on the main queue after
+  // all selected files have been saved.
+  NSBlockOperation *sendListOperation = [NSBlockOperation blockOperationWithBlock:^{
+    if (saveError != nil) {


may worth commenting that "No race condition here since sendListOperation is only called after all saveOperations are complete"

It says a few lines above this:

// This operation will be executed on the main queue after // all selected files have been saved.

hellohuanlin · 2022-12-19T19:10:21Z

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

+                     saveError = error;
+                   }
+                 }];
+    [sendListOperation addDependency:saveOperation];


Nice API. Yeah it's weird to mix GCD and NSOperationQueue - in pure GCD I would use DispatchGroup for this.

…ving (flutter/plugins#6858)

* 697c8b3ee [image_picker_ios] Pass through error message from image saving (flutter/plugins#6858) * 72f810851 [local_auth] Bump `intl` from ^0.17.0 to ">=0.17.0 <0.19.0" (flutter/plugins#6848) * acbe9b452 [gh_actions]: Bump github/codeql-action from 2.1.35 to 2.1.37 (flutter/plugins#6860) * 3d8b73bf0 [camera] Remove deprecated Optional type (flutter/plugins#6870) * c5220efae [in_app_purchase] Add support for macOS (flutter/plugins#6519) * 54fc2066d Roll Flutter from 028c6e2 to dbc9306 (11 revisions) (flutter/plugins#6849)

…#117456) * 697c8b3ee [image_picker_ios] Pass through error message from image saving (flutter/plugins#6858) * 72f810851 [local_auth] Bump `intl` from ^0.17.0 to ">=0.17.0 <0.19.0" (flutter/plugins#6848) * acbe9b452 [gh_actions]: Bump github/codeql-action from 2.1.35 to 2.1.37 (flutter/plugins#6860) * 3d8b73bf0 [camera] Remove deprecated Optional type (flutter/plugins#6870) * c5220efae [in_app_purchase] Add support for macOS (flutter/plugins#6519) * 54fc2066d Roll Flutter from 028c6e2 to dbc9306 (11 revisions) (flutter/plugins#6849)

…ter#6858) * [image_picker_ios] Pass through error message from image saving * Review edits * Format * addObject

jmagman self-assigned this Dec 17, 2022

jmagman requested a review from cyanglaz as a code owner December 17, 2022 01:27

github-actions bot added p: image_picker platform-ios labels Dec 17, 2022

jmagman force-pushed the ip-error-operation branch 2 times, most recently from c9b05ad to 4ec89b8 Compare December 17, 2022 01:37

jmagman commented Dec 17, 2022

View reviewed changes

jmagman force-pushed the ip-error-operation branch from 4ec89b8 to 9c30e3b Compare December 17, 2022 01:46

[image_picker_ios] Pass through error message from image saving

bf5fff6

jmagman force-pushed the ip-error-operation branch from 9c30e3b to bf5fff6 Compare December 17, 2022 01:48

jmagman commented Dec 17, 2022

View reviewed changes

jmagman requested review from hellohuanlin and vashworth December 17, 2022 02:06

jmagman commented Dec 17, 2022

View reviewed changes

vashworth approved these changes Dec 19, 2022

View reviewed changes

hellohuanlin reviewed Dec 19, 2022

View reviewed changes

jmagman added 3 commits December 19, 2022 11:26

Review edits

3aca95e

Format

f38ea4d

addObject

824b8ad

hellohuanlin approved these changes Dec 19, 2022

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 19, 2022

auto-submit bot merged commit 697c8b3 into flutter:main Dec 19, 2022

jmagman deleted the ip-error-operation branch December 19, 2022 23:15

engine-flutter-autoroll mentioned this pull request Dec 20, 2022

Roll Plugins from 840a04954fa0 to 72f8108512f7 (2 revisions) flutter/flutter#117393

Closed

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

697c8b3ee [image_picker_ios] Pass through error message from image sa…

b661948

…ving (flutter/plugins#6858)

engine-flutter-autoroll mentioned this pull request Dec 21, 2022

Roll Plugins from 840a04954fa0 to 54fc2066d636 (6 revisions) flutter/flutter#117456

Merged

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

697c8b3ee [image_picker_ios] Pass through error message from image sa…

5da34ee

…ving (flutter/plugins#6858)

jmagman mentioned this pull request Dec 28, 2022

[image_picker] 0.8.6 imageQuality set value will cause crash on iphone(ios16+ iphone xr & mini12) flutter/flutter#117670

Closed

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

[image_picker_ios] Pass through error message from image saving (flut…

9ac9b1d

…ter#6858) * [image_picker_ios] Pass through error message from image saving * Review edits * Format * addObject

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[image_picker_ios] Pass through error message from image saving #6858

[image_picker_ios] Pass through error message from image saving #6858

jmagman commented Dec 17, 2022

jmagman Dec 17, 2022

jmagman Dec 17, 2022

jmagman Dec 17, 2022

jmagman Dec 17, 2022

jmagman Dec 17, 2022

jmagman Dec 17, 2022

jmagman Dec 17, 2022

vashworth left a comment

hellohuanlin Dec 19, 2022

hellohuanlin Dec 19, 2022

jmagman Dec 19, 2022

hellohuanlin Dec 19, 2022

jmagman Dec 19, 2022

hellohuanlin Dec 19, 2022

jmagman Dec 19, 2022

jmagman Dec 19, 2022

jmagman Dec 19, 2022

hellohuanlin Dec 19, 2022

hellohuanlin Dec 19, 2022 •

edited

Loading

jmagman Dec 19, 2022

hellohuanlin Dec 19, 2022 •

edited

Loading

hellohuanlin Dec 19, 2022

hellohuanlin Dec 19, 2022

jmagman Dec 19, 2022

hellohuanlin Dec 19, 2022

	if (self.callContext) {
	[self sendCallResultWithError:[FlutterError errorWithCode:@"multiple_request"
	message:@"Cancelled by a second request"
	details:nil]];
	self.callContext = nil;

		@@ -29,10 +29,7 @@ - (instancetype)initWithResult:(nonnull FlutterResultAdapter)result {

		#pragma mark -

		@interface FLTImagePickerPlugin () <UINavigationControllerDelegate,

		[sendListOperation addDependency:saveOperation];
		[backgroundQueue addOperation:saveOperation];

		// NSNull means it hasn't saved yet.
		pathList[index] = [NSNull null];

[image_picker_ios] Pass through error message from image saving #6858

[image_picker_ios] Pass through error message from image saving #6858

Conversation

jmagman commented Dec 17, 2022

Pre-launch Checklist

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

vashworth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin Dec 19, 2022 • edited Loading

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

hellohuanlin Dec 19, 2022 •

edited

Loading

hellohuanlin Dec 19, 2022 •

edited

Loading