Media: Add support for remaining asset types #21664

kean · 2023-09-29T22:03:24Z

There are multiple changes in this PR, so please bear with me. Once this is merged, the new screens will cover 100% of the original MediaLibraryViewController functionally (and a little bit more). I've been commenting out the re-implement lines as I was working on this project, so this is how I know.

Changes:

Add support for remaining document types: video, document, audio
Add GIF rendering and increase the scale of requested GIFs to the max of 2x. Downscaling too much resulted in artifacts during playback.
Add accessibility support
Pop details view controller if media is deleted

I have a couple of more performance improvements in mind, but these will be done later.

To test:

Verify that the existing video assets display the duration
Verify that when you upload a new video, it displays the duration
Verify that documents and audio have a respective icon and their filenames displayed
Verify that cells are accessibility elements (copied from the previous version)
Verify that after going to the details page and deleting the asset on it, the details page gets popped from the stack (see recording)
Verify that GIFs are now animated (we already had the files, so I thought, why not just animate them; it wasn't supported before)

Regression Notes

Potential unintended areas of impact: n/a (no production code changed)
What I did to test those areas of impact (or what existing automated tests I relied on): manual
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)

kean · 2023-09-29T22:03:49Z

WordPress/Classes/Services/MediaImageService.swift

+            let scale = UIScreen.main.scale
+            var targetSize = targetSize
+            if remoteURL.isGif {
+                targetSize = targetSize


Downscaling from 3x to 1x turned out to be a bit too much.

kean · 2023-09-29T22:04:24Z

WordPress/Classes/ViewRelated/Media/SiteMedia/Views/SiteMediaCollectionCell.swift

+import Combine
+import Gifu
+
+final class SiteMediaCollectionCell: UICollectionViewCell {


I don't know why it didn't register the rename 😞

wpmobilebot · 2023-09-29T22:23:25Z

📲 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	pr21664-3cfa373
	Version	23.4
	Bundle ID	com.jetpack.alpha
	Commit	`3cfa373`
	App Center Build	jetpack-installable-builds #6329

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

wpmobilebot · 2023-09-29T22:23:54Z

📲 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	pr21664-3cfa373
	Version	23.4
	Bundle ID	org.wordpress.alpha
	Commit	`3cfa373`
	App Center Build	WPiOS - One-Offs #7293

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

kean · 2023-10-02T21:39:06Z

Next steps is to test it with self-hosted sites, add some unit tests. I expect there are going to be some complication with video thumbnails.

peril-wordpress-mobile · 2023-10-03T15:54:25Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

momo-ozawa

Works as described!

When uploading files, the cell doesn't update to include the filename once it's finished uploading the file. Pull to refresh doesn't update the cells either. You have to exit the screen then come back to see the correct icon / filename.

RPReplay_Final1696427175.MP4

WordPress/Classes/ViewRelated/Media/SiteMedia/Views/SiteMediaCollectionCellViewModel.swift

momo-ozawa · 2023-10-04T13:34:11Z

WordPress/Classes/Services/MediaImageService.swift

@@ -299,6 +311,9 @@ private extension Data {
    // JPEG magic numbers https://en.wikipedia.org/wiki/JPEG
    static let jpegMagicNumbers: [UInt8] = [0xFF, 0xD8, 0xFF]

+    // GIF magic numbers https://en.wikipedia.org/wiki/GIF
+    static let gifMagicNumbers: [UInt8] = [0x47, 0x49, 0x46]


momo-ozawa · 2023-10-04T13:36:30Z

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

@@ -95,7 +95,7 @@ final class SiteMediaCollectionViewController: UIViewController, NSFetchedResult
    }

    private func configureCollectionView() {
-        collectionView.register(MediaCollectionCell.self, forCellWithReuseIdentifier: Constants.cellID)
+        collectionView.register(SiteMediaCollectionCell.self, forCellWithReuseIdentifier: Constants.cellID)


[nit] If you make SiteMediaCollectionCell conform to Reusable, you can use defaultReuseID

I didn't know it was there. Thanks – updated!

…ollectionCellViewModel.swift Co-authored-by: Momo Ozawa <[email protected]>

kean · 2023-10-04T15:19:04Z

When uploading files, the cell doesn't update to include the filename once it's finished uploading the file.

I think I haven't tested this flow: only existing uploads from the web. Let me address that in a separate PR. I put it on the list.

kean · 2023-10-04T16:52:23Z

Just to add to the last comment: there are too many parts of the media that can change after the initial cell display. I want to simplify the way it reloads a bit to fix this issue. I think it'll be best to review it separately.

momo-ozawa · 2023-10-05T14:52:19Z

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

@@ -95,7 +95,7 @@ final class SiteMediaCollectionViewController: UIViewController, NSFetchedResult
    }

    private func configureCollectionView() {
-        collectionView.register(SiteMediaCollectionCell.self, forCellWithReuseIdentifier: Constants.cellID)
+        collectionView.register(cell: SiteMediaCollectionCell.self)


Did you mean to do

collectionView.register(SiteMediaCollectionCell.self, forCellWithReuseIdentifier: SiteMediaCollectioinCell.defaultReuseID)

Looks like you're using a extension method defined in Kanvas

func register<T: UICollectionViewCell>(cell cellType: T.Type) { register(cellType, forCellWithReuseIdentifier: cellType.identifier) }

This works, because UICollectionReusableView conforms to Kanvas.Identifiable. Just wanted to point out that it's not leveraging Reusable, but Kanvas.Identifiable instead

Hmm, Kanvas.Identifiable is nice, but we should probably not rely on it. I want to get this merged before the trunk gets too far ahead. Let me address it separately together with the filename fix.

momo-ozawa

Approving - up to you to decide about my comment above 🙇‍♀️

kean added Media [Type] Task UI Modernization labels Sep 29, 2023

kean added this to the Pending milestone Sep 29, 2023

kean commented Sep 29, 2023

View reviewed changes

kean mentioned this pull request Sep 29, 2023

Modernize Media screen #21457

Closed

21 tasks

kean requested a review from momo-ozawa October 2, 2023 20:34

kean added 7 commits October 2, 2023 16:51

Load previews thumbnails only for images and video

c8924f3

Add SiteMedia prefix to the remaining views

af65ac4

Add duration label

2c56362

Add support for other document types

a78edc9

Add GIF playback

2d4ed31

Add accessibility support

0d27ce7

Pop details VC if media is deleted

669c2e4

kean force-pushed the task/media-foundation-remaining-types branch from 20d6bb2 to 669c2e4 Compare October 2, 2023 20:51

kean added 2 commits October 3, 2023 11:37

Fix an issue with GIF reuse

28a49fe

Fix an issue where failed responses were still decoded

fbe65eb

momo-ozawa reviewed Oct 4, 2023

View reviewed changes

Update WordPress/Classes/ViewRelated/Media/SiteMedia/Views/SiteMediaC…

2ac82bc

…ollectionCellViewModel.swift Co-authored-by: Momo Ozawa <[email protected]>

Adopt reusable

3cfa373

kean requested a review from momo-ozawa October 5, 2023 13:32

momo-ozawa reviewed Oct 5, 2023

View reviewed changes

momo-ozawa approved these changes Oct 5, 2023

View reviewed changes

kean enabled auto-merge October 5, 2023 17:06

kean merged commit d7c69fd into trunk Oct 5, 2023

kean deleted the task/media-foundation-remaining-types branch October 5, 2023 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media: Add support for remaining asset types #21664

Media: Add support for remaining asset types #21664

kean commented Sep 29, 2023 •

edited

Loading

kean Sep 29, 2023

kean Sep 29, 2023

wpmobilebot commented Sep 29, 2023 •

edited

Loading

wpmobilebot commented Sep 29, 2023 •

edited

Loading

kean commented Oct 2, 2023

peril-wordpress-mobile bot commented Oct 3, 2023

momo-ozawa left a comment

momo-ozawa Oct 4, 2023

momo-ozawa Oct 4, 2023

kean Oct 4, 2023

kean commented Oct 4, 2023

kean commented Oct 4, 2023

momo-ozawa Oct 5, 2023

kean Oct 5, 2023

momo-ozawa left a comment

Media: Add support for remaining asset types #21664

Media: Add support for remaining asset types #21664

Conversation

kean commented Sep 29, 2023 • edited Loading

To test:

Regression Notes

kean Sep 29, 2023

Choose a reason for hiding this comment

kean Sep 29, 2023

Choose a reason for hiding this comment

wpmobilebot commented Sep 29, 2023 • edited Loading

wpmobilebot commented Sep 29, 2023 • edited Loading

kean commented Oct 2, 2023

peril-wordpress-mobile bot commented Oct 3, 2023

momo-ozawa left a comment

Choose a reason for hiding this comment

momo-ozawa Oct 4, 2023

Choose a reason for hiding this comment

momo-ozawa Oct 4, 2023

Choose a reason for hiding this comment

kean Oct 4, 2023

Choose a reason for hiding this comment

kean commented Oct 4, 2023

kean commented Oct 4, 2023

momo-ozawa Oct 5, 2023

Choose a reason for hiding this comment

kean Oct 5, 2023

Choose a reason for hiding this comment

momo-ozawa left a comment

Choose a reason for hiding this comment

kean commented Sep 29, 2023 •

edited

Loading

wpmobilebot commented Sep 29, 2023 •

edited

Loading

wpmobilebot commented Sep 29, 2023 •

edited

Loading