Closed
Bug 370013
Opened 18 years ago
Closed 18 years ago
Bookmarks toolbar folder should be a child of the bookmark menu folder
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3
People
(Reporter: asaf, Assigned: dietrich)
References
()
Details
(Whiteboard: [Fx2-parity])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Fx2-parity: the bookmarks toolbar folder should be a child of the bookmark menu folder.
We should also rename the Bookmarks Menu folder back to just Bookmarks.
Comment 1•18 years ago
|
||
this came up at the last meeting in #places:
http://wiki.mozilla.org/Places/StatusMeetings/2007-02-08
[5:30pm] Mano: [17:49] by the way,
[5:30pm] Mano: [17:50] we need to make the toolbar folder a child of the bookmark menu folder
[5:30pm] Mano: [17:50] for Fx2-parity.
[5:30pm] mconnor: [17:57] so it shows up in the bookmark menu?
[5:30pm] Mano: [17:58] right
[5:30pm] Mano: [18:01] and at the same point rename the bookmarks menu folder back to Bookmarks.
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → dietrich
Reporter | ||
Updated•18 years ago
|
Whiteboard: [Fx2-parity]
Assignee | ||
Comment 2•18 years ago
|
||
+ move toolbar under menu in default_places.html
+ change "Bookmarks Menu" to "Bookmarks"
+ change "Bookmarks Toolbar" to "Bookmarks Toolbar Folder"
This patch also updates the bookmarks xpcshell tests to be a bit more tolerant of changes to the default bookmarks structure.
Attachment #256927 -
Flags: review?(mano)
Assignee | ||
Comment 3•18 years ago
|
||
Comment on attachment 256927 [details] [diff] [review]
patch
rethinking this
dietrich: ManoLaptop: what change to CreateRoot for 370013?
[2:13pm] ManoLaptop: dietrich: CreateRoots iirc
[2:13pm] dietrich: InitRoots, CreateRoot
[2:13pm] ManoLaptop: it creates the toolbar folder as a root folder
[2:14pm] dietrich: oh, so you mean i should set the parent-child relationship in there?
[2:14pm] philor joined the chat room.
[2:15pm] ManoLaptop: dietrich: does it make sense to have it listed there, at all?
[2:15pm] ManoLaptop: it's no longer a root folder
[2:15pm] dietrich: really, after 370013 and 370020 the BTF isn't a "root" anymore
[2:15pm] dietrich: yes
[2:15pm] dietrich: er, i mean, yes i agree. no it's not a root.
[2:16pm] ManoLaptop: dietrich: with 370020 it could be a root
[2:16pm] dietrich: it's "special", regardless
[2:16pm] dietrich: hm, yes, then we'd have one root pointing at another
[2:17pm] ManoLaptop: anyway, shouldn't you simply remove this from InitRoots?
[2:17pm] ManoLaptop: that may have some side-effects, i guess
[2:17pm] dietrich: an alternative to making it a root could be to add a col onto moz_bookmarks folders
[2:19pm] dietrich: ManoLaptop: i'll clear review request and think about how to do this in a clearer way
Attachment #256927 -
Flags: review?(mano)
Assignee | ||
Comment 4•18 years ago
|
||
The toolbar is no longer a "root". Toolbar-ness is specified via the |type| column in moz_bookmarks_folders table.
This patch also contains the fix for 370020, and a fix for a problem where if you hide the bookmarks toolbar and restart, when you try to show it again, the contents of the toolbar are all in the chevron. Let me know if you want me to separate those parts out into separate patches.
Attachment #256927 -
Attachment is obsolete: true
Attachment #257172 -
Flags: review?(mano)
Reporter | ||
Comment 5•18 years ago
|
||
Comment on attachment 257172 [details] [diff] [review]
patch v2 (combined w/ 370020)
>Index: browser/base/content/browser.js
>===================================================================
>
>+#ifdef MOZ_PLACES_BOOKMARKS
>+/**
>+ * Initialize the bookmarks toolbar
>+ */
>+function initBookmarksToolbar() {
>+ var bt = document.getElementById("personal-bookmarks");
>+ if (!bt)
>+ return;
nit: get bookmarksBarContent instead and use that when setting the place property.
>+ //bt.addEventListener("DOMAttrModified", , false);
?
>+ // create a query
>+ var history = PlacesUtils.history;
>+ var options = history.getNewQueryOptions();
>+ options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
>+ var query = history.getNewQuery();
>+ query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);
Hrm, maybe add something like getQueryForFolder(aFolderId) to places utils? we've this code pattern in few places.
>+ // serialize the query and init the toolbar
>+ var place = history.queriesToQueryString([query], 1, options);
>+ document.getElementById("bookmarksBarContent").place = place || null;
why || null?
>Index: browser/components/places/content/controller.js
===================================================================
>@@ -276,16 +276,22 @@ PlacesController.prototype = {
> // children of a live bookmark (legacy bookmarks UI doesn't support
> // this)
> if (PlacesUtils.nodeIsURI() &&
> PlacesUtils.nodeIsLivemarkItem(selectedNode))
> return true;
> #endif
> }
> return false;
>+ case "placesCmd_personalToolbarFolder":
placesCmd_setAsToolbarFolder?
>+ if (this._view.hasSingleSelection) {
>+ var selectedNode = this._view.selectedNode;
>+ if (PlacesUtils.nodeIsFolder(selectedNode))
>+ return true;
>+ }
Fx2 disables the command when the current bookmarks toolbar folder is selected.
>
> /**
>+ * Makes the selected node the bookmarks toolbar folder.
>+ */
>+ setPersonalToolbarFolder: function PC_setPersonalToolbarFolder() {
>+ if (!this._view.hasSingleSelection)
>+ return false;
>+ var selectedNode = this._view.selectedNode;
>+ PlacesUtils.bookmarks.toolbarFolder = selectedNode.folderId;
>+ },
no transaction?
Attachment #257172 -
Flags: review?(mano) → review-
Reporter | ||
Comment 6•18 years ago
|
||
Comment on attachment 257172 [details] [diff] [review]
patch v2 (combined w/ 370020)
>Index: toolkit/components/places/src/nsNavBookmarks.cpp
>===================================================================
>+NS_IMETHODIMP
>+nsNavBookmarks::SetToolbarFolder(PRInt64 aFolderId)
>+{
>+ mozIStorageConnection *dbConn = DBConn();
>+ mozStorageTransaction transaction(dbConn, PR_FALSE);
>+
>+ // XXX - validate that input is an int and a valid folder id
>+
PRInt64 is always an int ;)
>+ // unset old toolbar folder
>+ nsCAutoString buffer;
>+ buffer.AssignLiteral("UPDATE moz_bookmarks_folders SET type = ''");
>+ buffer.AppendLiteral("WHERE id = ");
Initialize the string with both?
>Index: toolkit/components/places/src/nsNavHistory.cpp
>===================================================================
> sMenuRootAtom = NS_NewAtom("menu-root");
>- sToolbarRootAtom = NS_NewAtom("toolbar-root");
>+ sToolbarFolderAtom = NS_NewAtom("toolbar-root");
nit: it's no longer a root.
Assignee | ||
Comment 7•18 years ago
|
||
Addresses mano's comments.
Also, fixed livemark container identification to compare against it's iid instead of just checking if the type field was empty.
Attachment #257172 -
Attachment is obsolete: true
Attachment #257553 -
Flags: review?(mano)
Reporter | ||
Comment 8•18 years ago
|
||
Comment on attachment 257553 [details] [diff] [review]
patch v3 (combined w/ 370020)
>Index: browser/base/content/browser.js
>===================================================================
>+function initBookmarksToolbar() {
>+ var bt = document.getElementById("bookmarksBarContent");
>+ if (!bt)
>+ return;
>+ var query =
>+ PlacesUtils.getQueryStringForFolder(PlacesUtils.bookmarks.toolbarFolder);
>+ bt.place = query || null;
>+}
>+#endif
>+
|bt.place = query;| covers both cases ;)
>Index: browser/components/places/content/controller.js
>===================================================================
> /**
>+ * Makes the selected node the bookmarks toolbar folder.
>+ */
>+ setBookmarksToolbarFolder: function PC_setBookmarksToolbarFolder() {
>+ if (!this._view.hasSingleSelection)
>+ return false;
>+ var selectedNode = this._view.selectedNode;
>+ var txn = new PlacesAggregateTransaction("SetBookmarksToolbar", [
>+ new PlacesSetBookmarksToolbarTransaction(selectedNode.folderId)
>+ ]);
You shouldn't aggregate one transaction.
>Index: browser/components/places/content/toolbar.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v
>retrieving revision 1.74
>diff -u -8 -p -r1.74 toolbar.xml
>--- browser/components/places/content/toolbar.xml 6 Mar 2007 19:41:15 -0000 1.74
>+++ browser/components/places/content/toolbar.xml 6 Mar 2007 20:14:09 -0000
>@@ -355,21 +355,28 @@
> <setter><![CDATA[
> this.setAttribute("place", val);
>
> var history = PlacesUtils.history;
> var queries = { }, options = { };
> history.queryStringToQueries(val, queries, { }, options);
> if (!queries.value.length)
> queries.value = [history.getNewQuery()];
>- this._result =
>- history.executeQueries(queries.value, queries.value.length,
>- options.value);
>- this._result.root.containerOpen = true;
>- this._rebuild();
>+ try {
>+ this._result =
>+ history.executeQueries(queries.value, queries.value.length,
>+ options.value);
>+ this._result.root.containerOpen = true;
>+ this._rebuild();
>+ }
>+ catch(ex) {
>+ // Invalid query, or had no results.
>+ // This is valid, eg: user deletes their bookmarks toolbar folder.
>+ return null;
>+ }
> return val;
> ]]></setter>
> </property>
Setters should either return the value set or throw (as in, make the catch block a no-op, I guess).
>Index: browser/components/places/content/utils.js
>===================================================================
>+ getQueryStringForFolder: function PU_getQueryStringForFolder(aFolderId) {
>+ var options = this.history.getNewQueryOptions();
>+ options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
>+ var query = this.history.getNewQuery();
>+ query.setFolders([aFolderId], 1);
>+ return this.history.queriesToQueryString([query], 1, options);
> }
documentation?
> nsNavBookmarks::GetFolderReadonly(PRInt64 aFolder, PRBool *aResult)
> {
> // Ask the folder's nsIRemoteContainer for the readonly property.
> *aResult = PR_FALSE;
> nsCAutoString type;
> nsresult rv = GetFolderType(aFolder, type);
> NS_ENSURE_SUCCESS(rv, rv);
>
>- if (!type.IsEmpty()) {
>+ if (type.Equals(NS_LIVEMARKSERVICE_CONTRACTID)) {
> nsCOMPtr<nsIRemoteContainer> container =
> do_GetService(type.get(), &rv);
> if (NS_SUCCEEDED(rv)) {
> rv = container->GetChildrenReadOnly(aResult);
> NS_ENSURE_SUCCESS(rv, rv);
> }
> }
Why?
Attachment #257553 -
Flags: review?(mano) → review-
Assignee | ||
Comment 9•18 years ago
|
||
Addresses mano's comments.
> > nsNavBookmarks::GetFolderReadonly(PRInt64 aFolder, PRBool *aResult)
> > {
> > // Ask the folder's nsIRemoteContainer for the readonly property.
> > *aResult = PR_FALSE;
> > nsCAutoString type;
> > nsresult rv = GetFolderType(aFolder, type);
> > NS_ENSURE_SUCCESS(rv, rv);
> >
> >- if (!type.IsEmpty()) {
> >+ if (type.Equals(NS_LIVEMARKSERVICE_CONTRACTID)) {
> > nsCOMPtr<nsIRemoteContainer> container =
> > do_GetService(type.get(), &rv);
> > if (NS_SUCCEEDED(rv)) {
> > rv = container->GetChildrenReadOnly(aResult);
> > NS_ENSURE_SUCCESS(rv, rv);
> > }
> > }
>
> Why?
>
That test is no longer valid, given that the toolbar uses the type column to store it's type as well.
Attachment #257553 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #257580 -
Flags: review?(mano)
Reporter | ||
Updated•18 years ago
|
Attachment #257580 -
Flags: review?(mano)
Assignee | ||
Comment 10•18 years ago
|
||
removes livemark checks per IRC conversation.
[4:13pm] ManoLaptop: dietrich: shouldn't we just avoid that check then?
[4:13pm] dietrich: ManoLaptop: the livemark one?
[4:13pm] ManoLaptop: dietrich: this makes GetFolderReadonly not work for other remote containers
[4:13pm] ManoLaptop: yes
[4:14pm] ManoLaptop: queryinterface will fail anyway
[4:15pm] ManoLaptop: dietrich: actualy, you shouldn't change it at all
[4:15pm] ManoLaptop: we may still have an empty type column
[4:16pm] ManoLaptop: another option: check the node type
[4:16pm] ManoLaptop: there's probably no easy way to do so without a resultnode though
[4:17pm] dietrich: ManoLaptop: don't know what you mean by "we may still have an empty column"
[4:17pm] ManoLaptop: dietrich: empty string in the type column, that is
[4:19pm] ManoLaptop: but anyway, if the the type string isn't empty and you've no way to check whether the given Id points to a remote container, letting GetService fail is the ay to go
Attachment #257580 -
Attachment is obsolete: true
Attachment #257593 -
Flags: review?(mano)
Reporter | ||
Comment 11•18 years ago
|
||
Comment on attachment 257593 [details] [diff] [review]
patch v5 - removes livemark checks
r=mano
Attachment #257593 -
Flags: review?(mano) → review+
Assignee | ||
Comment 12•18 years ago
|
||
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js
new revision: 1.769; previous revision: 1.768
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v <-- browser.xul
new revision: 1.338; previous revision: 1.337
done
Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v <-- nsIEProfileMigrator.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in browser/components/migration/src/nsOperaProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp,v <-- nsOperaProfileMigrator.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in browser/components/migration/src/nsSafariProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp,v <-- nsSafariProfileMigrator.cpp
new revision: 1.37; previous revision: 1.36
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v <-- controller.js
new revision: 1.133; previous revision: 1.132
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v <-- places.xul
new revision: 1.64; previous revision: 1.63
done
Checking in browser/components/places/content/placesOverlay.xul;
/cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v <-- placesOverlay.xul
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v <-- toolbar.xml
new revision: 1.75; previous revision: 1.74
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v <-- utils.js
new revision: 1.19; previous revision: 1.18
done
Checking in browser/components/places/tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/bookmarks/test_bookmarks.js,v <-- test_bookmarks.js
new revision: 1.4; previous revision: 1.3
done
Checking in browser/locales/en-US/chrome/browser/places/default_places.html;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/default_places.html,v <-- default_places.html
new revision: 1.8; previous revision: 1.7
done
Checking in browser/locales/en-US/chrome/browser/places/places.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v <-- places.dtd
new revision: 1.19; previous revision: 1.18
done
Checking in extensions/metrics/src/nsProfileCollector.cpp;
/cvsroot/mozilla/extensions/metrics/src/nsProfileCollector.cpp,v <-- nsProfileCollector.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/places/public/nsINavBookmarksService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v <-- nsINavBookmarksService.idl
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/components/places/src/nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v <-- nsBookmarksHTML.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavBookmarks.cpp
new revision: 1.71; previous revision: 1.70
done
Checking in toolkit/components/places/src/nsNavBookmarks.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v <-- nsNavBookmarks.h
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp
new revision: 1.107; previous revision: 1.106
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h
new revision: 1.70; previous revision: 1.69
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- nsNavHistoryResult.cpp
new revision: 1.81; previous revision: 1.80
done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•18 years ago
|
||
Attachment #257593 -
Attachment is obsolete: true
Comment 14•15 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•