diff --git a/chrome/content/zotero/xpcom/collectionTreeView.js b/chrome/content/zotero/xpcom/collectionTreeView.js index c0a35b979c..c2b2c89d5b 100644 --- a/chrome/content/zotero/xpcom/collectionTreeView.js +++ b/chrome/content/zotero/xpcom/collectionTreeView.js @@ -59,8 +59,7 @@ Zotero.CollectionTreeView = function() 25 ); this._containerState = {}; - this._duplicateLibraries = []; - this._unfiledLibraries = []; + this._virtualCollectionLibraries = {}; 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) { - // 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(); - } - + this.selectAfterRowRemoval(selectedIndex); } 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 - && this._unfiledLibraries.indexOf(libraryID) == -1 + // Duplicate Items not shown + && (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 - && this._duplicateLibraries.indexOf(libraryID) != -1); - var showUnfiled = this._unfiledLibraries.indexOf(libraryID) != -1; + // Virtual collections default to showing if not explicitly hidden + var showDuplicates = this.hideSources.indexOf('duplicates') == -1 + && this._virtualCollectionLibraries.duplicates[libraryID] !== false; + var showUnfiled = this._virtualCollectionLibraries.unfiled[libraryID] !== false; var showTrash = this.hideSources.indexOf('trash') == -1; } else { diff --git a/chrome/content/zotero/xpcom/libraryTreeView.js b/chrome/content/zotero/xpcom/libraryTreeView.js index 00f3b1c06e..9dd5cd89de 100644 --- a/chrome/content/zotero/xpcom/libraryTreeView.js +++ b/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; } diff --git a/chrome/content/zotero/xpcom/utilities_internal.js b/chrome/content/zotero/xpcom/utilities_internal.js index 31fffa03fa..6c7e39f0f7 100644 --- a/chrome/content/zotero/xpcom/utilities_internal.js +++ b/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] diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index f3788786be..e7d83ed15a 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -941,68 +941,42 @@ var ZoteroPane = new function() }); - this.setVirtual = Zotero.Promise.coroutine(function* (libraryID, mode, show) { - switch (mode) { + this.setVirtual = Zotero.Promise.coroutine(function* (libraryID, type, show) { + switch (type) { case 'duplicates': - var prefKey = 'duplicateLibraries'; - var lastViewedFolderID = 'D' + libraryID; + var treeViewID = 'D' + libraryID; break; case 'unfiled': - var prefKey = 'unfiledLibraries'; - var lastViewedFolderID = 'U' + libraryID; + var treeViewID = 'U' + libraryID; break; default: - throw new Error("Invalid virtual mode '" + mode + "'"); + throw new Error("Invalid virtual collection type '" + type + "'"); } - try { - var ids = Zotero.Prefs.get(prefKey).split(','); - } - catch (e) { - var ids = []; - } + Zotero.Utilities.Internal.setVirtualCollectionStateForLibrary(libraryID, type, show); - var newids = []; - 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); - } + var cv = this.collectionsView; - // Add the current library if it's not already set - if (show && newids.indexOf(libraryID) == -1) { - newids.push(libraryID); - } + var deferred = Zotero.Promise.defer(); + cv.addEventListener('select', () => deferred.resolve()); + var selectedRow = cv.selection.currentIndex; - newids.sort(); - - Zotero.Prefs.set(prefKey, newids.join()); - - yield this.collectionsView.refresh(); + yield cv.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; }); diff --git a/test/tests/collectionTreeViewTest.js b/test/tests/collectionTreeViewTest.js index b1b05558c4..c1ee3fba53 100644 --- a/test/tests/collectionTreeViewTest.js +++ b/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('unfiledLibraries', ""); + Zotero.Prefs.set('duplicateLibraries', `{"${userLibraryID}": false}`); + 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 - assert.equal(cv.rowCount, originalRowCount + 7); + // Group, collections, Duplicates, Unfiled, and trash + assert.equal(cv.rowCount, originalRowCount + 9); var spy = sinon.spy(cv, "refresh"); try { diff --git a/test/tests/zoteroPaneTest.js b/test/tests/zoteroPaneTest.js index a52ed226b2..40865d29ce 100644 --- a/test/tests/zoteroPaneTest.js +++ b/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 - Zotero.Prefs.set('duplicateLibraries', ""); - Zotero.Prefs.set('unfiledLibraries', ""); + // Start hidden (tested in collectionTreeViewTest) + Zotero.Prefs.set('duplicateLibraries', `{"${userLibraryID}": false}`); + 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* () { - // Start shown - Zotero.Prefs.set('duplicateLibraries', "" + userLibraryID); - Zotero.Prefs.set('unfiledLibraries', "" + userLibraryID); + it("should hide a virtual collection in a group", function* () { yield cv.refresh(); - // Hide Duplicate Items - var id = "D" + userLibraryID; - assert.ok(yield cv.selectByID(id)); - yield waitForItemsLoad(win); - yield zp.setVirtual(userLibraryID, 'duplicates', false); - assert.isFalse(cv.getRowIndexByID(id)); - assert.equal(cv.getSelectedLibraryID(), userLibraryID); + var group = yield createGroup(); + var groupRow = cv.getRowIndexByID(group.treeViewID); + var rowCount = cv.rowCount; + // 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); - yield zp.setVirtual(userLibraryID, 'unfiled', false); + // Hide Unfiled Items + 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); }); });