From db4b4ab506aa99f43d0a0535b0c0638aa72db2f5 Mon Sep 17 00:00:00 2001 From: Alexey Martemyanov Date: Fri, 30 Aug 2024 12:41:40 +0600 Subject: [PATCH] Fix cells highlighting in different modes --- DuckDuckGo.xcodeproj/project.pbxproj | 6 +-- .../Model/BookmarkOutlineViewDataSource.swift | 19 ++++--- ...kmarkManagementSidebarViewController.swift | 3 +- .../View/BookmarkOutlineCellView.swift | 49 +++++++++++++------ .../Bookmarks/View/BookmarksOutlineView.swift | 10 +++- .../View/BookmarksBarMenuPopover.swift | 0 .../View/BookmarksBarMenuViewController.swift | 0 Submodules/privacy-reference-tests | 2 +- .../BookmarkOutlineViewDataSourceTests.swift | 27 ++++++++-- 9 files changed, 81 insertions(+), 35 deletions(-) rename DuckDuckGo/{Bookmarks => BookmarksBar}/View/BookmarksBarMenuPopover.swift (100%) rename DuckDuckGo/{Bookmarks => BookmarksBar}/View/BookmarksBarMenuViewController.swift (100%) diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index 711eabab9e..aaa2887597 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -6056,6 +6056,8 @@ 4BE5336A286912D40019DBFD /* BookmarksBarCollectionViewItem.swift */, 4BE53369286912D40019DBFD /* BookmarksBarCollectionViewItem.xib */, 85774AFE2A713D3B00DE0561 /* BookmarksBarMenuFactory.swift */, + 84F1C8CE2C7705B500716446 /* BookmarksBarMenuPopover.swift */, + 848648A02C76F4B20082282D /* BookmarksBarMenuViewController.swift */, 4BD18EFF283F0BC500058124 /* BookmarksBarViewController.swift */, 4BE41A5D28446EAD00760399 /* BookmarksBarViewModel.swift */, 4BE5336D286915A10019DBFD /* HorizontallyCenteredLayout.swift */, @@ -6482,8 +6484,8 @@ 859F30622A72A7A900C20372 /* Prompt */ = { isa = PBXGroup; children = ( - 859F30632A72A7BB00C20372 /* BookmarksBarPromptPopover.swift */, 859F30662A72B38500C20372 /* BookmarksBarPromptAssets.xcassets */, + 859F30632A72A7BB00C20372 /* BookmarksBarPromptPopover.swift */, ); path = Prompt; sourceTree = ""; @@ -7673,8 +7675,6 @@ 4B9292C72667123700AD2C21 /* BookmarkManagementSidebarViewController.swift */, 4B9292C82667123700AD2C21 /* BookmarkManagementSplitViewController.swift */, 4B92928726670D1600AD2C21 /* BookmarkOutlineCellView.swift */, - 84F1C8CE2C7705B500716446 /* BookmarksBarMenuPopover.swift */, - 848648A02C76F4B20082282D /* BookmarksBarMenuViewController.swift */, 4B92928526670D1600AD2C21 /* BookmarksOutlineView.swift */, 4B92928926670D1700AD2C21 /* BookmarkTableCellView.swift */, 4B9292C92667123700AD2C21 /* BookmarkTableRowView.swift */, diff --git a/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift b/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift index e422812048..365bfab540 100644 --- a/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift +++ b/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift @@ -22,7 +22,7 @@ import Foundation final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSource, NSOutlineViewDelegate { - enum ContentMode { + enum ContentMode: CaseIterable { case bookmarksAndFolders case foldersOnly case bookmarksMenu @@ -33,6 +33,13 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou case .foldersOnly: false } } + + var showMenuButtonOnHover: Bool { + switch self { + case .bookmarksAndFolders, .bookmarksMenu: true + case .foldersOnly: false + } + } } @Published var selectedFolders: [BookmarkFolder] = [] @@ -72,7 +79,6 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou private let treeController: BookmarkTreeController private let bookmarkManager: BookmarkManager private let dragDropManager: BookmarkDragDropManager - private let showMenuButtonOnHover: Bool private let presentFaviconsFetcherOnboarding: (() -> Void)? init( @@ -81,14 +87,12 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou treeController: BookmarkTreeController, dragDropManager: BookmarkDragDropManager = .shared, sortMode: BookmarksSortMode, - showMenuButtonOnHover: Bool = true, presentFaviconsFetcherOnboarding: (() -> Void)? = nil ) { self.contentMode = contentMode self.bookmarkManager = bookmarkManager self.dragDropManager = dragDropManager self.treeController = treeController - self.showMenuButtonOnHover = showMenuButtonOnHover self.presentFaviconsFetcherOnboarding = presentFaviconsFetcherOnboarding super.init() @@ -205,11 +209,10 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou ?? OutlineSeparatorViewCell(isSeparatorVisible: contentMode.isSeparatorVisible) } - let cell = outlineView.makeView(withIdentifier: .init(BookmarkOutlineCellView.className()), owner: self) as? BookmarkOutlineCellView - ?? BookmarkOutlineCellView(identifier: .init(BookmarkOutlineCellView.className())) - cell.shouldShowMenuButton = showMenuButtonOnHover + let cell = outlineView.makeView(withIdentifier: BookmarkOutlineCellView.identifier(for: contentMode), owner: self) as? BookmarkOutlineCellView + ?? BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: contentMode)) cell.delegate = self - cell.update(from: node, isSearch: isSearching, isMenuPopover: contentMode == .bookmarksMenu) + cell.update(from: node, isSearch: isSearching) if let bookmark = node.representedObject as? Bookmark, bookmark.favicon(.small) == nil { presentFaviconsFetcherOnboarding?() diff --git a/DuckDuckGo/Bookmarks/View/BookmarkManagementSidebarViewController.swift b/DuckDuckGo/Bookmarks/View/BookmarkManagementSidebarViewController.swift index 10f85605b2..1b34c36646 100644 --- a/DuckDuckGo/Bookmarks/View/BookmarkManagementSidebarViewController.swift +++ b/DuckDuckGo/Bookmarks/View/BookmarkManagementSidebarViewController.swift @@ -56,8 +56,7 @@ final class BookmarkManagementSidebarViewController: NSViewController { bookmarkManager: bookmarkManager, treeController: treeController, dragDropManager: dragDropManager, - sortMode: selectedSortMode, - showMenuButtonOnHover: false) + sortMode: selectedSortMode) private var cancellables = Set() private var selectedSortMode: BookmarksSortMode diff --git a/DuckDuckGo/Bookmarks/View/BookmarkOutlineCellView.swift b/DuckDuckGo/Bookmarks/View/BookmarkOutlineCellView.swift index 69a1fb2610..65088eedb2 100644 --- a/DuckDuckGo/Bookmarks/View/BookmarkOutlineCellView.swift +++ b/DuckDuckGo/Bookmarks/View/BookmarkOutlineCellView.swift @@ -26,6 +26,10 @@ protocol BookmarkOutlineCellViewDelegate: AnyObject { final class BookmarkOutlineCellView: NSTableCellView { private static let sizingCellIdentifier = NSUserInterfaceItemIdentifier("sizing") + static func identifier(for mode: BookmarkOutlineViewDataSource.ContentMode) -> NSUserInterfaceItemIdentifier { + NSUserInterfaceItemIdentifier("\(mode)_\(self.className())") + } + static let sizingCell = BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.sizingCellIdentifier) static let rowHeight: CGFloat = 28 @@ -51,7 +55,18 @@ final class BookmarkOutlineCellView: NSTableCellView { } } - var shouldShowMenuButton = false + var contentMode: BookmarkOutlineViewDataSource.ContentMode? { + BookmarkOutlineViewDataSource.ContentMode.allCases.first { mode in + Self.identifier(for: mode) == self.identifier + } + } + + var shouldShowMenuButton: Bool { + contentMode != .foldersOnly + } + var shouldShowChevron: Bool { + contentMode == .bookmarksMenu + } weak var delegate: BookmarkOutlineCellViewDelegate? @@ -222,7 +237,9 @@ final class BookmarkOutlineCellView: NSTableCellView { } if !titleLabel.isEnabled { titleLabel.textColor = .disabledControlTextColor - } else if highlight && isInKeyWindow { + } else if highlight, + isInKeyWindow, + contentMode != .foldersOnly { titleLabel.textColor = .selectedMenuItemTextColor urlLabel.textColor = .selectedMenuItemTextColor } else { @@ -253,19 +270,19 @@ final class BookmarkOutlineCellView: NSTableCellView { || representedObject is PseudoFolder || representedObject is MenuItemNode else { return 0 } sizingCell.frame = .zero - sizingCell.update(from: representedObject, isMenuPopover: true) + sizingCell.update(from: representedObject) sizingCell.layoutSubtreeIfNeeded() return sizingCell.frame.width + 6 } - func update(from object: Any, isSearch: Bool = false, isMenuPopover: Bool) { + func update(from object: Any, isSearch: Bool = false) { let representedObject = (object as? BookmarkNode)?.representedObject ?? object switch representedObject { case let bookmark as Bookmark: update(from: bookmark, isSearch: isSearch, showURL: identifier != Self.sizingCellIdentifier) case let folder as BookmarkFolder: - update(from: folder, isSearch: isSearch, showChevron: isMenuPopover) + update(from: folder, isSearch: isSearch) case let folder as PseudoFolder: update(from: folder) case let menuItem as MenuItemNode: @@ -298,18 +315,18 @@ final class BookmarkOutlineCellView: NSTableCellView { updateConstraints(isSearch: isSearch) } - func update(from folder: BookmarkFolder, isSearch: Bool = false, showChevron: Bool) { + func update(from folder: BookmarkFolder, isSearch: Bool = false) { faviconImageView.image = .folder faviconImageView.isHidden = false titleLabel.stringValue = folder.title titleLabel.isEnabled = true - favoriteImageView.image = showChevron ? .chevronMediumRight16 : nil + favoriteImageView.image = shouldShowChevron ? .chevronMediumRight16 : nil favoriteImageView.isHidden = favoriteImageView.image == nil urlLabel.stringValue = "" self.toolTip = nil let totalChildBookmarks = folder.totalChildBookmarks - if totalChildBookmarks > 0 && !showChevron { + if totalChildBookmarks > 0 && !shouldShowChevron { countLabel.stringValue = String(totalChildBookmarks) countLabel.isHidden = false } else { @@ -373,12 +390,12 @@ extension BookmarkOutlineCellView { BookmarkOutlineCellView(identifier: .init("")), BookmarkOutlineCellView(identifier: .init("")), BookmarkOutlineCellView(identifier: .init("")), + BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: .bookmarksMenu)), BookmarkOutlineCellView(identifier: .init("")), BookmarkOutlineCellView(identifier: .init("")), BookmarkOutlineCellView(identifier: .init("")), - BookmarkOutlineCellView(identifier: .init("")), - BookmarkOutlineCellView(identifier: .init("")), - BookmarkOutlineCellView(identifier: .init("")), + BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: .bookmarksMenu)), + BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: .bookmarksMenu)), BookmarkOutlineCellView(identifier: .init("")), BookmarkOutlineCellView(identifier: .init("")), ] @@ -397,22 +414,22 @@ extension BookmarkOutlineCellView { let bkm2 = Bookmark(id: "3", url: "http://a.b", title: "Bookmark with longer title to test width", isFavorite: false) cells[2].update(from: bkm2, showURL: false) - cells[3].update(from: BookmarkFolder(id: "4", title: "Bookmark Folder with a reasonably long name"), showChevron: true) - cells[4].update(from: BookmarkFolder(id: "5", title: "Bookmark Folder with 42 bookmark children", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)), showChevron: false) + cells[3].update(from: BookmarkFolder(id: "4", title: "Bookmark Folder with a reasonably long name")) + cells[4].update(from: BookmarkFolder(id: "5", title: "Bookmark Folder with 42 bookmark children", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42))) PseudoFolder.favorites.count = 64 cells[5].update(from: PseudoFolder.favorites) PseudoFolder.bookmarks.count = 256 cells[6].update(from: PseudoFolder.bookmarks) let node = BookmarkNode(representedObject: MenuItemNode(identifier: "", title: UserText.bookmarksOpenInNewTabs, isEnabled: true), parent: BookmarkNode.genericRootNode()) - cells[7].update(from: node, isMenuPopover: true) + cells[7].update(from: node) let emptyNode = BookmarkNode(representedObject: MenuItemNode(identifier: "", title: UserText.bookmarksBarFolderEmpty, isEnabled: false), parent: BookmarkNode.genericRootNode()) - cells[8].update(from: emptyNode, isMenuPopover: true) + cells[8].update(from: emptyNode) let sbkm = Bookmark(id: "3", url: "http://a.b", title: "Bookmark in Search mode", isFavorite: false) cells[9].update(from: sbkm, isSearch: true, showURL: false) - cells[10].update(from: BookmarkFolder(id: "5", title: "Folder in Search mode", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)), isSearch: true, showChevron: false) + cells[10].update(from: BookmarkFolder(id: "5", title: "Folder in Search mode", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)), isSearch: true) widthAnchor.constraint(equalToConstant: 258).isActive = true heightAnchor.constraint(equalToConstant: CGFloat((28 + 1) * cells.count)).isActive = true diff --git a/DuckDuckGo/Bookmarks/View/BookmarksOutlineView.swift b/DuckDuckGo/Bookmarks/View/BookmarksOutlineView.swift index b1ecfeecc6..46b2fde825 100644 --- a/DuckDuckGo/Bookmarks/View/BookmarksOutlineView.swift +++ b/DuckDuckGo/Bookmarks/View/BookmarksOutlineView.swift @@ -83,6 +83,9 @@ final class BookmarksOutlineView: NSOutlineView { return nil } + private var isInPopover: Bool { + popover != nil + } private var isInKeyPopover: Bool { guard highlightedRow != nil else { return false } // is there a child menu popover window owned by our window? @@ -103,6 +106,11 @@ final class BookmarksOutlineView: NSOutlineView { // mark highlight with inactive color for non-key popover menu and with active color for key popover menu private func updateIsInKeyPopoverState() { + guard isInPopover else { + highlightedRowView?.isInKeyWindow = false + highlightedCellView?.isInKeyWindow = false + return + } // when no highlighted row - our parent is the key popover guard highlightedRow != nil else { parentMenuOutlineView?.updateIsInKeyPopoverState() @@ -366,7 +374,7 @@ final class BookmarksOutlineView: NSOutlineView { } if highlightedRow != row { highlightedRow = row - } else { + } else if isInPopover { highlightedRowView?.isInKeyWindow = true highlightedCellView?.isInKeyWindow = true } diff --git a/DuckDuckGo/Bookmarks/View/BookmarksBarMenuPopover.swift b/DuckDuckGo/BookmarksBar/View/BookmarksBarMenuPopover.swift similarity index 100% rename from DuckDuckGo/Bookmarks/View/BookmarksBarMenuPopover.swift rename to DuckDuckGo/BookmarksBar/View/BookmarksBarMenuPopover.swift diff --git a/DuckDuckGo/Bookmarks/View/BookmarksBarMenuViewController.swift b/DuckDuckGo/BookmarksBar/View/BookmarksBarMenuViewController.swift similarity index 100% rename from DuckDuckGo/Bookmarks/View/BookmarksBarMenuViewController.swift rename to DuckDuckGo/BookmarksBar/View/BookmarksBarMenuViewController.swift diff --git a/Submodules/privacy-reference-tests b/Submodules/privacy-reference-tests index 6133e7d9d9..afb4f6128a 160000 --- a/Submodules/privacy-reference-tests +++ b/Submodules/privacy-reference-tests @@ -1 +1 @@ -Subproject commit 6133e7d9d9cd5f1b925cab1971b4d785dc639df7 +Subproject commit afb4f6128a3b50d53ddcb1897ea1fb4df6858aa1 diff --git a/UnitTests/Bookmarks/Model/BookmarkOutlineViewDataSourceTests.swift b/UnitTests/Bookmarks/Model/BookmarkOutlineViewDataSourceTests.swift index 76cd699840..da8fdc537f 100644 --- a/UnitTests/Bookmarks/Model/BookmarkOutlineViewDataSourceTests.swift +++ b/UnitTests/Bookmarks/Model/BookmarkOutlineViewDataSourceTests.swift @@ -223,35 +223,54 @@ class BookmarkOutlineViewDataSourceTests: XCTestCase { } @MainActor - func testWhenShowMenuButtonOnHoverIsTrue_ThenCellShouldHaveShouldMenuButtonFlagTrue() throws { + func testWhenContentModeIsBookmarksAndFolders_ThenCellShouldHaveShouldMenuButtonFlagTrue() throws { // GIVEN let mockFolder = BookmarkFolder.mock let mockOutlineView = NSOutlineView(frame: .zero) let treeController = createTreeController(with: [mockFolder]) let mockFolderNode = treeController.node(representing: mockFolder)! - let dataSource = BookmarkOutlineViewDataSource(contentMode: .bookmarksAndFolders, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual, showMenuButtonOnHover: true) + let dataSource = BookmarkOutlineViewDataSource(contentMode: .bookmarksAndFolders, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual) // WHEN let cell = try XCTUnwrap(dataSource.outlineView(mockOutlineView, viewFor: nil, item: mockFolderNode) as? BookmarkOutlineCellView) // THEN XCTAssertTrue(cell.shouldShowMenuButton) + XCTAssertFalse(cell.shouldShowChevron) } @MainActor - func testWhenShowMenuButtonOnHoverIsFalse_ThenCellShouldHaveShouldMenuButtonFlagFalse() throws { + func testWhenContentModeIsFolders_ThenCellShouldHaveShouldMenuButtonFlagFalse() throws { // GIVEN let mockFolder = BookmarkFolder.mock let mockOutlineView = NSOutlineView(frame: .zero) let treeController = createTreeController(with: [mockFolder]) let mockFolderNode = treeController.node(representing: mockFolder)! - let dataSource = BookmarkOutlineViewDataSource(contentMode: .foldersOnly, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual, showMenuButtonOnHover: false) + let dataSource = BookmarkOutlineViewDataSource(contentMode: .foldersOnly, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual) // WHEN let cell = try XCTUnwrap(dataSource.outlineView(mockOutlineView, viewFor: nil, item: mockFolderNode) as? BookmarkOutlineCellView) // THEN XCTAssertFalse(cell.shouldShowMenuButton) + XCTAssertFalse(cell.shouldShowChevron) + } + + @MainActor + func testWhenContentModeIsBookmarksMenu_ThenCellShouldHaveShouldMenuButtonFlagFalse() throws { + // GIVEN + let mockFolder = BookmarkFolder.mock + let mockOutlineView = NSOutlineView(frame: .zero) + let treeController = createTreeController(with: [mockFolder]) + let mockFolderNode = treeController.node(representing: mockFolder)! + let dataSource = BookmarkOutlineViewDataSource(contentMode: .bookmarksMenu, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual) + + // WHEN + let cell = try XCTUnwrap(dataSource.outlineView(mockOutlineView, viewFor: nil, item: mockFolderNode) as? BookmarkOutlineCellView) + + // THEN + XCTAssertTrue(cell.shouldShowMenuButton) + XCTAssertTrue(cell.shouldShowChevron) } // MARK: - Private