Closed Bug 370013 Opened 17 years ago Closed 17 years ago

Bookmarks toolbar folder should be a child of the bookmark menu folder

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3

People

(Reporter: asaf, Assigned: dietrich)

References

()

Details

(Whiteboard: [Fx2-parity])

Attachments

(1 file, 5 obsolete files)

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.
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.
Blocks: 370020
Blocks: 370099
Blocks: 371077
Assignee: nobody → dietrich
Whiteboard: [Fx2-parity]
Attached patch patch (obsolete) — Splinter Review
+ 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)
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)
Attached patch patch v2 (combined w/ 370020) (obsolete) — Splinter Review
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)
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-
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.
Attached patch patch v3 (combined w/ 370020) (obsolete) — Splinter Review
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)
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-
Attached patch patch v4 (obsolete) — Splinter Review
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
Attachment #257580 - Flags: review?(mano)
Attachment #257580 - Flags: review?(mano)
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)
Comment on attachment 257593 [details] [diff] [review]
patch v5 - removes livemark checks

r=mano
Attachment #257593 - Flags: review?(mano) → review+
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: 17 years ago
Resolution: --- → FIXED
Attached patch as checked inSplinter Review
Attachment #257593 - Attachment is obsolete: true
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.

Attachment

General

Created:
Updated:
Size: