From 6942506eba12f2d18794dadac71c50ccd041affb Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 28 Jun 2024 02:09:26 -0400 Subject: [PATCH] Rework object type checking in items list a532cfb4759 added `isCollection()`, `isSearch()`, and `isItem()` methods to data objects to handle collections and searches in the trash, with `isItem()` checking whether `._ObjectType` was `Item`. That left out feed items (`._ObjectType` == `FeedItem`), and when c384fef86753 made `getSelectedItems()` return only items, it used `isItem()`, so feed items were excluded, which broke feed-item toggling between read and unread [1] and possibly some other things. The simple fix would be to make `isItem` match feed items as well (which could potentially fix other bugs related to feed items), but there was actually no need to add new methods (which can get confused with `CollectionTreeRow` methods) when we can just check the object type with `obj instanceof Zotero.Item`, which gets the benefit of inheritance and matches `Zotero.FeedItem` instances as well. [1] https://forums.zotero.org/discussion/115571/cannot-change-the-status-of-title-in-subscribtion --- chrome/content/zotero/collectionTree.jsx | 4 +- chrome/content/zotero/elements/itemPane.js | 8 +-- chrome/content/zotero/itemTree.jsx | 55 ++++++++++--------- .../zotero/xpcom/data/dataObjectUtilities.js | 9 --- chrome/content/zotero/xpcom/data/feedItems.js | 4 ++ chrome/content/zotero/xpcom/data/item.js | 4 -- chrome/content/zotero/zoteroPane.js | 8 +-- test/tests/itemPaneTest.js | 28 ++++++++++ 8 files changed, 72 insertions(+), 48 deletions(-) diff --git a/chrome/content/zotero/collectionTree.jsx b/chrome/content/zotero/collectionTree.jsx index 0a795a97ae..f76174b8af 100644 --- a/chrome/content/zotero/collectionTree.jsx +++ b/chrome/content/zotero/collectionTree.jsx @@ -905,8 +905,8 @@ var CollectionTree = class CollectionTree extends LibraryTree { await this._addSortedRow('collection', id); await this.selectByID(currentTreeRow.id); // Invalidate parent in case it's become non-empty - let parentRow = this.getRowIndexByID("C" + collection.parentID); - if (parentRow !== false) { + if (collection.parentID) { + let parentRow = this.getRowIndexByID("C" + collection.parentID); this.tree.invalidateRow(parentRow); } } diff --git a/chrome/content/zotero/elements/itemPane.js b/chrome/content/zotero/elements/itemPane.js index 72528eb9f0..73938d0410 100644 --- a/chrome/content/zotero/elements/itemPane.js +++ b/chrome/content/zotero/elements/itemPane.js @@ -113,7 +113,7 @@ let item = this.data[0]; // If a collection or search is selected, it must be in the trash. - if (item.isCollection() || item.isSearch()) { + if (item instanceof Zotero.Collection || item instanceof Zotero.Search) { renderStatus = this.renderMessage(); } else if (item.isNote()) { @@ -228,13 +228,13 @@ // In the trash, we have to check the object type if (this.collectionTreeRow.isTrash()) { let obj = this.data[0]; - if (this.data.every(x => x.isCollection())) { + if (this.data.every(x => x instanceof Zotero.Collection)) { key = 'item-pane-message-collections-selected'; } - else if (this.data.every(x => x.isSearch())) { + else if (this.data.every(x => x instanceof Zotero.Search)) { key = 'item-pane-message-searches-selected'; } - else if (this.data.every(x => x.isItem())) { + else if (this.data.every(x => x instanceof Zotero.Item)) { key = 'item-pane-message-items-selected'; } else { diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index 63fef6dbd1..487065e698 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -224,7 +224,11 @@ var ItemTree = class ItemTree extends LibraryTree { newSearchItems = newSearchItems.filter(item => !item.isAnnotation()); // Remove notes and attachments if necessary if (this.regularOnly) { - newSearchItems = newSearchItems.filter(item => item.isCollection() || item.isRegularItem()); + newSearchItems = newSearchItems.filter((item) => { + return item instanceof Zotero.Collection + || item instanceof Zotero.Search + || item.isRegularItem(); + }); } let newSearchItemIDs = new Set(newSearchItems.map(item => item.id)); // Find the items that aren't yet in the tree @@ -255,7 +259,7 @@ var ItemTree = class ItemTree extends LibraryTree { if (row.level == 0) { // A top-level attachment moved into a parent. Don't copy, it will be added // via this loop for the parent item. - if (row.ref.parentID && row.ref.isItem()) { + if (row.ref instanceof Zotero.Item && row.ref.parentID) { continue; } let isSearchParent = newSearchParentIDs.has(row.ref.treeViewID); @@ -1761,22 +1765,22 @@ var ItemTree = class ItemTree extends LibraryTree { this._refreshRowMap(); this.tree.invalidate(); - // Create an array of selected items - var ids = Array.from(this.selection.selected).filter(index => this.getRow(index).ref.isItem()).map(index => this.getRow(index).id); + let selectedObjects = [...this.selection.selected].map(index => this.getRow(index).ref); + let selectedItems = selectedObjects.filter(o => o instanceof Zotero.Item); + let selectedItemIDs = selectedItems.map(o => o.id); - var collectionTreeRow = this.collectionTreeRow; + let collectionTreeRow = this.collectionTreeRow; if (collectionTreeRow.isBucket()) { collectionTreeRow.ref.deleteItems(ids); } - if (collectionTreeRow.isTrash()) { - let selectedObjects = Array.from(this.selection.selected).map(index => this.getRow(index).ref); + else if (collectionTreeRow.isTrash()) { let [trashedCollectionIDs, trashedSearches] = [[], []]; for (let obj of selectedObjects) { - if (obj.isCollection()) { + if (obj instanceof Zotero.Collection) { trashedCollectionIDs.push(obj.id); } - if (obj.isSearch()) { + if (obj instanceof Zotero.Search) { trashedSearches.push(obj.id); } } @@ -1786,8 +1790,8 @@ var ItemTree = class ItemTree extends LibraryTree { if (trashedSearches.length > 0) { await Zotero.Searches.erase(trashedSearches); } - if (ids.length > 0) { - await Zotero.Items.erase(ids); + if (selectedItemIDs.length > 0) { + await Zotero.Items.erase(selectedItemIDs); } } else if (collectionTreeRow.isLibrary(true) @@ -1796,7 +1800,7 @@ var ItemTree = class ItemTree extends LibraryTree { || collectionTreeRow.isRetracted() || collectionTreeRow.isDuplicates() || force) { - await Zotero.Items.trashTx(ids); + await Zotero.Items.trashTx(selectedItemIDs); } else if (collectionTreeRow.isCollection()) { let collectionIDs = [collectionTreeRow.ref.id]; @@ -1805,8 +1809,7 @@ var ItemTree = class ItemTree extends LibraryTree { } await Zotero.DB.executeTransaction(async () => { - for (let itemID of ids) { - let item = Zotero.Items.get(itemID); + for (let item of selectedItems) { for (let collectionID of collectionIDs) { item.removeFromCollection(collectionID); } @@ -1817,7 +1820,7 @@ var ItemTree = class ItemTree extends LibraryTree { }); } else if (collectionTreeRow.isPublications()) { - await Zotero.Items.removeFromPublications(ids.map(id => Zotero.Items.get(id))); + await Zotero.Items.removeFromPublications(selectedItems); } } finally { @@ -1844,7 +1847,7 @@ var ItemTree = class ItemTree extends LibraryTree { * Get selected items, omitting collections and searches in the trash */ getSelectedItems(asIDs) { - var items = this.getSelectedObjects().filter(x => x.isItem()); + var items = this.getSelectedObjects().filter(o => o instanceof Zotero.Item); return asIDs ? items.map(x => x.id) : items; } @@ -2821,10 +2824,10 @@ var ItemTree = class ItemTree extends LibraryTree { try { // Special treatment for trashed collections or searches since they are not an actual // item and do not have an item type - if (item.isSearch()) { + if (item instanceof Zotero.Collection) { itemTypeAriaLabel = Zotero.getString('searchConditions.collection') + '.'; } - else if (item.isCollection()) { + else if (item instanceof Zotero.Search) { itemTypeAriaLabel = Zotero.getString('searchConditions.savedSearch') + '.'; } else { @@ -3181,10 +3184,13 @@ var ItemTree = class ItemTree extends LibraryTree { return this._rowCache[itemID]; } - let row = {}; + let row = { + // Not a collection or search in the trash + isItem: treeRow.ref instanceof Zotero.Item + }; // Mark items not matching search as context rows, displayed in gray - if (this._searchMode && !this._searchItemIDs.has(itemID) && treeRow.ref.isItem()) { + if (row.isItem && this._searchMode && !this._searchItemIDs.has(itemID)) { row.contextRow = true; } @@ -3201,7 +3207,7 @@ var ItemTree = class ItemTree extends LibraryTree { row.unread = true; } - if (!(treeRow.ref.isCollection() || treeRow.ref.isSearch())) { + if (!(treeRow.ref instanceof Zotero.Collection || treeRow.ref instanceof Zotero.Search)) { row.itemType = Zotero.ItemTypes.getLocalizedString(treeRow.ref.itemTypeID); } // Year column is just date field truncated @@ -3257,7 +3263,6 @@ var ItemTree = class ItemTree extends LibraryTree { } row[key] = val; } - row.isItem = treeRow.ref.isItem(); return this._rowCache[itemID] = row; } @@ -3833,12 +3838,12 @@ var ItemTree = class ItemTree extends LibraryTree { var item = this.getRow(index).ref; // Non-item objects that can be appear in the trash - if (item.isCollection() || item.isSearch()) { + if (item instanceof Zotero.Collection || item instanceof Zotero.Search) { let icon; - if (item.isCollection()) { + if (item instanceof Zotero.Collection) { icon = getCSSIcon('collection'); } - else if (item.isSearch()) { + else if (item instanceof Zotero.Search) { icon = getCSSIcon('search'); } icon.classList.add('icon-item-type'); diff --git a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js index 9edd1c20e8..bc4ec601e2 100644 --- a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js +++ b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js @@ -734,9 +734,6 @@ Zotero.DataObjectUtilities = { * Most of these are overriden by Zotero.Item. */ itemTreeMockProperties: { - isCollection: function () { - return this._ObjectType == "Collection"; - }, isAnnotation: () => false, isNote: () => false, numNotes: () => 0, @@ -744,16 +741,10 @@ Zotero.DataObjectUtilities = { numAttachments: () => false, getColoredTags: () => false, isRegularItem: () => false, // Should be false to prevent items dropped into deleted searches - isSearch: function () { - return this._ObjectType == "Search"; - }, getNotes: () => [], getAttachments: () => [], isFileAttachment: () => false, isTopLevelItem: () => false, - isItem: function () { - return this._ObjectType == "Item"; - }, getField: function (field, _) { return this['_' + field] || ""; }, diff --git a/chrome/content/zotero/xpcom/data/feedItems.js b/chrome/content/zotero/xpcom/data/feedItems.js index 3771bbb58f..08cafe3c9b 100644 --- a/chrome/content/zotero/xpcom/data/feedItems.js +++ b/chrome/content/zotero/xpcom/data/feedItems.js @@ -133,6 +133,10 @@ Zotero.FeedItems = new Proxy(function() { ids = [ids]; } + if (!ids.length) { + throw new Error("No ids passed"); + } + let items = yield this.getAsync(ids); if (state == undefined) { diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index bd8ffe520a..6d5714638b 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -4994,10 +4994,6 @@ Zotero.Item.prototype._eraseData = Zotero.Promise.coroutine(function* (env) { }); -Zotero.Item.prototype.isCollection = function() { - return false; -} - /** * Populate the object's data from an API JSON data object * diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 290be17a7d..e5d1bdfd56 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -1232,8 +1232,8 @@ var ZoteroPane = new function() // If no items or an unreasonable number, don't try if (!objects.length || objects.length > 100) return; - var collections = objects.filter(o => o.isCollection()); - var items = objects.filter(o => o.isItem()); + var collections = objects.filter(o => o instanceof Zotero.Collection); + var items = objects.filter(o => o instanceof Zotero.Item); // Get parent collections of collections var toHighlight = []; @@ -2421,7 +2421,7 @@ var ZoteroPane = new function() let parent = this.itemsView.getRow(row).ref; let childIDs = []; let subcollections = []; - if (parent.isCollection()) { + if (parent instanceof Zotero.Collection) { // If the restored item is a collection, restore its subcollections too if (isSelected(parent)) { subcollections = parent.getDescendents(false, 'collection', true).map(col => col.id); @@ -3924,7 +3924,7 @@ var ZoteroPane = new function() menu.childNodes[m.showInLibrary].setAttribute('label', Zotero.getString('general.showInLibrary')); } // For collections and search, only keep restore/delete options - if (items.some(item => item.isCollection() || item.isSearch())) { + if (items.some(item => item instanceof Zotero.Collection || item instanceof Zotero.Search)) { for (let option of options) { if (!['restoreToLibrary', 'deleteFromLibrary'].includes(option)) { show.delete(m[option]); diff --git a/test/tests/itemPaneTest.js b/test/tests/itemPaneTest.js index 32afea71bb..2f8e3d73bf 100644 --- a/test/tests/itemPaneTest.js +++ b/test/tests/itemPaneTest.js @@ -1215,6 +1215,34 @@ describe("Item pane", function () { describe("Feed buttons", function() { describe("Mark as Read/Unread", function() { + it("should change an item from unread to read", async function () { + var feed = await createFeed(); + await select(win, feed); + + var item = await createDataObject('feedItem', { libraryID: feed.libraryID }); + + // Skip timed mark-as-read + var stub = sinon.stub(win.ZoteroPane, 'startItemReadTimeout'); + await select(win, item); + + // Click "Mark as Read" + var promise = waitForItemEvent('modify'); + var button = ZoteroPane.itemPane.getCurrentPane().querySelector('.feed-item-toggleRead-button'); + assert.equal(button.label, Zotero.getString('pane.item.markAsRead')); + assert.isFalse(item.isRead); + button.click(); + var ids = await promise; + + assert.sameMembers(ids, [item.id]); + assert.isTrue(item.isRead); + // Button is re-created + button = ZoteroPane.itemPane.getCurrentPane().querySelector('.feed-item-toggleRead-button'); + assert.equal(button.label, Zotero.getString('pane.item.markAsUnread')); + + stub.restore(); + }); + + it("should update label when state of an item changes", function* () { let feed = yield createFeed(); yield selectLibrary(win, feed.libraryID);