Change LocalAccountRefresher to use a delegate so that it can better handle duplicate feed requests

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, Error>) -> 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, Error>) -> 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, Error>) -> Void)) {
@@ -146,6 +155,12 @@ final class CloudKitAccountDelegate: AccountDelegate {
 	}
 	
 	func importOPML(for account:Account, opmlFile: URL, completion: @escaping (Result<Void, Error>) -> 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
+					self.refreshAll(for: account, downloadFeeds: false)
-						if case .failure(let error) = result {
-							os_log(.error, log: self.log, "Error while doing intial refresh: %@", error.localizedDescription)
-						}
-					}
 				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, Error>) -> 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() }) {
+								self.refresher?.refreshFeeds(webFeeds)
-									account.metadata.lastArticleFetchEndTime = Date()
-									self.refreshProgress.clear()
-									completion(.success(()))
-								}
 
 							case .failure(let error):
-								self.processAccountError(account, error)
+								fail(error)
-								self.refreshProgress.clear()
-								completion(.failure(error))
 							}
 						}
 						
 					case .failure(let error):
-						self.processAccountError(account, error)
+						fail(error)
-						self.refreshProgress.clear()
-						completion(.failure(error))
 					}
 
 				}
 				
 			case .failure(let error):
-				self.processAccountError(account, error)
+				fail(error)
-				self.refreshProgress.clear()
-				completion(.failure(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
+	}
+	
+}

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, Error>) -> Void)? = nil
 	
 	func receiveRemoteNotification(for account: Account, userInfo: [AnyHashable : Any], completion: @escaping () -> Void) {
 		completion()
 	}
 	
 	func refreshAll(for account: Account, completion: @escaping (Result<Void, Error>) -> 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() }) {
+		refresher?.refreshFeeds(webFeeds)
-			self.refreshProgress.clear()
-			account.metadata.lastArticleFetchEndTime = Date()
-			completion(.success(()))
-		}
 	}
 
 	func sendArticleStatus(for account: Account, completion: @escaping ((Result<Void, Error>) -> 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
+	}
+	
+}

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<WebFeed>, feedCompletionBlock: @escaping (WebFeed) -> Void, completion: @escaping () -> Void) {
+	public func refreshFeeds(_ feeds: Set<WebFeed>) {
-		self.feedCompletionBlock = feedCompletionBlock
-		self.completion = completion
 		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?()
+		delegate?.localAccountRefresherDidFinish(self)
-		completion = nil
 	}
 
 }

submodules/RSWeb

@@ -1 +1 @@
-Subproject commit 88d634f5fd42aab203b6e53c7b551a92b03ffc97
+Subproject commit f54e1cbad3917822e40bc2310ed24bd7cb688a4f