515394 - Allow access to the complete bookmark hierarchy

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect, P1)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 508705 simplifies the bookmark list in Fennec. The intent is to allow the user to see only "mobile" bookmarks in the initial list.

However, syncing with weave or other tools would bring all (or many) of the bookmarks from desktop Firefox (or other locations). We need a way to access the full bookmark system from the simplified list. Bug 508705 has a basic design:

When synced with weave, there will be hierarchy introduced because it exists in
desktop Firefox.  Ideally, what a user would see post-sync is, first, a list of
their bookmarks created on the mobile device, and then a folder called
something along the lines of "synced bookmarks" which could contain the
hierarchy inherited from the desktop (unsorted, bookmarks menu, and bookmarks
toolbar).

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

This will probably require a string for the "Synced Bookmarks" folder

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (string only)

Patch with the required strings needed to implement this feature. The other root folder strings can come from toolkit:

http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/places/places.properties#1

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed strings:
https://hg.mozilla.org/mobile-browser/rev/79d5f333c64f

Comment 4

[Francesco Lodolo [:flod]]

Please, add localization comment on this kind of strings. In this case "desktop" stands for "desktop pc", is it right?

Comment 5

[Axel Hecht [:Pike]]

More questions in http://groups.google.com/group/mozilla.dev.l10n/msg/8646e0c53c91f3f9

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

In the desktop Firefox bookmark organizer, we use "All Bookmarks".

Maybe that is better than "Desktop Bookmarks"

Comment 7

[:Gavin Sharp [email: [e-mail]]]

How about we add an l10n note that "All Bookmarks" is an acceptable alternative?

Comment 8

[Madhava Enros [:madhava]]

I think I'd prefer "Synced Bookmarks" to "All Bookmarks."  The latter doesn't convey that these new bookmarks you're seeing in your list are the ones from elsewhere, via weave sync.

If these other bookmarks are "All Bookmarks" then why are these other ones outside of the folder?  Also, the text in the awesomescreen is "See all bookmarks," which takes you to the top level of the bookmark list.

Comment 9

[Axel Hecht [:Pike]]

Mind filing up a follow up bug for that for the next version of fennec?

Comment 10

[Madhava Enros [:madhava]]

Attachment: mockup of what this would all look like

mockup of what this scheme should look like in fennec

Comment 11

[Madhava Enros [:madhava]]

Attachment: same, but without accidentally included extras

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-1

First wip, it should do all the works but i need to clean it up a bit

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 416843 [details] [diff] [review]
Patch

There are a bunch of instances of trailing whitespace or extra newlines in this patch, need to be sure to clean those up before landing.

>diff -r b05200e571c8 chrome/content/bindings.xml

>+      <method name="isDesktopFolderEmpty">

>+            query.setFolders(folders, 1);

Marco points out that this should be 3 rather than 1...

>+            rootNode.containerOpen = true;
>+            return !rootNode.childCount;

... and he also suggests closing the container before returning, just to avoid getting notifications and such in the time it takes the object to be GCed.

>+      <method name="_getDesktopChildren">

Make this a field instead? It can even reference this._titles (see below)

>+      <method name="_getParentFolderId">

How about something like this instead?

In the constructor:
this._folderParents = {};
this._folderParents[this._fakeDesktopFolderId] = this.mobileRoot;
this._folderParents[PlacesUtils.bookmarks.unfiledBookmarksFolder] = this._fakeDesktopFolderId;
this._folderParents[PlacesUtils.bookmarksMenuFolderId] = this._fakeDesktopFolderId;
this._folderParents[PlacesUtils.toolbarFolderId] = this._fakeDesktopFolderId;

And then this method should be able to just be:

return this._folderParents[aId] || PlacesUtils.bookmarks.getFolderIdForItem(aId);

This won't catch other "fake" roots, but I could live with not handling that for now, since I don't know of any cases where they would exist.

>+      <method name="_getItemTitle">

Rather than calling getString() for each invocation, how about getting these strings and storing them on an object in the constructor?

this._titles = {};
this._titles[PlacesUtils.bookmarks.bookmarksMenuFolderId] = PlacesUtils.getString("BookmarksMenuFolderTitle");
...

Then this method can just be |return this._titles[aId] || PlacesUtils.bookmarks.getItemTitle(aId);|

>             let childItems = this._getChildren(aRootFolder);
>+
>+            if (aRootFolder == this.mobileRoot && !this.isDesktopFolderEmpty())
>+              fragment.appendChild(this.createItem(this._getDesktopFolder()));
>+            else if (aRootFolder == this._fakeDesktopFolderId)
>+              childItems = this._getDesktopChildren();

