From 2924c0e6cc05ca62dba091313f29fb36587b4ba0 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 2 Apr 2020 12:00:10 -0500 Subject: [PATCH] Rework download progress so that the delegate always manages it to make for smoother progress bar progressions. --- .../CloudKit/CloudKitAccountDelegate.swift | 134 ++++++++++++------ .../CloudKit/CloudKitAccountZone.swift | 1 - .../CloudKitAccountZoneDelegate.swift | 1 - .../CloudKit/CloudKitArticlesZone.swift | 1 - .../Account/CloudKit/CloudKitZone.swift | 18 --- .../LocalAccount/LocalAccountDelegate.swift | 12 +- .../LocalAccount/LocalAccountRefresher.swift | 23 ++- 7 files changed, 114 insertions(+), 76 deletions(-) diff --git a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift index 879b8bd0e..02636e56d 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift @@ -43,9 +43,7 @@ final class CloudKitAccountDelegate: AccountDelegate { var credentials: Credentials? var accountMetadata: AccountMetadata? - var refreshProgress: DownloadProgress { - return refresher.progress - } + var refreshProgress = DownloadProgress(numberOfTasks: 0) init(dataFolder: String) { accountZone = CloudKitAccountZone(container: container) @@ -53,8 +51,6 @@ final class CloudKitAccountDelegate: AccountDelegate { let databaseFilePath = (dataFolder as NSString).appendingPathComponent("Sync.sqlite3") database = SyncDatabase(databaseFilePath: databaseFilePath) - - accountZone.refreshProgress = refreshProgress } func receiveRemoteNotification(for account: Account, userInfo: [AnyHashable : Any], completion: @escaping () -> Void) { @@ -76,39 +72,7 @@ final class CloudKitAccountDelegate: AccountDelegate { } func refreshAll(for account: Account, completion: @escaping (Result) -> Void) { - BatchUpdate.shared.start() - accountZone.fetchChangesInZone() { result in - BatchUpdate.shared.end() - switch result { - case .success: - - self.sendArticleStatus(for: account) { result in - switch result { - case .success: - - self.refreshArticleStatus(for: account) { result in - switch result { - case .success: - - self.refresher.refreshFeeds(account.flattenedWebFeeds()) { - account.metadata.lastArticleFetchEndTime = Date() - completion(.success(())) - } - - case .failure(let error): - completion(.failure(error)) - } - } - case .failure(let error): - completion(.failure(error)) - } - - } - case .failure(let error): - BatchUpdate.shared.end() - completion(.failure(error)) - } - } + refreshAll(for: account, downloadFeeds: true, completion: completion) } func sendArticleStatus(for account: Account, completion: @escaping ((Result) -> Void)) { @@ -204,9 +168,10 @@ final class CloudKitAccountDelegate: AccountDelegate { return } - refreshProgress.addToNumberOfTasksAndRemaining(1) + refreshProgress.addToNumberOfTasksAndRemaining(3) FeedFinder.find(url: url) { result in + self.refreshProgress.completeTask() switch result { case .success(let feedSpecifiers): guard let bestFeedSpecifier = FeedSpecifier.bestFeed(in: feedSpecifiers), let url = URL(string: bestFeedSpecifier.urlString) else { @@ -222,6 +187,8 @@ final class CloudKitAccountDelegate: AccountDelegate { } self.accountZone.createWebFeed(url: bestFeedSpecifier.urlString, editedName: name, container: container) { result in + + self.refreshProgress.completeTask() switch result { case .success(let externalID): @@ -242,13 +209,12 @@ final class CloudKitAccountDelegate: AccountDelegate { } case .failure(let error): - self.refreshProgress.completeTask() completion(.failure(error)) } } case .failure: - self.refreshProgress.completeTask() + self.refreshProgress.clear() completion(.failure(AccountError.createErrorNotFound)) } @@ -258,7 +224,9 @@ final class CloudKitAccountDelegate: AccountDelegate { func renameWebFeed(for account: Account, with feed: WebFeed, to name: String, completion: @escaping (Result) -> Void) { let editedName = name.isEmpty ? nil : name + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.renameWebFeed(feed, editedName: editedName) { result in + self.refreshProgress.completeTask() switch result { case .success: feed.editedName = name @@ -270,7 +238,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func removeWebFeed(for account: Account, with feed: WebFeed, from container: Container, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.removeWebFeed(feed, from: container) { result in + self.refreshProgress.completeTask() switch result { case .success: container.removeWebFeed(feed) @@ -282,7 +252,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func moveWebFeed(for account: Account, with feed: WebFeed, from fromContainer: Container, to toContainer: Container, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.moveWebFeed(feed, from: fromContainer, to: toContainer) { result in + self.refreshProgress.completeTask() switch result { case .success: fromContainer.removeWebFeed(feed) @@ -295,7 +267,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func addWebFeed(for account: Account, with feed: WebFeed, to container: Container, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.addWebFeed(feed, to: container) { result in + self.refreshProgress.completeTask() switch result { case .success: container.addWebFeed(feed) @@ -307,7 +281,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func restoreWebFeed(for account: Account, feed: WebFeed, container: Container, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.createWebFeed(url: feed.url, editedName: feed.editedName, container: container) { result in + self.refreshProgress.completeTask() switch result { case .success(let externalID): feed.externalID = externalID @@ -320,7 +296,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func createFolder(for account: Account, name: String, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.createFolder(name: name) { result in + self.refreshProgress.completeTask() switch result { case .success(let externalID): if let folder = account.ensureFolder(with: name) { @@ -336,7 +314,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func renameFolder(for account: Account, with folder: Folder, to name: String, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.renameFolder(folder, to: name) { result in + self.refreshProgress.completeTask() switch result { case .success: folder.name = name @@ -348,7 +328,9 @@ final class CloudKitAccountDelegate: AccountDelegate { } func removeFolder(for account: Account, with folder: Folder, completion: @escaping (Result) -> Void) { + refreshProgress.addToNumberOfTasksAndRemaining(1) accountZone.removeFolder(folder) { result in + self.refreshProgress.completeTask() switch result { case .success: account.removeFolder(folder) @@ -365,19 +347,24 @@ final class CloudKitAccountDelegate: AccountDelegate { return } + let feedsToRestore = folder.topLevelWebFeeds + refreshProgress.addToNumberOfTasksAndRemaining(1 + feedsToRestore.count) + accountZone.createFolder(name: name) { result in + self.refreshProgress.completeTask() switch result { case .success(let externalID): folder.externalID = externalID account.addFolder(folder) let group = DispatchGroup() - for feed in folder.topLevelWebFeeds { + for feed in feedsToRestore { folder.topLevelWebFeeds.remove(feed) group.enter() self.restoreWebFeed(for: account, feed: feed, container: folder) { result in + self.refreshProgress.completeTask() group.leave() switch result { case .success: @@ -424,7 +411,7 @@ final class CloudKitAccountDelegate: AccountDelegate { switch result { case .success(let externalID): account.externalID = externalID - self.refreshAll(for: account) { result in + self.refreshAll(for: account, downloadFeeds: false) { result in if case .failure(let error) = result { os_log(.error, log: self.log, "Error while doing intial refresh: %@", error.localizedDescription) } @@ -466,3 +453,64 @@ final class CloudKitAccountDelegate: AccountDelegate { database.resume() } } + +private extension CloudKitAccountDelegate { + + func refreshAll(for account: Account, downloadFeeds: Bool, completion: @escaping (Result) -> Void) { + + let intialWebFeedsCount = downloadFeeds ? account.flattenedWebFeeds().count : 0 + refreshProgress.addToNumberOfTasksAndRemaining(3 + intialWebFeedsCount) + + BatchUpdate.shared.start() + accountZone.fetchChangesInZone() { result in + BatchUpdate.shared.end() + switch result { + case .success: + + let webFeeds = account.flattenedWebFeeds() + if downloadFeeds { + self.refreshProgress.addToNumberOfTasksAndRemaining(webFeeds.count - intialWebFeedsCount) + } + + self.refreshProgress.completeTask() + self.sendArticleStatus(for: account) { result in + switch result { + case .success: + + self.refreshProgress.completeTask() + self.refreshArticleStatus(for: account) { result in + switch result { + case .success: + + self.refreshProgress.completeTask() + + guard downloadFeeds else { + completion(.success(())) + return + } + + self.refresher.refreshFeeds(webFeeds, feedCompletionBlock: { _ in self.refreshProgress.completeTask() }) { + account.metadata.lastArticleFetchEndTime = Date() + completion(.success(())) + } + + case .failure(let error): + completion(.failure(error)) + } + } + + case .failure(let error): + completion(.failure(error)) + } + + } + + case .failure(let error): + self.refreshProgress.clear() + BatchUpdate.shared.end() + completion(.failure(error)) + } + } + } + +} diff --git a/Frameworks/Account/CloudKit/CloudKitAccountZone.swift b/Frameworks/Account/CloudKit/CloudKitAccountZone.swift index dc996baed..30611af51 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountZone.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountZone.swift @@ -22,7 +22,6 @@ final class CloudKitAccountZone: CloudKitZone { weak var container: CKContainer? weak var database: CKDatabase? - weak var refreshProgress: DownloadProgress? var delegate: CloudKitZoneDelegate? struct CloudKitWebFeed { diff --git a/Frameworks/Account/CloudKit/CloudKitAccountZoneDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountZoneDelegate.swift index b00561ad5..6ab4004fa 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountZoneDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountZoneDelegate.swift @@ -162,7 +162,6 @@ private extension CloudKitAcountZoneDelegate { return webFeed } - func addUnclaimedWebFeed(url: URL, editedName: String?, webFeedExternalID: String, containerExternalID: String) { if var unclaimedWebFeeds = self.unclaimedWebFeeds[containerExternalID] { unclaimedWebFeeds.append(UnclaimedWebFeed(url: url, editedName: editedName, webFeedExternalID: webFeedExternalID)) diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift index 36860f1e7..af34f8f6f 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift @@ -22,7 +22,6 @@ final class CloudKitArticlesZone: CloudKitZone { weak var container: CKContainer? weak var database: CKDatabase? - weak var refreshProgress: DownloadProgress? = nil var delegate: CloudKitZoneDelegate? = nil struct CloudKitArticleStatus { diff --git a/Frameworks/Account/CloudKit/CloudKitZone.swift b/Frameworks/Account/CloudKit/CloudKitZone.swift index 9cc28defa..15b91baf4 100644 --- a/Frameworks/Account/CloudKit/CloudKitZone.swift +++ b/Frameworks/Account/CloudKit/CloudKitZone.swift @@ -32,7 +32,6 @@ protocol CloudKitZone: class { var container: CKContainer? { get } var database: CKDatabase? { get } - var refreshProgress: DownloadProgress? { get set } var delegate: CloudKitZoneDelegate? { get set } } @@ -105,13 +104,10 @@ extension CloudKitZone { return } - refreshProgress?.addToNumberOfTasksAndRemaining(1) - database.perform(query, inZoneWith: Self.zoneID) { [weak self] records, error in switch CloudKitZoneResult.resolve(error) { case .success: DispatchQueue.main.async { - self?.refreshProgress?.completeTask() if let records = records { completion(.success(records)) } else { @@ -124,7 +120,6 @@ extension CloudKitZone { } default: DispatchQueue.main.async { - self?.refreshProgress?.completeTask() completion(.failure(error!)) } } @@ -139,12 +134,10 @@ extension CloudKitZone { let recordID = CKRecord.ID(recordName: externalID, zoneID: Self.zoneID) - refreshProgress?.addToNumberOfTasksAndRemaining(1) database?.fetch(withRecordID: recordID) { [weak self] record, error in switch CloudKitZoneResult.resolve(error) { case .success: DispatchQueue.main.async { - self?.refreshProgress?.completeTask() if let record = record { completion(.success(record)) } else { @@ -157,7 +150,6 @@ extension CloudKitZone { } default: DispatchQueue.main.async { - self?.refreshProgress?.completeTask() completion(.failure(error!)) } } @@ -201,7 +193,6 @@ extension CloudKitZone { switch CloudKitZoneResult.resolve(error) { case .success: DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.success(())) } case .zoneNotFound: @@ -211,14 +202,12 @@ extension CloudKitZone { self.modify(recordsToSave: recordsToSave, recordIDsToDelete: recordIDsToDelete, completion: completion) case .failure(let error): DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.failure(error)) } } } case .userDeletedZone: DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.failure(CloudKitZoneError.userDeletedZone)) } case .retry(let timeToWait): @@ -253,13 +242,11 @@ extension CloudKitZone { default: DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.failure(error!)) } } } - refreshProgress?.addToNumberOfTasksAndRemaining(1) database?.add(op) } @@ -324,7 +311,6 @@ extension CloudKitZone { switch CloudKitZoneResult.resolve(error) { case .success: DispatchQueue.main.async { - self.refreshProgress?.completeTask() self.delegate?.cloudKitDidModify(changed: changedRecords, deleted: deletedRecordKeys, completion: completion) } case .zoneNotFound: @@ -334,14 +320,12 @@ extension CloudKitZone { self.fetchChangesInZone(completion: completion) case .failure(let error): DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.failure(error)) } } } case .userDeletedZone: DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.failure(CloudKitZoneError.userDeletedZone)) } case .retry(let timeToWait): @@ -355,14 +339,12 @@ extension CloudKitZone { } default: DispatchQueue.main.async { - self.refreshProgress?.completeTask() completion(.failure(error!)) } } } - refreshProgress?.addToNumberOfTasksAndRemaining(1) database?.add(op) } diff --git a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift index 3483d758e..a1751ff16 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift @@ -18,6 +18,8 @@ public enum LocalAccountDelegateError: String, Error { final class LocalAccountDelegate: AccountDelegate { + private let refresher = LocalAccountRefresher() + let behaviors: AccountBehaviors = [] let isOPMLImportInProgress = false @@ -25,18 +27,16 @@ final class LocalAccountDelegate: AccountDelegate { var credentials: Credentials? var accountMetadata: AccountMetadata? - private let refresher = LocalAccountRefresher() - - var refreshProgress: DownloadProgress { - return refresher.progress - } + let refreshProgress = DownloadProgress(numberOfTasks: 0) func receiveRemoteNotification(for account: Account, userInfo: [AnyHashable : Any], completion: @escaping () -> Void) { completion() } func refreshAll(for account: Account, completion: @escaping (Result) -> Void) { - refresher.refreshFeeds(account.flattenedWebFeeds()) { + let webFeeds = account.flattenedWebFeeds() + refreshProgress.addToNumberOfTasksAndRemaining(webFeeds.count) + refresher.refreshFeeds(webFeeds, feedCompletionBlock: { _ in self.refreshProgress.completeTask() }) { account.metadata.lastArticleFetchEndTime = Date() completion(.success(())) } diff --git a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift index 870113d3d..7d13f058b 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift @@ -14,6 +14,7 @@ import Articles final class LocalAccountRefresher { + private var feedCompletionBlock: ((WebFeed) -> Void)? private var completion: (() -> Void)? private var isSuspended = false @@ -21,11 +22,8 @@ final class LocalAccountRefresher { return DownloadSession(delegate: self) }() - var progress: DownloadProgress { - return downloadSession.progress - } - - public func refreshFeeds(_ feeds: Set, completion: @escaping () -> Void) { + public func refreshFeeds(_ feeds: Set, feedCompletionBlock: @escaping (WebFeed) -> Void, completion: @escaping () -> Void) { + self.feedCompletionBlock = feedCompletionBlock self.completion = completion downloadSession.downloadObjects(feeds as NSSet) } @@ -62,28 +60,37 @@ extension LocalAccountRefresher: DownloadSessionDelegate { } func downloadSession(_ downloadSession: DownloadSession, downloadDidCompleteForRepresentedObject representedObject: AnyObject, response: URLResponse?, data: Data, error: NSError?, completion: @escaping () -> Void) { - guard let feed = representedObject as? WebFeed, !data.isEmpty, !isSuspended else { + let feed = representedObject as! WebFeed + + guard !data.isEmpty, !isSuspended else { completion() + feedCompletionBlock?(feed) return } if let error = error { print("Error downloading \(feed.url) - \(error)") completion() + feedCompletionBlock?(feed) return } let dataHash = data.md5String if dataHash == feed.contentHash { completion() + feedCompletionBlock?(feed) return } let parserData = ParserData(url: feed.url, data: data) FeedParser.parse(parserData) { (parsedFeed, error) in + guard let account = feed.account, let parsedFeed = parsedFeed, error == nil else { + completion() + self.feedCompletionBlock?(feed) return } + account.update(feed, with: parsedFeed) { error in if error == nil { if let httpResponse = response as? HTTPURLResponse { @@ -93,7 +100,9 @@ extension LocalAccountRefresher: DownloadSessionDelegate { feed.contentHash = dataHash } completion() + self.feedCompletionBlock?(feed) } + } } @@ -122,6 +131,8 @@ extension LocalAccountRefresher: DownloadSessionDelegate { } func downloadSession(_ downloadSession: DownloadSession, didReceiveNotModifiedResponse: URLResponse, representedObject: AnyObject) { + let feed = representedObject as! WebFeed + feedCompletionBlock?(feed) } func downloadSessionDidCompleteDownloadObjects(_ downloadSession: DownloadSession) {