diff --git a/DuckDuckGo/Bookmarks/View/AddBookmarkPopoverView.swift b/DuckDuckGo/Bookmarks/View/AddBookmarkPopoverView.swift index 1faa240398..bcef29a4c5 100644 --- a/DuckDuckGo/Bookmarks/View/AddBookmarkPopoverView.swift +++ b/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, diff --git a/DuckDuckGo/Common/Localizables/UserText.swift b/DuckDuckGo/Common/Localizables/UserText.swift index b7ace49675..a3ee25f1f8 100644 --- a/DuckDuckGo/Common/Localizables/UserText.swift +++ b/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") diff --git a/DuckDuckGo/Localizable.xcstrings b/DuckDuckGo/Localizable.xcstrings index 673ca12212..caa202fc24 100644 --- a/DuckDuckGo/Localizable.xcstrings +++ b/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", diff --git a/DuckDuckGo/PinnedTabs/Model/PinnedTabsViewModel.swift b/DuckDuckGo/PinnedTabs/Model/PinnedTabsViewModel.swift index 7324862828..5c77dfdf96 100644 --- a/DuckDuckGo/PinnedTabs/Model/PinnedTabsViewModel.swift +++ b/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() @@ -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)) } diff --git a/DuckDuckGo/PinnedTabs/View/PinnedTabView.swift b/DuckDuckGo/PinnedTabs/View/PinnedTabView.swift index c9a0c87f8b..7bc278f80e 100644 --- a/DuckDuckGo/PinnedTabs/View/PinnedTabView.swift +++ b/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 { diff --git a/DuckDuckGo/TabBar/View/TabBarViewController.swift b/DuckDuckGo/TabBar/View/TabBarViewController.swift index b2d4be517a..f1e18cea7d 100644 --- a/DuckDuckGo/TabBar/View/TabBarViewController.swift +++ b/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 diff --git a/DuckDuckGo/TabBar/View/TabBarViewItem.swift b/DuckDuckGo/TabBar/View/TabBarViewItem.swift index 18042eb73f..4acca3216c 100644 --- a/DuckDuckGo/TabBar/View/TabBarViewItem.swift +++ b/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 diff --git a/UnitTests/HomePage/Mocks/MockBookmarkManager.swift b/UnitTests/HomePage/Mocks/MockBookmarkManager.swift index d1fafb18be..cf0c87be4d 100644 --- a/UnitTests/HomePage/Mocks/MockBookmarkManager.swift +++ b/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 { diff --git a/UnitTests/PinnedTabs/PinnedTabsViewModelTests.swift b/UnitTests/PinnedTabs/PinnedTabsViewModelTests.swift index c9f063d54f..f54376f4df 100644 --- a/UnitTests/PinnedTabs/PinnedTabsViewModelTests.swift +++ b/UnitTests/PinnedTabs/PinnedTabsViewModelTests.swift @@ -31,6 +31,7 @@ class PinnedTabsViewModelTests: XCTestCase { var model: PinnedTabsViewModel! var collection: TabCollection! + var bookmarkManagerMock: MockBookmarkManager! override func setUpWithError() throws { try super.setUpWithError() @@ -41,7 +42,8 @@ class PinnedTabsViewModelTests: XCTestCase { Tab(content: .url("http://d.com".url!, source: .link)), Tab(content: .url("http://e.com".url!, source: .link)) ]) - model = PinnedTabsViewModel(collection: collection) + bookmarkManagerMock = .init() + model = PinnedTabsViewModel(collection: collection, bookmarkManager: bookmarkManagerMock) } func testInitialState() throws { @@ -150,10 +152,11 @@ class PinnedTabsViewModelTests: XCTestCase { model.removeFireproofing(tabB) model.close(tabA) model.muteOrUmute(tabB) + model.removeBookmark(tabA) cancellable.cancel() - XCTAssertEqual(events.count, 7) + XCTAssertEqual(events.count, 8) guard case .bookmark(tabA) = events[0], case .unpin(1) = events[1], @@ -161,13 +164,38 @@ class PinnedTabsViewModelTests: XCTestCase { case .fireproof(tabA) = events[3], case .removeFireproofing(tabB) = events[4], case .close(0) = events[5], - case .muteOrUnmute(tabB) = events[6] + case .muteOrUnmute(tabB) = events[6], + case .removeBookmark(tabA) = events[7] else { XCTFail("Incorrect context menu action") return } } + func testWhenIsPinnedTabBookmarkedCalledAndURLIsBookmarkedThenReturnTrue() { + // GIVEN + bookmarkManagerMock.isUrlBookmarked = true + let tab = Tab(content: .url(URL.duckDuckGo, source: .link)) + + // WHEN + let result = model.isPinnedTabBookmarked(tab) + + // THEN + XCTAssertTrue(result) + } + + func testWhenIsPinnedTabBookmarkedCalledAndURLIsNotBookmarkedThenReturnFalse() { + // GIVEN + bookmarkManagerMock.isUrlBookmarked = false + let tab = Tab(content: .url(URL.duckDuckGo, source: .link)) + + // WHEN + let result = model.isPinnedTabBookmarked(tab) + + // THEN + XCTAssertFalse(result) + } + } private extension Array where Element == Tab { diff --git a/UnitTests/TabBar/View/MockTabViewItemDelegate.swift b/UnitTests/TabBar/View/MockTabViewItemDelegate.swift index 63b4a7fc97..680f840d9c 100644 --- a/UnitTests/TabBar/View/MockTabViewItemDelegate.swift +++ b/UnitTests/TabBar/View/MockTabViewItemDelegate.swift @@ -27,7 +27,10 @@ class MockTabViewItemDelegate: TabBarViewItemDelegate { var hasItemsToTheLeft = false var hasItemsToTheRight = false var audioState: WKWebView.AudioState? + var isTabBarItemAlreadyBookmarked = false + private(set) var tabBarViewItemBookmarkThisPageActionCalled = false + private(set) var tabBarViewItemRemoveBookmarkActionCalled = false private(set) var tabBarViewItemBookmarkAllOpenTabsActionCalled = false func tabBarViewItem(_ tabBarViewItem: DuckDuckGo_Privacy_Browser.TabBarViewItem, isMouseOver: Bool) { @@ -74,8 +77,16 @@ class MockTabViewItemDelegate: TabBarViewItemDelegate { mockedCurrentTab?.content.canBeBookmarked ?? true } + func tabBarViewItemIsAlreadyBookmarked(_ tabBarViewItem: TabBarViewItem) -> Bool { + isTabBarItemAlreadyBookmarked + } + func tabBarViewItemBookmarkThisPageAction(_ tabBarViewItem: DuckDuckGo_Privacy_Browser.TabBarViewItem) { + tabBarViewItemBookmarkThisPageActionCalled = true + } + func tabBarViewItemRemoveBookmarkAction(_ tabBarViewItem: TabBarViewItem) { + tabBarViewItemRemoveBookmarkActionCalled = true } func tabBarViewAllItemsCanBeBookmarked(_ tabBarViewItem: DuckDuckGo_Privacy_Browser.TabBarViewItem) -> Bool { diff --git a/UnitTests/TabBar/View/TabBarViewItemTests.swift b/UnitTests/TabBar/View/TabBarViewItemTests.swift index 19505467d6..975116e54b 100644 --- a/UnitTests/TabBar/View/TabBarViewItemTests.swift +++ b/UnitTests/TabBar/View/TabBarViewItemTests.swift @@ -46,7 +46,6 @@ final class TabBarViewItemTests: XCTestCase { XCTAssertEqual(menu.item(at: 1)?.title, UserText.pinTab) XCTAssertTrue(menu.item(at: 2)?.isSeparatorItem ?? false) XCTAssertEqual(menu.item(at: 3)?.title, UserText.fireproofSite) - XCTAssertEqual(menu.item(at: 4)?.title, UserText.bookmarkThisPage) XCTAssertTrue(menu.item(at: 5)?.isSeparatorItem ?? false) XCTAssertEqual(menu.item(at: 6)?.title, UserText.bookmarkAllTabs) XCTAssertTrue(menu.item(at: 7)?.isSeparatorItem ?? false) @@ -82,6 +81,28 @@ final class TabBarViewItemTests: XCTestCase { XCTAssertTrue(menu.item(at: 3)?.isSeparatorItem ?? false) } + func testWhenURLIsNotBookmarkedThenBookmarkThisPageIsShown() { + // GIVEN + delegate.isTabBarItemAlreadyBookmarked = false + + // WHEN + tabBarViewItem.menuNeedsUpdate(menu) + + // THEN + XCTAssertEqual(menu.item(at: 4)?.title, UserText.bookmarkThisPage) + } + + func testWhenURLIsBookmarkedThenDeleteBookmarkIsShown() { + // GIVEN + delegate.isTabBarItemAlreadyBookmarked = true + + // WHEN + tabBarViewItem.menuNeedsUpdate(menu) + + // THEN + XCTAssertEqual(menu.item(at: 4)?.title, UserText.deleteBookmark) + } + func testWhenOneTabCloseThenOtherTabsItemIsDisabled() { tabBarViewItem.menuNeedsUpdate(menu) @@ -259,4 +280,32 @@ final class TabBarViewItemTests: XCTestCase { XCTAssertTrue(delegate.tabBarViewItemBookmarkAllOpenTabsActionCalled) } + func testWhenClickingOnBookmarkThisPageThenTheActionDelegateIsNotified() throws { + // GIVEN + delegate.isTabBarItemAlreadyBookmarked = false + tabBarViewItem.menuNeedsUpdate(menu) + let index = try XCTUnwrap(menu.indexOfItem(withTitle: UserText.bookmarkThisPage)) + XCTAssertFalse(delegate.tabBarViewItemBookmarkThisPageActionCalled) + + // WHEN + menu.performActionForItem(at: index) + + // THEN + XCTAssertTrue(delegate.tabBarViewItemBookmarkThisPageActionCalled) + } + + func testWhenClickingOnDeleteBookmarkThenTheActionDelegateIsNotified() throws { + // GIVEN + delegate.isTabBarItemAlreadyBookmarked = true + tabBarViewItem.menuNeedsUpdate(menu) + let index = try XCTUnwrap(menu.indexOfItem(withTitle: UserText.deleteBookmark)) + XCTAssertFalse(delegate.tabBarViewItemRemoveBookmarkActionCalled) + + // WHEN + menu.performActionForItem(at: index) + + // THEN + XCTAssertTrue(delegate.tabBarViewItemRemoveBookmarkActionCalled) + } + }