Update Bookmark this page to Delete bookmark when url is already book…

…marked (#2774)

Task/Issue URL: https://app.asana.com/0/1177771139624306/1203352129640292/f

**Description**:
This PR changes the behaviour by showing “Delete bookmark” if a URL is bookmarked on the Tab contextual menu.

DuckDuckGo/Bookmarks/View/AddBookmarkPopoverView.swift

@@ -48,7 +48,7 @@ struct AddBookmarkPopoverView: View {
             selectedFolder: $model.selectedFolder,
             isURLFieldHidden: true,
             addFolderAction: model.addFolderButtonAction,
-            otherActionTitle: UserText.remove,
+            otherActionTitle: UserText.delete,
             isOtherActionDisabled: false,
             otherAction: model.removeButtonAction,
             defaultActionTitle: UserText.done,

DuckDuckGo/Common/Localizables/UserText.swift

@@ -29,6 +29,7 @@ struct UserText {
     static let cancel = NSLocalizedString("cancel", value: "Cancel", comment: "Cancel button")
     static let notNow = NSLocalizedString("notnow", value: "Not Now", comment: "Not Now button")
     static let remove = NSLocalizedString("generic.remove.button", value: "Remove", comment: "Label of a button that allows the user to remove an item")
+    static let delete = NSLocalizedString("generic.delete.button", value: "Delete", comment: "Label of a button that allows the user to delete an item")
     static let discard = NSLocalizedString("generic.discard.button", value: "Discard", comment: "Label of a button that allows the user discard an action/change")
     static let neverForThisSite = NSLocalizedString("never.for.this.site", value: "Never Ask for This Site", comment: "Never ask to save login credentials for this site button")
     static let open = NSLocalizedString("open", value: "Open", comment: "Open button")

DuckDuckGo/Localizable.xcstrings

@@ -20715,6 +20715,66 @@
         }
       }
     },
