diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index c237b505d363..e79340a13748 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -4,6 +4,7 @@ * [*] [internal] Refactored the Core Data operations (saving the site data) after a new site is created. [#20270] * [*] [internal] Refactored updating user role in the "People" screen on the "My Sites" tab. [#20244] * [*] [internal] Refactor managing social connections and social buttons in the "Sharing" screen. [#20265] +* [*] [internal] Refactor uploading media assets. [#20294] 21.9 ----- diff --git a/WordPress/Classes/Services/MediaService.m b/WordPress/Classes/Services/MediaService.m index ed2c5f6ef89f..eea3bec35b72 100644 --- a/WordPress/Classes/Services/MediaService.m +++ b/WordPress/Classes/Services/MediaService.m @@ -679,7 +679,7 @@ - (void)getMediaLibraryServerCountForBlog:(Blog *)blog - (MediaThumbnailService *)thumbnailService { if (!_thumbnailService) { - _thumbnailService = [[MediaThumbnailService alloc] initWithManagedObjectContext:self.managedObjectContext]; + _thumbnailService = [[MediaThumbnailService alloc] initWithContextManager:[ContextManager sharedInstance]]; if (self.concurrentThumbnailGeneration) { _thumbnailService.exportQueue = dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0); } diff --git a/WordPress/Classes/Services/MediaThumbnailCoordinator.swift b/WordPress/Classes/Services/MediaThumbnailCoordinator.swift index b395688ed1f3..4eade2adf11b 100644 --- a/WordPress/Classes/Services/MediaThumbnailCoordinator.swift +++ b/WordPress/Classes/Services/MediaThumbnailCoordinator.swift @@ -9,7 +9,7 @@ class MediaThumbnailCoordinator: NSObject { @objc static let shared = MediaThumbnailCoordinator() - private var coreDataStack: CoreDataStack { + private var coreDataStack: CoreDataStackSwift { ContextManager.shared } @@ -48,11 +48,9 @@ class MediaThumbnailCoordinator: NSObject { } } - coreDataStack.performAndSave { context in - let mediaThumbnailService = MediaThumbnailService(managedObjectContext: context) - mediaThumbnailService.exportQueue = self.queue - mediaThumbnailService.thumbnailURL(forMedia: media, preferredSize: size, onCompletion: success, onError: failure) - } + let mediaThumbnailService = MediaThumbnailService(coreDataStack: coreDataStack) + mediaThumbnailService.exportQueue = self.queue + mediaThumbnailService.thumbnailURL(forMedia: media, preferredSize: size, onCompletion: success, onError: failure) } /// Tries to generate a thumbnail for the specified media object that is stub with the size requested @@ -80,13 +78,13 @@ class MediaThumbnailCoordinator: NSObject { return } - coreDataStack.performAndSave { context in - let mediaService = MediaService(managedObjectContext: context) - mediaService.getMediaWithID(mediaID, in: media.blog, success: { (loadedMedia) in - onCompletion(loadedMedia, nil) - }, failure: { (error) in - onCompletion(nil, error) - }) - } + // It's only safe to use the main context as this 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) + }, failure: { (error) in + onCompletion(nil, error) + }) } } diff --git a/WordPress/Classes/Services/MediaThumbnailService.swift b/WordPress/Classes/Services/MediaThumbnailService.swift index a718ba9db68c..229f7d8d1b0e 100644 --- a/WordPress/Classes/Services/MediaThumbnailService.swift +++ b/WordPress/Classes/Services/MediaThumbnailService.swift @@ -3,7 +3,7 @@ import Foundation /// A service for handling the process of retrieving and generating thumbnail images /// for existing Media objects, whether remote or locally available. /// -class MediaThumbnailService: LocalCoreDataService { +class MediaThumbnailService: NSObject { /// Completion handler for a thumbnail URL. /// @@ -19,24 +19,43 @@ class MediaThumbnailService: LocalCoreDataService { return MediaThumbnailService.defaultExportQueue }() + private let coreDataStack: CoreDataStackSwift + + /// The initialiser for Objective-C code. + /// + /// Using `ContextManager` as the argument becuase `CoreDataStackSwift` is not accessible from Objective-C code. + @objc + convenience init(contextManager: ContextManager) { + self.init(coreDataStack: contextManager) + } + + init(coreDataStack: CoreDataStackSwift) { + self.coreDataStack = coreDataStack + } + /// Generate a URL to a thumbnail of the Media, if available. /// /// - Parameters: /// - media: The Media object the URL should be a thumbnail of. /// - preferredSize: An ideal size of the thumbnail in points. If `zero`, the maximum dimension of the UIScreen is used. - /// - 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`. /// /// - 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 { + // 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 + context.perform { var objectInContext: NSManagedObject? do { - objectInContext = try self.managedObjectContext.existingObject(with: media.objectID) + objectInContext = try context.existingObject(with: media.objectID) } catch { - onError?(error) + self.exportQueue.async { + onError?(error) + } return } guard let mediaInContext = objectInContext as? Media else { @@ -56,7 +75,9 @@ class MediaThumbnailService: LocalCoreDataService { // Check if there is already an exported thumbnail available. if let identifier = mediaInContext.localThumbnailIdentifier, let availableThumbnail = exporter.availableThumbnail(with: identifier) { - onCompletion(availableThumbnail) + self.exportQueue.async { + onCompletion(availableThumbnail) + } return } @@ -69,12 +90,10 @@ class MediaThumbnailService: LocalCoreDataService { // Configure a handler for any thumbnail exports let onThumbnailExport: MediaThumbnailExporter.OnThumbnailExport = { (identifier, export) in - self.managedObjectContext.perform { - self.handleThumbnailExport(media: mediaInContext, - identifier: identifier, - export: export, - onCompletion: onCompletion) - } + self.handleThumbnailExport(media: mediaInContext, + identifier: identifier, + export: export, + onCompletion: onCompletion) } // Configure an error handler let onThumbnailExportError: OnExportError = { (error) in @@ -83,16 +102,12 @@ class MediaThumbnailService: LocalCoreDataService { // Configure an attempt to download a remote thumbnail and export it as a thumbnail. let attemptDownloadingThumbnail: () -> Void = { - self.downloadThumbnail(forMedia: mediaInContext, preferredSize: preferredSize, onCompletion: { (image) in + self.downloadThumbnail(forMedia: mediaInContext, preferredSize: preferredSize, callbackQueue: self.exportQueue, onCompletion: { (image) in guard let image = image else { onError?(MediaThumbnailExporter.ThumbnailExportError.failedToGenerateThumbnailFileURL) return } - self.exportQueue.async { - exporter.exportThumbnail(forImage: image, - onCompletion: onThumbnailExport, - onError: onThumbnailExportError) - } + exporter.exportThumbnail(forImage: image, onCompletion: onThumbnailExport, onError: onThumbnailExportError) }, onError: { (error) in onError?(error) }) @@ -117,7 +132,7 @@ class MediaThumbnailService: LocalCoreDataService { onError: { (error) in // If an error occurred with the remote video URL, try and download the Media's // remote thumbnail instead. - self.managedObjectContext.perform { + context.perform { attemptDownloadingThumbnail() } }) @@ -135,87 +150,74 @@ class MediaThumbnailService: LocalCoreDataService { /// - Parameters: /// - media: The Media object. /// - preferredSize: The preferred size of the image, in points, to configure remote URLs for. + /// - callbackQueue: The queue to execute the `onCompletion` or the `onError` callback. /// - onCompletion: Completes if everything was successful, but nil if no image is available. /// - onError: An error was encountered either from the server or locally, depending on the Media object or blog. /// /// - Note: based on previous implementation in MediaService.m. /// - fileprivate func downloadThumbnail(forMedia media: Media, - preferredSize: CGSize, - onCompletion: @escaping (UIImage?) -> Void, - onError: @escaping (Error) -> Void) { + private func downloadThumbnail( + forMedia media: Media, + preferredSize: CGSize, + callbackQueue: DispatchQueue, + onCompletion: @escaping (UIImage?) -> Void, + onError: @escaping (Error) -> Void + ) { var remoteURL: URL? // Check if the Media item is a video or image. if media.mediaType == .video { // If a video, ensure there is a remoteThumbnailURL - guard let remoteThumbnailURL = media.remoteThumbnailURL else { - // No video thumbnail available. - onCompletion(nil) - return + if let remoteThumbnailURL = media.remoteThumbnailURL { + remoteURL = URL(string: remoteThumbnailURL) } - remoteURL = URL(string: remoteThumbnailURL) } else { // Check if a remote URL for the media itself is available. - guard let remoteAssetURLStr = media.remoteURL, let remoteAssetURL = URL(string: remoteAssetURLStr) else { - // No remote asset URL available. - onCompletion(nil) - return - } - // Get an expected WP URL, for sizing. - if media.blog.isPrivateAtWPCom() || (!media.blog.isHostedAtWPcom && media.blog.isBasicAuthCredentialStored()) { - remoteURL = WPImageURLHelper.imageURLWithSize(preferredSize, forImageURL: remoteAssetURL) - } else { - remoteURL = PhotonImageURLHelper.photonURL(with: preferredSize, forImageURL: remoteAssetURL) + if let remoteAssetURLStr = media.remoteURL, let remoteAssetURL = URL(string: remoteAssetURLStr) { + // Get an expected WP URL, for sizing. + if media.blog.isPrivateAtWPCom() || (!media.blog.isHostedAtWPcom && media.blog.isBasicAuthCredentialStored()) { + remoteURL = WPImageURLHelper.imageURLWithSize(preferredSize, forImageURL: remoteAssetURL) + } else { + remoteURL = PhotonImageURLHelper.photonURL(with: preferredSize, forImageURL: remoteAssetURL) + } } } guard let imageURL = remoteURL else { // No URL's available, no images available. - onCompletion(nil) - return - } - let inContextImageHandler: (UIImage?) -> Void = { (image) in - self.managedObjectContext.perform { - onCompletion(image) - } - } - let inContextErrorHandler: (Error?) -> Void = { (error) in - self.managedObjectContext.perform { - guard let error = error else { - onCompletion(nil) - return - } - onError(error) + callbackQueue.async { + onCompletion(nil) } + return } - let download = AuthenticatedImageDownload(url: imageURL, blog: media.blog, onSuccess: inContextImageHandler, onFailure: inContextErrorHandler) + let download = AuthenticatedImageDownload(url: imageURL, blog: media.blog, callbackQueue: callbackQueue, onSuccess: onCompletion, onFailure: onError) download.start() } // MARK: - Helpers - fileprivate func handleThumbnailExport(media: Media, identifier: MediaThumbnailExporter.ThumbnailIdentifier, export: MediaExport, onCompletion: @escaping OnThumbnailURL) { - // Make sure the Media object hasn't been deleted. - guard media.isDeleted == false else { - onCompletion(nil) - return - } - if media.localThumbnailIdentifier != identifier { - media.localThumbnailIdentifier = identifier - ContextManager.sharedInstance().save(managedObjectContext) - } - onCompletion(export.url) + private func handleThumbnailExport(media: Media, identifier: MediaThumbnailExporter.ThumbnailIdentifier, export: MediaExport, onCompletion: @escaping OnThumbnailURL) { + coreDataStack.performAndSave({ context in + let object = try context.existingObject(with: media.objectID) + // It's safe to force-unwrap here, since the `object`, if exists, must be a `Media` type. + let mediaInContext = object as! Media + mediaInContext.localThumbnailIdentifier = identifier + }, completion: { (result: Result) in + switch result { + case .success: + onCompletion(export.url) + case .failure: + onCompletion(nil) + } + }, on: exportQueue) } /// Handle the OnError callback and logging any errors encountered. /// - fileprivate func handleExportError(_ error: MediaExportError, errorHandler: OnError?) { + private func handleExportError(_ error: MediaExportError, errorHandler: OnError?) { MediaImportService.logExportError(error) - if let errorHandler = errorHandler { - self.managedObjectContext.perform { - errorHandler(error.toNSError()) - } + exportQueue.async { + errorHandler?(error.toNSError()) } } } diff --git a/WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift b/WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift index 8ceca707da0e..14bdcc39ac8e 100644 --- a/WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift +++ b/WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift @@ -5,12 +5,14 @@ import Gridicons final class AuthenticatedImageDownload: AsyncOperation { let url: URL let blog: Blog + private let callbackQueue: DispatchQueue 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) -> ()) { self.url = url self.blog = blog + self.callbackQueue = callbackQueue self.onSuccess = onSuccess self.onFailure = onFailure } @@ -29,7 +31,7 @@ final class AuthenticatedImageDownload: AsyncOperation { ImageDownloader.shared.downloadImage(for: request) { (image, error) in self.state = .isFinished - DispatchQueue.main.async { + self.callbackQueue.async { guard let image = image else { DDLogError("Unable to download image for attachment with url = \(String(describing: request.url)). Details: \(String(describing: error?.localizedDescription))") if let error = error { @@ -44,11 +46,14 @@ final class AuthenticatedImageDownload: AsyncOperation { self.onSuccess(image) } } - }, + }, onFailure: { error in self.state = .isFinished - self.onFailure(error) - }) + self.callbackQueue.async { + self.onFailure(error) + } + } + ) } } @@ -140,6 +145,7 @@ class EditorMediaUtility { let imageDownload = AuthenticatedImageDownload( url: requestURL, blog: post.blog, + callbackQueue: .main, onSuccess: success, onFailure: failure) diff --git a/WordPress/WordPressTest/MediaPicker/MediaLibraryPickerDataSourceTests.swift b/WordPress/WordPressTest/MediaPicker/MediaLibraryPickerDataSourceTests.swift index 3232eb6fe2f2..b9cbf1b4eb54 100644 --- a/WordPress/WordPressTest/MediaPicker/MediaLibraryPickerDataSourceTests.swift +++ b/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 expect.fulfill() if let _ = error { XCTFail("Media should be created without error")