325342 - Allow bookmark folders to be sorted

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brett Wilson]

Implement the "Sort by name" item in the context menu of the places view.

Comment 1

[Brett Wilson]

It might be nice to add the sorting function to the bookmark service so anybody could easily call it.

Comment 3

[Neil Deakin]

Is this command supposed to just change the sort such that the tree is sorted by title, or is it supposed to take the contents of the folder that was context-clicked and sorts the underlying data permanently? 

Comment 4

[Asaf Romano (gone)]

The later.

Comment 5

[Neil Deakin]

Attachment: work in progress

This is a work in progress. It sorts by name, but doesn't update the UI properly. The code in nsNavBookmarks::SetItemIndex needs to be updated to ensure this.

Comment 6

[Neil Deakin]

Attachment: add sort by name function

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 261901 [details] [diff] [review]
add sort by name function

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.141
>diff -u -p -8 -r1.141 controller.js
>--- browser/components/places/content/controller.js	30 Mar 2007 23:16:24 -0000	1.141
>+++ browser/components/places/content/controller.js	18 Apr 2007 00:31:43 -0000
>@@ -200,16 +200,20 @@ 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_sortby:name":

"sortBy" would be more in line with the style here.

>   /**
>+   * Changes the index for a bookmark item.
>+   */
>+  void setItemIndex(in PRInt64 id, in PRInt32 index);
>+

can you add @param documentation for this?

> NS_IMETHODIMP
>+nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
>+{
>+  nsresult rv;
>+  PRInt32 type = 0;
>+  PRInt32 oldIndex = 0;
>+  PRInt64 parent = 0;
>+  PRInt64 folderKey = -1;
>+
>+  mozIStorageConnection *dbConn = DBConn();
>+  nsCOMPtr<nsIURI> uri;
>+  {
>+    nsCOMPtr<mozIStorageStatement> getStatement;
>+    rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT b.type, p.url, b.position, b.parent, b.fk "
>+      "FROM moz_bookmarks b "
>+      "JOIN moz_places p ON b.fk = p.id "
>+      "WHERE b.id = ?1"),
>+    getter_AddRefs(getStatement));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+

can you add the type column to the mDBGetBookmarkProperties statement and use that, instead of creating a new one?

>+
>+  if (type == TYPE_FOLDER) {
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnFolderRemoved(folderKey, parent, oldIndex))
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnFolderAdded(folderKey, parent, aNewIndex))
>+  }
>+  else if (type == TYPE_SEPARATOR) {
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnSeparatorRemoved(parent, oldIndex))
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnSeparatorAdded(parent, aNewIndex))
>+  }
>+  else {
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnItemRemoved(aItemId, uri, parent, oldIndex))
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnItemAdded(aItemId, uri, parent, aNewIndex))
>+  }

yikes, there's got to be a better way. can you file a followup bug for using onItemChanged for this?

Comment 8

[Neil Deakin]

Attachment: address review comments

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 262033 [details] [diff] [review]
address review comments

r=me, please have mano do a pass on this as well. thanks neil!

>   /**
>+   * Changes the index for a bookmark item. This method does not change
>+   * the indicies of any other bookmarks in the same folder.

nit: s/indicies/indices/

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> (From update of attachment 262033 [details] [diff] [review])
> r=me, please have mano do a pass on this as well. thanks neil!
> 
> >   /**
> >+   * Changes the index for a bookmark item. This method does not change
> >+   * the indicies of any other bookmarks in the same folder.
> 
> nit: s/indicies/indices/
> 

another request: please add a warning that calling setItemIndex *will* cause the indexes to be corrupted if not called for each child of a container.

hrm, the whole deal should probably be moved into the bookmarks service at some point, would be safer.

Comment 11

[Neil Deakin]

Attachment: adjust comments

Comment 12

[Asaf Romano (gone)]

Comment on attachment 262037 [details] [diff] [review]
adjust comments

>Index: browser/components/places/content/controller.js
>===================================================================

>@@ -200,16 +200,20 @@ 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_sortBy:name":
>+      if (this._view.selectedNode)
>+        return PlacesUtils.nodeIsFolder(this._view.selectedNode);

This allows sorting a read-only folder (e.g. livemark-containers).

>   onEvent: function PC_onEvent(eventName) { },
>@@ -442,21 +449,21 @@ PlacesController.prototype = {  
>     // be filtered (e.g. left list). 
>     var enoughChildrenToSort = false;
>     if (PlacesUtils.nodeIsFolder(sortFolder)) {
>       var folder = asFolder(sortFolder);
>       var contents = this.getFolderContents(folder.folderId, false, false);
>       enoughChildrenToSort = contents.childCount > 1;
>     }
>     var metadata = this._buildSelectionMetadata();
>-    this._setEnabled("placesCmd_sortby:name", 
>+    this._setEnabled("placesCmd_sortBy:name", 
>       (sortingChildren || !inSysArea) && canInsert && viewIsFolder && 
>       !("mixed" in metadata) && enoughChildrenToSort);
> 
>-    var command = document.getElementById("placesCmd_sortby:name");
>+    var command = document.getElementById("placesCmd_sortBy:name");
>     

just remove this code, it's no longer used.
>   /**
>+   * Sort a folder by name
>+   */

