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

[DanielDervishi]

📜 Tickets

Jira ticket
Github issue

💡 Description

After entering a bookmarks folder in the as you view the bookmarks panel, you can now close the app and will be shown the same folder upon re-entering the bookmarks panel

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[lmarceau]

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

[lmarceau]

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?

[DanielDervishi]

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.

[lmarceau]

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

[lmarceau]

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!

[DanielDervishi]

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

[MattLichtenstein]

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?