Improve PDF reader selection menu

Zotero/Controllers/Architecture/Coordinator.swift

@@ -20,7 +20,13 @@ protocol Coordinator: AnyObject {
 
     func start(animated: Bool)
     func childDidFinish(_ child: Coordinator)
-    func share(item: Any, sourceView: SourceView, presenter: UIViewController?, completionWithItemsHandler: UIActivityViewController.CompletionWithItemsHandler?)
+    func share(
+        item: Any,
+        sourceView: SourceView,
+        presenter: UIViewController?,
+        userInterfaceStyle: UIUserInterfaceStyle?,
+        completionWithItemsHandler: UIActivityViewController.CompletionWithItemsHandler?
+    )
 }
 
 extension Coordinator {
@@ -36,8 +42,17 @@ extension Coordinator {
         }
     }
 
-    func share(item: Any, sourceView: SourceView, presenter: UIViewController? = nil, completionWithItemsHandler: UIActivityViewController.CompletionWithItemsHandler? = nil) {
+    func share(
+        item: Any,
+        sourceView: SourceView,
+        presenter: UIViewController? = nil,
+        userInterfaceStyle: UIUserInterfaceStyle? = nil,
+        completionWithItemsHandler: UIActivityViewController.CompletionWithItemsHandler? = nil
+    ) {
         let controller = UIActivityViewController(activityItems: [item], applicationActivities: nil)
+        if let userInterfaceStyle {
+            controller.overrideUserInterfaceStyle = userInterfaceStyle
+        }
         controller.modalPresentationStyle = .pageSheet
         controller.completionWithItemsHandler = completionWithItemsHandler
 

Zotero/Scenes/Detail/PDF/Models/PDFReaderAction.swift

@@ -44,6 +44,7 @@ enum PDFReaderAction {
     case createNote(pageIndex: PageIndex, origin: CGPoint)
     case createImage(pageIndex: PageIndex, origin: CGPoint)
     case createHighlight(pageIndex: PageIndex, rects: [CGRect])
+    case createUnderline(pageIndex: PageIndex, rects: [CGRect])
     case parseAndCacheText(key: String, text: String, font: UIFont)
     case parseAndCacheComment(key: String, comment: String)
     case setComment(key: String, comment: NSAttributedString)

Zotero/Scenes/Detail/PDF/PDFCoordinator.swift

@@ -26,7 +26,7 @@ protocol PdfReaderCoordinatorDelegate: AnyObject {
     func show(error: PDFReaderState.Error)
     func show(error: PDFDocumentExporter.Error)
     func share(url: URL, barButton: UIBarButtonItem)
-    func share(text: String, rect: CGRect, view: UIView)
+    func share(text: String, rect: CGRect, view: UIView, userInterfaceStyle: UIUserInterfaceStyle)
     func lookup(text: String, rect: CGRect, view: UIView, userInterfaceStyle: UIUserInterfaceStyle)
     func showDeletedAlertForPdf(completion: @escaping (Bool) -> Void)
     func showSettings(with settings: PDFSettings, sender: UIBarButtonItem) -> ViewModel<ReaderSettingsActionHandler>
@@ -289,13 +289,15 @@ extension PDFCoordinator: PdfReaderCoordinatorDelegate {
         self.share(item: url, sourceView: .item(barButton))
     }
 
-    func share(text: String, rect: CGRect, view: UIView) {
-        self.share(item: text, sourceView: .view(view, rect))
+    func share(text: String, rect: CGRect, view: UIView, userInterfaceStyle: UIUserInterfaceStyle) {
+        self.share(item: text, sourceView: .view(view, rect), userInterfaceStyle: userInterfaceStyle)
     }
 
     func lookup(text: String, rect: CGRect, view: UIView, userInterfaceStyle: UIUserInterfaceStyle) {
         DDLogInfo("PDFCoordinator: show lookup")
-        let controller = UIReferenceLibraryViewController(term: text)
+        // When presented as a popover, UIReferenceLibraryViewController ignores overrideUserInterfaceStyle, so we wrap it in a navigation controller to force it.
+        let controller = UINavigationController(rootViewController: UIReferenceLibraryViewController(term: text))
+        controller.setNavigationBarHidden(true, animated: false)
         controller.overrideUserInterfaceStyle = userInterfaceStyle
         controller.modalPresentationStyle = .popover
         controller.popoverPresentationController?.sourceView = view

Zotero/Scenes/Detail/PDF/ViewModels/PDFReaderActionHandler.swift

@@ -226,7 +226,10 @@ final class PDFReaderActionHandler: ViewModelActionHandler, BackgroundDbProcessi
             addNote(onPage: pageIndex, origin: origin, in: viewModel)
 
         case .createHighlight(let pageIndex, let rects):
-            addHighlight(onPage: pageIndex, rects: rects, in: viewModel)
+            addHighlightOrUnderline(isHighlight: true, onPage: pageIndex, rects: rects, in: viewModel)
+
+        case .createUnderline(let pageIndex, let rects):
+            addHighlightOrUnderline(isHighlight: false, onPage: pageIndex, rects: rects, in: viewModel)
 
         case .setVisiblePage(let page, let userActionFromDocument, let fromThumbnailList):
             set(page: page, userActionFromDocument: userActionFromDocument, fromThumbnailList: fromThumbnailList, in: viewModel)
@@ -1182,22 +1185,22 @@ final class PDFReaderActionHandler: ViewModelActionHandler, BackgroundDbProcessi
         }
     }
 
-    private func addHighlight(onPage pageIndex: PageIndex, rects: [CGRect], in viewModel: ViewModel<PDFReaderActionHandler>) {
-        guard let activeColor = viewModel.state.toolColors[tool(from: .highlight)] else { return }
+    private func addHighlightOrUnderline(isHighlight: Bool, onPage pageIndex: PageIndex, rects: [CGRect], in viewModel: ViewModel<PDFReaderActionHandler>) {
+        guard let activeColor = viewModel.state.toolColors[tool(from: isHighlight ? .highlight : .underline)] else { return }
         let (color, alpha, blendMode) = AnnotationColorGenerator.color(from: activeColor, isHighlight: true, userInterfaceStyle: viewModel.state.interfaceStyle)
 
-        let highlight = HighlightAnnotation()
-        highlight.rects = rects
-        highlight.boundingBox = AnnotationBoundingBoxCalculator.boundingBox(from: rects)
-        highlight.alpha = alpha
-        highlight.color = color
+        let annotation = isHighlight ? HighlightAnnotation() : UnderlineAnnotation()
+        annotation.rects = rects
+        annotation.boundingBox = AnnotationBoundingBoxCalculator.boundingBox(from: rects)
+        annotation.alpha = alpha
+        annotation.color = color
         if let blendMode {
-            highlight.blendMode = blendMode
+            annotation.blendMode = blendMode
         }
-        highlight.pageIndex = pageIndex
+        annotation.pageIndex = pageIndex
 
-        viewModel.state.document.undoController.recordCommand(named: nil, adding: [highlight]) {
-            viewModel.state.document.add(annotations: [highlight], options: nil)
+        viewModel.state.document.undoController.recordCommand(named: nil, adding: [annotation]) {
+            viewModel.state.document.add(annotations: [annotation], options: nil)
         }
     }
 

Zotero/Scenes/Detail/PDF/Views/PDFDocumentViewController.swift

@@ -688,9 +688,9 @@ extension PDFDocumentViewController: PDFViewControllerDelegate {
     }
 
     func pdfViewController(_ sender: PDFViewController, menuForText glyphs: GlyphSequence, onPageView pageView: PDFPageView, appearance: EditMenuAppearance, suggestedMenu: UIMenu) -> UIMenu {
-        return self.filterActions(
+        return filterActions(
             forMenu: suggestedMenu,
-            predicate: { menuId, action -> UIAction? in
+            predicate: { menuId, action -> UIMenuElement? in
                 switch menuId {
                 case .standardEdit:
                     switch action.identifier {
@@ -709,40 +709,44 @@ extension PDFDocumentViewController: PDFViewControllerDelegate {
                 case .share:
                     guard action.identifier == .PSPDFKit.share else { return nil }
                     return action.replacing(handler: { [weak self] _ in
-                        guard let view = self?.pdfController?.view else { return }
-                        self?.coordinatorDelegate?.share(text: glyphs.text, rect: glyphs.boundingBox, view: view)
+                        guard let self else { return }
+                        coordinatorDelegate?.share(
+                            text: glyphs.text,
+                            rect: pageView.convert(glyphs.boundingBox, from: pageView.pdfCoordinateSpace),
+                            view: pageView,
+                            userInterfaceStyle: viewModel.state.settings.appearanceMode.userInterfaceStyle
+                        )
                     })
 
                 case .pspdfkitActions:
                     switch action.identifier {
                     case .PSPDFKit.define:
                         return action.replacing(title: L10n.lookUp, handler: { [weak self] _ in
-                            guard let self, let view = self.pdfController?.view else { return }
-                            self.coordinatorDelegate?.lookup(
+                            guard let self else { return }
+                            coordinatorDelegate?.lookup(
                                 text: glyphs.text,
-                                rect: glyphs.boundingBox,
-                                view: view,
-                                userInterfaceStyle: self.viewModel.state.settings.appearanceMode.userInterfaceStyle
+                                rect: pageView.convert(glyphs.boundingBox, from: pageView.pdfCoordinateSpace),
+                                view: pageView,
+                                userInterfaceStyle: viewModel.state.settings.appearanceMode.userInterfaceStyle
                             )
                         })
 
                     case .PSPDFKit.searchDocument:
                         return action.replacing(handler: { [weak self] _ in
-                            guard let self, let pdfController = self.pdfController else { return }
-                            self.parentDelegate?.showSearch(pdfController: pdfController, text: glyphs.text)
+                            guard let self, let pdfController else { return }
+                            parentDelegate?.showSearch(pdfController: pdfController, text: glyphs.text)
                         })
 
                     default:
                         return action
                     }
 
                 case .PSPDFKit.annotate:
-                    let rects = pageView.selectionView.selectionRects.map({ pageView.convert($0.cgRectValue, to: pageView.pdfCoordinateSpace) })
-                    return action.replacing(title: L10n.Pdf.highlight, handler: { [weak self] _ in
-                        guard let self else { return }
-                        self.viewModel.process(action: .createHighlight(pageIndex: pageView.pageIndex, rects: rects))
-                        pageView.selectionView.selectedGlyphs = nil
-                    })
+                    let actions = [
+                        action.replacing(title: L10n.Pdf.highlight, handler: createHighlightActionHandler(for: pageView, in: viewModel)),
+                        UIAction(title: L10n.Pdf.underline, identifier: .underline, handler: createUnderlineActionHandler(for: pageView, in: viewModel))
+                    ]
+                    return UIMenu(options: [.displayInline], children: actions)
 
                 default:
                     return action
@@ -751,41 +755,55 @@ extension PDFDocumentViewController: PDFViewControllerDelegate {
             populatingEmptyMenu: { menu -> [UIAction]? in
                 switch menu.identifier {
                 case .PSPDFKit.annotate:
-                    let rects = pageView.selectionView.selectionRects.map({ pageView.convert($0.cgRectValue, to: pageView.pdfCoordinateSpace) })
                     return [
-                        UIAction(title: L10n.Pdf.highlight, handler: { [weak self] _ in
-                            guard let self else { return }
-                            self.viewModel.process(action: .createHighlight(pageIndex: pageView.pageIndex, rects: rects))
-                            pageView.selectionView.selectedGlyphs = nil
-                        })
+                        UIAction(title: L10n.Pdf.highlight, handler: createHighlightActionHandler(for: pageView, in: viewModel)),
+                        UIAction(title: L10n.Pdf.underline, identifier: .underline, handler: createUnderlineActionHandler(for: pageView, in: viewModel))
                     ]
 
                 default:
                     return nil
                 }
             }
         )
-    }
 
-    private func filterActions(forMenu menu: UIMenu, predicate: (UIMenu.Identifier, UIAction) -> UIAction?, populatingEmptyMenu: (UIMenu) -> [UIAction]?) -> UIMenu {
-        return menu.replacingChildren(menu.children.compactMap { element in
-            if let action = element as? UIAction {
-                if let action = predicate(menu.identifier, action) {
-                    return action
-                } else {
-                    return nil
-                }
-            } else if let menu = element as? UIMenu {
-                if menu.children.isEmpty {
-                    return populatingEmptyMenu(menu).flatMap({ menu.replacingChildren($0) }) ?? menu
+        func filterActions(forMenu menu: UIMenu, predicate: (UIMenu.Identifier, UIAction) -> UIMenuElement?, populatingEmptyMenu: (UIMenu) -> [UIAction]?) -> UIMenu {
+            return menu.replacingChildren(menu.children.compactMap { element -> UIMenuElement? in
+                if let action = element as? UIAction {
+                    if let element = predicate(menu.identifier, action) {
+                        return element
+                    } else {
+                        return nil
+                    }
+                } else if let menu = element as? UIMenu {
+                    if menu.children.isEmpty {
+                        return populatingEmptyMenu(menu).flatMap({ menu.replacingChildren($0) }) ?? menu
+                    } else {
+                        // Filter children of submenus recursively.
+                        return filterActions(forMenu: menu, predicate: predicate, populatingEmptyMenu: populatingEmptyMenu)
+                    }
                 } else {
-                    // Filter children of submenus recursively.
-                    return self.filterActions(forMenu: menu, predicate: predicate, populatingEmptyMenu: populatingEmptyMenu)
+                    return element
                 }
-            } else {
-                return element
+            })
+        }
+
+        func createHighlightActionHandler(for pageView: PDFPageView, in viewModel: ViewModel<PDFReaderActionHandler>) -> UIActionHandler {
+            let rects = pageView.selectionView.selectionRects.map({ pageView.convert($0.cgRectValue, to: pageView.pdfCoordinateSpace) })
+            return { [weak viewModel] _ in
+                guard let viewModel else { return }
+                viewModel.process(action: .createHighlight(pageIndex: pageView.pageIndex, rects: rects))
+                pageView.selectionView.selectedGlyphs = nil
             }
-        })
+        }
+
+        func createUnderlineActionHandler(for pageView: PDFPageView, in viewModel: ViewModel<PDFReaderActionHandler>) -> UIActionHandler {
+            let rects = pageView.selectionView.selectionRects.map({ pageView.convert($0.cgRectValue, to: pageView.pdfCoordinateSpace) })
+            return { [weak viewModel] _ in
+                guard let viewModel else { return }
+                viewModel.process(action: .createUnderline(pageIndex: pageView.pageIndex, rects: rects))
+                pageView.selectionView.selectedGlyphs = nil
+            }
+        }
     }
 
     func pdfViewController(_ pdfController: PDFViewController, didSelectText text: String, with glyphs: [Glyph], at rect: CGRect, on pageView: PDFPageView) {
@@ -1038,3 +1056,7 @@ extension UIAction {
         )
     }
 }
+
+extension UIAction.Identifier {
+    fileprivate static let underline = UIAction.Identifier(rawValue: "org.zotero.menu")
+}