Bug 426649 - Reordering folders in the left pane of Places Library is inconsistent/broken (for mak77@supereva.it, r=mano, a=mconnor)

browser/components/places/content/controller.js

@@ -77,16 +77,33 @@ const REMOVE_PAGES_MAX_SINGLEREMOVES = 10;
  *          is provided for informational purposes only!
  * @param   [optional] aIsTag
  *          Indicates if parent container is a tag
+ * @param   [optional] aDropNearItemId
+ *          When defined we will calculate index based on this itemId
  * @constructor
  */
-function InsertionPoint(aItemId, aIndex, aOrientation, aIsTag) {
+function InsertionPoint(aItemId, aIndex, aOrientation, aIsTag,
+                        aDropNearItemId) {
   this.itemId = aItemId;
-  this.index = aIndex;
+  this._index = aIndex;
   this.orientation = aOrientation;
   this.isTag = aIsTag;
+  this.dropNearItemId = aDropNearItemId;
 }
-InsertionPoint.prototype.toString = function IP_toString() {
-  return "[object InsertionPoint(folder:" + this.itemId + ",index:" + this.index + ",orientation:" + this.orientation + ",isTag:" + this.isTag + ")]";
+
+InsertionPoint.prototype = {
+  set index(val) {
+    return this._index = val;
+  },
+
+  get index() {
+    if (this.dropNearItemId > 0) {
+      // If dropNearItemId is set up we must calculate the real index of
+      // the item near which we will drop.
+      var index = PlacesUtils.bookmarks.getItemIndex(this.dropNearItemId);
+      return this.orientation == Ci.nsITreeView.DROP_BEFORE ? index : index + 1;
+    }
+    return this._index;
+  }
 };
 
 /**

browser/components/places/content/tree.xml

@@ -507,6 +507,7 @@
           var result = this.getResult();
           var resultview = this.getResultView();
           var container = result.root;
+          var dropNearItemId = -1;
           NS_ASSERT(container, "null container");
           // When there's no selection, assume the container is the container
           // the view is populated from (i.e. the result's itemId).
@@ -520,7 +521,8 @@
             }
             else if (!this._disallowInsertion(lastSelected) &&
                      lastSelected.containerOpen &&
-                     orientation == Ci.nsITreeView.DROP_AFTER) {
+                     orientation == Ci.nsITreeView.DROP_AFTER &&
+                     lastSelected.hasChildren) {
              // If the last selected item is an open container and the user is
              // trying to drag into it as a first item, really insert into it.
              container = lastSelected;
@@ -539,12 +541,19 @@
                 return null;
 
               var queryOptions = asQuery(result.root).queryOptions;
-              if (queryOptions.excludeItems || queryOptions.excludeQueries ||
-                  queryOptions.excludeReadOnlyFolders ||
-                  queryOptions.sortingMode != Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
-                // If we are within either a sorted view or a view in which
-                // some items may be invisible, insert at the end
-                index = -1;   
+              if (queryOptions.sortingMode !=
+                    Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
+                // If we are within a sorted view, insert at the end
+                index = -1;
+              }
+              else if (queryOptions.excludeItems ||
+                       queryOptions.excludeQueries ||
+                       queryOptions.excludeReadOnlyFolders) {
+                // Some item may be invisible, insert near last selected one.
+                // We don't replace index here to avoid requests to the db,
+                // instead it will be calculated later by the controller.
+                index = -1;
+                dropNearItemId = lastSelected.itemId;
               }
               else {
                 var lsi = PlacesUtils.getIndexOfNode(lastSelected);
@@ -558,7 +567,8 @@
 
           return new InsertionPoint(PlacesUtils.getConcreteItemId(container),
                                     index, orientation,
-                                    PlacesUtils.nodeIsTagQuery(container));
+                                    PlacesUtils.nodeIsTagQuery(container),
+                                    dropNearItemId);
         ]]></body>
       </method>
 

browser/components/places/content/treeView.js

@@ -1019,6 +1019,7 @@ PlacesTreeView.prototype = {
 
   _getInsertionPoint: function PTV__getInsertionPoint(index, orientation) {
     var container = this._result.root;
+    var dropNearItemId = -1;
     // When there's no selection, assume the container is the container
     // the view is populated from (i.e. the result's itemId).
     if (index != -1) {
@@ -1031,7 +1032,8 @@ PlacesTreeView.prototype = {
       }
       else if (!this._disallowInsertion(lastSelected) &&
                lastSelected.containerOpen &&
-               orientation == Ci.nsITreeView.DROP_AFTER) {
+               orientation == Ci.nsITreeView.DROP_AFTER &&
+               lastSelected.hasChildren) {
         // If the last selected item is an open container and the user is
         // trying to drag into it as a first item, really insert into it.
         container = lastSelected;
@@ -1049,8 +1051,24 @@ PlacesTreeView.prototype = {
         if (this._disallowInsertion(container))
           return null;
 
-        var lsi = PlacesUtils.getIndexOfNode(lastSelected);
-        index = orientation == Ci.nsITreeView.DROP_BEFORE ? lsi : lsi + 1;
+        var queryOptions = asQuery(this._result.root).queryOptions;
+        if (queryOptions.sortingMode != Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
+          // If we are within a sorted view, insert at the end
+          index = -1;
+        }
+        else if (queryOptions.excludeItems ||
+                 queryOptions.excludeQueries ||
+                 queryOptions.excludeReadOnlyFolders) {
+          // Some item may be invisible, insert near last selected one.
+          // We don't replace index here to avoid requests to the db,
+          // instead it will be calculated later by the controller.
+          index = -1;
+          dropNearItemId = lastSelected.itemId;
+        }
+        else {
+          var lsi = PlacesUtils.getIndexOfNode(lastSelected);
+          index = orientation == Ci.nsITreeView.DROP_BEFORE ? lsi : lsi + 1;
+        }
       }
     }
 
@@ -1059,7 +1077,8 @@ PlacesTreeView.prototype = {
 
     return new InsertionPoint(PlacesUtils.getConcreteItemId(container),
                               index, orientation,
-                              PlacesUtils.nodeIsTagQuery(container));
+                              PlacesUtils.nodeIsTagQuery(container),
+                              dropNearItemId);
   },
 
   drop: function PTV_drop(aRow, aOrientation) {