Bug 1410940 - Unify the parts of onDrop and paste that get the transaction information. r?mak draft
authorMark Banner <standard8@mozilla.com>
Mon, 23 Oct 2017 18:18:00 +0100
changeset 692672 01d69fbad9fe0878fcdfeb307a701e4d9d206b53
parent 692671 24b8a93f00c7895493fc35979676ee66965d9064
child 738811 5b23fe8490038fe561da35b63f946db01c3dc76e
push id87562
push userbmo:standard8@mozilla.com
push dateFri, 03 Nov 2017 11:18:20 +0000
reviewersmak
bugs1410940
milestone58.0a1
Bug 1410940 - Unify the parts of onDrop and paste that get the transaction information. r?mak This also fixes inserting bookmarks on paste at the right insertion points. MozReview-Commit-ID: Km93oZt1UHm
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser_controller_onDrop.js
browser/components/places/tests/browser/browser_paste_bookmarks.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1313,45 +1313,29 @@ PlacesController.prototype = {
     }
 
     let itemsToSelect = [];
     if (PlacesUIUtils.useAsyncTransactions) {
       if (ip.isTag) {
         let urls = items.filter(item => "uri" in item).map(item => Services.io.newURI(item.uri));
         await PlacesTransactions.Tag({ urls, tag: ip.tagName }).transact();
       } else {
-        let transactionData = [];
+        let transactions = [];
 
         let insertionIndex = await ip.getIndex();
-        let parent = ip.guid;
-
-        for (let item of items) {
-          let doCopy = action == "copy";
-
-          // If this is not a copy, check for safety that we can move the
-          // source, otherwise report an error and fallback to a copy.
-          if (!doCopy &&
-              !PlacesControllerDragHelper.canMoveUnwrappedNode(item)) {
-            Components.utils.reportError("Tried to move an unmovable " +
-                           "Places node, reverting to a copy operation.");
-            doCopy = true;
-          }
-
-          transactionData.push(PlacesUIUtils.getTransactionForData(
-            item, type, parent, insertionIndex, doCopy));
-
-          // Adjust index to make sure items are pasted in the correct
-          // position.  If index is DEFAULT_INDEX, items are just appended.
-          if (insertionIndex != PlacesUtils.bookmarks.DEFAULT_INDEX)
-            insertionIndex++;
+        let doCopy = action == "copy";
+        let newTransactions = await getTransactionsForTransferItems(type,
+          items, insertionIndex, ip.guid, doCopy);
+        if (newTransactions.length) {
+          transactions = [...transactions, ...newTransactions];
         }
 
-        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactionData.length, async () => {
+        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
           await PlacesTransactions.batch(async () => {
-            for (let transaction of transactionData) {
+            for (let transaction of transactions) {
               let guid = await transaction.transact();
               itemsToSelect.push(await PlacesUtils.promiseItemId(guid));
             }
           });
         });
       }
     } else {
       let transactions = [];
@@ -1629,17 +1613,16 @@ var PlacesControllerDragHelper = {
    *                                dropped to. This allows batching to take
    *                                place.
    */
   async onDrop(insertionPoint, dt, view) {
     let doCopy = ["copy", "link"].includes(dt.dropEffect);
 
     let transactions = [];
     let dropCount = dt.mozItemCount;
-    let movedCount = 0;
     let parentGuid = insertionPoint.guid;
     let tagName = insertionPoint.tagName;
 
     // Following flavors may contain duplicated data.
     let duplicable = new Map();
     duplicable.set(PlacesUtils.TYPE_UNICODE, new Set());
     duplicable.set(PlacesUtils.TYPE_X_MOZ_URL, new Set());
 
@@ -1677,61 +1660,25 @@ var PlacesControllerDragHelper = {
         throw new Error("bogus data was passed as a tab");
 
       if (PlacesUIUtils.useAsyncTransactions) {
         // If dragging over a tag container we should tag the item.
         if (insertionPoint.isTag) {
           let urls = nodes.filter(item => "uri" in item).map(item => item.uri);
           transactions.push(PlacesTransactions.Tag({ urls, tag: tagName }));
         } else {
-          for (let unwrapped of nodes) {
-            let index = await insertionPoint.getIndex();
-
-            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);
-
-              // 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 (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;
-                }
-              }
-            }
-
-            // If this is not a copy, check for safety that we can move the
-            // source, otherwise report an error and fallback to a copy.
-            if (!doCopy && !PlacesControllerDragHelper.canMoveUnwrappedNode(unwrapped)) {
-              Components.utils.reportError("Tried to move an unmovable Places " +
-                                           "node, reverting to a copy operation.");
-              doCopy = true;
-            }
-            transactions.push(
-              PlacesUIUtils.getTransactionForData(unwrapped,
-                                                  flavor,
-                                                  parentGuid,
-                                                  index,
-                                                  doCopy));
+          let insertionIndex = await insertionPoint.getIndex();
+          let newTransactions = await getTransactionsForTransferItems(flavor,
+            nodes, insertionIndex, parentGuid, doCopy);
+          if (newTransactions.length) {
+            transactions = [...transactions, ...newTransactions];
           }
         }
       } else {
+        let movedCount = 0;
         for (let unwrapped of nodes) {
           let index = await insertionPoint.getIndex();
 
           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);
@@ -1857,8 +1804,69 @@ function doGetPlacesControllerForCommand
   return null;
 }
 
 function goDoPlacesCommand(aCommand) {
   let controller = doGetPlacesControllerForCommand(aCommand);
   if (controller && controller.isCommandEnabled(aCommand))
     controller.doCommand(aCommand);
 }
+
+/**
+ * Processes a set of transfer items and returns transactions to insert or
+ * move them.
+ *
+ * @param {String} dataFlavor The transfer flavor for the items.
+ * @param {Array} items A list of unwrapped nodes to get transactions for.
+ * @param {Integer} insertionIndex The requested index for insertion.
+ * @param {String} insertionParentGuid The guid of the parent folder to insert
+ *                                     or move the items to.
+ * @param {Boolean} doCopy Set to true to copy the items, false will move them
+ *                         if possible.
+ * @return {Array} Returns an array of created PlacesTransactions.
+ */
+async function getTransactionsForTransferItems(dataFlavor, items, insertionIndex,
+                                               insertionParentGuid, doCopy) {
+  let transactions = [];
+  let index = insertionIndex;
+
+  for (let item of items) {
+    if (index != -1 && item.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(item.itemGuid);
+
+      // If we're dropping on the same folder, then we may need to adjust
+      // the index to insert at the correct place.
+      if (existingBookmark && insertionParentGuid == existingBookmark.parentGuid) {
+        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 if (index == existingBookmark.index) {
+          // This isn't moving so we skip it.
+          continue;
+        }
+      }
+    }
+
+    // If this is not a copy, check for safety that we can move the
+    // source, otherwise report an error and fallback to a copy.
+    if (!doCopy && !PlacesControllerDragHelper.canMoveUnwrappedNode(item)) {
+      Components.utils.reportError("Tried to move an unmovable Places " +
+                                   "node, reverting to a copy operation.");
+      doCopy = true;
+    }
+    transactions.push(
+      PlacesUIUtils.getTransactionForData(item,
+                                          dataFlavor,
+                                          insertionParentGuid,
+                                          index,
+                                          doCopy));
+
+    if (index != -1 && item.itemGuid) {
+      index++;
+    }
+  }
+  return transactions;
+}
--- a/browser/components/places/tests/browser/browser_controller_onDrop.js
+++ b/browser/components/places/tests/browser/browser_controller_onDrop.js
@@ -148,13 +148,21 @@ add_task(async function test_simple_move
 });
 
 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, 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);