Can you re-arrange this code to avoid caling _getChildren in the aRootFolder == this._fakeDesktopFolderId case?

>+      <method name="_getDesktopFolder">

This can be a <field> too:
<field name="_desktopFolder"><!CDATA[({ itemId: ... })]]></field>

This looks good otherwise, but want to take another look once comments are addressed.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Adressed comments

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 417001 [details] [diff] [review]
Patch v0.2

>diff -r 80aad2fc31ba chrome/content/bindings.xml

>+          this._folderTitles = {};
>+          this._folderTitles[this._fakeDesktopFolderId] = Elements.browserBundle.getString("bookmarkList.desktop");
>+          this._folderTitles[PlacesUtils.bookmarksMenuFolderId] = PlacesUtils.getString("BookmarksMenuFolderTitle");
>+          this._folderTitles[PlacesUtils.toolbarFolderId] = PlacesUtils.getString("BookmarksToolbarFolderTitle");

I was about to point out that you missed unfiledBookmarksRoot, but this reveals that fact that we probably only need a single string member for the fakeDesktopFolder - getItemTitle() seems to work just fine for the default roots. Slightly less efficient to retrieve them each time, perhaps, but we're already calling it for most elements so saving three calls here probably isn't worth the code footprint.

>+      <field name="_fakeDesktopFolderId" readonly="true">-1000</field>
>+      <field name="_DesktopFolder" readonly="true">{}</field>

unused :)

>+      <field name="_desktopFolder" readonly="true"><![CDATA[
>+        [{

readonly="" doesn't do anything on fields (only valid for <properties>), so remove these.

This could just be an object directly, since you only ever use _desktopFolder[0] (no need for the array wrapper).

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

(In reply to comment #16)
> (From update of attachment 417001 [details] [diff] [review])
> >diff -r 80aad2fc31ba chrome/content/bindings.xml
> 
> >+          this._folderTitles = {};
> >+          this._folderTitles[this._fakeDesktopFolderId] = Elements.browserBundle.getString("bookmarkList.desktop");
> >+          this._folderTitles[PlacesUtils.bookmarksMenuFolderId] = PlacesUtils.getString("BookmarksMenuFolderTitle");
> >+          this._folderTitles[PlacesUtils.toolbarFolderId] = PlacesUtils.getString("BookmarksToolbarFolderTitle");
> 
> I was about to point out that you missed unfiledBookmarksRoot, but this reveals
> that fact that we probably only need a single string member for the
> fakeDesktopFolder - getItemTitle() seems to work just fine for the default
> roots. Slightly less efficient to retrieve them each time, perhaps, but we're
> already calling it for most elements so saving three calls here probably isn't
> worth the code footprint.

Make sense
> 
> >+      <field name="_fakeDesktopFolderId" readonly="true">-1000</field>
> >+      <field name="_DesktopFolder" readonly="true">{}</field>
> 
> unused :)
> 
> >+      <field name="_desktopFolder" readonly="true"><![CDATA[
> >+        [{
> 
> readonly="" doesn't do anything on fields (only valid for <properties>), so
> remove these.
> 

Adressed.

> This could just be an object directly, since you only ever use
> _desktopFolder[0] (no need for the array wrapper).

If i'm do it this way this._desktopFolder return undefined, that's why i've used an array.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 417021 [details] [diff] [review]
Patch v0.3

I made a few minor tweaks when I pushed this, please double check them just as a sanity check:
https://hg.mozilla.org/mobile-browser/rev/7b6427524b8c

Comment 19

[Aakash Desai [:aakashd]]

This is pretty dang amazing. verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091215 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091215 Firefox/3.7a1pre Fennec/1.0b5


We should get this added to our test suites for Weave.

Comment 20

[Aakash Desai [:aakashd]]

This weave testcase is for tracy, we don't need to add it to our fennec 1.0 test run unless that's how tracy wants to create litmus testcases for weave.

Comment 21

[Tony Chung [:tchung]]

Assigning to self to create a litmus testcase.

Comment 22

[Reed Loden [:reed]]

(In reply to comment #21)
> Assigning to self to create a litmus testcase.

The assignee field is only for the developer who wrote the patch.

Comment 23

[Tony Chung [:tchung]]

litmus test: https://litmus.mozilla.org/show_test.cgi?id=12591