From 36edc9cd446ac1da0ee6720113be2de20e43a0ea Mon Sep 17 00:00:00 2001 From: jahorton Date: Mon, 20 Jul 2020 10:36:02 +0700 Subject: [PATCH] refactor(ios/engine): pkg-based resource updates --- .../LanguageLMDetailViewController.swift | 2 +- .../Classes/Notification/Notifications.swift | 5 +- .../ResourceDownloadManager.swift | 165 +++++++++--------- .../ResourceDownloadQueue.swift | 103 +++++------ .../InstalledLanguagesViewController.swift | 17 +- 5 files changed, 147 insertions(+), 145 deletions(-) diff --git a/ios/engine/KMEI/KeymanEngine/Classes/LanguagePicker/LanguageLMDetailViewController.swift b/ios/engine/KMEI/KeymanEngine/Classes/LanguagePicker/LanguageLMDetailViewController.swift index e44253c73d..a536e87ad2 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/LanguagePicker/LanguageLMDetailViewController.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/LanguagePicker/LanguageLMDetailViewController.swift @@ -144,7 +144,7 @@ class LanguageLMDetailViewController: UITableViewController, UIAlertViewDelegate let package = packages[lexicalModelIndex] let lmFullID = package.0.fullID let completionClosure: ResourceDownloadManager.CompletionHandler = { package, error in - ResourceDownloadManager.shared.standardLexicalModelInstallCompletionBlock(forFullID: lmFullID)(package, error) + try? ResourceDownloadManager.shared.standardLexicalModelInstallCompletionBlock(forFullID: lmFullID)(package, error) if let lm = package?.findResource(withID: lmFullID) { self.onSuccessClosure?(lm) diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Notification/Notifications.swift b/ios/engine/KMEI/KeymanEngine/Classes/Notification/Notifications.swift index 240bb62ae3..b1076d3e2d 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Notification/Notifications.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Notification/Notifications.swift @@ -38,9 +38,8 @@ public struct PackageDownloadFailedNotification { public typealias BatchUpdateStartedNotification = [AnyLanguageResource] public struct BatchUpdateCompletedNotification { - public let successes: [[AnyLanguageResource]] - public let failures: [[AnyLanguageResource]] - public let errors: [Error] + public let successes: [KeymanPackage.Key] + public let failures: [(KeymanPackage.Key, Error)] } public typealias KeyboardLoadedNotification = InstallableKeyboard diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift index 576b5bfc1e..3ec9c7f6e4 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift @@ -22,7 +22,7 @@ public class ResourceDownloadManager { public static let DISTRIBUTION_CACHE_VALIDITY_THRESHOLD = TimeInterval(60*24*7) // in seconds. 60 minutes, 24 hrs, 7 days. - public typealias CompletionHandler = (Package?, Error?) -> Void + public typealias CompletionHandler = (Package?, Error?) throws -> Void public typealias BatchCompletionHandler = () -> Void internal typealias InternalBatchCompletionHandler = (CompositeBatch) -> Void @@ -54,7 +54,7 @@ public class ResourceDownloadManager { guard let result = result, error == nil else { log.info("Error occurred requesting location for \(fullID.description)") self.resourceDownloadFailed(withKey: packageKey, with: error ?? .noData) - completionBlock?(nil, error ?? .noData) + try? completionBlock?(nil, error ?? .noData) return } @@ -65,7 +65,7 @@ public class ResourceDownloadManager { } } self.resourceDownloadFailed(withKey: packageKey, with: Queries.ResultError.unqueried) - completionBlock?(nil, Queries.ResultError.unqueried) + try? completionBlock?(nil, Queries.ResultError.unqueried) return } @@ -74,7 +74,7 @@ public class ResourceDownloadManager { let err = self.downloader.state.error ?? NSError(domain: "Keyman", code: 0, userInfo: [NSLocalizedDescriptionKey: "Already busy downloading something"]) self.resourceDownloadFailed(withKey: packageKey, with: err) - completionBlock?(nil, err) + try? completionBlock?(nil, err) return } @@ -384,71 +384,68 @@ public class ResourceDownloadManager { } } - // Possibly worth not deprecating, as its continued existence doesn't exactly hurt anything. - // But its intended partner method will no longer exist, so... yeah. - @available(*, deprecated, message: "Deprecated in favor of `getKeysForUpdatablePackages`.") - public func getAvailableUpdates() -> [AnyLanguageResource]? { - let updatablePackages = getKeysForUpdatablePackages() + public func performBatchUpdate(forPackageKeys keysToUpdate: Set? = nil, + withNotifications: Bool = true, + completionBlock: (([KeymanPackage.Key], [(KeymanPackage.Key, Error)]) -> Void)? = nil) { + let engineUpdatables = getKeysForUpdatablePackages() + var updatables = Set() - // To maintain the original behavior. - guard updatablePackages.count > 0 else { - return nil - } + var invalids: [(KeymanPackage.Key, Error)] = [] - // As updatablePackages is a Set (and keys are Hashable), the lookups are O(1). - let updatables: [AnyLanguageResource] = Storage.active.userDefaults.userResources?.compactMap { updatablePackages.contains($0.packageKey) ? $0 : nil } ?? [] - - if updatables.count > 0 { - return updatables + if keysToUpdate == nil { + updatables = engineUpdatables } else { - return nil - } - } + // Verify that KeymanEngine actually can update the entry. + updatables = engineUpdatables.union(keysToUpdate!) - @available(*, deprecated, message: "") // TODO: Properly document once the target method is written. - public func performUpdates(forResources resources: [AnyLanguageResource]) { - // The plan is to create new notifications to handle batch updates here, rather than - // require a UI to manage the update queue. - var batches: [AnyDownloadBatch] = [] - - /* For fixing the TODOs: we can probably map the resources to a Set of their - * represented package keys, then just use the package-key based update method. - * - * You know, once it's written. - */ - - // TODO: All package updates are currently broken, as apiKeyboardRepository will specify the wrong file. - // TODO: Merge the keyboard and lexical model pathways; it's WET code. - resources.forEach { res in - if let kbd = res as? InstallableKeyboard { -// if let filename = Manager.shared.apiKeyboardRepository.keyboards?[kbd.id]?.filename, -// let path = URL.init(string: filename) { -// let batch = self.buildPackageBatch(forFullID: kbd.fullID, from: path, withResource: kbd) { package, error in -// if let package = package { -// try? ResourceFileManager.shared.install(resourceWithID: kbd.fullID, from: package) -// } -// // else error: already handled by wrapping closure set within buildPackageBatch. -// } -// batches.append(batch) -// } - } else if let lex = res as? InstallableLexicalModel { -// if let filename = Manager.shared.apiLexicalModelRepository.lexicalModels?[lex.id]?.packageFilename, -// let path = URL.init(string: filename) { -// let batch = self.buildPackageBatch(withKey: lex.packageKey, from: path, withResource: lex) { package, error in -// if let package = package { -// try? ResourceFileManager.shared.install(resourceWithID: lex.fullID, from: package) -// } -// // else error: already handled by wrapping closure set within buildPackageBatch. -// } -// batches.append(batch) -// } + // Generate errors for any entries that KeymanEngine cannot update. + keysToUpdate!.forEach { key in + if !updatables.contains(key) { + // TODO: Better error definition + invalids.append( (key, NSError()) ) + } } } - - let batchUpdate = CompositeBatch(queue: batches.map { return DownloadNode.simpleBatch($0) }, - startBlock: resourceBatchUpdateStartClosure(for: resources), - completionBlock: resourceBatchUpdateCompletionClosure(for: resources)) - downloader.queue(.compositeBatch(batchUpdate)) + + var updateMapping: Dictionary = [:] + updatables.forEach { key in + guard let downloadURL = Storage.active.userDefaults.cachedPackageQueryResult(forPackageKey: key)!.downloadURL else { + // Note error - download URL missing. Shouldn't be possible, but still. + // TODO: Better error definition + invalids.append( (key, NSError()) ) + return + } + + updateMapping[key] = self.buildPackageBatch(withKey: key, + from: downloadURL) { package, error in + guard let package = package, error == nil else { + let errString = error != nil ? String(describing: error!) : "" + log.error("Could not successfully download package \(key) for update: \(errString)") + return + } + + if let kbdPackage = package as? KeyboardKeymanPackage { + let updatables = ResourceFileManager.shared.findPotentialUpdates(in: kbdPackage) + try ResourceFileManager.shared.install(resourcesWithIDs: updatables.map { $0.fullID }, from: kbdPackage) + } else if let lmPackage = package as? LexicalModelKeymanPackage { + let updatables = ResourceFileManager.shared.findPotentialUpdates(in: lmPackage) + try ResourceFileManager.shared.install(resourcesWithIDs: updatables.map { $0.fullID }, from: lmPackage) + } + } + } + + // Build the composite batch. + let batchNodes: [DownloadNode] = updateMapping.values.map { .simpleBatch($0) } + let updateStartClosure: (() -> Void)? = nil + let updateCompletionClosure: InternalBatchCompletionHandler = resourceBatchUpdateCompletionClosure(withNotifications: withNotifications, completionBlock: completionBlock) + let updateBatch = CompositeBatch(queue: batchNodes, startBlock: updateStartClosure, completionBlock: updateCompletionClosure) + + downloader.queue(.compositeBatch(updateBatch)) + } + + static func packageKeys(forResources resources: [AnyLanguageResource]) -> Set { + let keys = resources.map { $0.packageKey } + return Set(keys) } @available(*, deprecated) @@ -484,7 +481,11 @@ public class ResourceDownloadManager { self.resourceDownloadCompleted(with: package) } - handler?(package, error) + do { + try handler?(package, error) + } catch { + log.error("Unhandled error occurred after resource successfully downloaded: \(String(describing: error))") + } // After the custom handler operates, ensure that any changes it made are synchronized for use // with the app extension, too. @@ -503,30 +504,32 @@ public class ResourceDownloadManager { } } - internal func resourceBatchUpdateCompletionClosure(for resources: [AnyLanguageResource], - completionBlock: BatchCompletionHandler? = nil) + internal func resourceBatchUpdateCompletionClosure(withNotifications: Bool, + completionBlock: (([KeymanPackage.Key], [(KeymanPackage.Key, Error)]) -> Void)? = nil) -> InternalBatchCompletionHandler { return { batch in var successes: [KeymanPackage.Key] = [] - var failures: [KeymanPackage.Key] = [] - var errors: [Error] = [] + var failures: [(KeymanPackage.Key, Error)] = [] - // Remember, since this is for .composite batches, batch.tasks is of type [DownloadBatch]. - for (index, _) in batch.tasks.enumerated() { - if batch.errors[index] == nil { - successes.append(contentsOf: batch.batches[index].packageKeys) - } else { - failures.append(contentsOf: batch.batches[index].packageKeys) - errors.append(batch.errors[index]!) + batch.batchQueue.forEach{ tuple in + if case let .simpleBatch(node) = tuple.0 { + if let err = node.errors.first(where: { $0 != nil }), let error = err { + failures.append( (node.packageKey, error) ) + } else { + successes.append(node.packageKey) + } } } - // TODO: Rework batch update notifications to return package keys. - let notification = BatchUpdateCompletedNotification(successes: [], failures: [], errors: errors) - NotificationCenter.default.post(name: Notifications.batchUpdateCompleted, - object: self, - value: notification) - completionBlock?() + if withNotifications { + // Send update notification to the UI. + let notification = BatchUpdateCompletedNotification(successes: successes, failures: failures) + NotificationCenter.default.post(name: Notifications.batchUpdateCompleted, + object: self, + value: notification) + } + + completionBlock?(successes, failures) } } diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift index 48605ffcc9..da10ca3f99 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift @@ -28,16 +28,7 @@ enum DownloadNode { case .simpleBatch(let batch): return batch.errors case .compositeBatch(let node): - return node.errors - } - } - - set(value) { - switch(self) { - case .simpleBatch(var batch): - batch.errors = value - case .compositeBatch(let node): - node.errors = value + return node.batchQueue.map{ $0.1 } } } } @@ -50,7 +41,6 @@ protocol AnyDownloadTask { } class DownloadTask: AnyDownloadTask { - public final var packageKey: KeymanPackage.Key? public final var request: HTTPDownloadRequest public var file: URL { if let file = finalFile { @@ -66,13 +56,11 @@ class DownloadTask: AnyDownloadTask { public init(do request: HTTPDownloadRequest, forPackage packageKey: KeymanPackage.Key?) { self.request = request - self.packageKey = packageKey } public init(forPackageWithKey packageKey: KeymanPackage.Key, from url: URL, as destURL: URL, tempURL: URL) { - self.packageKey = packageKey let request = HTTPDownloadRequest(url: url, userInfo: [:]) request.destinationFile = tempURL.path @@ -110,14 +98,17 @@ enum DownloadActivityType { protocol AnyDownloadBatch { var tasks: [AnyDownloadTask] { get } + var packageKey: KeymanPackage.Key { get } + + // Needed for DownloadNode compliance. var packageKeys: [KeymanPackage.Key] { get } var startBlock: (() -> Void)? { get } var errors: [Error?] { get set } - func completeWithCancellation() -> Void - func completeWithError(error: Error) -> Void - func completeWithPackage(fromKMP file: URL) -> Void + func completeWithCancellation() throws -> Void + func completeWithError(error: Error) throws -> Void + func completeWithPackage(fromKMP file: URL) throws -> Void } /** @@ -127,25 +118,17 @@ class DownloadBatch: AnyDownloadBatch { typealias CompletionHandler = ResourceDownloadManager.CompletionHandler public final var downloadTasks: [DownloadTask] + public let packageKey: KeymanPackage.Key var errors: [Error?] // Only used by the ResourceDownloadQueue. public final var startBlock: (() -> Void)? = nil public final var completionBlock: CompletionHandler? = nil - public init?(do tasks: [DownloadTask], - startBlock: (() -> Void)? = nil, - completionBlock: CompletionHandler? = nil) { - self.downloadTasks = tasks - - self.errors = Array(repeating: nil, count: tasks.count) - self.startBlock = startBlock - self.completionBlock = completionBlock - } - public init(forPackageWithKey packageKey: KeymanPackage.Key, from url: URL, startBlock: (() -> Void)?, completionBlock: CompletionHandler?) { // If we can't build a proper DownloadTask, we can't build the batch. + self.packageKey = packageKey let tempArtifact = ResourceFileManager.shared.packageDownloadTempPath(forKey: packageKey) let finalFile = ResourceFileManager.shared.cachedPackagePath(forKey: packageKey) @@ -168,60 +151,57 @@ class DownloadBatch: AnyDownloadBatch { public var packageKeys: [KeymanPackage.Key] { get { - return downloadTasks.compactMap { $0.packageKey } + return [ packageKey ] } } - public func completeWithCancellation() { + public func completeWithCancellation() throws { let complete = completionBlock completionBlock = nil - complete?(nil, nil) + try complete?(nil, nil) } - public func completeWithError(error: Error) { + public func completeWithError(error: Error) throws { let complete = completionBlock completionBlock = nil - complete?(nil, error) + try complete?(nil, error) } - public func completeWithPackage(fromKMP file: URL) { + public func completeWithPackage(fromKMP file: URL) throws { let complete = completionBlock completionBlock = nil do { if let package = try ResourceFileManager.shared.prepareKMPInstall(from: file) as? Package { - complete?(package, nil) + try complete?(package, nil) } else { - complete?(nil, KMPError.invalidPackage) + try complete?(nil, KMPError.invalidPackage) } } catch { - complete?(nil, error) + try complete?(nil, error) } } } class CompositeBatch { - public final var batches: [DownloadNode] - var errors: [Error?] // Only used by the ResourceDownloadQueue. + public final var batchQueue: [(DownloadNode, Error?)] public final var startBlock: (() -> Void)? = nil public final var completionBlock: ResourceDownloadManager.InternalBatchCompletionHandler? = nil public init(queue: [DownloadNode], startBlock: (() -> Void)? = nil, completionBlock: ResourceDownloadManager.InternalBatchCompletionHandler? = nil) { - self.batches = queue - self.errors = Array(repeating: nil, count: batches.count) - + self.batchQueue = queue.map { ($0, nil) } self.startBlock = startBlock self.completionBlock = completionBlock } public var tasks: [DownloadNode] { - return batches + return batchQueue.map { $0.0 } } public var packageKeys: [KeymanPackage.Key] { - return batches.flatMap { $0.packageKeys } + return batchQueue.flatMap { $0.0.packageKeys } } } @@ -336,7 +316,7 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { } } - private func finalizeCurrentBatch() { + private func finalizeCurrentBatch(withError error: Error? = nil) { let frame = queueStack[queueStack.count - 1] if queueStack.count == 1 { @@ -347,6 +327,9 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { // when everything's done instead. Might be worth a thought. frame.nodes.remove(at: 0) } else { + if case let .compositeBatch(batch) = frame.batch { + batch.batchQueue[frame.index].1 = error + } // Batches in subframes should be kept so that we can report progress; increment the index instead. frame.index += 1 } @@ -368,14 +351,16 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { if frame.index == frame.nodes.count { // We've hit the end of this stack frame's commands; time to pop and continue from the previous frame's perspective. _ = queueStack.popLast() - - // Of course, this means we've "finished" a batch download. We can use the same handlers as before. - finalizeCurrentBatch() + // Of course, this means we've "finished" a batch download. We can use the same handlers as before. // if-check below: "if the current stack-frame came from a composite batch, let node = that 'composite batch'" if case .compositeBatch(let node) = frame.batch { // The base handler requires access to the batch's tracked success/failure data. + let error = node.batchQueue.first(where: { $0.1 != nil })?.1 + finalizeCurrentBatch(withError: error) node.completionBlock?(node) + } else { + fatalError("Unexpected download queue state; cannot recover") } if autoExecute { @@ -456,11 +441,14 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { let batch = queue.userInfo[Key.downloadBatch] as! AnyDownloadBatch let packagePath = batch.tasks[0].file - batch.completeWithPackage(fromKMP: packagePath) - - // Completing the queue means having completed a batch. We should only move forward in this class's - // queue at this time, once a batch's task queue is complete. - finalizeCurrentBatch() + do { + try batch.completeWithPackage(fromKMP: packagePath) + // Completing the queue means having completed a batch. We should only move forward in this class's + // queue at this time, once a batch's task queue is complete. + finalizeCurrentBatch() + } catch { + finalizeCurrentBatch(withError: error) + } if autoExecute { executeNext() @@ -469,7 +457,7 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { func downloadQueueCancelled(_ queue: HTTPDownloader) { if case .simpleBatch(let batch) = self.currentBatch { - batch.completeWithCancellation() + try? batch.completeWithCancellation() } // In case we're part of a 'composite' operation, we should still keep the queue moving. @@ -495,7 +483,10 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { let errorMessage = "\(request.responseStatusMessage ?? ""): \(request.url)" let error = NSError(domain: "Keyman", code: 0, userInfo: [NSLocalizedDescriptionKey: errorMessage]) - currentFrame.batch?.errors[currentFrame.index] = error + + if case var .simpleBatch(batch) = currentFrame.batch { + batch.errors[currentFrame.index] = error + } // Now that we've synthesized an appropriate error instance, use the same handler // as for HTTPDownloader's 'failed' condition. @@ -513,9 +504,9 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { } func downloadRequestFailed(_ request: HTTPDownloadRequest, with error: Error?) { - currentFrame.batch?.errors[currentFrame.index] = error let task = request.userInfo[Key.downloadTask] as! AnyDownloadTask - let batch = request.userInfo[Key.downloadBatch] as! AnyDownloadBatch + var batch = request.userInfo[Key.downloadBatch] as! AnyDownloadBatch + batch.errors[currentFrame.index] = error var err: Error if let error = error { @@ -526,7 +517,7 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { } try? task.downloadFinalizationBlock?(false) - batch.completeWithError(error: err) + try? batch.completeWithError(error: err) downloader!.cancelAllOperations() } } diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Settings/InstalledLanguagesViewController.swift b/ios/engine/KMEI/KeymanEngine/Classes/Settings/InstalledLanguagesViewController.swift index b57c2cdae2..afa19abff9 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Settings/InstalledLanguagesViewController.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Settings/InstalledLanguagesViewController.swift @@ -130,8 +130,13 @@ public class InstalledLanguagesViewController: UITableViewController, UIAlertVie // Do the actual updates! // TODO: Consider prompting per resource, rather than wholesale as a group. // (This would be an enhancement, though.) - let availableUpdates = ResourceDownloadManager.shared.getAvailableUpdates()! - ResourceDownloadManager.shared.performUpdates(forResources: availableUpdates) + let availableUpdates = ResourceDownloadManager.shared.getKeysForUpdatablePackages() + ResourceDownloadManager.shared.performBatchUpdate(forPackageKeys: availableUpdates, + withNotifications: true, + completionBlock: { successes, failures in + // TODO: future feature: consider reworking the notification listener for use with this callback. + // (See `batchUpdateCompleted` later in this file.) + }) } override public func numberOfSections(in tableView: UITableView) -> Int { @@ -378,10 +383,14 @@ public class InstalledLanguagesViewController: UITableViewController, UIAlertVie toolbar.displayStatus("Updating\u{2026}", withIndicator: true) } - + private func batchUpdateCompleted(results: BatchUpdateCompletedNotification) { if let toolbar = navigationController?.toolbar as? ResourceDownloadStatusToolbar { - toolbar.displayStatus("Updates successfully downloaded!", withIndicator: false, duration: 3.0) + if results.failures.count == 0 { + toolbar.displayStatus("\(results.successes.count) updates successfully downloaded!", withIndicator: false, duration: 3.0) + } else { + toolbar.displayStatus("Updates complete: \(results.successes.count) successful, \(results.failures.count) failed", withIndicator: false, duration: 3.0) + } } restoreNavigation()