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
--- 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");