feat: GutenbergKit Media Library support #23721

dcalhoun · 2024-10-29T18:04:23Z

Description

Enable attaching media selected from the Media Library.

To test: See wordpress-mobile/GutenbergKit#35.

Regression Notes

Potential unintended areas of impact
Existing Media Library support in Gutenberg Mobile, Aztec, etc.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested Media Library support in alternative editors.
What automated tests I added (or what prevented me from doing so)
Not a priority for this experimental editor.

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.

Testing checklist:

A single Media Library callback resulted in erroneously mutating unselected blocks.

Unable to recreate the original erroneously replacement of media attachments for unselected blocks. This now feels unnecessary.

Address lint warning.

Enable retaining selection for gallery addition/editing in the block editor.

Reusing the `initialSelection` reduces complexity.

This no longer disrupts initial selection as the setting the initial selection now occurs within `setEditing` rather than initialization.

This appears to be unnecessary with the latest implementation.

Without returning the initial selection, canceling cleared out media attached to Gallery blocks in the web-based editor, as it presumes the existing media will be returned as "selected."

Workaround existing logic that dismisses the dialog if `allowMuliple` is true and selection is not empty. The existing logic allows for quickly selecting a single media item, but inhibits the ability to showcase a pre-existing selection for single-select contexts. This diverges from the web experience, but is the easiest path forward to avoid a significant refactor of the logic that would impact all other existing use cases--Gutenberg Mobile, Aztec, etc.

Map the Media lookup results to the original array of media IDs so that the sort order is preserved.

dangermattic · 2024-10-29T18:05:09Z

	2 Warnings
⚠️	`Modules/Package.swift` was changed without updating its corresponding `Package.resolved`. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running `swift package resolve`).
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

wpmobilebot · 2024-10-29T18:16:09Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23721-48be44a
	Version	25.4
	Bundle ID	org.wordpress.alpha
	Commit	`48be44a`
	App Center Build	WPiOS - One-Offs #11028

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2024-10-29T18:17:53Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23721-48be44a
	Version	25.4
	Bundle ID	com.jetpack.alpha
	Commit	`48be44a`
	App Center Build	jetpack-installable-builds #10067

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

dcalhoun · 2024-10-30T12:11:22Z

...ress/Classes/ViewRelated/Media/SiteMedia/Controllers/SiteMediaCollectionViewController.swift

+        if let selectedMedia = initialSelection, allowsMultipleSelection {
+            setInitialSelection(selectedMedia)
+        } else {
+            deselectAll()
+        }


In order to support an initial selection for Gallery blocks—which is required by the web Gallery block implementation—we must now conditionally deselect all. Retaining an initial selection also improves the UX.

The restriction of initial selections to allowsMultipleSelection diverges from the web Media Library experience, but is a workaround for the native Media Library's current implementation that dismisses the dialog when a selection exists and multiple is disallowed.

The native approach creates a flow where tapping an image selects and dismisses the dialog for single-select contexts, reducing the number of taps to complete the flow. Rather than attempting a larger change to modify this behavior and align with web, I disabled initial selection when multiple selection is disabled—i.e., any block besides a Gallery.

dcalhoun · 2024-10-30T12:16:02Z

WordPress/Classes/ViewRelated/Media/SiteMedia/SiteMediaPickerViewController.swift


    private func buttonCancelTapped() {
-        delegate?.siteMediaPickerViewController(self, didFinishWithSelection: [])
+        delegate?.siteMediaPickerViewController(self, didFinishWithSelection: initialSelection)


Given initialSelection should be [] outside of the specific context of the "new" Gutenberg, this change should, in theory, have no negative impact on existing use case—e.g., Gutenberg Mobile, Aztec.

dcalhoun · 2024-10-30T13:04:15Z

WordPress/Classes/ViewRelated/NewGutenberg/NewGutenbergViewController.swift

This approach was copied and adapted from GutenbergViewController.

jkmassel · 2024-11-08T21:28:56Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

            .showSiteMediaPicker(blog: post.blog, delegate: self)
    }

