Add FXIOS-9744 Hide desktop bookmarks folder #23074
Conversation
| recursiveAddSubFolders(folder, folders: &folders) | ||
| } | ||
| recursive: true).uponQueue(.main) { data in | ||
| self.profile.places.countBookmarksInTrees(folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) |
There was a problem hiding this comment.
I think to enhance readability by removing double nesting we can split this method in an async one like:
private func countBookmarksInTrees() async -> Int? {
}And call it directly before the continuation since fetchFolder is an async method.
| continuation.resume(returning: folders) | ||
| } | ||
| guard let rootFolder = data.successValue as? BookmarkFolderData, | ||
| let desktopBookmarksCount = bookmarksCountResult.successValue else { return } |
There was a problem hiding this comment.
we can uplift this statement at the start of fetchFolder if we go for having a another method for counting bookmarks
| @@ -32,6 +32,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]) -> Deferred<Maybe<Int>> | |||
There was a problem hiding this comment.
i think we should remove Deferred from our code base since i guess it is legacy code. what do you think to refactor this either with an async method if possible otherwise a closure ? 👀
| profile.places.countBookmarksInTrees(folderGuids: BookmarkRoots.DesktopRoots.map { $0 }) | ||
| .uponQueue(.main) { bookmarksCountResult in | ||
| var desktopBookmarksCount: Int? | ||
| defer { continuation.resume(returning: desktopBookmarksCount) } |
There was a problem hiding this comment.
i'd put the defer on multi line so it is clearly visible
defer {
}
| let profile: Profile | ||
| let rootFolderGUID: String | ||
|
|
||
| func fetchFolders() async -> [Folder] { | ||
| let numDesktopBookmarks = await countDesktopBookmarks() |
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func recursiveAddSubFolders(_ folder: BookmarkFolderData, | ||
| folders: inout [Folder], | ||
| hasDesktopBookmarks: Bool = false, |
There was a problem hiding this comment.
i'd remove the default value since we are passing it everywhere
| @@ -199,6 +201,17 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver { | |||
| } | |||
| } | |||
|
|
|||
| public func countBookmarksInTrees(folderGuids: [GUID], completion: @escaping (Result<Int, Error>) -> Void) { | |||
| withReader { connection in | |||
There was a problem hiding this comment.
i'd think the reader needs to be stored inside a variable and check wether there was reader errors, because it could happen that there were connection error and callback provided to withReader will never be called. I assume that by reading the implementation of withReader
| @@ -137,6 +138,7 @@ public class RustPlaces: BookmarksHandler, HistoryMetadataObserver { | |||
| return deferred | |||
| } | |||
|
|
|||
| @discardableResult | |||
There was a problem hiding this comment.
I think we should remove this following up the previous comment
MattLichtenstein
force-pushed
the
ml/FXIOS-9744_hide-desktop-bookmarks-folder
branch
from
73acc28 to
9a9658c
Compare
MattLichtenstein
had a problem deploying
to
github-pages
GitHub Actions
Failure
Client.app: Coverage: 30.17
libStorage.a: Coverage: 55.99
Generated by 🚫 Danger Swift against 9a9658c |
MattLichtenstein
deleted the
ml/FXIOS-9744_hide-desktop-bookmarks-folder
branch
📜 Tickets
Jira ticket
Github issue
💡 Description
Desktop Bookmarksroot folder (and nestedToolbar,Menu, andUnfiled) if it does not contain recursively any bookmarks (even when signed into a Mozilla Account)📝 Discussion
Toolbar,Menu, andUnfiledare now conditionally hidden fromBookmarksViewController,LegacyBookmarkDetailPanelandEditBookmarkViewController📦 Dependencies
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)