Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 23 Aug 2017 12:24:53 +0100
changeset 656399 d78e348d8bb3
parent 656346 04b6be50a252
child 729128 641ca77703de
push id77200
push userbmo:standard8@mozilla.com
push dateThu, 31 Aug 2017 05:56:30 +0000
reviewersmak
bugs1391166
milestone57.0a1
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r?mak MozReview-Commit-ID: 7Adz2Et0NQn
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_controller_onDrop.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1597,20 +1597,37 @@ var PlacesControllerDragHelper = {
         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.
-        let dragginUp = insertionPoint.itemId == unwrapped.parent &&
-                        index < (await PlacesUtils.bookmarks.fetch(unwrapped.itemGuid)).index;
-        if (index != -1 && dragginUp)
-          index += movedCount++;
+        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 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)
             transactions.push(PlacesTransactions.Tag({ uri, tag: tagName }));
           else
@@ -1633,17 +1650,21 @@ var PlacesControllerDragHelper = {
           } else {
             transactions.push(PlacesUIUtils.makeTransaction(unwrapped,
                                 flavor, insertionPoint.itemId,
                                 index, doCopy));
           }
         }
       }
     }
-
+    // Check if we actually have something to add, if we don't it probably wasn't
+    // valid, or it was moving to the same location, so just ignore it.
+    if (!transactions.length) {
+      return;
+    }
     if (PlacesUIUtils.useAsyncTransactions) {
       await PlacesTransactions.batch(transactions);
     } else {
       let txn = new PlacesAggregatedTransaction("DropItems", transactions);
       PlacesUtils.transactionManager.doTransaction(txn);
     }
   },
 
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -23,16 +23,17 @@ support-files =
 [browser_bookmarkProperties_bookmarkAllTabs.js]
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 [browser_bookmarksProperties.js]
 [browser_check_correct_controllers.js]
 [browser_click_bookmarks_on_toolbar.js]
 [browser_cutting_bookmarks.js]
 subsuite = clipboard
+[browser_controller_onDrop.js]
 [browser_copy_folder_tree.js]
 [browser_copy_query_without_tree.js]
 subsuite = clipboard
 [browser_drag_bookmarks_on_toolbar.js]
 [browser_forgetthissite_single.js]
 [browser_history_sidebar_search.js]
 [browser_library_batch_delete.js]
 [browser_library_commands.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_controller_onDrop.js
@@ -0,0 +1,132 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+/* global sinon */
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
+
+const sandbox = sinon.sandbox.create();
+
+var bookmarks;
+var bookmarkIds;
+
+add_task(async function setup() {
+  registerCleanupFunction(async function() {
+    sandbox.restore();
+    delete window.sinon;
+    await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesTestUtils.clearHistory();
+  });
+
+  sandbox.stub(PlacesUIUtils, "getTransactionForData");
+  sandbox.stub(PlacesTransactions, "batch");
+
+  bookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: [{
+      title: "bm1",
+      url: "http://example1.com"
+    }, {
+      title: "bm2",
+      url: "http://example2.com"
+    }, {
+      title: "bm3",
+      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) {
+  // 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,
+      index: insertionIndex,
+      orientation: Ci.nsITreeView.DROP_ON
+    });
+
+    let bookmarkWithId = JSON.stringify(Object.assign({
+      id: bookmarkIds.get(dragBookmark.guid),
+      itemGuid: dragBookmark.guid,
+      parent: PlacesUtils.unfiledBookmarksFolderId,
+    }, dragBookmark));
+
+    let dt = {
+      dropEffect: "move",
+      mozCursor: "auto",
+      mozItemCount: 1,
+      types: [ PlacesUtils.TYPE_X_MOZ_PLACE ],
+      mozTypesAt(i) {
+        return this.types;
+      },
+      mozGetDataAt(i) {
+        return bookmarkWithId;
+      }
+    };
+
+    await PlacesControllerDragHelper.onDrop(ip, dt);
+
+    if (!expectTransactionCreated) {
+      Assert.ok(PlacesUIUtils.getTransactionForData.notCalled,
+        "Should not have created transaction data");
+      return;
+    }
+
+    Assert.ok(PlacesUIUtils.getTransactionForData.calledOnce,
+      "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,
+      "Should have called getTransactionForData with the correct parent guid");
+    Assert.equal(args[3], realInsertionIndex,
+      "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);
+});
+
+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);
+});
+
+add_task(async function test_simple_move_to_same() {
+  // 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);
+});