Closed Bug 1380063 Opened 2 years ago Closed 2 years ago
Make a combined local + buffer Mobile Bookmarks folder the default bookmarks view
Since we are going to sync the outgoing bookmarks into the "Mobile Bookmarks" folder, it makes sense to make this folder the top-level view.
This is perhaps the most fiddly part of this bug tree. The most future-proof and simplest way to do this work is to union the existing overlay queries and fit it into the existing framework used for the non-fallback UI (the one used when a merge has already occurred). That is: display the remote buffer overlaid on the mirror, and append to that the local overlay. That will give you the contents of the server-side Mobile Bookmarks folder plus any bookmarks created since the last sync. N.B., test the following situations: - Add bookmarks before creating an account. They should be visible. - Sign in to an account with existing bookmarks. Both sets of bookmarks should be visible in the same place. - Adding new local bookmarks after signing in, but now without access to the sync server. The expected behavior is that both sets of bookmarks appear in the same place, even though the new ones haven't synced. - Look inside the Desktop Bookmarks virtual folder. There should be no Mobile Bookmarks folder at any time. - Sign back out of the account. The expected behavior in the current system is that all of your bookmarks disappear -- they're now owned by the account, not your device. (You could sign in to a different account…)
Hardware: Other → All
Copy-pasting some of rnewman's notes: [18:26:27] <rnewman> UnsyncedBookmarksFallbackModelFactory shows Desktop Bookmarks from the buffer, and Mobile Bookmarks from local, with a "Mobile Bookmarks" folder inside "Desktop Bookmarks" containing the server's data [18:27:04] <rnewman> what you're trying to do is alter that factory's behavior to show mobile bookmarks from local _plus_ any mobile bookmarks from the buffer [18:27:22] <rnewman> and then exclude Mobile Bookmarks from the Desktop Bookmarks folder [18:29:42] <rnewman> you can decompose that behavior to be two things: implementing a factory that unions two other factories (two mobile bookmarks factories, one buffer and one local) [18:29:58] <rnewman> and then a filtering factory that excludes a given GUID from its results [18:30:31] <rnewman> this is a little fiddly, and I understand that it will be tempting to say "this is way too complex, there must be a simpler way" [18:31:08] <rnewman> but it turns out that this is the least painful way to back a hierarchical view with a database like ours
Summary: Make Mobile Bookmarks the default bookmarks view → Make a combined local + buffer Mobile Bookmarks folder the default bookmarks view
Comment on attachment 8893598 [details] [review] Pull request Reviewed on GH last week.
Attachment #8893598 - Flags: review?(rnewman) → review+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Re-opening, the root view now can have non-navigable folders. This patch is incomplete. STR: On desktop Fx, create a sub-folder of Mobile Bookmarks and add a bookmark in that folder. On Fx iOS, try to navigate to that new folder. It will display the folder in the tableview as empty.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Probably related problem: deletions are always made against the local factory instead of the buffer one: https://github.com/mozilla-mobile/firefox-ios/blob/245b21fa3c90bf845a09ea0d4834ed6f236c7616/Storage/SQL/SQLiteBookmarksModel.swift#L837
(In reply to Edouard Oger [:eoger] from comment #8) > Probably related problem: deletions are always made against the local > factory instead of the buffer one That's 'cos right now only local items should be deletable; making the deletions work should be in Bug 1387492.
Whiteboard: [MobileCore][BookmarkSync] → [MobileCore][BookmarkSync][Sync Q3 OKR]
Completed: https://github.com/mozilla-mobile/firefox-ios/commit/0efc34bdfabaef628eca94e6aa6ed7983bc192b7 Follow-up (blocks this bug) to add automated tests.
Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.