From e6d4a0cd603393e47dc7139ee33fc73f3c586ecf Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:55:20 -0600 Subject: [PATCH 1/6] Make TextBundleWrapper init nullable and fail on non-UTF-8 text --- Modules/Sources/TextBundle/TextBundleWrapper.m | 12 +++++++++--- .../Sources/TextBundle/include/TextBundleWrapper.h | 6 +++--- .../Sources/Services/ShareExtractor.swift | 5 +++-- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/Modules/Sources/TextBundle/TextBundleWrapper.m b/Modules/Sources/TextBundle/TextBundleWrapper.m index 2ee06e42b01d..301eb58931aa 100644 --- a/Modules/Sources/TextBundle/TextBundleWrapper.m +++ b/Modules/Sources/TextBundle/TextBundleWrapper.m @@ -50,7 +50,7 @@ - (instancetype)init return self; } -- (instancetype)initWithContentsOfURL:(NSURL *)url options:(NSFileWrapperReadingOptions)options error:(NSError **)error +- (nullable instancetype)initWithContentsOfURL:(NSURL *)url options:(NSFileWrapperReadingOptions)options error:(NSError **)error { self = [self init]; if (self) { @@ -63,7 +63,7 @@ - (instancetype)initWithContentsOfURL:(NSURL *)url options:(NSFileWrapperReading return self; } -- (instancetype)initWithFileWrapper:(NSFileWrapper *)fileWrapper error:(NSError **)error +- (nullable instancetype)initWithFileWrapper:(NSFileWrapper *)fileWrapper error:(NSError **)error { self = [self init]; if (self) { @@ -158,6 +158,12 @@ - (BOOL)readFromFilewrapper:(NSFileWrapper *)textBundleFileWrapper error:(NSErro NSFileWrapper *textFileWrapper = [[textBundleFileWrapper fileWrappers] objectForKey:[self textFileNameInFileWrapper:textBundleFileWrapper]]; if (textFileWrapper) { self.text = [[NSString alloc] initWithData:textFileWrapper.regularFileContents encoding:NSUTF8StringEncoding]; + if (self.text == nil) { + if (error) { + *error = [NSError errorWithDomain:TextBundleErrorDomain code:TextBundleErrorInvalidFormat userInfo:nil]; + } + return NO; + } } else { if (error) { @@ -201,7 +207,7 @@ - (NSString *)textFilenameForType:(NSString *)type #pragma mark - Assets -- (NSFileWrapper *)fileWrapperForAssetFilename:(NSString *)filename +- (nullable NSFileWrapper *)fileWrapperForAssetFilename:(NSString *)filename { __block NSFileWrapper *fileWrapper = nil; [[self.assetsFileWrapper fileWrappers] enumerateKeysAndObjectsUsingBlock:^(NSString * _Nonnull __unused key, NSFileWrapper * _Nonnull __unused obj, BOOL * _Nonnull __unused stop) { diff --git a/Modules/Sources/TextBundle/include/TextBundleWrapper.h b/Modules/Sources/TextBundle/include/TextBundleWrapper.h index 1cfdcf6c1cbb..3e63e70a1505 100644 --- a/Modules/Sources/TextBundle/include/TextBundleWrapper.h +++ b/Modules/Sources/TextBundle/include/TextBundleWrapper.h @@ -89,7 +89,7 @@ typedef NS_ENUM(NSInteger, TextBundleError) @param error If an error occurs, upon return contains an NSError object that describes the problem. Pass NULL if you do not want error information. @return A new TextBundleWrapper for the content at url. */ -- (instancetype)initWithContentsOfURL:(NSURL *)url options:(NSFileWrapperReadingOptions)options error:(NSError **)error; +- (nullable instancetype)initWithContentsOfURL:(NSURL *)url options:(NSFileWrapperReadingOptions)options error:(NSError **)error; /** @@ -99,7 +99,7 @@ typedef NS_ENUM(NSInteger, TextBundleError) @param error If an error occurs, upon return contains an NSError object that describes the problem. Pass NULL if you do not want error information. @return A new TextBundleWrapper for the content of the fileWrapper. */ -- (instancetype)initWithFileWrapper:(NSFileWrapper *)fileWrapper error:(NSError **)error; +- (nullable instancetype)initWithFileWrapper:(NSFileWrapper *)fileWrapper error:(NSError **)error; /** @@ -122,7 +122,7 @@ typedef NS_ENUM(NSInteger, TextBundleError) @param filename A filename in the asset/ folder @return A NSFilewrapper represeting filename or nil it the file doesn't exist */ -- (NSFileWrapper *)fileWrapperForAssetFilename:(NSString *)filename; +- (nullable NSFileWrapper *)fileWrapperForAssetFilename:(NSString *)filename; diff --git a/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift b/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift index 4ce83d708356..5c1c8617bde1 100644 --- a/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift +++ b/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift @@ -398,8 +398,9 @@ private struct URLExtractor: TypeBasedExtensionContentExtractor { } private func handleTextBundle(url: URL) -> ExtractedItem? { - var error: NSError? - let bundleWrapper = TextBundleWrapper(contentsOf: url, options: .immediate, error: &error) + guard let bundleWrapper = try? TextBundleWrapper(contentsOf: url, options: .immediate) else { + return nil + } var returnedItem = ExtractedItem() var cachedImages = [String: ExtractedImage]() From fb540cd1d53a4bf42ea9c21502b26c6ed917065d Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:42:50 -0600 Subject: [PATCH 2/6] Add TextBundleWrapper nullability regression tests --- Modules/Package.swift | 1 + .../TextBundleWrapperTests.swift | 86 +++++++++++++++++++ Package.swift | 5 ++ 3 files changed, 92 insertions(+) create mode 100644 Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift diff --git a/Modules/Package.swift b/Modules/Package.swift index 46aba57d1de7..a24dede30f69 100644 --- a/Modules/Package.swift +++ b/Modules/Package.swift @@ -20,6 +20,7 @@ let package = Package( .library(name: "ShareExtensionCore", targets: ["ShareExtensionCore"]), .library(name: "SFHFKeychainUtils", targets: ["SFHFKeychainUtils"]), .library(name: "Support", targets: ["Support"]), + .library(name: "TextBundle", targets: ["TextBundle"]), .library(name: "WordPressFlux", targets: ["WordPressFlux"]), .library(name: "WordPressShared", targets: ["WordPressShared"]), .library(name: "WordPressUI", targets: ["WordPressUI"]), diff --git a/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift b/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift new file mode 100644 index 000000000000..821a1e10fcc4 --- /dev/null +++ b/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift @@ -0,0 +1,86 @@ +import Foundation +import Testing + +import TextBundle + +/// Regression tests for the nullability hardening in `TextBundleWrapper`: +/// a read failure must surface as a thrown error (the initializer is now `nullable`, +/// so it bridges to a throwing Swift initializer) rather than a nil-holding instance, +/// and a text file that isn't valid UTF-8 must fail the read instead of leaving +/// the `nonnull` `text` property nil. +struct TextBundleWrapperTests { + + // MARK: Helpers + + /// Writes a `.textbundle` directory to a unique temporary location and returns its URL. + /// Pass `nil` for `info` or `textFileName`/`textData` to omit that member. + private func makeBundle(info: Data?, textFileName: String?, textData: Data?) throws -> URL { + let root = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true) + .appendingPathComponent("TextBundleTests-\(UUID().uuidString)", isDirectory: true) + let bundle = root.appendingPathComponent("document.textbundle", isDirectory: true) + try FileManager.default.createDirectory(at: bundle, withIntermediateDirectories: true) + if let info { + try info.write(to: bundle.appendingPathComponent("info.json")) + } + if let textFileName, let textData { + try textData.write(to: bundle.appendingPathComponent(textFileName)) + } + return bundle + } + + private var validInfoJSON: Data { + Data(#"{"version":2,"type":"net.daringfireball.markdown"}"#.utf8) + } + + // MARK: Happy path + + @Test func validBundleLoadsText() throws { + let url = try makeBundle(info: validInfoJSON, textFileName: "text.markdown", textData: Data("# Hello".utf8)) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + let wrapper = try TextBundleWrapper(contentsOf: url, options: .immediate) + #expect(wrapper.text == "# Hello") + #expect(wrapper.type == kUTTypeMarkdown) + } + + // MARK: Finding 1 — read failures must throw (nullable initializer) + + @Test func missingTextFileThrows() throws { + let url = try makeBundle(info: validInfoJSON, textFileName: nil, textData: nil) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + #expect(throws: (any Error).self) { + _ = try TextBundleWrapper(contentsOf: url, options: .immediate) + } + } + + @Test func missingInfoJSONThrows() throws { + let url = try makeBundle(info: nil, textFileName: "text.markdown", textData: Data("hi".utf8)) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + #expect(throws: (any Error).self) { + _ = try TextBundleWrapper(contentsOf: url, options: .immediate) + } + } + + @Test func unreadableURLThrows() { + let url = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true) + .appendingPathComponent("does-not-exist-\(UUID().uuidString).textbundle", isDirectory: true) + + #expect(throws: (any Error).self) { + _ = try TextBundleWrapper(contentsOf: url, options: .immediate) + } + } + + // MARK: Finding 2 — non-UTF-8 text must fail the read, not leave `text` nil + + @Test func nonUTF8TextThrows() throws { + // 0xFF is never a valid UTF-8 byte, so NSString decoding returns nil. + let url = try makeBundle(info: validInfoJSON, textFileName: "text.markdown", textData: Data([0xFF, 0xFE, 0xFF])) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + #expect(throws: (any Error).self) { + _ = try TextBundleWrapper(contentsOf: url, options: .immediate) + } + } +} diff --git a/Package.swift b/Package.swift index 6a9397e7670e..727ba047a4e4 100644 --- a/Package.swift +++ b/Package.swift @@ -56,6 +56,11 @@ let package = Package( ], path: "Modules/Tests/GutenbergProcessorsTests", swiftSettings: [.swiftLanguageMode(.v5)] + ), + .testTarget( + name: "TextBundleTests", + dependencies: [.product(name: "TextBundle", package: "Modules")], + path: "Modules/Tests/TextBundleTests" ) ] ) From 31946723b27df45e77e2cce5d273b0162f65e07a Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:13:21 -0600 Subject: [PATCH 3/6] Add TextBundleWrapper tests for assets, type, and malformed info.json --- .../TextBundleWrapperTests.swift | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift b/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift index 821a1e10fcc4..e070ecc574f7 100644 --- a/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift +++ b/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift @@ -83,4 +83,44 @@ struct TextBundleWrapperTests { _ = try TextBundleWrapper(contentsOf: url, options: .immediate) } } + + // MARK: Assets + + @Test func assetsAreLoadedAndLookedUpByFilename() throws { + let url = try makeBundle(info: validInfoJSON, textFileName: "text.markdown", textData: Data("# Hi".utf8)) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + let assets = url.appendingPathComponent("assets", isDirectory: true) + try FileManager.default.createDirectory(at: assets, withIntermediateDirectories: true) + try Data([0x89, 0x50, 0x4E, 0x47]).write(to: assets.appendingPathComponent("foo.png")) + + let wrapper = try TextBundleWrapper(contentsOf: url, options: .immediate) + #expect(wrapper.assetsFileWrapper.fileWrappers?["foo.png"] != nil) + #expect(wrapper.fileWrapper(forAssetFilename: "foo.png") != nil) + #expect(wrapper.fileWrapper(forAssetFilename: "missing.png") == nil) + } + + // MARK: Metadata + + @Test func nonMarkdownTypeIsReported() throws { + let info = Data(#"{"version":2,"type":"public.plain-text"}"#.utf8) + let url = try makeBundle(info: info, textFileName: "text.txt", textData: Data("hi".utf8)) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + let wrapper = try TextBundleWrapper(contentsOf: url, options: .immediate) + #expect(wrapper.type == "public.plain-text") + #expect(wrapper.type != kUTTypeMarkdown) + } + + @Test func invalidInfoJSONThrows() throws { + let url = try makeBundle( + info: Data("{ not valid json".utf8), + textFileName: "text.markdown", + textData: Data("hi".utf8) + ) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + #expect(throws: (any Error).self) { + _ = try TextBundleWrapper(contentsOf: url, options: .immediate) + } + } } From 0fcf7c461c2e323928a1026de7a7ec02415f551c Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:23:57 -0600 Subject: [PATCH 4/6] Guard against non-dictionary info.json in TextBundleWrapper --- Modules/Sources/TextBundle/TextBundleWrapper.m | 9 ++++++++- .../TextBundleTests/TextBundleWrapperTests.swift | 11 +++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/Modules/Sources/TextBundle/TextBundleWrapper.m b/Modules/Sources/TextBundle/TextBundleWrapper.m index 301eb58931aa..8eeef4e8369f 100644 --- a/Modules/Sources/TextBundle/TextBundleWrapper.m +++ b/Modules/Sources/TextBundle/TextBundleWrapper.m @@ -134,7 +134,14 @@ - (BOOL)readFromFilewrapper:(NSFileWrapper *)textBundleFileWrapper error:(NSErro if (error) { *error = jsonReadError; } return NO; } - + + if (![jsonObject isKindOfClass:[NSDictionary class]]) { + if (error) { + *error = [NSError errorWithDomain:TextBundleErrorDomain code:TextBundleErrorInvalidFormat userInfo:nil]; + } + return NO; + } + self.metadata = [jsonObject mutableCopy]; self.version = self.metadata[kTextBundleVersion]; self.type = self.metadata[kTextBundleType]; diff --git a/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift b/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift index e070ecc574f7..7601469042e6 100644 --- a/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift +++ b/Modules/Tests/TextBundleTests/TextBundleWrapperTests.swift @@ -123,4 +123,15 @@ struct TextBundleWrapperTests { _ = try TextBundleWrapper(contentsOf: url, options: .immediate) } } + + @Test func nonDictionaryInfoJSONThrows() throws { + // Valid JSON but a top-level array (not an object): must fail the read + // instead of crashing on -[NSArray objectForKeyedSubscript:]. + let url = try makeBundle(info: Data("[1,2,3]".utf8), textFileName: "text.markdown", textData: Data("hi".utf8)) + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + #expect(throws: (any Error).self) { + _ = try TextBundleWrapper(contentsOf: url, options: .immediate) + } + } } From fa835298e2e55640e4311dc37e2a052f34e711c2 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:55:43 -0600 Subject: [PATCH 5/6] Propagate TextBundle read errors through the share extractor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that the TextBundleWrapper initializer bridges to a throwing Swift initializer, surface its read failures instead of swallowing them with try?. Make TypeBasedExtensionContentExtractor.convert(payload:) throwing and thread the error up through handleTextBundle, handleTextPack, processLocalFile, and URLExtractor.convert. The loadItem completion — which already receives an Error — logs both load and conversion failures and balances the dispatch group via defer. --- .../Sources/Services/ShareExtractor.swift | 43 +++++++++++-------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift b/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift index 5c1c8617bde1..24b3d528e165 100644 --- a/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift +++ b/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift @@ -231,7 +231,7 @@ private protocol ExtensionContentExtractor { private protocol TypeBasedExtensionContentExtractor: ExtensionContentExtractor, Sendable { associatedtype Payload var acceptedType: String { get } - func convert(payload: Payload) -> ExtractedItem? + func convert(payload: Payload) throws -> ExtractedItem? } private extension TypeBasedExtensionContentExtractor { @@ -263,13 +263,22 @@ private extension TypeBasedExtensionContentExtractor { for provider in itemProviders { syncGroup.enter() // Remember, this is an async call.... - provider.loadItem(forTypeIdentifier: acceptedType, options: nil) { payload, _ in - let payload = payload as? Payload - let result = payload.flatMap(self.convert(payload:)) - if let result { - results.append(result) + provider.loadItem(forTypeIdentifier: acceptedType, options: nil) { payload, error in + defer { syncGroup.leave() } + if let error { + DDLogError("Failed to load shared item of type \(self.acceptedType): \(error.localizedDescription)") + return + } + guard let payload = payload as? Payload else { + return + } + do { + if let result = try self.convert(payload: payload) { + results.append(result) + } + } catch { + DDLogError("Failed to extract shared item of type \(self.acceptedType): \(error.localizedDescription)") } - syncGroup.leave() } } @@ -340,9 +349,9 @@ private struct URLExtractor: TypeBasedExtensionContentExtractor { typealias Payload = URL let acceptedType = UTType.url.identifier - func convert(payload: URL) -> ExtractedItem? { + func convert(payload: URL) throws -> ExtractedItem? { guard !payload.isFileURL else { - return processLocalFile(url: payload) + return try processLocalFile(url: payload) } var returnedItem = ExtractedItem() @@ -351,12 +360,12 @@ private struct URLExtractor: TypeBasedExtensionContentExtractor { return returnedItem } - private func processLocalFile(url: URL) -> ExtractedItem? { + private func processLocalFile(url: URL) throws -> ExtractedItem? { switch url.pathExtension { case "textbundle": - return handleTextBundle(url: url) + return try handleTextBundle(url: url) case "textpack": - return handleTextPack(url: url) + return try handleTextPack(url: url) case "text", "txt": return handlePlainTextFile(url: url) case "md", "markdown": @@ -366,7 +375,7 @@ private struct URLExtractor: TypeBasedExtensionContentExtractor { } } - private func handleTextPack(url: URL) -> ExtractedItem? { + private func handleTextPack(url: URL) throws -> ExtractedItem? { let fileManager = FileManager() guard let temporaryDirectoryURL = try? FileManager.default.url(for: .itemReplacementDirectory, in: .userDomainMask, @@ -394,13 +403,11 @@ private struct URLExtractor: TypeBasedExtensionContentExtractor { return nil } - return handleTextBundle(url: textBundleURL) + return try handleTextBundle(url: textBundleURL) } - private func handleTextBundle(url: URL) -> ExtractedItem? { - guard let bundleWrapper = try? TextBundleWrapper(contentsOf: url, options: .immediate) else { - return nil - } + private func handleTextBundle(url: URL) throws -> ExtractedItem? { + let bundleWrapper = try TextBundleWrapper(contentsOf: url, options: .immediate) var returnedItem = ExtractedItem() var cachedImages = [String: ExtractedImage]() From 19e22d7c416220d8e80bd2e29106e50b02c6b36c Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:39:52 -0600 Subject: [PATCH 6/6] Surface share-extraction failures instead of loading blindly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadShare now returns a ShareLoadOutcome — the assembled share, the attachments that were skipped, and whether anything usable was extracted. extract() yields (items, failures) per provider, accumulated under a lock since loadItem calls back concurrently. The share and draft extensions act on it: cancel the request when nothing extracted and something errored, show a non-blocking notice on a partial failure, and open the editor as before when the share was merely empty. --- .../Sources/Services/ShareExtractor.swift | 111 ++++++++++++------ ...ShareExtensionAbstractViewController.swift | 52 ++++++++ .../ShareExtensionEditorViewController.swift | 31 +++-- .../UI/ShareModularViewController.swift | 12 +- 4 files changed, 162 insertions(+), 44 deletions(-) diff --git a/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift b/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift index 24b3d528e165..b2d86295ce8e 100644 --- a/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift +++ b/WordPress/WordPressShareExtension/Sources/Services/ShareExtractor.swift @@ -64,6 +64,24 @@ struct ExtractedImage { var insertionState: InsertionState } +/// The outcome of loading shared content: the assembled share, any per-attachment +/// failures that were skipped, and whether any usable content was extracted at all. +struct ShareLoadOutcome { + let share: ExtractedShare + let failures: [Error] + + /// `true` when at least one attachment yielded content (text or an image). + let didExtractContent: Bool + + /// `true` only when nothing usable was extracted *and* at least one attachment errored. + /// + /// A share that was simply empty — an empty text selection, say — has no content and no + /// failures. That is not a failure: it should still open the editor, as it did before. + var extractionDidFail: Bool { + !didExtractContent && !failures.isEmpty + } +} + /// Extracts valid information from an extension context. /// struct ShareExtractor { @@ -73,15 +91,17 @@ struct ShareExtractor { self.extensionContext = extensionContext } - /// Loads the content asynchronously. + /// Loads the shared content asynchronously. + /// + /// Unlike the extractors it drives, this always calls `completion` — even when nothing + /// could be read. Inspect `ShareLoadOutcome.didExtractContent` to decide whether there's + /// anything to present, and `failures` for the attachments that were skipped. /// - /// - Important: This method will only call completion if it can successfully extract content. - /// - Parameters: - /// - completion: the block to be called when the extractor has obtained content. + /// - Parameter completion: called with the assembled `ShareLoadOutcome`. /// - func loadShare(completion: @escaping (ExtractedShare) -> Void) { - extractText { extractedTextResults in - self.extractImages { extractedImages in + func loadShare(completion: @escaping (ShareLoadOutcome) -> Void) { + extractText { extractedTextResults, textFailures in + self.extractImages { extractedImages, imageFailures in let title = extractedTextResults?.title ?? "" let description = extractedTextResults?.description ?? "" let selectedText = extractedTextResults?.selectedText ?? "" @@ -92,12 +112,20 @@ struct ShareExtractor { returnedImages.append(contentsOf: extractedImageURLs) } - completion(ExtractedShare(title: title, - description: description, - url: url, - selectedText: selectedText, - importedText: importedText, - images: returnedImages)) + let share = ExtractedShare( + title: title, + description: description, + url: url, + selectedText: selectedText, + importedText: importedText, + images: returnedImages + ) + let didExtractContent = extractedTextResults != nil || !returnedImages.isEmpty + completion(ShareLoadOutcome( + share: share, + failures: textFailures + imageFailures, + didExtractContent: didExtractContent + )) } } } @@ -165,14 +193,14 @@ private extension ShareExtractor { }) } - func extractText(completion: @escaping (ExtractedItem?) -> Void) { + func extractText(completion: @escaping (ExtractedItem?, [Error]) -> Void) { guard let textExtractor else { - completion(nil) + completion(nil, []) return } - textExtractor.extract(context: extensionContext) { extractedItems in + textExtractor.extract(context: extensionContext) { extractedItems, failures in guard !extractedItems.isEmpty else { - completion(nil) + completion(nil, failures) return } @@ -189,23 +217,28 @@ private extension ShareExtractor { let urls = extractedItems.compactMap({ $0.url }) - completion(ExtractedItem(selectedText: combinedSelectedText, - importedText: combinedImportedText, - description: combinedDescription, - url: urls.first, - title: combinedTitle, - images: extractedImages)) + completion( + ExtractedItem( + selectedText: combinedSelectedText, + importedText: combinedImportedText, + description: combinedDescription, + url: urls.first, + title: combinedTitle, + images: extractedImages + ), + failures + ) } } - func extractImages(completion: @escaping ([ExtractedImage]) -> Void) { + func extractImages(completion: @escaping ([ExtractedImage], [Error]) -> Void) { guard let imageExtractor else { - completion([]) + completion([], []) return } - imageExtractor.extract(context: extensionContext) { extractedItems in + imageExtractor.extract(context: extensionContext) { extractedItems, failures in guard !extractedItems.isEmpty else { - completion([]) + completion([], failures) return } var extractedImages = [ExtractedImage]() @@ -215,14 +248,14 @@ private extension ShareExtractor { }) }) - completion(extractedImages) + completion(extractedImages, failures) } } } private protocol ExtensionContentExtractor { func canHandle(context: NSExtensionContext) -> Bool - func extract(context: NSExtensionContext, completion: @escaping ([ExtractedItem]) -> Void) + func extract(context: NSExtensionContext, completion: @escaping (_ items: [ExtractedItem], _ failures: [Error]) -> Void) func saveToSharedContainer(image: UIImage) -> URL? func saveToSharedContainer(wrapper: FileWrapper) -> URL? func copyToSharedContainer(url: URL) -> URL? @@ -247,18 +280,20 @@ private extension TypeBasedExtensionContentExtractor { return !context.itemProviders(ofType: acceptedType).isEmpty } - func extract(context: NSExtensionContext, completion: @escaping ([ExtractedItem]) -> Void) { + func extract(context: NSExtensionContext, completion: @escaping (_ items: [ExtractedItem], _ failures: [Error]) -> Void) { let itemProviders = context.itemProviders(ofType: acceptedType) - print(acceptedType) var results = [ExtractedItem]() + var failures = [Error]() guard !itemProviders.isEmpty else { DispatchQueue.main.async { - completion(results) + completion(results, failures) } return } - // There 1 or more valid item providers here, lets work through them + // `loadItem` calls back on arbitrary queues and can run concurrently, so guard the + // accumulators with a lock. + let lock = NSLock() let syncGroup = DispatchGroup() for provider in itemProviders { syncGroup.enter() @@ -267,6 +302,9 @@ private extension TypeBasedExtensionContentExtractor { defer { syncGroup.leave() } if let error { DDLogError("Failed to load shared item of type \(self.acceptedType): \(error.localizedDescription)") + lock.lock() + failures.append(error) + lock.unlock() return } guard let payload = payload as? Payload else { @@ -274,17 +312,22 @@ private extension TypeBasedExtensionContentExtractor { } do { if let result = try self.convert(payload: payload) { + lock.lock() results.append(result) + lock.unlock() } } catch { DDLogError("Failed to extract shared item of type \(self.acceptedType): \(error.localizedDescription)") + lock.lock() + failures.append(error) + lock.unlock() } } } // Call the completion handler after all of the provider items are loaded syncGroup.notify(queue: DispatchQueue.main) { - completion(results) + completion(results, failures) } } diff --git a/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionAbstractViewController.swift b/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionAbstractViewController.swift index 957211ff6292..fd887e950bf2 100644 --- a/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionAbstractViewController.swift +++ b/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionAbstractViewController.swift @@ -147,6 +147,58 @@ extension ShareExtensionAbstractViewController { present(alertController, animated: true) } + /// Presents an error and then cancels the extension request — used when nothing usable + /// could be extracted from the shared content. + func presentContentExtractionFailure() { + let title = AppLocalizedString( + "shareExtension.contentError.title", + value: "Unable to load shared content", + comment: "Share extension error dialog title, shown when nothing could be read from what the user shared." + ) + let message = AppLocalizedString( + "shareExtension.contentError.message", + value: "Something went wrong reading what you shared. Please try again.", + comment: "Share extension error dialog message, shown when nothing could be read from what the user shared." + ) + let dismiss = AppLocalizedString( + "shareExtension.contentError.dismiss", + value: "Cancel sharing", + comment: "Share extension error dialog dismiss button, which closes the extension." + ) + + let alertController = UIAlertController(title: title, message: message, preferredStyle: .alert) + let alertAction = UIAlertAction(title: dismiss, style: .default) { [weak self] _ in + self?.cleanUpSharedContainerAndCache() + self?.dismissalCompletionBlock?(false) + } + alertController.addAction(alertAction) + present(alertController, animated: true) + } + + /// Presents a non-blocking notice that some shared attachments were skipped, then lets the + /// user keep editing whatever did load. + func presentSkippedAttachmentsNotice() { + let title = AppLocalizedString( + "shareExtension.partialError.title", + value: "Some content was skipped", + comment: "Share extension notice title, shown when some — but not all — shared items failed to load." + ) + let message = AppLocalizedString( + "shareExtension.partialError.message", + value: "One or more shared items could not be loaded and were skipped.", + comment: "Share extension notice message, shown when some — but not all — shared items failed to load." + ) + let dismiss = AppLocalizedString( + "shareExtension.partialError.dismiss", + value: "OK", + comment: "Share extension notice dismiss button." + ) + + let alertController = UIAlertController(title: title, message: message, preferredStyle: .alert) + alertController.addAction(UIAlertAction(title: dismiss, style: .default)) + present(alertController, animated: true) + } + func cleanUpSharedContainerAndCache() { ShareExtensionAbstractViewController.clearCache() diff --git a/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionEditorViewController.swift b/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionEditorViewController.swift index 116f37a11799..4b396f464304 100644 --- a/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionEditorViewController.swift +++ b/WordPress/WordPressShareExtension/Sources/UI/ShareExtensionEditorViewController.swift @@ -1263,25 +1263,38 @@ private extension ShareExtensionEditorViewController { } ShareExtractor(extensionContext: extensionContext) - .loadShare { [weak self] share in - self?.setTitleText(share.title) - self?.richTextView.setHTML(share.combinedContentHTML) + .loadShare { [weak self] outcome in + guard let self else { + return + } + + guard !outcome.extractionDidFail else { + self.presentContentExtractionFailure() + return + } + + let share = outcome.share + self.setTitleText(share.title) + self.richTextView.setHTML(share.combinedContentHTML) share.images.forEach({ extractedImage in if extractedImage.insertionState == .requiresInsertion { - self?.insertImageAttachment(with: extractedImage.url) + self.insertImageAttachment(with: extractedImage.url) } else { - self?.shareData.sharedImageDict.updateValue(UUID().uuidString, forKey: extractedImage.url) + self.shareData.sharedImageDict.updateValue(UUID().uuidString, forKey: extractedImage.url) } }) // Lets an extra

at the bottom to make editing a little easier - if let currentHTML = self?.richTextView.getHTML() { - self?.richTextView.setHTML(currentHTML + "

") - } + let currentHTML = self.richTextView.getHTML() + self.richTextView.setHTML(currentHTML + "

") // Clear out the extension context after loading it once. We don't need it anymore. - self?.context = nil + self.context = nil + + if !outcome.failures.isEmpty { + self.presentSkippedAttachmentsNotice() + } } } } diff --git a/WordPress/WordPressShareExtension/Sources/UI/ShareModularViewController.swift b/WordPress/WordPressShareExtension/Sources/UI/ShareModularViewController.swift index 3ca3a1b3eb46..9e90e28ae4b9 100644 --- a/WordPress/WordPressShareExtension/Sources/UI/ShareModularViewController.swift +++ b/WordPress/WordPressShareExtension/Sources/UI/ShareModularViewController.swift @@ -144,7 +144,13 @@ class ShareModularViewController: ShareExtensionAbstractViewController { } ShareExtractor(extensionContext: extensionContext) - .loadShare { share in + .loadShare { outcome in + guard !outcome.extractionDidFail else { + self.presentContentExtractionFailure() + return + } + + let share = outcome.share self.shareData.title = share.title self.shareData.contentBody = share.combinedContentHTML @@ -164,6 +170,10 @@ class ShareModularViewController: ShareExtensionAbstractViewController { // Clear out the extension context after loading it once. We don't need it anymore. self.context = nil self.refreshModulesTable() + + if !outcome.failures.isEmpty { + self.presentSkippedAttachmentsNotice() + } } }