Fix #940, UI not updating when dragging child item between parents

chrome/content/zotero/xpcom/data/items.js

@@ -491,6 +491,7 @@ Zotero.Items = function() {
 		var onRow = function (row, setFunc) {
 			var itemID = row.getResultByIndex(0);
 			
+			// If we've finished a set of rows for an item, process them
 			if (lastItemID && itemID !== lastItemID) {
 				setFunc(lastItemID, rows);
 				rows = [];
@@ -543,9 +544,14 @@ Zotero.Items = function() {
 				}
 			}
 		);
+		// Process unprocessed rows
 		if (lastItemID) {
 			setAttachmentItem(lastItemID, rows);
 		}
+		// Otherwise clear existing entries for passed items
+		else if (ids.length) {
+			ids.forEach(id => setAttachmentItem(id, []));
+		}
 		
 		//
 		// Notes
@@ -591,9 +597,14 @@ Zotero.Items = function() {
 				}
 			}
 		);
+		// Process unprocessed rows
 		if (lastItemID) {
 			setNoteItem(lastItemID, rows);
 		}
+		// Otherwise clear existing entries for passed items
+		else if (ids.length) {
+			ids.forEach(id => setNoteItem(id, []));
+		}
 		
 		// Mark all top-level items as having child items loaded
 		sql = "SELECT itemID FROM items I WHERE libraryID=?" + idSQL + " AND itemID NOT IN "

chrome/content/zotero/xpcom/itemTreeView.js

@@ -712,9 +712,13 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio
 						
 						sort = id;
 					}
+					// If item was moved from one parent to another, remove from old parent
+					else if (parentItemID && parentIndex != -1 && this._rowMap[parentItemID] != parentIndex) {
+						this._removeRow(row);
+					}
 					// If not moved from under one item to another, just resort the row,
 					// which also invalidates it and refreshes it
