-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[image_picker_ios] Add photo to simulator Photos library during test #9759
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
Conversation
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.
Code Review
This pull request adds a test photo to the iOS simulator's Photos library on app launch to ensure UI tests can run on newer iOS versions where the library is empty. I've provided feedback on AppDelegate.m
to improve robustness by checking authorization status and to fix code formatting. Additionally, I've pointed out a small typo in the CHANGELOG.md
.
- (BOOL)application:(UIApplication *)application | ||
didFinishLaunchingWithOptions:(NSDictionary *)launchOptions { | ||
[GeneratedPluginRegistrant registerWithRegistry:self]; | ||
return [super application:application didFinishLaunchingWithOptions:launchOptions]; | ||
didFinishLaunchingWithOptions:(NSDictionary *)launchOptions { | ||
[GeneratedPluginRegistrant registerWithRegistry:self]; | ||
|
||
if (@available(iOS 14, *)) { | ||
NSBundle *bundle = [NSBundle bundleForClass:[self class]]; | ||
__block NSError *saveError = nil; | ||
[PHPhotoLibrary requestAuthorizationForAccessLevel:PHAccessLevelAddOnly handler:^(PHAuthorizationStatus status) { | ||
if (![PHPhotoLibrary.sharedPhotoLibrary performChangesAndWait:^{ | ||
NSURL *jpgImageTall = [bundle URLForResource:@"jpgImageTall" withExtension:@"jpg"]; | ||
[PHAssetChangeRequest creationRequestForAssetFromImageAtFileURL:jpgImageTall]; | ||
} error:&saveError]) { | ||
os_log_error(OS_LOG_DEFAULT, "%@", saveError); | ||
} | ||
}]; | ||
} | ||
return [super application:application didFinishLaunchingWithOptions:launchOptions]; | ||
} |
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.
Consider checking the PHAuthorizationStatus
returned in the handler before attempting to save the photo. This would make the logic clearer and avoid an unexpected failure if permissions are denied. Also, the indentation in this method is inconsistent with the project's style.1
- (BOOL)application:(UIApplication *)application
didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
[GeneratedPluginRegistrant registerWithRegistry:self];
if (@available(iOS 14, *)) {
NSBundle *bundle = [NSBundle bundleForClass:[self class]];
__block NSError *saveError = nil;
[PHPhotoLibrary requestAuthorizationForAccessLevel:PHAccessLevelAddOnly handler:^(PHAuthorizationStatus status) {
if (status == PHAuthorizationStatusAuthorized) { // Check authorization status
if (![PHPhotoLibrary.sharedPhotoLibrary performChangesAndWait:^{
NSURL *jpgImageTall = [bundle URLForResource:@"jpgImageTall" withExtension:@"jpg"];
[PHAssetChangeRequest creationRequestForAssetFromImageAtFileURL:jpgImageTall];
} error:&saveError]) {
os_log_error(OS_LOG_DEFAULT, "%@", saveError);
}
} else {
os_log(OS_LOG_DEFAULT, "Photos authorization not granted");
}
}];
}
return [super application:application didFinishLaunchingWithOptions:launchOptions];
}
Style Guide References
Footnotes
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.
Consider checking the PHAuthorizationStatus returned in the handler before attempting to save the photo.
This is fine for a test app where we know our permission handler will do the right thing.
the indentation in this method is inconsistent with the project's style
Very true, waiting on the clang formatter to run.
@@ -15,23 +15,14 @@ | |||
3A72BAD3FAE6E0FA9D80826B /* libPods-RunnerTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 35AE65F25E0B8C8214D8372B /* libPods-RunnerTests.a */; }; | |||
3B3967161E833CAA004F5970 /* AppFrameworkInfo.plist in Resources */ = {isa = PBXBuildFile; fileRef = 3B3967151E833CAA004F5970 /* AppFrameworkInfo.plist */; }; | |||
5C9513011EC38BD300040975 /* GeneratedPluginRegistrant.m in Sources */ = {isa = PBXBuildFile; fileRef = 5C9513001EC38BD300040975 /* GeneratedPluginRegistrant.m */; }; | |||
680049382280F2B9006DD6AB /* pngImage.png in Resources */ = {isa = PBXBuildFile; fileRef = 680049352280F2B8006DD6AB /* pngImage.png */; }; |
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.
The test images were being included in the RunnerUITest target, and were unused. Remove it.
BE6173D826A958B800D0974D /* ImagePickerFromLimitedGalleryUITests.m in Sources */ = {isa = PBXBuildFile; fileRef = BE6173D726A958B800D0974D /* ImagePickerFromLimitedGalleryUITests.m */; }; | ||
F4F7A436CCA4BF276270A3AE /* libPods-Runner.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EC32F6993F4529982D9519F1 /* libPods-Runner.a */; }; | ||
F706BF732E433163001263B9 /* jpgImageTall.jpg in Resources */ = {isa = PBXBuildFile; fileRef = F706BF722E433163001263B9 /* jpgImageTall.jpg */; }; |
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.
Include all test images to the example app resources (not just the RunnerTest unit test app) so they can be added when the app finishes launching. Right now only one is added, but I can see someone in the future trying to add a new one and being confused why the URL is nil.
These images are very small.
...ages/image_picker/image_picker_ios/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m
Outdated
Show resolved
Hide resolved
There's still one failure on Xcode 26:
![]() |
238246a
to
f452ca2
Compare
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.
LGTM
The iOS 26 simulator Photos library does not pre-populate example images.
When the example app launches, add a test photo to the library. Only add a photo on first launch of the example app, enforced for a
NSUserDefault
.Fixes flutter/flutter#173327
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3