Fix a couple bugs with combined refresh/sync progress that gets reported in status bar.

Account/Sources/Account/AccountManager.swift

@@ -88,12 +88,9 @@ public final class AccountManager: UnreadCountProvider {
 		}
 		return false
 	}
-	
-	public var combinedRefreshProgress: CombinedRefreshProgress {
-		let downloadProgressArray = activeAccounts.map { $0.refreshProgress }
-		return CombinedRefreshProgress(downloadProgressArray: downloadProgressArray)
-	}
-	
+
+	public let combinedRefreshProgress = CombinedRefreshProgress()
+
 	public init(accountsFolder: String) {
 		self.accountsFolder = accountsFolder
 		
@@ -243,12 +240,17 @@ public final class AccountManager: UnreadCountProvider {
 		}
 	}
 
-	public func refreshAll(errorHandler: @escaping (Error) -> Void, completion: (() -> Void)? = nil) {
-		guard let reachability = try? Reachability(hostname: "apple.com"), reachability.connection != .unavailable else { return }
+	public func refreshAll(errorHandler: ((Error) -> Void)? = nil, completion: (() -> Void)? = nil) {
+
+		guard let reachability = try? Reachability(hostname: "apple.com"), reachability.connection != .unavailable else {
+			return
+		}
+
+		combinedRefreshProgress.reset()
 
 		let group = DispatchGroup()
 		
-		activeAccounts.forEach { account in
+		for account in activeAccounts {
 			group.enter()
 			account.refreshAll() { result in
 				group.leave()
@@ -256,43 +258,16 @@ public final class AccountManager: UnreadCountProvider {
 				case .success:
 					break
 				case .failure(let error):
-					errorHandler(error)
+					errorHandler?(error)
 				}
 			}
 		}
 		
 		group.notify(queue: DispatchQueue.main) {
+			self.combinedRefreshProgress.reset()
 			completion?()
 		}
 	}
-	
-	public func refreshAll(completion: (() -> Void)? = nil) {
-		guard let reachability = try? Reachability(hostname: "apple.com"), reachability.connection != .unavailable else { return }
-
-		var syncErrors = [AccountSyncError]()
-		let group = DispatchGroup()
-		
-		activeAccounts.forEach { account in
-			group.enter()
-			account.refreshAll() { result in
-				group.leave()
-				switch result {
-				case .success:
-					break
-				case .failure(let error):
-					syncErrors.append(AccountSyncError(account: account, error: error))
-				}
-			}
-		}
-		
-		group.notify(queue: DispatchQueue.main) {
-			if syncErrors.count > 0 {
-				NotificationCenter.default.post(Notification(name: .AccountsDidFailToSyncWithErrors, object: self, userInfo: [Account.UserInfoKey.syncErrors: syncErrors]))
-			}
-			completion?()
-		}
-		
-	}
 
 	public func sendArticleStatusAll(completion: (() -> Void)? = nil) {
 		let group = DispatchGroup()

Account/Sources/Account/AccountSyncError.swift

@@ -1,29 +0,0 @@
-//
-//  AccountSyncError.swift
-//  Account
-//
-//  Created by Stuart Breckenridge on 24/7/20.
-//  Copyright © 2020 Ranchero Software, LLC. All rights reserved.
-//
-
-import Foundation
-import os.log
-
-public extension Notification.Name {
-	static let AccountsDidFailToSyncWithErrors = Notification.Name("AccountsDidFailToSyncWithErrors")
-}
-
-public struct AccountSyncError {
-	
-	private static let log = OSLog(subsystem: Bundle.main.bundleIdentifier!, category: "Application")
-	public let account: Account
-	public let error: Error
-	
-	init(account: Account, error: Error) {
-		self.account = account
-		self.error = error
-		os_log(.error, log: AccountSyncError.log, "%@", error.localizedDescription)
-	}
-	
-}
-

Account/Sources/Account/CloudKit/CloudKitAccountDelegate.swift

@@ -87,6 +87,8 @@ final class CloudKitAccountDelegate: AccountDelegate {
 			return
 		}
 
+		syncProgress.reset()
+		
 		let reachability = SCNetworkReachabilityCreateWithName(nil, "apple.com")
 		var flags = SCNetworkReachabilityFlags()
 		guard SCNetworkReachabilityGetFlags(reachability!, &flags), flags.contains(.reachable) else {
@@ -511,7 +513,7 @@ private extension CloudKitAccountDelegate {
 
 		func fail(_ error: Error) {
 			self.processAccountError(account, error)
-			self.syncProgress.clear()
+			self.syncProgress.reset()
 			completion(.failure(error))
 		}
 
@@ -529,7 +531,7 @@ private extension CloudKitAccountDelegate {
 					case .success:
 
 						self.combinedRefresh(account, webFeeds) {
-							self.syncProgress.clear()
+							self.syncProgress.reset()
 							account.metadata.lastArticleFetchEndTime = Date()
 						}
 
@@ -549,7 +551,7 @@ private extension CloudKitAccountDelegate {
 
 		func fail(_ error: Error) {
 			self.processAccountError(account, error)
-			self.syncProgress.clear()
+			self.syncProgress.reset()
 			completion(.failure(error))
 		}
 
@@ -566,7 +568,7 @@ private extension CloudKitAccountDelegate {
 						self.syncProgress.completeTask()
 						self.combinedRefresh(account, webFeeds) {
 							self.sendArticleStatus(for: account, showProgress: true) { _ in
-								self.syncProgress.clear()
+								self.syncProgress.reset()
 								account.metadata.lastArticleFetchEndTime = Date()
 								completion(.success(()))
 							}

Account/Sources/Account/CombinedRefreshProgress.swift

@@ -9,34 +9,86 @@
 import Foundation
 import RSWeb
 
-// Combines the refresh progress of multiple accounts into one struct,
-// for use by refresh status view and so on.
+extension Notification.Name {
+	public static let combinedRefreshProgressDidChange = Notification.Name("combinedRefreshProgressDidChange")
+}
 
-public struct CombinedRefreshProgress {
+/// Combine the refresh progress of multiple accounts into one place,
+/// for use by refresh status view and so on.
+public final class CombinedRefreshProgress {
 
-	public let numberOfTasks: Int
-	public let numberRemaining: Int
-	public let numberCompleted: Int
-	public let isComplete: Bool
+	public private(set) var numberOfTasks = 0
+	public private(set) var numberRemaining = 0
+	public private(set) var numberCompleted = 0
 
-	init(numberOfTasks: Int, numberRemaining: Int, numberCompleted: Int) {
-		self.numberOfTasks = max(numberOfTasks, 0)
-		self.numberRemaining = max(numberRemaining, 0)
-		self.numberCompleted = max(numberCompleted, 0)
-		self.isComplete = numberRemaining < 1
+	public var isComplete: Bool {
+		numberRemaining < 1
 	}
 
-	public init(downloadProgressArray: [DownloadProgress]) {
-		var numberOfTasks = 0
-		var numberRemaining = 0
-		var numberCompleted = 0
+	init() {
 
-		for downloadProgress in downloadProgressArray {
-			numberOfTasks += downloadProgress.numberOfTasks
-			numberRemaining += downloadProgress.numberRemaining
-			numberCompleted += downloadProgress.numberCompleted
+		NotificationCenter.default.addObserver(self, selector: #selector(refreshProgressDidChange(_:)), name: .DownloadProgressDidChange, object: nil)
+	}
+
+	func reset() {
+
+		let didMakeChange = numberOfTasks != 0 || numberRemaining != 0 || numberCompleted != 0
+
+		numberOfTasks = 0
+		numberRemaining = 0
+		numberCompleted = 0
+
+		if didMakeChange {
+			postDidChangeNotification()
+		}
+	}
+
+	@objc func refreshProgressDidChange(_ notification: Notification) {
+
+		var updatedNumberOfTasks = 0
+		var updatedNumberRemaining = 0
+		var updatedNumberCompleted = 0
+
+		var didMakeChange = false
+
+		let downloadProgresses = AccountManager.shared.activeAccounts.map { $0.refreshProgress }
+		for downloadProgress in downloadProgresses {
+			updatedNumberOfTasks += downloadProgress.numberOfTasks
+			updatedNumberRemaining += downloadProgress.numberRemaining
+			updatedNumberCompleted += downloadProgress.numberCompleted
 		}
 
-		self.init(numberOfTasks: numberOfTasks, numberRemaining: numberRemaining, numberCompleted: numberCompleted)
+		if updatedNumberOfTasks > numberOfTasks {
+			numberOfTasks = updatedNumberOfTasks
+			didMakeChange = true
+		}
+
+		assert(updatedNumberRemaining <= numberOfTasks)
+		updatedNumberRemaining = max(updatedNumberRemaining, numberRemaining)
+		updatedNumberRemaining = min(updatedNumberRemaining, numberOfTasks)
+		if updatedNumberRemaining != numberRemaining {
+			numberRemaining = updatedNumberRemaining
+			didMakeChange = true
+		}
+
+		assert(updatedNumberCompleted <= numberOfTasks)
+		updatedNumberCompleted = max(updatedNumberCompleted, numberCompleted)
+		updatedNumberCompleted = min(updatedNumberCompleted, numberOfTasks)
+		if updatedNumberCompleted != numberCompleted {
+			numberCompleted = updatedNumberCompleted
+			didMakeChange = true
+		}
+
+		if didMakeChange {
+			postDidChangeNotification()
+		}
+	}
+}
+
+private extension CombinedRefreshProgress {
+
+	func postDidChangeNotification() {
+
+		NotificationCenter.default.post(name: .combinedRefreshProgressDidChange, object: self)
 	}
 }

Account/Sources/Account/Feedbin/FeedbinAccountDelegate.swift

@@ -80,7 +80,8 @@ final class FeedbinAccountDelegate: AccountDelegate {
 	}
 
 	func refreshAll(for account: Account, completion: @escaping (Result<Void, Error>) -> Void) {
-		
+
+		refreshProgress.reset()
 		refreshProgress.addToNumberOfTasksAndRemaining(5)
 		
 		refreshAccount(account) { result in
@@ -93,7 +94,7 @@ final class FeedbinAccountDelegate: AccountDelegate {
 						completion(.success(()))
 					case .failure(let error):
 						DispatchQueue.main.async {
-							self.refreshProgress.clear()
+							self.refreshProgress.reset()
 							let wrappedError = AccountError.wrappedError(error: error, account: account)
 							completion(.failure(wrappedError))
 						}
@@ -102,7 +103,7 @@ final class FeedbinAccountDelegate: AccountDelegate {
 				
 			case .failure(let error):
 				DispatchQueue.main.async {
-					self.refreshProgress.clear()
+					self.refreshProgress.reset()
 					let wrappedError = AccountError.wrappedError(error: error, account: account)
 					completion(.failure(wrappedError))
 				}
@@ -719,7 +720,7 @@ private extension FeedbinAccountDelegate {
 									case .success:
 										
 										DispatchQueue.main.async {
-											self.refreshProgress.clear()
+											self.refreshProgress.reset()
 											completion(.success(()))
 										}
 										

Account/Sources/Account/Feedly/FeedlyAccountDelegate.swift

@@ -123,10 +123,12 @@ final class FeedlyAccountDelegate: AccountDelegate {
 			return
 		}
 		
+		refreshProgress.reset()
+
 		let log = self.log
-		
+
 		let syncAllOperation = FeedlySyncAllOperation(account: account, feedlyUserId: credentials.username, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetchStartTime, downloadProgress: refreshProgress, log: log)
-		
+
 		syncAllOperation.downloadProgress = refreshProgress
 		
 		let date = Date()
@@ -138,7 +140,7 @@ final class FeedlyAccountDelegate: AccountDelegate {
 			
 			os_log(.debug, log: log, "Sync took %{public}.3f seconds", -date.timeIntervalSinceNow)
 			completion(result)
-			self?.refreshProgress.clear()
+			self?.refreshProgress.reset()
 		}
 		
 		currentSyncAllOperation = syncAllOperation

Account/Sources/Account/NewsBlur/NewsBlurAccountDelegate.swift

@@ -63,6 +63,9 @@ final class NewsBlurAccountDelegate: AccountDelegate {
 	}
 	
 	func refreshAll(for account: Account, completion: @escaping (Result<Void, Error>) -> ()) {
+
+		refreshProgress.reset()
+		
 		self.refreshProgress.addToNumberOfTasksAndRemaining(4)
 
 		refreshFeeds(for: account) { result in
@@ -91,7 +94,7 @@ final class NewsBlurAccountDelegate: AccountDelegate {
 
 									case .failure(let error):
 										DispatchQueue.main.async {
-											self.refreshProgress.clear()
+											self.refreshProgress.reset()
 											let wrappedError = AccountError.wrappedError(error: error, account: account)
 											completion(.failure(wrappedError))
 										}

Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift

@@ -105,6 +105,8 @@ final class ReaderAPIAccountDelegate: AccountDelegate {
 	}
 	
 	func refreshAll(for account: Account, completion: @escaping (Result<Void, Error>) -> Void) {
+
+		refreshProgress.reset()
 		refreshProgress.addToNumberOfTasksAndRemaining(6)
 		
 		refreshAccount(account) { result in
@@ -120,7 +122,7 @@ final class ReaderAPIAccountDelegate: AccountDelegate {
 								self.refreshArticleStatus(for: account) { _ in
 									self.refreshProgress.completeTask()
 									self.refreshMissingArticles(account) {
-										self.refreshProgress.clear()
+										self.refreshProgress.reset()
 										DispatchQueue.main.async {
 											completion(.success(()))
 										}
@@ -135,7 +137,7 @@ final class ReaderAPIAccountDelegate: AccountDelegate {
 
 			case .failure(let error):
 				DispatchQueue.main.async {
-					self.refreshProgress.clear()
+					self.refreshProgress.reset()
 					
 					let wrappedError = AccountError.wrappedError(error: error, account: account)
 					if wrappedError.isCredentialsError, let basicCredentials = try? account.retrieveCredentials(type: .readerBasic), let endpoint = account.endpointURL {
@@ -405,7 +407,7 @@ final class ReaderAPIAccountDelegate: AccountDelegate {
 			case .success(let feedSpecifiers):
 				let feedSpecifiers = feedSpecifiers.filter { !$0.urlString.contains("json") }
 				guard let bestFeedSpecifier = FeedSpecifier.bestFeed(in: feedSpecifiers) else {
-					self.refreshProgress.clear()
+					self.refreshProgress.reset()
 					completion(.failure(AccountError.createErrorNotFound))
 					return
 				}
@@ -431,7 +433,7 @@ final class ReaderAPIAccountDelegate: AccountDelegate {
 					
 				}
 			case .failure:
-				self.refreshProgress.clear()
+				self.refreshProgress.reset()
 				completion(.failure(AccountError.createErrorNotFound))
 			}
 			
@@ -961,7 +963,7 @@ private extension ReaderAPIAccountDelegate {
 					self.refreshArticleStatus(for: account) { _ in
 						self.refreshProgress.completeTask()
 						self.refreshMissingArticles(account) {
-							self.refreshProgress.clear()
+							self.refreshProgress.reset()
 							DispatchQueue.main.async {
 								completion(.success(feed))
 							}

Mac/MainWindow/Sidebar/SidebarStatusBarView.swift

@@ -21,11 +21,6 @@ final class SidebarStatusBarView: NSView {
 
 	private var isAnimatingProgress = false
 
-	private var progress: CombinedRefreshProgress? = nil {
-		didSet {
-			CoalescingQueue.standard.add(self, #selector(updateUI))
-		}
-	}
 	override var isFlipped: Bool {
 		return true
 	}
@@ -39,25 +34,20 @@ final class SidebarStatusBarView: NSView {
 		progressLabel.font = NSFont.monospacedDigitSystemFont(ofSize: progressLabelFontSize, weight: NSFont.Weight.regular)
 		progressLabel.stringValue = ""		
 		
-		NotificationCenter.default.addObserver(self, selector: #selector(progressDidChange(_:)), name: .AccountRefreshProgressDidChange, object: nil)
+		NotificationCenter.default.addObserver(self, selector: #selector(progressDidChange(_:)), name: .combinedRefreshProgressDidChange, object: nil)
 	}
 
 	@objc func updateUI() {
 
-		guard let progress = progress else {
-			stopProgressIfNeeded()
-			return
-		}
-
-		updateProgressIndicator(progress)
-		updateProgressLabel(progress)
+		updateProgressIndicator()
+		updateProgressLabel()
 	}
 
 	// MARK: Notifications
 
 	@objc dynamic func progressDidChange(_ notification: Notification) {
 
-		progress = AccountManager.shared.combinedRefreshProgress
+		CoalescingQueue.standard.add(self, #selector(updateUI))
 	}
 }
 
@@ -73,13 +63,13 @@ private extension SidebarStatusBarView {
 			return
 		}
 		isAnimatingProgress = false
-		self.progressIndicator.stopAnimation(self)
+		progressIndicator.stopAnimation(self)
 		progressIndicator.isHidden = true
 		progressLabel.isHidden = true
 
 		superview?.layoutSubtreeIfNeeded()
 
-		NSAnimationContext.runAnimationGroup{ (context) in
+		NSAnimationContext.runAnimationGroup { context in
 			context.duration = SidebarStatusBarView.animationDuration
 			context.allowsImplicitAnimation = true
 			bottomConstraint.constant = -(heightConstraint.constant)
@@ -99,7 +89,7 @@ private extension SidebarStatusBarView {
 
 		superview?.layoutSubtreeIfNeeded()
 
-		NSAnimationContext.runAnimationGroup{ (context) in
+		NSAnimationContext.runAnimationGroup { context in
 			context.duration = SidebarStatusBarView.animationDuration
 			context.allowsImplicitAnimation = true
 			bottomConstraint.constant = 0
@@ -107,7 +97,9 @@ private extension SidebarStatusBarView {
 		}
 	}
 
-	func updateProgressIndicator(_ progress: CombinedRefreshProgress) {
+	func updateProgressIndicator() {
+
+		let progress = AccountManager.shared.combinedRefreshProgress
 
 		if progress.isComplete {
 			stopProgressIfNeeded()
@@ -127,7 +119,9 @@ private extension SidebarStatusBarView {
 		}
 	}
 
-	func updateProgressLabel(_ progress: CombinedRefreshProgress) {
+	func updateProgressLabel() {
+
+		let progress = AccountManager.shared.combinedRefreshProgress
 
 		if progress.isComplete {
 			progressLabel.stringValue = ""
@@ -135,8 +129,8 @@ private extension SidebarStatusBarView {
 		}
 
 		let formatString = NSLocalizedString("%@ of %@", comment: "Status bar progress")
-		let s = NSString(format: formatString as NSString, NSNumber(value: progress.numberCompleted), NSNumber(value: progress.numberOfTasks))
+		let s = String(format: formatString, NSNumber(value: progress.numberCompleted), NSNumber(value: progress.numberOfTasks))
 
-		progressLabel.stringValue = s as String
+		progressLabel.stringValue = s
 	}
 }

RSWeb/Sources/RSWeb/DownloadProgress.swift

@@ -82,8 +82,9 @@ public final class DownloadProgress {
 		}
 	}
 	
-	public func clear() {
+	public func reset() {
 		assert(Thread.isMainThread)
+		numberRemaining = 0
 		numberOfTasks = 0
 	}
 }

Shared/Timer/AccountRefreshTimer.swift

@@ -9,7 +9,7 @@
 import Foundation
 import Account
 
-class AccountRefreshTimer {
+final class AccountRefreshTimer {
 	
 	var shuttingDown = false
 
@@ -73,8 +73,6 @@ class AccountRefreshTimer {
 		lastTimedRefresh = Date()
 		update()
 		
-		//AccountManager.shared.refreshAll(errorHandler: ErrorHandler.log)
-		AccountManager.shared.refreshAll(completion: nil)
+		AccountManager.shared.refreshAll()
 	}
-	
 }