Skip to content

Commit

Permalink
Add FXIOS-9744 Hide desktop bookmarks folder (#23074)
Browse files Browse the repository at this point in the history
  • Loading branch information
@MattLichtenstein
MattLichtenstein authored Nov 15, 2024
1 parent 80863c3 commit acbdac0
Show file tree
Hide file tree
Showing 6 changed files with 182 additions and 51 deletions.
22 changes: 17 additions & 5 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Shared
import class MozillaAppServices.BookmarkFolderData
import enum MozillaAppServices.BookmarkRoots

class BookmarksPanelViewModel {
class BookmarksPanelViewModel: BookmarksRefactorFeatureFlagProvider {
enum BookmarksSection: Int, CaseIterable {
case bookmarks
}
Expand Down Expand Up @@ -114,10 +114,22 @@ class BookmarksPanelViewModel {
self.bookmarkFolder = mobileFolder
self.bookmarkNodes = mobileFolder.fxChildren ?? []

let desktopFolder = LocalDesktopFolder()
self.bookmarkNodes.insert(desktopFolder, at: 0)

completion()
// Create a local "Desktop bookmarks" folder only if there exists a bookmark in one of it's nested
// subfolders
self.bookmarksHandler.countBookmarksInTrees(folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) { result in
DispatchQueue.main.async {
switch result {
case .success(let bookmarkCount):
if bookmarkCount > 0 || !self.isBookmarkRefactorEnabled {
let desktopFolder = LocalDesktopFolder()
self.bookmarkNodes.insert(desktopFolder, at: 0)
}
case .failure(let error):
self.logger.log("Error counting bookmarks: \(error)", level: .debug, category: .library)
}
completion()
}
}
}
}

Expand Down
105 changes: 65 additions & 40 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/LegacyBookmarkDetailPanel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class BookmarkDetailPanelError: MaybeErrorType {
public var description = "Unable to save BookmarkNode."
}

class LegacyBookmarkDetailPanel: SiteTableViewController {
class LegacyBookmarkDetailPanel: SiteTableViewController, BookmarksRefactorFeatureFlagProvider {
private struct UX {
static let FieldRowHeight: CGFloat = 58
static let FolderIconSize = CGSize(width: 24, height: 24)
Expand Down Expand Up @@ -113,6 +113,8 @@ class LegacyBookmarkDetailPanel: SiteTableViewController {
button.addTarget(self, action: #selector(self.deleteBookmarkButtonTapped), for: .touchUpInside)
}

private var logger: Logger

// MARK: - Initializers
convenience init(
profile: Profile,
Expand Down Expand Up @@ -181,13 +183,15 @@ class LegacyBookmarkDetailPanel: SiteTableViewController {
bookmarkNodeType: BookmarkNodeType,
parentBookmarkFolder: FxBookmarkNode,
presentedFromToast fromToast: Bool = false,
bookmarkItemURL: String? = nil
bookmarkItemURL: String? = nil,
logger: Logger = DefaultLogger.shared
) {
self.bookmarkNodeGUID = bookmarkNodeGUID
self.bookmarkNodeType = bookmarkNodeType
self.parentBookmarkFolder = parentBookmarkFolder
self.isPresentedFromToast = fromToast
self.bookmarkItemURL = bookmarkItemURL
self.logger = logger

super.init(profile: profile, windowUUID: windowUUID)

Expand Down Expand Up @@ -253,48 +257,69 @@ class LegacyBookmarkDetailPanel: SiteTableViewController {
override func reloadData() {
// Can be called while app backgrounded and the db closed, don't try to reload the data source in this case
if profile.isShutdown { return }
profile.places.getBookmarksTree(rootGUID: BookmarkRoots.RootGUID, recursive: true).uponQueue(.main) { result in
guard let rootFolder = result.successValue as? BookmarkFolderData else {
// TODO: Handle error case?
self.bookmarkFolders = []
self.tableView.reloadData()
return
}

var bookmarkFolders: [(folder: BookmarkFolderData, indent: Int)] = []

func addFolder(_ folder: BookmarkFolderData, indent: Int = 0) {
// Do not append itself and the top "root" folder to this list as
// bookmarks cannot be stored directly within it.
if folder.guid != BookmarkRoots.RootGUID && folder.guid != self.bookmarkNodeGUID {
bookmarkFolders.append((folder, indent))
}

var folderChildren: [BookmarkNodeData]?
// Suitable to be appended
if folder.guid != self.bookmarkNodeGUID {
folderChildren = folder.children
}

for case let childFolder as BookmarkFolderData in folderChildren ?? [] {
// Any "root" folders (i.e. "Mobile Bookmarks") should
// have an indentation of 0.
if childFolder.isRoot {
addFolder(childFolder)
profile.places.getBookmarksTree(rootGUID: BookmarkRoots.RootGUID, recursive: true)
.uponQueue(.main) { bookmarksTreeResult in
self.profile.places.countBookmarksInTrees(
folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) { bookmarksCountResult in
DispatchQueue.main.async {
guard let rootFolder = bookmarksTreeResult.successValue as? BookmarkFolderData else {
// TODO: Handle error case?
self.bookmarkFolders = []
self.tableView.reloadData()
return
}
// Otherwise, all non-root folder should increase the
// indentation by 1.
else {
addFolder(childFolder, indent: indent + 1)

var bookmarkFolders: [(folder: BookmarkFolderData, indent: Int)] = []

func addFolderAndDescendants(_ folder: BookmarkFolderData, indent: Int = 0) {
// Do not append itself and the top "root" folder to this list as
// bookmarks cannot be stored directly within it.
if folder.guid != BookmarkRoots.RootGUID && folder.guid != self.bookmarkNodeGUID {
bookmarkFolders.append((folder, indent))
}

var folderChildren: [BookmarkNodeData]?
// Suitable to be appended
if folder.guid != self.bookmarkNodeGUID {
folderChildren = folder.children
}

func addFolder(childFolder: BookmarkFolderData) {
// Any "root" folders (i.e. "Mobile Bookmarks") should
// have an indentation of 0.
if childFolder.isRoot {
addFolderAndDescendants(childFolder)
}
// Otherwise, all non-root folder should increase the
// indentation by 1.
else {
addFolderAndDescendants(childFolder, indent: indent + 1)
}
}

for case let childFolder as BookmarkFolderData in folderChildren ?? [] {
// Only append desktop folders if they already contain bookmarks
if !BookmarkRoots.DesktopRoots.contains(childFolder.guid) {
addFolder(childFolder: childFolder)
} else {
switch bookmarksCountResult {
case .success(let bookmarkCount):
if (bookmarkCount > 0 && BookmarkRoots.DesktopRoots.contains(childFolder.guid))
|| !self.isBookmarkRefactorEnabled {
addFolder(childFolder: childFolder)
}
case .failure(let error):
self.logger.log("Error counting bookmarks: \(error)", level: .debug, category: .library)
}
}
}
}
addFolderAndDescendants(rootFolder)
self.bookmarkFolders = bookmarkFolders
self.tableView.reloadData()
}
}
}

addFolder(rootFolder)

self.bookmarkFolders = bookmarkFolders
self.tableView.reloadData()
}
}

func updateSaveButton() {
Expand Down
35 changes: 30 additions & 5 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Utitlity/FolderHierarchyFetcher.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,36 +29,61 @@ struct Folder: Equatable {
}
}

struct DefaultFolderHierarchyFetcher: FolderHierarchyFetcher {
struct DefaultFolderHierarchyFetcher: FolderHierarchyFetcher, BookmarksRefactorFeatureFlagProvider {
let profile: Profile
let rootFolderGUID: String

func fetchFolders() async -> [Folder] {
let numDesktopBookmarks = await countDesktopBookmarks()
return await withCheckedContinuation { continuation in
profile.places.getBookmarksTree(rootGUID: rootFolderGUID,
recursive: true).uponQueue(.main) { data in
recursive: true).uponQueue(.main) { data in
var folders = [Folder]()
defer {
continuation.resume(returning: folders)
}
guard let rootFolder = data.successValue as? BookmarkFolderData else { return }
let hasDesktopBookmarks = (numDesktopBookmarks ?? 0) > 0

let childrenFolders = rootFolder.children?.compactMap {
return $0 as? BookmarkFolderData
}

for folder in childrenFolders ?? [] {
recursiveAddSubFolders(folder, folders: &folders)
recursiveAddSubFolders(folder, folders: &folders, hasDesktopBookmarks: hasDesktopBookmarks)
}
}
}
}

private func recursiveAddSubFolders(_ folder: BookmarkFolderData,
folders: inout [Folder],
hasDesktopBookmarks: Bool,
indent: Int = 0) {
folders.append(Folder(title: folder.title, guid: folder.guid, indentation: indent))
if !BookmarkRoots.DesktopRoots.contains(folder.guid) || hasDesktopBookmarks || !isBookmarkRefactorEnabled {
folders.append(Folder(title: folder.title, guid: folder.guid, indentation: indent))
} else { return }
for case let subFolder as BookmarkFolderData in folder.children ?? [] {
let indentation = subFolder.isRoot ? 0 : indent + 1
recursiveAddSubFolders(subFolder, folders: &folders, indent: indentation)
recursiveAddSubFolders(
subFolder,
folders: &folders,
hasDesktopBookmarks: hasDesktopBookmarks,
indent: indentation
)
}
}

private func countDesktopBookmarks() async -> Int? {
return await withCheckedContinuation { continuation in
profile.places.countBookmarksInTrees(folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) { result in
switch result {
case .success(let count):
continuation.resume(returning: count)
case .failure:
continuation.resume(returning: nil)
}
}
}
}
}
15 changes: 15 additions & 0 deletions firefox-ios/Storage/Rust/RustPlaces.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import struct MozillaAppServices.VisitTransitionSet
public protocol BookmarksHandler {
func getRecentBookmarks(limit: UInt, completion: @escaping ([BookmarkItemData]) -> Void)
func getBookmarksTree(rootGUID: GUID, recursive: Bool) -> Deferred<Maybe<BookmarkNodeData?>>
func countBookmarksInTrees(folderGuids: [GUID], completion: @escaping (Result<Int, Error>) -> Void)
func updateBookmarkNode(
guid: GUID,
parentGUID: GUID?,
Expand Down Expand Up @@ -200,6 +201,20 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver {
}
}

public func countBookmarksInTrees(folderGuids: [GUID], completion: @escaping (Result<Int, Error>) -> Void) {
let deferredResponse = withReader { connection in
return try connection.countBookmarksInTrees(folderGuids: folderGuids)
}

deferredResponse.upon { result in
if let count = result.successValue {
completion(.success(count))
} else if let error = result.failureValue {
completion(.failure(error))
}
}
}

public func getRecentBookmarks(limit: UInt) -> Deferred<Maybe<[BookmarkItemData]>> {
return withReader { connection in
return try connection.getRecentBookmarks(limit: limit)
Expand Down
4 changes: 4 additions & 0 deletions ...os/firefox-ios-tests/Tests/ClientTests/Frontend/Home/Bookmarks/BookmarksHandlerMock.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,8 @@ class BookmarksHandlerMock: BookmarksHandler {
url: String?) -> Success {
succeed()
}

func countBookmarksInTrees(folderGuids: [GUID], completion: @escaping (Result<Int, Error>) -> Void) {
completion(.success(0))
}
}
52 changes: 51 additions & 1 deletion ...s/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/BookmarkPanelViewModelTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import XCTest

@testable import Client

class BookmarksPanelViewModelTests: XCTestCase {
class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {
private var profile: MockProfile!

override func setUp() {
Expand Down Expand Up @@ -58,6 +58,7 @@ class BookmarksPanelViewModelTests: XCTestCase {

func testShouldReload_whenMobileEmptyBookmarks() throws {
profile.reopen()
LegacyFeatureFlagsManager.shared.initializeDeveloperFeatures(with: profile)
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
let expectation = expectation(description: "Subject reloaded")
subject.reloadData {
Expand All @@ -68,6 +69,55 @@ class BookmarksPanelViewModelTests: XCTestCase {
waitForExpectations(timeout: 5)
}

func testShouldReload_whenMobileEmptyBookmarksWithBookmarksRefactor() throws {
profile.reopen()
LegacyFeatureFlagsManager.shared.initializeDeveloperFeatures(with: profile)
featureFlags.set(feature: .bookmarksRefactor, to: true, isDebug: true)
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
let expectation = expectation(description: "Subject reloaded")
subject.reloadData {
XCTAssertNotNil(subject.bookmarkFolder)
XCTAssertEqual(subject.bookmarkNodes.count, 0, "Contains no folders")
expectation.fulfill()
}
waitForExpectations(timeout: 5)
}

func testShouldReload_whenDesktopBookmarksExist() throws {
profile.reopen()
LegacyFeatureFlagsManager.shared.initializeDeveloperFeatures(with: profile)
featureFlags.set(feature: .bookmarksRefactor, to: true)
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)

let expectation = expectation(description: "Subject reloaded")

profile.places.createBookmark(
parentGUID: BookmarkRoots.MenuFolderGUID,
url: "https://www.firefox.com",
title: "Firefox",
position: 0
).uponQueue(.main) { _ in
self.profile.places.countBookmarksInTrees(folderGuids: [BookmarkRoots.MenuFolderGUID]) { result in
DispatchQueue.main.async {
switch result {
case .success(let bookmarkCount):
XCTAssertEqual(bookmarkCount, 1, "Menu folder contains one bookmark")

subject.reloadData {
XCTAssertNotNil(subject.bookmarkFolder)
XCTAssertEqual(subject.bookmarkNodes.count, 1, "Mobile folder contains the local desktop folder")
expectation.fulfill()
}
case .failure(let error):
XCTFail("Failed to count bookmarks: \(error)")
expectation.fulfill()
}
}
}
}
waitForExpectations(timeout: 5)
}

func testShouldReload_whenLocalDesktopFolder() {
profile.reopen()
let subject = createSubject(guid: LocalDesktopFolder.localDesktopFolderGuid)
Expand Down

0 comments on commit acbdac0

Please sign in to comment.