From 3ed5a43de3af604abf7e599972eb47a6ab0dbf6b Mon Sep 17 00:00:00 2001 From: Kiel Gillard Date: Fri, 15 Nov 2019 07:59:44 +1100 Subject: [PATCH 1/3] Improves the behaviour and fixes some issues with cancelling of Feedly operations. --- .../Feedly/Operations/FeedlyOperation.swift | 14 ++++++++------ .../Feedly/Operations/FeedlySyncAllOperation.swift | 5 ++--- .../FeedlySyncStarredArticlesOperation.swift | 9 ++++++++- .../FeedlySyncStreamContentsOperation.swift | 9 ++++++++- .../FeedlySyncUnreadStatusesOperation.swift | 9 ++++++++- 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift index 8c231bc53..5c6b16afc 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift @@ -21,8 +21,8 @@ class FeedlyOperation: Operation { func didFinish() { assert(Thread.isMainThread) assert(!isFinished, "Finished operation is attempting to finish again.") - self.isExecutingOperation = false - self.isFinishedOperation = true + isExecutingOperation = false + isFinishedOperation = true } func didFinish(_ error: Error) { @@ -33,16 +33,18 @@ class FeedlyOperation: Operation { } override func start() { + guard !isCancelled else { + isExecutingOperation = false + isFinishedOperation = true + return + } + isExecutingOperation = true DispatchQueue.main.async { self.main() } } - override func cancel() { - super.cancel() - } - override var isExecuting: Bool { return isExecutingOperation } diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift index 43faee339..b93031677 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift @@ -94,10 +94,9 @@ final class FeedlySyncAllOperation: FeedlyOperation { os_log(.debug, log: log, "Cancelling sync %{public}@", syncUUID.uuidString) self.operationQueue.cancelAllOperations() - syncCompletionHandler?(.failure(URLError(.cancelled))) - syncCompletionHandler = nil + super.cancel() - self.didFinish() + didFinish() } override func main() { diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncStarredArticlesOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncStarredArticlesOperation.swift index c4955362b..ea608ccbb 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncStarredArticlesOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncStarredArticlesOperation.swift @@ -96,13 +96,15 @@ final class FeedlySyncStarredArticlesOperation: FeedlyOperation, FeedlyOperation } override func cancel() { + os_log(.debug, log: log, "Canceling sync starred articles") operationQueue.cancelAllOperations() super.cancel() + didFinish() } override func main() { guard !isCancelled else { - didFinish() + // override of cancel calls didFinish(). return } @@ -110,6 +112,11 @@ final class FeedlySyncStarredArticlesOperation: FeedlyOperation, FeedlyOperation } func feedlyGetStreamContentsOperation(_ operation: FeedlyGetStreamContentsOperation, didGetContentsOf stream: FeedlyStream) { + guard !isCancelled else { + os_log(.debug, log: log, "Cancelled starred stream contents for %@", stream.id) + return + } + entryProvider.addEntries(from: operation) os_log(.debug, log: log, "Collecting %i items from %@", stream.items.count, stream.id) diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncStreamContentsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncStreamContentsOperation.swift index 2d416b697..667857073 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncStreamContentsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncStreamContentsOperation.swift @@ -42,13 +42,15 @@ final class FeedlySyncStreamContentsOperation: FeedlyOperation, FeedlyOperationD } override func cancel() { + os_log(.debug, log: log, "Canceling sync stream contents") operationQueue.cancelAllOperations() super.cancel() + didFinish() } override func main() { guard !isCancelled else { - didFinish() + // override of cancel calls didFinish(). return } @@ -92,6 +94,11 @@ final class FeedlySyncStreamContentsOperation: FeedlyOperation, FeedlyOperationD } func feedlyGetStreamContentsOperation(_ operation: FeedlyGetStreamContentsOperation, didGetContentsOf stream: FeedlyStream) { + guard !isCancelled else { + os_log(.debug, log: log, "Cancelled requesting page for %@", resource.id) + return + } + os_log(.debug, log: log, "Ingesting %i items from %@", stream.items.count, stream.id) guard let continuation = stream.continuation else { diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncUnreadStatusesOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncUnreadStatusesOperation.swift index 15280aa44..6cae9d468 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncUnreadStatusesOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncUnreadStatusesOperation.swift @@ -77,13 +77,15 @@ final class FeedlySyncUnreadStatusesOperation: FeedlyOperation, FeedlyOperationD } override func cancel() { + os_log(.debug, log: log, "Canceling sync unread statuses") operationQueue.cancelAllOperations() super.cancel() + didFinish() } override func main() { guard !isCancelled else { - didFinish() + // override of cancel calls didFinish(). return } @@ -91,6 +93,11 @@ final class FeedlySyncUnreadStatusesOperation: FeedlyOperation, FeedlyOperationD } func feedlyGetStreamIdsOperation(_ operation: FeedlyGetStreamIdsOperation, didGet streamIds: FeedlyStreamIds) { + guard !isCancelled else { + os_log(.debug, log: log, "Cancelled unread stream ids.") + return + } + os_log(.debug, log: log, "Collecting %i unread article ids from %@", streamIds.ids.count, resource.id) unreadEntryIdsProvider.addEntryIds(from: operation) From b317a99594f834b6c1fa1800a8abe94f18bb02d5 Mon Sep 17 00:00:00 2001 From: Kiel Gillard Date: Fri, 15 Nov 2019 09:32:02 +1100 Subject: [PATCH 2/3] Fix failing Feedly tests. --- .../Account/AccountTests/Feedly/FeedlyOperationTests.swift | 1 + .../AccountTests/Feedly/FeedlySyncAllOperationTests.swift | 4 +++- .../Account/Feedly/Operations/FeedlySyncAllOperation.swift | 3 +++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift b/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift index dc880597d..12992d748 100644 --- a/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift +++ b/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift @@ -102,6 +102,7 @@ class FeedlyOperationTests: XCTestCase { func testOperationCancellationFlags() { let testOperation = TestOperation() testOperation.didCallMainExpectation = expectation(description: "Did Call Main") + testOperation.didCallMainExpectation?.isInverted = true let completionExpectation = expectation(description: "Operation Completed") testOperation.completionBlock = { diff --git a/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift b/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift index cd1a988d3..5c7869439 100644 --- a/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift +++ b/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift @@ -67,11 +67,13 @@ class FeedlySyncAllOperationTests: XCTestCase { } let syncCompletionExpectation = expectation(description: "Did Finish Sync") + syncCompletionExpectation.isInverted = true syncAll.syncCompletionHandler = { result in switch result { case .success: - XCTFail("Expected failure.") + XCTFail("Sync operation was cancelled, not successful.") case .failure: + XCTFail("Sync operation should cancel silently.") break } syncCompletionExpectation.fulfill() diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift index b93031677..ff6a1df96 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift @@ -97,6 +97,9 @@ final class FeedlySyncAllOperation: FeedlyOperation { super.cancel() didFinish() + + // Operation should silently cancel. + syncCompletionHandler = nil } override func main() { From bec80922ce1f9994ee1de0a5b286433f282a597e Mon Sep 17 00:00:00 2001 From: Kiel Gillard Date: Mon, 11 Nov 2019 18:42:31 +1100 Subject: [PATCH 3/3] Enables accounts and their delegates to prepare to be deleted. Provides a variation of the Account Inspector on iOS for Feedly. --- Frameworks/Account/Account.swift | 4 +++ Frameworks/Account/AccountDelegate.swift | 2 ++ Frameworks/Account/AccountManager.swift | 2 ++ .../Feedbin/FeedbinAccountDelegate.swift | 3 ++ .../Feedly/FeedlyAccountDelegate.swift | 4 +++ .../LocalAccount/LocalAccountDelegate.swift | 3 ++ .../ReaderAPI/ReaderAPIAccountDelegate.swift | 3 ++ .../AccountsDetailViewController.swift | 17 ++++++++-- .../AccountInspectorViewController.swift | 34 +++++++++++++++---- 9 files changed, 63 insertions(+), 9 deletions(-) diff --git a/Frameworks/Account/Account.swift b/Frameworks/Account/Account.swift index 597fd920e..64972e775 100644 --- a/Frameworks/Account/Account.swift +++ b/Frameworks/Account/Account.swift @@ -399,6 +399,10 @@ public final class Account: DisplayNameProvider, UnreadCountProvider, Container, opmlFile.save() } + public func prepareForDeletion() { + delegate.accountWillBeDeleted(self) + } + func loadOPMLItems(_ items: [RSOPMLItem], parentFolder: Folder?) { var feedsToAdd = Set() diff --git a/Frameworks/Account/AccountDelegate.swift b/Frameworks/Account/AccountDelegate.swift index de13bf9cb..d03b7def7 100644 --- a/Frameworks/Account/AccountDelegate.swift +++ b/Frameworks/Account/AccountDelegate.swift @@ -46,6 +46,8 @@ protocol AccountDelegate { // Called at the end of account’s init method. func accountDidInitialize(_ account: Account) + + func accountWillBeDeleted(_ account: Account) static func validateCredentials(transport: Transport, credentials: Credentials, endpoint: URL?, completion: @escaping (Result) -> Void) diff --git a/Frameworks/Account/AccountManager.swift b/Frameworks/Account/AccountManager.swift index 385b5e389..1c29e6495 100644 --- a/Frameworks/Account/AccountManager.swift +++ b/Frameworks/Account/AccountManager.swift @@ -138,6 +138,8 @@ public final class AccountManager: UnreadCountProvider { return } + account.prepareForDeletion() + accountsDictionary.removeValue(forKey: account.accountID) account.isDeleted = true diff --git a/Frameworks/Account/Feedbin/FeedbinAccountDelegate.swift b/Frameworks/Account/Feedbin/FeedbinAccountDelegate.swift index ea3027a4f..0c741921f 100644 --- a/Frameworks/Account/Feedbin/FeedbinAccountDelegate.swift +++ b/Frameworks/Account/Feedbin/FeedbinAccountDelegate.swift @@ -541,6 +541,9 @@ final class FeedbinAccountDelegate: AccountDelegate { credentials = try? account.retrieveCredentials(type: .basic) } + func accountWillBeDeleted(_ account: Account) { + } + static func validateCredentials(transport: Transport, credentials: Credentials, endpoint: URL? = nil, completion: @escaping (Result) -> Void) { let caller = FeedbinAPICaller(transport: transport) diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift index fbec23b02..d80e7699f 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift +++ b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift @@ -486,6 +486,10 @@ final class FeedlyAccountDelegate: AccountDelegate { operationQueue.addOperation(refreshAccessToken) } + func accountWillBeDeleted(_ account: Account) { + + } + static func validateCredentials(transport: Transport, credentials: Credentials, endpoint: URL?, completion: @escaping (Result) -> Void) { assertionFailure("An `account` instance should enqueue an \(FeedlyRefreshAccessTokenOperation.self) instead.") completion(.success(credentials)) diff --git a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift index 25a62e74c..fb8412438 100644 --- a/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift +++ b/Frameworks/Account/LocalAccount/LocalAccountDelegate.swift @@ -195,6 +195,9 @@ final class LocalAccountDelegate: AccountDelegate { func accountDidInitialize(_ account: Account) { } + + func accountWillBeDeleted(_ account: Account) { + } static func validateCredentials(transport: Transport, credentials: Credentials, endpoint: URL? = nil, completion: (Result) -> Void) { return completion(.success(nil)) diff --git a/Frameworks/Account/ReaderAPI/ReaderAPIAccountDelegate.swift b/Frameworks/Account/ReaderAPI/ReaderAPIAccountDelegate.swift index 4de3ff830..b396d0537 100644 --- a/Frameworks/Account/ReaderAPI/ReaderAPIAccountDelegate.swift +++ b/Frameworks/Account/ReaderAPI/ReaderAPIAccountDelegate.swift @@ -418,6 +418,9 @@ final class ReaderAPIAccountDelegate: AccountDelegate { credentials = try? account.retrieveCredentials(type: .readerAPIKey) } + func accountWillBeDeleted(_ account: Account) { + } + static func validateCredentials(transport: Transport, credentials: Credentials, endpoint: URL?, completion: @escaping (Result) -> Void) { guard let endpoint = endpoint else { completion(.failure(TransportError.noURL)) diff --git a/Mac/Preferences/Accounts/AccountsDetailViewController.swift b/Mac/Preferences/Accounts/AccountsDetailViewController.swift index 902293e39..4a04625f7 100644 --- a/Mac/Preferences/Accounts/AccountsDetailViewController.swift +++ b/Mac/Preferences/Accounts/AccountsDetailViewController.swift @@ -28,6 +28,19 @@ final class AccountsDetailViewController: NSViewController, NSTextFieldDelegate super.init(coder: coder) } + private var hidesCredentialsButton: Bool { + guard let account = account else { + return true + } + switch account.type { + case .onMyMac, + .feedly: + return true + default: + return false + } + } + override func viewDidLoad() { super.viewDidLoad() @@ -35,7 +48,7 @@ final class AccountsDetailViewController: NSViewController, NSTextFieldDelegate typeLabel.stringValue = account?.defaultName ?? "" nameTextField.stringValue = account?.name ?? "" activeButton.state = account?.isActive ?? false ? .on : .off - credentialsButton.isHidden = account?.type ?? .onMyMac == .onMyMac + credentialsButton.isHidden = hidesCredentialsButton } func controlTextDidEndEditing(_ obj: Notification) { @@ -66,8 +79,6 @@ final class AccountsDetailViewController: NSViewController, NSTextFieldDelegate accountsFreshRSSWindowController.account = account accountsFreshRSSWindowController.runSheetOnWindow(self.view.window!) accountsWindowController = accountsFreshRSSWindowController - case .feedly: - assertionFailure("Implement feedly logout window controller") break default: break diff --git a/iOS/Inspector/AccountInspectorViewController.swift b/iOS/Inspector/AccountInspectorViewController.swift index b4e3b4d7a..09d41e12c 100644 --- a/iOS/Inspector/AccountInspectorViewController.swift +++ b/iOS/Inspector/AccountInspectorViewController.swift @@ -64,10 +64,20 @@ class AccountInspectorViewController: UITableViewController { } @IBAction func deleteAccount(_ sender: Any) { - let title = NSLocalizedString("Delete Account", comment: "Delete Account") - let message = NSLocalizedString("Are you sure you want to delete this account? This can not be undone.", comment: "Delete Account") - let alertController = UIAlertController(title: title, message: message, preferredStyle: .alert) + guard let account = account else { + return + } + let title = NSLocalizedString("Delete Account", comment: "Delete Account") + let message: String = { + switch account.type { + case .feedly: + return NSLocalizedString("Are you sure you want to delete this account? NetNewsWire will no longer be able to access articles and feeds unless the account is added again.", comment: "Log Out and Delete Account") + default: + return NSLocalizedString("Are you sure you want to delete this account? This can not be undone.", comment: "Delete Account") + } + }() + let alertController = UIAlertController(title: title, message: message, preferredStyle: .alert) let cancelTitle = NSLocalizedString("Cancel", comment: "Cancel") let cancelAction = UIAlertAction(title: cancelTitle, style: .cancel) alertController.addAction(cancelAction) @@ -86,19 +96,31 @@ class AccountInspectorViewController: UITableViewController { present(alertController, animated: true) } - } // MARK: Table View extension AccountInspectorViewController { + + var hidesCredentialsSection: Bool { + guard let account = account else { + return true + } + switch account.type { + case .onMyMac, + .feedly: + return true + default: + return false + } + } override func numberOfSections(in tableView: UITableView) -> Int { guard let account = account else { return 0 } if account == AccountManager.shared.defaultAccount { return 1 - } else if account.type == .onMyMac { + } else if hidesCredentialsSection { return 2 } else { return super.numberOfSections(in: tableView) @@ -124,7 +146,7 @@ extension AccountInspectorViewController { override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell { let cell: UITableViewCell - if indexPath.section == 1, let account = account, account.type == .onMyMac { + if indexPath.section == 1, hidesCredentialsSection { cell = super.tableView(tableView, cellForRowAt: IndexPath(row: 0, section: 2)) } else { cell = super.tableView(tableView, cellForRowAt: indexPath)