From 22b00c33de35454b687207e598da449f7d7c20e1 Mon Sep 17 00:00:00 2001
From: Abe Jellinek <jellinek@berkeley.edu>
Date: Thu, 18 Aug 2022 21:14:34 -0400
Subject: [PATCH] Empty Trash: Visually remove deleted items, refresh icon
 (#2606)

There were a few problems causing the incorrect behavior:
1. Rows were being removed only if they had no non-deleted children, which
   wasn't the right check. We want to remove all rows with no *deleted*
   children.
2. Children of the removed rows weren't being removed with them.
3. We weren't invalidating the tree (which _removeRows() doesn't do).

Also:

* Erase trashed annotation after getAnnotations() test

Because ItemTree#notify() doesn't yet correctly handle refresh events on
parent items that are themselves children (three-level nesting: item ->
attachment -> annotation), this test was causing a failure in
itemTreeTest.js.
---
 chrome/content/zotero/collectionTree.jsx  | 14 ++++++++------
 chrome/content/zotero/itemTree.jsx        | 13 +++++++++++--
 chrome/content/zotero/xpcom/data/items.js |  1 +
 test/tests/itemTest.js                    |  4 ++++
 test/tests/itemTreeTest.js                |  2 +-
 5 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/chrome/content/zotero/collectionTree.jsx b/chrome/content/zotero/collectionTree.jsx
index b46b65cf98..0b6e11ff46 100644
--- a/chrome/content/zotero/collectionTree.jsx
+++ b/chrome/content/zotero/collectionTree.jsx
@@ -640,11 +640,8 @@ var CollectionTree = class CollectionTree extends LibraryTree {
 			this.tree.invalidate();
 			return;
 		}
-		if (action == 'refresh') {
-			// If trash is refreshed, we probably need to update the icon from full to empty
-			if (type == 'trash') {
-				this.tree.invalidate();
-			}
+		if (action == 'refresh' && type != 'trash') {
+			// Trash handled below
 			return;
 		}
 		if (type == 'feed' && (action == 'unreadCountUpdated' || action == 'statusChanged')) {
@@ -826,6 +823,11 @@ var CollectionTree = class CollectionTree extends LibraryTree {
 				}
 			}
 		}
+		else if (action == 'refresh' && type == 'trash') {
+			// We need to update the trash's status (full or empty), and if empty,
+			// the row might be removed
+			await this.reload();
+		}
 
 		this.forceUpdate();
 		var promise = this.waitForSelect();
@@ -2307,7 +2309,7 @@ var CollectionTree = class CollectionTree extends LibraryTree {
 		}
 		
 		if (showTrash) {
-			let deletedItems = await Zotero.Items.getDeleted(libraryID);
+			let deletedItems = await Zotero.Items.getDeleted(libraryID, true);
 			if (deletedItems.length || Zotero.Prefs.get("showTrashWhenEmpty")) {
 				var ref = {
 					libraryID: libraryID
diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx
index d8229d2389..604691fcc9 100644
--- a/chrome/content/zotero/itemTree.jsx
+++ b/chrome/content/zotero/itemTree.jsx
@@ -446,11 +446,20 @@ var ItemTree = class ItemTree extends LibraryTree {
 					let row = this.getRowIndexByID(id);
 					if (row === false) continue;
 					let item = Zotero.Items.get(id);
-					if (!item.deleted && !item.numChildren()) {
+					// Remove parent row if it isn't deleted and doesn't have any deleted children
+					// (shown by the numChildren including deleted being the same as numChildren not including deleted)
+					if (!item.deleted && (!item.isRegularItem() || item.numChildren(true) == item.numChildren(false))) {
 						rows.push(row);
+						// And all its children in the tree
+						for (let child = row + 1; child < this.rowCount && this.getLevel(child) > this.getLevel(row); child++) {
+							rows.push(child);
+						}
 					}
 				}
-				this._removeRows(rows);
+				if (rows.length) {
+					this._removeRows(rows);
+					this.tree.invalidate();
+				}
 			}
 
 			return;
diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js
index 6525722096..73daf8e8bf 100644
--- a/chrome/content/zotero/xpcom/data/items.js
+++ b/chrome/content/zotero/xpcom/data/items.js
@@ -1502,6 +1502,7 @@ Zotero.Items = function() {
 				);
 			}
 			Zotero.debug("Emptied " + deleted.length + " item(s) from trash in " + (new Date() - t) + " ms");
+			Zotero.Notifier.trigger('refresh', 'trash', libraryID);
 		}
 		
 		return deleted.length;
diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js
index 09cb88f02b..f31095142f 100644
--- a/test/tests/itemTest.js
+++ b/test/tests/itemTest.js
@@ -1465,6 +1465,10 @@ describe("Zotero.Item", function () {
 				await annotation2.saveTx();
 			});
 			
+			after(async function () {
+				await annotation2.eraseTx();
+			});
+			
 			it("should return annotations not in trash", async function () {
 				var items = attachment.getAnnotations();
 				assert.sameMembers(items, [annotation1]);
diff --git a/test/tests/itemTreeTest.js b/test/tests/itemTreeTest.js
index 99852a0274..de27f09b6d 100644
--- a/test/tests/itemTreeTest.js
+++ b/test/tests/itemTreeTest.js
@@ -684,7 +684,7 @@ describe("Zotero.ItemTree", function() {
 		});
 		
 		describe("Trash", function () {
-			it.skip("should remove untrashed parent item when last trashed child is deleted", function* () {
+			it("should remove untrashed parent item when last trashed child is deleted", function* () {
 				var userLibraryID = Zotero.Libraries.userLibraryID;
 				var item = yield createDataObject('item');
 				var note = yield createDataObject(