From 47f3c1efe6bc4d0e44521b66a789acafd723b9ae Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 7 May 2015 15:05:37 -0400 Subject: [PATCH] Don't reselect items unnecessarily Store and check the last selected items in ZoteroPane.itemSelected() to see if it's necessary to refresh the item pane. This prevents loss of textbox focus if another write occurs while editing a field. Also optimize row adding/removing in itemTreeView.js --- chrome/content/zotero/xpcom/itemTreeView.js | 126 +++++++++++--------- chrome/content/zotero/zoteroPane.js | 24 +++- test/tests/itemTreeViewTest.js | 49 +++++++- 3 files changed, 134 insertions(+), 65 deletions(-) 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);