+add_task(async function test_simple_move_different_folder_append() {
+  // When we move items to a different folder, the insertion index will be -1
+  // and shouldn't change.
+  await run_drag_test(0, -1, bookmarks[3].guid, -1);
+  await run_drag_test(2, -1, bookmarks[3].guid, -1);
 });
+
+add_task(async function test_move_different_folder_insert_at() {
+  // When we move items to a different folder, the insertion index will be -1
+  // and shouldn't change.
+  await run_drag_test(0, 0, bookmarks[3].guid, 0);
+  await run_drag_test(2, 2, bookmarks[3].guid, 2);
+});
--- a/browser/components/places/tests/browser/browser_paste_bookmarks.js
+++ b/browser/components/places/tests/browser/browser_paste_bookmarks.js
@@ -1,52 +1,50 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 const TEST_URL = "http://example.com/";
-const TEST_URL1 = "https://example.com/otherbrowser";
+const TEST_URL1 = "https://example.com/otherbrowser/";
 
 var PlacesOrganizer;
 var ContentTree;
-var bookmark;
-var bookmarkId;
 
 add_task(async function setup() {
   await PlacesUtils.bookmarks.eraseEverything();
   let organizer = await promiseLibrary();
 
   registerCleanupFunction(async function() {
     await promiseLibraryClosed(organizer);
     await PlacesUtils.bookmarks.eraseEverything();
   });
 
   PlacesOrganizer = organizer.PlacesOrganizer;
   ContentTree = organizer.ContentTree;
+});
 
