Bug 1439697 - Re-organise the copy/paste & drag/drop algorithm to make it easier to understand. r?mak
MozReview-Commit-ID: BrQEU2e4Nr0
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -917,71 +917,16 @@ var PlacesUIUtils = {
} finally {
if (itemsBeingChanged > ITEM_CHANGED_BATCH_NOTIFICATION_THRESHOLD) {
resultNode.onEndUpdateBatch();
}
}
},
/**
- * Constructs a Places Transaction for the drop or paste of a blob of data
- * into a container.
- *
- * @param aData
- * The unwrapped data blob of dropped or pasted data.
- * @param aNewParentGuid
- * GUID of the container the data was dropped or pasted into.
- * @param aIndex
- * The index within the container the item was dropped or pasted at.
- * @param aCopy
- * The drag action was copy, so don't move folders or links.
- *
- * @return a Places Transaction that can be transacted for performing the
- * move/insert command.
- */
- getTransactionForData(aData, aNewParentGuid, aIndex, aCopy) {
- if (!this.SUPPORTED_FLAVORS.includes(aData.type))
- throw new Error(`Unsupported '${aData.type}' data type`);
-
- if ("itemGuid" in aData && "instanceId" in aData &&
- aData.instanceId == PlacesUtils.instanceId) {
- if (!this.PLACES_FLAVORS.includes(aData.type))
- throw new Error(`itemGuid unexpectedly set on ${aData.type} data`);
-
- let info = { guid: aData.itemGuid,
- newParentGuid: aNewParentGuid,
- newIndex: aIndex };
- if (aCopy) {
- info.excludingAnnotation = "Places/SmartBookmark";
- return PlacesTransactions.Copy(info);
- }
- return PlacesTransactions.Move(info);
- }
-
- // Since it's cheap and harmless, we allow the paste of separators and
- // bookmarks from builds that use legacy transactions (i.e. when itemGuid
- // was not set on PLACES_FLAVORS data). Containers are a different story,
- // and thus disallowed.
- if (aData.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
- throw new Error("Can't copy a container from a legacy-transactions build");
-
- if (aData.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
- return PlacesTransactions.NewSeparator({ parentGuid: aNewParentGuid,
- index: aIndex });
- }
-
- let title = aData.type != PlacesUtils.TYPE_UNICODE ? aData.title
- : aData.uri;
- return PlacesTransactions.NewBookmark({ url: Services.io.newURI(aData.uri),
- title,
- parentGuid: aNewParentGuid,
- index: aIndex });
- },
-
- /**
* Processes a set of transfer items that have been dropped or pasted.
* Batching will be applied where necessary.
*
* @param {Array} items A list of unwrapped nodes to process.
* @param {Object} insertionPoint The requested point for insertion.
* @param {Boolean} doCopy Set to true to copy the items, false will move them
* if possible.
* @paramt {Object} view The view that should be used for batching.
@@ -994,17 +939,17 @@ var PlacesUIUtils = {
if (insertionPoint.isTag) {
let urls = items.filter(item => "uri" in item).map(item => item.uri);
itemsCount = urls.length;
transactions = [PlacesTransactions.Tag({ urls, tag: insertionPoint.tagName })];
} else {
let insertionIndex = await insertionPoint.getIndex();
itemsCount = items.length;
transactions = await getTransactionsForTransferItems(
- items, insertionIndex, insertionPoint.guid, doCopy);
+ items, insertionIndex, insertionPoint.guid, !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 [];
}
@@ -1193,30 +1138,77 @@ function getResultForBatching(viewOrElem
if (viewOrElement && viewOrElement.result) {
return viewOrElement.result;
}
return null;
}
+
/**
* Processes a set of transfer items and returns transactions to insert or
* move them.
*
* @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.
+ * @param {Boolean} doMove Set to true to MOVE the items if possible, false will
+ * copy them.
* @return {Array} Returns an array of created PlacesTransactions.
*/
async function getTransactionsForTransferItems(items, insertionIndex,
- insertionParentGuid, doCopy) {
+ insertionParentGuid, doMove) {
+ let canMove = true;
+ for (let item of items) {
+ if (!PlacesUIUtils.SUPPORTED_FLAVORS.includes(item.type)) {
+ throw new Error(`Unsupported '${item.type}' data type`);
+ }
+
+ // Work out if this is data from the same app session we're running in.
+ if ("instanceId" in item && item.instanceId == PlacesUtils.instanceId) {
+ if ("itemGuid" in item && !PlacesUIUtils.PLACES_FLAVORS.includes(item.type)) {
+ throw new Error(`itemGuid unexpectedly set on ${item.type} data`);
+ }
+ } else {
+ if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
+ throw new Error("Can't copy a container from a legacy-transactions build");
+ }
+
+ // We can never move from an external copy.
+ canMove = false;
+ }
+
+ if (doMove && canMove) {
+ canMove = canMoveUnwrappedNode(item);
+ }
+ }
+
+ if (doMove && !canMove) {
+ Cu.reportError("Tried to move an unmovable Places " +
+ "node, reverting to a copy operation.");
+ doMove = false;
+ }
+
+ return doMove ? getTransactionsForMove(items, insertionIndex, insertionParentGuid) :
+ getTransactionsForCopy(items, insertionIndex, insertionParentGuid);
+}
+
+/**
+ * Processes a set of transfer items and returns an array of transactions.
+ *
+ * @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.
+ * @return {Array} Returns an array of created PlacesTransactions.
+ */
+async function getTransactionsForMove(items, insertionIndex,
+ insertionParentGuid) {
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.
@@ -1232,27 +1224,69 @@ async function getTransactionsForTransfe
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 && !canMoveUnwrappedNode(item)) {
- Cu.reportError("Tried to move an unmovable Places " +
- "node, reverting to a copy operation.");
- doCopy = true;
- }
- transactions.push(
- PlacesUIUtils.getTransactionForData(item,
- insertionParentGuid,
- index,
- doCopy));
+ transactions.push(PlacesTransactions.Move({
+ guid: item.itemGuid,
+ newIndex: index,
+ newParentGuid: insertionParentGuid,
+ }));
if (index != -1 && item.itemGuid) {
index++;
}
}
return transactions;
}
+
+/**
+ * Processes a set of transfer items and returns an array of transactions.
+ *
+ * @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.
+ * @return {Array} Returns an array of created PlacesTransactions.
+ */
+async function getTransactionsForCopy(items, insertionIndex,
+ insertionParentGuid) {
+ let transactions = [];
+ let index = insertionIndex;
+
+ for (let item of items) {
+ let transaction;
+
+ if ("itemGuid" in item && "instanceId" in item &&
+ item.instanceId == PlacesUtils.instanceId) {
+ transaction = PlacesTransactions.Copy({
+ excludingAnnotation: "Places/SmartBookmark",
+ guid: item.itemGuid,
+ newIndex: index,
+ newParentGuid: insertionParentGuid,
+ });
+ } else if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
+ transaction = PlacesTransactions.NewSeparator({
+ index,
+ parentGuid: insertionParentGuid,
+ });
+ } else {
+ let title = item.type != PlacesUtils.TYPE_UNICODE ? item.title : item.uri;
+ transaction = PlacesTransactions.NewBookmark({
+ index,
+ parentGuid: insertionParentGuid,
+ title,
+ url: item.uri,
+ });
+ }
+
+ transactions.push(transaction);
+
+ if (index != -1) {
+ index++;
+ }
+ }
+ return transactions;
+}
--- a/browser/components/places/tests/browser/browser_controller_onDrop.js
+++ b/browser/components/places/tests/browser/browser_controller_onDrop.js
@@ -15,17 +15,17 @@ var bookmarkIds;
add_task(async function setup() {
registerCleanupFunction(async function() {
sandbox.restore();
delete window.sinon;
await PlacesUtils.bookmarks.eraseEverything();
await PlacesUtils.history.clear();
});
- sandbox.stub(PlacesUIUtils, "getTransactionForData");
+ sandbox.stub(PlacesTransactions, "Move");
sandbox.stub(PlacesTransactions, "batch");
bookmarks = await PlacesUtils.bookmarks.insertTree({
guid: PlacesUtils.bookmarks.unfiledGuid,
children: [{
title: "bm1",
url: "http://example1.com"
}, {
@@ -59,17 +59,17 @@ add_task(async function setup() {
async function run_drag_test(startBookmarkIndex, insertionIndex, newParentGuid,
expectedInsertionIndex, expectTransactionCreated = true) {
if (!newParentGuid) {
newParentGuid = PlacesUtils.bookmarks.unfiledGuid;
}
// Reset the stubs so that previous test runs don't count against us.
- PlacesUIUtils.getTransactionForData.reset();
+ PlacesTransactions.Move.reset();
PlacesTransactions.batch.reset();
let dragBookmark = bookmarks[startBookmarkIndex];
await withSidebarTree("bookmarks", async (tree) => {
tree.selectItems([PlacesUtils.bookmarks.unfiledGuid]);
PlacesUtils.asContainer(tree.selectedNode).containerOpen = true;
@@ -78,21 +78,27 @@ async function run_drag_test(startBookma
// insertion point and drag data and call the function direct.
let ip = new PlacesInsertionPoint({
parentId: await PlacesUtils.promiseItemId(PlacesUtils.bookmarks.unfiledGuid),
parentGuid: newParentGuid,
index: insertionIndex,
orientation: Ci.nsITreeView.DROP_ON
});
- let bookmarkWithId = JSON.stringify(Object.assign({
+ let dragData = Object.assign({
id: bookmarkIds.get(dragBookmark.guid),
itemGuid: dragBookmark.guid,
parent: PlacesUtils.unfiledBookmarksFolderId,
- }, dragBookmark));
+ instanceId: PlacesUtils.instanceId,
+ }, dragBookmark);
+
+ // Force the type.
+ dragData.type = PlacesUtils.TYPE_X_MOZ_PLACE;
+ let bookmarkWithId = JSON.stringify(dragData);
+
let dt = {
dropEffect: "move",
mozCursor: "auto",
mozItemCount: 1,
types: [ PlacesUtils.TYPE_X_MOZ_PLACE ],
mozTypesAt(i) {
return this.types;
@@ -100,34 +106,32 @@ async function run_drag_test(startBookma
mozGetDataAt(i) {
return bookmarkWithId;
}
};
await PlacesControllerDragHelper.onDrop(ip, dt);
if (!expectTransactionCreated) {
- Assert.ok(PlacesUIUtils.getTransactionForData.notCalled,
+ Assert.ok(PlacesTransactions.Move.notCalled,
"Should not have created transaction data");
return;
}
- Assert.ok(PlacesUIUtils.getTransactionForData.calledOnce,
+ Assert.ok(PlacesTransactions.Move.calledOnce,
"Should have called getTransactionForData at least once.");
- let args = PlacesUIUtils.getTransactionForData.args[0];
+ let moveData = PlacesTransactions.Move.args[0][0];
- Assert.deepEqual(args[0], JSON.parse(bookmarkWithId),
+ Assert.deepEqual(moveData.guid, dragBookmark.guid,
"Should have called getTransactionForData with the correct unwrapped bookmark");
- Assert.equal(args[1], newParentGuid,
+ Assert.equal(moveData.newParentGuid, newParentGuid,
"Should have called getTransactionForData with the correct parent guid");
- Assert.equal(args[2], expectedInsertionIndex,
+ Assert.equal(moveData.newIndex, expectedInsertionIndex,
"Should have called getTransactionForData with the correct index");
- Assert.equal(args[3], 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.
--- a/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
+++ b/browser/components/places/tests/browser/browser_controller_onDrop_tagFolder.js
@@ -80,17 +80,17 @@ async function run_drag_test(startBookma
mozGetDataAt(i) {
return bookmarkWithId;
}
};
await PlacesControllerDragHelper.onDrop(ip, dt);
Assert.ok(PlacesTransactions.Tag.calledOnce,
- "Should have called getTransactionForData at least once.");
+ "Should have called PlacesTransactions.Tag at least once.");
let arg = PlacesTransactions.Tag.args[0][0];
Assert.equal(arg.urls.length, 1,
"Should have called PlacesTransactions.Tag with an array of one url");
Assert.equal(arg.urls[0], dragBookmark.url,
"Should have called PlacesTransactions.Tag with the correct url");
Assert.equal(arg.tag, TAG_NAME,