diff --git a/chrome/content/zotero/bindings/itembox.xml b/chrome/content/zotero/bindings/itembox.xml index 4252596e6..2461891cf 100644 --- a/chrome/content/zotero/bindings/itembox.xml +++ b/chrome/content/zotero/bindings/itembox.xml @@ -1680,20 +1680,9 @@ - - 0) { //Add extra creators for (var i=0;i - + }, this); + ]]> @@ -2030,7 +2016,7 @@ - + = 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; diff --git a/chrome/content/zotero/xpcom/db.js b/chrome/content/zotero/xpcom/db.js index c83712310..e9d70166a 100644 --- a/chrome/content/zotero/xpcom/db.js +++ b/chrome/content/zotero/xpcom/db.js @@ -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; }; diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js index e6b38a38b..96cc0f5f5 100644 --- a/chrome/content/zotero/xpcom/itemTreeView.js +++ b/chrome/content/zotero/xpcom/itemTreeView.js @@ -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 '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 * diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js index 417711190..be7cc2f03 100644 --- a/chrome/content/zotero/xpcom/search.js +++ b/chrome/content/zotero/xpcom/search.js @@ -972,7 +972,7 @@ Zotero.Search.idsToTempTable = function (ids) { yield Zotero.DB.queryAsync(sql); return tmpTable; - }); + }, { exclusive: true }); } diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 41fe1a998..cf9ea0275 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -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); }); diff --git a/test/tests/dbTest.js b/test/tests/dbTest.js index dd4c59260..084e319ee 100644 --- a/test/tests/dbTest.js +++ b/test/tests/dbTest.js @@ -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* () { diff --git a/test/tests/itemTreeViewTest.js b/test/tests/itemTreeViewTest.js index 44ba3e726..f9f121e1f 100644 --- a/test/tests/itemTreeViewTest.js +++ b/test/tests/itemTreeViewTest.js @@ -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);