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.

Sign up for GitHub

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

Move uploading media to WP Media Library to MediaImportService #20322

Merged
merged 12 commits into from
Mar 24, 2023
Merged

Move uploading media to WP Media Library to MediaImportService #20322

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
371104a
Use `CoreDataStack` in `MediaImportService`
crazytonyli Mar 10, 2023
0960761
Make uploading media function thread-safe
crazytonyli Mar 14, 2023
1d25d16
Remove the `allowableFileExtensions` instance variable
crazytonyli Mar 14, 2023
65fd4e4
Remove an unnecessary local variable
crazytonyli Mar 22, 2023
4d4fd8a
Capture a weak reference to self
crazytonyli Mar 22, 2023
df3eef8
Update API docs
crazytonyli Mar 22, 2023
cce1004
Remove an unnecessary local variable
crazytonyli Mar 22, 2023
65b7249
Add a link to the issue described in the comments
crazytonyli Mar 22, 2023
e8b02b8
Pass the `Media` instance to the closure upon import failure
crazytonyli Mar 22, 2023
9ec2568
Merge branch 'trunk' into tonyli-refactor-media-import-service
crazytonyli Mar 24, 2023
f6cc6e4
Update release notes
crazytonyli Mar 24, 2023
0191a21
Remove a space between Word and Press
mokagio Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 82 additions & 52 deletions WordPress/Classes/Services/MediaCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,8 @@ class MediaCoordinator: NSObject {
/// - parameter blog: The blog that the asset should be added to.
/// - parameter origin: The location in the app where the upload was initiated (optional).
///
@discardableResult
func addMedia(from asset: ExportableAsset, to blog: Blog, analyticsInfo: MediaAnalyticsInfo? = nil) -> Media? {
let coordinator = mediaLibraryProgressCoordinator
return addMedia(from: asset, blog: blog, post: nil, coordinator: coordinator, analyticsInfo: analyticsInfo)
func addMedia(from asset: ExportableAsset, to blog: Blog, analyticsInfo: MediaAnalyticsInfo? = nil) {
addMedia(from: asset, blog: blog, post: nil, coordinator: mediaLibraryProgressCoordinator, analyticsInfo: analyticsInfo)
}

/// Adds the specified media asset to the specified post. The upload process
Expand All @@ -153,57 +151,88 @@ class MediaCoordinator: NSObject {
///
@discardableResult
func addMedia(from asset: ExportableAsset, to post: AbstractPost, analyticsInfo: MediaAnalyticsInfo? = nil) -> Media? {
let coordinator = self.coordinator(for: post)
return addMedia(from: asset, blog: post.blog, post: post, coordinator: coordinator, analyticsInfo: analyticsInfo)
addMedia(from: asset, post: post, coordinator: coordinator(for: post), analyticsInfo: analyticsInfo)
}

@discardableResult
private func addMedia(from asset: ExportableAsset, blog: Blog, post: AbstractPost?, coordinator: MediaProgressCoordinator, analyticsInfo: MediaAnalyticsInfo? = nil) -> Media? {
/// Create a `Media` instance from the main context and upload the asset to the Meida Library.
///
/// - Warning: This function must be called from the main thread.
///
/// - SeeAlso: `MediaImportService.createMedia(with:blog:post:thumbnailCallback:completion:)`
private func addMedia(from asset: ExportableAsset, post: AbstractPost, coordinator: MediaProgressCoordinator, analyticsInfo: MediaAnalyticsInfo? = nil) -> Media? {
coordinator.track(numberOfItems: 1)
let service = mediaServiceFactory.create(mainContext)
let service = MediaImportService(coreDataStack: coreDataStack)
let totalProgress = Progress.discreteProgress(totalUnitCount: MediaExportProgressUnits.done)
var creationProgress: Progress? = nil
let mediaOptional = service.createMedia(with: asset,
blog: blog,
post: post,
progress: &creationProgress,
thumbnailCallback: { [weak self] media, url in
self?.thumbnailReady(url: url, for: media)
},
completion: { [weak self] media, error in
guard let strongSelf = self else {
return
}
if let error = error as NSError? {
if let media = media {
coordinator.attach(error: error as NSError, toMediaID: media.uploadID)
strongSelf.fail(error as NSError, media: media)
} else {
// If there was an error and we don't have a media object we just say to the coordinator that one item was finished
coordinator.finishOneItem()
}
return
}
guard let media = media, !media.isDeleted else {
return
}

strongSelf.trackUploadOf(media, analyticsInfo: analyticsInfo)

let uploadProgress = strongSelf.uploadMedia(media)
totalProgress.addChild(uploadProgress, withPendingUnitCount: MediaExportProgressUnits.threeQuartersDone)
})
guard let media = mediaOptional else {
let result = service.createMedia(
with: asset,
blog: post.blog,
post: post,
thumbnailCallback: { [weak self] media, url in
self?.thumbnailReady(url: url, for: media)
},
completion: { [weak self] media, error in
self?.handleMediaImportResult(coordinator: coordinator, totalProgress: totalProgress, analyticsInfo: analyticsInfo, media: media, error: error)
}
)
guard let (media, creationProgress) = result else {
return nil
}

processing(media)
if let creationProgress = creationProgress {
totalProgress.addChild(creationProgress, withPendingUnitCount: MediaExportProgressUnits.quarterDone)
coordinator.track(progress: totalProgress, of: media, withIdentifier: media.uploadID)
}

totalProgress.addChild(creationProgress, withPendingUnitCount: MediaExportProgressUnits.quarterDone)
coordinator.track(progress: totalProgress, of: media, withIdentifier: media.uploadID)

return media
}

/// Create a `Media` instance and upload the asset to the Meida Library.
///
/// - SeeAlso: `MediaImportService.createMedia(with:blog:post:receiveUpdate:thumbnailCallback:completion:)`
private func addMedia(from asset: ExportableAsset, blog: Blog, post: AbstractPost?, coordinator: MediaProgressCoordinator, analyticsInfo: MediaAnalyticsInfo? = nil) {
coordinator.track(numberOfItems: 1)
let service = MediaImportService(coreDataStack: coreDataStack)
let totalProgress = Progress.discreteProgress(totalUnitCount: MediaExportProgressUnits.done)
let creationProgress = service.createMedia(
with: asset,
blog: blog,
post: post,
receiveUpdate: { [weak self] media in
self?.processing(media)
coordinator.track(progress: totalProgress, of: media, withIdentifier: media.uploadID)
},
thumbnailCallback: { [weak self] media, url in
self?.thumbnailReady(url: url, for: media)
},
completion: { [weak self] media, error in
self?.handleMediaImportResult(coordinator: coordinator, totalProgress: totalProgress, analyticsInfo: analyticsInfo, media: media, error: error)
}
)

totalProgress.addChild(creationProgress, withPendingUnitCount: MediaExportProgressUnits.quarterDone)
}

private func handleMediaImportResult(coordinator: MediaProgressCoordinator, totalProgress: Progress, analyticsInfo: MediaAnalyticsInfo?, media: Media?, error: Error?) -> Void {
if let error = error as NSError? {
if let media = media {
Comment on lines +215 to +217
Copy link
Contributor

Choose a reason for hiding this comment

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

As soon as I saw that media, error pair in the code about my mind screamed: Use Result! But here I see that it wouldn't work properly in we can have both an error and a media 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I now noticed there are a couple places I should have passed the Media instance along with the error, which are updated in e8b02b8.

coordinator.attach(error: error as NSError, toMediaID: media.uploadID)
fail(error as NSError, media: media)
} else {
// If there was an error and we don't have a media object we just say to the coordinator that one item was finished
coordinator.finishOneItem()
}
return
}
guard let media = media, !media.isDeleted else {
return
}

trackUploadOf(media, analyticsInfo: analyticsInfo)

let uploadProgress = uploadMedia(media)
totalProgress.addChild(uploadProgress, withPendingUnitCount: MediaExportProgressUnits.threeQuartersDone)
}

/// Retry the upload of a media object that previously has failed.
///
/// - Parameters:
Expand Down Expand Up @@ -330,13 +359,14 @@ class MediaCoordinator: NSObject {
self.fail(nserror, media: media)
}

coreDataStack.performAndSave { context in
let service = self.mediaServiceFactory.create(context)
var progress: Progress? = nil
service.uploadMedia(media, automatedRetry: automatedRetry, progress: &progress, success: success, failure: failure)
if let progress {
resultProgress.addChild(progress, withPendingUnitCount: resultProgress.totalUnitCount)
}
// For some reason, this `MediaService` instance has to be created with the main context, otherwise
// the successfully uploaded media is shown as a "local" assets incorrectly (see the issue comment linked below).
// https://github.com/wordpress-mobile/WordPress-iOS/issues/20298#issuecomment-1465319707
let service = self.mediaServiceFactory.create(coreDataStack.mainContext)
crazytonyli marked this conversation as resolved.
Show resolved Hide resolved
var progress: Progress? = nil
service.uploadMedia(media, automatedRetry: automatedRetry, progress: &progress, success: success, failure: failure)
if let progress {
resultProgress.addChild(progress, withPendingUnitCount: resultProgress.totalUnitCount)
}

uploading(media, progress: resultProgress)
Expand Down
Loading