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.

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

Improve performance of Image and Gallery block processors to avoid long delay when saving a post #22896

Merged
merged 20 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
14d9ddf
Run new Gutenberg processors
fluiddot Mar 14, 2024
231f595
Use content parser in Image block processor
fluiddot Mar 14, 2024
af3c5ad
Use content parser in Gallery block processor
fluiddot Mar 15, 2024
8ba73eb
Use content parser in File block processor
fluiddot Mar 14, 2024
e3e8eb9
Update Gallery block processor
fluiddot Mar 26, 2024
a22f0a5
Update release notes
fluiddot Mar 26, 2024
f73df72
Merge branch 'add/gutenberg-content-parsing' into update/improve-imag…
fluiddot Mar 26, 2024
cbbd09d
Update File block processor
fluiddot Mar 26, 2024
32f122b
Update release notes
fluiddot Mar 26, 2024
f9eadad
Merge branch 'update/improve-image-blocks-processor' into update/impr…
fluiddot Mar 26, 2024
ccffe5b
Merge branch 'trunk' into update/improve-image-blocks-processor
fluiddot May 16, 2024
cd1b26e
Merge remote-tracking branch 'origin/trunk' into update/improve-image…
twstokes May 16, 2024
d3e4077
Update release notes.
twstokes May 16, 2024
527d98d
Merge remote-tracking branch 'origin/update/improve-image-blocks-proc…
twstokes May 16, 2024
5612643
Update release notes.
twstokes May 16, 2024
3a22f64
Update expected content in `testSyncNewDraftWithImageBlock` unit test
fluiddot May 20, 2024
a9c30fb
Merge branch 'update/improve-image-blocks-processor' into update/impr…
fluiddot May 20, 2024
5733b49
Improve performance of File block processor (#22897)
twstokes May 20, 2024
d60c1d1
Use `guard`/`if let` statements to avoid using optionals
fluiddot May 21, 2024
7694cb2
Parse `serverMediaID` to string instead of using interpolation
fluiddot May 21, 2024
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
2 changes: 2 additions & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
-----
* [*] Fix button overlap in the "Post Published" sheet on small devices [#23210]
* [*] [internal] Incorporate a parser to handle Gutenberg blocks more efficiently for improved performance [#22886]
* [**] Improve performance of Image and Gallery block processors to avoid long delay when saving a post [#22896]
* [*] Improve performance of File block processor [#22897]

24.9
-----
Expand Down
37 changes: 22 additions & 15 deletions WordPress/Classes/Services/PostCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -721,26 +721,30 @@ class PostCoordinator: NSObject {
}

// Ensure that all synced media references are up to date
post.media.forEach { media in
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be cautious about making changes to PostCoordinator because the large potion of it is or will be rewritten. prepareToSave is unchanged, so it's a safe change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kean for the call out 🙇 ! This PR mainly focuses on the logic related to processing blocks before saving a post, which requires touching up some bits of the PostCoordinator. I'm planning to modify only necessary here, so hopefully there won't be much conflict with ongoing changes. In any case, I'll be happy to contribute to the new version by applying the same updates.

if media.remoteStatus == .sync {
self.updateReferences(to: media, in: post)
}
}
let syncedMedia = post.media.filter { $0.remoteStatus == .sync }
updateMediaBlocksBeforeSave(in: post, with: syncedMedia)

let uuid = observeMedia(for: post, completion: completion)
trackObserver(receipt: uuid, for: post)

return
} else {
// Ensure that all media references are up to date
post.media.forEach { media in
self.updateReferences(to: media, in: post)
}
updateMediaBlocksBeforeSave(in: post, with: post.media)
}

completion(.success(post))
}

func updateMediaBlocksBeforeSave(in post: AbstractPost, with media: Set<Media>) {
guard let postContent = post.content else {
return
}
let contentParser = GutenbergContentParser(for: postContent)
media.forEach { self.updateReferences(to: $0, in: contentParser.blocks, post: post) }
post.content = contentParser.html()
}

// - warning: deprecated (kahu-offline-mode)
func cancelAnyPendingSaveOf(post: AbstractPost) {
removeObserver(for: post)
Expand Down Expand Up @@ -879,7 +883,7 @@ class PostCoordinator: NSObject {
switch state {
case .ended:
let successHandler = {
self.updateReferences(to: media, in: post)
self.updateMediaBlocksBeforeSave(in: post, with: [media])
if self.isSyncPublishingEnabled {
if post.media.allSatisfy({ $0.remoteStatus == .sync }) {
self.removeObserver(for: post)
Expand Down Expand Up @@ -915,7 +919,7 @@ class PostCoordinator: NSObject {
}, forMediaFor: post)
}

private func updateReferences(to media: Media, in post: AbstractPost) {
private func updateReferences(to media: Media, in contentBlocks: [GutenbergParsedBlock], post: AbstractPost) {
guard var postContent = post.content,
let mediaID = media.mediaID?.intValue,
let remoteURLStr = media.remoteURL else {
Expand All @@ -935,19 +939,21 @@ class PostCoordinator: NSObject {
if media.remoteStatus == .failed {
return
}
var gutenbergProcessors = [Processor]()
var aztecProcessors = [Processor]()

var gutenbergBlockProcessors: [GutenbergProcessor] = []
var gutenbergProcessors: [Processor] = []
Comment on lines +943 to +944
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to update gradually the processors to adopt the new GutenbergContentParser. In the meantime, we'll have two different processors.

var aztecProcessors: [Processor] = []

// File block can upload any kind of media.
let gutenbergFileProcessor = GutenbergFileUploadProcessor(mediaUploadID: gutenbergMediaUploadID, serverMediaID: mediaID, remoteURLString: remoteURLStr)
gutenbergProcessors.append(gutenbergFileProcessor)
gutenbergBlockProcessors.append(gutenbergFileProcessor)

if media.mediaType == .image {
let gutenbergImgPostUploadProcessor = GutenbergImgUploadProcessor(mediaUploadID: gutenbergMediaUploadID, serverMediaID: mediaID, remoteURLString: imageURL)
gutenbergProcessors.append(gutenbergImgPostUploadProcessor)
gutenbergBlockProcessors.append(gutenbergImgPostUploadProcessor)

let gutenbergGalleryPostUploadProcessor = GutenbergGalleryUploadProcessor(mediaUploadID: gutenbergMediaUploadID, serverMediaID: mediaID, remoteURLString: imageURL, mediaLink: mediaLink)
gutenbergProcessors.append(gutenbergGalleryPostUploadProcessor)
gutenbergBlockProcessors.append(gutenbergGalleryPostUploadProcessor)

let imgPostUploadProcessor = ImgUploadProcessor(mediaUploadID: mediaUploadID, remoteURLString: remoteURLStr, width: media.width?.intValue, height: media.height?.intValue)
aztecProcessors.append(imgPostUploadProcessor)
Expand Down Expand Up @@ -980,6 +986,7 @@ class PostCoordinator: NSObject {
}

// Gutenberg processors need to run first because they are more specific/and target only content inside specific blocks
gutenbergBlockProcessors.forEach { $0.process(contentBlocks) }
postContent = gutenbergProcessors.reduce(postContent) { (content, processor) -> String in
return processor.process(content)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import Foundation
import Aztec

class GutenbergFileUploadProcessor: Processor {
class GutenbergFileUploadProcessor: GutenbergProcessor {
private struct FileBlockKeys {
static var name = "wp:file"
static var id = "id"
Expand All @@ -18,38 +17,24 @@ class GutenbergFileUploadProcessor: Processor {
self.remoteURLString = remoteURLString
}

lazy var fileHtmlProcessor = HTMLProcessor(for: "a", replacer: { (file) in
var attributes = file.attributes
func processFileBlocks(_ blocks: [GutenbergParsedBlock]) {
blocks.filter { $0.name == FileBlockKeys.name }.forEach { block in
guard let mediaID = block.attributes[FileBlockKeys.id] as? Int,
mediaID == self.mediaUploadID else {
return
}

attributes.set(.string(self.remoteURLString), forKey: FileBlockKeys.href)
// Update attributes
block.attributes[FileBlockKeys.id] = self.serverMediaID
block.attributes[FileBlockKeys.href] = self.remoteURLString

var html = "<a "
let attributeSerializer = ShortcodeAttributeSerializer()
html += attributeSerializer.serialize(attributes)
html += ">\(file.content ?? "")</a>"
return html
})

lazy var fileBlockProcessor = GutenbergBlockProcessor(for: FileBlockKeys.name, replacer: { fileBlock in
guard let mediaID = fileBlock.attributes[FileBlockKeys.id] as? Int,
mediaID == self.mediaUploadID else {
return nil
}
var block = "<!-- \(FileBlockKeys.name) "
var attributes = fileBlock.attributes
attributes[FileBlockKeys.id] = self.serverMediaID
attributes[FileBlockKeys.href] = self.remoteURLString
if let jsonData = try? JSONSerialization.data(withJSONObject: attributes, options: .sortedKeys),
let jsonString = String(data: jsonData, encoding: .utf8) {
block += jsonString
// Update href of `a` tags
let aTags = try? block.elements.select("a")
aTags?.forEach { _ = try? $0.attr(FileBlockKeys.href, self.remoteURLString) }
}
block += " -->"
block += self.fileHtmlProcessor.process(fileBlock.content)
block += "<!-- /\(FileBlockKeys.name) -->"
return block
})
}

func process(_ text: String) -> String {
return fileBlockProcessor.process(text)
func process(_ blocks: [GutenbergParsedBlock]) {
processFileBlocks(blocks)
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Foundation
import Aztec
import SwiftSoup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aztec was imported in this file to use HTMLProcessor. Now the HTML processing is performed by using SwiftSoup.


class GutenbergGalleryUploadProcessor: Processor {
class GutenbergGalleryUploadProcessor: GutenbergProcessor {

let mediaUploadID: Int32
let remoteURLString: String
Expand All @@ -28,76 +28,67 @@ class GutenbergGalleryUploadProcessor: Processor {
static let dataLink = "data-link"
}

lazy var imgPostMediaUploadProcessor = HTMLProcessor(for: ImageKeys.name, replacer: { (img) in
guard let imgClassAttributeValue = img.attributes[ImageKeys.classAttributes]?.value,
case let .string(imgClass) = imgClassAttributeValue else {
return nil
}
func processImgPostMediaUpload(_ element: Element) {
let imgTags = try? element.select(ImageKeys.name)
imgTags?.forEach {imgTag in
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: Could be in an if let, or could guard and return early to remove the optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied this suggestion in d60c1d1.

guard let imgClass = try? imgTag.attr(ImageKeys.classAttributes) else {
return
}

let classAttributes = imgClass.components(separatedBy: " ")
let classAttributes = imgClass.components(separatedBy: " ")

guard let imageIDAttribute = classAttributes.filter({ (value) -> Bool in
value.hasPrefix(GutenbergImgUploadProcessor.imgClassIDPrefixAttribute)
}).first else {
return nil
}
guard let imageIDAttribute = classAttributes.filter({ (value) -> Bool in
value.hasPrefix(GutenbergImgUploadProcessor.imgClassIDPrefixAttribute)
}).first else {
return
}

let imageIDString = String(imageIDAttribute.dropFirst(ImageKeys.classIDPrefix.count))
let imgUploadID = Int32(imageIDString)
let imageIDString = String(imageIDAttribute.dropFirst(ImageKeys.classIDPrefix.count))
let imgUploadID = Int32(imageIDString)

guard imgUploadID == self.mediaUploadID else {
return nil
}
guard imgUploadID == self.mediaUploadID else {
return
}

let newImgClassAttributes = imgClass.replacingOccurrences(of: imageIDAttribute, with: ImageKeys.classIDPrefix + String(self.serverMediaID))
let newImgClassAttributes = imgClass.replacingOccurrences(of: imageIDAttribute, with: ImageKeys.classIDPrefix + String(self.serverMediaID))

var attributes = img.attributes
attributes.set(.string(self.remoteURLString), forKey: "src")
attributes.set(.string(newImgClassAttributes), forKey: "class")
attributes.set(.string("\(self.serverMediaID)"), forKey: ImageKeys.dataID)
attributes.set(.string(self.remoteURLString), forKey: ImageKeys.dataFullURL)
if attributes.contains(where: { $0.key == ImageKeys.dataLink } ) {
attributes.set(.string(self.mediaLink), forKey: ImageKeys.dataLink)
}
_ = try? imgTag.attr("src", self.remoteURLString)
_ = try? imgTag.attr("class", newImgClassAttributes)
_ = try? imgTag.attr(ImageKeys.dataID, "\(self.serverMediaID)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit and more of a personal pref, but it may be more common to convert the Int -> String via String(self.serverMediaID) if you're not interpolating anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied this suggestion in 7694cb2.

_ = try? imgTag.attr(ImageKeys.dataFullURL, self.remoteURLString)

var html = "<\(ImageKeys.name) "
let attributeSerializer = ShortcodeAttributeSerializer()
html += attributeSerializer.serialize(attributes)
html += " />"
return html
})
let dataLinkAttribute = try? imgTag.attr(ImageKeys.dataLink)
if dataLinkAttribute != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about adding an if let or guarding to remove the optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied this suggestion in d60c1d1.

_ = try? imgTag.attr(ImageKeys.dataLink, self.mediaLink)
}
}
}

private struct LinkKeys {
static let name = "a"
}

lazy var linkPostMediaUploadProcessor = HTMLProcessor(for: LinkKeys.name, replacer: { (link) in

guard let linkOriginalContent = link.content else {
return nil
}
func processLinkPostMediaUpload(_ block: GutenbergParsedBlock) {
let aTags = try? block.elements.select(LinkKeys.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about adding an if let or guarding to remove the optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied this suggestion in d60c1d1.

aTags?.forEach { aTag in
guard let linkOriginalContent = try? aTag.html() else {
return
}

let linkUpdatedContent = self.imgPostMediaUploadProcessor.process(linkOriginalContent)
processImgPostMediaUpload(aTag)
let linkUpdatedContent = try? aTag.html()

guard linkUpdatedContent != linkOriginalContent else {
return nil
}
guard linkUpdatedContent != linkOriginalContent else {
return
}

var attributes = link.attributes
if let linkToURL = self.linkToURL {
attributes.set(.string(linkToURL), forKey: "href")
} else {
attributes.set(.string(self.remoteURLString), forKey: "href")
if let linkToURL = self.linkToURL {
_ = try? aTag.attr("href", linkToURL)
} else {
_ = try? aTag.attr("href", self.remoteURLString)
}
}

var html = "<\(LinkKeys.name) "
let attributeSerializer = ShortcodeAttributeSerializer()
html += attributeSerializer.serialize(attributes)
html += " >"
html += linkUpdatedContent
html += "</\(LinkKeys.name)>"
return html
})
}

private struct GalleryBlockKeys {
static let name = "wp:gallery"
Expand All @@ -117,42 +108,35 @@ class GutenbergGalleryUploadProcessor: Processor {
return ids
}

lazy var galleryBlockProcessor = GutenbergBlockProcessor(for: GalleryBlockKeys.name, replacer: { block in
guard let idsAny = block.attributes[GalleryBlockKeys.ids] as? [Any] else {
return nil
}
var ids = self.convertToIntArray(idsAny)
guard ids.contains(self.mediaUploadID) else {
return nil
}
var updatedBlock = "<!-- \(GalleryBlockKeys.name) "
var attributes = block.attributes
if let index = ids.firstIndex(of: self.mediaUploadID ) {
ids[index] = Int32(self.serverMediaID)
}
attributes[GalleryBlockKeys.ids] = ids

if let jsonData = try? JSONSerialization.data(withJSONObject: attributes, options: .sortedKeys),
let jsonString = String(data: jsonData, encoding: .utf8) {
updatedBlock += jsonString
}
updatedBlock += " -->"
if let linkTo = block.attributes[GalleryBlockKeys.linkTo] as? String, linkTo != "none" {
if linkTo == "file" {
self.linkToURL = self.remoteURLString
} else if linkTo == "post" {
self.linkToURL = self.mediaLink
func processGalleryBlocks(_ blocks: [GutenbergParsedBlock]) {
let galleryBlocks = blocks.filter { $0.name == GalleryBlockKeys.name }
galleryBlocks.forEach { block in
guard let idsAny = block.attributes[GalleryBlockKeys.ids] as? [Any] else {
return
}
var ids = self.convertToIntArray(idsAny)
guard ids.contains(self.mediaUploadID) else {
return
}
if let index = ids.firstIndex(of: self.mediaUploadID ) {
ids[index] = Int32(self.serverMediaID)
}
block.attributes[GalleryBlockKeys.ids] = ids

if let linkTo = block.attributes[GalleryBlockKeys.linkTo] as? String, linkTo != "none" {
if linkTo == "file" {
self.linkToURL = self.remoteURLString
} else if linkTo == "post" {
self.linkToURL = self.mediaLink
}
processLinkPostMediaUpload(block)
} else {
block.elements.forEach { processImgPostMediaUpload($0) }
}
updatedBlock += self.linkPostMediaUploadProcessor.process(block.content)
} else {
updatedBlock += self.imgPostMediaUploadProcessor.process(block.content)
}
updatedBlock += "<!-- /\(GalleryBlockKeys.name) -->"
return updatedBlock
})
}

func process(_ text: String) -> String {
let result = galleryBlockProcessor.process(text)
return result
func process(_ blocks: [GutenbergParsedBlock]) {
processGalleryBlocks(blocks)
}
}
Loading