Skip to content
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

[image_picker] Removes use of PHAsset on IOS 14+ #8190

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

WenHaozhan
Copy link
Contributor

@WenHaozhan WenHaozhan commented Nov 27, 2024

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.

Pre-launch Checklist

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

@loic-sharma
Copy link
Member

@WenHaozhan Hey there, thanks for updating this! I think this PR looks good, but I'm new to this package and I'd like to confirm with my co-workers. It's the holidays in the US, so I will likely loop back next week. I apologize for the delay

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - thanks for the contribution!

@jmagman
Copy link
Member

jmagman commented Dec 26, 2024

Adding @hellohuanlin as a second reviewer.

@jmagman jmagman requested a review from hellohuanlin December 26, 2024 20:07
PHAsset *originalAsset;
// Only if requested, fetch the full "PHAsset" metadata, which requires "Photo Library Usage"
// permissions.
if (self.requestFullMetadata) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did PHPickerViewController always return the full metadata after iOS 14? Curious why have separate branches here in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that PHPickerViewController returns full metadata. The user has the option to remove certain metadata such as location
Simulator Screenshot - iPhone 16 - 2024-12-27 at 09 42 27
but by default they are included.
Simulator Screenshot - iPhone 16 - 2024-12-27 at 09 42 22

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question. Otherwise LGTM

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 3, 2025
@auto-submit auto-submit bot merged commit 733869c into flutter:main Jan 3, 2025
77 checks passed
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Jan 6, 2025
flutter/packages@eb73582...07ae98c

2025-01-06 49699333+dependabot[bot]@users.noreply.github.com
[dependabot]: Bump org.json:json from 20240303 to 20241224 in
/packages/in_app_purchase/in_app_purchase/example/android/app
(flutter/packages#8371)
2025-01-05 32538273+ValentinVignal@users.noreply.github.com
[google_maps_flutter] Activate leak testing (flutter/packages#8368)
2025-01-05 32538273+ValentinVignal@users.noreply.github.com
[flutter_markdown] Fix some memory leaks and activate leak testing
[prod-leak-fix] (flutter/packages#8367)
2025-01-03 36674458+WenHaozhan@users.noreply.github.com [image_picker]
Removes use of PHAsset on IOS 14+ (flutter/packages#8190)
2025-01-03 matanlurey@users.noreply.github.com Delete unused templates
`skeleton` and `app_shared` and release as `5.0.0`.
(flutter/packages#8360)
2025-01-02 stuartmorgan@google.com [tool] Ensure that
leak_tracker_flutter_testing is dev-only (flutter/packages#8365)
2025-01-02 stuartmorgan@google.com [pigeon] Discuss stability in README
(flutter/packages#8366)
2025-01-02 tarrinneal@gmail.com [shared_preferences] Add information
about shared preferences android to docs (flutter/packages#8296)
2024-12-30 mchudy@users.noreply.github.com [camera] Remove OCMock from
permission tests (flutter/packages#8350)
2024-12-29 kevmoo@users.noreply.github.com Drop vector bits from
allowed_unpinned_deps.yaml (flutter/packages#8327)
2024-12-28 32538273+ValentinVignal@users.noreply.github.com [camera] Fix
memory leaks in example and activate leak testing
(flutter/packages#8287)
2024-12-27 10687576+bparrishMines@users.noreply.github.com [pigeon] Adds
platform for imports that aren't support on a platform
(flutter/packages#8338)
2024-12-26 jessiewong401@gmail.com Bump Plugin Example Apps to
TargetSdkVersion >= 34 (flutter/packages#8285)
2024-12-26 matanlurey@users.noreply.github.com Re-create
`templates/app`, add deprecation notices for `app_shared` and
`skeleton`. (flutter/packages#8336)
2024-12-26 jessiewong401@gmail.com Update Gradle Command Test to Only
Accept Gradle Declarative Apply (flutter/packages#8325)
2024-12-24 49699333+dependabot[bot]@users.noreply.github.com
[dependabot]: Bump com.google.guava:guava from 33.3.1-android to
33.4.0-android in /packages/camera/camera_android_camerax/android
(flutter/packages#8331)
2024-12-24 49699333+dependabot[bot]@users.noreply.github.com
[dependabot]: Bump camerax_version from 1.3.4 to 1.4.1 in
/packages/camera/camera_android_camerax/android (flutter/packages#8330)
2024-12-24 49699333+dependabot[bot]@users.noreply.github.com [gradle]:
Bump com.google.truth:truth, com.google.code.gson:gson,
com.squareup.okhttp3:okhttp in /packages/espresso/android
(flutter/packages#8089)
2024-12-24 49699333+dependabot[bot]@users.noreply.github.com
[image_picker]: Bump androidx.activity:activity from 1.9.2 to 1.9.3 in
/packages/image_picker/image_picker_android/android
(flutter/packages#7897)
2024-12-23 ditman@gmail.com [ci] Cumulative fixes to reopen the tree.
(flutter/packages#8344)
2024-12-18 magder@google.com Group dependabot updates for some gradle
dependencies (flutter/packages#8100)
2024-12-18 jessiewong401@gmail.com Applied Gradle Plugins Declaratively
All Remaining Example Apps (flutter/packages#8312)
2024-12-18 49699333+dependabot[bot]@users.noreply.github.com
[lifecycle]: Bump androidx.annotation:annotation from 1.7.0 to 1.9.1 in
/packages/flutter_plugin_android_lifecycle/android
(flutter/packages#7974)
2024-12-18 stuartmorgan@google.com [ci] Re-enable macOS sandboxing
(flutter/packages#8293)
2024-12-18 49699333+dependabot[bot]@users.noreply.github.com [espresso]:
Bump com.android.tools.build:gradle from 7.4.1 to 8.7.2 in
/packages/espresso/android (flutter/packages#8013)
2024-12-18 reidbaker@google.com [shared_preferences] Increase minimum
android endorsed version (flutter/packages#8318)
2024-12-17 stuartmorgan@google.com Revert "[shared_preferences] Add
shared preferences devtools" (flutter/packages#8314)
2024-12-16 adson@soraschools.com [shared_preferences] Add shared
preferences devtools (flutter/packages#6749)
2024-12-16 ditman@gmail.com [google_adsense] Add optional init
parameters. (flutter/packages#8297)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: image_picker platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants