From d79720104de6072b906270a14a2e7016717e6765 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Tue, 16 Jul 2024 13:00:36 +0300 Subject: [PATCH 1/8] Improve AttachmentFileCleanupController code --- .../AttachmentFileCleanupController.swift | 417 +++++++++--------- .../ViewModels/ItemDetailActionHandler.swift | 2 +- .../Items/ViewModels/ItemsActionHandler.swift | 2 +- 3 files changed, 206 insertions(+), 215 deletions(-) diff --git a/Zotero/Controllers/AttachmentFileCleanupController.swift b/Zotero/Controllers/AttachmentFileCleanupController.swift index d2a6109f8..8f9342837 100644 --- a/Zotero/Controllers/AttachmentFileCleanupController.swift +++ b/Zotero/Controllers/AttachmentFileCleanupController.swift @@ -21,10 +21,17 @@ final class AttachmentFileCleanupController { var notification: AttachmentFileDeletedNotification { switch self { - case .all: return .all - case .library(let id): return .library(id) - case .allForItems(let keys, let libraryId): return .allForItems(keys: keys, libraryId: libraryId) - case .individual(let attachment, let parentKey): return .individual(key: attachment.key, parentKey: parentKey, libraryId: attachment.libraryId) + case .all: + return .all + + case .library(let id): + return .library(id) + + case .allForItems(let keys, let libraryId): + return .allForItems(keys: keys, libraryId: libraryId) + + case .individual(let attachment, let parentKey): + return .individual(key: attachment.key, parentKey: parentKey, libraryId: attachment.libraryId) } } } @@ -36,39 +43,30 @@ final class AttachmentFileCleanupController { private let disposeBag: DisposeBag init(fileStorage: FileStorage, dbStorage: DbStorage) { - let queue = DispatchQueue(label: "org.zotero.FileCleanupQueue", qos: .userInitiated) self.fileStorage = fileStorage self.dbStorage = dbStorage - self.scheduler = SerialDispatchQueueScheduler(queue: queue, internalSerialQueueName: "org.zotero.FileCleanupScheduler") - self.queue = queue - self.disposeBag = DisposeBag() + queue = DispatchQueue(label: "org.zotero.FileCleanupQueue", qos: .userInitiated) + scheduler = SerialDispatchQueueScheduler(queue: queue, internalSerialQueueName: "org.zotero.FileCleanupScheduler") + disposeBag = DisposeBag() NotificationCenter.default.rx .notification(.attachmentDeleted) - .observe(on: self.scheduler) - .subscribe(onNext: { [weak self] notification in - if let file = notification.object as? File { - self?.delete(file: file) - } + .observe(on: scheduler) + .subscribe(onNext: { [weak fileStorage] notification in + guard let fileStorage, let file = notification.object as? File else { return } + // Don't need to check for errors, the attachment doesn't have to have the file downloaded locally, so this will throw for all attachments + // without local files. If the file was not removed properly it can always be seen and done in settings. + try? fileStorage.remove(file) }) - .disposed(by: self.disposeBag) - } - - private func delete(file: File) { - // Don't need to check for errors, the attachment doesn't have to have the file downloaded locally, so this will throw for all attachments - // without local files. If the file was not removed properly it can always be seen and done in settings. - try? self.fileStorage.remove(file) + .disposed(by: disposeBag) } /// Delete attachments based on deletion type. /// - parameter type: Indicates which attachments should be deleted. /// - completed: Called on main queue when attachments are deleted. `true` if attachments were deleted, `false` in case of error. - func delete(_ type: DeletionType, completed: ((Bool) -> Void)?) { - self.queue.async { - let newTypes = self.delete(type) - - guard !newTypes.isEmpty || completed != nil else { return } - + func delete(_ type: DeletionType, completed: ((Bool) -> Void)? = nil) { + queue.async { [weak self] in + let newTypes = self?.delete(type) ?? [] DispatchQueue.main.async { for type in newTypes { NotificationCenter.default.post(name: .attachmentFileDeleted, object: type.notification) @@ -84,246 +82,239 @@ final class AttachmentFileCleanupController { private func delete(_ type: DeletionType) -> [DeletionType] { switch type { case .all: - return self.deleteAll() + return deleteAll() case .allForItems(let keys, let libraryId): - return self.deleteAttachments(for: keys, libraryId: libraryId).flatMap({ [$0] }) ?? [] + return deleteAttachments(for: keys, libraryId: libraryId).flatMap({ [$0] }) ?? [] case .library(let libraryId): - return self.delete(in: libraryId).flatMap({ [$0] }) ?? [] + return delete(in: libraryId).flatMap({ [$0] }) ?? [] case .individual(let attachment, _): - if self.delete(attachment: attachment) { - return [type] - } - return [] + return delete(attachment: attachment) ? [type] : [] } - } - /// Tries deleting all attachments. - /// - returns: Either `.all` deletion type if all attachments can be deleted. If only some can be deleted, it returns `.allForItems` for individual libraries. In case of error empty array is returned. - private func deleteAll() -> [DeletionType] { - do { - var libraryIds: [LibraryIdentifier] = [] - var forUpload: [LibraryIdentifier: [String]] = [:] + /// Tries deleting all attachments. + /// - returns: Either `.all` deletion type if all attachments can be deleted. If only some can be deleted, it returns `.allForItems` for individual libraries. + /// In case of error empty array is returned. + func deleteAll() -> [DeletionType] { + do { + var libraryIds: [LibraryIdentifier] = [] + var forUpload: [LibraryIdentifier: [String]] = [:] - try self.dbStorage.perform(on: self.queue, with: { coordinator in - let groups = try coordinator.perform(request: ReadAllGroupsDbRequest()) - libraryIds = [.custom(.myLibrary)] + groups.map({ .group($0.identifier) }) + try dbStorage.perform(on: queue, with: { coordinator in + let groups = try coordinator.perform(request: ReadAllGroupsDbRequest()) + libraryIds = [.custom(.myLibrary)] + groups.map({ .group($0.identifier) }) - for item in try coordinator.perform(request: ReadAllItemsForUploadDbRequest()) { - guard let libraryId = item.libraryId else { continue } + for item in try coordinator.perform(request: ReadAllItemsForUploadDbRequest()) { + guard let libraryId = item.libraryId else { continue } - if var keys = forUpload[libraryId] { + var keys = forUpload[libraryId, default: []] keys.append(item.key) forUpload[libraryId] = keys - } else { - forUpload[libraryId] = [item.key] } - } - // TODO: - exclude those for upload - try? coordinator.perform(request: MarkAllFilesAsNotDownloadedDbRequest()) + // TODO: - exclude those for upload + try? coordinator.perform(request: MarkAllFilesAsNotDownloadedDbRequest()) - coordinator.invalidate() - }) + coordinator.invalidate() + }) - let deletedIndividually = self.delete(downloadsIn: libraryIds, forUpload: forUpload) - // Annotations are not guaranteed to exist and they can be removed even if the parent PDF was not deleted due to upload state. - // These are generated on device, so they'll just be recreated. - try? self.fileStorage.remove(Files.annotationPreviews) - // Remove page thumbnails - try? self.fileStorage.remove(Files.pageThumbnails) - // When removing all local files clear cache as well. - try? self.fileStorage.remove(Files.cache) - - if deletedIndividually.isEmpty { - return [.all] - } + let deletedIndividually = delete(downloadsIn: libraryIds, forUpload: forUpload) + // Annotations are not guaranteed to exist and they can be removed even if the parent PDF was not deleted due to upload state. + // These are generated on device, so they'll just be recreated. + try? fileStorage.remove(Files.annotationPreviews) + // Remove page thumbnails + try? fileStorage.remove(Files.pageThumbnails) + // When removing all local files clear cache as well. + try? fileStorage.remove(Files.cache) - return deletedIndividually.map { libraryId, keys in - return .allForItems(keys, libraryId) + if deletedIndividually.isEmpty { + return [.all] + } + + return deletedIndividually.map { libraryId, keys in + return .allForItems(keys, libraryId) + } + } catch let error { + DDLogError("AttachmentFileCleanupController: can't remove download directory - \(error)") + return [] } - } catch let error { - DDLogError("AttachmentFileCleanupController: can't remove download directory - \(error)") - return [] } - } - /// Tries deleting all attachments in given library. - /// - returns: Either `.library` deletion type if all attachments can be deleted. If only some can be deleted, it returns `.allForItems` for given library. In case of error `nil` is returned. - private func delete(in libraryId: LibraryIdentifier) -> DeletionType? { - do { - var forUpload: [String] = [] - - try self.dbStorage.perform(on: self.queue, with: { coordinator in - let items = try coordinator.perform(request: ReadItemsForUploadDbRequest(libraryId: libraryId)) - forUpload = Array(items.map({ $0.key })) + /// Tries deleting individual files from given library. + /// - parameter keys: Individual keys of files to delete. + /// - parameter libraryId: Library identifier from which files are deleted. + /// - returns: `nil` if no files could be deleted. `.allForItems` for actually deleted files. + func deleteAttachments(for keys: Set, libraryId: LibraryIdentifier) -> DeletionType? { + guard !keys.isEmpty else { return nil } + + do { + var toDelete: Set = [] + var toReport: Set = [] + + try dbStorage.perform(on: queue, with: { coordinator in + let items = try coordinator.perform(request: ReadItemsWithKeysDbRequest(keys: keys, libraryId: libraryId)) + + for item in items { + // Either the selected item was attachment + if item.rawType == ItemTypes.attachment { + // Don't delete attachments that need to sync + guard !item.attachmentNeedsSync else { continue } + toDelete.insert(item.key) + toReport.insert(item.key) + continue + } + + // Or the item was a parent item and it may have multiple attachments + for child in item.children.filter(.item(type: ItemTypes.attachment)) { + // Don't delete attachments that need to sync + guard !child.attachmentNeedsSync else { continue } + // Always report parent key so that the originally requested key gets a report about deletion, even if a child was deleted. + toReport.insert(item.key) + toDelete.insert(child.key) + } + } - // TODO: - exclude those for upload - try? coordinator.perform(request: MarkLibraryFilesAsNotDownloadedDbRequest(libraryId: libraryId)) + try? coordinator.perform(request: MarkItemsFilesAsNotDownloadedDbRequest(keys: toDelete, libraryId: libraryId)) - coordinator.invalidate() - }) + coordinator.invalidate() + }) - let deletedIndividually = self.delete(downloadsIn: [libraryId], forUpload: [libraryId: forUpload]) - - // Annotations are not guaranteed to exist and they can be removed even if the parent PDF was not deleted due to upload state. - // These are generated on device, so they'll just be recreated. - try? self.fileStorage.remove(Files.annotationPreviews(for: libraryId)) - // Cleanup page thumbnails - try? self.fileStorage.remove(Files.pageThumbnails(for: libraryId)) + for key in toDelete { + // Some files might not exist, just continue deleting + try? removeFiles(for: key, libraryId: libraryId) + } - if let keys = deletedIndividually[libraryId], !keys.isEmpty { - return .allForItems(keys, libraryId) + return toReport.isEmpty ? nil : .allForItems(toReport, libraryId) + } catch let error { + DDLogError("AttachmentFileCleanupController: can't remove attachments for item - \(error)") + return nil } - return .library(libraryId) - } catch let error { - DDLogError("AttachmentFileCleanupController: can't remove library downloads - \(error)") - return nil } - } - /// Deletes downloads in given libraries, but keeps files queued for upload intact. - /// - parameter libraries: Libraries to delete from. - /// - parameter forUpload: Keys of items which need upload for given libraries. - /// - returns: Returns individual keys for given libraries which were actually deleted. Some deletions are ignored if files are queued for upload. - private func delete(downloadsIn libraries: [LibraryIdentifier], forUpload: [LibraryIdentifier: [String]]) -> [LibraryIdentifier: Set] { - var deletedIndividually: [LibraryIdentifier: Set] = [:] - - for libraryId in libraries { - guard let keysForUpload = forUpload[libraryId], !keysForUpload.isEmpty else { - // No items are queued for upload, just delete the whole directory. Ignore thrown error because some may not exist locally (user didn't download anything in given library yet). - try? self.fileStorage.remove(Files.downloads(for: libraryId)) - continue - } + /// Tries deleting all attachments in given library. + /// - returns: Either `.library` deletion type if all attachments can be deleted. If only some can be deleted, it returns `.allForItems` for given library. In case of error `nil` is returned. + func delete(in libraryId: LibraryIdentifier) -> DeletionType? { + do { + var forUpload: [String] = [] - // If there are pending uploads, delete only directories with uploaded files. If no folders are stored locally just skip. - guard let contents: [File] = try? self.fileStorage.contentsOfDirectory(at: Files.downloads(for: libraryId)) else { continue } + try dbStorage.perform(on: self.queue, with: { coordinator in + let items = try coordinator.perform(request: ReadItemsForUploadDbRequest(libraryId: libraryId)) + forUpload = Array(items.map({ $0.key })) - let toDelete = contents.filter({ file in - // Check whether it's item folder and whether upload is pending for given item. - if file.relativeComponents.count == 3, let key = file.relativeComponents.last, key.count == KeyGenerator.length { - return !keysForUpload.contains(key) - } - // If it's something else, just delete it - return true - }) + // TODO: - exclude those for upload + try? coordinator.perform(request: MarkLibraryFilesAsNotDownloadedDbRequest(libraryId: libraryId)) - var keys: Set = [] - for file in toDelete { - if file.relativeComponents.count == 3, let key = file.relativeComponents.last, key.count == KeyGenerator.length { - keys.insert(key) - } - } - deletedIndividually[libraryId] = keys + coordinator.invalidate() + }) - for file in toDelete { - do { - try self.fileStorage.remove(file) - } catch let error { - DDLogError("AttachmentFileCleanupController: could not remove file \(file) - \(error)") + let deletedIndividually = delete(downloadsIn: [libraryId], forUpload: [libraryId: forUpload]) + + // Annotations are not guaranteed to exist and they can be removed even if the parent PDF was not deleted due to upload state. + // These are generated on device, so they'll just be recreated. + try? fileStorage.remove(Files.annotationPreviews(for: libraryId)) + // Cleanup page thumbnails + try? fileStorage.remove(Files.pageThumbnails(for: libraryId)) + + if let keys = deletedIndividually[libraryId], !keys.isEmpty { + return .allForItems(keys, libraryId) } + return .library(libraryId) + } catch let error { + DDLogError("AttachmentFileCleanupController: can't remove library downloads - \(error)") + return nil } } - return deletedIndividually - } + /// Tries deleting given attachment. + /// - parameter attachment: Attachment to delete. + /// - returns: `true` if file could be deleted, `false` otherwise. + func delete(attachment: Attachment) -> Bool { + do { + // Don't delete linked files + guard case .file(_, _, _, let linkType, _) = attachment.type, linkType != .linkedFile else { return false } - /// Tries deleting given attachment. - /// - parameter attachment: Attachment to delete. - /// - returns: `true` if file could be deleted, `false` otherwise. - private func delete(attachment: Attachment) -> Bool { - do { - // Don't delete linked files - guard case .file(_, _, _, let linkType, _) = attachment.type, linkType != .linkedFile else { return false } + var canDelete: Bool = false - var canDelete: Bool = false + try dbStorage.perform(on: queue, with: { coordinator in + let item = try coordinator.perform(request: ReadItemDbRequest(libraryId: attachment.libraryId, key: attachment.key)) + let attachmentNeedsSync = item.attachmentNeedsSync - try self.dbStorage.perform(on: self.queue, with: { coordinator in - let item = try coordinator.perform(request: ReadItemDbRequest(libraryId: attachment.libraryId, key: attachment.key)) - let attachmentNeedsSync = item.attachmentNeedsSync + // Don't delete attachments that need to sync + guard !attachmentNeedsSync else { + canDelete = false + return + } - // Don't delete attachments that need to sync - guard !attachmentNeedsSync else { - canDelete = false - return - } + try? coordinator.perform(request: MarkFileAsDownloadedDbRequest(key: attachment.key, libraryId: attachment.libraryId, downloaded: false)) - try? coordinator.perform(request: MarkFileAsDownloadedDbRequest(key: attachment.key, libraryId: attachment.libraryId, downloaded: false)) + coordinator.invalidate() - coordinator.invalidate() + canDelete = true + }) - canDelete = true - }) + if canDelete { + try? removeFiles(for: attachment.key, libraryId: attachment.libraryId) + } - if canDelete { - try? self.removeFiles(for: attachment.key, libraryId: attachment.libraryId) + return canDelete + } catch let error { + DDLogError("AttachmentFileCleanupController: can't remove attachment file - \(error)") + return false } - - return canDelete - } catch let error { - DDLogError("AttachmentFileCleanupController: can't remove attachment file - \(error)") - return false } - } - /// Tries deleting individual files from given library. - /// - parameter keys: Individual keys of files to delete. - /// - parameter libraryId: Library identifier from which files are deleted. - /// - returns: `nil` if no files could be deleted. `.allForItems` for actually deleted files. - private func deleteAttachments(for keys: Set, libraryId: LibraryIdentifier) -> DeletionType? { - guard !keys.isEmpty else { return nil } - - do { - var toDelete: Set = [] - var toReport: Set = [] - - try self.dbStorage.perform(on: self.queue, with: { coordinator in - let items = try coordinator.perform(request: ReadItemsWithKeysDbRequest(keys: keys, libraryId: libraryId)) - - for item in items { - // Either the selected item was attachment - if item.rawType == ItemTypes.attachment { - // Don't delete attachments that need to sync - guard !item.attachmentNeedsSync else { continue } - toDelete.insert(item.key) - toReport.insert(item.key) - continue - } - - // Or the item was a parent item and it may have multiple attachments - for child in item.children.filter(.item(type: ItemTypes.attachment)) { - // Don't delete attachments that need to sync - guard !child.attachmentNeedsSync else { continue } - // Always report parent key so that the originally requested key gets a report about deletion, even if a child was deleted. - toReport.insert(item.key) - toDelete.insert(child.key) - } + /// Deletes downloads in given libraries, but keeps files queued for upload intact. + /// - parameter libraries: Libraries to delete from. + /// - parameter forUpload: Keys of items which need upload for given libraries. + /// - returns: Returns individual keys for given libraries which were actually deleted. Some deletions are ignored if files are queued for upload. + func delete(downloadsIn libraries: [LibraryIdentifier], forUpload: [LibraryIdentifier: [String]]) -> [LibraryIdentifier: Set] { + var deletedIndividually: [LibraryIdentifier: Set] = [:] + + for libraryId in libraries { + let keysForUpload = forUpload[libraryId, default: []] + if keysForUpload.isEmpty { + // No items are queued for upload, just delete the whole directory. Ignore thrown error because some may not exist locally (user didn't download anything in given library yet). + try? fileStorage.remove(Files.downloads(for: libraryId)) + continue } - try? coordinator.perform(request: MarkItemsFilesAsNotDownloadedDbRequest(keys: toDelete, libraryId: libraryId)) + // If there are pending uploads, delete only directories with uploaded files. If no folders are stored locally just skip. + guard let contents: [File] = try? fileStorage.contentsOfDirectory(at: Files.downloads(for: libraryId)) else { continue } - coordinator.invalidate() - }) - - for key in toDelete { - // Some files might not exist, just continue deleting - try? self.removeFiles(for: key, libraryId: libraryId) + var keys: Set = [] + let toDelete = contents.filter({ file in + // Check whether it's item folder and whether upload is pending for given item. + guard file.relativeComponents.count == 3, let key = file.relativeComponents.last, key.count == KeyGenerator.length else { + // If it's something else, just delete it. + return true + } + guard !keysForUpload.contains(key) else { return false } + // Delete it and record it's key. + keys.insert(key) + return true + }) + deletedIndividually[libraryId] = keys + + for file in toDelete { + do { + try fileStorage.remove(file) + } catch let error { + DDLogError("AttachmentFileCleanupController: could not remove file \(file) - \(error)") + } + } } - return toReport.isEmpty ? nil : .allForItems(toReport, libraryId) - } catch let error { - DDLogError("AttachmentFileCleanupController: can't remove attachments for item - \(error)") - return nil + return deletedIndividually } - } - private func removeFiles(for key: String, libraryId: LibraryIdentifier) throws { - try self.fileStorage.remove(Files.attachmentDirectory(in: libraryId, key: key)) - // Annotations are not guaranteed to exist. - try? self.fileStorage.remove(Files.annotationPreviews(for: key, libraryId: libraryId)) - // Cleanup page thumbnails - try? self.fileStorage.remove(Files.pageThumbnails(for: key, libraryId: libraryId)) + func removeFiles(for key: String, libraryId: LibraryIdentifier) throws { + try fileStorage.remove(Files.attachmentDirectory(in: libraryId, key: key)) + // Annotations are not guaranteed to exist. + try? fileStorage.remove(Files.annotationPreviews(for: key, libraryId: libraryId)) + // Cleanup page thumbnails + try? fileStorage.remove(Files.pageThumbnails(for: key, libraryId: libraryId)) + } } } diff --git a/Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift b/Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift index 08cb4c355..b5f5ee60b 100644 --- a/Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift +++ b/Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift @@ -467,7 +467,7 @@ struct ItemDetailActionHandler: ViewModelActionHandler, BackgroundDbProcessingAc } private func deleteFile(of attachment: Attachment, in viewModel: ViewModel) { - self.fileCleanupController.delete(.individual(attachment: attachment, parentKey: viewModel.state.key), completed: nil) + self.fileCleanupController.delete(.individual(attachment: attachment, parentKey: viewModel.state.key)) } private func updateDeletedAttachmentFiles(_ notification: AttachmentFileDeletedNotification, in viewModel: ViewModel) { diff --git a/Zotero/Scenes/Detail/Items/ViewModels/ItemsActionHandler.swift b/Zotero/Scenes/Detail/Items/ViewModels/ItemsActionHandler.swift index 34258dbb1..d79f8f212 100644 --- a/Zotero/Scenes/Detail/Items/ViewModels/ItemsActionHandler.swift +++ b/Zotero/Scenes/Detail/Items/ViewModels/ItemsActionHandler.swift @@ -180,7 +180,7 @@ struct ItemsActionHandler: ViewModelActionHandler, BackgroundDbProcessingActionH self.downloadAttachments(for: keys, in: viewModel) case .removeDownloads(let ids): - self.fileCleanupController.delete(.allForItems(ids, viewModel.state.library.identifier), completed: nil) + self.fileCleanupController.delete(.allForItems(ids, viewModel.state.library.identifier)) case .emptyTrash: self.emptyTrash(in: viewModel) From acf443e24f7781a991856d96ada65e23b633f640 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Tue, 16 Jul 2024 18:24:09 +0300 Subject: [PATCH 2/8] Add remove downloads action to collection context menu --- Zotero/Assets/en.lproj/Localizable.strings | 1 + Zotero/Extensions/Localizable.swift | 2 + .../Models/CollectionsAction.swift | 1 + .../ViewModels/CollectionsActionHandler.swift | 22 ++++++- ...ableCollectionsCollectionViewHandler.swift | 60 +++++++++++-------- Zotero/Scenes/Master/MasterCoordinator.swift | 20 ++++--- 6 files changed, 72 insertions(+), 34 deletions(-) diff --git a/Zotero/Assets/en.lproj/Localizable.strings b/Zotero/Assets/en.lproj/Localizable.strings index 06cfbfe62..050a6099d 100644 --- a/Zotero/Assets/en.lproj/Localizable.strings +++ b/Zotero/Assets/en.lproj/Localizable.strings @@ -98,6 +98,7 @@ "collections.expand_all" = "Expand All"; "collections.create_bibliography" = "Create Bibliography from Collection"; "collections.download_attachments" = "Download Attachments"; +"collections.delete_attachment_files" = "Remove Downloads"; "sync_toolbar.starting" = "Sync starting"; "sync_toolbar.groups" = "Syncing groups"; diff --git a/Zotero/Extensions/Localizable.swift b/Zotero/Extensions/Localizable.swift index 8454a469d..ea7cd0999 100644 --- a/Zotero/Extensions/Localizable.swift +++ b/Zotero/Extensions/Localizable.swift @@ -343,6 +343,8 @@ internal enum L10n { internal static let createTitle = L10n.tr("Localizable", "collections.create_title", fallback: "Create Collection") /// Delete Collection internal static let delete = L10n.tr("Localizable", "collections.delete", fallback: "Delete Collection") + /// Remove Downloads + internal static let deleteAttachmentFiles = L10n.tr("Localizable", "collections.delete_attachment_files", fallback: "Remove Downloads") /// Delete Collection and Items internal static let deleteWithItems = L10n.tr("Localizable", "collections.delete_with_items", fallback: "Delete Collection and Items") /// Download Attachments diff --git a/Zotero/Scenes/Master/Collections/Models/CollectionsAction.swift b/Zotero/Scenes/Master/Collections/Models/CollectionsAction.swift index 38b0a4e6c..641db879e 100644 --- a/Zotero/Scenes/Master/Collections/Models/CollectionsAction.swift +++ b/Zotero/Scenes/Master/Collections/Models/CollectionsAction.swift @@ -21,4 +21,5 @@ enum CollectionsAction { case toggleCollapsed(Collection) case loadItemKeysForBibliography(Collection) case downloadAttachments(CollectionIdentifier) + case removeDownloads(CollectionIdentifier) } diff --git a/Zotero/Scenes/Master/Collections/ViewModels/CollectionsActionHandler.swift b/Zotero/Scenes/Master/Collections/ViewModels/CollectionsActionHandler.swift index 5e0465562..bc43375f2 100644 --- a/Zotero/Scenes/Master/Collections/ViewModels/CollectionsActionHandler.swift +++ b/Zotero/Scenes/Master/Collections/ViewModels/CollectionsActionHandler.swift @@ -19,12 +19,14 @@ struct CollectionsActionHandler: ViewModelActionHandler, BackgroundDbProcessingA private unowned let fileStorage: FileStorage unowned let dbStorage: DbStorage private unowned let attachmentDownloader: AttachmentDownloader + private unowned let fileCleanupController: AttachmentFileCleanupController - init(dbStorage: DbStorage, fileStorage: FileStorage, attachmentDownloader: AttachmentDownloader) { - self.backgroundQueue = DispatchQueue(label: "org.zotero.CollectionsActionHandler.backgroundQueue", qos: .userInitiated) + init(dbStorage: DbStorage, fileStorage: FileStorage, attachmentDownloader: AttachmentDownloader, fileCleanupController: AttachmentFileCleanupController) { + backgroundQueue = DispatchQueue(label: "org.zotero.CollectionsActionHandler.backgroundQueue", qos: .userInitiated) self.dbStorage = dbStorage self.fileStorage = fileStorage self.attachmentDownloader = attachmentDownloader + self.fileCleanupController = fileCleanupController } func process(action: CollectionsAction, in viewModel: ViewModel) { @@ -67,6 +69,9 @@ struct CollectionsActionHandler: ViewModelActionHandler, BackgroundDbProcessingA case .downloadAttachments(let identifier): self.downloadAttachments(in: identifier, viewModel: viewModel) + + case .removeDownloads(let identifier): + removeDownloads(in: identifier, viewModel: viewModel) } } @@ -106,6 +111,19 @@ struct CollectionsActionHandler: ViewModelActionHandler, BackgroundDbProcessingA } } + private func removeDownloads(in collectionId: CollectionIdentifier, viewModel: ViewModel) { + backgroundQueue.async { [weak viewModel] in + guard let viewModel else { return } + do { + let items = try dbStorage.perform(request: ReadItemsDbRequest(collectionId: collectionId, libraryId: viewModel.state.library.identifier), on: backgroundQueue) + let keys = Set(items.map { $0.key }) + fileCleanupController.delete(.allForItems(keys, viewModel.state.library.identifier)) + } catch let error { + DDLogError("CollectionsActionHandler: remove downloads - \(error)") + } + } + } + private func emptyTrash(in viewModel: ViewModel) { self.perform(request: EmptyTrashDbRequest(libraryId: viewModel.state.library.identifier)) { error in guard let error = error else { return } diff --git a/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift b/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift index ddf62b143..72dbf1866 100644 --- a/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift +++ b/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift @@ -101,59 +101,69 @@ final class ExpandableCollectionsCollectionViewHandler: NSObject { } private func createContextMenu(for collection: Collection) -> UIMenu? { + var actions: [UIAction] = [] switch collection.identifier { case .collection(let key): - var actions: [UIAction] = [] - if viewModel.state.library.metadataEditable { - let edit = UIAction(title: L10n.edit, image: UIImage(systemName: "pencil")) { [weak self] _ in - self?.viewModel.process(action: .startEditing(.edit(collection))) + let edit = UIAction(title: L10n.edit, image: UIImage(systemName: "pencil")) { [weak viewModel] _ in + viewModel?.process(action: .startEditing(.edit(collection))) } - let subcollection = UIAction(title: L10n.Collections.newSubcollection, image: UIImage(systemName: "folder.badge.plus")) { [weak self] _ in - self?.viewModel.process(action: .startEditing(.addSubcollection(collection))) + let subcollection = UIAction(title: L10n.Collections.newSubcollection, image: UIImage(systemName: "folder.badge.plus")) { [weak viewModel] _ in + viewModel?.process(action: .startEditing(.addSubcollection(collection))) } actions.append(contentsOf: [edit, subcollection]) } - let createBibliography = UIAction(title: L10n.Collections.createBibliography, image: UIImage(systemName: "doc.on.doc")) { [weak self] _ in - self?.viewModel.process(action: .loadItemKeysForBibliography(collection)) + let createBibliography = UIAction(title: L10n.Collections.createBibliography, image: UIImage(systemName: "doc.on.doc")) { [weak viewModel] _ in + viewModel?.process(action: .loadItemKeysForBibliography(collection)) } actions.append(createBibliography) if viewModel.state.library.metadataEditable { - let delete = UIAction(title: L10n.delete, image: UIImage(systemName: "trash"), attributes: .destructive) { [weak self] _ in - self?.viewModel.process(action: .deleteCollection(key)) + let delete = UIAction(title: L10n.delete, image: UIImage(systemName: "trash"), attributes: .destructive) { [weak viewModel] _ in + viewModel?.process(action: .deleteCollection(key)) } actions.append(delete) } if collection.itemCount > 0 { - let downloadAttachments = UIAction(title: L10n.Collections.downloadAttachments, image: UIImage(systemName: "arrow.down.to.line.compact")) { [weak self] _ in - self?.viewModel.process(action: .downloadAttachments(collection.identifier)) - } - actions.insert(downloadAttachments, at: 0) + actions.insert(contentsOf: [downloadAttachmentsAction(for: collection.identifier, in: viewModel), removeDownloadsAction(for: collection.identifier, in: viewModel)], at: 0) } - return UIMenu(title: "", children: actions) - case .custom(let type): + guard collection.itemCount > 0 else { break } + actions.append(removeDownloadsAction(for: collection.identifier, in: viewModel)) + switch type { case .trash: - guard self.viewModel.state.library.metadataEditable && collection.itemCount > 0 else { return nil } - let trash = UIAction(title: L10n.Collections.emptyTrash, image: UIImage(systemName: "trash"), attributes: .destructive) { [weak self] _ in - self?.viewModel.process(action: .emptyTrash) + if viewModel.state.library.metadataEditable { + let trash = UIAction(title: L10n.Collections.emptyTrash, image: UIImage(systemName: "trash"), attributes: .destructive) { [weak viewModel] _ in + viewModel?.process(action: .emptyTrash) + } + actions.append(trash) } - return UIMenu(title: "", children: [trash]) case .publications, .all, .unfiled: - let downloadAttachments = UIAction(title: L10n.Collections.downloadAttachments, image: UIImage(systemName: "arrow.down.to.line.compact")) { [weak self] _ in - self?.viewModel.process(action: .downloadAttachments(collection.identifier)) - } - return UIMenu(title: "", children: [downloadAttachments]) + actions.insert(downloadAttachmentsAction(for: collection.identifier, in: viewModel), at: 0) } case .search: - return nil + break + } + + guard !actions.isEmpty else { return nil } + return UIMenu(children: actions) + + func downloadAttachmentsAction(for identifier: CollectionIdentifier, in viewModel: ViewModel) -> UIAction { + UIAction(title: L10n.Collections.downloadAttachments, image: UIImage(systemName: "arrow.down.to.line.compact")) { [weak viewModel] _ in + viewModel?.process(action: .downloadAttachments(identifier)) + } + } + + func removeDownloadsAction(for identifier: CollectionIdentifier, in viewModel: ViewModel) -> UIAction { + UIAction(title: L10n.Collections.deleteAttachmentFiles, image: UIImage(systemName: "trash")) { [weak viewModel] _ in + viewModel?.process(action: .removeDownloads(identifier)) + } } } diff --git a/Zotero/Scenes/Master/MasterCoordinator.swift b/Zotero/Scenes/Master/MasterCoordinator.swift index ae127da86..51f2824b1 100644 --- a/Zotero/Scenes/Master/MasterCoordinator.swift +++ b/Zotero/Scenes/Master/MasterCoordinator.swift @@ -66,7 +66,8 @@ final class MasterCoordinator: NSObject, Coordinator { selectedCollectionId: Defaults.shared.selectedCollectionId, dbStorage: userControllers.dbStorage, syncScheduler: userControllers.syncScheduler, - attachmentDownloader: userControllers.fileDownloader + attachmentDownloader: userControllers.fileDownloader, + fileCleanupController: userControllers.fileCleanupController ) self.navigationController?.setViewControllers([librariesController, collectionsController], animated: animated) } @@ -83,10 +84,11 @@ final class MasterCoordinator: NSObject, Coordinator { selectedCollectionId: CollectionIdentifier, dbStorage: DbStorage, syncScheduler: SynchronizationScheduler, - attachmentDownloader: AttachmentDownloader + attachmentDownloader: AttachmentDownloader, + fileCleanupController: AttachmentFileCleanupController ) -> CollectionsViewController { DDLogInfo("MasterTopCoordinator: show collections for \(selectedCollectionId.id); \(libraryId)") - let handler = CollectionsActionHandler(dbStorage: dbStorage, fileStorage: self.controllers.fileStorage, attachmentDownloader: attachmentDownloader) + let handler = CollectionsActionHandler(dbStorage: dbStorage, fileStorage: controllers.fileStorage, attachmentDownloader: attachmentDownloader, fileCleanupController: fileCleanupController) let state = CollectionsState(libraryId: libraryId, selectedCollectionId: selectedCollectionId) let viewModel = ViewModel(initialState: state, handler: handler) return CollectionsViewController(viewModel: viewModel, dragDropController: controllers.dragDropController, syncScheduler: syncScheduler, coordinatorDelegate: self) @@ -120,7 +122,8 @@ extension MasterCoordinator: MasterLibrariesCoordinatorDelegate { selectedCollectionId: collectionId, dbStorage: userControllers.dbStorage, syncScheduler: userControllers.syncScheduler, - attachmentDownloader: userControllers.fileDownloader + attachmentDownloader: userControllers.fileDownloader, + fileCleanupController: userControllers.fileCleanupController ) let animated: Bool @@ -173,7 +176,8 @@ extension MasterCoordinator: MasterLibrariesCoordinatorDelegate { selectedCollectionId: collectionId, dbStorage: userControllers.dbStorage, syncScheduler: userControllers.syncScheduler, - attachmentDownloader: userControllers.fileDownloader + attachmentDownloader: userControllers.fileDownloader, + fileCleanupController: userControllers.fileCleanupController ) self.navigationController?.pushViewController(controller, animated: true) } @@ -190,7 +194,8 @@ extension MasterCoordinator: MasterLibrariesCoordinatorDelegate { selectedCollectionId: collectionId, dbStorage: userControllers.dbStorage, syncScheduler: userControllers.syncScheduler, - attachmentDownloader: userControllers.fileDownloader + attachmentDownloader: userControllers.fileDownloader, + fileCleanupController: userControllers.fileCleanupController ) navigationController.pushViewController(controller, animated: animated) } else if libraryId != self.visibleLibraryId { @@ -200,7 +205,8 @@ extension MasterCoordinator: MasterLibrariesCoordinatorDelegate { selectedCollectionId: collectionId, dbStorage: userControllers.dbStorage, syncScheduler: userControllers.syncScheduler, - attachmentDownloader: userControllers.fileDownloader + attachmentDownloader: userControllers.fileDownloader, + fileCleanupController: userControllers.fileCleanupController ) var viewControllers = navigationController.viewControllers From 700f9aa63e4c75e26f83adc0d6cb6240024bb8c3 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Wed, 17 Jul 2024 12:52:55 +0300 Subject: [PATCH 3/8] Improve ItemsToolbarController code --- .../ViewModels/ItemsToolbarController.swift | 259 +++++++++--------- 1 file changed, 131 insertions(+), 128 deletions(-) diff --git a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift index 5865273cb..0d05ffccf 100644 --- a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift +++ b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift @@ -37,48 +37,48 @@ final class ItemsToolbarController { init(viewController: UIViewController, initialState: ItemsState, delegate: ItemsToolbarControllerDelegate) { self.viewController = viewController self.delegate = delegate - self.editingActions = Self.editingActions(for: initialState) - self.disposeBag = DisposeBag() + editingActions = createEditingActions(for: initialState) + disposeBag = DisposeBag() - self.createToolbarItems(for: initialState) - } - - func willAppear() { - self.viewController.navigationController?.setToolbarHidden(false, animated: false) - } + createToolbarItems(for: initialState) - private static func editingActions(for state: ItemsState) -> [ItemAction] { - if state.collection.identifier.isTrash && state.library.metadataEditable { - return [ItemAction(type: .restore), ItemAction(type: .delete)] - } + func createEditingActions(for state: ItemsState) -> [ItemAction] { + if state.collection.identifier.isTrash && state.library.metadataEditable { + return [ItemAction(type: .restore), ItemAction(type: .delete)] + } - var actions: [ItemAction] = [] - if state.library.metadataEditable { - actions.append(contentsOf: [ItemAction(type: .addToCollection), ItemAction(type: .trash)]) - } - switch state.collection.identifier { - case .collection: + var actions: [ItemAction] = [] if state.library.metadataEditable { - actions.insert(ItemAction(type: .removeFromCollection), at: 1) + actions.append(contentsOf: [ItemAction(type: .addToCollection), ItemAction(type: .trash)]) } + switch state.collection.identifier { + case .collection: + if state.library.metadataEditable { + actions.insert(ItemAction(type: .removeFromCollection), at: 1) + } - case .custom, .search: - break + case .custom, .search: + break + } + actions.append(ItemAction(type: .share)) + return actions } - actions.append(ItemAction(type: .share)) - return actions + } + + func willAppear() { + viewController.navigationController?.setToolbarHidden(false, animated: false) } // MARK: - Actions func createToolbarItems(for state: ItemsState) { if state.isEditing { - self.viewController.toolbarItems = self.createEditingToolbarItems(from: self.editingActions) - self.updateEditingToolbarItems(for: state.selectedItems, results: state.results) + viewController.toolbarItems = createEditingToolbarItems(from: editingActions) + updateEditingToolbarItems(for: state.selectedItems, results: state.results) } else { - let filters = self.sizeClassSpecificFilters(from: state.filters) - self.viewController.toolbarItems = self.createNormalToolbarItems(for: filters) - self.updateNormalToolbarItems( + let filters = sizeClassSpecificFilters(from: state.filters) + viewController.toolbarItems = createNormalToolbarItems(for: filters) + updateNormalToolbarItems( for: filters, downloadBatchData: state.downloadBatchData, remoteDownloadBatchData: state.remoteDownloadBatchData, @@ -86,14 +86,111 @@ final class ItemsToolbarController { results: state.results ) } + + func createEditingToolbarItems(from actions: [ItemAction]) -> [UIBarButtonItem] { + let spacer = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil) + let items = actions.map({ action -> UIBarButtonItem in + let item = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) + switch action.type { + case .addToCollection, .trash, .delete, .removeFromCollection, .restore: + item.tag = ToolbarItem.empty.tag + + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .share, .removeDownload, .download, .duplicate: + break + } + switch action.type { + case .addToCollection: + item.accessibilityLabel = L10n.Accessibility.Items.addToCollection + + case .trash: + item.accessibilityLabel = L10n.Accessibility.Items.trash + + case .delete: + item.accessibilityLabel = L10n.Accessibility.Items.delete + + case .removeFromCollection: + item.accessibilityLabel = L10n.Accessibility.Items.removeFromCollection + + case .restore: + item.accessibilityLabel = L10n.Accessibility.Items.restore + + case .share: + item.accessibilityLabel = L10n.Accessibility.Items.share + + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .removeDownload, .download, .duplicate: + break + } + item.rx.tap.subscribe(onNext: { [weak self] _ in + self?.delegate?.process(action: action.type, button: item) + }) + .disposed(by: disposeBag) + return item + }) + return [spacer] + (0..<(2 * items.count)).map({ idx -> UIBarButtonItem in idx % 2 == 0 ? items[idx / 2] : spacer }) + } + + func createNormalToolbarItems(for filters: [ItemsFilter]) -> [UIBarButtonItem] { + let fixedSpacer = UIBarButtonItem(barButtonSystemItem: .fixedSpace, target: nil, action: nil) + fixedSpacer.width = 16 + let flexibleSpacer = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil) + + let filterImageName = filters.isEmpty ? "line.horizontal.3.decrease.circle" : "line.horizontal.3.decrease.circle.fill" + let filterButton = UIBarButtonItem(image: UIImage(systemName: filterImageName), style: .plain, target: nil, action: nil) + filterButton.tag = ToolbarItem.filter.tag + filterButton.accessibilityLabel = L10n.Accessibility.Items.filterItems + filterButton.rx.tap.subscribe(onNext: { [weak self] _ in + self?.delegate?.process(action: .filter, button: filterButton) + }) + .disposed(by: disposeBag) + + let action = ItemAction(type: .sort) + let sortButton = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) + sortButton.accessibilityLabel = L10n.Accessibility.Items.sortItems + sortButton.rx.tap.subscribe(onNext: { [weak self] _ in + self?.delegate?.process(action: action.type, button: sortButton) + }) + .disposed(by: disposeBag) + + let titleButton = UIBarButtonItem(customView: createTitleView()) + titleButton.tag = ToolbarItem.title.tag + + return [fixedSpacer, filterButton, flexibleSpacer, titleButton, flexibleSpacer, sortButton, fixedSpacer] + + func createTitleView() -> UIStackView { + // Filter title label + let filterLabel = UILabel() + filterLabel.adjustsFontForContentSizeCategory = true + filterLabel.textColor = .label + filterLabel.font = .preferredFont(forTextStyle: .footnote) + filterLabel.textAlignment = .center + filterLabel.isHidden = true + + // Batch download view + let progressView = ItemsToolbarDownloadProgressView() + let tap = UITapGestureRecognizer() + tap.rx + .event + .observe(on: MainScheduler.instance) + .subscribe(onNext: { [weak self] _ in + self?.delegate?.showLookup() + }) + .disposed(by: self.disposeBag) + progressView.addGestureRecognizer(tap) + progressView.isHidden = true + + let stackView = UIStackView(arrangedSubviews: [filterLabel, progressView]) + stackView.axis = .horizontal + return stackView + } + } } func reloadToolbarItems(for state: ItemsState) { if state.isEditing { - self.updateEditingToolbarItems(for: state.selectedItems, results: state.results) + updateEditingToolbarItems(for: state.selectedItems, results: state.results) } else { - self.updateNormalToolbarItems( - for: self.sizeClassSpecificFilters(from: state.filters), + updateNormalToolbarItems( + for: sizeClassSpecificFilters(from: state.filters), downloadBatchData: state.downloadBatchData, remoteDownloadBatchData: state.remoteDownloadBatchData, identifierLookupBatchData: state.identifierLookupBatchData, @@ -123,7 +220,7 @@ final class ItemsToolbarController { // MARK: - Helpers private func updateEditingToolbarItems(for selectedItems: Set, results: Results?) { - self.viewController.toolbarItems?.forEach({ item in + viewController.toolbarItems?.forEach({ item in switch ToolbarItem(rawValue: item.tag) { case .empty: item.isEnabled = !selectedItems.isEmpty @@ -144,12 +241,12 @@ final class ItemsToolbarController { identifierLookupBatchData: ItemsState.IdentifierLookupBatchData, results: Results? ) { - if let item = self.viewController.toolbarItems?.first(where: { $0.tag == ToolbarItem.filter.tag }) { + if let item = viewController.toolbarItems?.first(where: { $0.tag == ToolbarItem.filter.tag }) { let filterImageName = filters.isEmpty ? "line.horizontal.3.decrease.circle" : "line.horizontal.3.decrease.circle.fill" item.image = UIImage(systemName: filterImageName) } - if let item = self.viewController.toolbarItems?.first(where: { $0.tag == ToolbarItem.title.tag }), + if let item = viewController.toolbarItems?.first(where: { $0.tag == ToolbarItem.title.tag }), let stackView = item.customView as? UIStackView { if let filterLabel = stackView.subviews.first as? UILabel { let itemCount = results?.count ?? 0 @@ -196,98 +293,4 @@ final class ItemsToolbarController { stackView.sizeToFit() } } - - private func createNormalToolbarItems(for filters: [ItemsFilter]) -> [UIBarButtonItem] { - let fixedSpacer = UIBarButtonItem(barButtonSystemItem: .fixedSpace, target: nil, action: nil) - fixedSpacer.width = 16 - let flexibleSpacer = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil) - - let filterImageName = filters.isEmpty ? "line.horizontal.3.decrease.circle" : "line.horizontal.3.decrease.circle.fill" - let filterButton = UIBarButtonItem(image: UIImage(systemName: filterImageName), style: .plain, target: nil, action: nil) - filterButton.tag = ToolbarItem.filter.tag - filterButton.accessibilityLabel = L10n.Accessibility.Items.filterItems - filterButton.rx.tap.subscribe(onNext: { [weak self] _ in - self?.delegate?.process(action: .filter, button: filterButton) - }) - .disposed(by: self.disposeBag) - - let action = ItemAction(type: .sort) - let sortButton = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) - sortButton.accessibilityLabel = L10n.Accessibility.Items.sortItems - sortButton.rx.tap.subscribe(onNext: { [weak self] _ in - self?.delegate?.process(action: action.type, button: sortButton) - }) - .disposed(by: self.disposeBag) - - let titleButton = UIBarButtonItem(customView: self.createTitleView()) - titleButton.tag = ToolbarItem.title.tag - - return [fixedSpacer, filterButton, flexibleSpacer, titleButton, flexibleSpacer, sortButton, fixedSpacer] - } - - private func createEditingToolbarItems(from actions: [ItemAction]) -> [UIBarButtonItem] { - let spacer = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil) - let items = actions.map({ action -> UIBarButtonItem in - let item = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) - switch action.type { - case .addToCollection, .trash, .delete, .removeFromCollection, .restore: - item.tag = ToolbarItem.empty.tag - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .share, .removeDownload, .download, .duplicate: break - } - switch action.type { - case .addToCollection: - item.accessibilityLabel = L10n.Accessibility.Items.addToCollection - - case .trash: - item.accessibilityLabel = L10n.Accessibility.Items.trash - - case .delete: - item.accessibilityLabel = L10n.Accessibility.Items.delete - - case .removeFromCollection: - item.accessibilityLabel = L10n.Accessibility.Items.removeFromCollection - - case .restore: - item.accessibilityLabel = L10n.Accessibility.Items.restore - - case .share: - item.accessibilityLabel = L10n.Accessibility.Items.share - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .removeDownload, .download, .duplicate: break - } - item.rx.tap.subscribe(onNext: { [weak self] _ in - guard let self = self else { return } - self.delegate?.process(action: action.type, button: item) - }) - .disposed(by: self.disposeBag) - return item - }) - return [spacer] + (0..<(2 * items.count)).map({ idx -> UIBarButtonItem in idx % 2 == 0 ? items[idx / 2] : spacer }) - } - - private func createTitleView() -> UIStackView { - // Filter title label - let filterLabel = UILabel() - filterLabel.adjustsFontForContentSizeCategory = true - filterLabel.textColor = .label - filterLabel.font = .preferredFont(forTextStyle: .footnote) - filterLabel.textAlignment = .center - filterLabel.isHidden = true - - // Batch download view - let progressView = ItemsToolbarDownloadProgressView() - let tap = UITapGestureRecognizer() - tap.rx - .event - .observe(on: MainScheduler.instance) - .subscribe(onNext: { [weak self] _ in - self?.delegate?.showLookup() - }) - .disposed(by: self.disposeBag) - progressView.addGestureRecognizer(tap) - progressView.isHidden = true - - let stackView = UIStackView(arrangedSubviews: [filterLabel, progressView]) - stackView.axis = .horizontal - return stackView - } } From 1d533d2a8834a4672ae9f970d3779996ab0c6754 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Wed, 17 Jul 2024 13:02:40 +0300 Subject: [PATCH 4/8] Fix ItemsToolbarController enable mode for share button --- .../Detail/Items/ViewModels/ItemsToolbarController.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift index 0d05ffccf..3786e982c 100644 --- a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift +++ b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift @@ -92,10 +92,10 @@ final class ItemsToolbarController { let items = actions.map({ action -> UIBarButtonItem in let item = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) switch action.type { - case .addToCollection, .trash, .delete, .removeFromCollection, .restore: + case .addToCollection, .trash, .delete, .removeFromCollection, .restore, .share: item.tag = ToolbarItem.empty.tag - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .share, .removeDownload, .download, .duplicate: + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .removeDownload, .download, .duplicate: break } switch action.type { From 8870e7fcd8ad138f8730719f00af2ec2bf901b66 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Wed, 17 Jul 2024 13:53:24 +0300 Subject: [PATCH 5/8] Add to ItemsToolbarController remove downloads for selected items button --- Zotero/Assets/en.lproj/Localizable.strings | 1 + Zotero/Extensions/Localizable.swift | 2 + .../Detail/Items/Models/ItemAction.swift | 2 +- .../ViewModels/ItemsToolbarController.swift | 37 ++++++++++--------- 4 files changed, 24 insertions(+), 18 deletions(-) diff --git a/Zotero/Assets/en.lproj/Localizable.strings b/Zotero/Assets/en.lproj/Localizable.strings index 050a6099d..7c17db2d8 100644 --- a/Zotero/Assets/en.lproj/Localizable.strings +++ b/Zotero/Assets/en.lproj/Localizable.strings @@ -533,6 +533,7 @@ "accessibility.items.restore" = "Restore selected items"; "accessibility.items.duplicate" = "Duplicate selected item"; "accessibility.items.share" = "Share selected items"; +"accessibility.items.remove_downloads" = "Remove downloads for selected items"; "accessibility.item_detail.download_and_open" = "Double tap to download and open"; "accessibility.item_detail.open" = "Double tap to open"; "accessibility.pdf.sidebar_open" = "Open sidebar"; diff --git a/Zotero/Extensions/Localizable.swift b/Zotero/Extensions/Localizable.swift index ea7cd0999..cee653fed 100644 --- a/Zotero/Extensions/Localizable.swift +++ b/Zotero/Extensions/Localizable.swift @@ -174,6 +174,8 @@ internal enum L10n { internal static let filterItems = L10n.tr("Localizable", "accessibility.items.filter_items", fallback: "Filter items") /// Open item info internal static let openItem = L10n.tr("Localizable", "accessibility.items.open_item", fallback: "Open item info") + /// Remove downloads for selected items + internal static let removeDownloads = L10n.tr("Localizable", "accessibility.items.remove_downloads", fallback: "Remove downloads for selected items") /// Remove selected items from collection internal static let removeFromCollection = L10n.tr("Localizable", "accessibility.items.remove_from_collection", fallback: "Remove selected items from collection") /// Restore selected items diff --git a/Zotero/Scenes/Detail/Items/Models/ItemAction.swift b/Zotero/Scenes/Detail/Items/Models/ItemAction.swift index f7157c085..595dde88b 100644 --- a/Zotero/Scenes/Detail/Items/Models/ItemAction.swift +++ b/Zotero/Scenes/Detail/Items/Models/ItemAction.swift @@ -95,7 +95,7 @@ struct ItemAction { case .removeDownload: self.title = L10n.Items.Action.removeDownload - self._image = .system("trash") + self._image = .system("arrow.down.circle.dotted") } } } diff --git a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift index 3786e982c..e3861af0f 100644 --- a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift +++ b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift @@ -43,25 +43,25 @@ final class ItemsToolbarController { createToolbarItems(for: initialState) func createEditingActions(for state: ItemsState) -> [ItemAction] { + var types: [ItemAction.Kind] = [] if state.collection.identifier.isTrash && state.library.metadataEditable { - return [ItemAction(type: .restore), ItemAction(type: .delete)] - } - - var actions: [ItemAction] = [] - if state.library.metadataEditable { - actions.append(contentsOf: [ItemAction(type: .addToCollection), ItemAction(type: .trash)]) - } - switch state.collection.identifier { - case .collection: + types.append(contentsOf: [.restore, .delete, .removeDownload]) + } else { if state.library.metadataEditable { - actions.insert(ItemAction(type: .removeFromCollection), at: 1) + types.append(contentsOf: [.addToCollection, .trash]) } + switch state.collection.identifier { + case .collection: + if state.library.metadataEditable { + types.insert(.removeFromCollection, at: 1) + } - case .custom, .search: - break + case .custom, .search: + break + } + types.append(contentsOf: [.removeDownload, .share]) } - actions.append(ItemAction(type: .share)) - return actions + return types.map { .init(type: $0) } } } @@ -92,10 +92,10 @@ final class ItemsToolbarController { let items = actions.map({ action -> UIBarButtonItem in let item = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) switch action.type { - case .addToCollection, .trash, .delete, .removeFromCollection, .restore, .share: + case .addToCollection, .trash, .delete, .removeFromCollection, .restore, .share, .removeDownload: item.tag = ToolbarItem.empty.tag - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .removeDownload, .download, .duplicate: + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .download, .duplicate: break } switch action.type { @@ -117,7 +117,10 @@ final class ItemsToolbarController { case .share: item.accessibilityLabel = L10n.Accessibility.Items.share - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .removeDownload, .download, .duplicate: + case .removeDownload: + item.accessibilityLabel = L10n.Accessibility.Items.removeDownloads + + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .download, .duplicate: break } item.rx.tap.subscribe(onNext: { [weak self] _ in From 9edf93884144e956a0779c21280cac3ae509eff4 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Thu, 18 Jul 2024 11:01:49 +0300 Subject: [PATCH 6/8] Change icon for collections remove downloads menu action --- .../Views/ExpandableCollectionsCollectionViewHandler.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift b/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift index 72dbf1866..344539381 100644 --- a/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift +++ b/Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift @@ -161,7 +161,7 @@ final class ExpandableCollectionsCollectionViewHandler: NSObject { } func removeDownloadsAction(for identifier: CollectionIdentifier, in viewModel: ViewModel) -> UIAction { - UIAction(title: L10n.Collections.deleteAttachmentFiles, image: UIImage(systemName: "trash")) { [weak viewModel] _ in + UIAction(title: L10n.Collections.deleteAttachmentFiles, image: UIImage(systemName: "arrow.down.circle.dotted")) { [weak viewModel] _ in viewModel?.process(action: .removeDownloads(identifier)) } } From 2ea48be9a600dd8806bfcad175d4fd38022a7da6 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Thu, 18 Jul 2024 11:26:51 +0300 Subject: [PATCH 7/8] Add to ItemsToolbarController selected items download attachments button --- Zotero/Assets/en.lproj/Localizable.strings | 1 + Zotero/Extensions/Localizable.swift | 2 ++ .../Items/ViewModels/ItemsToolbarController.swift | 13 ++++++++----- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Zotero/Assets/en.lproj/Localizable.strings b/Zotero/Assets/en.lproj/Localizable.strings index 7c17db2d8..78db3b5d9 100644 --- a/Zotero/Assets/en.lproj/Localizable.strings +++ b/Zotero/Assets/en.lproj/Localizable.strings @@ -533,6 +533,7 @@ "accessibility.items.restore" = "Restore selected items"; "accessibility.items.duplicate" = "Duplicate selected item"; "accessibility.items.share" = "Share selected items"; +"accessibility.items.download_attachments" = "Download attachments for selected items"; "accessibility.items.remove_downloads" = "Remove downloads for selected items"; "accessibility.item_detail.download_and_open" = "Double tap to download and open"; "accessibility.item_detail.open" = "Double tap to open"; diff --git a/Zotero/Extensions/Localizable.swift b/Zotero/Extensions/Localizable.swift index cee653fed..ae19fcbad 100644 --- a/Zotero/Extensions/Localizable.swift +++ b/Zotero/Extensions/Localizable.swift @@ -168,6 +168,8 @@ internal enum L10n { internal static let delete = L10n.tr("Localizable", "accessibility.items.delete", fallback: "Delete selected items") /// Deselect All Items internal static let deselectAllItems = L10n.tr("Localizable", "accessibility.items.deselect_all_items", fallback: "Deselect All Items") + /// Download attachments for selected items + internal static let downloadAttachments = L10n.tr("Localizable", "accessibility.items.download_attachments", fallback: "Download attachments for selected items") /// Duplicate selected item internal static let duplicate = L10n.tr("Localizable", "accessibility.items.duplicate", fallback: "Duplicate selected item") /// Filter items diff --git a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift index e3861af0f..194b4d204 100644 --- a/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift +++ b/Zotero/Scenes/Detail/Items/ViewModels/ItemsToolbarController.swift @@ -45,7 +45,7 @@ final class ItemsToolbarController { func createEditingActions(for state: ItemsState) -> [ItemAction] { var types: [ItemAction.Kind] = [] if state.collection.identifier.isTrash && state.library.metadataEditable { - types.append(contentsOf: [.restore, .delete, .removeDownload]) + types.append(contentsOf: [.restore, .delete, .download, .removeDownload]) } else { if state.library.metadataEditable { types.append(contentsOf: [.addToCollection, .trash]) @@ -59,7 +59,7 @@ final class ItemsToolbarController { case .custom, .search: break } - types.append(contentsOf: [.removeDownload, .share]) + types.append(contentsOf: [.download, .removeDownload, .share]) } return types.map { .init(type: $0) } } @@ -92,10 +92,10 @@ final class ItemsToolbarController { let items = actions.map({ action -> UIBarButtonItem in let item = UIBarButtonItem(image: action.image, style: .plain, target: nil, action: nil) switch action.type { - case .addToCollection, .trash, .delete, .removeFromCollection, .restore, .share, .removeDownload: + case .addToCollection, .trash, .delete, .removeFromCollection, .restore, .share, .download, .removeDownload: item.tag = ToolbarItem.empty.tag - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .download, .duplicate: + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .duplicate: break } switch action.type { @@ -117,10 +117,13 @@ final class ItemsToolbarController { case .share: item.accessibilityLabel = L10n.Accessibility.Items.share + case .download: + item.accessibilityLabel = L10n.Accessibility.Items.downloadAttachments + case .removeDownload: item.accessibilityLabel = L10n.Accessibility.Items.removeDownloads - case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .download, .duplicate: + case .sort, .filter, .createParent, .copyCitation, .copyBibliography, .duplicate: break } item.rx.tap.subscribe(onNext: { [weak self] _ in From 419b77c368dc0bd662b5db256e0b718653c3f012 Mon Sep 17 00:00:00 2001 From: Miltiadis Vasilakis Date: Thu, 18 Jul 2024 13:07:49 +0300 Subject: [PATCH 8/8] Preserve selection when reloading attachments in ItemsTableViewHandler --- .../Scenes/Detail/Items/Views/ItemsTableViewHandler.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Zotero/Scenes/Detail/Items/Views/ItemsTableViewHandler.swift b/Zotero/Scenes/Detail/Items/Views/ItemsTableViewHandler.swift index f9726dd06..2f7277cd2 100644 --- a/Zotero/Scenes/Detail/Items/Views/ItemsTableViewHandler.swift +++ b/Zotero/Scenes/Detail/Items/Views/ItemsTableViewHandler.swift @@ -219,7 +219,11 @@ final class ItemsTableViewHandler: NSObject { } func reloadAllAttachments() { - self.tableView.reloadData() + if viewModel.state.isEditing && !viewModel.state.selectedItems.isEmpty, let indexPathsForSelectedRows = tableView.indexPathsForSelectedRows { + tableView.reconfigureRows(at: indexPathsForSelectedRows) + } else { + tableView.reloadData() + } } func reload(snapshot: Results, modifications: [Int], insertions: [Int], deletions: [Int], completion: (() -> Void)? = nil) {