Use `CoreDataStack` in `MediaThumbnailService` #20294

crazytonyli · 2023-03-09T20:40:27Z

Unlike other similar PR, this PR has changes that are not just simply swapping NSManagedObjectContext with CoreDataStack. I'll add some PR comments for detailed reasons.

Test instructions

Mainly the uploading feature in the "Media" screen is impacted. Verifying uploading media still works should be sufficient.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
What are described in the "Test instructions" above.
What automated tests I added (or what prevented me from doing so)
None.

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.

crazytonyli · 2023-03-09T20:44:05Z

WordPress/Classes/Services/MediaThumbnailCoordinator.swift

-        }
+        let mediaService = MediaService(managedObjectContext: coreDataStack.mainContext)
+        mediaService.getMediaWithID(mediaID, in: media.blog, success: { (loadedMedia) in
+            onCompletion(loadedMedia, nil)


Since a Media object is leaked out of MediaService's lifecycle(or MediaService.managedObjectContext's to be exact), it's only safe to use the main context as the MediaService instance's context.

crazytonyli · 2023-03-09T20:51:43Z

WordPress/Classes/Services/MediaThumbnailService.swift

    ///
    /// - Note: Images may be downloaded and resized if required, avoid requesting multiple explicit preferredSizes
    ///   as several images could be downloaded, resized, and cached, if there are several variations in size.
    ///
    @objc func thumbnailURL(forMedia media: Media, preferredSize: CGSize, onCompletion: @escaping OnThumbnailURL, onError: OnError?) {
-        managedObjectContext.perform {
+        let context = coreDataStack.mainContext


Using the main context here because this function only read the Media instance, without changing it.

Also, there is no time consuming work done in the block that's passed to the perform function, all heavy lifting are done in background queues.

Suggested change

let context = coreDataStack.mainContext

// We can use the main context here because we only read the `Media` instance, without changing it, and all

// the time consuming work is done in background queues.

let context = coreDataStack.mainContext

crazytonyli · 2023-03-09T20:56:57Z

WordPress/Classes/Services/MediaThumbnailService.swift

-    ///   - onCompletion: Completion handler passing the URL once available, or nil if unavailable.
-    ///   - onError: Error handler.
+    ///   - onCompletion: Completion handler passing the URL once available, or nil if unavailable. This closure is called on the `exportQueue`.
+    ///   - onError: Error handler. This closure is called on the `exportQueue`.


Originally, these closures are called on the managedObjectContext queue. Since we don't have a long living context object anymore, they are changed to use the exportQueue.

crazytonyli · 2023-03-09T20:59:33Z

WordPress/Classes/Services/MediaThumbnailService.swift

+                    remoteURL = WPImageURLHelper.imageURLWithSize(preferredSize, forImageURL: remoteAssetURL)
+                } else {
+                    remoteURL = PhotonImageURLHelper.photonURL(with: preferredSize, forImageURL: remoteAssetURL)
+                }


The above changes are mainly for removing the onCompletion(nil) calls above, which are now covered by the guard let imageURL = remoteURL case below.

crazytonyli · 2023-03-09T21:02:24Z

WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift

    private let onSuccess: (UIImage) -> ()
    private let onFailure: (Error) -> ()

-    init(url: URL, blog: Blog, onSuccess: @escaping (UIImage) -> (), onFailure: @escaping (Error) -> ()) {
+    init(url: URL, blog: Blog, callbackQueue: DispatchQueue, onSuccess: @escaping (UIImage) -> (), onFailure: @escaping (Error) -> ()) {


Originally the onSuccess closure is called on the main queue, and the onFailure closure is called on a background thread that runs this operation. Adding this calbackQueue to unify their calling queue. Also, it makes the enforcing callback queue code in MediaThumbnailService nicer, without requiring additional async calls there.

crazytonyli · 2023-03-09T21:04:38Z

WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift

@@ -140,6 +145,7 @@ class EditorMediaUtility {
        let imageDownload = AuthenticatedImageDownload(
            url: requestURL,
            blog: post.blog,
+            callbackQueue: .main,


This should be an improvement to the existing behaviour, since onFailure wasn't called on the main thread, but I believe this function requires the error handling to be on the main thread.

crazytonyli · 2023-03-09T21:05:55Z

WordPress/WordPressTest/MediaPicker/MediaLibraryPickerDataSourceTests.swift

@@ -121,8 +121,7 @@ class MediaLibraryPickerDataSourceTests: CoreDataTestCase {

        let mediaService = MediaService(managedObjectContext: mainContext)
        let expect = self.expectation(description: "Media should be create with success")
-        mediaService.createMedia(with: url as NSURL, blog: blog, post: post, progress: nil, thumbnailCallback: { (media, url) in
-        }, completion: { (media, error) in
+        mediaService.createMedia(with: url as NSURL, blog: blog, post: post, progress: nil, thumbnailCallback: nil, completion: { (media, error) in


This is just a small thing I noticed—no point passing an empty closure if nil is an acceptable argument.

wpmobilebot · 2023-03-09T21:08:11Z

📲 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	pr20294-62f47cc
	Version	21.9
	Bundle ID	org.wordpress.alpha
	Commit	`62f47cc`
	App Center Build	WPiOS - One-Offs #5191

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

wpmobilebot · 2023-03-09T21:44:03Z

📲 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	pr20294-62f47cc
	Version	21.9
	Bundle ID	com.jetpack.alpha
	Commit	`62f47cc`
	App Center Build	jetpack-installable-builds #4216

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

mokagio

Unlike other similar PR, this PR has changes that are not just simply swapping NSManagedObjectContext with CoreDataStack. I'll add some PR comments for detailed reasons.

Thanks!

Mainly the uploading feature in the "Media" screen is impacted. Verifying uploading media still works should be sufficient.

Upload successful and the thumbnails appear alright.

mokagio · 2023-03-10T04:07:23Z

WordPress/Classes/Services/MediaThumbnailCoordinator.swift

+        let mediaService = MediaService(managedObjectContext: coreDataStack.mainContext)
+        mediaService.getMediaWithID(mediaID, in: media.blog, success: { (loadedMedia) in
+            onCompletion(loadedMedia, nil)


What do you think of tracking what your wrote in the comment above in the code?

Suggested change

let mediaService = MediaService(managedObjectContext: coreDataStack.mainContext)

mediaService.getMediaWithID(mediaID, in: media.blog, success: { (loadedMedia) in

onCompletion(loadedMedia, nil)

// It's only safe to use the main context as the MediaService instance's context because a Media object is

// leaked out of MediaService's lifecycle (MediaService.managedObjectContext's to be exact.

let mediaService = MediaService(managedObjectContext: coreDataStack.mainContext)

mediaService.getMediaWithID(mediaID, in: media.blog, success: { (loadedMedia) in

onCompletion(loadedMedia, nil)

Also, SwiftLint might at some point complain about the redundant () around loadedMedia. 🤷‍♂️

mokagio · 2023-03-10T04:14:19Z

WordPress/Classes/Services/MediaThumbnailService.swift

    ///
    /// - Note: Images may be downloaded and resized if required, avoid requesting multiple explicit preferredSizes
    ///   as several images could be downloaded, resized, and cached, if there are several variations in size.
    ///
    @objc func thumbnailURL(forMedia media: Media, preferredSize: CGSize, onCompletion: @escaping OnThumbnailURL, onError: OnError?) {
-        managedObjectContext.perform {
+        let context = coreDataStack.mainContext


Suggested change

let context = coreDataStack.mainContext

// We can use the main context here because we only read the `Media` instance, without changing it, and all

// the time consuming work is done in background queues.

let context = coreDataStack.mainContext

mokagio · 2023-03-10T04:15:16Z

WordPress/Classes/Services/MediaThumbnailService.swift

+    private func downloadThumbnail(
+        forMedia media: Media,
+        preferredSize: CGSize,
+    callbackQueue: DispatchQueue,


Suggested change

callbackQueue: DispatchQueue,

callbackQueue: DispatchQueue,

mokagio · 2023-03-10T04:17:49Z

WordPress/WordPressTest/MediaPicker/MediaLibraryPickerDataSourceTests.swift

@@ -121,8 +121,7 @@ class MediaLibraryPickerDataSourceTests: CoreDataTestCase {

        let mediaService = MediaService(managedObjectContext: mainContext)
        let expect = self.expectation(description: "Media should be create with success")
-        mediaService.createMedia(with: url as NSURL, blog: blog, post: post, progress: nil, thumbnailCallback: { (media, url) in
-        }, completion: { (media, error) in
+        mediaService.createMedia(with: url as NSURL, blog: blog, post: post, progress: nil, thumbnailCallback: nil, completion: { (media, error) in


Use CoreDataStack in MediaThumbnailService

b587ff6

crazytonyli added the Core Data Issues related to Core Data label Mar 9, 2023

crazytonyli added this to the 22.0 milestone Mar 9, 2023

crazytonyli requested a review from mokagio March 9, 2023 20:40

crazytonyli self-assigned this Mar 9, 2023

Update release notes

a028f6f

crazytonyli commented Mar 9, 2023

View reviewed changes

mokagio approved these changes Mar 10, 2023

View reviewed changes

Add some code comments

62f47cc

crazytonyli enabled auto-merge March 14, 2023 03:01

crazytonyli merged commit 152708d into trunk Mar 14, 2023

crazytonyli deleted the tonyli-refactor-media-service-types branch March 14, 2023 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `CoreDataStack` in `MediaThumbnailService` #20294

Use `CoreDataStack` in `MediaThumbnailService` #20294

crazytonyli commented Mar 9, 2023

crazytonyli Mar 9, 2023

crazytonyli Mar 9, 2023

crazytonyli Mar 9, 2023

mokagio Mar 10, 2023

crazytonyli Mar 9, 2023

crazytonyli Mar 9, 2023

crazytonyli Mar 9, 2023

crazytonyli Mar 9, 2023

crazytonyli Mar 9, 2023

mokagio Mar 10, 2023

wpmobilebot commented Mar 9, 2023 •

edited

Loading

wpmobilebot commented Mar 9, 2023 •

edited

Loading

mokagio left a comment

mokagio Mar 10, 2023

mokagio Mar 10, 2023

mokagio Mar 10, 2023

mokagio Mar 10, 2023

mokagio Mar 10, 2023

Use CoreDataStack in MediaThumbnailService #20294

Use CoreDataStack in MediaThumbnailService #20294

Conversation

crazytonyli commented Mar 9, 2023

Test instructions

Regression Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Mar 9, 2023 • edited Loading

wpmobilebot commented Mar 9, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `CoreDataStack` in `MediaThumbnailService` #20294

Use `CoreDataStack` in `MediaThumbnailService` #20294

wpmobilebot commented Mar 9, 2023 •

edited

Loading

wpmobilebot commented Mar 9, 2023 •

edited

Loading