From cbb481c3f7f8cc753ce65c7ddaf22c71a044c9e4 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 31 Oct 2020 17:26:43 -0500 Subject: [PATCH 1/6] Don't allow a feed to be in more than one folder for Reader API accounts --- Account/Sources/Account/AccountBehaviors.swift | 5 +++++ .../ReaderAPI/ReaderAPIAccountDelegate.swift | 2 +- .../Sidebar/SidebarOutlineDataSource.swift | 18 ++++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/Account/Sources/Account/AccountBehaviors.swift b/Account/Sources/Account/AccountBehaviors.swift index 90776cdd3..fed082b4c 100644 --- a/Account/Sources/Account/AccountBehaviors.swift +++ b/Account/Sources/Account/AccountBehaviors.swift @@ -28,6 +28,11 @@ public enum AccountBehavior: Equatable { */ case disallowFeedInRootFolder + /** + Account doesn't support a feed being in more than one folder. + */ + case disallowFeedInMultipleFolders + /** Account doesn't support folders */ diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift index 60eeecd14..8d9310cc8 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift @@ -29,7 +29,7 @@ final class ReaderAPIAccountDelegate: AccountDelegate { private var log = OSLog(subsystem: Bundle.main.bundleIdentifier!, category: "ReaderAPI") var behaviors: AccountBehaviors { - var behaviors: AccountBehaviors = [.disallowOPMLImports] + var behaviors: AccountBehaviors = [.disallowOPMLImports, .disallowFeedInMultipleFolders] if variant == .freshRSS { behaviors.append(.disallowFeedInRootFolder) } diff --git a/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift b/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift index ec919b073..df593b664 100644 --- a/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift +++ b/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift @@ -644,6 +644,10 @@ private extension SidebarOutlineDataSource { return true } + if violatesDisallowFeedInMultipleFolders(parentNode, draggedFeeds) { + return true + } + return false } @@ -677,6 +681,20 @@ private extension SidebarOutlineDataSource { return false } + func violatesDisallowFeedInMultipleFolders(_ parentNode: Node, _ draggedFeeds: Set) -> Bool { + guard let parentAccount = nodeAccount(parentNode), parentAccount.behaviors.contains(.disallowFeedInMultipleFolders) else { + return false + } + + for draggedFeed in draggedFeeds { + if parentAccount.hasWebFeed(withURL: draggedFeed.url) { + return true + } + } + + return false + } + func indexWhereDraggedFeedWouldAppear(_ parentNode: Node, _ draggedFeed: PasteboardWebFeed) -> Int { let draggedFeedWrapper = PasteboardFeedObjectWrapper(pasteboardFeed: draggedFeed) let draggedFeedNode = Node(representedObject: draggedFeedWrapper, parent: nil) From 5fdbd4b9d0c1d7c538fcc8380aba756026342ef6 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 31 Oct 2020 17:31:48 -0500 Subject: [PATCH 2/6] Fixed misleading variable name --- .../Sidebar/SidebarOutlineDataSource.swift | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift b/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift index df593b664..fe244ac27 100644 --- a/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift +++ b/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift @@ -631,63 +631,63 @@ private extension SidebarOutlineDataSource { return false } - func violatesAccountSpecificBehavior(_ parentNode: Node, _ draggedFeed: PasteboardWebFeed) -> Bool { - return violatesAccountSpecificBehavior(parentNode, Set([draggedFeed])) + func violatesAccountSpecificBehavior(_ dropTargetNode: Node, _ draggedFeed: PasteboardWebFeed) -> Bool { + return violatesAccountSpecificBehavior(dropTargetNode, Set([draggedFeed])) } - func violatesAccountSpecificBehavior(_ parentNode: Node, _ draggedFeeds: Set) -> Bool { - if violatesDisallowFeedInRootFolder(parentNode) { + func violatesAccountSpecificBehavior(_ dropTargetNode: Node, _ draggedFeeds: Set) -> Bool { + if violatesDisallowFeedInRootFolder(dropTargetNode) { return true } - if violatesDisallowFeedCopyInRootFolder(parentNode, draggedFeeds) { + if violatesDisallowFeedCopyInRootFolder(dropTargetNode, draggedFeeds) { return true } - if violatesDisallowFeedInMultipleFolders(parentNode, draggedFeeds) { + if violatesDisallowFeedInMultipleFolders(dropTargetNode, draggedFeeds) { return true } return false } - func violatesDisallowFeedInRootFolder(_ parentNode: Node) -> Bool { - guard let parentAccount = nodeAccount(parentNode), parentAccount.behaviors.contains(.disallowFeedInRootFolder) else { + func violatesDisallowFeedInRootFolder(_ dropTargetNode: Node) -> Bool { + guard let parentAccount = nodeAccount(dropTargetNode), parentAccount.behaviors.contains(.disallowFeedInRootFolder) else { return false } - if parentNode.representedObject is Account { + if dropTargetNode.representedObject is Account { return true } return false } - func violatesDisallowFeedCopyInRootFolder(_ parentNode: Node, _ draggedFeeds: Set) -> Bool { - guard let parentAccount = nodeAccount(parentNode), parentAccount.behaviors.contains(.disallowFeedCopyInRootFolder) else { + func violatesDisallowFeedCopyInRootFolder(_ dropTargetNode: Node, _ draggedFeeds: Set) -> Bool { + guard let dropTargetAccount = nodeAccount(dropTargetNode), dropTargetAccount.behaviors.contains(.disallowFeedCopyInRootFolder) else { return false } for draggedFeed in draggedFeeds { - if parentAccount.accountID != draggedFeed.accountID { + if dropTargetAccount.accountID != draggedFeed.accountID { return false } } - if parentNode.representedObject is Account && (NSApplication.shared.currentEvent?.modifierFlags.contains(.option) ?? false) { + if dropTargetNode.representedObject is Account && (NSApplication.shared.currentEvent?.modifierFlags.contains(.option) ?? false) { return true } return false } - func violatesDisallowFeedInMultipleFolders(_ parentNode: Node, _ draggedFeeds: Set) -> Bool { - guard let parentAccount = nodeAccount(parentNode), parentAccount.behaviors.contains(.disallowFeedInMultipleFolders) else { + func violatesDisallowFeedInMultipleFolders(_ dropTargetNode: Node, _ draggedFeeds: Set) -> Bool { + guard let dropTargetAccount = nodeAccount(dropTargetNode), dropTargetAccount.behaviors.contains(.disallowFeedInMultipleFolders) else { return false } for draggedFeed in draggedFeeds { - if parentAccount.hasWebFeed(withURL: draggedFeed.url) { + if dropTargetAccount.hasWebFeed(withURL: draggedFeed.url) { return true } } From 2395c0c7df062a2a94ea21befbfc88a41e7c164c Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 31 Oct 2020 17:37:25 -0500 Subject: [PATCH 3/6] Correct validation so that we can still move feeds in Reader API accounts --- Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift b/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift index fe244ac27..11ad9399f 100644 --- a/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift +++ b/Mac/MainWindow/Sidebar/SidebarOutlineDataSource.swift @@ -687,8 +687,14 @@ private extension SidebarOutlineDataSource { } for draggedFeed in draggedFeeds { - if dropTargetAccount.hasWebFeed(withURL: draggedFeed.url) { - return true + if dropTargetAccount.accountID == draggedFeed.accountID { + if NSApplication.shared.currentEvent?.modifierFlags.contains(.option) ?? false { + return true + } + } else { + if dropTargetAccount.hasWebFeed(withURL: draggedFeed.url) { + return true + } } } From 75fb3fa5fbfe59e4ac87a10ba9a3966da93aca2a Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 31 Oct 2020 20:14:50 -0500 Subject: [PATCH 4/6] Implement account specific drag and drop behaviors for iOS --- .../Cell/MasterFeedTableViewIdentifier.swift | 15 +++++++++++++++ .../MasterFeedViewController+Drop.swift | 14 ++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/iOS/MasterFeed/Cell/MasterFeedTableViewIdentifier.swift b/iOS/MasterFeed/Cell/MasterFeedTableViewIdentifier.swift index e6d8172bb..9173f70e4 100644 --- a/iOS/MasterFeed/Cell/MasterFeedTableViewIdentifier.swift +++ b/iOS/MasterFeed/Cell/MasterFeedTableViewIdentifier.swift @@ -18,6 +18,7 @@ final class MasterFeedTableViewIdentifier: NSObject, NSCopying { let isEditable: Bool let isPsuedoFeed: Bool + let isAccount: Bool let isFolder: Bool let isWebFeed: Bool @@ -26,6 +27,19 @@ final class MasterFeedTableViewIdentifier: NSObject, NSCopying { let unreadCount: Int let childCount: Int + var account: Account? { + if isAccount, let containerID = containerID { + return AccountManager.shared.existingContainer(with: containerID) as? Account + } + if isFolder, let parentContainerID = parentContainerID { + return AccountManager.shared.existingContainer(with: parentContainerID) as? Account + } + if isWebFeed, let feedID = feedID { + return (AccountManager.shared.existingFeed(with: feedID) as? WebFeed)?.account + } + return nil + } + init(node: Node, unreadCount: Int) { let feed = node.representedObject as! Feed self.feedID = feed.feedID @@ -34,6 +48,7 @@ final class MasterFeedTableViewIdentifier: NSObject, NSCopying { self.isEditable = !(node.representedObject is PseudoFeed) self.isPsuedoFeed = node.representedObject is PseudoFeed + self.isAccount = node.representedObject is Account self.isFolder = node.representedObject is Folder self.isWebFeed = node.representedObject is WebFeed self.nameForDisplay = feed.nameForDisplay diff --git a/iOS/MasterFeed/MasterFeedViewController+Drop.swift b/iOS/MasterFeed/MasterFeedViewController+Drop.swift index 8692e3020..32fab4f7d 100644 --- a/iOS/MasterFeed/MasterFeedViewController+Drop.swift +++ b/iOS/MasterFeed/MasterFeedViewController+Drop.swift @@ -22,10 +22,24 @@ extension MasterFeedViewController: UITableViewDropDelegate { destIndexPath.section > 0, tableView.hasActiveDrag, let destIdentifier = dataSource.itemIdentifier(for: destIndexPath), + let destAccount = destIdentifier.account, let destCell = tableView.cellForRow(at: destIndexPath) else { return UITableViewDropProposal(operation: .forbidden) } + + // Validate account specific behaviors... + if destAccount.behaviors.contains(.disallowFeedInRootFolder) && destIdentifier.isAccount { + return UITableViewDropProposal(operation: .forbidden) + } + if destAccount.behaviors.contains(.disallowFeedInMultipleFolders), + let sourceFeedID = (session.localDragSession?.items.first?.localObject as? MasterFeedTableViewIdentifier)?.feedID, + let sourceWebFeed = AccountManager.shared.existingFeed(with: sourceFeedID) as? WebFeed, + sourceWebFeed.account?.accountID != destAccount.accountID && destAccount.hasWebFeed(withURL: sourceWebFeed.url) { + return UITableViewDropProposal(operation: .forbidden) + } + + // Determine the correct drop proposal if destIdentifier.isFolder { if session.location(in: destCell).y >= 0 { return UITableViewDropProposal(operation: .move, intent: .insertIntoDestinationIndexPath) From 100a596d023b4a1ba13099520d11db568a9eedaa Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 31 Oct 2020 21:01:58 -0500 Subject: [PATCH 5/6] Make sure all the read and starred statuses are recorded before retrieving the missing articles --- .../ReaderAPI/ReaderAPIAccountDelegate.swift | 46 +++++++++++++++---- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift index 8d9310cc8..219f337b6 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift @@ -186,8 +186,9 @@ final class ReaderAPIAccountDelegate: AccountDelegate { caller.retrieveItemIDs(type: .unread) { result in switch result { case .success(let articleIDs): - self.syncArticleReadState(account: account, articleIDs: articleIDs) - group.leave() + self.syncArticleReadState(account: account, articleIDs: articleIDs) { + group.leave() + } case .failure(let error): os_log(.info, log: self.log, "Retrieving unread entries failed: %@.", error.localizedDescription) group.leave() @@ -199,8 +200,9 @@ final class ReaderAPIAccountDelegate: AccountDelegate { caller.retrieveItemIDs(type: .starred) { result in switch result { case .success(let articleIDs): - self.syncArticleStarredState(account: account, articleIDs: articleIDs) - group.leave() + self.syncArticleStarredState(account: account, articleIDs: articleIDs) { + group.leave() + } case .failure(let error): os_log(.info, log: self.log, "Retrieving starred entries failed: %@.", error.localizedDescription) group.leave() @@ -1001,7 +1003,7 @@ private extension ReaderAPIAccountDelegate { } - func syncArticleReadState(account: Account, articleIDs: [String]?) { + func syncArticleReadState(account: Account, articleIDs: [String]?, completion: @escaping (() -> Void)) { guard let articleIDs = articleIDs else { return } @@ -1016,13 +1018,25 @@ private extension ReaderAPIAccountDelegate { return } + let group = DispatchGroup() + // Mark articles as unread let deltaUnreadArticleIDs = updatableReaderUnreadArticleIDs.subtracting(currentUnreadArticleIDs) - account.markAsUnread(deltaUnreadArticleIDs) + group.enter() + account.markAsUnread(deltaUnreadArticleIDs) { _ in + group.leave() + } // Mark articles as read let deltaReadArticleIDs = currentUnreadArticleIDs.subtracting(updatableReaderUnreadArticleIDs) - account.markAsRead(deltaReadArticleIDs) + group.enter() + account.markAsRead(deltaReadArticleIDs) { _ in + group.leave() + } + + group.notify(queue: DispatchQueue.main) { + completion() + } } } @@ -1037,7 +1051,7 @@ private extension ReaderAPIAccountDelegate { } - func syncArticleStarredState(account: Account, articleIDs: [String]?) { + func syncArticleStarredState(account: Account, articleIDs: [String]?, completion: @escaping (() -> Void)) { guard let articleIDs = articleIDs else { return } @@ -1052,13 +1066,25 @@ private extension ReaderAPIAccountDelegate { return } + let group = DispatchGroup() + // Mark articles as starred let deltaStarredArticleIDs = updatableReaderUnreadArticleIDs.subtracting(currentStarredArticleIDs) - account.markAsStarred(deltaStarredArticleIDs) + group.enter() + account.markAsStarred(deltaStarredArticleIDs) { _ in + group.leave() + } // Mark articles as unstarred let deltaUnstarredArticleIDs = currentStarredArticleIDs.subtracting(updatableReaderUnreadArticleIDs) - account.markAsUnstarred(deltaUnstarredArticleIDs) + group.enter() + account.markAsUnstarred(deltaUnstarredArticleIDs) { _ in + group.leave() + } + + group.notify(queue: DispatchQueue.main) { + completion() + } } } From ea1537118ccf3d628274527bb4396d5158c115ea Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Sat, 31 Oct 2020 21:02:49 -0500 Subject: [PATCH 6/6] Change the number of articles we retrieve to 150 --- .../Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift index 219f337b6..89bfa3ea4 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift @@ -920,7 +920,7 @@ private extension ReaderAPIAccountDelegate { let group = DispatchGroup() let articleIDs = Array(fetchedArticleIDs) - let chunkedArticleIDs = articleIDs.chunked(into: 100) + let chunkedArticleIDs = articleIDs.chunked(into: 150) self.refreshProgress.addToNumberOfTasksAndRemaining(chunkedArticleIDs.count - 1)