Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FXIOS-10928 [Bookmarks Evolution] Last Viewed Bookmarks Folder Persists Across App Sessions #24047

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+62 −9
Open

Add FXIOS-10928 [Bookmarks Evolution] Last Viewed Bookmarks Folder Persists Across App Sessions #24047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions BrowserKit/Sources/Shared/Prefs.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ public struct PrefsKeys {
// The guid of the bookmark folder that was most recently created or saved to by the user.
// Used to indicate where we should save the next bookmark by default.
public static let RecentBookmarkFolder = "RecentBookmarkFolder"

// The guid of the bookmark folder that was last viewed.
// Used to indicate which folder should be opened across app sessions.
public static let LastViewedBookmarkFolder = "LastViewedBookmarkFolder"

// Represents whether or not the bookmark refactor feature flag is enabled
// Used in the share extension
public static let IsBookmarksRefactorEnabled = "IsBookmarksRefactorEnabled"
Expand Down
6 changes: 3 additions & 3 deletions firefox-ios/Client/Coordinators/Library/BookmarksCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Common
import MozillaAppServices

protocol BookmarksCoordinatorDelegate: AnyObject, LibraryPanelCoordinatorDelegate {
func start(from folder: FxBookmarkNode)
func start(fromGUID folderGUID: String, animated: Bool)

/// Shows the bookmark detail to modify a bookmark folder
func showBookmarkDetail(for node: FxBookmarkNode, folder: FxBookmarkNode, completion: (() -> Void)?)
Expand Down Expand Up @@ -75,10 +75,10 @@ class BookmarksCoordinator: BaseCoordinator,

// MARK: - BookmarksCoordinatorDelegate

func start(from folder: FxBookmarkNode) {
func start(fromGUID folderGUID: String, animated: Bool = true) {
let viewModel = BookmarksPanelViewModel(profile: profile,
bookmarksHandler: profile.places,
bookmarkFolderGUID: folder.guid)
bookmarkFolderGUID: folderGUID)
if isBookmarkRefactorEnabled {
let controller = BookmarksViewController(viewModel: viewModel, windowUUID: windowUUID)
controller.bookmarkCoordinatorDelegate = self
Expand Down
45 changes: 44 additions & 1 deletion firefox-ios/Client/Coordinators/Library/LibraryCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Foundation
import Shared
import Storage

import enum MozillaAppServices.VisitType
import MozillaAppServices

protocol LibraryCoordinatorDelegate: AnyObject, LibraryPanelDelegate, RecentlyClosedPanelDelegate {
func didFinishLibrary(from coordinator: Coordinator)
Expand Down Expand Up @@ -135,12 +135,55 @@ class LibraryCoordinator: BaseCoordinator,
if isBookmarkRefactorEnabled {
(navigationController.topViewController as? BookmarksViewController)?
.bookmarkCoordinatorDelegate = bookmarksCoordinator
if let recentFolderGUID = profile.prefs.stringForKey(PrefsKeys.LastViewedBookmarkFolder) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I understand why this is easier to be done here, but IMO we shouldn't have any business logic inside the coordinator. Coordinators role should be as simple as possible, and only hold navigation logic (no decision making apart from feature flags, and no database/network calls). Coordinator mostly creates VC, pass down dependencies, hold child coordinators, start flows, but they get told which navigation flow to start since they are not responsible per say of anything. Decision making normally falls into VC or other managers of some sort.

Now, what does this mean here? Well, IMO that some changes are needed 🙈 Maybe a way to keep this simple is to have a bookmarks state manager of some sort? The LibraryViewController could be responsible to fetch the path, then start the proper coordinator flow (through a parameter on the start(panelType:) function? Just trying to throw in some ideas, let me know if you have a better idea!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I'll look into a different way of doing this that keeps the business logic out of the coordinators 😃 👍

CATransaction.begin()
CATransaction.setDisableActions(true)

populateBookmarksNavigationController(currentFolderGUID: recentFolderGUID,
bookmarksCoordinator: bookmarksCoordinator) {
CATransaction.commit()
}
}
} else {
(navigationController.topViewController as? LegacyBookmarksPanel)?
.bookmarkCoordinatorDelegate = bookmarksCoordinator
}
}

private func populateBookmarksNavigationController(currentFolderGUID: String,
bookmarksCoordinator: BookmarksCoordinator,
completion: @escaping () -> Void) {
profile.places.getBookmarksTree(rootGUID: BookmarkRoots.MobileFolderGUID,
lmarceau marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to need to account for potentially loading into one of the desktop folders, so we'll need to pass in BookmarkRoots.RootGUID instead of BookmarksRoots.MobileFolderGUID. This gets you most of the way there, but because of the way the folder structure is set up in the places db, things get a little tricky. The internal db folder structure looks something like this:

root
├ menu
├ toolbar
├ unfiled
├ mobile
│ ├ Example folder 1
│ ├ Example folder 2
│ ├ Example folder n

As you can see, the places db has no concept of the "Desktop Bookmarks" folder that, in the UI, parents the 3 desktop folders (menu, toolbar, and unfiled). That is something we create just for a better UX. This means if we restore the app to:

  • Any of the 3 desktop folders: Clicking "<" will take us to the root
  • Desktop Bookmarks: well, it will never find this guid because it doesn't exist in the db

The first one, not so bad, we can live with that, the second one... well the library panel probably won't load.

I haven't done much investigation into what a proper fix would look like, so I am not sure the level of effort, but if it is a lot, as a backup I think we can just avoid the really unhappy path and hardcode it such that if a user is restoring into the "Desktop Bookmarks" folder, we just load them back to the "mobile" folder. What do you think?

recursive: true).uponQueue(.main) { result in
guard let maybeBookmarkTreeRoot = result.successValue,
let bookmarkTreeRoot = maybeBookmarkTreeRoot else { return }
guard var path = self.findPathToFolder(targetGUID: currentFolderGUID,
treeRootNode: bookmarkTreeRoot),
!path.isEmpty else {return}
path.removeFirst()
path.forEach { el in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does el stands for? Can we have another name for this variable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, when this forEach is called, what does it look like if we have a lot of nested folders? Animations are set to false, so I guess to the user it's as if we magically appeared inside the nested folder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the behaviour is the same as if we were to close the library panel, and re-open it without restarting the app. From what I understood this was the behaviour we were trying to mimic. If we would like to have animations active I can do that as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without animation is good! Just wanted to confirm I understood properly, thank you!

bookmarksCoordinator.start(fromGUID: el.guid, animated: false)
}
completion()
}
}

private func findPathToFolder(targetGUID: String, treeRootNode: BookmarkNodeData) -> [BookmarkNodeData]? {
guard let treeRootFolder = treeRootNode as? BookmarkFolderData else { return nil}
guard targetGUID != treeRootFolder.guid else {return [treeRootNode]}
var path: [BookmarkNodeData]?

guard let children = treeRootFolder.children else {return nil}
for childNode in children {
if let pathToFolder = findPathToFolder(targetGUID: targetGUID, treeRootNode: childNode) {
path = pathToFolder
}
}
guard path != nil else {return nil}
path?.insert(treeRootNode, at: 0)
return path
}

private func makeHistoryCoordinator(navigationController: UINavigationController) {
guard !childCoordinators.contains(where: { $0 is HistoryCoordinator }) else { return }
let router = DefaultRouter(navigationController: navigationController)
Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/Client/Frontend/Browser/BrowserPrompts.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ protocol NewJSPromptAlertControllerDelegate: AnyObject {
class NewJSPromptAlertController: UIAlertController {
var alertInfo: NewJSAlertInfo?
weak var delegate: NewJSPromptAlertControllerDelegate?
private var handledAction: Bool = false
private var handledAction = false
private var dismissalResult: Any?

convenience init(
Expand Down
7 changes: 6 additions & 1 deletion firefox-ios/Client/Frontend/Library/Bookmarks/BookmarksViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ class BookmarksViewController: SiteTableViewController,
setupEmptyStateView()
}

override func viewDidAppear(_ animated: Bool) {
super.viewDidAppear(animated)
profile.prefs.setString(viewModel.bookmarkFolderGUID, forKey: PrefsKeys.LastViewedBookmarkFolder)
}

override func viewWillTransition(to size: CGSize, with coordinator: UIViewControllerTransitionCoordinator) {
super.viewWillTransition(to: size, with: coordinator)

Expand Down Expand Up @@ -453,7 +458,7 @@ class BookmarksViewController: SiteTableViewController,
libraryPanelDelegate?.libraryPanel(didSelectURL: url, visitType: .bookmark)
} else {
guard let folder = bookmarkCell as? FxBookmarkNode else { return }
bookmarkCoordinatorDelegate?.start(from: folder)
bookmarkCoordinatorDelegate?.start(fromGUID: folder.guid, animated: true)
}
}

Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/LegacyBookmarksPanel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ class LegacyBookmarksPanel: SiteTableViewController,
libraryPanelDelegate?.libraryPanel(didSelectURL: url, visitType: .bookmark)
} else {
guard let folder = bookmarkCell as? FxBookmarkNode else { return }
bookmarkCoordinatorDelegate?.start(from: folder)
bookmarkCoordinatorDelegate?.start(fromGUID: folder.guid, animated: true)
}
}

Expand Down
4 changes: 2 additions & 2 deletions .../firefox-ios-tests/Tests/ClientTests/Coordinators/Library/BookmarksCoordinatorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ final class BookmarksCoordinatorTests: XCTestCase {
let subject = createSubject()
let folder = LocalDesktopFolder()

subject.start(from: folder)
subject.start(fromGUID: folder.guid, animated: true)

XCTAssertTrue(router.pushedViewController is LegacyBookmarksPanel)
XCTAssertEqual(router.pushCalled, 1)
Expand Down Expand Up @@ -77,7 +77,7 @@ final class BookmarksCoordinatorTests: XCTestCase {
let subject = createSubject(isBookmarkRefactorEnabled: true)
let folder = LocalDesktopFolder()

subject.start(from: folder)
subject.start(fromGUID: folder.guid, animated: true)

XCTAssertTrue(router.pushedViewController is BookmarksViewController)
XCTAssertEqual(router.pushCalled, 1)
Expand Down