Expand/collapse library fixes

- Fixes #994, 5.0: "+" doesn't expand all collections within a library
- If a container (library, collection) is closed directly, the open state of
  all containers below it are now restored when it's reopened. Previously all
  collections would be closed on a manual reopen (though they might have been
  restored on the next Zotero restart).
- If "-" is pressed, all containers are closed, and reopening the library will
  show only top-level collections.

chrome/content/zotero/xpcom/collectionTreeView.js

@@ -98,7 +98,7 @@ Zotero.CollectionTreeView.prototype.setTree = Zotero.Promise.coroutine(function*
 			
 			var key = String.fromCharCode(event.which);
 			if (key == '+' && !(event.ctrlKey || event.altKey || event.metaKey)) {
-				this.expandLibrary(libraryID);
+				this.expandLibrary(libraryID, true);
 			}
 			else if (key == '-' && !(event.shiftKey || event.ctrlKey ||
 					event.altKey || event.metaKey)) {
@@ -854,6 +854,7 @@ Zotero.CollectionTreeView.prototype.toggleOpenState = Zotero.Promise.coroutine(f
 	this._rows[row].isOpen = true;
 	this._treebox.invalidateRow(row);
 	this._refreshRowMap();
+	this._startRememberOpenStatesTimer();
 });
 
 
@@ -873,9 +874,25 @@ Zotero.CollectionTreeView.prototype._closeContainer = function (row) {
 	this._rows[row].isOpen = false;
 	this._treebox.invalidateRow(row);
 	this._refreshRowMap();
+	this._startRememberOpenStatesTimer();
 }
 
 
+/**
+ * After a short delay, persist the open states of the tree, or if already queued, cancel and requeue.
+ * This avoids repeated saving while opening or closing multiple rows.
+ */
+Zotero.CollectionTreeView.prototype._startRememberOpenStatesTimer = function () {
+	if (this._rememberOpenStatesTimeoutID) {
+		clearTimeout(this._rememberOpenStatesTimeoutID);
+	}
+	this._rememberOpenStatesTimeoutID = setTimeout(() => {
+		this._rememberOpenStates();
+		this._rememberOpenStatesTimeoutID = null;
+	}, 250)
+};
+
+
 Zotero.CollectionTreeView.prototype.isSelectable = function (row, col) {
 	var treeRow = this.getRow(row);
 	switch (treeRow.type) {
@@ -915,7 +932,11 @@ Zotero.CollectionTreeView.prototype.__defineGetter__('editable', function () {
 });
 
 
-Zotero.CollectionTreeView.prototype.expandLibrary = Zotero.Promise.coroutine(function* (libraryID) {
+/**
+ * @param {Integer} libraryID
+ * @param {Boolean} [recursive=false] - Expand all collections and subcollections
+ */
+Zotero.CollectionTreeView.prototype.expandLibrary = Zotero.Promise.coroutine(function* (libraryID, recursive) {
 	var row = this._rowMap['L' + libraryID]
 	if (row === undefined) {
 		return false;
@@ -923,6 +944,15 @@ Zotero.CollectionTreeView.prototype.expandLibrary = Zotero.Promise.coroutine(fun
 	if (!this.isContainerOpen(row)) {
 		yield this.toggleOpenState(row);
 	}
+	
+	if (recursive) {
+		for (let i = row; i < this.rowCount && this.getRow(i).ref.libraryID == libraryID; i++) {
+			if (this.isContainer(i) && !this.isContainerOpen(i)) {
+				yield this.toggleOpenState(i);
+			}
+		}
+	}
+	
 	return true;
 });
 
@@ -932,8 +962,35 @@ Zotero.CollectionTreeView.prototype.collapseLibrary = function (libraryID) {
 	if (row === undefined) {
 		return false;
 	}
-	this._closeContainer(row);
+	
+	var closed = [];
+	var found = false;
+	for (let i = this.rowCount - 1; i >= row; i--) {
+		let treeRow = this.getRow(i);
+		if (treeRow.ref.libraryID !== libraryID) {
+			// Once we've moved beyond the original library, stop looking
+			if (found) {
+				break;
+			}
+			continue;
+		}
+		found = true;
+		
+		if (this.isContainer(i) && this.isContainerOpen(i)) {
+			closed.push(treeRow.id);
+			this._closeContainer(i);
+		}
+	}
+	
+	// Select the collapsed library
 	this.selection.select(row);
+	
+	// We have to manually delete closed rows from the container state object, because otherwise
+	// _rememberOpenStates() wouldn't see any of the rows under the library (since the library is now
+	// collapsed) and they'd remain as open in the persisted object.
+	closed.forEach(id => { delete this._containerState[id]; });
+	this._rememberOpenStates();
+	
 	return true;
 };
 
@@ -1145,7 +1202,7 @@ Zotero.CollectionTreeView.prototype.deleteSelection = Zotero.Promise.coroutine(f
 
 
 /**
- * Expand row based on last state, or manually from toggleOpenState()
+ * Expand row based on last state
  */
 Zotero.CollectionTreeView.prototype._expandRow = Zotero.Promise.coroutine(function* (rows, row, forceOpen) {
 	var treeRow = rows[row];
@@ -1179,8 +1236,7 @@ Zotero.CollectionTreeView.prototype._expandRow = Zotero.Promise.coroutine(functi
 		var showTrash = false;
 	}
 	
-	// If not a manual open and either the library is set to be hidden
-	// or this is a collection that isn't explicitly opened,
+	// If not a manual open and either the library is set to be collapsed or this is a collection that isn't explicitly opened,
 	// set the initial state to closed
 	if (!forceOpen &&
 			(this._containerState[treeRow.id] === false
@@ -1312,6 +1368,9 @@ Zotero.CollectionTreeView.prototype._refreshRowMap = function() {
 }
 
 
+/**
+ * Persist the current open/closed state of rows to a pref
+ */
 Zotero.CollectionTreeView.prototype._rememberOpenStates = Zotero.Promise.coroutine(function* () {
 	var state = this._containerState;
 	

test/tests/collectionTreeViewTest.js

@@ -70,6 +70,52 @@ describe("Zotero.CollectionTreeView", function() {
 		})
 	})
 	
+	describe("#expandLibrary()", function () {
+		var libraryRow, col1, col2, col3;
+		
+		before(function* () {
+			yield cv.selectLibrary(userLibraryID);
+			libraryRow = cv.selection.currentIndex;
+		});
+		
+		beforeEach(function* () {
+			// My Library
+			//   - A
+			//     - B
+			//       - C
+			col1 = yield createDataObject('collection');
+			col2 = yield createDataObject('collection', { parentID: col1.id });
+			col3 = yield createDataObject('collection', { parentID: col2.id });
+		});
+		
+		it("should open a library and respect stored container state", function* () {
+			// Collapse B
+			yield cv.toggleOpenState(cv.getRowIndexByID(col2.collectionTreeViewID));
+			yield cv._rememberOpenStates();
+			
+			// Close and reopen library
+			yield cv.toggleOpenState(libraryRow);
+			yield cv.expandLibrary(userLibraryID);
+			
+			assert.ok(cv.getRowIndexByID(col1.collectionTreeViewID))
+			assert.ok(cv.getRowIndexByID(col2.collectionTreeViewID))
+			assert.isFalse(cv.getRowIndexByID(col3.collectionTreeViewID))
+		});
+		
+		it("should open a library and all subcollections in recursive mode", function* () {
+			yield cv.toggleOpenState(cv.getRowIndexByID(col2.collectionTreeViewID));
+			yield cv._rememberOpenStates();
+			
+			// Close and reopen library
+			yield cv.toggleOpenState(libraryRow);
+			yield cv.expandLibrary(userLibraryID, true);
+			
+			assert.ok(cv.getRowIndexByID(col1.collectionTreeViewID))
+			assert.ok(cv.getRowIndexByID(col2.collectionTreeViewID))
+			assert.ok(cv.getRowIndexByID(col3.collectionTreeViewID))
+		});
+	});
+	
 	describe("#expandToCollection()", function () {
 		it("should expand a collection to a subcollection", function* () {
 			var collection1 = yield createDataObject('collection');