From d3942ad1f0ac205e2f96949451bbdf6d04bc09f1 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Mon, 11 Jul 2022 01:28:15 -0400
Subject: [PATCH] Better fix for errors from invalid sort fields

Just catch the error from `ItemTree::sort()` and clear the
secondary-sort and fallback-sort prefs so that sorting works on the next
attempt.

Replacement for a8ed30ce803a9c5b3b5c04d50f6bd6b94faeee86

https://groups.google.com/g/zotero-dev/c/kc0-C6-SA74/m/bhHniGceAQAJ
---
 chrome/content/zotero/itemTree.jsx | 31 +++++++++++++++++++----------
 test/tests/itemTreeTest.js         | 32 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx
index 4ec883a5cc..d8229d2389 100644
--- a/chrome/content/zotero/itemTree.jsx
+++ b/chrome/content/zotero/itemTree.jsx
@@ -1427,18 +1427,27 @@ var ItemTree = class ItemTree extends LibraryTree {
 		var openItemIDs = this._saveOpenState(true);
 		
 		// Sort specific items
-		if (itemIDs) {
-			let idsToSort = new Set(itemIDs);
-			this._rows.sort((a, b) => {
-				// Don't re-sort existing items. This assumes a stable sort(), which is the case in Firefox
-				// but not Chrome/v8.
-				if (!idsToSort.has(a.ref.id) && !idsToSort.has(b.ref.id)) return 0;
-				return rowSort(a, b) * order;
-			});
+		try {
+			if (itemIDs) {
+				let idsToSort = new Set(itemIDs);
+				this._rows.sort((a, b) => {
+					// Don't re-sort existing items. This assumes a stable sort(), which is the case in Firefox
+					// but not Chrome/v8.
+					if (!idsToSort.has(a.ref.id) && !idsToSort.has(b.ref.id)) return 0;
+					return rowSort(a, b) * order;
+				});
+			}
+			// Full sort
+			else {
+				this._rows.sort((a, b) => rowSort(a, b) * order);
+			}
 		}
-		// Full sort
-		else {
-			this._rows.sort((a, b) => rowSort(a, b) * order);
+		catch (e) {
+			Zotero.logError("Error sorting fields: " + e.message);
+			Zotero.debug(e, 1);
+			// Clear anything that might be contributing to the error
+			Zotero.Prefs.clear('secondarySort.' + this.getSortField());
+			Zotero.Prefs.clear('fallbackSort');
 		}
 		
 		this._refreshRowMap();
diff --git a/test/tests/itemTreeTest.js b/test/tests/itemTreeTest.js
index fe93e7596a..f04c7b8150 100644
--- a/test/tests/itemTreeTest.js
+++ b/test/tests/itemTreeTest.js
@@ -151,6 +151,38 @@ describe("Zotero.ItemTree", function() {
 		})
 	})
 	
+	describe("#sort()", function () {
+		it("should ignore invalid secondary-sort field", async function () {
+			await createDataObject('item', { title: 'A' });
+			await createDataObject('item', { title: 'A' });
+			
+			// Set invalid field as secondary sort for title
+			Zotero.Prefs.set('secondarySort.title', 'invalidField');
+			
+			// Sort by title
+			var colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'title');
+			await itemsView.tree._columns.toggleSort(colIndex);
+			
+			var e = await getPromiseError(zp.itemsView.sort());
+			assert.isFalse(e);
+			assert.isUndefined(Zotero.Prefs.get('secondarySort.title'));
+		});
+		
+		it("should ignore invalid fallback-sort field", async function () {
+			Zotero.Prefs.clear('fallbackSort');
+			var originalFallback = Zotero.Prefs.get('fallbackSort');
+			Zotero.Prefs.set('fallbackSort', 'invalidField,' + originalFallback);
+			
+			// Sort by title
+			var colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'title');
+			await itemsView.tree._columns.toggleSort(colIndex);
+			
+			var e = await getPromiseError(zp.itemsView.sort());
+			assert.isFalse(e);
+			assert.equal(Zotero.Prefs.get('fallbackSort'), originalFallback);
+		});
+	});
+	
 	describe("#notify()", function () {
 		beforeEach(function () {
 			sinon.spy(win.ZoteroPane, "itemSelected");