+    "generic.delete.button" : {
+      "comment" : "Label of a button that allows the user to delete an item",
+      "extractionState" : "extracted_with_value",
+      "localizations" : {
+        "de" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Löschen"
+          }
+        },
+        "en" : {
+          "stringUnit" : {
+            "state" : "new",
+            "value" : "Delete"
+          }
+        },
+        "es" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Eliminar"
+          }
+        },
+        "fr" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Supprimer"
+          }
+        },
+        "it" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Elimina"
+          }
+        },
+        "nl" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Verwijderen"
+          }
+        },
+        "pl" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Usuń"
+          }
+        },
+        "pt" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Eliminar"
+          }
+        },
+        "ru" : {
+          "stringUnit" : {
+            "state" : "translated",
+            "value" : "Удалить"
+          }
+        }
+      }
+    },
     "generic.discard.button" : {
       "comment" : "Label of a button that allows the user discard an action/change",
       "extractionState" : "extracted_with_value",

DuckDuckGo/PinnedTabs/Model/PinnedTabsViewModel.swift

@@ -83,10 +83,15 @@ final class PinnedTabsViewModel: ObservableObject {
 
     // MARK: -
 
-    init(collection: TabCollection, fireproofDomains: FireproofDomains = .shared) {
+    init(
+        collection: TabCollection,
+        fireproofDomains: FireproofDomains = .shared,
+        bookmarkManager: BookmarkManager = LocalBookmarkManager.shared
+    ) {
         tabsDidReorderPublisher = tabsDidReorderSubject.eraseToAnyPublisher()
         contextMenuActionPublisher = contextMenuActionSubject.eraseToAnyPublisher()
         self.fireproofDomains = fireproofDomains
+        self.bookmarkManager = bookmarkManager
         tabsCancellable = collection.$tabs.assign(to: \.items, onWeaklyHeld: self)
 
         dragMovesWindowCancellable = $items
@@ -100,6 +105,7 @@ final class PinnedTabsViewModel: ObservableObject {
     private var tabsCancellable: AnyCancellable?
     private var dragMovesWindowCancellable: AnyCancellable?
     private var fireproofDomains: FireproofDomains
+    private var bookmarkManager: BookmarkManager
 
     private func updateItemsWithoutSeparator() {
         var items = Set<Tab>()
@@ -136,6 +142,7 @@ extension PinnedTabsViewModel {
         case unpin(Int)
         case duplicate(Int)
         case bookmark(Tab)
+        case removeBookmark(Tab)
         case fireproof(Tab)
         case removeFireproofing(Tab)
         case close(Int)
@@ -179,10 +186,19 @@ extension PinnedTabsViewModel {
         contextMenuActionSubject.send(.close(index))
     }
 
+    func isPinnedTabBookmarked(_ tab: Tab) -> Bool {
+        guard let url = tab.url else { return false }
+        return bookmarkManager.isUrlBookmarked(url: url)
+    }
+
     func bookmark(_ tab: Tab) {
         contextMenuActionSubject.send(.bookmark(tab))
     }
 
+    func removeBookmark(_ tab: Tab) {
+        contextMenuActionSubject.send(.removeBookmark(tab))
+    }
+
     func fireproof(_ tab: Tab) {
         contextMenuActionSubject.send(.fireproof(tab))
     }

DuckDuckGo/PinnedTabs/View/PinnedTabView.swift

@@ -89,11 +89,7 @@ struct PinnedTabView: View {
             collectionModel?.unpin(model)
         }
         Divider()
-        Button(UserText.bookmarkThisPage) { [weak collectionModel, weak model] in
-            guard let model = model else { return }
-            collectionModel?.bookmark(model)
-        }
-
+        bookmarkAction
         fireproofAction
         Divider()
         switch collectionModel.audioStateView {
@@ -127,6 +123,21 @@ struct PinnedTabView: View {
             }
         }
     }
+
+    @ViewBuilder
+    private var bookmarkAction: some View {
+        if collectionModel.isPinnedTabBookmarked(model) {
+            Button(UserText.deleteBookmark) { [weak collectionModel, weak model] in
+                guard let model = model else { return }
+                collectionModel?.removeBookmark(model)
+            }
+        } else {
+            Button(UserText.bookmarkThisPage) { [weak collectionModel, weak model] in
+                guard let model = model else { return }
+                collectionModel?.bookmark(model)
+            }
+        }
+    }
 }
 
 private struct BorderView: View {

DuckDuckGo/TabBar/View/TabBarViewController.swift

@@ -23,6 +23,8 @@ import Lottie
 import SwiftUI
 import WebKit
 
+// swiftlint:disable file_length
+
 final class TabBarViewController: NSViewController {
 
     enum HorizontalSpace: CGFloat {
@@ -309,6 +311,12 @@ final class TabBarViewController: NSViewController {
                 return
             }
             bookmarkTab(with: url, title: tabViewModel.title)
+        case let .removeBookmark(tab):
+            guard let url = tab.url else {
+                os_log("TabBarViewController: Failed to get url from tab")
+                return
+            }
+            deleteBookmark(with: url)
         case let .fireproof(tab):
             fireproof(tab)
         case let .removeFireproofing(tab):
@@ -773,6 +781,14 @@ extension TabBarViewController: TabCollectionViewModelDelegate {
         }
     }
 
+    private func deleteBookmark(with url: URL) {
+        guard let bookmark = bookmarkManager.getBookmark(for: url) else {
+            os_log("TabBarViewController: Failed to fetch bookmark for url \(url)", type: .error)
+            return
+        }
+        bookmarkManager.remove(bookmark: bookmark)
+    }
+
     private func fireproof(_ tab: Tab) {
         guard let url = tab.url, let host = url.host else {
             os_log("TabBarViewController: Failed to get url of tab bar view item", type: .error)
@@ -790,8 +806,25 @@ extension TabBarViewController: TabCollectionViewModelDelegate {
 
         FireproofDomains.shared.remove(domain: host)
     }
+
+    // MARK: - TabViewItem
+
+    func urlAndTitle(for tabBarViewItem: TabBarViewItem) -> (url: URL, title: String)? {
+        guard
+            let indexPath = collectionView.indexPath(for: tabBarViewItem),
+            let tabViewModel = tabCollectionViewModel.tabViewModel(at: indexPath.item),
+            let url = tabViewModel.tab.content.userEditableUrl
+        else {
+            os_log("TabBarViewController: Failed to get index path of tab bar view item", type: .error)
+            return nil
+        }
+
+        return (url, tabViewModel.title)
+    }
 }
 
+// MARK: - NSCollectionViewDelegateFlowLayout
+
 extension TabBarViewController: NSCollectionViewDelegateFlowLayout {
 
     func collectionView(_ collectionView: NSCollectionView, layout collectionViewLayout: NSCollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> NSSize {
@@ -801,6 +834,8 @@ extension TabBarViewController: NSCollectionViewDelegateFlowLayout {
 
 }
 
+// MARK: - NSCollectionViewDataSource
+
 extension TabBarViewController: NSCollectionViewDataSource {
 
     func numberOfSections(in collectionView: NSCollectionView) -> Int {
@@ -854,6 +889,8 @@ extension TabBarViewController: NSCollectionViewDataSource {
     }
 }
 
+// MARK: - NSCollectionViewDelegate
+
 extension TabBarViewController: NSCollectionViewDelegate {
 
     func collectionView(_ collectionView: NSCollectionView,
@@ -978,6 +1015,8 @@ extension TabBarViewController: NSCollectionViewDelegate {
 
 }
 
+// MARK: - TabBarViewItemDelegate
+
 extension TabBarViewController: TabBarViewItemDelegate {
 
     func tabBarViewItem(_ tabBarViewItem: TabBarViewItem, isMouseOver: Bool) {
@@ -1038,15 +1077,22 @@ extension TabBarViewController: TabBarViewItemDelegate {
         return tabCollectionViewModel.tabViewModel(at: indexPath.item)?.tab.content.canBeBookmarked ?? false
     }
 
+    func tabBarViewItemIsAlreadyBookmarked(_ tabBarViewItem: TabBarViewItem) -> Bool {
+        guard let url = urlAndTitle(for: tabBarViewItem)?.url else { return false }
+
+        return bookmarkManager.isUrlBookmarked(url: url)
+    }
+
     func tabBarViewItemBookmarkThisPageAction(_ tabBarViewItem: TabBarViewItem) {
-        guard let indexPath = collectionView.indexPath(for: tabBarViewItem),
-              let tabViewModel = tabCollectionViewModel.tabViewModel(at: indexPath.item),
-              let url = tabViewModel.tab.content.userEditableUrl else {
-            os_log("TabBarViewController: Failed to get index path of tab bar view item", type: .error)
-            return
-        }
+        guard let (url, title) = urlAndTitle(for: tabBarViewItem) else { return }
+
+        bookmarkTab(with: url, title: title)
+    }
+
+    func tabBarViewItemRemoveBookmarkAction(_ tabBarViewItem: TabBarViewItem) {
+        guard let url = urlAndTitle(for: tabBarViewItem)?.url else { return }
 
-        bookmarkTab(with: url, title: tabViewModel.title)
+        deleteBookmark(with: url)
     }
 
     func tabBarViewAllItemsCanBeBookmarked(_ tabBarViewItem: TabBarViewItem) -> Bool {
@@ -1180,6 +1226,8 @@ extension TabBarViewController: TabBarViewItemDelegate {
 
 }
 
+// MARK: - TabBarViewItemPasteboardWriter
+
 final class TabBarViewItemPasteboardWriter: NSObject, NSPasteboardWriting {
 
     static let utiInternalType = NSPasteboard.PasteboardType(rawValue: "com.duckduckgo.tab.internal")
@@ -1193,3 +1241,5 @@ final class TabBarViewItemPasteboardWriter: NSObject, NSPasteboardWriting {
     }
 
 }
+
+// swiftlint:enable file_length

DuckDuckGo/TabBar/View/TabBarViewItem.swift

@@ -33,6 +33,7 @@ protocol TabBarViewItemDelegate: AnyObject {
     func tabBarViewItemCanBeDuplicated(_ tabBarViewItem: TabBarViewItem) -> Bool
     func tabBarViewItemCanBePinned(_ tabBarViewItem: TabBarViewItem) -> Bool
     func tabBarViewItemCanBeBookmarked(_ tabBarViewItem: TabBarViewItem) -> Bool
+    func tabBarViewItemIsAlreadyBookmarked(_ tabBarViewItem: TabBarViewItem) -> Bool
     func tabBarViewAllItemsCanBeBookmarked(_ tabBarViewItem: TabBarViewItem) -> Bool
 
     func tabBarViewItemCloseAction(_ tabBarViewItem: TabBarViewItem)
@@ -43,6 +44,7 @@ protocol TabBarViewItemDelegate: AnyObject {
     func tabBarViewItemDuplicateAction(_ tabBarViewItem: TabBarViewItem)
     func tabBarViewItemPinAction(_ tabBarViewItem: TabBarViewItem)
     func tabBarViewItemBookmarkThisPageAction(_ tabBarViewItem: TabBarViewItem)
+    func tabBarViewItemRemoveBookmarkAction(_ tabBarViewItem: TabBarViewItem)
     func tabBarViewItemBookmarkAllOpenTabsAction(_ tabBarViewItem: TabBarViewItem)
     func tabBarViewItemMoveToNewWindowAction(_ tabBarViewItem: TabBarViewItem)
     func tabBarViewItemMoveToNewBurnerWindowAction(_ tabBarViewItem: TabBarViewItem)
@@ -199,6 +201,10 @@ final class TabBarViewItem: NSCollectionViewItem {
         delegate?.tabBarViewItemBookmarkThisPageAction(self)
     }
 
+    @objc func removeFromBookmarksAction(_ sender: Any) {
+        delegate?.tabBarViewItemRemoveBookmarkAction(self)
+    }
+
     @objc func bookmarkAllOpenTabsAction(_ sender: Any) {
         delegate?.tabBarViewItemBookmarkAllOpenTabsAction(self)
     }
@@ -502,7 +508,11 @@ extension TabBarViewItem: NSMenuDelegate {
 
         // Section 2
         addFireproofMenuItem(to: menu)
-        addBookmarkMenuItem(to: menu)
+        if let delegate, delegate.tabBarViewItemIsAlreadyBookmarked(self) {
+            removeBookmarkMenuItem(to: menu)
+        } else {
+            addBookmarkMenuItem(to: menu)
+        }
         menu.addItem(.separator())
 
         // Section 3
@@ -538,6 +548,12 @@ extension TabBarViewItem: NSMenuDelegate {
         menu.addItem(bookmarkMenuItem)
     }
 
+    private func removeBookmarkMenuItem(to menu: NSMenu) {
+        let bookmarkMenuItem = NSMenuItem(title: UserText.deleteBookmark, action: #selector(removeFromBookmarksAction(_:)), keyEquivalent: "")
+        bookmarkMenuItem.target = self
+        menu.addItem(bookmarkMenuItem)
+    }
+
     private func addBookmarkAllTabsMenuItem(to menu: NSMenu) {
         let bookmarkMenuItem = NSMenuItem(title: UserText.bookmarkAllTabs, action: #selector(bookmarkAllOpenTabsAction(_:)), keyEquivalent: "")
         bookmarkMenuItem.target = self

UnitTests/HomePage/Mocks/MockBookmarkManager.swift

@@ -25,8 +25,9 @@ class MockBookmarkManager: BookmarkManager {
         return false
     }
 
+    var isUrlBookmarked = false
     func isUrlBookmarked(url: URL) -> Bool {
-        return false
+        return isUrlBookmarked
     }
 
     func allHosts() -> Set<String> {