Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders. r=mak

MozReview-Commit-ID: GjnehR5PumZ

--HG--
extra : rebase_source : a913bf0584803993f9c5ecd061acdc1b40264254

browser/components/places/content/controller.js

@@ -1634,28 +1634,39 @@ var PlacesControllerDragHelper = {
       for (let unwrapped of nodes) {
         let index = await insertionPoint.getIndex();
 
-        // Adjust insertion index to prevent reversal of dragged items. When you
-        // drag multiple elts upward: need to increment index or each successive
-        // elt will be inserted at the same index, each above the previous.
         if (index != -1 && unwrapped.itemGuid) {
           // Note: we use the parent from the existing bookmark as the sidebar
           // gives us an unwrapped.parent that is actually a query and not the real
           // parent.
           let existingBookmark = await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid);
-          let dragginUp = parentGuid == existingBookmark.parentGuid &&
-                          index < existingBookmark.index;
 
-          if (dragginUp) {
-            index += movedCount++;
-          } else if (PlacesUIUtils.useAsyncTransactions) {
-            if (index == existingBookmark.index) {
-              // We're moving to the same index, so there's nothing for us to do.
-              continue;
+          // If we're dropping on the same folder, then we may need to adjust
+          // the index to insert at the correct place.
+          if (existingBookmark && parentGuid == existingBookmark.parentGuid) {
+            if (PlacesUIUtils.useAsyncTransactions) {
+              if (index < existingBookmark.index) {
+                // When you drag multiple elts upward: need to increment index or
+                // each successive elt will be inserted at the same index, each
+                // above the previous.
+                index += movedCount++;
+              } else if (index > existingBookmark.index) {
+                // If we're dragging down, we need to go one lower to insert at
+                // the real point as moving the element changes the index of
+                // everything below by 1.
+                index--;
+              } else {
+                // This isn't moving so we skip it.
+                continue;
+              }
+            } else {
+              // Sync Transactions. Adjust insertion index to prevent reversal
+              // of dragged items. When you drag multiple elts upward: need to
+              // increment index or each successive elt will be inserted at the
+              // same index, each above the previous.
+              if (index < existingBookmark.index) { // eslint-disable-line no-lonely-if
+                index += movedCount++;
+              }
             }
-            // If we're dragging down, we need to go one lower to insert at
-            // the real point as moving the element changes the index of
-            // everything below by 1.
-            index--;
           }
         }
 

browser/components/places/tests/browser/browser_controller_onDrop.js

@@ -34,6 +34,19 @@ add_task(async function setup() {
     }, {
       title: "bm3",
       url: "http://example3.com"
+    }, {
+      title: "folder1",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      children: [{
+        title: "bm4",
+        url: "http://example1.com"
+      }, {
+        title: "bm5",
+        url: "http://example2.com"
+      }, {
+        title: "bm6",
+        url: "http://example3.com"
+      }]
     }]
   });
 
@@ -44,13 +57,17 @@ add_task(async function setup() {
   ]);
 });
 
-async function run_drag_test(startBookmarkIndex, insertionIndex,
-                             realInsertionIndex, expectTransactionCreated = true) {
+async function run_drag_test(startBookmarkIndex, insertionIndex, newParentGuid,
+                             expectedInsertionIndex, expectTransactionCreated = true) {
   if (!PlacesUIUtils.useAsyncTransactions) {
     Assert.ok(true, "Skipping test as async transactions are turned off");
     return;
   }
 
+  if (!newParentGuid) {
+    newParentGuid = PlacesUtils.bookmarks.unfiledGuid;
+  }
+
   // Reset the stubs so that previous test runs don't count against us.
   PlacesUIUtils.getTransactionForData.reset();
   PlacesTransactions.batch.reset();
@@ -66,7 +83,7 @@ async function run_drag_test(startBookmarkIndex, insertionIndex,
     // insertion point and drag data and call the function direct.
     let ip = new InsertionPoint({
       parentId: await PlacesUtils.promiseItemId(PlacesUtils.bookmarks.unfiledGuid),
-      parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+      parentGuid: newParentGuid,
       index: insertionIndex,
       orientation: Ci.nsITreeView.DROP_ON
     });
@@ -107,9 +124,9 @@ async function run_drag_test(startBookmarkIndex, insertionIndex,
       "Should have called getTransactionForData with the correct unwrapped bookmark");
     Assert.equal(args[1], PlacesUtils.TYPE_X_MOZ_PLACE,
       "Should have called getTransactionForData with the correct flavor");
-    Assert.equal(args[2], PlacesUtils.bookmarks.unfiledGuid,
+    Assert.equal(args[2], newParentGuid,
       "Should have called getTransactionForData with the correct parent guid");
-    Assert.equal(args[3], realInsertionIndex,
+    Assert.equal(args[3], expectedInsertionIndex,
       "Should have called getTransactionForData with the correct index");
     Assert.equal(args[4], false,
       "Should have called getTransactionForData with a move");
@@ -121,17 +138,23 @@ add_task(async function test_simple_move_down() {
   // than where we actually want to insert to - as the item is being moved up,
   // everything shifts down one. Hence the index to pass to the transaction should
   // be one less than the supplied index.
-  await run_drag_test(0, 2, 1);
+  await run_drag_test(0, 2, null, 1);
 });
 
 add_task(async function test_simple_move_up() {
   // When we move items up the list, we want the matching index to be passed to
   // the transaction as there's no changes below the item in the list.
-  await run_drag_test(2, 0, 0);
+  await run_drag_test(2, 0, null, 0);
 });
 
-add_task(async function test_simple_move_to_same() {
+add_task(async function test_simple_move_to_same_index() {
   // If we move to the same index, then we don't expect any transactions to be
   // created.
-  await run_drag_test(1, 1, 1, false);
+  await run_drag_test(1, 1, null, 1, false);
+});
+
+add_task(async function test_simple_move_different_folder() {
+  // When we move items to a different folder, the index should never change.
+  await run_drag_test(0, 2, bookmarks[3].guid, 2);
+  await run_drag_test(2, 0, bookmarks[3].guid, 0);
 });