Skip to content

Commit

Permalink
Support close tabs to left and move close actions to submenu (#2664)
Browse files Browse the repository at this point in the history
Task/Issue URL:
https://app.asana.com/0/1199230911884351/1207117125840765/f
Tech Design URL:
CC: @ayoy 

**Description**:
This PR adds the option to close all tabs to the left when right
clicking a tab. It also moves this action as well as "close to right"
and "close other tabs" to a submenu in the tab context menu.

**Steps to test this PR**:
1. Open some tabs
2. Right click a tab, ensure "Close Other Tabs" opens a sub menu
3. Clicking "Close Tabs to the Left" should close all tabs to the left
of the clicked tab except pinned tabs
4. "Close to the right" and "Close all other tabs" should work as
expected.

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)

---------

Co-authored-by: Dominik Kapusta <[email protected]>
  • Loading branch information
SlayterDev and ayoy authored Apr 22, 2024
1 parent 05aa76a commit 585742d
Show file tree
Hide file tree
Showing 9 changed files with 191 additions and 10 deletions.
2 changes: 2 additions & 0 deletions DuckDuckGo/Common/Localizables/UserText.swift
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
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
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
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
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
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
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
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
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

0 comments on commit 585742d

Please sign in to comment.