[image_picker] Removes use of PHAsset when using PHPickerViewController #8020
Conversation
WenHaozhan
force-pushed
the
main
branch
2 times, most recently
from
78db413 to
b051146
Compare
|
Any update on this? |
|
@WenHaozhan Hello, this is on my queue to review. I'm not familiar with the image picker plugin yet and need to ramp up first before I'm able to review. I apologize for the delay. |
|
Hello, I'm ramping up and trying to understand the situation. My understanding:
@vashworth's message in flutter/flutter#90373 (comment) indicates animated GIFs are a concern. I've verified that this PR is still able to load & animate GIFs as expected. @stuartmorgan @jmagman Would you have any recommendations on things we should check before landing this change? AFAICT, this change appears to be correct. |
| @@ -529,11 +529,19 @@ - (void)testPickImageAuthorizationDenied API_AVAILABLE(ios(14)) { | |||
| quality:nil | |||
| fullMetadata:YES | |||
| completion:^(NSString *result, FlutterError *error) { | |||
| // Does not error due to access denied | |||
There was a problem hiding this comment.
Should we remove this test entirely as this is no longer a concern?
There was a problem hiding this comment.
I removed this test.
| @@ -113,11 +113,7 @@ - (void)launchPHPickerWithContext:(nonnull FLTImagePickerMethodCallContext *)con | |||
| pickerViewController.presentationController.delegate = self; | |||
| self.callContext = context; | |||
|
|
|||
| if (context.requestFullMetadata) { | |||
| [self checkPhotoAuthorizationWithPHPicker:pickerViewController]; | |||
There was a problem hiding this comment.
Does anything use checkPhotoAuthorizationWithPHPicker now? Can we remove it?
There was a problem hiding this comment.
Don't think anything uses this, removed.
| // Only if requested, fetch the full "PHAsset" metadata, which requires "Photo Library Usage" | ||
| // permissions. | ||
| if (self.requestFullMetadata) { | ||
| originalAsset = [FLTImagePickerPhotoAssetUtil getAssetFromPHPickerResult:self.result]; |
There was a problem hiding this comment.
Does anything use getAssetFromPHPickerResult now? Can we remove it?
There was a problem hiding this comment.
Removed function and a test that uses it.
…e_picker to remove needing permissions.
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
| @@ -499,7 +499,7 @@ - (void)testPickImageRequestAuthorization API_AVAILABLE(ios(14)) { | |||
| id mockPhotoLibrary = OCMClassMock([PHPhotoLibrary class]); | |||
There was a problem hiding this comment.
Should we rename this test to something like testPickImageDoesntRequestAuthorization?
WenHaozhan
requested review from
bparrishMines and
domesticmouse
as code owners
github-actions
bot
added
p: flutter_markdown
platform-android
p: webview_flutter
p: flutter_svg
and removed
p: flutter_markdown
platform-android
p: webview_flutter
labels
|
I'm not required now that |
|
Hey, sorry about this, I messed up a rebase. I will create a new pull request. |
Original PR #8020 Fixes issue flutter/flutter#90373 Currently on IOS 14+, images are picked using `PHPickerViewController` which does not need photo permissions and also gets the full image metadata regardless of `requestFullMetadata`. However, it currently retrieves the metadata using `PHAsset` which does require permission and causes the gallery opening twice issue. Another issue is that an error is thrown when permission is denied even if none are required.
Fixes issue flutter/flutter#90373
Currently on IOS 14+, images are picked using
PHPickerViewControllerwhich does not need photo permissions and also gets the full image metadata regardless ofrequestFullMetadata. However, it currently retrieves the metadata usingPHAssetwhich does require permission and causes the gallery opening twice issue. Another issue is that an error is thrown when permission is denied even if none are required.Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.