Closed
Bug 515394
Opened 15 years ago
Closed 15 years ago
Allow access to the complete bookmark hierarchy
Categories
(Firefox for Android Graveyard :: Bookmarks, defect, P1)
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)
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Updated•15 years ago
|
tracking-fennec: --- → ?
Reporter | ||
Comment 1•15 years ago
|
||
This will probably require a string for the "Synced Bookmarks" folder
Whiteboard: [fennec l10n]
Reporter | ||
Updated•15 years ago
|
Priority: -- → P1
Reporter | ||
Comment 2•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #413404 -
Flags: review?(gavin.sharp) → review+
Reporter | ||
Comment 3•15 years ago
|
||
pushed strings:
https://hg.mozilla.org/mobile-browser/rev/79d5f333c64f
Comment 4•15 years ago
|
||
Please, add localization comment on this kind of strings. In this case "desktop" stands for "desktop pc", is it right?
Comment 5•15 years ago
|
||
More questions in http://groups.google.com/group/mozilla.dev.l10n/msg/8646e0c53c91f3f9
Reporter | ||
Comment 6•15 years ago
|
||
In the desktop Firefox bookmark organizer, we use "All Bookmarks".
Maybe that is better than "Desktop Bookmarks"
Comment 7•15 years ago
|
||
How about we add an l10n note that "All Bookmarks" is an acceptable alternative?
Updated•15 years ago
|
tracking-fennec: ? → 1.0-
Comment 8•15 years ago
|
||
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•15 years ago
|
||
Mind filing up a follow up bug for that for the next version of fennec?
Comment 10•15 years ago
|
||
mockup of what this scheme should look like in fennec
Comment 11•15 years ago
|
||
Updated•15 years ago
|
tracking-fennec: 1.0- → 1.0+
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → 21
Assignee | ||
Comment 12•15 years ago
|
||
First wip, it should do all the works but i need to clean it up a bit
Assignee | ||
Comment 13•15 years ago
|
||
Attachment #416836 -
Attachment is obsolete: true
Attachment #416843 -
Flags: review?(gavin.sharp)
Comment 14•15 years ago
|
||
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)
Assignee | ||
Comment 15•15 years ago
|
||
Adressed comments
Attachment #416843 -
Attachment is obsolete: true
Attachment #417001 -
Flags: review?(gavin.sharp)
Comment 16•15 years ago
|
||
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).
Assignee | ||
Comment 17•15 years ago
|
||
(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 18•15 years ago
|
||
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+
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
Comment 19•15 years ago
|
||
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?
Comment 20•15 years ago
|
||
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.
Updated•15 years ago
|
Component: General → Bookmarks
Updated•14 years ago
|
Assignee: 21 → tchung
Comment 21•14 years ago
|
||
Assigning to self to create a litmus testcase.
Comment 22•14 years ago
|
||
(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
Updated•14 years ago
|
Flags: in-litmus? → in-litmus?(tchung)
Comment 23•14 years ago
|
||
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.
Description
•