From 0bab038925c20936cf45b1f96d79ab18c1c7a2cf Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 9 May 2016 13:26:33 -0400 Subject: [PATCH] Fix #984, 5.0: Avoid scrolling items list when adding items via sync --- chrome/content/zotero/xpcom/itemTreeView.js | 35 ++-- .../content/zotero/xpcom/libraryTreeView.js | 47 ++++++ test/tests/itemTreeViewTest.js | 157 ++++++++++++++++++ 3 files changed, 215 insertions(+), 24 deletions(-) diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js index 8fd9a0c3bc..960a622654 100644 --- a/chrome/content/zotero/xpcom/itemTreeView.js +++ b/chrome/content/zotero/xpcom/itemTreeView.js @@ -486,7 +486,8 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio var sort = false; var savedSelection = this.getSelectedItems(true); - var previousFirstRow = this._rowMap[ids[0]]; + var previousFirstSelectedRow = this._rowMap[ids[0]]; + var scrollPosition = this._saveScrollPosition(); // Redraw the tree (for tag color and progress changes) if (action == 'redraw') { @@ -896,9 +897,9 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio if (action == 'remove' || action == 'trash' || action == 'delete') { // In duplicates view, select the next set on delete if (collectionTreeRow.isDuplicates()) { - if (this._rows[previousFirstRow]) { + if (this._rows[previousFirstSelectedRow]) { // Mirror ZoteroPane.onTreeMouseDown behavior - var itemID = this._rows[previousFirstRow].ref.id; + var itemID = this._rows[previousFirstSelectedRow].ref.id; var setItemIDs = collectionTreeRow.ref.getSetItemsByItemID(itemID); this.selectItems(setItemIDs); } @@ -907,17 +908,18 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio // If this was a child item and the next item at this // position is a top-level item, move selection one row // up to select a sibling or parent - if (ids.length == 1 && previousFirstRow > 0) { + if (ids.length == 1 && previousFirstSelectedRow > 0) { let previousItem = Zotero.Items.get(ids[0]); if (previousItem && !previousItem.isTopLevelItem()) { - if (this._rows[previousFirstRow] && this.getLevel(previousFirstRow) == 0) { - previousFirstRow--; + if (this._rows[previousFirstSelectedRow] + && this.getLevel(previousFirstSelectedRow) == 0) { + previousFirstSelectedRow--; } } } - if (previousFirstRow !== undefined && this._rows[previousFirstRow]) { - this.selection.select(previousFirstRow); + if (previousFirstSelectedRow !== undefined && this._rows[previousFirstSelectedRow]) { + this.selection.select(previousFirstSelectedRow); } // If no item at previous position, select last item in list else if (this._rows[this._rows.length - 1]) { @@ -930,6 +932,7 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio } } + this._rememberScrollPosition(scrollPosition); this._treebox.invalidate(); } // For special case in which an item needs to be selected without changes @@ -2053,22 +2056,6 @@ Zotero.ItemTreeView.prototype.expandMatchParents = function () { } -Zotero.ItemTreeView.prototype.saveFirstRow = function() { - var row = this._treebox.getFirstVisibleRow(); - if (row) { - return this.getRow(row).ref.id; - } - return false; -} - - -Zotero.ItemTreeView.prototype.rememberFirstRow = function(firstRow) { - if (firstRow && this._rowMap[firstRow]) { - this._treebox.scrollToRow(this._rowMap[firstRow]); - } -} - - Zotero.ItemTreeView.prototype.expandAllRows = function () { var unsuppress = this.selection.selectEventsSuppressed = true; this._treebox.beginUpdateBatch(); diff --git a/chrome/content/zotero/xpcom/libraryTreeView.js b/chrome/content/zotero/xpcom/libraryTreeView.js index bfdb1d36ef..00f3b1c06e 100644 --- a/chrome/content/zotero/xpcom/libraryTreeView.js +++ b/chrome/content/zotero/xpcom/libraryTreeView.js @@ -95,6 +95,53 @@ Zotero.LibraryTreeView.prototype = { }, + /** + * Return an object describing the current scroll position to restore after changes + * + * @return {Object|Boolean} - Object with .id (a treeViewID) and .offset, or false if no rows + */ + _saveScrollPosition: function() { + var treebox = this._treebox; + var first = treebox.getFirstVisibleRow(); + if (!first) { + return false; + } + var last = treebox.getLastVisibleRow(); + var firstSelected = null; + for (let i = first; i <= last; i++) { + // If an object is selected, keep the first selected one in position + if (this.selection.isSelected(i)) { + return { + id: this.getRow(i).ref.treeViewID, + offset: i - first + }; + } + } + + // Otherwise keep the first visible row in position + return { + id: this.getRow(first).ref.treeViewID, + offset: 0 + }; + }, + + + /** + * Restore a scroll position returned from _saveScrollPosition() + */ + _rememberScrollPosition: function (scrollPosition) { + if (!scrollPosition) { + return; + } + var row = this.getRowIndexByID(scrollPosition.id); + Zotero.debug(scrollPosition.id); + if (row === false) { + return; + } + this._treebox.scrollToRow(Math.max(row - scrollPosition.offset, 0)); + }, + + onSelect: function () { return this._runListeners('select'); }, diff --git a/test/tests/itemTreeViewTest.js b/test/tests/itemTreeViewTest.js index cf34db63da..73e6726527 100644 --- a/test/tests/itemTreeViewTest.js +++ b/test/tests/itemTreeViewTest.js @@ -260,6 +260,163 @@ describe("Zotero.ItemTreeView", function() { yield Zotero.Items.erase(items.map(item => item.id)); }) + it("should keep first visible item in view when other items are added or removed with skipSelect and nothing in view is selected", function* () { + var collection = yield createDataObject('collection'); + yield waitForItemsLoad(win); + itemsView = zp.itemsView; + + var treebox = itemsView._treebox; + var numVisibleRows = treebox.getPageLength(); + + // Get a numeric string left-padded with zeroes + function getTitle(i, max) { + return new String(new Array(max + 1).join(0) + i).slice(-1 * max); + } + + var num = numVisibleRows + 10; + yield Zotero.DB.executeTransaction(function* () { + for (let i = 0; i < num; i++) { + let title = getTitle(i, num); + let item = createUnsavedDataObject('item', { title }); + item.addToCollection(collection.id); + yield item.save(); + } + }.bind(this)); + + // Scroll halfway + treebox.scrollToRow(Math.round(num / 2) - Math.round(numVisibleRows / 2)); + + var firstVisibleItemID = itemsView.getRow(treebox.getFirstVisibleRow()).ref.id; + + // Add one item at the beginning + var item = createUnsavedDataObject( + 'item', { title: getTitle(0, num), collections: [collection.id] } + ); + yield item.saveTx({ + skipSelect: true + }); + // Then add a few more in a transaction + yield Zotero.DB.executeTransaction(function* () { + for (let i = 0; i < 3; i++) { + var item = createUnsavedDataObject( + 'item', { title: getTitle(0, num), collections: [collection.id] } + ); + yield item.save({ + skipSelect: true + }); + } + }.bind(this)); + + // Make sure the same item is still in the first visible row + assert.equal(itemsView.getRow(treebox.getFirstVisibleRow()).ref.id, firstVisibleItemID); + }); + + it("should keep first visible selected item in position when other items are added or removed with skipSelect", function* () { + var collection = yield createDataObject('collection'); + yield waitForItemsLoad(win); + itemsView = zp.itemsView; + + var treebox = itemsView._treebox; + var numVisibleRows = treebox.getPageLength(); + + // Get a numeric string left-padded with zeroes + function getTitle(i, max) { + return new String(new Array(max + 1).join(0) + i).slice(-1 * max); + } + + var num = numVisibleRows + 10; + yield Zotero.DB.executeTransaction(function* () { + for (let i = 0; i < num; i++) { + let title = getTitle(i, num); + let item = createUnsavedDataObject('item', { title }); + item.addToCollection(collection.id); + yield item.save(); + } + }.bind(this)); + + // Scroll halfway + treebox.scrollToRow(Math.round(num / 2) - Math.round(numVisibleRows / 2)); + + // Select an item + itemsView.selection.select(Math.round(num / 2)); + var selectedItem = itemsView.getSelectedItems()[0]; + var offset = itemsView.getRowIndexByID(selectedItem.treeViewID) - treebox.getFirstVisibleRow(); + + // Add one item at the beginning + var item = createUnsavedDataObject( + 'item', { title: getTitle(0, num), collections: [collection.id] } + ); + yield item.saveTx({ + skipSelect: true + }); + // Then add a few more in a transaction + yield Zotero.DB.executeTransaction(function* () { + for (let i = 0; i < 3; i++) { + var item = createUnsavedDataObject( + 'item', { title: getTitle(0, num), collections: [collection.id] } + ); + yield item.save({ + skipSelect: true + }); + } + }.bind(this)); + + // Make sure the selected item is still at the same position + assert.equal(itemsView.getSelectedItems()[0], selectedItem); + var newOffset = itemsView.getRowIndexByID(selectedItem.treeViewID) - treebox.getFirstVisibleRow(); + assert.equal(newOffset, offset); + }); + + it("shouldn't scroll items list if at top when other items are added or removed with skipSelect", function* () { + var collection = yield createDataObject('collection'); + yield waitForItemsLoad(win); + itemsView = zp.itemsView; + + var treebox = itemsView._treebox; + var numVisibleRows = treebox.getPageLength(); + + // Get a numeric string left-padded with zeroes + function getTitle(i, max) { + return new String(new Array(max + 1).join(0) + i).slice(-1 * max); + } + + var num = numVisibleRows + 10; + yield Zotero.DB.executeTransaction(function* () { + // Start at "*1" so we can add items before + for (let i = 1; i < num; i++) { + let title = getTitle(i, num); + let item = createUnsavedDataObject('item', { title }); + item.addToCollection(collection.id); + yield item.save(); + } + }.bind(this)); + + // Scroll to top + treebox.scrollToRow(0); + + // Add one item at the beginning + var item = createUnsavedDataObject( + 'item', { title: getTitle(0, num), collections: [collection.id] } + ); + yield item.saveTx({ + skipSelect: true + }); + // Then add a few more in a transaction + yield Zotero.DB.executeTransaction(function* () { + for (let i = 0; i < 3; i++) { + var item = createUnsavedDataObject( + 'item', { title: getTitle(0, num), collections: [collection.id] } + ); + yield item.save({ + skipSelect: true + }); + } + }.bind(this)); + + // Make sure the first row is still at the top + assert.equal(treebox.getFirstVisibleRow(), 0); + }); + it("should update search results when items are added", function* () { var search = createUnsavedDataObject('search'); var title = Zotero.Utilities.randomString();