From 353c304a1a7f7f66179013dbd72e889fec5b38de Mon Sep 17 00:00:00 2001 From: Lucian Date: Sun, 7 Jun 2026 13:25:25 +0700 Subject: [PATCH] Avoid blocking when permanently deleting many items --- .../content/zotero/collectionViewItemTree.jsx | 10 +- chrome/content/zotero/xpcom/data/items.js | 60 ++++++++++-- chrome/content/zotero/xpcom/zotero.js | 94 +++++++++++++++++-- chrome/content/zotero/zoteroPane.js | 76 +++++++++++++-- chrome/content/zotero/zoteroPane.xhtml | 15 ++- chrome/locale/en-US/zotero/zotero.properties | 3 + scss/components/_mainWindow.scss | 61 ++++++++++-- test/tests/collectionViewItemTreeTest.js | 36 +++++++ test/tests/itemsTest.js | 51 ++++++++++ test/tests/zoteroPaneTest.js | 50 ++++++++++ 10 files changed, 421 insertions(+), 35 deletions(-) diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index 7ed1ed2031..fd573cc08a 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -1695,7 +1695,7 @@ class CollectionViewItemTree extends ItemTree { // // /////////////////////////////////////////////////////////////////////////// - async deleteSelection(force) { + async deleteSelection(force, options = {}) { if (this.selection.count == 0) { return; } @@ -1720,7 +1720,9 @@ class CollectionViewItemTree extends ItemTree { // If all selected items are annotations, for now erase them skipping trash if (selectedItems.length && selectedItems.every(item => item.isAnnotation())) { - await Zotero.Items.erase(selectedItemIDs); + await Zotero.Items.eraseInChunks(selectedItemIDs, { + onProgress: options.onProgress + }); } else if (collectionTreeRow.isBucket()) { collectionTreeRow.ref.deleteItems(ids); @@ -1742,7 +1744,9 @@ class CollectionViewItemTree extends ItemTree { await Zotero.Searches.erase(trashedSearches); } if (selectedItemIDs.length > 0) { - await Zotero.Items.erase(selectedItemIDs); + await Zotero.Items.eraseInChunks(selectedItemIDs, { + onProgress: options.onProgress + }); } } else if (collectionTreeRow.isRecentlyRead() && !force) { diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index 0f869f7bbc..401c8bdd73 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/chrome/content/zotero/xpcom/data/items.js @@ -1062,6 +1062,55 @@ Zotero.Items = function () { } + /** + * Permanently delete items in separate transactions to avoid blocking the UI and + * accumulating a very large notifier queue. + * + * @param {Integer|Integer[]} ids + * @param {Object} [options] + * @param {Integer} [options.chunkSize=50] + * @param {Function} [options.onProgress] - fn(progress, progressMax) + */ + this.eraseInChunks = async function (ids, options = {}) { + ids = Zotero.flattenArguments(ids); + if (!ids.length) { + return; + } + + let chunkSize = options.chunkSize === undefined ? 50 : options.chunkSize; + if (!Number.isInteger(chunkSize) || chunkSize < 1) { + throw new Error("'chunkSize' must be a positive integer"); + } + + let processed = 0; + let eraseOptions = Object.assign({}, options); + delete eraseOptions.chunkSize; + delete eraseOptions.onProgress; + + let reportedProgress = 0; + await Zotero.Utilities.Internal.forEachChunkAsync( + ids, + chunkSize, + async chunk => { + let chunkOptions = Object.assign({}, eraseOptions); + if (options.onProgress) { + chunkOptions.onProgress = progress => { + reportedProgress = processed + progress; + options.onProgress(reportedProgress, ids.length); + }; + } + await this.erase(chunk, chunkOptions); + processed += chunk.length; + if (options.onProgress && reportedProgress < processed) { + reportedProgress = processed; + options.onProgress(reportedProgress, ids.length); + } + await Zotero.Promise.delay(); + } + ); + }; + + /** * @param {Integer} libraryID - Library to delete from * @param {Object} [options] @@ -1101,20 +1150,17 @@ Zotero.Items = function () { // Show progress meter during deletions let eraseOptions = options.onProgress ? { - onProgress: function (progress, progressMax) { + onProgress: function (progress) { options.onProgress(processed + progress, deleted.length); } } : undefined; for (let x of ['top', 'child']) { - await Zotero.Utilities.Internal.forEachChunkAsync( + await this.eraseInChunks( toDelete[x], - 1000, - async function (chunk) { - await this.erase(chunk, eraseOptions); - processed += chunk.length; - }.bind(this) + eraseOptions ); + processed += toDelete[x].length; } Zotero.debug("Emptied " + deleted.length + " item(s) from trash in " + (new Date() - t) + " ms"); Zotero.Notifier.trigger('refresh', 'trash', libraryID); diff --git a/chrome/content/zotero/xpcom/zotero.js b/chrome/content/zotero/xpcom/zotero.js index e6c17ba7b8..1a807cc287 100644 --- a/chrome/content/zotero/xpcom/zotero.js +++ b/chrome/content/zotero/xpcom/zotero.js @@ -1730,6 +1730,28 @@ const { CommandLineOptions } = ChromeUtils.importESModule("chrome://zotero/conte else { label.hidden = true; } + let header = win.ZoteroPane.document.getElementById('zotero-pane-progress-header'); + let progressIcon = win.ZoteroPane.document.getElementById('zotero-pane-progress-icon'); + if (progressIcon) { + progressIcon.hidden = !icon; + progressIcon.setAttribute( + 'class', + `icon icon-20 icon-css${icon ? ` icon-${icon}` : ''}` + ); + } + if (header) { + header.hidden = !msg && !icon; + } + let detail = win.ZoteroPane.document.getElementById('zotero-pane-progress-detail'); + if (detail) { + detail.hidden = true; + detail.value = ""; + } + let percentage = win.ZoteroPane.document.getElementById('zotero-pane-progress-percentage'); + if (percentage) { + percentage.hidden = !determinate; + percentage.value = determinate ? "0%" : ""; + } // This is the craziest thing. In Firefox 52.6.0, the very presence of this line // causes Zotero on Linux to burn 5% CPU at idle, even if everything below it in // the block is commented out. Same if the progressmeter itself is hidden="true". @@ -1745,6 +1767,9 @@ const { CommandLineOptions } = ChromeUtils.importESModule("chrome://zotero/conte if (!progressMeter) { progressMeter = doc.createElement('progress'); progressMeter.id = id; + progressMeter.classList.add('downloadProgress'); + progressMeter.setAttribute('aria-labelledby', 'zotero-pane-progress-label'); + progressMeter.setAttribute('aria-describedby', 'zotero-pane-progress-detail'); } if (determinate) { progressMeter.setAttribute('value', 0); @@ -1766,31 +1791,86 @@ const { CommandLineOptions } = ChromeUtils.importESModule("chrome://zotero/conte /** - * @param {Number} percentage Percentage complete as integer or float + * @param {Number} percentage - Percentage complete as integer or float + * @param {String} [msg] - Updated progress title + * @param {String} [detail] - Updated progress detail */ - this.updateZoteroPaneProgressMeter = function (percentage) { - if(percentage !== null) { + this.updateZoteroPaneProgressMeter = function (percentage, msg, detail) { + let progressValue = null; + let displayedPercentage = null; + if (percentage !== null) { if (percentage < 0 || percentage > 100) { Zotero.debug("Invalid percentage value '" + percentage + "' in Zotero.updateZoteroPaneProgressMeter()"); return; } - percentage = Math.round(percentage * 10); + displayedPercentage = Math.round(percentage); + progressValue = Math.round(percentage * 10); } - if (percentage === _lastPercentage) { + if (progressValue === _lastPercentage && msg === undefined && detail === undefined) { return; } + + if (msg !== undefined) { + _progressMessage = msg = msg || ""; + } + let enumerator = Services.wm.getEnumerator("navigator:browser"); + while (enumerator.hasMoreElements()) { + let win = enumerator.getNext(); + if (!win.ZoteroPane) continue; + + let doc = win.ZoteroPane.document; + let label = doc.getElementById('zotero-pane-progress-label'); + let detailLabel = doc.getElementById('zotero-pane-progress-detail'); + let percentageLabel = doc.getElementById('zotero-pane-progress-percentage'); + if (!label || !detailLabel || !percentageLabel) { + continue; + } + if (msg !== undefined) { + if (msg) { + label.hidden = false; + label.value = msg; + } + else { + label.hidden = true; + } + let header = doc.getElementById('zotero-pane-progress-header'); + let progressIcon = doc.getElementById('zotero-pane-progress-icon'); + if (header && progressIcon) { + header.hidden = !msg && progressIcon.hidden; + } + } + if (detail !== undefined) { + if (detail) { + detailLabel.hidden = false; + detailLabel.value = detail; + } + else { + detailLabel.hidden = true; + detailLabel.value = ""; + } + } + if (percentage !== null) { + percentageLabel.hidden = false; + percentageLabel.value = `${displayedPercentage}%`; + } + else { + percentageLabel.hidden = true; + percentageLabel.value = ""; + } + } + for (let pm of _progressMeters) { if (percentage !== null) { if (!pm.hasAttribute('value')) { pm.max = 1000; } - pm.setAttribute('value', percentage); + pm.setAttribute('value', progressValue); } else if (pm.hasAttribute('value')) { pm.removeAttribute('value'); } } - _lastPercentage = percentage; + _lastPercentage = progressValue; } diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 5a48549b52..20a6c3c952 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -2201,6 +2201,28 @@ var ZoteroPane = new function () { this.deleteSelectedItems(); } + function getDeleteProgressHandler() { + let lastPercentage = -1; + let numberFormat = new Intl.NumberFormat(); + return (progress, progressMax) => { + let percentage = progressMax + ? Math.min(100, Math.round((progress / progressMax) * 100)) + : 0; + if (percentage == lastPercentage) { + return; + } + lastPercentage = percentage; + Zotero.updateZoteroPaneProgressMeter( + percentage, + undefined, + Zotero.getString( + 'pane.items.delete.progress.count', + [numberFormat.format(progress), numberFormat.format(progressMax)] + ) + ); + }; + } + /* * Remove, trash, or delete item(s), depending on context * @@ -2306,7 +2328,39 @@ var ZoteroPane = new function () { } if (!prompt || Services.prompt.confirm(window, prompt.title, prompt.text)) { - await this.itemsView.deleteSelection(force); + let selectedItems = this.itemsView.getSelectedItems() + .filter(item => item instanceof Zotero.Item); + let isPermanentDelete = collectionTreeRow.isTrash() + || (selectedItems.length && selectedItems.every(item => item.isAnnotation())); + let showProgress = isPermanentDelete && selectedItems.length >= 50; + + if (showProgress) { + Zotero.showZoteroPaneProgressMeter( + Zotero.getString('pane.items.delete.progress'), + true, + 'trash' + ); + Zotero.updateZoteroPaneProgressMeter( + 0, + undefined, + Zotero.getString( + 'pane.items.delete.progress.count', + [new Intl.NumberFormat().format(0), new Intl.NumberFormat().format(selectedItems.length)] + ) + ); + } + + try { + await this.itemsView.deleteSelection( + force, + showProgress ? { onProgress: getDeleteProgressHandler() } : undefined + ); + } + finally { + if (showProgress) { + Zotero.hideZoteroPaneOverlays(); + } + } } } @@ -2523,26 +2577,32 @@ var ZoteroPane = new function () { + Zotero.getString('general.actionCannotBeUndone') ); if (result) { - Zotero.showZoteroPaneProgressMeter(null, true); + Zotero.showZoteroPaneProgressMeter( + Zotero.getString('pane.items.delete.progress'), + true, + 'trash' + ); try { let deletedSearches = await Zotero.Searches.getDeleted(libraryID, true); await Zotero.Searches.erase(deletedSearches); let deletedCollections = await Zotero.Collections.getDeleted(libraryID, true); await Zotero.Collections.erase(deletedCollections); - let deleted = await Zotero.Items.emptyTrash( + await Zotero.Items.emptyTrash( libraryID, { - onProgress: (progress, progressMax) => { - var percentage = Math.round((progress / progressMax) * 100); - Zotero.updateZoteroPaneProgressMeter(percentage); - } + onProgress: getDeleteProgressHandler() } ); + Zotero.updateZoteroPaneProgressMeter( + null, + Zotero.getString('pane.items.delete.progress.cleaning'), + null + ); + await Zotero.purgeDataObjects(); } finally { Zotero.hideZoteroPaneOverlays(); } - await Zotero.purgeDataObjects(); } }; diff --git a/chrome/content/zotero/zoteroPane.xhtml b/chrome/content/zotero/zoteroPane.xhtml index 62de1383fd..46a6ab21a9 100644 --- a/chrome/content/zotero/zoteroPane.xhtml +++ b/chrome/content/zotero/zoteroPane.xhtml @@ -1436,13 +1436,22 @@ - - + + + + + + - + diff --git a/chrome/locale/en-US/zotero/zotero.properties b/chrome/locale/en-US/zotero/zotero.properties index 81dff45f67..d41b7ca6e6 100644 --- a/chrome/locale/en-US/zotero/zotero.properties +++ b/chrome/locale/en-US/zotero/zotero.properties @@ -262,6 +262,9 @@ pane.feed.deleteWithItems = Are you sure you want to unsubscribe from this feed? pane.collections.deleteSearch.title = Delete Search pane.collections.deleteSearch = Are you sure you want to delete the selected search? pane.collections.emptyTrash = Are you sure you want to permanently remove items in the Trash? +pane.items.delete.progress = Permanently deleting items +pane.items.delete.progress.count = %1$S of %2$S items +pane.items.delete.progress.cleaning = Finishing deletion pane.collections.newSavedSeach = New Saved Search pane.collections.savedSearchName = Enter a name for this saved search: pane.collections.library = My Library diff --git a/scss/components/_mainWindow.scss b/scss/components/_mainWindow.scss index 1398e24fb4..85a00346a7 100644 --- a/scss/components/_mainWindow.scss +++ b/scss/components/_mainWindow.scss @@ -34,19 +34,66 @@ } #zotero-pane-overlay { - background: rgba(0,0,0,.3); + background: rgba(0, 0, 0, 0.24); } #zotero-pane-progress-box { - background: var(--color-sidepane); - border-radius: 5px; - border: var(--color-panedivider); - height: 30px; - width: 300px; + background: var(--material-background); + border: var(--material-border); + border-radius: $border-radius-large; + box-shadow: 0 12px 32px rgba(0, 0, 0, 0.22); + box-sizing: border-box; + gap: $space-sm; + padding: $space-md; + width: min(420px, calc(100vw - #{$space-xxl})); +} + +#zotero-pane-progress-header { + gap: $space-xs; + min-width: 0; +} + +#zotero-pane-progress-icon { + flex: 0 0 auto; +} + +#zotero-pane-progress-label { + color: var(--fill-primary); + font-size: $font-size-h2; + font-weight: 600; + margin: 0; + min-width: 0; } #zotero-pane-progressmeter-container { - padding: 10px; + gap: $space-xs; + min-width: 0; + padding: 0; +} + +#zotero-pane-progressmeter { + margin: 0; + width: 100%; +} + +#zotero-pane-progress-status { + min-height: $line-height-computed; + min-width: 0; +} + +#zotero-pane-progress-detail, +#zotero-pane-progress-percentage { + color: var(--fill-secondary); + margin: 0; +} + +#zotero-pane-progress-detail { + min-width: 0; +} + +#zotero-pane-progress-percentage { + font-variant-numeric: tabular-nums; + margin-inline-start: $space-md; } #zotero-layout-switcher { diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index f3b9182314..8f06741ede 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -1639,6 +1639,42 @@ describe("CollectionViewItemTree", function () { assert.equal(zp.itemsView.rowCount, 0); }); + it("should permanently delete selected items in chunks", async function () { + let items = []; + for (let i = 0; i < 3; i++) { + items.push(await createDataObject('item', { deleted: true })); + } + let ids = items.map(item => item.id); + + await selectTrash(win); + await itemsView.selectItems(ids); + + let eraseInChunksSpy = sinon.spy(Zotero.Items, 'eraseInChunks'); + let progressEvents = []; + try { + await itemsView.deleteSelection( + false, + { + onProgress: (progress, progressMax) => { + progressEvents.push([progress, progressMax]); + } + } + ); + } + finally { + eraseInChunksSpy.restore(); + } + + assert.isTrue(eraseInChunksSpy.calledOnce); + assert.sameMembers(eraseInChunksSpy.firstCall.args[0], ids); + assert.isFunction(eraseInChunksSpy.firstCall.args[1].onProgress); + assert.deepEqual(progressEvents.at(-1), [ids.length, ids.length]); + for (let id of ids) { + assert.isFalse(await Zotero.Items.getAsync(id)); + assert.isFalse(itemsView.getRowIndexByID(id)); + } + }); + it("should show only top-most trashed collection", async function () { var c1 = await createDataObject('collection', { deleted: true }); var c2 = await createDataObject('collection', { parentID: c1.id }); diff --git a/test/tests/itemsTest.js b/test/tests/itemsTest.js index 2f101ed204..6d8670b69a 100644 --- a/test/tests/itemsTest.js +++ b/test/tests/itemsTest.js @@ -258,6 +258,57 @@ describe("Zotero.Items", function () { }); + describe("#eraseInChunks()", function () { + it("should erase items in separate transactions and report overall progress", async function () { + let ids = []; + for (let i = 0; i < 5; i++) { + let item = createUnsavedDataObject('item'); + item.deleted = true; + ids.push(await item.saveTx()); + } + + let progressEvents = []; + let eraseSpy = sinon.spy(Zotero.Items, 'erase'); + try { + await Zotero.Items.eraseInChunks( + ids, + { + chunkSize: 2, + onProgress: (progress, progressMax) => { + progressEvents.push([progress, progressMax]); + } + } + ); + } + finally { + eraseSpy.restore(); + } + + assert.deepEqual( + eraseSpy.getCalls().map(call => call.args[0]), + [ + ids.slice(0, 2), + ids.slice(2, 4), + ids.slice(4) + ] + ); + assert.deepEqual( + progressEvents, + [ + [1, 5], + [2, 5], + [3, 5], + [4, 5], + [5, 5] + ] + ); + for (let id of ids) { + assert.isFalse(await Zotero.Items.getAsync(id)); + } + }); + }); + + describe("#emptyTrash()", function () { it("should delete items in the trash", async function () { var item1 = createUnsavedDataObject('item'); diff --git a/test/tests/zoteroPaneTest.js b/test/tests/zoteroPaneTest.js index b4fe9b147e..17722f445e 100644 --- a/test/tests/zoteroPaneTest.js +++ b/test/tests/zoteroPaneTest.js @@ -839,6 +839,56 @@ describe("ZoteroPane", function () { }); }); + describe("#emptyTrash()", function () { + it("should show deletion progress and cleanup status", async function () { + let stubs = [ + sinon.stub(Zotero.Searches, 'getDeleted').resolves([]), + sinon.stub(Zotero.Searches, 'erase').resolves(), + sinon.stub(Zotero.Collections, 'getDeleted').resolves([]), + sinon.stub(Zotero.Collections, 'erase').resolves(), + sinon.stub(Zotero.Items, 'emptyTrash').callsFake(async (_libraryID, options) => { + options.onProgress(50, 100); + }), + sinon.stub(Zotero, 'purgeDataObjects').resolves() + ]; + let showSpy = sinon.spy(Zotero, 'showZoteroPaneProgressMeter'); + let updateSpy = sinon.spy(Zotero, 'updateZoteroPaneProgressMeter'); + let hideSpy = sinon.spy(Zotero, 'hideZoteroPaneOverlays'); + + try { + let dialogPromise = waitForDialog(); + let emptyTrashPromise = zp.emptyTrash(); + await dialogPromise; + await emptyTrashPromise; + } + finally { + for (let stub of stubs) { + stub.restore(); + } + showSpy.restore(); + updateSpy.restore(); + hideSpy.restore(); + } + + assert.isTrue(showSpy.calledWith( + Zotero.getString('pane.items.delete.progress'), + true, + 'trash' + )); + assert.isTrue(updateSpy.calledWith( + 50, + undefined, + Zotero.getString('pane.items.delete.progress.count', [50, 100]) + )); + assert.isTrue(updateSpy.calledWith( + null, + Zotero.getString('pane.items.delete.progress.cleaning'), + null + )); + assert.isTrue(hideSpy.calledOnce); + }); + }); + describe("#deleteSelectedCollection()", function () { it("should move collection to trash but not descendant items by default", async function () { var collection = await createDataObject('collection');