From 1a49018bdcaa8c1704374ad568946a6ae89df231 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 3 Feb 2017 00:04:05 -0500
Subject: [PATCH] Fix moving items between collections

`mozSourceNode` seems to no longer be set in `dataTransfer` objects
during drags, so we now store it in `Zotero.DragDrop`.
---
 chrome/content/zotero/xpcom/itemTreeView.js   | 12 ++++++
 .../content/zotero/xpcom/libraryTreeView.js   |  4 +-
 chrome/content/zotero/xpcom/zotero.js         |  8 ++--
 chrome/content/zotero/zoteroPane.xul          |  3 +-
 test/tests/collectionTreeViewTest.js          | 43 +++++++++++++++++--
 5 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js
index 922a4e87b..3126da6df 100644
--- a/chrome/content/zotero/xpcom/itemTreeView.js
+++ b/chrome/content/zotero/xpcom/itemTreeView.js
@@ -2518,6 +2518,12 @@ Zotero.ItemTreeView.prototype.onDragStart = function (event) {
 	
 	var itemIDs = this.getSelectedItems(true);
 	event.dataTransfer.setData("zotero/item", itemIDs);
+	// dataTransfer.mozSourceNode doesn't seem to be properly set anymore (tested in 50), so store
+	// target separately
+	if (!event.dataTransfer.mozSourceNode) {
+		Zotero.debug("mozSourceNode not set -- storing source node");
+		Zotero.DragDrop.currentSourceNode = event.target;
+	}
 	
 	var items = Zotero.Items.get(itemIDs);
 	
@@ -2609,6 +2615,12 @@ Zotero.ItemTreeView.prototype.onDragStart = function (event) {
 	}
 };
 
+Zotero.ItemTreeView.prototype.onDragEnd = function (event) {
+	setTimeout(function () {
+		Zotero.DragDrop.currentDragSource = null;
+	});
+}
+
 
 // Implements nsIFlavorDataProvider for dragging attachment files to OS
 //
