Skip to content

Commit

Permalink
Alessandro/bookmarks ship review design (#2386)
Browse files Browse the repository at this point in the history
  • Loading branch information
alessandroboron committed Mar 14, 2024
1 parent 872b40d commit 72f84ec
Show file tree
Hide file tree
Showing 10 changed files with 116 additions and 10 deletions.
14 changes: 14 additions & 0 deletions DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -2459,6 +2459,8 @@
9FA173EB2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FA173EA2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift */; };
9FA173EC2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FA173EA2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift */; };
9FA173ED2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FA173EA2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift */; };
9FA75A3E2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FA75A3D2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift */; };
9FA75A3F2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FA75A3D2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift */; };
9FDA6C212B79A59D00E099A9 /* BookmarkFavoriteView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FDA6C202B79A59D00E099A9 /* BookmarkFavoriteView.swift */; };
9FDA6C222B79A59D00E099A9 /* BookmarkFavoriteView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FDA6C202B79A59D00E099A9 /* BookmarkFavoriteView.swift */; };
9FDA6C232B79A59D00E099A9 /* BookmarkFavoriteView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9FDA6C202B79A59D00E099A9 /* BookmarkFavoriteView.swift */; };
Expand Down Expand Up @@ -4056,6 +4058,7 @@
9FA173E22B7A12B600EE4E6E /* BookmarkDialogFolderManagementView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarkDialogFolderManagementView.swift; sourceTree = "<group>"; };
9FA173E62B7B122E00EE4E6E /* BookmarkDialogStackedContentView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarkDialogStackedContentView.swift; sourceTree = "<group>"; };
9FA173EA2B7B232200EE4E6E /* AddEditBookmarkDialogView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AddEditBookmarkDialogView.swift; sourceTree = "<group>"; };
9FA75A3D2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarksBarMenuFactoryTests.swift; sourceTree = "<group>"; };
9FDA6C202B79A59D00E099A9 /* BookmarkFavoriteView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarkFavoriteView.swift; sourceTree = "<group>"; };
9FEE98642B846870002E44E8 /* AddEditBookmarkView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AddEditBookmarkView.swift; sourceTree = "<group>"; };
9FEE98682B85B869002E44E8 /* BookmarksDialogViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarksDialogViewModel.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -6750,6 +6753,14 @@
path = Dialog;
sourceTree = "<group>";
};
9FA75A3C2BA00DF500DA5FA6 /* Factory */ = {
isa = PBXGroup;
children = (
9FA75A3D2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift */,
);
path = Factory;
sourceTree = "<group>";
};
AA0877B626D515EE00B05660 /* UserAgent */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -7098,6 +7109,7 @@
isa = PBXGroup;
children = (
9F872D9B2B9058B000138637 /* Extensions */,
9FA75A3C2BA00DF500DA5FA6 /* Factory */,
9F982F102B82264400231028 /* ViewModels */,
AA652CAE25DD8228009059CC /* Model */,
AA652CAF25DD822C009059CC /* Services */,
Expand Down Expand Up @@ -10722,6 +10734,7 @@
3706FE4A293F661700E42796 /* BookmarkManagedObjectTests.swift in Sources */,
EEC8EB402982CD550065AA39 /* JSAlertViewModelTests.swift in Sources */,
3706FE4B293F661700E42796 /* BookmarksHTMLImporterTests.swift in Sources */,
9FA75A3F2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift in Sources */,
56D145E929E6BB6300E3488A /* CapturingDataImportProvider.swift in Sources */,
3706FE4C293F661700E42796 /* CSVParserTests.swift in Sources */,
3706FE4D293F661700E42796 /* OnboardingTests.swift in Sources */,
Expand Down Expand Up @@ -12595,6 +12608,7 @@
B6656E122B29E3BE008798A1 /* DownloadListStoreMock.swift in Sources */,
37D23780287EFEE200BCE03B /* PinnedTabsManagerTests.swift in Sources */,
AA0877BA26D5161D00B05660 /* WebKitVersionProviderTests.swift in Sources */,
9FA75A3E2BA00E1400DA5FA6 /* BookmarksBarMenuFactoryTests.swift in Sources */,
B69B50462726C5C200758A2B /* AtbAndVariantCleanupTests.swift in Sources */,
567DA94529E95C3F008AC5EE /* YoutubeOverlayUserScriptTests.swift in Sources */,
4B59024C26B38BB800489384 /* ChromiumLoginReaderTests.swift in Sources */,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ final class BookmarkOutlineViewDataSource: NSObject, NSOutlineViewDataSource, NS
@Published var selectedFolders: [BookmarkFolder] = []

let treeController: BookmarkTreeController
var expandedNodesIDs = Set<String>()
private(set) var expandedNodesIDs = Set<String>()

private let contentMode: ContentMode
private let bookmarkManager: BookmarkManager
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/Bookmarks/Services/ContextualMenu.swift
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ private extension ContextualMenu {
deleteBookmarkMenuItem(bookmark: bookmark),
moveToEndMenuItem(entity: bookmark, parent: parent),
NSMenuItem.separator(),
addFolderMenuItem(folder: nil),
addFolderMenuItem(folder: parent),
manageBookmarksMenuItem(),
]
}
Expand Down
3 changes: 2 additions & 1 deletion DuckDuckGo/Bookmarks/View/BookmarkListViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,8 @@ final class BookmarkListViewController: NSViewController {
}