+    private func mapMediaIdsToMedia(_ mediaIds: [Int]) -> [Media] {


Any Media objects returned from this method aren't thread-safe – it looks like a lot of what's going on here should happen on the main thread, but can we add assertions to ensure that is the case?

We could also convert these to thread-safe objects, but that might be overkill at this point?

Thank you for noting this. I am quite unfamiliar with the subject of thread safety, so I appreciate that inquiries and suggestions.

I added an assertion in 1867499 based on what I understand of your feedback. Please let me know if I misunderstood or you have alternatives to suggest.

jkmassel

I tested this and it works well. I have a few code-level suggestions, but I don't think this needs additional review if/when they're addressed

Improve clarity.

The File block does not provide these value.

The returned `Media` entities are not thread-safe.

dcalhoun added 14 commits October 25, 2024 16:59

wip: GutenbergKit Media Library support

544964f

fix: JSON encode media array and pass as string to WebView

d184095

fix: Prevent global callback collisions

4152806

A single Media Library callback resulted in erroneously mutating unselected blocks.

refactor: Remove Media Library UUID usage

f8b3e2b

Unable to recreate the original erroneously replacement of media attachments for unselected blocks. This now feels unnecessary.

style: Remove unnecessary white space

54a91fb

Address lint warning.

feat: Media Library preserves initial selection

3706129

Enable retaining selection for gallery addition/editing in the block editor.

refactor: Remove unnecessary preserveSelection parameter

eec3de1

Reusing the `initialSelection` reduces complexity.

fix: Reinstate commented selection clearing code

c6b0ee2

This no longer disrupts initial selection as the setting the initial selection now occurs within `setEditing` rather than initialization.

refactor: Remove unnecessary application of initial selection

61e5950

This appears to be unnecessary with the latest implementation.

refactor: Remove unused preserveSelection parameter

0b72038

fix: Canceling media selection returns initial selection

ea862c0

Without returning the initial selection, canceling cleared out media attached to Gallery blocks in the web-based editor, as it presumes the existing media will be returned as "selected."

fix: Ensure correct media attachment sorting

a0660b7

Map the Media lookup results to the original array of media IDs so that the sort order is preserved.

build: Update GutenbergKit ref

bd817a6

dcalhoun added [Type] Enhancement Gutenberg Editing and display of Gutenberg blocks. labels Oct 29, 2024

dcalhoun mentioned this pull request Oct 29, 2024

feat: Media Library support (iOS) wordpress-mobile/GutenbergKit#35

Merged

dcalhoun added 3 commits October 29, 2024 14:48

build: Update GutenbergKit ref

2908de7

docs: Expand existing documentation to include new parameter

44e1c33

refactor: Update bridge method name to mirror GutenbergKit

c7d8364

dcalhoun commented Oct 30, 2024

View reviewed changes

dcalhoun marked this pull request as ready for review October 30, 2024 13:36

dcalhoun requested a review from jkmassel October 30, 2024 13:36

jkmassel reviewed Nov 8, 2024

View reviewed changes

jkmassel approved these changes Nov 8, 2024

View reviewed changes

dcalhoun added 2 commits November 11, 2024 14:01

refactor: Rename OpenMediaLibrary to OpenMediaLibraryAction

3f561cd

Improve clarity.

fix: Include VideoPress ID in metadata

2c1b2c4

dcalhoun added 4 commits November 11, 2024 14:19

fix: Account for null allowedTypes values

eafd0e7

The File block does not provide these value.

fix: Assert main thread for mapMediaIdsToMedia

1867499

The returned `Media` entities are not thread-safe.

build: Update GutenbergKit ref

e93dd1c

build: Update GutenbergKit ref

48be44a

dcalhoun enabled auto-merge November 11, 2024 20:12

dcalhoun added this pull request to the merge queue Nov 12, 2024

Merged via the queue into trunk with commit 5b3871e Nov 12, 2024
24 checks passed

dcalhoun deleted the feat/gutenberg-kit-media-library branch November 12, 2024 04:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: GutenbergKit Media Library support #23721

feat: GutenbergKit Media Library support #23721

Uh oh!

dcalhoun commented Oct 29, 2024 •

edited

Loading

Uh oh!

dangermattic commented Oct 29, 2024 •

edited

Loading

Uh oh!

wpmobilebot commented Oct 29, 2024 •

edited

Loading

Uh oh!

wpmobilebot commented Oct 29, 2024 •

edited

Loading

Uh oh!

dcalhoun Oct 30, 2024 •

edited

Loading

Uh oh!

dcalhoun Oct 30, 2024

Uh oh!

dcalhoun Oct 30, 2024 •

edited

Loading

Uh oh!

jkmassel Nov 8, 2024

Uh oh!

dcalhoun Nov 11, 2024

Uh oh!

jkmassel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat: GutenbergKit Media Library support #23721

feat: GutenbergKit Media Library support #23721

Uh oh!

Conversation

dcalhoun commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related

Description

Regression Notes

Uh oh!

dangermattic commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcalhoun Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcalhoun Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

dcalhoun Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkmassel Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

dcalhoun Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

jkmassel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dcalhoun commented Oct 29, 2024 •

edited

Loading

dangermattic commented Oct 29, 2024 •

edited

Loading

wpmobilebot commented Oct 29, 2024 •

edited

Loading

wpmobilebot commented Oct 29, 2024 •

edited

Loading

dcalhoun Oct 30, 2024 •

edited

Loading

dcalhoun Oct 30, 2024 •

edited

Loading