Closed Bug 515394 Opened 11 years ago Closed 11 years ago

Allow access to the complete bookmark hierarchy

Categories

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

x86
macOS
defect

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0
Tracking Status
fennec 1.0+ ---

People

(Reporter: mfinkle, Assigned: vingtetun)

References

Details

(Whiteboard: [fennec l10n])

Attachments

(4 files, 3 obsolete files)

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).
tracking-fennec: --- → ?
This will probably require a string for the "Synced Bookmarks" folder
Whiteboard: [fennec l10n]
Priority: -- → P1
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
Attachment #413404 - Flags: review?(gavin.sharp)
Attachment #413404 - Flags: review?(gavin.sharp) → review+
Please, add localization comment on this kind of strings. In this case "desktop" stands for "desktop pc", is it right?
In the desktop Firefox bookmark organizer, we use "All Bookmarks".

Maybe that is better than "Desktop Bookmarks"
How about we add an l10n note that "All Bookmarks" is an acceptable alternative?
tracking-fennec: ? → 1.0-
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.
Mind filing up a follow up bug for that for the next version of fennec?
mockup of what this scheme should look like in fennec
tracking-fennec: 1.0- → 1.0+
Attached patch wip-1 (obsolete) — Splinter Review
First wip, it should do all the works but i need to clean it up a bit
Attached patch Patch (obsolete) — Splinter Review
Attachment #416836 - Attachment is obsolete: true
Attachment #416843 - Flags: review?(gavin.sharp)
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.
Attachment #416843 - Flags: review?(gavin.sharp)
Attached patch Patch v0.2 (obsolete) — Splinter Review
Adressed comments
Attachment #416843 - Attachment is obsolete: true
Attachment #417001 - Flags: review?(gavin.sharp)
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).
Attached patch Patch v0.3Splinter Review
(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.
Attachment #417001 - Attachment is obsolete: true
Attachment #417021 - Flags: review?(gavin.sharp)
Attachment #417001 - Flags: review?(gavin.sharp)
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
Attachment #417021 - Flags: review?(gavin.sharp) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Blocks: 534914
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.
Component: General → Bookmarks
Assignee: 21 → tchung
Assigning to self to create a litmus testcase.
(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.
Assignee: tchung → 21
QA Contact: general → bookmarks
Flags: in-litmus? → in-litmus?(tchung)
litmus test: https://litmus.mozilla.org/show_test.cgi?id=12591
Flags: in-litmus?(tchung) → in-litmus+
You need to log in before you can comment on or make changes to this bug.