Fix #984, 5.0: Avoid scrolling items list when adding items via sync

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) {
+							if (this._rows[previousFirstSelectedRow]
-								previousFirstRow--;
+									&& this.getLevel(previousFirstSelectedRow) == 0) {
+								previousFirstSelectedRow--;
 							}
 						}
 					}
 					
-					if (previousFirstRow !== undefined && this._rows[previousFirstRow]) {
+					if (previousFirstSelectedRow !== undefined && this._rows[previousFirstSelectedRow]) {
-						this.selection.select(previousFirstRow);
+						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();

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');
 	},

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();