diff --git a/chrome/content/zotero/xpcom/libraryTreeView.js b/chrome/content/zotero/xpcom/libraryTreeView.js
index b35309ff7..de4ce9485 100644
--- a/chrome/content/zotero/xpcom/libraryTreeView.js
+++ b/chrome/content/zotero/xpcom/libraryTreeView.js
@@ -310,10 +310,10 @@ Zotero.LibraryTreeView.prototype = {
 		}
 		
 		if (event.dataTransfer.getData("zotero/item")) {
-			var sourceCollectionTreeRow = Zotero.DragDrop.getDragSource();
+			var sourceCollectionTreeRow = Zotero.DragDrop.getDragSource(event.dataTransfer);
 			if (sourceCollectionTreeRow) {
 				if (this.type == 'collection') {
-					var targetCollectionTreeRow = Zotero.DragDrop.getDragTarget();
+					var targetCollectionTreeRow = Zotero.DragDrop.getDragTarget(event);
 				}
 				else if (this.type == 'item') {
 					var targetCollectionTreeRow = this.collectionTreeRow;
diff --git a/chrome/content/zotero/xpcom/zotero.js b/chrome/content/zotero/xpcom/zotero.js
index d176a13ec..fc5cb064a 100644
--- a/chrome/content/zotero/xpcom/zotero.js
+++ b/chrome/content/zotero/xpcom/zotero.js
@@ -2312,6 +2312,7 @@ Zotero.VersionHeader = {
 Zotero.DragDrop = {
 	currentEvent: null,
 	currentOrientation: 0,
+	currentSourceNode: null,
 	
 	getDataFromDataTransfer: function (dataTransfer, firstOnly) {
 		var dt = dataTransfer;
@@ -2374,7 +2375,7 @@ Zotero.DragDrop = {
 		// For items, the drag source is the CollectionTreeRow of the parent window
 		// of the source tree
 		if (dataTransfer.types.contains("zotero/item")) {
-			var sourceNode = dataTransfer.mozSourceNode;
+			let sourceNode = dataTransfer.mozSourceNode || this.currentSourceNode;
 			if (!sourceNode || sourceNode.tagName != 'treechildren'
 					|| sourceNode.parentElement.id != 'zotero-items-tree') {
 				return false;
@@ -2385,9 +2386,8 @@ Zotero.DragDrop = {
 			}
 			return win.ZoteroPane.collectionsView.selectedTreeRow;
 		}
-		else {
-			return false;
-		}
+		
+		return false;
 	},
 	
 	
diff --git a/chrome/content/zotero/zoteroPane.xul b/chrome/content/zotero/zoteroPane.xul
index 46992e531..a30f1a1d5 100644
--- a/chrome/content/zotero/zoteroPane.xul
+++ b/chrome/content/zotero/zoteroPane.xul
@@ -554,7 +554,8 @@
 								<treechildren ondragstart="ZoteroPane_Local.itemsView.onDragStart(event)"
 									ondragenter="return ZoteroPane_Local.itemsView.onDragEnter(event)"
 									ondragover="return ZoteroPane_Local.itemsView.onDragOver(event)"
-									ondrop="return ZoteroPane_Local.itemsView.onDrop(event)"/>
+									ondrop="return ZoteroPane_Local.itemsView.onDrop(event)"
+									ondragend="ZoteroPane_Local.itemsView.onDragEnd(event)"/>
 							</tree>
 
 							<!-- Label for displaying messages when items pane is hidden
diff --git a/test/tests/collectionTreeViewTest.js b/test/tests/collectionTreeViewTest.js
index 745c93826..e782146ea 100644
--- a/test/tests/collectionTreeViewTest.js
+++ b/test/tests/collectionTreeViewTest.js
@@ -450,7 +450,7 @@ describe("Zotero.CollectionTreeView", function() {
 		 *     value returned after the drag. Otherwise, an 'add' event will be waited for, and
 		 *     an object with 'ids' and 'extraData' will be returned.
 		 */
-		var drop = Zotero.Promise.coroutine(function* (objectType, targetRow, ids, promise) {
+		var drop = Zotero.Promise.coroutine(function* (objectType, targetRow, ids, promise, action = 'copy') {
 			if (typeof targetRow == 'string') {
 				var row = cv.getRowIndexByID(targetRow);
 				var orient = 0;
@@ -465,9 +465,9 @@ describe("Zotero.CollectionTreeView", function() {
 				promise = waitForNotifierEvent("add", objectType);
 			}
 			yield cv.drop(row, orient, {
-				dropEffect: 'copy',
-				effectAllowed: 'copy',
-				mozSourceNode: win.document.getElementById(`zotero-${objectType}s-tree`),
+				dropEffect: action,
+				effectAllowed: action,
+				mozSourceNode: win.document.getElementById(`zotero-${objectType}s-tree`).treeBoxObject.treeBody,
 				types: {
 					contains: function (type) {
 						return type == `zotero/${objectType}`;
@@ -546,6 +546,41 @@ describe("Zotero.CollectionTreeView", function() {
 				assert.equal(treeRow.ref.id, item.id);
 			})
 			
+			it("should move an item from one collection to another", function* () {
+				var collection1 = yield createDataObject('collection');
+				yield waitForItemsLoad(win);
+				var collection2 = yield createDataObject('collection', false, { skipSelect: true });
+				var item = yield createDataObject('item', { collections: [collection1.id] });
+				
+				// Add observer to wait for collection add
+				var deferred = Zotero.Promise.defer();
+				var observerID = Zotero.Notifier.registerObserver({
+					notify: function (event, type, ids, extraData) {
+						if (type == 'collection-item' && event == 'add'
+								&& ids[0] == collection2.id + "-" + item.id) {
+							setTimeout(function () {
+								deferred.resolve();
+							});
+						}
+					}
+				}, 'collection-item', 'test');
+				
+				yield drop('item', 'C' + collection2.id, [item.id], deferred.promise, 'move');
+				
+				Zotero.Notifier.unregisterObserver(observerID);
+				
+				// Source collection should be empty
+				assert.equal(zp.itemsView.rowCount, 0);
+				
+				yield cv.selectCollection(collection2.id);
+				yield waitForItemsLoad(win);
+				
+				// Target collection should have item
+				assert.equal(zp.itemsView.rowCount, 1);
+				var treeRow = zp.itemsView.getRow(0);
+				assert.equal(treeRow.ref.id, item.id);
+			});
+			
 			it("should copy an item with an attachment to a group", function* () {
 				var group = yield createGroup();