diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js index e7de1bdcb..3209d22f7 100644 --- a/chrome/content/zotero/xpcom/itemTreeView.js +++ b/chrome/content/zotero/xpcom/itemTreeView.js @@ -352,7 +352,7 @@ Zotero.ItemTreeView.prototype.refresh = Zotero.serial(Zotero.Promise.coroutine(f yield item.loadItemData(); yield item.loadCollections(); - this._addRow( + this._addRowToArray( newRows, new Zotero.ItemTreeRow(item, 0, false), added + 1 @@ -366,7 +366,7 @@ Zotero.ItemTreeView.prototype.refresh = Zotero.serial(Zotero.Promise.coroutine(f for (let id in newSearchParentIDs) { if (!newSearchItemIDs[id]) { let item = yield Zotero.Items.getAsync(id); - this._addRow( + this._addRowToArray( newRows, new Zotero.ItemTreeRow(item, 0, false), added + 1 @@ -587,7 +587,6 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio if(row != null) { this._removeRow(row-i); - this._treebox.rowCountChanged(row-i,-1); } } @@ -619,15 +618,11 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio { var items = yield Zotero.Items.getAsync(ids); - for each(var item in items) { + for (let i = 0; i < items.length; i++) { + let item = items[i]; let id = item.id; - // Make sure row map is up to date - // if we made changes in a previous loop - if (madeChanges) { - this._refreshItemRowMap(); - } - var row = this._itemRowMap[id]; + let row = this._itemRowMap[id]; // Deleted items get a modify that we have to ignore when // not viewing the trash @@ -636,35 +631,25 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio } // Item already exists in this view - if( row != null) - { - var parentItemID = this.getRow(row).ref.parentItemID; - var parentIndex = this.getParentIndex(row); + if (row !== undefined) { + let parentItemID = this.getRow(row).ref.parentItemID; + let parentIndex = this.getParentIndex(row); - // Top-level item + // Top-level items just have to be resorted if (this.isContainer(row)) { - //yield this.toggleOpenState(row); - //yield this.toggleOpenState(row); sort = id; } // If item moved from top-level to under another item, remove the old row. - // The container refresh above takes care of adding the new row. - else if (!this.isContainer(row) && parentIndex == -1 && parentItemID) { + else if (parentIndex == -1 && parentItemID) { this._removeRow(row); - this._treebox.rowCountChanged(row, -1) } // If moved from under another item to top level, remove old row and add new one - else if (!this.isContainer(row) && parentIndex != -1 && !parentItemID) { + else if (parentIndex != -1 && !parentItemID) { this._removeRow(row); - this._treebox.rowCountChanged(row, -1) - this._addRow( - this._rows, - new Zotero.ItemTreeRow(item, 0, false), - this.rowCount - ); - this.rowCount++; - this._treebox.rowCountChanged(this.rowCount - 1, 1); + let beforeRow = this.rowCount; + this._addRow(new Zotero.ItemTreeRow(item, 0, false), beforeRow); + sort = id; } // If not moved from under one item to another, just resort the row, @@ -672,13 +657,14 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio else if (!(parentItemID && parentIndex != -1 && this._itemRowMap[parentItemID] != parentIndex)) { sort = id; } + madeChanges = true; } - // Otherwise, for a top-level item in a root view or a collection + // Otherwise, for a top-level item in a library root or a collection // containing the item, the item has to be added else if (item.isTopLevelItem()) { // Root view - let add = (collectionTreeRow.isLibrary() || collectionTreeRow.isGroup()) + let add = collectionTreeRow.isLibrary(true) && collectionTreeRow.ref.libraryID == item.libraryID; // Collection containing item if (!add && collectionTreeRow.isCollection()) { @@ -687,15 +673,10 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio } if (add) { //most likely, the note or attachment's parent was removed. - this._addRow( - this._rows, - new Zotero.ItemTreeRow(item, 0, false), - this.rowCount - ); - this.rowCount++; - this._treebox.rowCountChanged(this.rowCount-1,1); + let beforeRow = this.rowCount; + this._addRow(new Zotero.ItemTreeRow(item, 0, false), beforeRow); madeChanges = true; - sort = true; + sort = id; } } } @@ -756,13 +737,8 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio && this._itemRowMap[item.id] == null // Regular item or standalone note/attachment && item.isTopLevelItem()) { - this._addRow( - this._rows, - new Zotero.ItemTreeRow(item, 0, false), - this.rowCount - ); - this.rowCount++; - this._treebox.rowCountChanged(this.rowCount-1,1); + let beforeRow = this.rowCount; + this._addRow(new Zotero.ItemTreeRow(item, 0, false), beforeRow); madeChanges = true; } } @@ -1234,13 +1210,10 @@ Zotero.ItemTreeView.prototype.toggleOpenState = Zotero.Promise.coroutine(functio for (let i = 0; i < newRows.length; i++) { count++; this._addRow( - this._rows, new Zotero.ItemTreeRow(newRows[i], level + 1, false), row + i + 1 ); } - this.rowCount += count; - this._treebox.rowCountChanged(row + 1, count); } this._rows[row].isOpen = true; @@ -1270,11 +1243,11 @@ Zotero.ItemTreeView.prototype._closeContainer = function (row, skipItemMapRefres // Remove child rows while ((row + 1 < this._rows.length) && (this.getLevel(row + 1) > level)) { - this._removeRow(row+1); + // Skip the map update here and just refresh the whole map below, + // since we might be removing multiple rows + this._removeRow(row + 1, true); count--; } - // Mark as removed from the end of the row's children - this._treebox.rowCountChanged(row + 1 + Math.abs(count), count); this._rows[row].isOpen = false; @@ -1895,26 +1868,61 @@ Zotero.ItemTreeView.prototype.setFilter = Zotero.Promise.coroutine(function* (ty /** - * Add a tree row into a given array + * Add a tree row to the main array, update the row count, tell the treebox that the row count + * changed, and update the row map * * @param {Array} newRows - Array to operate on * @param {Zotero.ItemTreeRow} itemTreeRow * @param {Number} beforeRow - Row index to insert new row before */ -Zotero.ItemTreeView.prototype._addRow = function (newRows, itemTreeRow, beforeRow) { - newRows.splice(beforeRow, 0, itemTreeRow); +Zotero.ItemTreeView.prototype._addRow = function (itemTreeRow, beforeRow) { + this._addRowToArray(this._rows, itemTreeRow, beforeRow); + this.rowCount++; + this._treebox.rowCountChanged(beforeRow, 1); + // Increment all rows in map at or above insertion point + for (let i in this._itemRowMap) { + if (this._itemRowMap[j] >= beforeRow) { + this._itemRowMap[j]++ + } + } + // Add new row to map + this._itemRowMap[itemTreeRow.id] = beforeRow; } /** - * Remove a row from the main rows array + * Add a tree row into a given array + * + * @param {Array} array - Array to operate on + * @param {Zotero.ItemTreeRow} itemTreeRow + * @param {Number} beforeRow - Row index to insert new row before */ -Zotero.ItemTreeView.prototype._removeRow = function (row) { +Zotero.ItemTreeView.prototype._addRowToArray = function (array, itemTreeRow, beforeRow) { + array.splice(beforeRow, 0, itemTreeRow); +} + + + +/** + * Remove a row from the main array, decrement the row count, tell the treebox that the row + * count changed, delete the row from the map, and optionally update all rows above it in the map + */ +Zotero.ItemTreeView.prototype._removeRow = function (row, skipItemMapUpdate) { + var id = this._rows[row].id; this._rows.splice(row, 1); this.rowCount--; - if (this.selection.isSelected(row)) { - this.selection.toggleSelect(row); + this._treebox.rowCountChanged(row + 1, -1); + delete this._itemRowMap[id]; + if (!skipItemMapUpdate) { + for (let i in this._itemRowMap) { + if (this._itemRowMap[i] > row) { + this._itemRowMap[i]--; + } + } } + /*if (this.selection.isSelected(row)) { + this.selection.toggleSelect(row); + }*/ } /* diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 5108fd9d5..17fc69104 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -34,6 +34,7 @@ var ZoteroPane = new function() this.itemsView = false; this._listeners = {}; this.__defineGetter__('loaded', function () _loaded); + var _lastSelectedItems = []; //Privileged methods this.init = init; @@ -1229,11 +1230,23 @@ var ZoteroPane = new function() */ this.itemSelected = function (event) { return Zotero.spawn(function* () { + var selectedItems = this.itemsView.getSelectedItems(); + + // Check if selection has actually changed. The onselect event that calls this + // can be called in various situations where the selection didn't actually change, + // such as whenever selectEventsSuppressed is set to false. + var ids = selectedItems.map(item => item.id); + ids.sort(); + if (Zotero.Utilities.arrayEquals(_lastSelectedItems, ids)) { + return false; + } + _lastSelectedItems = ids; + yield Zotero.DB.waitForTransaction(); if (!this.itemsView) { Zotero.debug("Items view not available in itemSelected", 2); - return; + return false; } // Display restore button if items selected in Trash @@ -1259,7 +1272,7 @@ var ZoteroPane = new function() // Single item selected if (this.itemsView.selection.count == 1 && this.itemsView.selection.currentIndex != -1) { - var item = this.itemsView.getSelectedItems()[0]; + var item = selectedItems[0]; if (item.isNote()) { var noteEditor = document.getElementById('zotero-note-editor'); @@ -1362,7 +1375,7 @@ var ZoteroPane = new function() var displayNumItemsOnTypeError = count > 5 && count == this.itemsView.rowCount; // Initialize the merge pane with the selected items - Zotero_Duplicates_Pane.setItems(this.getSelectedItems(), displayNumItemsOnTypeError); + Zotero_Duplicates_Pane.setItems(selectedItems, displayNumItemsOnTypeError); } else { var msg = Zotero.getString('pane.item.duplicates.selectToMerge'); @@ -1394,12 +1407,15 @@ var ZoteroPane = new function() this.setItemPaneMessage(msg); } } + + return true; }, this) - .then(function () { + .then(function (result) { // See note in itemTreeView.js::selectItem() if (this.itemsView && this.itemsView._itemSelectedPromiseResolver) { this.itemsView._itemSelectedPromiseResolver.resolve(); } + return result; }.bind(this)) .catch(function (e) { Zotero.debug(e, 1); diff --git a/test/tests/itemTreeViewTest.js b/test/tests/itemTreeViewTest.js index 41f1da7a6..177eacdd0 100644 --- a/test/tests/itemTreeViewTest.js +++ b/test/tests/itemTreeViewTest.js @@ -31,10 +31,20 @@ describe("Zotero.ItemTreeView", function() { }) describe("#notify()", function () { + beforeEach(function () { + sinon.spy(win.ZoteroPane, "itemSelected"); + }) + + afterEach(function () { + win.ZoteroPane.itemSelected.restore(); + }) + it("should select a new item", function* () { itemsView.selection.clearSelection(); assert.lengthOf(itemsView.getSelectedItems(), 0); + assert.equal(win.ZoteroPane.itemSelected.callCount, 1); + // Create item var item = new Zotero.Item('book'); var id = yield item.save(); @@ -43,6 +53,10 @@ describe("Zotero.ItemTreeView", function() { var selected = itemsView.getSelectedItems(); assert.lengthOf(selected, 1); assert.equal(selected[0].id, id); + + // Item should have been selected once + assert.equal(win.ZoteroPane.itemSelected.callCount, 2); + assert.ok(win.ZoteroPane.itemSelected.returnValues[1].value()); }); it("shouldn't select a new item if skipNotifier is passed", function* () { @@ -52,12 +66,18 @@ describe("Zotero.ItemTreeView", function() { assert.lengthOf(selected, 1); assert.equal(selected[0], existingItemID); + // Reset call count on spy + win.ZoteroPane.itemSelected.reset(); + // Create item with skipNotifier flag var item = new Zotero.Item('book'); var id = yield item.save({ skipNotifier: true }); + // No select events should have occurred + assert.equal(win.ZoteroPane.itemSelected.callCount, 0); + // Existing item should still be selected selected = itemsView.getSelectedItems(true); assert.lengthOf(selected, 1); @@ -71,12 +91,20 @@ describe("Zotero.ItemTreeView", function() { assert.lengthOf(selected, 1); assert.equal(selected[0], existingItemID); + // Reset call count on spy + win.ZoteroPane.itemSelected.reset(); + // Create item with skipSelect flag var item = new Zotero.Item('book'); var id = yield item.save({ skipSelect: true }); + // itemSelected should have been called once (from 'selectEventsSuppressed = false' + // in notify()) as a no-op + assert.equal(win.ZoteroPane.itemSelected.callCount, 1); + assert.isFalse(win.ZoteroPane.itemSelected.returnValues[0].value()); + // Existing item should still be selected selected = itemsView.getSelectedItems(true); assert.lengthOf(selected, 1); @@ -91,15 +119,23 @@ describe("Zotero.ItemTreeView", function() { itemsView.selection.clearSelection(); assert.lengthOf(itemsView.getSelectedItems(), 0); + // Reset call count on spy + win.ZoteroPane.itemSelected.reset(); + // Modify item item.setField('title', 'no select on modify'); yield item.save(); + // itemSelected should have been called once (from 'selectEventsSuppressed = false' + // in notify()) as a no-op + assert.equal(win.ZoteroPane.itemSelected.callCount, 1); + assert.isFalse(win.ZoteroPane.itemSelected.returnValues[0].value()); + // Modified item should not be selected assert.lengthOf(itemsView.getSelectedItems(), 0); }); - it("should reselect a selected modified item", function* () { + it("should maintain selection on a selected modified item", function* () { // Create item var item = new Zotero.Item('book'); var id = yield item.save(); @@ -110,9 +146,18 @@ describe("Zotero.ItemTreeView", function() { assert.lengthOf(selected, 1); assert.equal(selected[0], id); - item.setField('title', 'reselect on modify'); + // Reset call count on spy + win.ZoteroPane.itemSelected.reset(); + + // Modify item + item.setField('title', 'maintain selection on modify'); yield item.save(); + // itemSelected should have been called once (from 'selectEventsSuppressed = false' + // in notify()) as a no-op + assert.equal(win.ZoteroPane.itemSelected.callCount, 1); + assert.isFalse(win.ZoteroPane.itemSelected.returnValues[0].value()); + // Modified item should still be selected selected = itemsView.getSelectedItems(true); assert.lengthOf(selected, 1);