From b7b246e741a00ebe9695ac747d53d25e65cb87a1 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Sat, 26 Mar 2016 02:59:54 -0400
Subject: [PATCH] Saved search fixes
- Fix saved search editing
- Refresh items list on search change
- Generate correct conditions array for search JSON
---
.../content/zotero/bindings/zoterosearch.xml | 9 ---
chrome/content/zotero/searchDialog.js | 8 ++-
chrome/content/zotero/xpcom/itemTreeView.js | 14 ++++-
chrome/content/zotero/xpcom/search.js | 5 +-
chrome/content/zotero/zoteroPane.js | 8 +--
test/content/support.js | 4 ++
test/tests/itemTreeViewTest.js | 58 +++++++++++++++++++
test/tests/searchTest.js | 49 ++++++++++++++++
test/tests/syncEngineTest.js | 2 +-
test/tests/zoteroPaneTest.js | 20 +++++++
10 files changed, 157 insertions(+), 20 deletions(-)
diff --git a/chrome/content/zotero/bindings/zoterosearch.xml b/chrome/content/zotero/bindings/zoterosearch.xml
index a91faccd5..ff744e21c 100644
--- a/chrome/content/zotero/bindings/zoterosearch.xml
+++ b/chrome/content/zotero/bindings/zoterosearch.xml
@@ -222,15 +222,6 @@
</body>
</method>
- <method name="save">
- <body>
- <![CDATA[
- this.updateSearch();
- return this.search.save();
- ]]>
- </body>
- </method>
-
<method name="handleKeyPress">
<parameter name="event"/>
<body>
diff --git a/chrome/content/zotero/searchDialog.js b/chrome/content/zotero/searchDialog.js
index 5b946a027..34d1afe26 100644
--- a/chrome/content/zotero/searchDialog.js
+++ b/chrome/content/zotero/searchDialog.js
@@ -23,6 +23,8 @@
***** END LICENSE BLOCK *****
*/
+"use strict";
+
var itemsView;
var collectionsView;
var io;
@@ -50,7 +52,11 @@ function doAccept()
{
document.getElementById('search-box').search.name = document.getElementById('search-name').value;
try {
- io.dataOut = document.getElementById('search-box').save();
+ let searchBox = document.getElementById('search-box');
+ searchBox.updateSearch();
+ io.dataOut = {
+ json: searchBox.search.toJSON()
+ };
}
catch (e) {
Zotero.debug(e, 1);
diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js
index 9221576b3..45b4c0050 100644
--- a/chrome/content/zotero/xpcom/itemTreeView.js
+++ b/chrome/content/zotero/xpcom/itemTreeView.js
@@ -55,7 +55,7 @@ Zotero.ItemTreeView = function (collectionTreeRow, sourcesOnly) {
this._unregisterID = Zotero.Notifier.registerObserver(
this,
- ['item', 'collection-item', 'item-tag', 'share-items', 'bucket', 'feedItem'],
+ ['item', 'collection-item', 'item-tag', 'share-items', 'bucket', 'feedItem', 'search'],
'itemTreeView',
50
);
@@ -348,8 +348,8 @@ Zotero.ItemTreeView.prototype.refresh = Zotero.serial(Zotero.Promise.coroutine(f
});
try {
- var newItems = yield this.collectionTreeRow.getItems();
Zotero.CollectionTreeCache.clear();
+ var newItems = yield this.collectionTreeRow.getItems();
if (!this.selection.selectEventsSuppressed) {
var unsuppress = this.selection.selectEventsSuppressed = true;
@@ -464,6 +464,14 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio
return;
}
+ if (type == 'search' && action == 'modify') {
+ // TODO: Only refresh on condition change (not currently available in extraData)
+ yield this.refresh();
+ this.sort();
+ this._treebox.invalidate();
+ return;
+ }
+
// Clear item type icon and tag colors when a tag is added to or removed from an item
if (type == 'item-tag') {
// TODO: Only update if colored tag changed?
@@ -757,7 +765,7 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio
}
else if(type == 'item' && action == 'add')
{
- let items = yield Zotero.Items.getAsync(ids);
+ let items = Zotero.Items.get(ids);
// In some modes, just re-run search
if (collectionTreeRow.isSearch() || collectionTreeRow.isTrash() || collectionTreeRow.isUnfiled()) {
diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js
index c0d358a3f..805d812d8 100644
--- a/chrome/content/zotero/xpcom/search.js
+++ b/chrome/content/zotero/xpcom/search.js
@@ -447,6 +447,7 @@ Zotero.Search.prototype.removeCondition = function (searchConditionID) {
}
delete this._conditions[searchConditionID];
+ this._maxSearchConditionID--;
this._markFieldChange('conditions', this._conditions);
this._changed.conditions = true;
}
@@ -827,6 +828,7 @@ Zotero.Search.prototype.fromJSON = function (json) {
}
this.name = json.name;
+ Object.keys(this.getConditions()).forEach(id => this.removeCondition(0));
for (let i = 0; i < json.conditions.length; i++) {
let condition = json.conditions[i];
this.addCondition(
@@ -851,7 +853,8 @@ Zotero.Search.prototype.toJSON = function (options = {}) {
obj.key = this.key;
obj.version = this.version;
obj.name = this.name;
- obj.conditions = this.getConditions();
+ var conditions = this.getConditions();
+ obj.conditions = Object.keys(conditions).map(x => conditions[x]);
return this._postToJSON(env);
}
diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js
index 53b81a232..e65e7d812 100644
--- a/chrome/content/zotero/zoteroPane.js
+++ b/chrome/content/zotero/zoteroPane.js
@@ -1899,10 +1899,7 @@ var ZoteroPane = new function()
}
}
else {
- let s = new Zotero.Search();
- s.id = row.ref.id;
- yield s.loadPrimaryData();
- yield s.loadConditions();
+ let s = row.ref.clone();
let groups = [];
// Promises don't work in the modal dialog, so get the group name here, if
// applicable, and pass it in. We only need the group that this search belongs
@@ -1920,7 +1917,8 @@ var ZoteroPane = new function()
};
window.openDialog('chrome://zotero/content/searchDialog.xul','','chrome,modal',io);
if (io.dataOut) {
- this.onCollectionSelected(); //reload itemsView
+ row.ref.fromJSON(io.dataOut.json);
+ yield row.ref.saveTx();
}
}
}
diff --git a/test/content/support.js b/test/content/support.js
index 570efdbe5..1efb12993 100644
--- a/test/content/support.js
+++ b/test/content/support.js
@@ -382,6 +382,10 @@ function createUnsavedDataObject(objectType, params = {}) {
break;
}
+ if (objectType == 'search') {
+ obj.addCondition('title', 'contains', 'test');
+ }
+
Zotero.Utilities.assignProps(obj, params, allowedParams);
return obj;
diff --git a/test/tests/itemTreeViewTest.js b/test/tests/itemTreeViewTest.js
index 8625e3a03..fca92264c 100644
--- a/test/tests/itemTreeViewTest.js
+++ b/test/tests/itemTreeViewTest.js
@@ -232,6 +232,64 @@ describe("Zotero.ItemTreeView", function() {
yield Zotero.Items.erase(items.map(item => item.id));
})
+ it("should update search results when items are added", function* () {
+ var search = createUnsavedDataObject('search');
+ var title = Zotero.Utilities.randomString();
+ search.fromJSON({
+ name: "Test",
+ conditions: [
+ {
+ condition: "title",
+ operator: "is",
+ value: title
+ }
+ ]
+ });
+ yield search.saveTx();
+
+ yield waitForItemsLoad(win);
+ assert.equal(zp.itemsView.rowCount, 0);
+
+ // Add an item matching search
+ var item = yield createDataObject('item', { title });
+
+ yield waitForItemsLoad(win);
+ assert.equal(zp.itemsView.rowCount, 1);
+ assert.equal(zp.itemsView.getRowIndexByID(item.id), 0);
+ });
+
+ it("should update search results when search conditions are changed", function* () {
+ var search = createUnsavedDataObject('search');
+ var title1 = Zotero.Utilities.randomString();
+ var title2 = Zotero.Utilities.randomString();
+ search.fromJSON({
+ name: "Test",
+ conditions: [
+ {
+ condition: "title",
+ operator: "is",
+ value: title1
+ }
+ ]
+ });
+ yield search.saveTx();
+
+ yield waitForItemsLoad(win);
+
+ // Add an item that doesn't match search
+ var item = yield createDataObject('item', { title: title2 });
+ yield waitForItemsLoad(win);
+ assert.equal(zp.itemsView.rowCount, 0);
+
+ // Modify conditions to match item
+ search.removeCondition(0);
+ search.addCondition("title", "is", title2);
+ yield search.saveTx();
+
+ yield waitForItemsLoad(win);
+
+ assert.equal(zp.itemsView.rowCount, 1);
+ });
it("should remove items from Unfiled Items when added to a collection", function* () {
var userLibraryID = Zotero.Libraries.userLibraryID;
diff --git a/test/tests/searchTest.js b/test/tests/searchTest.js
index ea71e7100..496c98071 100644
--- a/test/tests/searchTest.js
+++ b/test/tests/searchTest.js
@@ -147,4 +147,53 @@ describe("Zotero.Search", function() {
assert.deepEqual(matches, [foobarItem.id]);
});
});
+
+ describe("#toJSON()", function () {
+ it("should output all data", function* () {
+ let s = new Zotero.Search();
+ s.name = "Test";
+ s.addCondition('joinMode', 'any');
+ s.addCondition('fulltextWord', 'contains', 'afsgagsdg');
+ let json = s.toJSON();
+ assert.equal(json.name, "Test");
+ assert.lengthOf(json.conditions, 2);
+ assert.equal(json.conditions[0].condition, 'joinMode');
+ assert.equal(json.conditions[0].operator, 'any');
+ assert.equal(json.conditions[1].condition, 'fulltextWord');
+ assert.equal(json.conditions[1].operator, 'contains');
+ assert.equal(json.conditions[1].value, 'afsgagsdg');
+ });
+ });
+
+ describe("#fromJSON()", function () {
+ it("should update all data", function* () {
+ let s = new Zotero.Search();
+ s.name = "Test";
+ s.addCondition('joinMode', 'any');
+ let json = s.toJSON();
+ json.name = "Test 2";
+ json.conditions = [
+ {
+ condition: 'title',
+ operator: 'contains',
+ value: 'foo'
+ },
+ {
+ condition: 'year',
+ operator: 'is',
+ value: '2016'
+ }
+ ];
+ s.fromJSON(json);
+ assert.equal(s.name, "Test 2");
+ var conditions = s.getConditions();
+ assert.lengthOf(Object.keys(conditions), 2);
+ assert.equal(conditions["0"].condition, 'title');
+ assert.equal(conditions["0"].operator, 'contains');
+ assert.equal(conditions["0"].value, 'foo');
+ assert.equal(conditions["1"].condition, 'year');
+ assert.equal(conditions["1"].operator, 'is');
+ assert.equal(conditions["1"].value, '2016');
+ });
+ });
});
diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
index 2fc2ba5ee..2a02e3821 100644
--- a/test/tests/syncEngineTest.js
+++ b/test/tests/syncEngineTest.js
@@ -449,7 +449,7 @@ describe("Zotero.Sync.Data.Engine", function () {
break;
case 'search':
- assert.typeOf(cached.data.conditions, 'object');
+ assert.isArray(cached.data.conditions);
break;
}
}
diff --git a/test/tests/zoteroPaneTest.js b/test/tests/zoteroPaneTest.js
index 135a645e0..dfe1b7f5c 100644
--- a/test/tests/zoteroPaneTest.js
+++ b/test/tests/zoteroPaneTest.js
@@ -290,4 +290,24 @@ describe("ZoteroPane", function() {
assert.equal(cv.getSelectedLibraryID(), userLibraryID);
});
});
+
+ describe("#editSelectedCollection()", function () {
+ it("should edit a saved search", function* () {
+ var search = yield createDataObject('search');
+ var promise = waitForWindow('chrome://zotero/content/searchDialog.xul', function (win) {
+ let searchBox = win.document.getElementById('search-box');
+ var c = searchBox.search.getCondition(
+ searchBox.search.addCondition("title", "contains", "foo")
+ );
+ searchBox.addCondition(c);
+ Zotero.debug("ACCEPTING");
+ win.document.documentElement.acceptDialog();
+ });
+ yield zp.editSelectedCollection();
+ yield promise;
+ var conditions = search.getConditions();
+ Zotero.debug(conditions);
+ assert.lengthOf(Object.keys(conditions), 2);
+ });
+ });
})