Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support close tabs to left and move close actions to submenu #2664

Merged
merged 7 commits into from
Apr 22, 2024
Merged

Support close tabs to left and move close actions to submenu #2664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d028511
Support close tabs to left and move close actions to submenu
SlayterDev Apr 18, 2024
e92f7cb
Add submenu assertions
SlayterDev Apr 18, 2024
f2c2db3
Fix tab selection logic after removing tabs and add unit tests
SlayterDev Apr 18, 2024
7602865
Add more unit tests for removing tabs to the left
ayoy Apr 19, 2024
7352553
Fix tab selection
SlayterDev Apr 19, 2024
a746a9a
Revert mistakenly changed files
SlayterDev Apr 19, 2024
4b93941
Merge branch 'brad/address-tab-feedback' into brad/close-all-left
SlayterDev Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions DuckDuckGo/Common/Localizables/UserText.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ struct UserText {
static let muteTab = NSLocalizedString("mute.tab", value: "Mute Tab", comment: "Menu item. Mute tab")
static let unmuteTab = NSLocalizedString("unmute.tab", value: "Unmute Tab", comment: "Menu item. Unmute tab")
static let closeOtherTabs = NSLocalizedString("close.other.tabs", value: "Close Other Tabs", comment: "Menu item")
static let closeAllOtherTabs = NSLocalizedString("close.all.other.tabs", value: "Close All Other Tabs", comment: "Menu item")
static let closeTabsToTheLeft = NSLocalizedString("close.tabs.to.the.left", value: "Close Tabs to the Left", comment: "Menu item")
static let closeTabsToTheRight = NSLocalizedString("close.tabs.to.the.right", value: "Close Tabs to the Right", comment: "Menu item")
static let openInNewTab = NSLocalizedString("open.in.new.tab", value: "Open in New Tab", comment: "Menu item that opens the link in a new tab")
static let openInNewWindow = NSLocalizedString("open.in.new.window", value: "Open in New Window", comment: "Menu item that opens the link in a new window")
Expand Down
24 changes: 24 additions & 0 deletions DuckDuckGo/Localizable.xcstrings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10739,6 +10739,18 @@
}
}
},
"close.all.other.tabs" : {
"comment" : "Menu item",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Close All Other Tabs"
}
}
}
},
"close.other.tabs" : {
"comment" : "Menu item",
"extractionState" : "extracted_with_value",
Expand Down Expand Up @@ -10979,6 +10991,18 @@
}
}
},
"close.tabs.to.the.left" : {
"comment" : "Menu item",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Close Tabs to the Left"
}
}
}
},
"close.tabs.to.the.right" : {
"comment" : "Menu item",
"extractionState" : "extracted_with_value",
Expand Down
5 changes: 5 additions & 0 deletions DuckDuckGo/TabBar/Model/TabCollection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ final class TabCollection: NSObject {
tabs = tab.map { [$0] } ?? []
}

func removeTabs(before index: Int) {
tabsWillClose(range: 0..<index)
tabs.removeSubrange(0..<index)
}

func removeTabs(after index: Int) {
tabsWillClose(range: (index + 1)..<tabs.count)
tabs.removeSubrange((index + 1)...)
Expand Down
9 changes: 9 additions & 0 deletions DuckDuckGo/TabBar/View/TabBarViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,15 @@ extension TabBarViewController: TabBarViewItemDelegate {
tabCollectionViewModel.removeAllTabs(except: indexPath.item)
}

func tabBarViewItemCloseToTheLeftAction(_ tabBarViewItem: TabBarViewItem) {
guard let indexPath = collectionView.indexPath(for: tabBarViewItem) else {
assertionFailure("TabBarViewController: Failed to get index path of tab bar view item")
return
}

tabCollectionViewModel.removeTabs(before: indexPath.item)
}

func tabBarViewItemCloseToTheRightAction(_ tabBarViewItem: TabBarViewItem) {
guard let indexPath = collectionView.indexPath(for: tabBarViewItem) else {
assertionFailure("TabBarViewController: Failed to get index path of tab bar view item")
Expand Down
32 changes: 29 additions & 3 deletions DuckDuckGo/TabBar/View/TabBarViewItem.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ protocol TabBarViewItemDelegate: AnyObject {
func tabBarViewItemCloseAction(_ tabBarViewItem: TabBarViewItem)
func tabBarViewItemTogglePermissionAction(_ tabBarViewItem: TabBarViewItem)
func tabBarViewItemCloseOtherAction(_ tabBarViewItem: TabBarViewItem)
func tabBarViewItemCloseToTheLeftAction(_ tabBarViewItem: TabBarViewItem)
func tabBarViewItemCloseToTheRightAction(_ tabBarViewItem: TabBarViewItem)
func tabBarViewItemDuplicateAction(_ tabBarViewItem: TabBarViewItem)
func tabBarViewItemPinAction(_ tabBarViewItem: TabBarViewItem)
Expand Down Expand Up @@ -229,6 +230,10 @@ final class TabBarViewItem: NSCollectionViewItem {
delegate?.tabBarViewItemCloseOtherAction(self)
}

@objc func closeToTheLeftAction(_ sender: NSMenuItem) {
delegate?.tabBarViewItemCloseToTheLeftAction(self)
}

@objc func closeToTheRightAction(_ sender: NSMenuItem) {
delegate?.tabBarViewItemCloseToTheRightAction(self)
}
Expand Down Expand Up @@ -497,8 +502,7 @@ extension TabBarViewItem: NSMenuDelegate {

// Section 3
addCloseMenuItem(to: menu)
addCloseOtherMenuItem(to: menu, areThereOtherTabs: areThereOtherTabs)
addCloseTabsToTheRightMenuItem(to: menu, areThereTabsToTheRight: otherItemsState.hasItemsToTheRight)
addCloseOtherSubmenu(to: menu, tabBarItemState: otherItemsState)
if !isBurner {
addMoveToNewWindowMenuItem(to: menu, areThereOtherTabs: areThereOtherTabs)
}
Expand Down Expand Up @@ -555,13 +559,35 @@ extension TabBarViewItem: NSMenuDelegate {
menu.addItem(closeMenuItem)
}

private func addCloseOtherSubmenu(to menu: NSMenu, tabBarItemState: OtherTabBarViewItemsState) {
let closeOtherMenuItem = NSMenuItem(title: UserText.closeOtherTabs)
let submenu = NSMenu()
submenu.autoenablesItems = false

addCloseTabsToTheLeftMenuItem(to: submenu, areThereTabsToTheLeft: tabBarItemState.hasItemsToTheLeft)
addCloseTabsToTheRightMenuItem(to: submenu, areThereTabsToTheRight: tabBarItemState.hasItemsToTheRight)
addCloseOtherMenuItem(to: submenu, areThereOtherTabs: tabBarItemState.hasItemsToTheLeft || tabBarItemState.hasItemsToTheRight)

closeOtherMenuItem.submenu = submenu
menu.addItem(closeOtherMenuItem)
}

private func addCloseOtherMenuItem(to menu: NSMenu, areThereOtherTabs: Bool) {
let closeOtherMenuItem = NSMenuItem(title: UserText.closeOtherTabs, action: #selector(closeOtherAction(_:)), keyEquivalent: "")
let closeOtherMenuItem = NSMenuItem(title: UserText.closeAllOtherTabs, action: #selector(closeOtherAction(_:)), keyEquivalent: "")
closeOtherMenuItem.target = self
closeOtherMenuItem.isEnabled = areThereOtherTabs
menu.addItem(closeOtherMenuItem)
}

private func addCloseTabsToTheLeftMenuItem(to menu: NSMenu, areThereTabsToTheLeft: Bool) {
let closeTabsToTheLeftMenuItem = NSMenuItem(title: UserText.closeTabsToTheLeft,
action: #selector(closeToTheLeftAction(_:)),
keyEquivalent: "")
closeTabsToTheLeftMenuItem.target = self
closeTabsToTheLeftMenuItem.isEnabled = areThereTabsToTheLeft
menu.addItem(closeTabsToTheLeftMenuItem)
}

private func addCloseTabsToTheRightMenuItem(to menu: NSMenu, areThereTabsToTheRight: Bool) {
let closeTabsToTheRightMenuItem = NSMenuItem(title: UserText.closeTabsToTheRight,
action: #selector(closeToTheRightAction(_:)),
Expand Down
16 changes: 16 additions & 0 deletions DuckDuckGo/TabBar/ViewModel/TabCollectionViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,22 @@ final class TabCollectionViewModel: NSObject {
delegate?.tabCollectionViewModelDidMultipleChanges(self)
}

func removeTabs(before index: Int) {
guard changesEnabled else { return }

tabCollection.removeTabs(before: index)

if let currentSelection = selectionIndex, currentSelection.isUnpinnedTab {
if currentSelection.item < index {
selectionIndex = .unpinned(0)
} else {
selectionIndex = .unpinned(currentSelection.item - index)
}
}

delegate?.tabCollectionViewModelDidMultipleChanges(self)
}

func removeTabs(after index: Int) {
guard changesEnabled else { return }

Expand Down
7 changes: 6 additions & 1 deletion UnitTests/TabBar/View/MockTabViewItemDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class MockTabViewItemDelegate: TabBarViewItemDelegate {

var mockedCurrentTab: Tab?

var hasItemsToTheLeft = false
var hasItemsToTheRight = false
var audioState: WKWebView.AudioState?

Expand All @@ -42,6 +43,10 @@ class MockTabViewItemDelegate: TabBarViewItemDelegate {

}

func tabBarViewItemCloseToTheLeftAction(_ tabBarViewItem: DuckDuckGo_Privacy_Browser.TabBarViewItem) {

}

func tabBarViewItemCloseToTheRightAction(_ tabBarViewItem: DuckDuckGo_Privacy_Browser.TabBarViewItem) {

}
Expand Down Expand Up @@ -95,7 +100,7 @@ class MockTabViewItemDelegate: TabBarViewItemDelegate {
}

func otherTabBarViewItemsState(for tabBarViewItem: DuckDuckGo_Privacy_Browser.TabBarViewItem) -> DuckDuckGo_Privacy_Browser.OtherTabBarViewItemsState {
OtherTabBarViewItemsState(hasItemsToTheLeft: false, hasItemsToTheRight: hasItemsToTheRight)
OtherTabBarViewItemsState(hasItemsToTheLeft: hasItemsToTheLeft, hasItemsToTheRight: hasItemsToTheRight)
}

func clear() {
Expand Down
41 changes: 35 additions & 6 deletions UnitTests/TabBar/View/TabBarViewItemTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,16 @@ final class TabBarViewItemTests: XCTestCase {
XCTAssertTrue(menu.item(at: 5)?.isSeparatorItem ?? false)
XCTAssertEqual(menu.item(at: 6)?.title, UserText.closeTab)
XCTAssertEqual(menu.item(at: 7)?.title, UserText.closeOtherTabs)
XCTAssertEqual(menu.item(at: 8)?.title, UserText.closeTabsToTheRight)
XCTAssertEqual(menu.item(at: 9)?.title, UserText.moveTabToNewWindow)
XCTAssertEqual(menu.item(at: 8)?.title, UserText.moveTabToNewWindow)

// Check "Close Other Tabs" submenu
guard let submenu = menu.item(at: 7)?.submenu else {
XCTFail("\"Close Other Tabs\" menu item should have a submenu")
return
}
XCTAssertEqual(submenu.item(at: 0)?.title, UserText.closeTabsToTheLeft)
XCTAssertEqual(submenu.item(at: 1)?.title, UserText.closeTabsToTheRight)
XCTAssertEqual(submenu.item(at: 2)?.title, UserText.closeAllOtherTabs)
}

func testThatMuteIsShownWhenCurrentAudioStateIsUnmuted() {
Expand All @@ -75,15 +83,17 @@ final class TabBarViewItemTests: XCTestCase {
func testWhenOneTabCloseThenOtherTabsItemIsDisabled() {
tabBarViewItem.menuNeedsUpdate(menu)

let item = menu.items .first { $0.title == UserText.closeOtherTabs }
let submenu = menu.items .first { $0.title == UserText.closeOtherTabs }
let item = submenu?.submenu?.items .first { $0.title == UserText.closeAllOtherTabs }
XCTAssertFalse(item?.isEnabled ?? true)
}

func testWhenMultipleTabsThenCloseOtherTabsItemIsEnabled() {
delegate.hasItemsToTheRight = true
tabBarViewItem.menuNeedsUpdate(menu)

let item = menu.items .first { $0.title == UserText.closeOtherTabs }
let submenu = menu.items .first { $0.title == UserText.closeOtherTabs }
let item = submenu?.submenu?.items .first { $0.title == UserText.closeAllOtherTabs }
XCTAssertTrue(item?.isEnabled ?? false)
}

Expand All @@ -102,18 +112,37 @@ final class TabBarViewItemTests: XCTestCase {
XCTAssertTrue(item?.isEnabled ?? false)
}

func testWhenNoTabsToTheLeftThenCloseTabsToTheLeftIsDisabled() {
tabBarViewItem.menuNeedsUpdate(menu)

let submenu = menu.items .first { $0.title == UserText.closeOtherTabs }
let item = submenu?.submenu?.items .first { $0.title == UserText.closeTabsToTheLeft }
XCTAssertFalse(item?.isEnabled ?? true)
}

func testWhenTabsToTheLeftThenCloseTabsToTheLeftIsEnabled() {
delegate.hasItemsToTheLeft = true
tabBarViewItem.menuNeedsUpdate(menu)

let submenu = menu.items .first { $0.title == UserText.closeOtherTabs }
let item = submenu?.submenu?.items .first { $0.title == UserText.closeTabsToTheLeft }
XCTAssertTrue(item?.isEnabled ?? false)
}

func testWhenNoTabsToTheRightThenCloseTabsToTheRightIsDisabled() {
tabBarViewItem.menuNeedsUpdate(menu)

let item = menu.items .first { $0.title == UserText.closeTabsToTheRight }
let submenu = menu.items .first { $0.title == UserText.closeOtherTabs }
let item = submenu?.submenu?.items .first { $0.title == UserText.closeTabsToTheRight }
XCTAssertFalse(item?.isEnabled ?? true)
}

func testWhenTabsToTheRightThenCloseTabsToTheRightIsEnabled() {
delegate.hasItemsToTheRight = true
tabBarViewItem.menuNeedsUpdate(menu)

let item = menu.items .first { $0.title == UserText.closeTabsToTheRight }
let submenu = menu.items .first { $0.title == UserText.closeOtherTabs }
let item = submenu?.submenu?.items .first { $0.title == UserText.closeTabsToTheRight }
XCTAssertTrue(item?.isEnabled ?? false)
}

Expand Down
65 changes: 65 additions & 0 deletions UnitTests/TabBar/ViewModel/TabCollectionViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,71 @@ final class TabCollectionViewModelTests: XCTestCase {
XCTAssertEqual(firstTab, tabCollectionViewModel.selectedTabViewModel?.tab)
}

func testWhenTabsToTheLeftAreRemovedAndSelectionIsRemoved_ThenSelectionIsCorrectlyUpdated() {
let tabCollectionViewModel = TabCollectionViewModel.aTabCollectionViewModel()

tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()

tabCollectionViewModel.select(at: .unpinned(1))
tabCollectionViewModel.removeTabs(before: 2)

XCTAssertEqual(tabCollectionViewModel.selectionIndex?.item, 0)
}

func testWhenTabsToTheLeftAreRemovedAndSelectionRemains_ThenSelectionIsCorrectlyUpdated() {
let tabCollectionViewModel = TabCollectionViewModel.aTabCollectionViewModel()

tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.select(at: .unpinned(3))
tabCollectionViewModel.removeTabs(before: 3)

XCTAssertEqual(tabCollectionViewModel.selectionIndex?.item, 0)
}

func testWhenTabsToTheLeftAreRemovedAndSelectionRemainsAndIsToTheRight_ThenSelectionIsCorrectlyUpdated() {
let tabCollectionViewModel = TabCollectionViewModel.aTabCollectionViewModel()

tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.select(at: .unpinned(4))
tabCollectionViewModel.removeTabs(before: 2)

XCTAssertEqual(tabCollectionViewModel.selectionIndex?.item, 2)
}

func testWhenTabsToTheRightAreRemovedAndSelectionIsRemoved_ThenSelectionIsCorrectlyUpdated() {
let tabCollectionViewModel = TabCollectionViewModel.aTabCollectionViewModel()

tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()

tabCollectionViewModel.select(at: .unpinned(1))
tabCollectionViewModel.removeTabs(after: 0)

XCTAssertEqual(tabCollectionViewModel.selectionIndex?.item, 0)
}

func testWhenTabsToTheRightAreRemovedAndSelectionRemains_ThenSelectionIsCorrectlyUpdated() {
let tabCollectionViewModel = TabCollectionViewModel.aTabCollectionViewModel()

tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.appendNewTab()
tabCollectionViewModel.select(at: .unpinned(1))
tabCollectionViewModel.removeTabs(after: 1)

XCTAssertEqual(tabCollectionViewModel.selectionIndex?.item, 1)
}

func testWhenLastTabIsRemoved_ThenSelectionIsNil() {
let tabCollectionViewModel = TabCollectionViewModel.aTabCollectionViewModel()

Expand Down
Loading