From 3481def4f6b6eebd2d8a833805f586a92f4f7996 Mon Sep 17 00:00:00 2001 From: Abe Jellinek Date: Mon, 6 Jun 2022 22:32:27 -0700 Subject: [PATCH] Fix sorting by attachment (#2586) --- chrome/content/zotero/itemTree.jsx | 131 +++++++++-------------- chrome/content/zotero/xpcom/data/item.js | 6 +- test/tests/itemTest.js | 9 ++ 3 files changed, 61 insertions(+), 85 deletions(-) diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index 7a22cb5c3e..1b4b5ff483 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -1261,7 +1261,7 @@ var ItemTree = class ItemTree extends LibraryTree { sortFields.forEach(x => cache[x] = {}); // Get the display field for a row (which might be a placeholder title) - function getField(field, row) { + let getField = (field, row) => { var item = row.ref; switch (field) { @@ -1269,23 +1269,12 @@ var ItemTree = class ItemTree extends LibraryTree { return Zotero.Items.getSortTitle(item.getDisplayTitle()); case 'hasAttachment': - if (item.isFileAttachment()) { - var state = item.fileExistsCached() ? 1 : -1; - } - else if (item.isRegularItem()) { - var state = item.getBestAttachmentStateCached(); + if (this._canGetBestAttachmentState(item)) { + return item.getBestAttachmentStateCached(); } else { return 0; } - // Make sort order present, missing, empty when ascending - if (state === 1) { - state = 2; - } - else if (state === -1) { - state = 1; - } - return state; case 'numNotes': return row.numNotes(false, true) || 0; @@ -1349,7 +1338,11 @@ var ItemTree = class ItemTree extends LibraryTree { } if (sortField == 'hasAttachment') { - return fieldB - fieldA; + // PDFs at the top + const order = ['pdf', 'snapshot', 'other', 'none']; + fieldA = order.indexOf(fieldA.type || 'none') + (fieldA.exists ? 0 : 4); + fieldB = order.indexOf(fieldB.type || 'none') + (fieldB.exists ? 0 : 4); + return fieldA - fieldB; } if (sortField == 'callNumber') { @@ -2748,93 +2741,60 @@ var ItemTree = class ItemTree extends LibraryTree { // TEMP: For now, we use the blue bullet for all non-PDF attachments, but there's // commented-out code for showing different icons for snapshots, files, and URL/DOI links - if (this.isContainer(index)) { - if (item.isRegularItem()) { - const { type, exists } = item.getBestAttachmentStateCached(); - let icon = ""; - let ariaLabel; - // If the item has a child attachment - if (type !== null && type != 'none') { - if (type == 'pdf') { - icon = getDOMElement('IconTreeitemAttachmentPDF'); - ariaLabel = Zotero.getString('pane.item.attachments.hasPDF'); - if (!exists) { - icon.classList.add('icon-missing-file'); - } - } - else if (type == 'snapshot') { - //icon = getDOMElement('IconTreeitemAttachmentSnapshot'); - icon = exists ? getDOMElement('IconBulletBlue') : getDOMElement('IconBulletBlueEmpty'); - ariaLabel = Zotero.getString('pane.item.attachments.hasSnapshot'); - } - else { - //icon = getDOMElement('IconTreeitem'); - icon = exists ? getDOMElement('IconBulletBlue') : getDOMElement('IconBulletBlueEmpty'); - ariaLabel = Zotero.getString('pane.item.attachments.has'); - } - icon.classList.add('cell-icon'); - //if (!exists) { - // icon.classList.add('icon-missing-file'); - //} - } - //else if (type == 'none') { - // if (item.getField('url') || item.getField('DOI')) { - // icon = getDOMElement('IconLink'); - // ariaLabel = Zotero.getString('pane.item.attachments.hasLink'); - // icon.classList.add('cell-icon'); - // } - //} - if (ariaLabel) { - icon.setAttribute('aria-label', ariaLabel + '.'); - span.setAttribute('title', ariaLabel); - } - span.append(icon); - - // Don't run this immediately since it might cause a db check and disk access - // but delay for some time and see if the item is still visible in the tree - // (i.e. if we haven't scrolled right past it) - setTimeout(() => { - if (!this.tree.rowIsVisible(index)) return; - item.getBestAttachmentState() - // Refresh cell when promise is fulfilled - .then(({ type: newType, exists: newExists }) => { - if (newType !== type || newExists !== exists) { - this.tree.invalidateRow(index); - } - }); - }, ATTACHMENT_STATE_LOAD_DELAY); - } - } - - if (item.isFileAttachment()) { - const exists = item.fileExistsCached(); + if (this._canGetBestAttachmentState(item)) { + const { type, exists } = item.getBestAttachmentStateCached(); let icon = ""; - if (exists !== null) { - if (item.isPDFAttachment()) { + let ariaLabel; + // If the item has a child attachment + if (type !== null && type != 'none') { + if (type == 'pdf') { icon = getDOMElement('IconTreeitemAttachmentPDF'); + ariaLabel = Zotero.getString('pane.item.attachments.hasPDF'); if (!exists) { icon.classList.add('icon-missing-file'); } } - else if (item.isSnapshotAttachment()) { + else if (type == 'snapshot') { //icon = getDOMElement('IconTreeitemAttachmentSnapshot'); icon = exists ? getDOMElement('IconBulletBlue') : getDOMElement('IconBulletBlueEmpty'); + ariaLabel = Zotero.getString('pane.item.attachments.hasSnapshot'); } else { //icon = getDOMElement('IconTreeitem'); icon = exists ? getDOMElement('IconBulletBlue') : getDOMElement('IconBulletBlueEmpty'); + ariaLabel = Zotero.getString('pane.item.attachments.has'); } icon.classList.add('cell-icon'); //if (!exists) { // icon.classList.add('icon-missing-file'); //} } + //else if (type == 'none') { + // if (item.getField('url') || item.getField('DOI')) { + // icon = getDOMElement('IconLink'); + // ariaLabel = Zotero.getString('pane.item.attachments.hasLink'); + // icon.classList.add('cell-icon'); + // } + //} + if (ariaLabel) { + icon.setAttribute('aria-label', ariaLabel + '.'); + span.setAttribute('title', ariaLabel); + } span.append(icon); - item.fileExists() - // TODO: With no cell refreshing this is possibly somewhat inefficient - // Refresh cell when promise is fulfilled - .then(realExists => realExists != exists && this.tree.invalidateRow(index)); + // Don't run this immediately since it might cause a db check and disk access + // but delay for some time and see if the item is still visible in the tree + // (i.e. if we haven't scrolled right past it) + setTimeout(() => { + if (!this.tree.rowIsVisible(index)) return; + item.getBestAttachmentState() + // Refresh cell when promise is fulfilled + .then(({ type: newType, exists: newExists }) => { + if (newType !== type || newExists !== exists) { + this.tree.invalidateRow(index); + } + }); + }, ATTACHMENT_STATE_LOAD_DELAY); } return span; @@ -3545,7 +3505,7 @@ var ItemTree = class ItemTree extends LibraryTree { let t = new Date(); for (let i = 0; i < this._rows.length; i++) { let item = this.getRow(i).ref; - if (item.isRegularItem()) { + if (this._canGetBestAttachmentState(item)) { await item.getBestAttachmentState(); } } @@ -3823,6 +3783,11 @@ var ItemTree = class ItemTree extends LibraryTree { } return icon; } + + _canGetBestAttachmentState(item) { + return (item.isRegularItem() && item.numAttachments()) + || (item.isFileAttachment() && item.isTopLevelItem()); + } _isOnlyEmoji(str) { // Remove emoji, Zero Width Joiner, and Variation Selector-16 and see if anything's left diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 832f571ecf..d2f4b28d25 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -3709,7 +3709,7 @@ Zotero.Item.prototype.getBestAttachments = Zotero.Promise.coroutine(function* () /** - * Return state of best attachment + * Return state of best attachment (or this item if it's a standalone attachment) * * @return {Promise} - Promise for object with string 'type' ('none'|'pdf'|'snapshot'|'other') * and boolean 'exists' @@ -3718,7 +3718,9 @@ Zotero.Item.prototype.getBestAttachmentState = async function () { if (this._bestAttachmentState !== null) { return this._bestAttachmentState; } - var item = await this.getBestAttachment(); + var item = this.isAttachment() && this.isTopLevelItem() + ? this + : await this.getBestAttachment(); if (!item) { return this._bestAttachmentState = { type: 'none' diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index 88402089d9..5b9abf222c 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1169,6 +1169,15 @@ describe("Zotero.Item", function () { { type: 'other', exists: false } ); }) + + it("should cache state for a standalone attachment", async function () { + var standaloneAttachment = await importPDFAttachment(); + await standaloneAttachment.getBestAttachmentState(); + assert.deepEqual( + standaloneAttachment.getBestAttachmentStateCached(), + { type: 'pdf', exists: true } + ); + }); })