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

Integrate PHPickerViewController #21190

Closed
kean opened this issue Jul 26, 2023 · 10 comments
Closed

Integrate PHPickerViewController #21190

kean opened this issue Jul 26, 2023 · 10 comments
Assignees
Milestone

Comments

@kean
Copy link
Contributor

kean commented Jul 26, 2023

Integrate PHPickerViewController.

Screenshot 2023-08-09 at 5 55 35 PM

Changes

Out of the Scope:

  • Aztec (deprecated, won't risk introducing regressions)
  • Stories (low usage, integration is more complicated than on other screens meaning high risk of regressions)

Fixes

These fixes were made in the scope of the task:

Advantages

  • No need to request access to the photo library
  • Allows you to see your albums and favorites
  • Zoom
  • Search (if you have enough photos)
  • Consistent UI with the Photos app
  • Apple maintains and improves it every year
  • Allows excluding location metadata from the assets (iOS 17)
  • Runs out of process
  • Should be much better at supporting massive photo libraries
  • Modernize the codebase by removing the legacy Objective-C code
  • The new system for adding assets to the post eliminates memory spikes (more in Integrate PHPickerViewController in Page Editor #21360)

Limitations

  • There is no way to add custom UI to PHPickerViewController, so it won't be possible to add "Edit" feature to the picker. Fortunately, the app already allows the users to edit the photos after selection, and based on Tracks, the number of users who take advantage of editing is low (<2% of total uploads).
  • I'm yet to find a way to export full-size RAW images. NSFileProvider servers a downscaled version. More info here.
  • We can no longer show the video duration upload limit for free plan directly in the picker. More info here.

Closed Defects

Replacing the custom picker with a native allowed us to close the following issues:

More Info

@kean kean changed the title Replace Replace custom media picker with native PHPickerViewController Jul 26, 2023
@kean
Copy link
Contributor Author

kean commented Jul 27, 2023

One of the other benefits of using the native picker is that it runs out of process, so it doesn't contribute to the app's memory usage. It can be hugely beneficial, for example, if you are editing a post that already requires a lot of memory. It will also never bring the app down if it crashes.

Uploading media from the device must be one of the major reasons users will want the app on their phones. It has to work flawlessly.

@staskus
Copy link
Contributor

staskus commented Jul 27, 2023

@kean, thanks for the proposal!

I agree with your reasoning, especially given the benefits of memory management. Plus, since it's native it will require less maintenance compared to maintaining our own library MediaPicker-iOS.

PHPickerViewController is fairly new, when starting fresh I doubt we would choose to create our own MediaPicker-iOS.

To make a transparent transition, I think it would be good to map the features that we use of MediaPicker-iOS and PHPickerViewController to be transparent about what can be supported out of the box and what we would lose out (if anything) if we make the transition. A quick search shows that the picker is used not only for picking the photo from the device but also for WordPress Media Library, Free Photo Library, and Free GIF Library, at least from the editor (TenorPicker, StockPhotosPicker, MediaLibraryPicker).

@osullivanchris
Copy link

Came across this PR from your comment re; KPIs. Please push to make this happen and advocate for same on Android. Sounds great.

@osullivanchris
Copy link

osullivanchris commented Aug 1, 2023

This approach 100% makes sense when the user wants to access all their device images. I just wanted to check something. We have some explorations where we're showing media in line.

  • In the first example, the user has added an image block. Before opening the full library of photos on the device, Paul tried showing some recent images within the sheet
  • In the second example, I was playing around with showing images in the editor empty state. So a user could add an image in one tap, before even choosing an image block.

in-line media examples

Of course in both cases we'd need an entry point to the full 'phone library' of images. But I'm just wondering does what we're doing in these cases play well with what you're proposing here, or does it contradict what you're suggesting?

@kean
Copy link
Contributor Author

kean commented Aug 2, 2023

@osullivanchris, the iOS has the APIs to do both:

  • When you enter the fullscreen experience, we can show the native picker
  • There is also a way to display the recent local assets in any way you want in the app

@kean kean mentioned this issue Aug 9, 2023
13 tasks
@kean kean added this to the 23.1 milestone Aug 15, 2023
@kean kean self-assigned this Aug 15, 2023
@kean kean changed the title Replace custom media picker with native PHPickerViewController Integrate PHPickerViewController Aug 15, 2023
@kean
Copy link
Contributor Author

kean commented Aug 16, 2023

I agree with your reasoning, especially given the benefits of memory management. Plus, since it's native it will require less maintenance compared to maintaining our own library MediaPicker-iOS.

@staskus we are on track to completely eliminate the need for the MediaPicker repo: the Photos picker is already in the works, and the Media screen will be re-written to Swift in phase 2. It will be a huge win to help modernize the codebase. The only missing part is the "Free" libraries that, based on Tracks, virtually nobody use.

@staskus
Copy link
Contributor

staskus commented Aug 17, 2023

Thanks for sharing @kean, that's great news! 👍

@mokagio
Copy link
Contributor

mokagio commented Aug 21, 2023

I bumped some of the PRs in this issue from 23.1 to 23.2 because they were not ready to merge and the 23.1 code freeze will start shortly. I'll do the same with this issue, for consistency.

If this cannot wait two weeks and it's important that it makes it into this release, let me know and we'll organize a new beta once ready.

@kean
Copy link
Contributor Author

kean commented Aug 25, 2023

It's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants