diff --git a/Frameworks/Account/Account.swift b/Frameworks/Account/Account.swift index cc4fc37f4..8086be584 100644 --- a/Frameworks/Account/Account.swift +++ b/Frameworks/Account/Account.swift @@ -715,7 +715,7 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, webFeedDictionariesNeedUpdate = true } - func update(_ webFeed: WebFeed, with parsedFeed: ParsedFeed, _ completion: @escaping DatabaseCompletionBlock) { + func update(_ webFeed: WebFeed, with parsedFeed: ParsedFeed, _ completion: @escaping UpdateArticlesCompletionBlock) { // Used only by an On My Mac or iCloud account. precondition(Thread.isMainThread) precondition(type == .onMyMac || type == .cloudKit) @@ -723,14 +723,14 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, webFeed.takeSettings(from: parsedFeed) let parsedItems = parsedFeed.items guard !parsedItems.isEmpty else { - completion(nil) + completion(.success(NewAndUpdatedArticles())) return } update(webFeed.webFeedID, with: parsedItems, completion: completion) } - func update(_ webFeedID: String, with parsedItems: Set, completion: @escaping DatabaseCompletionBlock) { + func update(_ webFeedID: String, with parsedItems: Set, completion: @escaping UpdateArticlesCompletionBlock) { // Used only by an On My Mac or iCloud account. precondition(Thread.isMainThread) precondition(type == .onMyMac || type == .cloudKit) @@ -739,9 +739,9 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, switch updateArticlesResult { case .success(let newAndUpdatedArticles): self.sendNotificationAbout(newAndUpdatedArticles) - completion(nil) + completion(.success(newAndUpdatedArticles)) case .failure(let databaseError): - completion(databaseError) + completion(.failure(databaseError)) } } } @@ -800,39 +800,45 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, /// Mark articleIDs statuses based on statusKey and flag. /// Will create statuses in the database and in memory as needed. Sends a .StatusesDidChange notification. - func mark(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: DatabaseCompletionBlock? = nil) { + /// Returns a set of new article statuses. + func markAndFetchNew(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: ArticleIDsCompletionBlock? = nil) { guard !articleIDs.isEmpty else { - completion?(nil) + completion?(.success(Set())) return } - database.mark(articleIDs: articleIDs, statusKey: statusKey, flag: flag) { error in - if let error = error { - completion?(error) - return + database.markAndFetchNew(articleIDs: articleIDs, statusKey: statusKey, flag: flag) { result in + switch result { + case .success(let newArticleStatusIDs): + self.noteStatusesForArticleIDsDidChange(articleIDs) + completion?(.success(newArticleStatusIDs)) + case .failure(let databaseError): + completion?(.failure(databaseError)) } - self.noteStatusesForArticleIDsDidChange(articleIDs) - completion?(nil) } } /// Mark articleIDs as read. Will create statuses in the database and in memory as needed. Sends a .StatusesDidChange notification. - func markAsRead(_ articleIDs: Set, completion: DatabaseCompletionBlock? = nil) { - mark(articleIDs: articleIDs, statusKey: .read, flag: true, completion: completion) + /// Returns a set of new article statuses. + func markAsRead(_ articleIDs: Set, completion: ArticleIDsCompletionBlock? = nil) { + markAndFetchNew(articleIDs: articleIDs, statusKey: .read, flag: true, completion: completion) } /// Mark articleIDs as unread. Will create statuses in the database and in memory as needed. Sends a .StatusesDidChange notification. - func markAsUnread(_ articleIDs: Set, completion: DatabaseCompletionBlock? = nil) { - mark(articleIDs: articleIDs, statusKey: .read, flag: false, completion: completion) + /// Returns a set of new article statuses. + func markAsUnread(_ articleIDs: Set, completion: ArticleIDsCompletionBlock? = nil) { + markAndFetchNew(articleIDs: articleIDs, statusKey: .read, flag: false, completion: completion) } /// Mark articleIDs as starred. Will create statuses in the database and in memory as needed. Sends a .StatusesDidChange notification. - func markAsStarred(_ articleIDs: Set, completion: DatabaseCompletionBlock? = nil) { - mark(articleIDs: articleIDs, statusKey: .starred, flag: true, completion: completion) + /// Returns a set of new article statuses. + func markAsStarred(_ articleIDs: Set, completion: ArticleIDsCompletionBlock? = nil) { + markAndFetchNew(articleIDs: articleIDs, statusKey: .starred, flag: true, completion: completion) } /// Mark articleIDs as unstarred. Will create statuses in the database and in memory as needed. Sends a .StatusesDidChange notification. - func markAsUnstarred(_ articleIDs: Set, completion: DatabaseCompletionBlock? = nil) { - mark(articleIDs: articleIDs, statusKey: .starred, flag: false, completion: completion) + /// Returns a set of new article statuses. + func markAsUnstarred(_ articleIDs: Set, completion: ArticleIDsCompletionBlock? = nil) { + markAndFetchNew(articleIDs: articleIDs, statusKey: .starred, flag: false, completion: completion) } /// Empty caches that can reasonably be emptied. Call when the app goes in the background, for instance. @@ -887,7 +893,7 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, public func debugDropConditionalGetInfo() { #if DEBUG - flattenedWebFeeds().forEach{ $0.debugDropConditionalGetInfo() } + flattenedWebFeeds().forEach{ $0.dropConditionalGetInfo() } #endif } diff --git a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift index d6f03a63e..ca7ce0c0c 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift @@ -13,6 +13,7 @@ import SyncDatabase import RSCore import RSParser import Articles +import ArticlesDatabase import RSWeb public enum CloudKitAccountDelegateError: String, Error { @@ -34,7 +35,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 +51,7 @@ final class CloudKitAccountDelegate: AccountDelegate { var accountMetadata: AccountMetadata? var refreshProgress = DownloadProgress(numberOfTasks: 0) - + init(dataFolder: String) { accountZone = CloudKitAccountZone(container: container) articlesZone = CloudKitArticlesZone(container: container) @@ -76,6 +83,7 @@ final class CloudKitAccountDelegate: AccountDelegate { completion(.success(())) return } + refreshAll(for: account, downloadFeeds: true, completion: completion) } @@ -90,12 +98,12 @@ final class CloudKitAccountDelegate: AccountDelegate { return } - let starredArticleIDs = syncStatuses.filter({ $0.key == .starred && $0.flag == true }).map({ $0.articleID }) - account.fetchArticlesAsync(.articleIDs(Set(starredArticleIDs))) { result in + let articleIDs = syncStatuses.map({ $0.articleID }) + account.fetchArticlesAsync(.articleIDs(Set(articleIDs))) { result in - func processWithArticles(_ starredArticles: Set
) { + func processWithArticles(_ articles: Set
) { - self.articlesZone.sendArticleStatus(syncStatuses, starredArticles: starredArticles) { result in + self.articlesZone.sendArticleStatus(syncStatuses, articles: articles) { result in switch result { case .success: self.database.deleteSelectedForProcessing(syncStatuses.map({ $0.articleID }) ) @@ -111,8 +119,8 @@ final class CloudKitAccountDelegate: AccountDelegate { } switch result { - case .success(let starredArticles): - processWithArticles(starredArticles) + case .success(let articles): + processWithArticles(articles) case .failure(let databaseError): completion(.failure(databaseError)) } @@ -146,6 +154,11 @@ final class CloudKitAccountDelegate: AccountDelegate { } func importOPML(for account:Account, opmlFile: URL, completion: @escaping (Result) -> Void) { + guard refreshProgress.isComplete else { + completion(.success(())) + return + } + var fileData: Data? do { @@ -463,8 +476,13 @@ 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) + articlesZone.delegate = CloudKitArticlesZoneDelegate(account: account, + database: database, + articlesZone: articlesZone, + refreshProgress: refreshProgress) // Check to see if this is a new account and initialize anything we need if account.externalID == nil { @@ -472,11 +490,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) { _ in } case .failure(let error): os_log(.error, log: self.log, "Error adding account container: %@", error.localizedDescription) } @@ -521,6 +535,12 @@ private extension CloudKitAccountDelegate { let intialWebFeedsCount = downloadFeeds ? account.flattenedWebFeeds().count : 0 refreshProgress.addToNumberOfTasksAndRemaining(3 + intialWebFeedsCount) + func fail(_ error: Error) { + self.processAccountError(account, error) + self.refreshProgress.clear() + completion(.failure(error)) + } + BatchUpdate.shared.start() accountZone.fetchChangesInZone() { result in BatchUpdate.shared.end() @@ -549,31 +569,34 @@ private extension CloudKitAccountDelegate { return } - self.refresher.refreshFeeds(webFeeds, feedCompletionBlock: { _ in self.refreshProgress.completeTask() }) { + self.refresher.refreshFeeds(webFeeds) { + account.metadata.lastArticleFetchEndTime = Date() - self.refreshProgress.clear() - completion(.success(())) + + self.sendArticleStatus(for: account) { result in + switch result { + case .success: + completion(.success(())) + case .failure(let 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) } } case .failure(let error): - self.processAccountError(account, error) - self.refreshProgress.clear() - completion(.failure(error)) + fail(error) } } } @@ -588,3 +611,24 @@ private extension CloudKitAccountDelegate { } } + +extension CloudKitAccountDelegate: LocalAccountRefresherDelegate { + + func localAccountRefresher(_ refresher: LocalAccountRefresher, didProcess newAndUpdatedArticles: NewAndUpdatedArticles) { + if let newArticles = newAndUpdatedArticles.newArticles { + let syncStatuses = newArticles.map { article in + return SyncStatus(articleID: article.articleID, key: .read, flag: false) + } + database.insertStatuses(syncStatuses) + } + } + + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { + refreshProgress.completeTask() + } + + func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) { + refreshProgress.clear() + } + +} diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift index 080799ed8..02c84d78f 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift @@ -48,6 +48,7 @@ final class CloudKitArticlesZone: CloudKitZone { struct CloudKitArticleStatus { static let recordType = "ArticleStatus" struct Fields { + static let webFeedExternalID = "webFeedExternalID" static let read = "read" static let starred = "starred" static let userDeleted = "userDeleted" @@ -81,8 +82,11 @@ final class CloudKitArticlesZone: CloudKitZone { } } - func sendArticleStatus(_ syncStatuses: [SyncStatus], starredArticles: Set
, completion: @escaping ((Result) -> Void)) { - var records = makeStatusRecords(syncStatuses) + func sendArticleStatus(_ syncStatuses: [SyncStatus], articles: Set
, completion: @escaping ((Result) -> Void)) { + + var records = makeStatusRecords(syncStatuses, articles) + + let starredArticles = articles.filter({ $0.status.starred == true }) makeArticleRecordsIfNecessary(starredArticles) { result in switch result { case .success(let articleRecords): @@ -92,11 +96,11 @@ final class CloudKitArticlesZone: CloudKitZone { case .success: completion(.success(())) case .failure(let error): - self.handleSendArticleStatusError(error, syncStatuses: syncStatuses, starredArticles: starredArticles, completion: completion) + self.handleSendArticleStatusError(error, syncStatuses: syncStatuses, starredArticles: articles, completion: completion) } } case .failure(let error): - self.handleSendArticleStatusError(error, syncStatuses: syncStatuses, starredArticles: starredArticles, completion: completion) + self.handleSendArticleStatusError(error, syncStatuses: syncStatuses, starredArticles: articles, completion: completion) } } } @@ -106,7 +110,7 @@ final class CloudKitArticlesZone: CloudKitZone { self.createZoneRecord() { result in switch result { case .success: - self.sendArticleStatus(syncStatuses, starredArticles: starredArticles, completion: completion) + self.sendArticleStatus(syncStatuses, articles: starredArticles, completion: completion) case .failure(let error): completion(.failure(error)) } @@ -120,7 +124,13 @@ final class CloudKitArticlesZone: CloudKitZone { private extension CloudKitArticlesZone { - func makeStatusRecords(_ syncStatuses: [SyncStatus]) -> [CKRecord] { + func makeStatusRecords(_ syncStatuses: [SyncStatus], _ articles: Set
) -> [CKRecord] { + + var articleDict = [String: Article]() + for article in articles { + articleDict[article.articleID] = article + } + var records = [String: CKRecord]() for status in syncStatuses { @@ -132,6 +142,10 @@ private extension CloudKitArticlesZone { records[status.articleID] = record } + if let webFeedExternalID = articleDict[status.articleID]?.webFeed?.externalID { + record![CloudKitArticleStatus.Fields.webFeedExternalID] = webFeedExternalID + } + switch status.key { case .read: record![CloudKitArticleStatus.Fields.read] = status.flag ? "1" : "0" @@ -147,7 +161,6 @@ private extension CloudKitArticlesZone { func makeArticleRecordsIfNecessary(_ articles: Set
, completion: @escaping ((Result<[CKRecord], Error>) -> Void)) { let group = DispatchGroup() - var errorOccurred = false var records = [CKRecord]() for article in articles { @@ -164,9 +177,8 @@ private extension CloudKitArticlesZone { if !recordFound { records.append(contentsOf: self.makeArticleRecords(article)) } - case .failure(let error): - errorOccurred = true - os_log(.error, log: self.log, "Error occurred while checking for existing articles: %@", error.localizedDescription) + case .failure: + records.append(contentsOf: self.makeArticleRecords(article)) } group.leave() } @@ -174,11 +186,7 @@ private extension CloudKitArticlesZone { } group.notify(queue: DispatchQueue.main) { - if errorOccurred { - completion(.failure(CloudKitZoneError.unknown)) - } else { - completion(.success(records)) - } + completion(.success(records)) } } diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift index 6b712b932..1e1ed466c 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift @@ -9,8 +9,11 @@ import Foundation import os.log import RSParser +import RSWeb import CloudKit import SyncDatabase +import Articles +import ArticlesDatabase class CloudKitArticlesZoneDelegate: CloudKitZoneDelegate { @@ -19,11 +22,19 @@ class CloudKitArticlesZoneDelegate: CloudKitZoneDelegate { weak var account: Account? var database: SyncDatabase weak var articlesZone: CloudKitArticlesZone? + weak var refreshProgress: DownloadProgress? + + private lazy var refresher: LocalAccountRefresher = { + let refresher = LocalAccountRefresher() + refresher.delegate = self + return refresher + }() - init(account: Account, database: SyncDatabase, articlesZone: CloudKitArticlesZone) { + init(account: Account, database: SyncDatabase, articlesZone: CloudKitArticlesZone, refreshProgress: DownloadProgress?) { self.account = account self.database = database self.articlesZone = articlesZone + self.refreshProgress = refreshProgress } func cloudKitDidChange(record: CKRecord) { @@ -82,8 +93,40 @@ private extension CloudKitArticlesZoneDelegate { let group = DispatchGroup() group.enter() - account?.markAsUnread(updateableUnreadArticleIDs) { _ in - group.leave() + account?.markAsUnread(updateableUnreadArticleIDs) { result in + switch result { + case .success(let newArticleStatusIDs): + + if newArticleStatusIDs.isEmpty { + group.leave() + } else { + + var webFeedExternalIDDict = [String: String]() + for record in records { + if let webFeedExternalID = record[CloudKitArticlesZone.CloudKitArticleStatus.Fields.webFeedExternalID] as? String { + webFeedExternalIDDict[record.externalID] = webFeedExternalID + } + } + + var webFeeds = Set() + for newArticleStatusID in newArticleStatusIDs { + if let webFeedExternalID = webFeedExternalIDDict[newArticleStatusID], + let webFeed = self.account?.existingWebFeed(withExternalID: webFeedExternalID) { + webFeeds.insert(webFeed) + } + } + + webFeeds.forEach { $0.dropConditionalGetInfo() } + self.refreshProgress?.addToNumberOfTasksAndRemaining(webFeeds.count) + self.refresher.refreshFeeds(webFeeds) { + group.leave() + } + + } + + case .failure: + group.leave() + } } group.enter() @@ -104,9 +147,9 @@ private extension CloudKitArticlesZoneDelegate { for receivedStarredArticle in receivedStarredArticles { if let parsedItem = makeParsedItem(receivedStarredArticle) { group.enter() - self.account?.update(parsedItem.feedURL, with: Set([parsedItem])) { databaseError in + self.account?.update(parsedItem.feedURL, with: Set([parsedItem])) { result in group.leave() - if let databaseError = databaseError { + if case .failure(let databaseError) = result { os_log(.error, log: self.log, "Error occurred while storing starred items: %@", databaseError.localizedDescription) } } @@ -119,7 +162,6 @@ private extension CloudKitArticlesZoneDelegate { } - func makeParsedItem(_ articleRecord: CKRecord) -> ParsedItem? { var parsedAuthors = Set() @@ -160,3 +202,17 @@ private extension CloudKitArticlesZoneDelegate { } } + +extension CloudKitArticlesZoneDelegate: LocalAccountRefresherDelegate { + + func localAccountRefresher(_ refresher: LocalAccountRefresher, didProcess newAndUpdatedArticles: NewAndUpdatedArticles) { + } + + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { + refreshProgress?.completeTask() + } + + func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) { + } + +} diff --git a/Frameworks/Account/FeedFinder/FeedFinder.swift b/Frameworks/Account/FeedFinder/FeedFinder.swift index 8044295fa..6c00ebdf0 100644 --- a/Frameworks/Account/FeedFinder/FeedFinder.swift +++ b/Frameworks/Account/FeedFinder/FeedFinder.swift @@ -14,7 +14,7 @@ import RSCore class FeedFinder { static func find(url: URL, completion: @escaping (Result, Error>) -> Void) { - downloadUsingCache(url) { (data, response, error) in + downloadAddingToCache(url) { (data, response, error) in if response?.forcedStatusCode == 404 { completion(.failure(AccountError.createErrorNotFound)) return diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift index 02dc42043..0be82fa95 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift @@ -124,15 +124,19 @@ final class FeedlyIngestStarredArticleIdsOperation: FeedlyOperation { let results = StarredStatusResults() group.enter() - account.markAsStarred(remoteStarredArticleIDs) { error in - results.markAsStarredError = error + account.markAsStarred(remoteStarredArticleIDs) { result in + if case .failure(let error) = result { + results.markAsStarredError = error + } group.leave() } let deltaUnstarredArticleIDs = localStarredArticleIDs.subtracting(remoteStarredArticleIDs) group.enter() - account.markAsUnstarred(deltaUnstarredArticleIDs) { error in - results.markAsUnstarredError = error + account.markAsUnstarred(deltaUnstarredArticleIDs) { result in + if case .failure(let error) = result { + results.markAsUnstarredError = error + } group.leave() } diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift index 669a9672d..985b51006 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift @@ -124,15 +124,19 @@ final class FeedlyIngestUnreadArticleIdsOperation: FeedlyOperation { let results = ReadStatusResults() group.enter() - account.markAsUnread(remoteUnreadArticleIDs) { error in - results.markAsUnreadError = error + account.markAsUnread(remoteUnreadArticleIDs) { result in + if case .failure(let error) = result { + results.markAsUnreadError = error + } group.leave() } let articleIDsToMarkRead = localUnreadArticleIDs.subtracting(remoteUnreadArticleIDs) group.enter() - account.markAsRead(articleIDsToMarkRead) { error in - results.markAsReadError = error + account.markAsRead(articleIDsToMarkRead) { result in + if case .failure(let error) = result { + results.markAsReadError = error + } group.leave() } diff --git a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift index e99388d81..113841b64 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift @@ -10,6 +10,7 @@ import Foundation import RSCore import RSParser import Articles +import ArticlesDatabase import RSWeb public enum LocalAccountDelegateError: String, Error { @@ -18,7 +19,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 +35,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 +207,7 @@ final class LocalAccountDelegate: AccountDelegate { } func accountDidInitialize(_ account: Account) { + self.account = account } func accountWillBeDeleted(_ account: Account) { @@ -208,7 +220,7 @@ final class LocalAccountDelegate: AccountDelegate { // MARK: Suspend and Resume (for iOS) func suspendNetwork() { - refresher.suspend() + refresher?.suspend() } func suspendDatabase() { @@ -216,6 +228,24 @@ final class LocalAccountDelegate: AccountDelegate { } func resume() { - refresher.resume() + refresher?.resume() } } + +extension LocalAccountDelegate: LocalAccountRefresherDelegate { + + func localAccountRefresher(_ refresher: LocalAccountRefresher, didProcess newAndUpdatedArticles: NewAndUpdatedArticles) { + } + + 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..f109c7b59 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift @@ -11,20 +11,28 @@ import RSCore import RSParser import RSWeb import Articles +import ArticlesDatabase + +protocol LocalAccountRefresherDelegate { + func localAccountRefresher(_ refresher: LocalAccountRefresher, didProcess: NewAndUpdatedArticles) + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) + func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) +} final class LocalAccountRefresher { - private var feedCompletionBlock: ((WebFeed) -> Void)? - private var completion: (() -> Void)? + private var completions = [() -> 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, completion: (() -> Void)? = nil) { + if let completion = completion { + completions.append(completion) + } downloadSession.downloadObjects(feeds as NSSet) } @@ -64,21 +72,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,20 +95,20 @@ 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 } - account.update(feed, with: parsedFeed) { error in - if error == nil { + account.update(feed, with: parsedFeed) { result in + if case .success(let newAndUpdatedArticles) = result { + self.delegate?.localAccountRefresher(self, didProcess: newAndUpdatedArticles) if let httpResponse = response as? HTTPURLResponse { feed.conditionalGetInfo = HTTPConditionalGetInfo(urlResponse: httpResponse) } - feed.contentHash = dataHash } completion() - self.feedCompletionBlock?(feed) + self.delegate?.localAccountRefresher(self, requestCompletedFor: feed) } } @@ -109,7 +117,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 +126,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { } if data.isDefinitelyNotFeed() { - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return false } @@ -127,7 +135,7 @@ extension LocalAccountRefresher: DownloadSessionDelegate { if FeedParser.mightBeAbleToParseBasedOnPartialData(parserData) { return true } else { - feedCompletionBlock?(feed) + delegate?.localAccountRefresher(self, requestCompletedFor: feed) return false } } @@ -137,17 +145,23 @@ 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 + completions.forEach({ $0() }) + completions = [() -> Void]() + delegate?.localAccountRefresherDidFinish(self) } } diff --git a/Frameworks/Account/WebFeed.swift b/Frameworks/Account/WebFeed.swift index a63d28fb0..a964bc70b 100644 --- a/Frameworks/Account/WebFeed.swift +++ b/Frameworks/Account/WebFeed.swift @@ -221,9 +221,9 @@ public final class WebFeed: Feed, Renamable, Hashable { self.metadata = metadata } - // MARK: - Debug - - public func debugDropConditionalGetInfo() { + // MARK: - API + + public func dropConditionalGetInfo() { conditionalGetInfo = nil contentHash = nil } diff --git a/Frameworks/ArticlesDatabase/ArticlesDatabase.swift b/Frameworks/ArticlesDatabase/ArticlesDatabase.swift index 4ca73e177..83e2411db 100644 --- a/Frameworks/ArticlesDatabase/ArticlesDatabase.swift +++ b/Frameworks/ArticlesDatabase/ArticlesDatabase.swift @@ -27,6 +27,16 @@ public typealias SingleUnreadCountCompletionBlock = (SingleUnreadCountResult) -> public struct NewAndUpdatedArticles { public let newArticles: Set
? public let updatedArticles: Set
? + + public init() { + self.newArticles = Set
() + self.updatedArticles = Set
() + } + + public init(newArticles: Set
?, updatedArticles: Set
?) { + self.newArticles = newArticles + self.updatedArticles = updatedArticles + } } public typealias UpdateArticlesResult = Result @@ -222,8 +232,8 @@ public final class ArticlesDatabase { return try articlesTable.mark(articles, statusKey, flag) } - public func mark(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: @escaping DatabaseCompletionBlock) { - articlesTable.mark(articleIDs, statusKey, flag, completion) + public func markAndFetchNew(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: @escaping ArticleIDsCompletionBlock) { + articlesTable.markAndFetchNew(articleIDs, statusKey, flag, completion) } /// Create statuses for specified articleIDs. For existing statuses, don’t do anything. diff --git a/Frameworks/ArticlesDatabase/ArticlesTable.swift b/Frameworks/ArticlesDatabase/ArticlesTable.swift index 9aa7bc06b..899cc853b 100644 --- a/Frameworks/ArticlesDatabase/ArticlesTable.swift +++ b/Frameworks/ArticlesDatabase/ArticlesTable.swift @@ -194,7 +194,7 @@ final class ArticlesTable: DatabaseTable { func makeDatabaseCalls(_ database: FMDatabase) { let articleIDs = parsedItems.articleIDs() - let statusesDictionary = self.statusesTable.ensureStatusesForArticleIDs(articleIDs, false, database) //1 + let (statusesDictionary, _) = self.statusesTable.ensureStatusesForArticleIDs(articleIDs, false, database) //1 assert(statusesDictionary.count == articleIDs.count) let allIncomingArticles = Article.articlesWithParsedItems(parsedItems, webFeedID, self.accountID, statusesDictionary) //2 @@ -266,7 +266,7 @@ final class ArticlesTable: DatabaseTable { articleIDs.formUnion(parsedItems.articleIDs()) } - let statusesDictionary = self.statusesTable.ensureStatusesForArticleIDs(articleIDs, read, database) //1 + let (statusesDictionary, _) = self.statusesTable.ensureStatusesForArticleIDs(articleIDs, read, database) //1 assert(statusesDictionary.count == articleIDs.count) let allIncomingArticles = Article.articlesWithWebFeedIDsAndItems(webFeedIDsAndItems, self.accountID, statusesDictionary) //2 @@ -418,17 +418,17 @@ final class ArticlesTable: DatabaseTable { return statuses } - func mark(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ completion: @escaping DatabaseCompletionBlock) { + func markAndFetchNew(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ completion: @escaping ArticleIDsCompletionBlock) { queue.runInTransaction { databaseResult in switch databaseResult { case .success(let database): - self.statusesTable.mark(articleIDs, statusKey, flag, database) + let newStatusIDs = self.statusesTable.markAndFetchNew(articleIDs, statusKey, flag, database) DispatchQueue.main.async { - completion(nil) + completion(.success(newStatusIDs)) } case .failure(let databaseError): DispatchQueue.main.async { - completion(databaseError) + completion(.failure(databaseError)) } } } diff --git a/Frameworks/ArticlesDatabase/StatusesTable.swift b/Frameworks/ArticlesDatabase/StatusesTable.swift index 04916d3de..5858bc743 100644 --- a/Frameworks/ArticlesDatabase/StatusesTable.swift +++ b/Frameworks/ArticlesDatabase/StatusesTable.swift @@ -27,11 +27,11 @@ final class StatusesTable: DatabaseTable { // MARK: - Creating/Updating - func ensureStatusesForArticleIDs(_ articleIDs: Set, _ read: Bool, _ database: FMDatabase) -> [String: ArticleStatus] { + func ensureStatusesForArticleIDs(_ articleIDs: Set, _ read: Bool, _ database: FMDatabase) -> ([String: ArticleStatus], Set) { // Check cache. let articleIDsMissingCachedStatus = articleIDsWithNoCachedStatus(articleIDs) if articleIDsMissingCachedStatus.isEmpty { - return statusesDictionary(articleIDs) + return (statusesDictionary(articleIDs), Set()) } // Check database. @@ -43,7 +43,7 @@ final class StatusesTable: DatabaseTable { self.createAndSaveStatusesForArticleIDs(articleIDsNeedingStatus, read, database) } - return statusesDictionary(articleIDs) + return (statusesDictionary(articleIDs), articleIDsNeedingStatus) } func existingStatusesForArticleIDs(_ articleIDs: Set, _ database: FMDatabase) -> [String: ArticleStatus] { @@ -85,10 +85,11 @@ final class StatusesTable: DatabaseTable { return updatedStatuses } - func mark(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ database: FMDatabase) { - let statusesDictionary = ensureStatusesForArticleIDs(articleIDs, flag, database) + func markAndFetchNew(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ database: FMDatabase) -> Set { + let (statusesDictionary, newStatusIDs) = ensureStatusesForArticleIDs(articleIDs, flag, database) let statuses = Set(statusesDictionary.values) mark(statuses, statusKey, flag, database) + return newStatusIDs } // MARK: - Fetching diff --git a/Mac/Preferences/Accounts/AccountsAddViewController.swift b/Mac/Preferences/Accounts/AccountsAddViewController.swift index 4699e64e1..f2b360d1b 100644 --- a/Mac/Preferences/Accounts/AccountsAddViewController.swift +++ b/Mac/Preferences/Accounts/AccountsAddViewController.swift @@ -19,7 +19,7 @@ class AccountsAddViewController: NSViewController { #if DEBUG private var addableAccountTypes: [AccountType] = [.onMyMac, .feedbin, .feedly, .feedWrangler, .freshRSS, .cloudKit, .newsBlur] #else - private var addableAccountTypes: [AccountType] = [.onMyMac, .feedbin, .feedly] + private var addableAccountTypes: [AccountType] = [.onMyMac, .feedbin, .feedly, .cloudKit, .newsBlur] #endif init() { diff --git a/Mac/Preferences/Accounts/AccountsDetailViewController.swift b/Mac/Preferences/Accounts/AccountsDetailViewController.swift index a33d7bc66..5ca513c02 100644 --- a/Mac/Preferences/Accounts/AccountsDetailViewController.swift +++ b/Mac/Preferences/Accounts/AccountsDetailViewController.swift @@ -33,8 +33,7 @@ final class AccountsDetailViewController: NSViewController, NSTextFieldDelegate return true } switch account.type { - case .onMyMac, - .feedly: + case .onMyMac, .cloudKit, .feedly: return true default: return false diff --git a/Technotes/RetentionPolicy.markdown b/Technotes/RetentionPolicy.markdown new file mode 100644 index 000000000..c86799409 --- /dev/null +++ b/Technotes/RetentionPolicy.markdown @@ -0,0 +1,72 @@ +# Retention Policy + +This is a user interface issue, primarily — what articles should be displayed to the user? + +This article answers that question, and it describes how the decisions are implemented. + +And, at the end, there’s a special note about why we have limits at all. + +## Web-Based Sync Systems + +When used with Feedbin, Feedly, and other syncing systems, NetNewsWire should show the same unread articles that the user would see in the browser-based version. (The unread counts would necessarily be the same in NetNewsWire and on the web.) + +It should also show the exact same starred items. + +It does *not* have to show the exact same read items. Instead, it will show read items that arrived locally in the last 90 days. + +### Database Queries + +Most queries for articles should include this logic: + +* If an article is userDeleted, don’t show it +* If an article is starred or unread, show it no matter what +* If an article is read, and status.dateArrived < 90 days ago, then show it + +### Database Cleanup + +Database cleanups to do at startup: + +* Delete articles from feeds no-longer-subscribed-to, unless starred +* Delete read/not-starred articles where status.dateArrived > 90 days go (because these wouldn’t be shown in the UI) +* Delete statuses where status is read, not starred, and not userDeleted, and dateArrived > 180 days ago, and the associated article no longer exists in the articles table. + +We keep statuses a bit longer than articles, in case an article comes back. But we don’t keep most statuses forever. + +## Local and iCloud Accounts + +NetNewsWire should show articles that are currently in the feed. When an article drops off the feed, it no longer displays in the UI. + +The one exception is starred articles: as with sync systems, starred articles are kept forever. + +### Database Queries + +Most queries for articles should include this logic: + +* If an article is userDeleted, don’t show it +* If an article is starred, show it no matter what + +### Database Cleanup + +Database cleanups to do while running: + +* When processing a feed, delete articles that no longer appear in the feed — unless a feed comes back empty (with zero articles); do nothing in that case + +Database cleanups to do at startup: + +* Delete articles from feeds no-longer-subscribed-to, unless starred +* Delete statuses where not starred, not userDeleted, and dateArrived > 30 days ago, and the associated article no longer exists in the articles table. + +We keep statuses a bit longer than articles, in case an article comes back. (An article could come back when, for example, a publisher reconfigures their feed so that it includes more items. This could bring back articles that had previously fallen off the feed.) + +## Why Do We Have Limits At All? + +Most people don’t want NetNewsWire to just keep holding on to everything forever, but a few people do. + +And that’s understandable. It’s pretty cool to have a personal backup of your favorite parts of the web. It’s great for researchers, journalists, and bloggers. + +But the more articles we keep, the larger the database gets. It’s already not unusual for a database to become 1GB in size — but we can’t let it grow to many times that, because it will: + +* Make NetNewsWire unacceptably slow +* Take up an inordinate amount of disk space + +So we need to have limits. The point of NetNewsWire is to keep up with what’s new: it’s *not* an archiving system. So we’ve defined “what’s new” expansively, but not so expansively that we don’t have a definition for “what’s old.” diff --git a/iOS/Settings/AddAccountViewController.swift b/iOS/Settings/AddAccountViewController.swift index 1a2cf52f3..db24c743f 100644 --- a/iOS/Settings/AddAccountViewController.swift +++ b/iOS/Settings/AddAccountViewController.swift @@ -19,7 +19,7 @@ class AddAccountViewController: UITableViewController, AddAccountDismissDelegate #if DEBUG private var addableAccountTypes: [AccountType] = [.onMyMac, .feedbin, .feedly, .feedWrangler, .cloudKit, .newsBlur] #else - private var addableAccountTypes: [AccountType] = [.onMyMac, .feedbin, .feedly] + private var addableAccountTypes: [AccountType] = [.onMyMac, .feedbin, .feedly, .cloudKit, .newsBlur] #endif override func viewDidLoad() { diff --git a/submodules/RSWeb b/submodules/RSWeb index 88d634f5f..c524ce914 160000 --- a/submodules/RSWeb +++ b/submodules/RSWeb @@ -1 +1 @@ -Subproject commit 88d634f5fd42aab203b6e53c7b551a92b03ffc97 +Subproject commit c524ce9145dfe093500325b1c758ea83f82cc090