1380063 - Make a combined local + buffer Mobile Bookmarks folder the default bookmarks view

Status: RESOLVED FIXED

(Firefox for iOS :: Home screen, enhancement)

Description

[Edouard Oger [:eoger]]

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.

Comment 1

[Richard Newman [:rnewman]]

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…)

Comment 2

[Edouard Oger [:eoger]]

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

Comment 3

[:garvan]

Attachment: Pull request

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8893598 [details] [review]
Pull request

Reviewed on GH last week.

Comment 5

[:garvan]

Landed: https://github.com/mozilla-mobile/firefox-ios/commit/529fb6a46103f9a30593145a63be4a06df15bb0d

Comment 6

[Edouard Oger [:eoger]]

Landed (correction): https://github.com/mozilla-mobile/firefox-ios/commit/245b21fa3c90bf845a09ea0d4834ed6f236c7616

Comment 7

[:garvan]

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.

Comment 8

[Edouard Oger [:eoger]]

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

Comment 9

[Richard Newman [:rnewman]]

(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.

Comment 10

[Richard Newman [:rnewman]]

Completed:

https://github.com/mozilla-mobile/firefox-ios/commit/0efc34bdfabaef628eca94e6aa6ed7983bc192b7

Follow-up (blocks this bug) to add automated tests.