From 63c4d53ee9183d537fa4402cc038719aa6f79709 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Fri, 10 Apr 2020 11:20:35 -0500 Subject: [PATCH] Change LocalAccountRefresher to use a delegate so that it can better handle duplicate feed requests --- .../CloudKit/CloudKitAccountDelegate.swift | 82 ++++++++++++------- .../LocalAccount/LocalAccountDelegate.swift | 42 ++++++++-- .../LocalAccount/LocalAccountRefresher.swift | 40 +++++---- submodules/RSWeb | 2 +- 4 files changed, 112 insertions(+), 54 deletions(-) diff --git a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift index d6f03a63e..df9d5c6be 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift @@ -34,7 +34,13 @@ final class CloudKitAccountDelegate: AccountDelegate { private let accountZone: CloudKitAccountZone private let articlesZone: CloudKitArticlesZone - private let refresher = LocalAccountRefresher() + weak var account: Account? + + private lazy var refresher: LocalAccountRefresher? = { + let refresher = LocalAccountRefresher() + refresher.delegate = self + return refresher + }() let behaviors: AccountBehaviors = [] let isOPMLImportInProgress = false @@ -44,7 +50,8 @@ final class CloudKitAccountDelegate: AccountDelegate { var accountMetadata: AccountMetadata? var refreshProgress = DownloadProgress(numberOfTasks: 0) - + var refreshAllCompletion: ((Result) -> Void)? = nil + init(dataFolder: String) { accountZone = CloudKitAccountZone(container: container) articlesZone = CloudKitArticlesZone(container: container) @@ -72,11 +79,13 @@ final class CloudKitAccountDelegate: AccountDelegate { } func refreshAll(for account: Account, completion: @escaping (Result) -> Void) { - guard refreshProgress.isComplete else { + guard refreshAllCompletion == nil else { completion(.success(())) return } - refreshAll(for: account, downloadFeeds: true, completion: completion) + refreshAllCompletion = completion + + refreshAll(for: account, downloadFeeds: true) } func sendArticleStatus(for account: Account, completion: @escaping ((Result) -> Void)) { @@ -146,6 +155,12 @@ final class CloudKitAccountDelegate: AccountDelegate { } func importOPML(for account:Account, opmlFile: URL, completion: @escaping (Result) -> Void) { + guard refreshAllCompletion == nil else { + completion(.success(())) + return + } + refreshAllCompletion = completion + var fileData: Data? do { @@ -199,7 +214,7 @@ final class CloudKitAccountDelegate: AccountDelegate { } self.accountZone.importOPML(rootExternalID: rootExternalID, items: normalizedItems) { _ in - self.refreshAll(for: account, downloadFeeds: false, completion: completion) + self.refreshAll(for: account, downloadFeeds: false) } } @@ -463,6 +478,8 @@ final class CloudKitAccountDelegate: AccountDelegate { } func accountDidInitialize(_ account: Account) { + self.account = account + accountZone.delegate = CloudKitAcountZoneDelegate(account: account, refreshProgress: refreshProgress) articlesZone.delegate = CloudKitArticlesZoneDelegate(account: account, database: database, articlesZone: articlesZone) @@ -472,11 +489,7 @@ final class CloudKitAccountDelegate: AccountDelegate { switch result { case .success(let externalID): account.externalID = externalID - 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) - } - } + self.refreshAll(for: account, downloadFeeds: false) case .failure(let error): os_log(.error, log: self.log, "Error adding account container: %@", error.localizedDescription) } @@ -501,7 +514,7 @@ final class CloudKitAccountDelegate: AccountDelegate { // MARK: Suspend and Resume (for iOS) func suspendNetwork() { - refresher.suspend() + refresher?.suspend() } func suspendDatabase() { @@ -509,18 +522,25 @@ final class CloudKitAccountDelegate: AccountDelegate { } func resume() { - refresher.resume() + refresher?.resume() database.resume() } } private extension CloudKitAccountDelegate { - func refreshAll(for account: Account, downloadFeeds: Bool, completion: @escaping (Result) -> Void) { + func refreshAll(for account: Account, downloadFeeds: Bool) { let intialWebFeedsCount = downloadFeeds ? account.flattenedWebFeeds().count : 0 refreshProgress.addToNumberOfTasksAndRemaining(3 + intialWebFeedsCount) + func fail(_ error: Error) { + self.processAccountError(account, error) + self.refreshProgress.clear() + self.refreshAllCompletion?(.failure(error)) + self.refreshAllCompletion = nil + } + BatchUpdate.shared.start() accountZone.fetchChangesInZone() { result in BatchUpdate.shared.end() @@ -545,35 +565,26 @@ private extension CloudKitAccountDelegate { self.refreshProgress.completeTask() guard downloadFeeds else { - completion(.success(())) + self.refreshAllCompletion?(.success(())) + self.refreshAllCompletion = nil return } - self.refresher.refreshFeeds(webFeeds, feedCompletionBlock: { _ in self.refreshProgress.completeTask() }) { - account.metadata.lastArticleFetchEndTime = Date() - self.refreshProgress.clear() - completion(.success(())) - } + self.refresher?.refreshFeeds(webFeeds) case .failure(let error): - self.processAccountError(account, error) - self.refreshProgress.clear() - completion(.failure(error)) + fail(error) } } case .failure(let error): - self.processAccountError(account, error) - self.refreshProgress.clear() - completion(.failure(error)) + fail(error) } } case .failure(let error): - self.processAccountError(account, error) - self.refreshProgress.clear() - completion(.failure(error)) + fail(error) } } } @@ -588,3 +599,18 @@ private extension CloudKitAccountDelegate { } } + +extension CloudKitAccountDelegate: LocalAccountRefresherDelegate { + + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { + refreshProgress.completeTask() + } + + func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) { + self.refreshProgress.clear() + account?.metadata.lastArticleFetchEndTime = Date() + refreshAllCompletion?(.success(())) + refreshAllCompletion = nil + } + +} diff --git a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift index e99388d81..dd7309c72 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift @@ -18,7 +18,13 @@ public enum LocalAccountDelegateError: String, Error { final class LocalAccountDelegate: AccountDelegate { - private let refresher = LocalAccountRefresher() + weak var account: Account? + + private lazy var refresher: LocalAccountRefresher? = { + let refresher = LocalAccountRefresher() + refresher.delegate = self + return refresher + }() let behaviors: AccountBehaviors = [] let isOPMLImportInProgress = false @@ -28,19 +34,23 @@ final class LocalAccountDelegate: AccountDelegate { var accountMetadata: AccountMetadata? let refreshProgress = DownloadProgress(numberOfTasks: 0) + var refreshAllCompletion: ((Result) -> Void)? = nil func receiveRemoteNotification(for account: Account, userInfo: [AnyHashable : Any], completion: @escaping () -> Void) { completion() } func refreshAll(for account: Account, completion: @escaping (Result) -> Void) { + guard refreshAllCompletion == nil else { + completion(.success(())) + return + } + + refreshAllCompletion = completion + let webFeeds = account.flattenedWebFeeds() refreshProgress.addToNumberOfTasksAndRemaining(webFeeds.count) - refresher.refreshFeeds(webFeeds, feedCompletionBlock: { _ in self.refreshProgress.completeTask() }) { - self.refreshProgress.clear() - account.metadata.lastArticleFetchEndTime = Date() - completion(.success(())) - } + refresher?.refreshFeeds(webFeeds) } func sendArticleStatus(for account: Account, completion: @escaping ((Result) -> Void)) { @@ -196,6 +206,7 @@ final class LocalAccountDelegate: AccountDelegate { } func accountDidInitialize(_ account: Account) { + self.account = account } func accountWillBeDeleted(_ account: Account) { @@ -208,7 +219,7 @@ final class LocalAccountDelegate: AccountDelegate { // MARK: Suspend and Resume (for iOS) func suspendNetwork() { - refresher.suspend() + refresher?.suspend() } func suspendDatabase() { @@ -216,6 +227,21 @@ final class LocalAccountDelegate: AccountDelegate { } func resume() { - refresher.resume() + refresher?.resume() } } + +extension LocalAccountDelegate: LocalAccountRefresherDelegate { + + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { + refreshProgress.completeTask() + } + + func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) { + self.refreshProgress.clear() + account?.metadata.lastArticleFetchEndTime = Date() + refreshAllCompletion?(.success(())) + refreshAllCompletion = nil + } + +} diff --git a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift index 28c8fa47e..cb8c2f24e 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift @@ -12,19 +12,21 @@ import RSParser import RSWeb import Articles +protocol LocalAccountRefresherDelegate { + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) + func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) +} + final class LocalAccountRefresher { - private var feedCompletionBlock: ((WebFeed) -> Void)? - private var completion: (() -> Void)? private var isSuspended = false + var delegate: LocalAccountRefresherDelegate? private lazy var downloadSession: DownloadSession = { return DownloadSession(delegate: self) }() - public func refreshFeeds(_ feeds: Set, feedCompletionBlock: @escaping (WebFeed) -> Void, completion: @escaping () -> Void) { - self.feedCompletionBlock = feedCompletionBlock - self.completion = completion + public func refreshFeeds(_ feeds: Set) { downloadSession.downloadObjects(feeds as NSSet) } @@ -64,21 +66,21 @@ extension LocalAccountRefresher: DownloadSessionDelegate { guard !data.isEmpty, !isSuspended else { completion() - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return } if let error = error { print("Error downloading \(feed.url) - \(error)") completion() - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return } let dataHash = data.md5String if dataHash == feed.contentHash { completion() - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return } @@ -87,7 +89,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { guard let account = feed.account, let parsedFeed = parsedFeed, error == nil else { completion() - self.feedCompletionBlock?(feed) + self.delegate?.localAccountRefresher(self, requestCompletedFor: feed) return } @@ -100,7 +102,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { feed.contentHash = dataHash } completion() - self.feedCompletionBlock?(feed) + self.delegate?.localAccountRefresher(self, requestCompletedFor: feed) } } @@ -109,7 +111,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { func downloadSession(_ downloadSession: DownloadSession, shouldContinueAfterReceivingData data: Data, representedObject: AnyObject) -> Bool { let feed = representedObject as! WebFeed guard !isSuspended else { - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return false } @@ -118,7 +120,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { } if data.isDefinitelyNotFeed() { - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return false } @@ -127,7 +129,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { if FeedParser.mightBeAbleToParseBasedOnPartialData(parserData) { return true } else { - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return false } } @@ -137,17 +139,21 @@ extension LocalAccountRefresher: DownloadSessionDelegate { func downloadSession(_ downloadSession: DownloadSession, didReceiveUnexpectedResponse response: URLResponse, representedObject: AnyObject) { let feed = representedObject as! WebFeed - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) } func downloadSession(_ downloadSession: DownloadSession, didReceiveNotModifiedResponse: URLResponse, representedObject: AnyObject) { let feed = representedObject as! WebFeed - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) } + func downloadSession(_ downloadSession: DownloadSession, didDiscardDuplicateRepresentedObject representedObject: AnyObject) { + let feed = representedObject as! WebFeed + delegate?.localAccountRefresher(self, requestCompletedFor: feed) + } + func downloadSessionDidCompleteDownloadObjects(_ downloadSession: DownloadSession) { - completion?() - completion = nil + delegate?.localAccountRefresherDidFinish(self) } } diff --git a/submodules/RSWeb b/submodules/RSWeb index 88d634f5f..f54e1cbad 160000 --- a/submodules/RSWeb +++ b/submodules/RSWeb @@ -1 +1 @@ -Subproject commit 88d634f5fd42aab203b6e53c7b551a92b03ffc97 +Subproject commit f54e1cbad3917822e40bc2310ed24bd7cb688a4f