Fix support for file editing (#1174)

mvasilak · mvasilak · commit a9dd4527b1fe · 2025-09-18T12:17:43.000+03:00
* Fix add actions library editing permissions checks

* In adding by identifier if library not file editable ignore attachments

* In share extension if picked library not file editable ignore attachment

* Remove convert to standalone attachment action from url attachment menu

* Use first linked URL if any for item accessory without file attachments

* Improve code
diff --git a/ZShare/View Controllers/ShareViewController.swift b/ZShare/View Controllers/ShareViewController.swift
@@ -237,7 +237,13 @@ final class ShareViewController: UIViewController {
 
         let hasItem = state.processedAttachment != nil
         self.log(attachmentState: state.attachmentState, itemState: state.itemPickerState)
-        self.update(item: state.expectedItem, attachment: state.expectedAttachment, attachmentState: state.attachmentState, defaultTitle: state.title)
+        update(
+            libraryIsFilesEditable: state.collectionPickerState.libraryIsFilesEditable,
+            item: state.expectedItem,
+            attachment: state.expectedAttachment,
+            attachmentState: state.attachmentState,
+            defaultTitle: state.title
+        )
         self.update(attachmentState: state.attachmentState, itemState: state.itemPickerState, hasItem: hasItem, isSubmitting: state.isSubmitting)
         self.update(collectionPicker: state.collectionPickerState, recents: state.recents)
         self.update(itemPicker: state.itemPickerState, hasExpectedItem: (state.expectedItem != nil || state.expectedAttachment != nil))
@@ -287,7 +293,7 @@ final class ShareViewController: UIViewController {
         self.updateBottomProgress(for: state, itemState: itemState, hasItem: hasItem, isSubmitting: isSubmitting)
     }
 
-    private func update(item: ItemResponse?, attachment: (String, File)?, attachmentState: ExtensionViewModel.State.AttachmentState, defaultTitle title: String?) {
+    private func update(libraryIsFilesEditable: Bool, item: ItemResponse?, attachment: (String, File)?, attachmentState: ExtensionViewModel.State.AttachmentState, defaultTitle title: String?) {
         self.translationContainer.isHidden = false
 
         if item == nil && attachment == nil {
@@ -306,23 +312,52 @@ final class ShareViewController: UIViewController {
             return
         }
 
-        self.itemContainer.isHidden = item == nil
-        self.attachmentContainer.isHidden = attachment == nil
+        let itemFound: Bool
+        if let item {
+            itemFound = true
+            // Item was found, show its metadata.
+            let itemTitle = itemTitle(for: item, schemaController: schemaController, defaultValue: title ?? "")
+            setItem(title: itemTitle, type: item.rawType)
+            itemContainer.isHidden = false
+        } else {
+            itemFound = false
+            itemContainer.isHidden = true
+        }
+        if libraryIsFilesEditable, let (attachmentTitle, file) = attachment {
+            // The picked library allows file editing, and either item with attachment, or only attachment (local/remote file), was found, show its metadata.
+            setAttachment(title: attachmentTitle, file: file, state: attachmentState)
+            attachmentContainerLeft.constant = itemFound ? Self.childAttachmentLeftOffset : 0
+            attachmentContainer.isHidden = false
+        } else {
+            attachmentContainer.isHidden = true
+        }
+
+        func setAttachment(title: String, file: File, state: ExtensionViewModel.State.AttachmentState) {
+            attachmentTitleLabel.text = title
+            let type: Attachment.Kind = .file(filename: "", contentType: file.mimeType, location: .local, linkType: .importedFile, compressed: false)
+            let iconState = FileAttachmentView.State.stateFrom(type: type, progress: nil, error: state.error)
+            attachmentIcon.set(state: iconState, style: .shareExtension)
 
-        if let item = item, let (attachmentTitle, file) = attachment {
-            // Item with attachment was found, show their metadata
-            let itemTitle = self.itemTitle(for: item, schemaController: self.schemaController, defaultValue: title ?? "")
-            self.setItem(title: itemTitle, type: item.rawType)
-            self.attachmentContainerLeft.constant = ShareViewController.childAttachmentLeftOffset
-            self.setAttachment(title: attachmentTitle, file: file, state: attachmentState)
-        } else if let item = item {
-            // Only item was found, show metadata
-            let title = self.itemTitle(for: item, schemaController: self.schemaController, defaultValue: title ?? "")
-            self.setItem(title: title, type: item.rawType)
-        } else if let (title, file) = attachment {
-            // Only attachment (local/remote file) was found, show metadata
-            self.attachmentContainerLeft.constant = 0
-            self.setAttachment(title: title, file: file, state: attachmentState)
+            switch state {
+            case .downloading(let progress):
+                attachmentProgressView.isHidden = progress == 0
+                attachmentActivityIndicator.isHidden = progress > 0
+                attachmentProgressView.progress = CGFloat(progress)
+                if progress == 0 && !attachmentActivityIndicator.isAnimating {
+                    attachmentActivityIndicator.startAnimating()
+                }
+                attachmentIcon.alpha = 0.5
+                attachmentTitleLabel.alpha = 0.5
+
+            default:
+                attachmentProgressView.isHidden = true
+                if attachmentActivityIndicator.isAnimating {
+                    attachmentActivityIndicator.stopAnimating()
+                }
+                attachmentActivityIndicator.isHidden = true
+                attachmentIcon.alpha = 1
+                attachmentTitleLabel.alpha = 1
+            }
         }
     }
 
@@ -335,37 +370,6 @@ final class ShareViewController: UIViewController {
         self.itemIcon.image = UIImage(named: ItemTypes.iconName(for: type))
     }
 
-    private func setAttachment(title: String, file: File, state: ExtensionViewModel.State.AttachmentState) {
-        self.attachmentTitleLabel.text = title
-        let type: Attachment.Kind = .file(filename: "", contentType: file.mimeType, location: .local, linkType: .importedFile, compressed: false)
-        let iconState = FileAttachmentView.State.stateFrom(type: type, progress: nil, error: state.error)
-        self.attachmentIcon.set(state: iconState, style: .shareExtension)
-
-        switch state {
-        case .downloading(let progress):
-            self.attachmentProgressView.isHidden = progress == 0
-            self.attachmentActivityIndicator.isHidden = progress > 0
-            self.attachmentProgressView.progress = CGFloat(progress)
-            if progress == 0 && !self.attachmentActivityIndicator.isAnimating {
-                self.attachmentActivityIndicator.startAnimating()
-            }
-
-            self.attachmentIcon.alpha = 0.5
-            self.attachmentTitleLabel.alpha = 0.5
-
-        default:
-            if !self.attachmentContainer.isHidden {
-                self.attachmentProgressView.isHidden = true
-                if self.attachmentActivityIndicator.isAnimating {
-                    self.attachmentActivityIndicator.stopAnimating()
-                }
-                self.attachmentActivityIndicator.isHidden = true
-                self.attachmentIcon.alpha = 1
-                self.attachmentTitleLabel.alpha = 1
-            }
-        }
-    }
-
     private func updateNavigationItems(for state: ExtensionViewModel.State.AttachmentState, isSubmitting: Bool) {
         if let error = state.error {
             switch error {
diff --git a/ZShare/ViewModels/ExtensionViewModel.swift b/ZShare/ViewModels/ExtensionViewModel.swift
@@ -37,6 +37,16 @@ final class ExtensionViewModel {
         enum CollectionPickerState {
             case loading, failed
             case picked(Library, Collection?)
+
+            var libraryIsFilesEditable: Bool {
+                switch self {
+                case .loading, .failed:
+                    return false
+
+                case let .picked(library, _):
+                    return library.filesEditable
+                }
+            }
         }
 
         struct ItemPickerState {
@@ -1049,7 +1059,12 @@ final class ExtensionViewModel {
 
             case .itemWithAttachment(let item, let attachmentData, let attachmentFile):
                 let newTags = (Defaults.shared.shareExtensionIncludeTags ? item.tags + tags : tags)
-                attachment = .itemWithAttachment(item: item.copy(libraryId: libraryId, collectionKeys: collectionKeys, tags: newTags), attachment: attachmentData, attachmentFile: attachmentFile)
+                if state.collectionPickerState.libraryIsFilesEditable {
+                    attachment = .itemWithAttachment(item: item.copy(libraryId: libraryId, collectionKeys: collectionKeys, tags: newTags), attachment: attachmentData, attachmentFile: attachmentFile)
+                } else {
+                    // Picked library doesn't allow file editing, convert attachment to item only.
+                    attachment = .item(item.copy(libraryId: libraryId, collectionKeys: collectionKeys, tags: newTags))
+                }
 
             case .file: break
             }
diff --git a/Zotero/Controllers/AttachmentCreator.swift b/Zotero/Controllers/AttachmentCreator.swift
@@ -22,10 +22,9 @@ struct AttachmentCreator {
 
     private static let mainAttachmentContentTypes: Set<String> = ["text/html", "application/pdf", "image/png", "image/jpeg", "image/gif", "text/plain", "application/epub+zip"]
 
-    static func mainAttachment(for item: RItem, fileStorage: FileStorage) -> Attachment? {
+    static func mainAttachment(for item: RItem, fileStorage: FileStorage, urlDetector: UrlDetector?) -> Attachment? {
         if item.rawType == ItemTypes.attachment {
-            // If item is attachment, create `Attachment` and ignore linked attachments.
-            if let attachment = attachment(for: item, fileStorage: fileStorage, urlDetector: nil) {
+            if let attachment = attachment(for: item, fileStorage: fileStorage, urlDetector: urlDetector) {
                 switch attachment.type {
                 case .url:
                     return attachment
@@ -40,20 +39,54 @@ struct AttachmentCreator {
             return nil
         }
 
-        var attachmentData = attachmentData(for: item)
-
-        guard !attachmentData.isEmpty else { return nil }
-
-        attachmentData.sort { lData, rData in
+        if let fileAttachment = fileAttachmentData(for: item).sorted(by: { lData, rData in
             mainAttachmentsAreInIncreasingOrder(lData: (lData.1, lData.3, lData.4), rData: (rData.1, rData.3, rData.4))
+        }).first.flatMap({ fileAttachmentDataToAttachment($0) }) {
+            return fileAttachment
+        }
+        return firstLinkedURLAttachmentIfAny(for: item, urlDetector: urlDetector)
+
+        func fileAttachmentData(for item: RItem) -> [(Int, String, LinkMode, Bool, Date)] {
+            let itemUrl = item.fields.first(where: { $0.key == FieldKeys.Item.url })?.value
+            var data: [(Int, String, LinkMode, Bool, Date)] = []
+            for (idx, child) in item.children.enumerated() {
+                guard (child.rawType == ItemTypes.attachment) && (child.syncState != .dirty) && !child.trash,
+                      let linkMode = child.fields.first(where: { $0.key == FieldKeys.Item.Attachment.linkMode }).flatMap({ LinkMode(rawValue: $0.value) }),
+                      (linkMode == .importedUrl) || (linkMode == .importedFile),
+                      let contentType = contentType(for: child),
+                      AttachmentCreator.mainAttachmentContentTypes.contains(contentType) else { continue }
+
+                var hasMatchingUrlWithParent = false
+                if let url = itemUrl, let childUrl = child.fields.first(where: { $0.key == FieldKeys.Item.Attachment.url })?.value {
+                    hasMatchingUrlWithParent = url == childUrl
+                }
+                data.append((idx, contentType, linkMode, hasMatchingUrlWithParent, child.dateAdded))
+            }
+            return data
         }
 
-        guard let (idx, contentType, linkMode, _, _) = attachmentData.first else { return nil }
-        let rAttachment = item.children[idx]
-        let linkType: Attachment.FileLinkType = linkMode == .importedFile ? .importedFile : .importedUrl
-        guard let libraryId = rAttachment.libraryId else { return nil }
-        let type = importedType(for: rAttachment, contentType: contentType, libraryId: libraryId, fileStorage: fileStorage, linkType: linkType, compressed: rAttachment.fileCompressed)
-        return Attachment(item: rAttachment, type: type)
+        func fileAttachmentDataToAttachment(_ data: (Int, String, LinkMode, Bool, Date)) -> Attachment? {
+            let (idx, contentType, linkMode, _, _) = data
+            let rAttachment = item.children[idx]
+            let linkType: Attachment.FileLinkType = linkMode == .importedFile ? .importedFile : .importedUrl
+            guard let libraryId = rAttachment.libraryId else { return nil }
+            let type = importedType(for: rAttachment, contentType: contentType, libraryId: libraryId, fileStorage: fileStorage, linkType: linkType, compressed: rAttachment.fileCompressed)
+            return Attachment(item: rAttachment, type: type)
+        }
+
+        func firstLinkedURLAttachmentIfAny(for item: RItem, urlDetector: UrlDetector?) -> Attachment? {
+            guard let urlDetector else { return nil }
+            for child in item.children {
+                guard (child.rawType == ItemTypes.attachment) && (child.syncState != .dirty) && !child.trash,
+                      let linkMode = child.fields.first(where: { $0.key == FieldKeys.Item.Attachment.linkMode }).flatMap({ LinkMode(rawValue: $0.value) }),
+                      linkMode == .linkedUrl,
+                      let libraryId = child.libraryId,
+                      let attachmentType = linkedUrlType(for: child, libraryId: libraryId, urlDetector: urlDetector)
+                else { continue }
+                return Attachment(item: child, type: attachmentType)
+            }
+            return nil
+        }
     }
 
     static func mainPdfAttachment(from attachments: [Attachment], parentUrl: String?) -> Attachment? {
diff --git a/Zotero/Controllers/IdentifierLookupController.swift b/Zotero/Controllers/IdentifierLookupController.swift
@@ -455,16 +455,19 @@ final class IdentifierLookupController {
                     }
                     
                     func storeDataAndDownloadAttachmentIfNecessary(identifier: String, response: ItemResponse, attachments: [(Attachment, URL)]) throws {
-                        let request = CreateTranslatedItemsDbRequest(responses: [response], schemaController: schemaController, dateParser: dateParser)
-                        _ = try dbStorage.perform(request: request, on: backgroundQueue)
+                        var library: Library?
+                        try dbStorage.perform(on: backgroundQueue) { coordinator in
+                            _ = try coordinator.perform(request: CreateTranslatedItemsDbRequest(responses: [response], schemaController: schemaController, dateParser: dateParser))
+                            library = try coordinator.perform(request: ReadLibraryDbRequest(libraryId: libraryId))
+                        }
                         changeLookup(
                             for: identifier,
                             to: .translated(.init(response: response, attachments: attachments, libraryId: libraryId, collectionKeys: collectionKeys))
                         ) { [weak self] didChange in
                             guard let self, didChange else { return }
                             observable.on(.next(Update(kind: .itemStored(identifier: identifier, response: response, attachments: attachments), lookupData: Array(lookupData.values))))
                             
-                            if Defaults.shared.shareExtensionIncludeAttachment, !attachments.isEmpty {
+                            if Defaults.shared.shareExtensionIncludeAttachment, library?.filesEditable == true, !attachments.isEmpty {
                                 let downloadData = attachments.map({ ($0, $1, response.key) })
                                 remoteFileDownloader.download(data: downloadData)
                                 observable.on(.next(Update(kind: .pendingAttachments(identifier: identifier, response: response, attachments: attachments), lookupData: Array(lookupData.values))))
diff --git a/Zotero/Scenes/Detail/DetailCoordinator.swift b/Zotero/Scenes/Detail/DetailCoordinator.swift
@@ -441,15 +441,13 @@ extension DetailCoordinator: DetailItemsCoordinatorDelegate {
         let controller = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet)
         controller.popoverPresentationController?.sourceItem = button
 
-        if viewModel.state.library.metadataAndFilesEditable {
-            controller.addAction(UIAlertAction(title: L10n.Items.lookup, style: .default, handler: { [weak self] _ in
-                self?.showLookup(startWith: .manual(restoreLookupState: false))
-            }))
+        controller.addAction(UIAlertAction(title: L10n.Items.lookup, style: .default, handler: { [weak self] _ in
+            self?.showLookup(startWith: .manual(restoreLookupState: false))
+        }))
 
-            controller.addAction(UIAlertAction(title: L10n.Items.barcode, style: .default, handler: { [weak self] _ in
-                self?.showLookup(startWith: .scanner)
-            }))
-        }
+        controller.addAction(UIAlertAction(title: L10n.Items.barcode, style: .default, handler: { [weak self] _ in
+            self?.showLookup(startWith: .scanner)
+        }))
 
         controller.addAction(UIAlertAction(title: L10n.Items.new, style: .default, handler: { [weak self, weak viewModel] _ in
             guard let self, let viewModel else { return }
@@ -471,7 +469,7 @@ extension DetailCoordinator: DetailItemsCoordinatorDelegate {
             showNote(library: viewModel.state.library, kind: .standaloneCreation(collection: viewModel.state.collection), saveCallback: nil)
         }))
 
-        if viewModel.state.library.metadataAndFilesEditable {
+        if viewModel.state.library.filesEditable {
             controller.addAction(UIAlertAction(title: L10n.Items.newFile, style: .default, handler: { [weak self, weak viewModel] _ in
                 self?.showAttachmentPicker(save: { urls in
                     viewModel?.process(action: .addAttachments(urls))
diff --git a/Zotero/Scenes/Detail/ItemDetail/Views/ItemDetailCollectionViewHandler.swift b/Zotero/Scenes/Detail/ItemDetail/Views/ItemDetailCollectionViewHandler.swift
@@ -995,9 +995,11 @@ extension ItemDetailCollectionViewHandler: UICollectionViewDelegate {
             }
 
             if !viewModel.state.data.isAttachment {
-                actions.append(UIAction(title: L10n.ItemDetail.moveToStandaloneAttachment, image: UIImage(systemName: "arrow.up.to.line"), attributes: []) { [weak self] _ in
-                    self?.viewModel.process(action: .moveAttachmentToStandalone(attachment))
-                })
+                if case .file = attachment.type {
+                    actions.append(UIAction(title: L10n.ItemDetail.moveToStandaloneAttachment, image: UIImage(systemName: "arrow.up.to.line"), attributes: []) { [weak self] _ in
+                        self?.viewModel.process(action: .moveAttachmentToStandalone(attachment))
+                    })
+                }
 
                 actions.append(UIAction(title: L10n.moveToTrash, image: UIImage(systemName: "trash"), attributes: .destructive) { [weak self] _ in
                     self?.viewModel.process(action: .deleteAttachment(attachment))
diff --git a/Zotero/Scenes/General/Models/ItemAccessory.swift b/Zotero/Scenes/General/Models/ItemAccessory.swift
@@ -39,8 +39,14 @@ extension ItemAccessory {
     }
 
     static func create(from item: RItem, fileStorage: FileStorage, urlDetector: UrlDetector) -> ItemAccessory? {
-        if let attachment = AttachmentCreator.mainAttachment(for: item, fileStorage: fileStorage) {
-            return .attachment(attachment: attachment, parentKey: (item.key != attachment.key) ? item.key : nil)
+        if let attachment = AttachmentCreator.mainAttachment(for: item, fileStorage: fileStorage, urlDetector: urlDetector) {
+            switch attachment.type {
+            case .file:
+                return .attachment(attachment: attachment, parentKey: (item.key != attachment.key) ? item.key : nil)
+
+            case .url(let url):
+                return .url(url)
+            }
         }
 
         if let urlString = item.urlString, urlDetector.isUrl(string: urlString), let url = URL(string: urlString) {
diff --git a/Zotero/Scenes/General/Views/NoteEditorViewController.swift b/Zotero/Scenes/General/Views/NoteEditorViewController.swift
