Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders. r?mak draft
authorMark Banner <standard8@mozilla.com>
Tue, 19 Sep 2017 06:54:16 +0100
changeset 668247 9dc0e920033c8abbb4913e5f2eea79e92b2c3063
parent 668025 47f7b6c64265bc7bdd22eef7ab71abc97cf3f8bf
child 732653 b9d082166c1151079a63748f513abf1ed47ed73c
push id80993
push userbmo:standard8@mozilla.com
push dateThu, 21 Sep 2017 12:20:48 +0000
reviewersmak
bugs1400846
milestone57.0a1
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders. r?mak MozReview-Commit-ID: GjnehR5PumZ
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser_controller_onDrop.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1629,38 +1629,49 @@ var PlacesControllerDragHelper = {
                    title: data.label,
                    type: PlacesUtils.TYPE_X_MOZ_URL}];
       } else
         throw new Error("bogus data was passed as a tab");
 
       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--;
           }
         }
 
         // If dragging over a tag container we should tag the item.
         if (insertionPoint.isTag) {
           let uri = NetUtil.newURI(unwrapped.uri);
           let tagItemId = insertionPoint.itemId;
           if (PlacesUIUtils.useAsyncTransactions)
--- a/browser/components/places/tests/browser/browser_controller_onDrop.js
+++ b/browser/components/places/tests/browser/browser_controller_onDrop.js
@@ -29,49 +29,66 @@ add_task(async function setup() {
       title: "bm1",
       url: "http://example1.com"
     }, {
       title: "bm2",
       url: "http://example2.com"
     }, {
       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"
+      }]
     }]
   });
 
   bookmarkIds = await PlacesUtils.promiseManyItemIds([
     bookmarks[0].guid,
     bookmarks[1].guid,
     bookmarks[2].guid,
   ]);
 });
 
-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();
 
   let dragBookmark = bookmarks[startBookmarkIndex];
 
   await withSidebarTree("bookmarks", async (tree) => {
     tree.selectItems([PlacesUtils.unfiledBookmarksFolderId]);
     PlacesUtils.asContainer(tree.selectedNode).containerOpen = true;
 
     // Simulating a drag-drop with a tree view turns out to be really difficult
     // as you can't get a node for the source/target. Hence, we fake the
     // 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
     });
 
     let bookmarkWithId = JSON.stringify(Object.assign({
       id: bookmarkIds.get(dragBookmark.guid),
       itemGuid: dragBookmark.guid,
       parent: PlacesUtils.unfiledBookmarksFolderId,
@@ -102,36 +119,42 @@ async function run_drag_test(startBookma
       "Should have called getTransactionForData at least once.");
 
     let args = PlacesUIUtils.getTransactionForData.args[0];
 
     Assert.deepEqual(args[0], JSON.parse(bookmarkWithId),
       "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");
   });
 }
 
 add_task(async function test_simple_move_down() {
   // When we move items down the list, we'll get a drag index that is one higher
   // 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);
+});