suzanne.soy/zotero
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

DB isolation changes and item selection tweaks

Browse Source 
- Add an 'exclusive' option to transactions that causes them to block other
  transactions and wait for other transactions to finish before starting,
  instead of nesting
- Resolve Zotero.DB.waitForTransaction() promise before returning from
  executeTransaction()
- A side effect of the above: wait for a newly created item to be selected in
  the middle pane and rendered in the right-hand pane before returning from
  executeTransaction()
- Don't save items multiple times when adding/removing a non-final creator in
  the Info pane
- Use a simpler, non-recursive method for focusing the next field in the Info
  pane; this prevents "too much recursion" errors if something causes the
  right-hand pane not to be rendered when expected
This commit is contained in:
Dan Stillman 2015-05-03 03:17:53 -04:00
parent 4a0018ec63
commit bdd44e9a44
7 changed files with 182 additions and 146 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

149
chrome/content/zotero/bindings/itembox.xml
View File

@ -1680,20 +1680,9 @@
<method name="hideEditor">
<parameter name="textbox"/>
<body>
<![CDATA[
try {
<body><![CDATA[
return Zotero.spawn(function* () {
Zotero.debug('Hiding editor');
/*
var textbox = Zotero.getAncestorByTagName(t, 'textbox');
if (!textbox){
Zotero.debug('Textbox not found in hideEditor');
return;
}
*/
// TODO: get rid of this?
var saveChanges = this.saveOnEdit;
// Prevent autocomplete breakage in Firefox 3
if (textbox.mController) {
@ -1738,7 +1727,7 @@
if (creatorsToShift > 0) {
//Add extra creators
for (var i=0;i<nameArray.length;i++) {
this.modifyCreator(i+initNumCreators,otherFields);
yield this.modifyCreator(i + initNumCreators, otherFields, true);
}
//Shift existing creators
@ -1760,7 +1749,7 @@
otherFields.lastName=tempName;
otherFields.firstName='';
}
this.modifyCreator(creatorIndex,otherFields);
yield this.modifyCreator(creatorIndex, otherFields, true);
creatorIndex++;
}
this._tabDirection = tabDirectionBuffer;
@ -1773,9 +1762,13 @@
}
}
else {
this.modifyCreator(creatorIndex, otherFields);
yield this.modifyCreator(creatorIndex, otherFields);
}
if (this.saveOnEdit) {
yield this.item.save();
}
var val = this.item.getCreator(creatorIndex);
val = val ? val[creatorField] : null;
@ -1885,15 +1878,8 @@
var focusBox = this._dynamicFields;
this._focusNextField(focusBox, this._lastTabIndex, this._tabDirection == -1);
}
}
// Thrown errors don't seem to show up within XBL without explicit logging
catch (e) {
Zotero.debug(e);
Components.utils.reportError(e);
throw (e);
}
]]>
</body>
}, this);
]]></body>
</method>
@ -2030,7 +2016,7 @@
<method name="modifyCreator">
<parameter name="index"/>
<parameter name="fields"/>
<parameter name="changeGlobally"/>
<parameter name="skipSave"/>
<body><![CDATA[
return Zotero.Promise.try(function () {
var libraryID = this.item.libraryID;
@ -2047,14 +2033,14 @@
return;
}
this.item.removeCreator(index);
if (this.saveOnEdit) {
if (this.saveOnEdit && !skipSave) {
return this.item.save();
}
return;
}
var changed = this.item.setCreator(index, fields);
if (changed && this.saveOnEdit) {
if (changed && this.saveOnEdit && !skipSave) {
return this.item.save();
}
}.bind(this));
@ -2157,75 +2143,52 @@
<![CDATA[
tabindex = parseInt(tabindex);
if (back)
{
switch (tabindex)
{
case 1:
//Zotero.debug('At beginning');
document.getElementById('item-type-menu').focus();
return false;
case this._tabIndexMinCreators:
var nextIndex = 1; // Title field
// Get all fields with ztabindex attributes
var tabbableFields = box.querySelectorAll('label[ztabindex]');
if (!tabbableFields.length) {
Zotero.debug("No tabbable fields found");
return false;
}
var next;
if (back) {
Zotero.debug('Looking for previous tabindex before ' + tabindex, 4);
for (let i = tabbableFields.length - 1; i >= 0; i--) {
if (parseInt(tabbableFields[i].getAttribute('ztabindex')) < tabindex) {
next = tabbableFields[i];
break;
case this._tabIndexMinFields:
// No creators
if (this._tabIndexMaxCreators == 0) {
var nextIndex = 1; // Title field
}
else {
var nextIndex = this._tabIndexMaxCreators;
}
break;
default:
var nextIndex = tabindex - 1;
}
}
}
else
{
switch (tabindex)
{
case 1:
var nextIndex = this._tabIndexMinCreators;
break;
case this._tabIndexMaxCreators:
var nextIndex = this._tabIndexMinFields;
break;
case this._tabIndexMaxFields:
//Zotero.debug('At end');
return false;
default:
var nextIndex = tabindex + 1;
}
}
Zotero.debug('Looking for tabindex ' + nextIndex, 4);
var next = box.getElementsByAttribute('ztabindex', nextIndex);
if (!next[0])
{
//Zotero.debug("Next field not found");
return this._focusNextField(box, nextIndex, back);
}
next[0].click();
// DEBUG: next[0] is always equal to the target element,
// but for some reason it's necessary to scroll to the next
// element when moving forward for the target element to
// be fully in view
if (!back && next[0].parentNode.nextSibling) {
var visElem = next[0].parentNode.nextSibling;
}
else {
var visElem = next[0];
Zotero.debug('Looking for next tabindex after ' + tabindex, 4);
for (var pos = 0; pos < tabbableFields.length; pos++) {
if (parseInt(tabbableFields[pos].getAttribute('ztabindex')) > tabindex) {
next = tabbableFields[pos];
break;
}
}
}
if (!next) {
Zotero.debug("Next field not found");
return false;
}
next.click();
// 1) next.parentNode is always null for some reason
// 2) For some reason it's necessary to scroll to the next element when
// moving forward for the target element to be fully in view
if (!back && tabbableFields[pos + 1]) {
Zotero.debug("Scrolling to next field");
var visElem = tabbableFields[pos + 1];
}
else {
var visElem = next;
}
// DEBUG: This doesn't seem to work anymore
this.ensureElementIsVisible(visElem);
return true;

43
chrome/content/zotero/xpcom/db.js
View File

@ -441,6 +441,9 @@ Zotero.DBConnection.prototype.getNextName = Zotero.Promise.coroutine(function* (
* @return {Promise} - Promise for result of generator function
*/
Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(function* (func, options) {
options = options || {};
var resolve;
// Set temporary options for this transaction that will be reset at the end
var origOptions = {};
if (options) {
@ -450,18 +453,20 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
}
}
if ((options.exclusive && this._inTransaction) || this._inExclusiveTransaction) {
yield Zotero.DB.waitForTransaction();
}
try {
if (this._inTransaction) {
Zotero.debug("Async DB transaction in progress -- increasing level to "
+ ++this._asyncTransactionNestingLevel, 5);
if (options) {
if (options.onCommit) {
this._callbacks.current.commit.push(options.onCommit);
}
if (options.onRollback) {
this._callbacks.current.rollback.push(options.onRollback);
}
if (options.onCommit) {
this._callbacks.current.commit.push(options.onCommit);
}
if (options.onRollback) {
this._callbacks.current.rollback.push(options.onRollback);
}
try {
@ -481,12 +486,10 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
Zotero.debug("Beginning async DB transaction", 5);
this._inTransaction = true;
this._inExclusiveTransaction = options.exclusive;
var resolve;
var reject;
this._transactionPromise = new Zotero.Promise(function () {
resolve = arguments[0];
reject = arguments[1];
});
// Set a timestamp for this transaction
@ -535,8 +538,6 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
}
}
setTimeout(resolve, 0);
return result;
}
}
@ -544,6 +545,7 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
Zotero.debug("Rolled back async DB transaction", 5);
Zotero.debug(e, 1);
this._inTransaction = false;
this._inExclusiveTransaction = false;
if (options) {
// Function to run once transaction has been committed but before any
@ -566,12 +568,6 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
}
}
if (reject) {
setTimeout(function () {
reject(e);
}, 0);
}
throw e;
}
finally {
@ -581,18 +577,21 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
this[option] = origOptions[option];
}
}
// Process all resolvers
if (resolve) {
resolve.call();
}
}
});
Zotero.DBConnection.prototype.waitForTransaction = function () {
if (!this._transactionPromise) {
if (!this._inTransaction) {
return Zotero.Promise.resolve();
}
Zotero.debug("Waiting for transaction to finish");
return this._transactionPromise.then(function () {
Zotero.debug("Done waiting for transaction");
});
return this._transactionPromise;
};

34
chrome/content/zotero/xpcom/itemTreeView.js
View File

@ -932,7 +932,9 @@ Zotero.ItemTreeView.prototype.notify = Zotero.Promise.coroutine(function* (actio
}
//this._treebox.endUpdateBatch();
var promise = this._getItemSelectedPromise();
this.selection.selectEventsSuppressed = false;
yield promise;
});
/*
@ -1588,9 +1590,6 @@ Zotero.ItemTreeView.prototype.sort = Zotero.Promise.coroutine(function* (itemID)
* Select an item
*/
Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (id, expand, noRecurse) {
var selected = this.getSelectedItems(true);
var alreadySelected = selected.length == 1 && selected[0] == id;
// Don't change selection if UI updates are disabled (e.g., during sync)
if (Zotero.suppressUIUpdates) {
Zotero.debug("Sync is running; not selecting item");
@ -1655,20 +1654,14 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i
row = this._itemRowMap[id];
}
// This function calls nsITreeSelection.select(), which triggers the <tree>'s 'onselect'
// attribute, which calls ZoteroPane.itemSelected(), which calls ZoteroItemPane.viewItem(),
// which refreshes the itembox. But since the 'onselect' doesn't handle promises,
// itemSelected() isn't waited for and 'yield selectItem(itemID)' continues before the
// itembox has been refreshed. To get around this, we make a promise resolver that's
// triggered by itemSelected() when it's done.
if (!alreadySelected && !this.selection.selectEventsSuppressed) {
var itemSelectedPromise = new Zotero.Promise(function () {
this._itemSelectedPromiseResolver = {
resolve: arguments[0],
reject: arguments[1]
};
}.bind(this));
if (!this.selection.selectEventsSuppressed) {
var itemSelectedPromise = this._getItemSelectedPromise(id);
}
this.selection.select(row);
@ -1679,7 +1672,7 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i
}
this.selection.select(row);
if (!alreadySelected && !this.selection.selectEventsSuppressed) {
if (!this.selection.selectEventsSuppressed) {
yield itemSelectedPromise;
}
@ -1705,6 +1698,23 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i
});
Zotero.ItemTreeView.prototype._getItemSelectedPromise = function (itemID) {
if (itemID) {
var selected = this.getSelectedItems(true);
var alreadySelected = selected.length == 1 && selected[0] == itemID;
if (alreadySelected) {
return Zotero.Promise.resolve();
}
}
return new Zotero.Promise(function () {
this._itemSelectedPromiseResolver = {
resolve: arguments[0],
reject: arguments[1]
};
}.bind(this));
}
/**
* Select multiple top-level items
*

2
chrome/content/zotero/xpcom/search.js
View File

@ -972,7 +972,7 @@ Zotero.Search.idsToTempTable = function (ids) {
yield Zotero.DB.queryAsync(sql);
return tmpTable;
});
}, { exclusive: true });
}

3
chrome/content/zotero/zoteroPane.js
View File

@ -781,12 +781,11 @@ var ZoteroPane = new function()
// Update most-recently-used list for New Item menu
this.addItemTypeToNewItemTypeMRU(typeID);
yield this.selectItem(itemID);
// Focus the title field
document.getElementById('zotero-editpane-item-box').focusFirstField();
}
return Zotero.Items.get(itemID);
return Zotero.Items.getAsync(itemID);
});

90
test/tests/dbTest.js
View File

@ -1,7 +1,15 @@
describe("Zotero.DB", function() {
var tmpTable = "tmpDBTest";
beforeEach(function* () {
Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tmpTable);
});
after(function* () {
Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tmpTable);
});
describe("#executeTransaction()", function () {
it("should nest concurrent transactions", Zotero.Promise.coroutine(function* () {
var tmpTable = "tmpWaitForTransactions";
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
var resolve1, resolve2, reject1, reject2;
@ -18,8 +26,7 @@ describe("Zotero.DB", function() {
yield Zotero.Promise.delay(100);
assert.equal(Zotero.DB._asyncTransactionNestingLevel, 0);
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (2)");
// Make sure we're still in a transaction
Zotero.DB.transactionDate;
Zotero.DB.transactionDate; // Make sure we're still in a transaction
})
.then(resolve1)
.catch(reject1);
@ -27,8 +34,7 @@ describe("Zotero.DB", function() {
Zotero.DB.executeTransaction(function* () {
assert.equal(Zotero.DB._asyncTransactionNestingLevel, 1);
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (1)");
// Make sure we're still in a transaction
Zotero.DB.transactionDate;
Zotero.DB.transactionDate; // Make sure we're still in a transaction
})
.then(resolve2)
.catch(reject2);
@ -36,8 +42,77 @@ describe("Zotero.DB", function() {
yield Zotero.Promise.all([promise1, promise2]);
}));
it("shouldn't nest transactions if an exclusive transaction is open", Zotero.Promise.coroutine(function* () {
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
var resolve1, resolve2, reject1, reject2;
var promise1 = new Promise(function (resolve, reject) {
resolve1 = resolve;
reject1 = reject;
});
var promise2 = new Promise(function (resolve, reject) {
resolve2 = resolve;
reject2 = reject;
});
Zotero.DB.executeTransaction(function* () {
yield Zotero.Promise.delay(100);
assert.equal(Zotero.DB._asyncTransactionNestingLevel, 0);
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (1)");
Zotero.DB.transactionDate; // Make sure we're still in a transaction
}, { exclusive: true })
.then(resolve1)
.catch(reject1);
Zotero.DB.executeTransaction(function* () {
assert.equal(Zotero.DB._asyncTransactionNestingLevel, 0);
var num = yield Zotero.DB.valueQueryAsync("SELECT COUNT(*) FROM " + tmpTable);
assert.equal(num, 1);
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (2)");
Zotero.DB.transactionDate; // Make sure we're still in a transaction
})
.then(resolve2)
.catch(reject2);
yield Zotero.Promise.all([promise1, promise2]);
}));
it("shouldn't nest an exclusive transaction if another transaction is open", Zotero.Promise.coroutine(function* () {
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
var resolve1, resolve2, reject1, reject2;
var promise1 = new Promise(function (resolve, reject) {
resolve1 = resolve;
reject1 = reject;
});
var promise2 = new Promise(function (resolve, reject) {
resolve2 = resolve;
reject2 = reject;
});
Zotero.DB.executeTransaction(function* () {
yield Zotero.Promise.delay(100);
assert.equal(Zotero.DB._asyncTransactionNestingLevel, 0);
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (1)");
Zotero.DB.transactionDate; // Make sure we're still in a transaction
})
.then(resolve1)
.catch(reject1);
Zotero.DB.executeTransaction(function* () {
assert.equal(Zotero.DB._asyncTransactionNestingLevel, 0);
var num = yield Zotero.DB.valueQueryAsync("SELECT COUNT(*) FROM " + tmpTable);
assert.equal(num, 1);
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (2)");
Zotero.DB.transactionDate; // Make sure we're still in a transaction
}, { exclusive: true })
.then(resolve2)
.catch(reject2);
yield Zotero.Promise.all([promise1, promise2]);
}));
it("should roll back on error", function* () {
var tmpTable = "tmpRollbackOnError";
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
yield Zotero.DB.queryAsync("INSERT INTO " + tmpTable + " VALUES (1)");
try {
@ -59,7 +134,6 @@ describe("Zotero.DB", function() {
});
it("should run onRollback callbacks", function* () {
var tmpTable = "tmpOnRollback";
var callbackRan = false;
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
try {
@ -84,7 +158,6 @@ describe("Zotero.DB", function() {
});
it("should run onRollback callbacks for nested transactions", function* () {
var tmpTable = "tmpOnNestedRollback";
var callback1Ran = false;
var callback2Ran = false;
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
@ -120,7 +193,6 @@ describe("Zotero.DB", function() {
});
it("should not commit nested transactions", function* () {
var tmpTable = "tmpNoCommitNested";
yield Zotero.DB.queryAsync("CREATE TABLE " + tmpTable + " (foo INT)");
try {
yield Zotero.DB.executeTransaction(function* () {

7
test/tests/itemTreeViewTest.js
View File

@ -18,7 +18,6 @@ describe("Zotero.ItemTreeView", function() {
var item = new Zotero.Item('book');
existingItemID = yield item.save();
yield Zotero.Promise.delay(100);
});
after(function () {
if (win) {
@ -53,7 +52,6 @@ describe("Zotero.ItemTreeView", function() {
var id = yield item.save();
// New item should be selected
yield Zotero.Promise.delay(100);
var selected = itemsView.getSelectedItems();
assert.lengthOf(selected, 1);
assert.equal(selected[0].id, id);
@ -92,7 +90,6 @@ describe("Zotero.ItemTreeView", function() {
});
// Existing item should still be selected
yield Zotero.Promise.delay(100);
selected = itemsView.getSelectedItems(true);
assert.lengthOf(selected, 1);
assert.equal(selected[0], existingItemID);
@ -103,7 +100,6 @@ describe("Zotero.ItemTreeView", function() {
var item = new Zotero.Item('book');
var id = yield item.save();
item = yield Zotero.Items.getAsync(id);
yield Zotero.Promise.delay(100);
itemsView.selection.clearSelection();
assert.lengthOf(itemsView.getSelectedItems(), 0);
@ -112,7 +108,6 @@ describe("Zotero.ItemTreeView", function() {
yield item.save();
// Modified item should not be selected
yield Zotero.Promise.delay(100);
assert.lengthOf(itemsView.getSelectedItems(), 0);
});
@ -121,7 +116,6 @@ describe("Zotero.ItemTreeView", function() {
var item = new Zotero.Item('book');
var id = yield item.save();
item = yield Zotero.Items.getAsync(id);
yield Zotero.Promise.delay(100);
yield itemsView.selectItem(id);
var selected = itemsView.getSelectedItems(true);
@ -132,7 +126,6 @@ describe("Zotero.ItemTreeView", function() {
yield item.save();
// Modified item should still be selected
yield Zotero.Promise.delay(100);
selected = itemsView.getSelectedItems(true);
assert.lengthOf(selected, 1);
assert.equal(selected[0], id);