s/ a / the selected/ ?

> /**
>+ * Sort a folder by name
>+ */
>+function PlacesSortFolderByNameTransaction(aFolderId, aFolderIndex) {
>+  this._folderId = aFolderId;
>+  this._folderIndex = aFolderIndex;
>+  this._oldOrder = null,
>+  this.redoTransaction = this.doTransaction;
>+}
>+PlacesSortFolderByNameTransaction.prototype = {
>+  __proto__: PlacesBaseTransaction.prototype,
>+
>+  doTransaction: function PSSFBN_doTransaction() {
>+    this._oldOrder = [];
>+
>+    var items = [];
>+    var contents = PlacesUtils.getFolderContents(this._folderId, false, false);

s/PlacesUtils/this.utils/


>Index: toolkit/components/places/src/nsNavBookmarks.cpp
>===================================================================

>+  if (type == TYPE_FOLDER) {
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnFolderRemoved(folderKey, parent, oldIndex))
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnFolderAdded(folderKey, parent, aNewIndex))
>+  }
>+  else if (type == TYPE_SEPARATOR) {
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnSeparatorRemoved(parent, oldIndex))
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnSeparatorAdded(parent, aNewIndex))
>+  }
>+  else {
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnItemRemoved(aItemId, uri, parent, oldIndex))
>+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                        OnItemAdded(aItemId, uri, parent, aNewIndex))
>+  }
>+
>+  return NS_OK;
>+}

So it would be nice if we could somehow batch the setItemIndex calls and then call invalidateContainer... file a bug please.

Comment 13

[Neil Deakin]

Attachment: address review comments

Comment 14

[Asaf Romano (gone)]

Comment on attachment 262117 [details] [diff] [review]
address review comments

You were looking for PlacesUtils.folderIsReadonly as a general solution.

Either way, nodeIsLivemarkItem checks if whether a node is a _child_ of a live bookmark

Comment 15

[Neil Deakin]

mano, so something like the following:

    case "placesCmd_sortBy:name":
      var selectedNode = this._view.selectedNode;
      return selectedNode &&
             PlacesUtils.nodeIsFolder(selectedNode) &&
             !PlacesUtils.nodeIsReadOnly(selectedNode);

Comment 16

[Asaf Romano (gone)]

Seems right.

Comment 17

[Neil Deakin]

Attachment: check folders properly

Comment 18

[Asaf Romano (gone)]

Comment on attachment 262490 [details] [diff] [review]
check folders properly

r=mano

Comment 19

[Neil Deakin]

The patch in bug 376863 caused this to stop working, because the folders don't set the bookmarkId anymore. So what should I be using instead?

Comment 20

[Asaf Romano (gone)]

folderId for folder (asFolder(node).folderId that is).

Dietrich has a bug on unifying the two, I think.

Comment 21

[Dietrich Ayala (:dietrich)]

(In reply to comment #20)
> folderId for folder (asFolder(node).folderId that is).
> 
> Dietrich has a bug on unifying the two, I think.
> 

neil, you should still use mano's recommendation for getting the folder id. my patch changed the source of the folder ids, but not the accessor.

Comment 22

[Asaf Romano (gone)]

yeah well, i'm planning to merge the two soon enough.

Comment 23

[Asaf Romano (gone)]

...and this is done now, the generic node interface has an itemId attribute.

Comment 24

[Neil Deakin]

Attachment: update for itemid

Comment 25

[Asaf Romano (gone)]

Comment on attachment 264466 [details] [diff] [review]
update for itemid

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================


>   /**
>+   * Changes the index for a bookmark item. This method does not change
>+   * the indices of any other bookmarks in the same folder, so ensure that
>+   * the new index does not already exist, or change the index of other
>+   * bookmarks accordingly, otherwise the bookmark indices will become
>+   * corrupted.
>+   *
>+   *  @param id         The bookmark id to modify
>+   *  @param index      The new index
>+   */
>+  void setItemIndex(in PRInt64 id, in PRInt32 index);
>+

nit: use long long and long here.

r=mano otherwise.

Comment 26

[Asaf Romano (gone)]

nit #2: aItemId and aIndex to match other methods.

you should also remove "bookmark" from the method documentation - this method applies to all item types (bookmarks, folders and separators).

Comment 27

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a7pre) Gecko/2007070504 Minefield/3.0a7pre

Comment 28

[Tracy Walker [:tracy]]

Added test case http://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=4454 to Litmus FFT

Comment 29

[Gervase Markham [:gerv]]

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