@objc func newFolderButtonClicked(_ sender: AnyObject) {
let view = BookmarksDialogViewFactory.makeAddBookmarkFolderView(parentFolder: nil)
let parentFolder = sender.representedObject as? BookmarkFolder
let view = BookmarksDialogViewFactory.makeAddBookmarkFolderView(parentFolder: parentFolder)
showDialog(view: view)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,13 +211,25 @@ final class BookmarkManagementSidebarViewController: NSViewController {
// MARK: NSOutlineView Configuration

private func expandAndRestore(selectedNodes: [BookmarkNode]) {
// OutlineView doesn't allow multiple selections so there should be only one selected node at time.
let selectedNode = selectedNodes.first
// As the data source reloaded we need to refresh the previously selected nodes.
// Lets consider the scenario where we add a folder to a subfolder.
// When the folder is added we need to "refresh" the node because the previously selected node folder has changed (it has a child folder now).
var refreshedSelectedNodes: [BookmarkNode] = []

treeController.visitNodes { node in
if let objectID = (node.representedObject as? BaseBookmarkEntity)?.id {
if dataSource.expandedNodesIDs.contains(objectID) {
outlineView.expandItem(node)
} else {
outlineView.collapseItem(node)
}

// Add the node if it contains previously selected folder
if let folder = selectedNode?.representedObject as? BookmarkFolder, folder.id == objectID {
refreshedSelectedNodes.append(node)
}
}

// Expand the Bookmarks pseudo folder automatically.
Expand All @@ -226,7 +238,7 @@ final class BookmarkManagementSidebarViewController: NSViewController {
}
}

restoreSelection(to: selectedNodes)
restoreSelection(to: refreshedSelectedNodes)
}

private func restoreSelection(to nodes: [BookmarkNode]) {
Expand Down
7 changes: 7 additions & 0 deletions DuckDuckGo/BookmarksBar/View/BookmarksBarMenuFactory.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ struct BookmarksBarMenuFactory {
menu.addItem(makeMenuItem(prefs))
}

static func addToMenuWithManageBookmarksSection(_ menu: NSMenu, target: AnyObject, addFolderSelector: Selector, manageBookmarksSelector: Selector, prefs: AppearancePreferences = .shared) {
addToMenu(menu, prefs)
menu.addItem(.separator())
menu.addItem(NSMenuItem(title: UserText.addFolder, action: addFolderSelector, target: target))
menu.addItem(NSMenuItem(title: UserText.bookmarksManageBookmarks, action: manageBookmarksSelector, target: target))
}

private static func makeMenuItem( _ prefs: AppearancePreferences) -> NSMenuItem {
let item = NSMenuItem(title: UserText.showBookmarksBar, action: nil, keyEquivalent: "B")
item.submenu = NSMenu(items: [
Expand Down
21 changes: 17 additions & 4 deletions DuckDuckGo/BookmarksBar/View/BookmarksBarViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ private extension BookmarksBarViewController {
case .deleteEntity:
bookmarkManager.remove(bookmark: bookmark)
case .addFolder:
showDialog(view: BookmarksDialogViewFactory.makeAddBookmarkFolderView(parentFolder: nil))
addFolder(inParent: nil)
case .manageBookmarks:
manageBookmarks()
}
Expand All @@ -266,7 +266,7 @@ private extension BookmarksBarViewController {
case .deleteEntity:
bookmarkManager.remove(folder: folder)
case .addFolder:
showDialog(view: BookmarksDialogViewFactory.makeAddBookmarkFolderView(parentFolder: folder))
addFolder(inParent: folder)
case .openInNewTab:
openAllInNewTabs(folder: folder)
case .openInNewWindow:
Expand Down Expand Up @@ -314,14 +314,22 @@ private extension BookmarksBarViewController {
menu.popUp(positioning: nil, at: CGPoint(x: 0, y: view.frame.minY - 7), in: view)
}

func addFolder(inParent parent: BookmarkFolder?) {
showDialog(view: BookmarksDialogViewFactory.makeAddBookmarkFolderView(parentFolder: parent))
}

func showDialog(view: any ModalView) {
view.show(in: self.view.window)
}

func manageBookmarks() {
@objc func manageBookmarks() {
WindowControllersManager.shared.showBookmarksTab()
}

@objc func addFolder(sender: NSMenuItem) {
addFolder(inParent: nil)
}

}

// MARK: - Menu
Expand All @@ -330,7 +338,12 @@ extension BookmarksBarViewController: NSMenuDelegate {

public func menuNeedsUpdate(_ menu: NSMenu) {
menu.removeAllItems()
BookmarksBarMenuFactory.addToMenu(menu)
BookmarksBarMenuFactory.addToMenuWithManageBookmarksSection(
menu,
target: self,
addFolderSelector: #selector(addFolder(sender:)),
manageBookmarksSelector: #selector(manageBookmarks)
)
}

}
Expand Down
59 changes: 59 additions & 0 deletions UnitTests/Bookmarks/Factory/BookmarksBarMenuFactoryTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
//
// BookmarksBarMenuFactoryTests.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import XCTest
@testable import DuckDuckGo_Privacy_Browser

final class BookmarksBarMenuFactoryTests: XCTestCase {

func testReturnAddFolderAndManageBookmarksWhenAddToMenuWithManageBookmarksSectionIsCalled() {
// GIVEN
let menu = NSMenu(title: "")
let targetMock = BookmarksBarTargetMock()
XCTAssertTrue(menu.items.isEmpty)

// WHEN
BookmarksBarMenuFactory.addToMenuWithManageBookmarksSection(menu, target: targetMock, addFolderSelector: #selector(targetMock.addFolder(_:)), manageBookmarksSelector: #selector(targetMock.manageBookmarks))

// THEN
XCTAssertEqual(menu.items.count, 4)
XCTAssertEqual(menu.items[1].title, "")
XCTAssertNil(menu.items[1].action)
XCTAssertEqual(menu.items[2].title, UserText.addFolder)
XCTAssertEqual(menu.items[2].action, #selector(targetMock.addFolder(_:)))
XCTAssertEqual(menu.items[3].title, UserText.bookmarksManageBookmarks)
XCTAssertEqual(menu.items[3].action, #selector(targetMock.manageBookmarks))
}

func testShouldNotReturnAddFolderAndManageBookmarksWhenAddToMenuIsCalled() {
// GIVEN
let menu = NSMenu(title: "")
XCTAssertTrue(menu.items.isEmpty)

// WHEN
BookmarksBarMenuFactory.addToMenu(menu)

// THEN
XCTAssertEqual(menu.items.count, 1)
}
}

private class BookmarksBarTargetMock: NSObject {
@objc func addFolder(_ sender: NSMenuItem) {}
@objc func manageBookmarks() {}
}
2 changes: 1 addition & 1 deletion UnitTests/Bookmarks/Model/ContextualMenuTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ final class ContextualMenuTests: XCTestCase {
assertMenu(item: items[7], withTitle: UserText.bookmarksBarContextMenuDelete, selector: #selector(BookmarkMenuItemSelectors.deleteBookmark(_:)), representedObject: bookmark)
assertMenu(item: items[8], withTitle: UserText.bookmarksBarContextMenuMoveToEnd, selector: #selector(BookmarkMenuItemSelectors.moveToEnd(_:)), representedObject: BookmarkEntityInfo(entity: bookmark, parent: parent))
assertMenu(item: items[9], withTitle: "", selector: nil) // Separator
assertMenu(item: items[10], withTitle: UserText.addFolder, selector: #selector(BookmarkMenuItemSelectors.newFolder(_:)))
assertMenu(item: items[10], withTitle: UserText.addFolder, selector: #selector(BookmarkMenuItemSelectors.newFolder(_:)), representedObject: parent)
assertMenu(item: items[11], withTitle: UserText.bookmarksManageBookmarks, selector: #selector(BookmarkMenuItemSelectors.manageBookmarks(_:)))
}

Expand Down
2 changes: 1 addition & 1 deletion UnitTests/Bookmarks/Model/LocalBookmarkManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ final class LocalBookmarkManagerTests: XCTestCase {

bookmarkManager.update(folder: folder, andMoveToParent: .parent(uuid: parent.id))

let topLevelEntities = try XCTUnwrap(bookmarkList?.topLevelEntities)
withExtendedLifetime(cancellable) {}
XCTAssertTrue(bookmarkStoreMock.updateFolderAndMoveToParentCalled)
XCTAssertEqual(bookmarkStoreMock.capturedFolder, folder)
XCTAssertEqual(bookmarkStoreMock.capturedParentFolderType, .parent(uuid: parent.id))
Expand Down

0 comments on commit 72f84ec

Please sign in to comment.