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 in Page Editor #21360

Merged
merged 5 commits into from
Aug 22, 2023
Merged

Conversation

kean
Copy link
Contributor

@kean kean commented Aug 17, 2023

Part of #21190.

The changes affect both the WP and the Jetpack app.

Simulator.Screen.Recording.-.iPad.Air.5th.generation.-.2023-08-17.at.11.51.23.mp4

Tests

Important

  • Enable the "Native Photo Picker" feature flag before testing
  • Verify that the Image block uploads work
  • Verify that the Gallery block uploads work (make sure the order of inserted photos matches the selection)
  • Verify that the Video block uploads work

For additional test cases, see #21343. It covers the picker and the media uploads at great lengths. But there should be no need to repeat them because this PR builds on top of these changes.

[Tech] Preview URL

When the media is added to one of the Gutenberg blocks, it expects a URL. But the app has no URLs at the time the user makes a selection in the native picker.

In the current implementation, when you select the assets, the editor first requests their previews using PHImageManager.default().requestImage and .fastFormat and only then updates the Gutenberg blocks. It achieves that when the blocks are updated, the images already have previews, but it comes at a great cost.

The requested thumbnails are usually very small: example (120 ×90px), but with the next line thumbImage.resizedImage(asset.pixelSize(), the editor blows it up to the original image size: example (4032 × 3024px) and puts them on disk in the temporary folder. There are many problems with it:

  • The requests are performed in parallel with the work MediaImportService does to export the images and generate the previews (work duplication)
  • There is no limit on the number of concurrent requests
  • The resizing is expensive, and both the CPU and the RAM usage spike when you add the images: 480 MB → 680 MB when adding just five images
  • If the assets in the cloud, the blocks won't get updated until they are downloaded

As a result, the app ends up wasting a ton of CPU, RAM, and disk space just to show previews that almost immediately (a few hundred milliseconds later) get replaced by MediaImportService in the following callback:

 private func mediaObserver(media: Media, state: MediaCoordinator.MediaState) {
        let mediaUploadID = media.gutenbergUploadID
        case .thumbnailReady(let url):
            guard ReachabilityUtils.isInternetReachable() && media.remoteStatus != .failed else {
                gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .failed, progress: 0, url: url, serverID: nil)
                return
            }
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .uploading, progress: 0.20, url: url, serverID: nil)
            // ...

The solution I went with in this PR was to not generate the thumbnails at all and just pass placeholder URLs that point to nothing. It dramatically reduces the memory footprint and there is no delay after the selection is made in the picker:

  • Before: 480 MB → 680 MB (peak) (+200 MB)
  • After: 490 MB → 505 MB (peak) (+15 MB)

The previews get displayed as soon as the MediaImportService generates them.

Regression Notes

  1. Potential unintended areas of impact: Media uploads in Post and Page Editor
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual tests
  3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

import WordPressShared
import WPMediaPicker
import Gutenberg
import UniformTypeIdentifiers

public typealias GutenbergMediaPickerHelperCallback = ([WPMediaAsset]?) -> Void
public typealias GutenbergMediaPickerHelperCallback = ([Any]?) -> Void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am specifically not using the WPMediaAsset protocol because it’s part of the MediaPicker repo and is designed for displaying images in the picker’s collection view cells, but it ended up being used as a marker protocol in multiple places of the app. The goal is to remove the MediaPicker dependency entirely.

@@ -154,9 +180,6 @@ class GutenbergMediaInserterHelper: NSObject {
}

func syncUploads() {
if mediaObserverReceipt != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This duplicates the code from #21352 because these changes need to be tested together.

@kean kean requested a review from guarani August 17, 2023 16:14
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 17, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21360-8d08f37
Version23.0
Bundle IDorg.wordpress.alpha
Commit8d08f37
App Center BuildWPiOS - One-Offs #6798
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 17, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21360-8d08f37
Version23.0
Bundle IDcom.jetpack.alpha
Commit8d08f37
App Center Buildjetpack-installable-builds #5838
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio mokagio modified the milestones: 23.1, 23.2 Aug 21, 2023
@mokagio
Copy link
Contributor

mokagio commented Aug 21, 2023

I'm going to bump this to the next release because we'll be code freezing 23.1 today and this hasn't been approved yet.

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 kean requested review from momo-ozawa and removed request for guarani August 21, 2023 15:43
Base automatically changed from task/phpicker-media-screen to trunk August 21, 2023 19:15
Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as described

@geriux geriux self-requested a review August 22, 2023 16:15
Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! I've tested different cases using the Gutenberg editor and the new media picker. Users will benefit from this so much 🚀 Nice work!

@twstokes
Copy link
Contributor

I test drove this as well and it looked great! 🚀 Thank you @kean.

@kean
Copy link
Contributor Author

kean commented Aug 22, 2023

Thanks, @geriux and @twstokes. I really appreciate it!

I'll turn on the feature flag soon, so we'll have some more time to collect feedback before the release. And there are a few more improvements coming soon 😊

@kean kean enabled auto-merge August 22, 2023 18:15
@kean kean merged commit dd4c591 into trunk Aug 22, 2023
@kean kean deleted the task/phpicker-editor branch August 22, 2023 18:47
@kean kean mentioned this pull request Oct 4, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants