From 6a9620e4de278f9e0bb9a95d81634d97aa33f4ba Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Mon, 6 Apr 2020 15:37:26 -0500 Subject: [PATCH 01/11] Add NewsBlur and CloudKit to addable account types on production builds. --- Mac/Preferences/Accounts/AccountsAddViewController.swift | 2 +- iOS/Settings/AddAccountViewController.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/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() { From ae091afb183b4b3336e572d708ec7dd1ea26fee9 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Tue, 7 Apr 2020 19:02:25 -0500 Subject: [PATCH 02/11] Allow for error on query when looking for existing starred records. --- .../Account/CloudKit/CloudKitArticlesZone.swift | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift index 080799ed8..8a11fd951 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift @@ -147,7 +147,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 +163,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 +172,7 @@ private extension CloudKitArticlesZone { } group.notify(queue: DispatchQueue.main) { - if errorOccurred { - completion(.failure(CloudKitZoneError.unknown)) - } else { - completion(.success(records)) - } + completion(.success(records)) } } From e443085c525db1226b0c3123076fcde244ae5f52 Mon Sep 17 00:00:00 2001 From: Brent Simmons Date: Wed, 8 Apr 2020 17:45:32 -0700 Subject: [PATCH 03/11] Write Technote on Retention Policy. --- Technotes/RetentionPolicy.markdown | 72 ++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 Technotes/RetentionPolicy.markdown 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.” From 5106cde97699c4456d600ef227d4a72478292118 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 9 Apr 2020 19:29:37 -0500 Subject: [PATCH 04/11] Hide the credentials account button on the Mac for iCloud. --- Mac/Preferences/Accounts/AccountsDetailViewController.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 From 63c4d53ee9183d537fa4402cc038719aa6f79709 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Fri, 10 Apr 2020 11:20:35 -0500 Subject: [PATCH 05/11] 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 From 4418a4bb0207bbe7fc4b83ff7fafed205b9109ff Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Fri, 10 Apr 2020 15:19:33 -0500 Subject: [PATCH 06/11] Add completion block that returns new status records when we are marking statuses asynchronously. --- Frameworks/Account/Account.swift | 38 +++++++++++-------- ...edlyIngestStarredArticleIdsOperation.swift | 12 ++++-- ...eedlyIngestUnreadArticleIdsOperation.swift | 12 ++++-- .../ArticlesDatabase/ArticlesDatabase.swift | 4 +- .../ArticlesDatabase/ArticlesTable.swift | 12 +++--- .../ArticlesDatabase/StatusesTable.swift | 11 +++--- 6 files changed, 52 insertions(+), 37 deletions(-) diff --git a/Frameworks/Account/Account.swift b/Frameworks/Account/Account.swift index cc4fc37f4..e3b2e4890 100644 --- a/Frameworks/Account/Account.swift +++ b/Frameworks/Account/Account.swift @@ -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: ArticleStatusesResultBlock? = 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 newArticleStatuses): + self.noteStatusesForArticleIDsDidChange(articleIDs) + completion?(.success(newArticleStatuses)) + 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: ArticleStatusesResultBlock? = 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: ArticleStatusesResultBlock? = 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: ArticleStatusesResultBlock? = 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: ArticleStatusesResultBlock? = 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. 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/ArticlesDatabase/ArticlesDatabase.swift b/Frameworks/ArticlesDatabase/ArticlesDatabase.swift index 4ca73e177..179b3928f 100644 --- a/Frameworks/ArticlesDatabase/ArticlesDatabase.swift +++ b/Frameworks/ArticlesDatabase/ArticlesDatabase.swift @@ -222,8 +222,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 ArticleStatusesResultBlock) { + 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..a6c72b790 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 ArticleStatusesResultBlock) { 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..aa8d3b711 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 Set(newStatusIDs.compactMap({ statusesDictionary[$0] })) } // MARK: - Fetching From 983138366fac30fc8880112cf2e95aaab6932481 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Fri, 10 Apr 2020 16:25:58 -0500 Subject: [PATCH 07/11] Add code to process new article entries. --- Frameworks/Account/Account.swift | 18 ++++---- .../CloudKit/CloudKitAccountDelegate.swift | 6 ++- .../CloudKitArticlesZoneDelegate.swift | 41 +++++++++++++++++-- .../LocalAccount/LocalAccountRefresher.swift | 8 +++- Frameworks/Account/WebFeed.swift | 6 +-- .../ArticlesDatabase/ArticlesDatabase.swift | 2 +- .../ArticlesDatabase/ArticlesTable.swift | 2 +- .../ArticlesDatabase/StatusesTable.swift | 4 +- 8 files changed, 65 insertions(+), 22 deletions(-) diff --git a/Frameworks/Account/Account.swift b/Frameworks/Account/Account.swift index e3b2e4890..ecc3f2b6e 100644 --- a/Frameworks/Account/Account.swift +++ b/Frameworks/Account/Account.swift @@ -801,16 +801,16 @@ 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. /// Returns a set of new article statuses. - func markAndFetchNew(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: ArticleStatusesResultBlock? = nil) { + func markAndFetchNew(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: ArticleIDsCompletionBlock? = nil) { guard !articleIDs.isEmpty else { - completion?(.success(Set())) + completion?(.success(Set())) return } database.markAndFetchNew(articleIDs: articleIDs, statusKey: statusKey, flag: flag) { result in switch result { - case .success(let newArticleStatuses): + case .success(let newArticleStatusIDs): self.noteStatusesForArticleIDsDidChange(articleIDs) - completion?(.success(newArticleStatuses)) + completion?(.success(newArticleStatusIDs)) case .failure(let databaseError): completion?(.failure(databaseError)) } @@ -819,25 +819,25 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, /// Mark articleIDs as read. Will create statuses in the database and in memory as needed. Sends a .StatusesDidChange notification. /// Returns a set of new article statuses. - func markAsRead(_ articleIDs: Set, completion: ArticleStatusesResultBlock? = nil) { + 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. /// Returns a set of new article statuses. - func markAsUnread(_ articleIDs: Set, completion: ArticleStatusesResultBlock? = nil) { + 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. /// Returns a set of new article statuses. - func markAsStarred(_ articleIDs: Set, completion: ArticleStatusesResultBlock? = nil) { + 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. /// Returns a set of new article statuses. - func markAsUnstarred(_ articleIDs: Set, completion: ArticleStatusesResultBlock? = nil) { + func markAsUnstarred(_ articleIDs: Set, completion: ArticleIDsCompletionBlock? = nil) { markAndFetchNew(articleIDs: articleIDs, statusKey: .starred, flag: false, completion: completion) } @@ -893,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 df9d5c6be..4342525d9 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift @@ -481,7 +481,11 @@ final class CloudKitAccountDelegate: AccountDelegate { 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, + refresher: refresher, + refreshProgress: refreshProgress) // Check to see if this is a new account and initialize anything we need if account.externalID == nil { diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift index 6b712b932..12b807fb3 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift @@ -9,6 +9,7 @@ import Foundation import os.log import RSParser +import RSWeb import CloudKit import SyncDatabase @@ -19,11 +20,15 @@ class CloudKitArticlesZoneDelegate: CloudKitZoneDelegate { weak var account: Account? var database: SyncDatabase weak var articlesZone: CloudKitArticlesZone? + weak var refresher: LocalAccountRefresher? + weak var refreshProgress: DownloadProgress? - init(account: Account, database: SyncDatabase, articlesZone: CloudKitArticlesZone) { + init(account: Account, database: SyncDatabase, articlesZone: CloudKitArticlesZone, refresher: LocalAccountRefresher?, refreshProgress: DownloadProgress?) { self.account = account self.database = database self.articlesZone = articlesZone + self.refresher = refresher + self.refreshProgress = refreshProgress } func cloudKitDidChange(record: CKRecord) { @@ -82,8 +87,37 @@ 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 { + self.account?.fetchArticlesAsync(FetchType.articleIDs(newArticleStatusIDs)) { result in + switch result { + case .success(let articles): + + if articles.isEmpty { + group.leave() + } else { + let webFeeds = Set(articles.compactMap({ $0.webFeed })) + webFeeds.forEach { $0.dropConditionalGetInfo() } + self.refreshProgress?.addToNumberOfTasksAndRemaining(webFeeds.count) + self.refresher?.refreshFeeds(webFeeds) { + group.leave() + } + } + + case .failure: + group.leave() + } + } + } + + case .failure: + group.leave() + } } group.enter() @@ -119,7 +153,6 @@ private extension CloudKitArticlesZoneDelegate { } - func makeParsedItem(_ articleRecord: CKRecord) -> ParsedItem? { var parsedAuthors = Set() diff --git a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift index cb8c2f24e..508a6cf21 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift @@ -19,6 +19,7 @@ protocol LocalAccountRefresherDelegate { final class LocalAccountRefresher { + private var completions = [() -> Void]() private var isSuspended = false var delegate: LocalAccountRefresherDelegate? @@ -26,7 +27,10 @@ final class LocalAccountRefresher { return DownloadSession(delegate: self) }() - public func refreshFeeds(_ feeds: Set) { + public func refreshFeeds(_ feeds: Set, completion: (() -> Void)? = nil) { + if let completion = completion { + completions.append(completion) + } downloadSession.downloadObjects(feeds as NSSet) } @@ -154,6 +158,8 @@ extension LocalAccountRefresher: DownloadSessionDelegate { func downloadSessionDidCompleteDownloadObjects(_ downloadSession: DownloadSession) { delegate?.localAccountRefresherDidFinish(self) + completions.forEach({ $0() }) + completions = [() -> Void]() } } 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 179b3928f..fd30105c2 100644 --- a/Frameworks/ArticlesDatabase/ArticlesDatabase.swift +++ b/Frameworks/ArticlesDatabase/ArticlesDatabase.swift @@ -222,7 +222,7 @@ public final class ArticlesDatabase { return try articlesTable.mark(articles, statusKey, flag) } - public func markAndFetchNew(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: @escaping ArticleStatusesResultBlock) { + public func markAndFetchNew(articleIDs: Set, statusKey: ArticleStatus.Key, flag: Bool, completion: @escaping ArticleIDsCompletionBlock) { articlesTable.markAndFetchNew(articleIDs, statusKey, flag, completion) } diff --git a/Frameworks/ArticlesDatabase/ArticlesTable.swift b/Frameworks/ArticlesDatabase/ArticlesTable.swift index a6c72b790..899cc853b 100644 --- a/Frameworks/ArticlesDatabase/ArticlesTable.swift +++ b/Frameworks/ArticlesDatabase/ArticlesTable.swift @@ -418,7 +418,7 @@ final class ArticlesTable: DatabaseTable { return statuses } - func markAndFetchNew(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ completion: @escaping ArticleStatusesResultBlock) { + func markAndFetchNew(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ completion: @escaping ArticleIDsCompletionBlock) { queue.runInTransaction { databaseResult in switch databaseResult { case .success(let database): diff --git a/Frameworks/ArticlesDatabase/StatusesTable.swift b/Frameworks/ArticlesDatabase/StatusesTable.swift index aa8d3b711..5858bc743 100644 --- a/Frameworks/ArticlesDatabase/StatusesTable.swift +++ b/Frameworks/ArticlesDatabase/StatusesTable.swift @@ -85,11 +85,11 @@ final class StatusesTable: DatabaseTable { return updatedStatuses } - func markAndFetchNew(_ articleIDs: Set, _ statusKey: ArticleStatus.Key, _ flag: Bool, _ database: FMDatabase) -> Set { + 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 Set(newStatusIDs.compactMap({ statusesDictionary[$0] })) + return newStatusIDs } // MARK: - Fetching From a8dcf3eeee7d046f21d3e81e8399bc9c5169abfd Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Fri, 10 Apr 2020 17:23:39 -0500 Subject: [PATCH 08/11] Add the unread statuses on receipt to CloudKit. --- Frameworks/Account/Account.swift | 10 ++-- .../CloudKit/CloudKitAccountDelegate.swift | 49 ++++++++++++------- .../CloudKitArticlesZoneDelegate.swift | 4 +- .../LocalAccount/LocalAccountDelegate.swift | 4 ++ .../LocalAccount/LocalAccountRefresher.swift | 10 ++-- .../ArticlesDatabase/ArticlesDatabase.swift | 10 ++++ 6 files changed, 59 insertions(+), 28 deletions(-) diff --git a/Frameworks/Account/Account.swift b/Frameworks/Account/Account.swift index ecc3f2b6e..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)) } } } diff --git a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift index 4342525d9..d20e23e30 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 { @@ -50,7 +51,6 @@ final class CloudKitAccountDelegate: AccountDelegate { var accountMetadata: AccountMetadata? var refreshProgress = DownloadProgress(numberOfTasks: 0) - var refreshAllCompletion: ((Result) -> Void)? = nil init(dataFolder: String) { accountZone = CloudKitAccountZone(container: container) @@ -79,13 +79,12 @@ final class CloudKitAccountDelegate: AccountDelegate { } func refreshAll(for account: Account, completion: @escaping (Result) -> Void) { - guard refreshAllCompletion == nil else { + guard refreshProgress.isComplete else { completion(.success(())) return } - refreshAllCompletion = completion - refreshAll(for: account, downloadFeeds: true) + refreshAll(for: account, downloadFeeds: true, completion: completion) } func sendArticleStatus(for account: Account, completion: @escaping ((Result) -> Void)) { @@ -155,11 +154,10 @@ final class CloudKitAccountDelegate: AccountDelegate { } func importOPML(for account:Account, opmlFile: URL, completion: @escaping (Result) -> Void) { - guard refreshAllCompletion == nil else { + guard refreshProgress.isComplete else { completion(.success(())) return } - refreshAllCompletion = completion var fileData: Data? @@ -214,7 +212,7 @@ final class CloudKitAccountDelegate: AccountDelegate { } self.accountZone.importOPML(rootExternalID: rootExternalID, items: normalizedItems) { _ in - self.refreshAll(for: account, downloadFeeds: false) + self.refreshAll(for: account, downloadFeeds: false, completion: completion) } } @@ -493,7 +491,7 @@ final class CloudKitAccountDelegate: AccountDelegate { switch result { case .success(let externalID): account.externalID = externalID - self.refreshAll(for: account, downloadFeeds: false) + self.refreshAll(for: account, downloadFeeds: false) { _ in } case .failure(let error): os_log(.error, log: self.log, "Error adding account container: %@", error.localizedDescription) } @@ -533,7 +531,7 @@ final class CloudKitAccountDelegate: AccountDelegate { private extension CloudKitAccountDelegate { - func refreshAll(for account: Account, downloadFeeds: Bool) { + func refreshAll(for account: Account, downloadFeeds: Bool, completion: @escaping (Result) -> Void) { let intialWebFeedsCount = downloadFeeds ? account.flattenedWebFeeds().count : 0 refreshProgress.addToNumberOfTasksAndRemaining(3 + intialWebFeedsCount) @@ -541,8 +539,7 @@ private extension CloudKitAccountDelegate { func fail(_ error: Error) { self.processAccountError(account, error) self.refreshProgress.clear() - self.refreshAllCompletion?(.failure(error)) - self.refreshAllCompletion = nil + completion(.failure(error)) } BatchUpdate.shared.start() @@ -569,12 +566,24 @@ private extension CloudKitAccountDelegate { self.refreshProgress.completeTask() guard downloadFeeds else { - self.refreshAllCompletion?(.success(())) - self.refreshAllCompletion = nil + completion(.success(())) return } - self.refresher?.refreshFeeds(webFeeds) + self.refresher?.refreshFeeds(webFeeds) { + + account.metadata.lastArticleFetchEndTime = Date() + + self.sendArticleStatus(for: account) { result in + switch result { + case .success: + completion(.success(())) + case .failure(let error): + fail(error) + } + } + + } case .failure(let error): fail(error) @@ -605,6 +614,15 @@ 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() @@ -612,9 +630,6 @@ extension CloudKitAccountDelegate: LocalAccountRefresherDelegate { func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) { self.refreshProgress.clear() - account?.metadata.lastArticleFetchEndTime = Date() - refreshAllCompletion?(.success(())) - refreshAllCompletion = nil } } diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift index 12b807fb3..58469ebb2 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift @@ -138,9 +138,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) } } diff --git a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift index dd7309c72..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 { @@ -233,6 +234,9 @@ final class LocalAccountDelegate: AccountDelegate { extension LocalAccountDelegate: LocalAccountRefresherDelegate { + func localAccountRefresher(_ refresher: LocalAccountRefresher, didProcess newAndUpdatedArticles: NewAndUpdatedArticles) { + } + func localAccountRefresher(_ refresher: LocalAccountRefresher, requestCompletedFor: WebFeed) { refreshProgress.completeTask() } diff --git a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift index 508a6cf21..f109c7b59 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountRefresher.swift @@ -11,8 +11,10 @@ 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) } @@ -97,12 +99,12 @@ extension LocalAccountRefresher: DownloadSessionDelegate { 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() @@ -157,9 +159,9 @@ extension LocalAccountRefresher: DownloadSessionDelegate { } func downloadSessionDidCompleteDownloadObjects(_ downloadSession: DownloadSession) { - delegate?.localAccountRefresherDidFinish(self) completions.forEach({ $0() }) completions = [() -> Void]() + delegate?.localAccountRefresherDidFinish(self) } } diff --git a/Frameworks/ArticlesDatabase/ArticlesDatabase.swift b/Frameworks/ArticlesDatabase/ArticlesDatabase.swift index fd30105c2..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 From 6ec11119f8333b1dc7ee194809987d77993c5133 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Fri, 10 Apr 2020 18:10:54 -0500 Subject: [PATCH 09/11] Add external web feed to status so that it can prompt receiving system to pull down new articles. --- .../CloudKit/CloudKitAccountDelegate.swift | 12 +++--- .../CloudKit/CloudKitArticlesZone.swift | 26 ++++++++++--- .../CloudKitArticlesZoneDelegate.swift | 37 ++++++++++--------- 3 files changed, 46 insertions(+), 29 deletions(-) diff --git a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift index d20e23e30..b35dfb543 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift @@ -98,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 }) ) @@ -119,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)) } diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZone.swift index 8a11fd951..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" diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift index 58469ebb2..b33e73b9d 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift @@ -94,25 +94,28 @@ private extension CloudKitArticlesZoneDelegate { if newArticleStatusIDs.isEmpty { group.leave() } else { - self.account?.fetchArticlesAsync(FetchType.articleIDs(newArticleStatusIDs)) { result in - switch result { - case .success(let articles): - - if articles.isEmpty { - group.leave() - } else { - let webFeeds = Set(articles.compactMap({ $0.webFeed })) - webFeeds.forEach { $0.dropConditionalGetInfo() } - self.refreshProgress?.addToNumberOfTasksAndRemaining(webFeeds.count) - self.refresher?.refreshFeeds(webFeeds) { - group.leave() - } - } - - case .failure: - group.leave() + + 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: From 00d3249c86ee24963109d06b6de6bc268a1d1261 Mon Sep 17 00:00:00 2001 From: abilops Date: Sat, 11 Apr 2020 05:09:52 +0530 Subject: [PATCH 10/11] Avoid cached response when adding new feed --- Frameworks/Account/FeedFinder/FeedFinder.swift | 2 +- submodules/RSWeb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/submodules/RSWeb b/submodules/RSWeb index f54e1cbad..c524ce914 160000 --- a/submodules/RSWeb +++ b/submodules/RSWeb @@ -1 +1 @@ -Subproject commit f54e1cbad3917822e40bc2310ed24bd7cb688a4f +Subproject commit c524ce9145dfe093500325b1c758ea83f82cc090 From d6a40053363075e75cab0e92591abb1aef0ed304 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 11 Apr 2020 12:22:28 -0500 Subject: [PATCH 11/11] Separate refreshers so that we don't send out duplicate unread statuses for new records. --- .../CloudKit/CloudKitAccountDelegate.swift | 11 ++++---- .../CloudKitArticlesZoneDelegate.swift | 28 ++++++++++++++++--- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift index b35dfb543..ca7ce0c0c 100644 --- a/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitAccountDelegate.swift @@ -37,7 +37,7 @@ final class CloudKitAccountDelegate: AccountDelegate { weak var account: Account? - private lazy var refresher: LocalAccountRefresher? = { + private lazy var refresher: LocalAccountRefresher = { let refresher = LocalAccountRefresher() refresher.delegate = self return refresher @@ -482,7 +482,6 @@ final class CloudKitAccountDelegate: AccountDelegate { articlesZone.delegate = CloudKitArticlesZoneDelegate(account: account, database: database, articlesZone: articlesZone, - refresher: refresher, refreshProgress: refreshProgress) // Check to see if this is a new account and initialize anything we need @@ -516,7 +515,7 @@ final class CloudKitAccountDelegate: AccountDelegate { // MARK: Suspend and Resume (for iOS) func suspendNetwork() { - refresher?.suspend() + refresher.suspend() } func suspendDatabase() { @@ -524,7 +523,7 @@ final class CloudKitAccountDelegate: AccountDelegate { } func resume() { - refresher?.resume() + refresher.resume() database.resume() } } @@ -570,7 +569,7 @@ private extension CloudKitAccountDelegate { return } - self.refresher?.refreshFeeds(webFeeds) { + self.refresher.refreshFeeds(webFeeds) { account.metadata.lastArticleFetchEndTime = Date() @@ -629,7 +628,7 @@ extension CloudKitAccountDelegate: LocalAccountRefresherDelegate { } func localAccountRefresherDidFinish(_ refresher: LocalAccountRefresher) { - self.refreshProgress.clear() + refreshProgress.clear() } } diff --git a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift index b33e73b9d..1e1ed466c 100644 --- a/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift +++ b/Frameworks/Account/CloudKit/CloudKitArticlesZoneDelegate.swift @@ -12,6 +12,8 @@ import RSParser import RSWeb import CloudKit import SyncDatabase +import Articles +import ArticlesDatabase class CloudKitArticlesZoneDelegate: CloudKitZoneDelegate { @@ -20,14 +22,18 @@ class CloudKitArticlesZoneDelegate: CloudKitZoneDelegate { weak var account: Account? var database: SyncDatabase weak var articlesZone: CloudKitArticlesZone? - weak var refresher: LocalAccountRefresher? weak var refreshProgress: DownloadProgress? + + private lazy var refresher: LocalAccountRefresher = { + let refresher = LocalAccountRefresher() + refresher.delegate = self + return refresher + }() - init(account: Account, database: SyncDatabase, articlesZone: CloudKitArticlesZone, refresher: LocalAccountRefresher?, refreshProgress: DownloadProgress?) { + init(account: Account, database: SyncDatabase, articlesZone: CloudKitArticlesZone, refreshProgress: DownloadProgress?) { self.account = account self.database = database self.articlesZone = articlesZone - self.refresher = refresher self.refreshProgress = refreshProgress } @@ -112,7 +118,7 @@ private extension CloudKitArticlesZoneDelegate { webFeeds.forEach { $0.dropConditionalGetInfo() } self.refreshProgress?.addToNumberOfTasksAndRemaining(webFeeds.count) - self.refresher?.refreshFeeds(webFeeds) { + self.refresher.refreshFeeds(webFeeds) { group.leave() } @@ -196,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) { + } + +}