-					else if (!(parentItemID && parentIndex != -1 && this._rowMap[parentItemID] != parentIndex)) {
+					else {
 						sort = id;
 					}
 					
@@ -870,6 +874,7 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio
 		else {
 			this._refreshItemRowMap();
 		}
+		this._refreshItemRowMap();
 		
 		if (singleSelect) {
 			if (!extraData[singleSelect] || !extraData[singleSelect].skipSelect) {

test/tests/itemTest.js

@@ -372,6 +372,26 @@ describe("Zotero.Item", function () {
 		})
 	});
 	
+	describe("#getCreators()", function () {
+		it("should update after creators are removed", function* () {
+			var item = createUnsavedDataObject('item');
+			item.setCreators([
+				{
+					creatorType: "author",
+					name: "A"
+				}
+			]);
+			yield item.saveTx();
+			
+			assert.lengthOf(item.getCreators(), 1);
+			
+			item.setCreators([]);
+			yield item.saveTx();
+			
+			assert.lengthOf(item.getCreators(), 0);
+		});
+	});
+	
 	describe("#setCreators", function () {
 		it("should accept an array of creators in API JSON format", function* () {
 			var creators = [
@@ -459,6 +479,25 @@ describe("Zotero.Item", function () {
 			var item = yield createDataObject('item');
 			assert.lengthOf(item.getAttachments(), 0);
 		})
+		
+		it("should update after an attachment is moved to another item", function* () {
+			var item1 = yield createDataObject('item');
+			var item2 = yield createDataObject('item');
+			var item3 = new Zotero.Item('attachment');
+			item3.parentID = item1.id;
+			item3.attachmentLinkMode = 'linked_url';
+			item3.setField('url', 'http://example.com');
+			yield item3.saveTx();
+			
+			assert.lengthOf(item1.getAttachments(), 1);
+			assert.lengthOf(item2.getAttachments(), 0);
+			
+			item3.parentID = item2.id;
+			yield item3.saveTx();
+			
+			assert.lengthOf(item1.getAttachments(), 0);
+			assert.lengthOf(item2.getAttachments(), 1);
+		});
 	})
 	
 	describe("#getNotes()", function () {
@@ -499,7 +538,22 @@ describe("Zotero.Item", function () {
 		it("#should return an empty array for an item with no notes", function* () {
 			var item = yield createDataObject('item');
 			assert.lengthOf(item.getNotes(), 0);
-		})
+		});
+		
+		it("should update after a note is moved to another item", function* () {
+			var item1 = yield createDataObject('item');
+			var item2 = yield createDataObject('item');
+			var item3 = yield createDataObject('item', { itemType: 'note', parentID: item1.id });
+			
+			assert.lengthOf(item1.getNotes(), 1);
+			assert.lengthOf(item2.getNotes(), 0);
+			
+			item3.parentID = item2.id;
+			yield item3.saveTx();
+			
+			assert.lengthOf(item1.getNotes(), 0);
+			assert.lengthOf(item2.getNotes(), 1);
+		});
 	})
 	
 	describe("#attachmentCharset", function () {

test/tests/itemTreeViewTest.js

@@ -308,6 +308,86 @@ describe("Zotero.ItemTreeView", function() {
 	})
 	
 	describe("#drop()", function () {
+		it("should move a child item from one item to another", function* () {
+			var collection = yield createDataObject('collection');
+			yield waitForItemsLoad(win);
+			var item1 = yield createDataObject('item', { title: "A", collections: [collection.id] });
+			var item2 = yield createDataObject('item', { title: "B", collections: [collection.id] });
+			var item3 = yield createDataObject('item', { itemType: 'note', parentID: item1.id });
+			
+			let view = zp.itemsView;
+			yield view.selectItem(item3.id, true);
+			
+			var deferred = Zotero.Promise.defer();
+			view.addEventListener('select', () => deferred.resolve());
+			
+			view.drop(view.getRowIndexByID(item2.id), 0, {
+				dropEffect: 'copy',
+				effectAllowed: 'copy',
+				types: {
+					contains: function (type) {
+						return type == 'zotero/item';
+					}
+				},
+				getData: function (type) {
+					if (type == 'zotero/item') {
+						return item3.id + "";
+					}
+				},
+				mozItemCount: 1
+			})
+			
+			yield deferred.promise;
+			
+			// Old parent should be empty
+			assert.isFalse(view.isContainerOpen(view.getRowIndexByID(item1.id)));
+			assert.isTrue(view.isContainerEmpty(view.getRowIndexByID(item1.id)));
+			
+			// New parent should be open
+			assert.isTrue(view.isContainerOpen(view.getRowIndexByID(item2.id)));
+			assert.isFalse(view.isContainerEmpty(view.getRowIndexByID(item2.id)));
+		});
+		
+		it("should move a child item from last item in list to another", function* () {
+			var collection = yield createDataObject('collection');
+			yield waitForItemsLoad(win);
+			var item1 = yield createDataObject('item', { title: "A", collections: [collection.id] });
+			var item2 = yield createDataObject('item', { title: "B", collections: [collection.id] });
+			var item3 = yield createDataObject('item', { itemType: 'note', parentID: item2.id });
+			
+			let view = zp.itemsView;
+			yield view.selectItem(item3.id, true);
+			
+			var deferred = Zotero.Promise.defer();
+			view.addEventListener('select', () => deferred.resolve());
+			
+			view.drop(view.getRowIndexByID(item1.id), 0, {
+				dropEffect: 'copy',
+				effectAllowed: 'copy',
+				types: {
+					contains: function (type) {
+						return type == 'zotero/item';
+					}
+				},
+				getData: function (type) {
+					if (type == 'zotero/item') {
+						return item3.id + "";
+					}
+				},
+				mozItemCount: 1
+			})
+			
+			yield deferred.promise;
+			
+			// Old parent should be empty
+			assert.isFalse(view.isContainerOpen(view.getRowIndexByID(item2.id)));
+			assert.isTrue(view.isContainerEmpty(view.getRowIndexByID(item2.id)));
+			
+			// New parent should be open
+			assert.isTrue(view.isContainerOpen(view.getRowIndexByID(item1.id)));
+			assert.isFalse(view.isContainerEmpty(view.getRowIndexByID(item1.id)));
+		});
+		
 		it("should create a top-level attachment when a file is dragged", function* () {
 			var file = getTestDataDirectory();
 			file.append('test.png');