Show "Duplicate Items" and "Unfiled Items" for all libraries by default

Previously they only showed for My Library by default, which I suspect
meant that most people didn't know you could get them for other
libraries...

This hides "Duplicate Items" and "Unfiled Items" from the context menu
when they're active, which may or may not be desirable (but we don't
show, say, "Trash" in the context menu).

Also tweaks selection behavior after hide to select next appropriate row
instead of the parent library.

chrome/content/zotero/xpcom/collectionTreeView.js

@@ -59,8 +59,7 @@ Zotero.CollectionTreeView = function()
 		25
 	);
 	this._containerState = {};
-	this._duplicateLibraries = [];
+	this._virtualCollectionLibraries = {};
-	this._unfiledLibraries = [];
 	this._trashNotEmpty = {};
 }
 
@@ -163,26 +162,12 @@ Zotero.CollectionTreeView.prototype.refresh = Zotero.Promise.coroutine(function*
 	
 	var userLibraryID = Zotero.Libraries.userLibraryID;
 	
-	var readPref = function (pref) {
-		let ids = Zotero.Prefs.get(pref);
-		if (ids === "") {
-			this["_" + pref] = [];
-		}
-		else {
-			if (ids === undefined || typeof ids != 'string') {
-				ids = "" + userLibraryID;
-				Zotero.Prefs.set(pref, "" + userLibraryID);
-			}
-			this["_" + pref] = ids.split(',')
-				// Convert old id and convert to int
-				.map(id => id === "0" ? userLibraryID : parseInt(id));
-		}
-	}.bind(this);
-	
 	if (this.hideSources.indexOf('duplicates') == -1) {
-		readPref('duplicateLibraries');
+		this._virtualCollectionLibraries.duplicates =
+			Zotero.Utilities.Internal.getVirtualCollectionState('duplicates')
 	}
-	readPref('unfiledLibraries');
+	this._virtualCollectionLibraries.unfiled =
+			Zotero.Utilities.Internal.getVirtualCollectionState('unfiled')
 	
 	var oldCount = this.rowCount || 0;
 	var newRows = [];
@@ -372,7 +357,7 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function*
 	}
 	
 	if (action == 'delete') {
-		var selectedIndex = this.selection.count ? this.selection.currentIndex : 0;
+		let selectedIndex = this.selection.count ? this.selection.currentIndex : 0;
 		let refreshFeeds = false;
 		
 		// Since a delete involves shifting of rows, we have to do it in reverse order
@@ -432,27 +417,7 @@ Zotero.CollectionTreeView.prototype.notify = Zotero.Promise.coroutine(function*
 			this._refreshRowMap();
 		}
 		
-		if (!this.selection.count) {
+		this.selectAfterRowRemoval(selectedIndex);
-			// If last row was selected, stay on the last row
-			if (selectedIndex >= this.rowCount) {
-				selectedIndex = this.rowCount - 1;
-			};
-			this.selection.select(selectedIndex)
-		}
-		
-		// Make sure the selection doesn't land on a separator (e.g. deleting last feed)
-		let index = this.selection.currentIndex;
-		while (index >= 0 && !this.isSelectable(index)) {
-			// move up, since we got shifted down
-			index--;
-		}
-		
-		if (index >= 0) {
-			this.selection.select(index);
-		} else {
-			this.selection.clearSelection();
-		}
-		
 	}
 	else if (action == 'modify') {
 		let row;
@@ -829,8 +794,11 @@ Zotero.CollectionTreeView.prototype.isContainerEmpty = function(row)
 		
 		return !treeRow.ref.hasCollections()
 				&& !treeRow.ref.hasSearches()
-				&& this._duplicateLibraries.indexOf(libraryID) == -1
+				// Duplicate Items not shown
-				&& this._unfiledLibraries.indexOf(libraryID) == -1
+				&& (this.hideSources.indexOf('duplicates') != -1
+					|| this._virtualCollectionLibraries.duplicates[libraryID] === false)
+				// Unfiled Items not shown
+				&& this._virtualCollectionLibraries.unfiled[libraryID] === false
 				&& this.hideSources.indexOf('trash') != -1;
 	}
 	if (treeRow.isCollection()) {
@@ -1062,19 +1030,24 @@ Zotero.CollectionTreeView.prototype.expandToCollection = Zotero.Promise.coroutin
 ////////////////////////////////////////////////////////////////////////////////
 Zotero.CollectionTreeView.prototype.selectByID = Zotero.Promise.coroutine(function* (id) {
 	var type = id[0];
-	id = ('' + id).substr(1);
+	id = parseInt(('' + id).substr(1));
 	
 	switch (type) {
 	case 'L':
 		return yield this.selectLibrary(id);
 	
 	case 'C':
-		var found = yield this.expandToCollection(id);
+		yield this.expandToCollection(id);
 		break;
-		
+	
 	case 'S':
 		var search = yield Zotero.Searches.getAsync(id);
-		var found = yield this.expandLibrary(search.libraryID);
+		yield this.expandLibrary(search.libraryID);
+		break;
+	
+	case 'D':
+	case 'U':
+		yield this.expandLibrary(id);
 		break;
 	
 	case 'T':
@@ -1216,6 +1189,22 @@ Zotero.CollectionTreeView.prototype.deleteSelection = Zotero.Promise.coroutine(f
 });
 
 
+Zotero.CollectionTreeView.prototype.selectAfterRowRemoval = function (row) {
+	// If last row was selected, stay on the last row
+	if (row >= this.rowCount) {
+		row = this.rowCount - 1;
+	};
+	
+	// Make sure the selection doesn't land on a separator (e.g. deleting last feed)
+	while (row >= 0 && !this.isSelectable(row)) {
+		// move up, since we got shifted down
+		row--;
+	}
+	
+	this.selection.select(row);
+};
+
+
 /**
  * Expand row based on last state
  */
@@ -1239,9 +1228,10 @@ Zotero.CollectionTreeView.prototype._expandRow = Zotero.Promise.coroutine(functi
 	
 	if (isLibrary) {
 		var savedSearches = yield Zotero.Searches.getAll(libraryID);
-		var showDuplicates = (this.hideSources.indexOf('duplicates') == -1
+		// Virtual collections default to showing if not explicitly hidden
-				&& this._duplicateLibraries.indexOf(libraryID) != -1);
+		var showDuplicates = this.hideSources.indexOf('duplicates') == -1
-		var showUnfiled = this._unfiledLibraries.indexOf(libraryID) != -1;
+				&& this._virtualCollectionLibraries.duplicates[libraryID] !== false;
+		var showUnfiled = this._virtualCollectionLibraries.unfiled[libraryID] !== false;
 		var showTrash = this.hideSources.indexOf('trash') == -1;
 	}
 	else {

chrome/content/zotero/xpcom/libraryTreeView.js

@@ -134,7 +134,6 @@ Zotero.LibraryTreeView.prototype = {
 			return;
 		}
 		var row = this.getRowIndexByID(scrollPosition.id);
-		Zotero.debug(scrollPosition.id);
 		if (row === false) {
 			return;
 		}

chrome/content/zotero/xpcom/utilities_internal.js

@@ -1053,7 +1053,70 @@ Zotero.Utilities.Internal = {
 		elem.appendChild(menu);
 		return menu;
 	},
-
+	
+	
+	// TODO: Move somewhere better
+	getVirtualCollectionState: function (type) {
+		switch (type) {
+			case 'duplicates':
+				var prefKey = 'duplicateLibraries';
+				break;
+			
+			case 'unfiled':
+				var prefKey = 'unfiledLibraries';
+				break;
+			
+			default:
+				throw new Error("Invalid virtual collection type '" + type + "'");
+		}
+		var libraries;
+		try {
+			libraries = JSON.parse(Zotero.Prefs.get(prefKey) || '{}');
+			if (typeof libraries != 'object') {
+				throw true;
+			}
+		}
+		// Ignore old/incorrect formats
+		catch (e) {
+			Zotero.Prefs.clear(prefKey);
+			libraries = {};
+		}
+		
+		return libraries;
+	},
+	
+	
+	getVirtualCollectionStateForLibrary: function (libraryID, type) {
+		return this.getVirtualCollectionState(type)[libraryID] !== false;
+	},
+	
+	
+	setVirtualCollectionStateForLibrary: function (libraryID, type, show) {
+		switch (type) {
+			case 'duplicates':
+				var prefKey = 'duplicateLibraries';
+				break;
+			
+			case 'unfiled':
+				var prefKey = 'unfiledLibraries';
+				break;
+			
+			default:
+				throw new Error("Invalid virtual collection type '" + type + "'");
+		}
+		
+		var libraries = this.getVirtualCollectionState(type);
+		
+		// Update current library
+		libraries[libraryID] = !!show;
+		// Remove libraries that don't exist or that are set to true
+		for (let id of Object.keys(libraries).filter(id => libraries[id] || !Zotero.Libraries.exists(id))) {
+			delete libraries[id];
+		}
+		Zotero.Prefs.set(prefKey, JSON.stringify(libraries));
+	},
+	
+	
 	/**
 	 * Quits Zotero, optionally restarting.
 	 * @param {Boolean} [restart=false]

chrome/content/zotero/zoteroPane.js

@@ -941,68 +941,42 @@ var ZoteroPane = new function()
 	});
 	
 	
-	this.setVirtual = Zotero.Promise.coroutine(function* (libraryID, mode, show) {
+	this.setVirtual = Zotero.Promise.coroutine(function* (libraryID, type, show) {
-		switch (mode) {
+		switch (type) {
 			case 'duplicates':
-				var prefKey = 'duplicateLibraries';
+				var treeViewID = 'D' + libraryID;
-				var lastViewedFolderID = 'D' + libraryID;
 				break;
 			
 			case 'unfiled':
-				var prefKey = 'unfiledLibraries';
+				var treeViewID = 'U' + libraryID;
-				var lastViewedFolderID = 'U' + libraryID;
 				break;
 			
 			default:
-				throw new Error("Invalid virtual mode '" + mode + "'");
+				throw new Error("Invalid virtual collection type '" + type + "'");
 		}
 		
-		try {
+		Zotero.Utilities.Internal.setVirtualCollectionStateForLibrary(libraryID, type, show);
-			var ids = Zotero.Prefs.get(prefKey).split(',');
-		}
-		catch (e) {
-			var ids = [];
-		}
 		
-		var newids = [];
+		var cv = this.collectionsView;
-		for (let i = 0; i < ids.length; i++) {
-			let id = ids[i];
-			id = parseInt(id);
-			if (isNaN(id)) {
-				continue;
-			}
-			// Remove current library if hiding
-			if (id == libraryID && !show) {
-				continue;
-			}
-			// Remove libraries that no longer exist
-			if (!Zotero.Libraries.exists(id)) {
-				continue;
-			}
-			newids.push(id);
-		}
 		
-		// Add the current library if it's not already set
+		var deferred = Zotero.Promise.defer();
-		if (show && newids.indexOf(libraryID) == -1) {
+		cv.addEventListener('select', () => deferred.resolve());
-			newids.push(libraryID);
+		var selectedRow = cv.selection.currentIndex;
-		}
 		
-		newids.sort();
+		yield cv.refresh();
-		
-		Zotero.Prefs.set(prefKey, newids.join());
-		
-		yield this.collectionsView.refresh();
 		
 		// Select new row
 		if (show) {
-			yield this.collectionsView.selectByID(lastViewedFolderID);
+			yield this.collectionsView.selectByID(treeViewID);
 		}
-		// Select library root when hiding
+		// Select next appropriate row after removal
 		else {
-			yield this.collectionsView.selectLibrary(libraryID);
+			this.collectionsView.selectAfterRowRemoval(selectedRow);
 		}
 		
 		this.collectionsView.selection.selectEventsSuppressed = false;
+		
+		return deferred.promise;
 	});
 	
 	

test/tests/collectionTreeViewTest.js

@@ -18,24 +18,20 @@ describe("Zotero.CollectionTreeView", function() {
 	});
 	
 	describe("#refresh()", function () {
-		it("should show Duplicate Items and Unfiled Items in My Library by default", function* () {
+		it("should show Duplicate Items and Unfiled Items by default", function* () {
 			Zotero.Prefs.clear('duplicateLibraries');
 			Zotero.Prefs.clear('unfiledLibraries');
 			yield cv.refresh();
 			assert.ok(cv.getRowIndexByID("D" + userLibraryID));
 			assert.ok(cv.getRowIndexByID("U" + userLibraryID));
-			assert.equal(Zotero.Prefs.get('duplicateLibraries'), "" + userLibraryID);
-			assert.equal(Zotero.Prefs.get('unfiledLibraries'), "" + userLibraryID);
 		});
 		
 		it("shouldn't show Duplicate Items and Unfiled Items if hidden", function* () {
-			Zotero.Prefs.set('duplicateLibraries', "");
+			Zotero.Prefs.set('duplicateLibraries', `{"${userLibraryID}": false}`);
-			Zotero.Prefs.set('unfiledLibraries', "");
+			Zotero.Prefs.set('unfiledLibraries', `{"${userLibraryID}": false}`);
 			yield cv.refresh();
 			assert.isFalse(cv.getRowIndexByID("D" + userLibraryID));
 			assert.isFalse(cv.getRowIndexByID("U" + userLibraryID));
-			assert.strictEqual(Zotero.Prefs.get('duplicateLibraries'), "");
-			assert.strictEqual(Zotero.Prefs.get('unfiledLibraries'), "");
 		});
 		
 		it("should maintain open state of group", function* () {
@@ -390,8 +386,8 @@ describe("Zotero.CollectionTreeView", function() {
 			yield createDataObject('collection', { libraryID: group.libraryID });
 			yield createDataObject('collection', { libraryID: group.libraryID });
 			
-			// Group, collections, and trash
+			// Group, collections, Duplicates, Unfiled, and trash
-			assert.equal(cv.rowCount, originalRowCount + 7);
+			assert.equal(cv.rowCount, originalRowCount + 9);
 			
 			var spy = sinon.spy(cv, "refresh");
 			try {

test/tests/zoteroPaneTest.js

@@ -216,21 +216,25 @@ describe("ZoteroPane", function() {
 			return selectLibrary(win);
 		})
 		
-		it("should show a hidden virtual folder", function* () {
+		it("should show a hidden virtual collection in My Library", function* () {
 			// Create unfiled, duplicate items
 			var title = Zotero.Utilities.randomString();
 			var item1 = yield createDataObject('item', { title });
 			var item2 = yield createDataObject('item', { title });
 			
-			// Start hidden
+			// Start hidden (tested in collectionTreeViewTest)
-			Zotero.Prefs.set('duplicateLibraries', "");
+			Zotero.Prefs.set('duplicateLibraries', `{"${userLibraryID}": false}`);
-			Zotero.Prefs.set('unfiledLibraries', "");
+			Zotero.Prefs.set('unfiledLibraries', `{"${userLibraryID}": false}`);
 			yield cv.refresh();
 			
 			// Show Duplicate Items
 			var id = "D" + userLibraryID;
 			assert.isFalse(cv.getRowIndexByID(id));
 			yield zp.setVirtual(userLibraryID, 'duplicates', true);
+			// Duplicate Items should be selected
+			assert.equal(cv.selectedTreeRow.id, id);
+			// Should be missing from pref
+			assert.isUndefined(JSON.parse(Zotero.Prefs.get('duplicateLibraries'))[userLibraryID])
 			
 			// Clicking should select both items
 			var row = cv.getRowIndexByID(id);
@@ -247,10 +251,33 @@ describe("ZoteroPane", function() {
 			id = "U" + userLibraryID;
 			assert.isFalse(cv.getRowIndexByID(id));
 			yield zp.setVirtual(userLibraryID, 'unfiled', true);
+			// Unfiled Items should be selected
+			assert.equal(cv.selectedTreeRow.id, id);
+			// Should be missing from pref
+			assert.isUndefined(JSON.parse(Zotero.Prefs.get('unfiledLibraries'))[userLibraryID])
+		});
+		
+		it("should expand library if collapsed when showing virtual collection", function* () {
+			// Start hidden (tested in collectionTreeViewTest)
+			Zotero.Prefs.set('duplicateLibraries', `{"${userLibraryID}": false}`);
+			yield cv.refresh();
+			
+			var libraryRow = cv.getRowIndexByID(Zotero.Libraries.userLibrary.treeViewID);
+			if (cv.isContainerOpen(libraryRow)) {
+				yield cv.toggleOpenState(libraryRow);
+				cv._saveOpenStates();
+			}
+			
+			// Show Duplicate Items
+			var id = "D" + userLibraryID;
+			yield zp.setVirtual(userLibraryID, 'duplicates', true);
+			
+			// Library should have been expanded and Duplicate Items selected
 			assert.ok(cv.getRowIndexByID(id));
+			assert.equal(cv.selectedTreeRow.id, id);
 		});
 		
-		it("should hide a virtual folder shown by default", function* () {
+		it("should hide a virtual collection in My Library", function* () {
 			yield cv.refresh();
 			
 			// Hide Duplicate Items
@@ -258,36 +285,62 @@ describe("ZoteroPane", function() {
 			assert.ok(yield cv.selectByID(id));
 			yield zp.setVirtual(userLibraryID, 'duplicates', false);
 			assert.isFalse(cv.getRowIndexByID(id));
+			assert.isFalse(JSON.parse(Zotero.Prefs.get('duplicateLibraries'))[userLibraryID])
 			
 			// Hide Unfiled Items
 			id = "U" + userLibraryID;
 			assert.ok(yield cv.selectByID(id));
 			yield zp.setVirtual(userLibraryID, 'unfiled', false);
 			assert.isFalse(cv.getRowIndexByID(id));
+			assert.isFalse(JSON.parse(Zotero.Prefs.get('unfiledLibraries'))[userLibraryID])
 		});
 		
-		it("should hide an explicitly shown virtual folder", function* () {
+		it("should hide a virtual collection in a group", function* () {
-			// Start shown
-			Zotero.Prefs.set('duplicateLibraries', "" + userLibraryID);
-			Zotero.Prefs.set('unfiledLibraries', "" + userLibraryID);
 			yield cv.refresh();
 			
-			// Hide Duplicate Items
+			var group = yield createGroup();
-			var id = "D" + userLibraryID;
+			var groupRow = cv.getRowIndexByID(group.treeViewID);
-			assert.ok(yield cv.selectByID(id));
+			var rowCount = cv.rowCount;
-			yield waitForItemsLoad(win);
-			yield zp.setVirtual(userLibraryID, 'duplicates', false);
-			assert.isFalse(cv.getRowIndexByID(id));
-			assert.equal(cv.getSelectedLibraryID(), userLibraryID);
 			
+			// Make sure group is open
+			if (!cv.isContainerOpen(groupRow)) {
+				yield cv.toggleOpenState(groupRow);
+			}
+			
+			// Make sure Duplicate Items is showing
+			var id = "D" + group.libraryID;
+			assert.ok(cv.getRowIndexByID(id));
+			
+			// Hide Duplicate Items
+			assert.ok(yield cv.selectByID(id));
+			yield zp.setVirtual(group.libraryID, 'duplicates', false);
+			// Row should have been removed
+			assert.isFalse(cv.getRowIndexByID(id));
+			// Pref should have been updated
+			Zotero.debug(Zotero.Prefs.get('duplicateLibraries'));
+			assert.isFalse(JSON.parse(Zotero.Prefs.get('duplicateLibraries'))[group.libraryID]);
+			// Group row shouldn't have changed
+			assert.equal(cv.getRowIndexByID(group.treeViewID), groupRow);
+			// Group should remain open
+			assert.isTrue(cv.isContainerOpen(groupRow));
+			// Row count should be 1 less
+			assert.equal(cv.rowCount, --rowCount);
 			
 			// Hide Unfiled Items
-			id = "U" + userLibraryID;
+			id = "U" + group.libraryID;
 			assert.ok(yield cv.selectByID(id));
-			yield waitForItemsLoad(win);
+			// Hide Unfiled Items
-			yield zp.setVirtual(userLibraryID, 'unfiled', false);
+			yield zp.setVirtual(group.libraryID, 'unfiled', false);
+			// Row should have been removed
 			assert.isFalse(cv.getRowIndexByID(id));
-			assert.equal(cv.getSelectedLibraryID(), userLibraryID);
+			// Pref should have been udpated
+			assert.isFalse(JSON.parse(Zotero.Prefs.get('unfiledLibraries'))[group.libraryID]);
+			// Group row shouldn't have changed
+			assert.equal(cv.getRowIndexByID(group.treeViewID), groupRow);
+			// Group should remain open
+			assert.isTrue(cv.isContainerOpen(groupRow));
+			// Row count should be 1 less
+			assert.equal(cv.rowCount, --rowCount);
 		});
 	});