keep search conditionIDs in arithmetic sequence

When conditions are removed, shift conditionIDs so that
conditionIDs always go in increments of 1 (0, 1, 2 ...).
It prevents conditionIDs from conflicting with each other
when conditions are rearranged.

Fixes: #3434

chrome/content/zotero/elements/zoteroSearch.js

@@ -145,11 +145,20 @@
 			var conditionsBox = this.querySelector('#conditions');
 			
 			this.search.removeCondition(id);
-			
+			let found = false;
 			for (var i = 0, len = conditionsBox.childNodes.length; i < len; i++) {
 				if (conditionsBox.childNodes[i].conditionID == id) {
 					conditionsBox.removeChild(conditionsBox.childNodes[i]);
-					break;
+					found = true;
+					i--;
+					len--;
+					continue;
+				}
+				// When a condition is removed, ids of remaining conditions
+				// are shifted to remain in arithmetic sequence. The conditionID
+				// of the nodes need to be updated accordingly
+				if (found) {
+					conditionsBox.childNodes[i].conditionID--;
 				}
 			}
 			

chrome/content/zotero/xpcom/data/search.js

@@ -482,7 +482,20 @@ Zotero.Search.prototype.removeCondition = function (searchConditionID) {
 		throw new Error('Invalid searchConditionID ' + searchConditionID + ' in removeCondition()');
 	}
 	
-	delete this._conditions[searchConditionID];
+	searchConditionID = String(searchConditionID);
+	// Decrement the id of all conditions following
+	// the condition to be deleted. It ensures that
+	// all conditions remain in stict arithmetic sequence and prevents
+	// conditionIDs from colliding
+	let conditionIDs = Object.keys(this._conditions);
+	let conditionIndex = conditionIDs.indexOf(searchConditionID);
+	for (let i = conditionIndex + 1; i < conditionIDs.length; i++) {
+		let conditionID = conditionIDs[i];
+		this._conditions[conditionID - 1] = this._conditions[conditionID];
+		this._conditions[conditionID - 1].id = conditionID - 1;
+	}
+	// After all conditions are shifted, delete the last, empty one
+	delete this._conditions[this._maxSearchConditionID];
 	this._maxSearchConditionID--;
 	this._markFieldChange('conditions', this._conditions);
 	this._changed.conditions = true;
@@ -864,7 +877,10 @@ Zotero.Search.prototype.fromJSON = function (json, options = {}) {
 		this.name = json.name;
 	}
 	
-	Object.keys(this.getConditions()).forEach(id => this.removeCondition(id));
+	// Remove all conditions
+	while (Object.keys(this._conditions).length) {
+		this.removeCondition(Object.keys(this._conditions)[0]);
+	}
 	for (let i = 0; i < json.conditions.length; i++) {
 		let condition = json.conditions[i];
 		this.addCondition(

test/tests/searchTest.js

@@ -216,6 +216,27 @@ describe("Zotero.Search", function () {
 					var matches = await s.search();
 					assert.lengthOf(matches, 0);
 				});
+
+				it("should have same result after the same search conditions is removed and added", async function () {
+					var itemOne = await createDataObject('item', { title: "One" });
+					var itemTwo = await createDataObject('item', { title: "Two" });
+
+					var s = new Zotero.Search();
+					s.libraryID = itemOne.libraryID;
+					s.addCondition("joinMode", "any");
+					// Match both collections
+					s.addCondition('title', 'contains', 'One');
+					s.addCondition('title', 'contains', 'Two');
+					var matches = await s.search();
+					assert.sameMembers(matches, [itemOne.id, itemTwo.id]);
+
+					// Remove the first condition and add it again
+					s.removeCondition(1);
+					s.addCondition('title', 'contains', 'One');
+					matches = await s.search();
+					// Result should be the same
+					assert.sameMembers(matches, [itemOne.id, itemTwo.id]);
+				});
 			});
 			
 			describe("tag", function () {