+add_task(async function paste() {
   info("Selecting BookmarksToolbar in the left pane");
   PlacesOrganizer.selectLeftPaneQuery("BookmarksToolbar");
 
-  bookmark = await PlacesUtils.bookmarks.insert({
+  let bookmark = await PlacesUtils.bookmarks.insert({
     parentGuid: PlacesUtils.bookmarks.toolbarGuid,
     url: TEST_URL,
     title: "0"
   });
-  bookmarkId = await PlacesUtils.promiseItemId(bookmark.guid);
+  let bookmarkId = await PlacesUtils.promiseItemId(bookmark.guid);
 
   ContentTree.view.selectItems([bookmarkId]);
 
   await promiseClipboard(() => {
-    info("Copying selection");
+    info("Cutting selection");
     ContentTree.view.controller.cut();
   }, PlacesUtils.TYPE_X_MOZ_PLACE);
-});
 
-add_task(async function paste() {
   info("Selecting UnfiledBookmarks in the left pane");
   PlacesOrganizer.selectLeftPaneQuery("UnfiledBookmarks");
 
   info("Pasting clipboard");
   await ContentTree.view.controller.paste();
 
   let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
 
@@ -55,16 +53,174 @@ add_task(async function paste() {
   Assert.equal(tree.children[0].title, "0",
                "Should have the correct title");
   Assert.equal(tree.children[0].uri, TEST_URL,
                "Should have the correct URL");
 
   await PlacesUtils.bookmarks.remove(tree.children[0].guid);
 });
 
+
+add_task(async function paste_check_indexes() {
+  info("Selecting BookmarksToolbar in the left pane");
+  PlacesOrganizer.selectLeftPaneQuery("BookmarksToolbar");
+
+  let copyChildren = [];
+  let targetChildren = [];
+  for (let i = 0; i < 10; i++) {
+    copyChildren.push({
+      url: `${TEST_URL}${i}`,
+      title: `Copy ${i}`
+    });
+    targetChildren.push({
+      url: `${TEST_URL1}${i}`,
+      title: `Target ${i}`
+    });
+  }
+
+  let copyBookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    children: copyChildren
+  });
+
+  let targetBookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: targetChildren
+  });
+
+  let bookmarkIds = await PlacesUtils.promiseManyItemIds([
+    copyBookmarks[0].guid,
+    copyBookmarks[3].guid,
+    copyBookmarks[6].guid,
+    copyBookmarks[9].guid
+  ]);
+
+  ContentTree.view.selectItems([
+    bookmarkIds.get(copyBookmarks[0].guid),
+    bookmarkIds.get(copyBookmarks[3].guid),
+    bookmarkIds.get(copyBookmarks[6].guid),
+    bookmarkIds.get(copyBookmarks[9].guid),
+  ]);
+
+  await promiseClipboard(() => {
+    info("Cutting multiple selection");
+    ContentTree.view.controller.cut();
+  }, PlacesUtils.TYPE_X_MOZ_PLACE);
+
+  info("Selecting UnfiledBookmarks in the left pane");
+  PlacesOrganizer.selectLeftPaneQuery("UnfiledBookmarks");
+
+  let insertionBookmarkId = await PlacesUtils.promiseItemId(targetBookmarks[4].guid);
+
+  ContentTree.view.selectItems([insertionBookmarkId]);
+
+  info("Pasting clipboard");
+  await ContentTree.view.controller.paste();
+
+  let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
+
+  const expectedBookmarkOrder = [
+    targetBookmarks[0].guid,
+    targetBookmarks[1].guid,
+    targetBookmarks[2].guid,
+    targetBookmarks[3].guid,
+    copyBookmarks[0].guid,
+    copyBookmarks[3].guid,
+    copyBookmarks[6].guid,
+    copyBookmarks[9].guid,
+    targetBookmarks[4].guid,
+    targetBookmarks[5].guid,
+    targetBookmarks[6].guid,
+    targetBookmarks[7].guid,
+    targetBookmarks[8].guid,
+    targetBookmarks[9].guid,
+  ];
+
+  Assert.equal(tree.children.length, expectedBookmarkOrder.length,
+               "Should be the expected amount of bookmarks in the unfiled folder.");
+
+  for (let i = 0; i < expectedBookmarkOrder.length; ++i) {
+    Assert.equal(tree.children[i].guid, expectedBookmarkOrder[i],
+                 `Should be the expected item at index ${i}`);
+  }
+
+  await PlacesUtils.bookmarks.eraseEverything();
+});
+
+add_task(async function paste_check_indexes_same_folder() {
+  info("Selecting BookmarksToolbar in the left pane");
+  PlacesOrganizer.selectLeftPaneQuery("BookmarksToolbar");
+
+  let copyChildren = [];
+  for (let i = 0; i < 10; i++) {
+    copyChildren.push({
+      url: `${TEST_URL}${i}`,
+      title: `Copy ${i}`
+    });
+  }
+
+  let copyBookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    children: copyChildren
+  });
+
+  let bookmarkIds = await PlacesUtils.promiseManyItemIds([
+    copyBookmarks[0].guid,
+    copyBookmarks[3].guid,
+    copyBookmarks[6].guid,
+    copyBookmarks[9].guid
+  ]);
+
+  ContentTree.view.selectItems([
+    bookmarkIds.get(copyBookmarks[0].guid),
+    bookmarkIds.get(copyBookmarks[3].guid),
+    bookmarkIds.get(copyBookmarks[6].guid),
+    bookmarkIds.get(copyBookmarks[9].guid),
+  ]);
+
+  await promiseClipboard(() => {
+    info("Cutting multiple selection");
+    ContentTree.view.controller.cut();
+  }, PlacesUtils.TYPE_X_MOZ_PLACE);
+
+  let insertionBookmarkId = await PlacesUtils.promiseItemId(copyBookmarks[4].guid);
+
+  ContentTree.view.selectItems([insertionBookmarkId]);
+
+  info("Pasting clipboard");
+  await ContentTree.view.controller.paste();
+
+  let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.toolbarGuid);
+
+  // Although we've inserted at index 4, we've taken out two items below it, so
+  // we effectively insert after the third item.
+  const expectedBookmarkOrder = [
+    copyBookmarks[1].guid,
+    copyBookmarks[2].guid,
+    copyBookmarks[0].guid,
+    copyBookmarks[3].guid,
+    copyBookmarks[6].guid,
+    copyBookmarks[9].guid,
+    copyBookmarks[4].guid,
+    copyBookmarks[5].guid,
+    copyBookmarks[7].guid,
+    copyBookmarks[8].guid,
+  ];
+
+  Assert.equal(tree.children.length, expectedBookmarkOrder.length,
+               "Should be the expected amount of bookmarks in the unfiled folder.");
+
+  for (let i = 0; i < expectedBookmarkOrder.length; ++i) {
+    Assert.equal(tree.children[i].guid, expectedBookmarkOrder[i],
+                 `Should be the expected item at index ${i}`);
+  }
+
+  await PlacesUtils.bookmarks.eraseEverything();
+});
+
 add_task(async function paste_from_different_instance() {
   let xferable = Cc["@mozilla.org/widget/transferable;1"]
                    .createInstance(Ci.nsITransferable);
   xferable.init(null);
 
   // Fake data on the clipboard to pretend this is from a different instance
   // of Firefox.
   let data = {
@@ -81,18 +237,20 @@ add_task(async function paste_from_diffe
   data = JSON.stringify(data);
 
   xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
   xferable.setTransferData(PlacesUtils.TYPE_X_MOZ_PLACE, PlacesUtils.toISupportsString(data),
                            data.length * 2);
 
   Services.clipboard.setData(xferable, null, Ci.nsIClipboard.kGlobalClipboard);
 
+  info("Selecting UnfiledBookmarks in the left pane");
+  PlacesOrganizer.selectLeftPaneQuery("UnfiledBookmarks");
+
   info("Pasting clipboard");
-
   await ContentTree.view.controller.paste();
 
   let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
 
   Assert.equal(tree.children.length, 1,
                "Should be one bookmark in the unfiled folder.");
   Assert.equal(tree.children[0].title, "test",
                "Should have the correct title");