From 08cb63f66db072c7db64911aaff814c4f2e6859d Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sun, 15 Nov 2015 17:41:21 -0500 Subject: [PATCH] Fix various cases of saving items to selected collection This changes Zotero.Translate.Base.translate() to take an options object (in order to take a 'collections' parameter, which is passed to the Zotero.Translate.ItemSaver constructor). The old parameters are still supported with a deprecation warning, and there may be other places that still need to be updated. --- chrome/content/zotero/browser.js | 33 +++++--- chrome/content/zotero/xpcom/itemTreeView.js | 2 +- .../zotero/xpcom/translation/translate.js | 46 ++++++++--- .../xpcom/translation/translate_item.js | 11 ++- chrome/content/zotero/zoteroPane.js | 76 ++++++++++--------- test/tests/browserTest.js | 51 +++++++++++++ .../data/metadata/journalArticle-single.html | 12 +++ test/tests/zoteroPaneTest.js | 9 +++ 8 files changed, 181 insertions(+), 59 deletions(-) create mode 100644 test/tests/browserTest.js create mode 100644 test/tests/data/metadata/journalArticle-single.html diff --git a/chrome/content/zotero/browser.js b/chrome/content/zotero/browser.js index 0770a5ec87..f1187f95f7 100644 --- a/chrome/content/zotero/browser.js +++ b/chrome/content/zotero/browser.js @@ -150,29 +150,34 @@ var Zotero_Browser = new function() { * * @param {String} [translator] * @param {Event} [event] + * @return {Promise} */ - this.scrapeThisPage = function (translator, event) { + this.scrapeThisPage = Zotero.Promise.coroutine(function* (translator, event) { // Perform translation var tab = _getTabObject(Zotero_Browser.tabbrowser.selectedBrowser); var page = tab.getPageObject(); if(page.translators && page.translators.length) { page.translate.setTranslator(translator || page.translators[0]); - Zotero_Browser.performTranslation(page.translate); // TODO: async + yield Zotero_Browser.performTranslation(page.translate); } else { - this.saveAsWebPage( + yield this.saveAsWebPage( (event && event.shiftKey) ? !Zotero.Prefs.get('automaticSnapshots') : null ); } - } + }); - // Keep in sync with cmd_zotero_newItemFromCurrentPage + /** + * Keep in sync with cmd_zotero_newItemFromCurrentPage + * + * @return {Promise} + */ this.saveAsWebPage = function (includeSnapshots) { // DEBUG: Possible to just trigger command directly with event? Assigning it to the // command property of the icon doesn't seem to work, and neither does goDoCommand() // from chrome://global/content/globalOverlay.js. Getting the command by id and // running doCommand() works but doesn't pass the event. - ZoteroPane.addItemFromPage('temporaryPDFHack', includeSnapshots); + return ZoteroPane.addItemFromPage('temporaryPDFHack', includeSnapshots); } /* @@ -655,6 +660,8 @@ var Zotero_Browser = new function() { translate.clearHandlers("itemDone"); translate.clearHandlers("attachmentProgress"); + var deferred = Zotero.Promise.defer(); + translate.setHandler("done", function(obj, returnValue) { if(!returnValue) { Zotero_Browser.progress.show(); @@ -669,6 +676,8 @@ var Zotero_Browser = new function() { Zotero_Browser.progress.startCloseTimer(); } Zotero_Browser.isScraping = false; + + deferred.resolve(); }); translate.setHandler("itemDone", function(obj, dbItem, item) { @@ -683,11 +692,6 @@ var Zotero_Browser = new function() { Zotero.Utilities.determineAttachmentIcon(attachment), attachment.title, itemProgress)); } - - // add item to collection, if one was specified - if(collection) { - collection.addItem(dbItem.id); - } }); translate.setHandler("attachmentProgress", function(obj, attachment, progress, error) { @@ -702,7 +706,12 @@ var Zotero_Browser = new function() { } }); - translate.translate(libraryID); + translate.translate({ + libraryID, + collections: collection ? [collection.id] : false + }); + + return deferred.promise; }); diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js index b883bef3ed..e2f6b3c1b1 100644 --- a/chrome/content/zotero/xpcom/itemTreeView.js +++ b/chrome/content/zotero/xpcom/itemTreeView.js @@ -1600,7 +1600,7 @@ Zotero.ItemTreeView.prototype.selectItem = Zotero.Promise.coroutine(function* (i var selected = this.getSelectedItems(true); if (selected.length == 1 && selected[0] == id) { Zotero.debug("Item " + id + " is already selected"); - return; + return true; } var row = this._rowMap[id]; diff --git a/chrome/content/zotero/xpcom/translation/translate.js b/chrome/content/zotero/xpcom/translation/translate.js index 5ef6c6451b..e869289ef3 100644 --- a/chrome/content/zotero/xpcom/translation/translate.js +++ b/chrome/content/zotero/xpcom/translation/translate.js @@ -512,7 +512,11 @@ Zotero.Translate.Sandbox = { var newCallback = function(selectedItems) { callbackExecuted = true; if(haveAsyncHandler) { - translate.translate(translate._libraryID, translate._saveAttachments, selectedItems); + translate.translate({ + libraryID: translate._libraryID, + saveAttachments: translate._saveAttachments, + selectedItems + }); } else { returnedItems = transferObject(selectedItems); } @@ -1182,16 +1186,24 @@ Zotero.Translate.Base.prototype = { * @returns {Promise} Promise resolved with saved items * when translation complete */ - "translate":function(libraryID, saveAttachments) { // initialize properties specific to each translation + "translate": function (options = {}, ...args) { // initialize properties specific to each translation + if (typeof options == 'number') { + Zotero.debug("Translate: translate() now takes an object -- update your code", 2); + options = { + libraryID: options, + saveAttachments: args[0], + selectedItems: args[1] + }; + } + if(!this.translator || !this.translator.length) { - var args = arguments; Zotero.debug("Translate: translate called without specifying a translator. Running detection first."); this.setHandler('translators', function(me, translators) { if(!translators.length) { me.complete(false, "Could not find an appropriate translator"); } else { me.setTranslator(translators); - Zotero.Translate.Base.prototype.translate.apply(me, args); + Zotero.Translate.Base.prototype.translate.call(me, options); } }); this.getTranslators(); @@ -1200,8 +1212,9 @@ Zotero.Translate.Base.prototype = { this._currentState = "translate"; - this._libraryID = libraryID; - this._saveAttachments = saveAttachments === undefined || saveAttachments; + this._libraryID = options.libraryID; + this._collections = options.collections; + this._saveAttachments = options.saveAttachments === undefined || options.saveAttachments; this._savingAttachments = []; this._savingItems = 0; this._waitingForSave = false; @@ -1849,6 +1862,7 @@ Zotero.Translate.Web.prototype._getParameters = function() { Zotero.Translate.Web.prototype._prepareTranslation = function() { this._itemSaver = new Zotero.Translate.ItemSaver({ "libraryID":this._libraryID, + "collections": this._collections, "attachmentMode":Zotero.Translate.ItemSaver[(this._saveAttachments ? "ATTACHMENT_MODE_DOWNLOAD" : "ATTACHMENT_MODE_IGNORE")], "forceTagType":1, "cookieSandbox":this._cookieSandbox, @@ -1860,9 +1874,17 @@ Zotero.Translate.Web.prototype._prepareTranslation = function() { /** * Overload translate to set selectedItems */ -Zotero.Translate.Web.prototype.translate = function(libraryID, saveAttachments, selectedItems) { - this._selectedItems = selectedItems; - return Zotero.Translate.Base.prototype.translate.apply(this, [libraryID, saveAttachments]); +Zotero.Translate.Web.prototype.translate = function (options = {}, ...args) { + if (typeof options == 'number') { + Zotero.debug("Translate: translate() now takes an object -- update your code", 2); + options = { + libraryID: options, + saveAttachments: args[0], + selectedItems: args[1] + }; + } + this._selectedItems = options.selectedItems; + return Zotero.Translate.Base.prototype.translate.call(this, options); } /** @@ -2161,6 +2183,7 @@ Zotero.Translate.Import.prototype._prepareTranslation = function() { this._itemSaver = new Zotero.Translate.ItemSaver({ "libraryID":this._libraryID, + "collections": this._collections, "attachmentMode":Zotero.Translate.ItemSaver[(this._saveAttachments ? "ATTACHMENT_MODE_FILE" : "ATTACHMENT_MODE_IGNORE")], "baseURI":baseURI }); @@ -2411,7 +2434,10 @@ Zotero.Translate.Search.prototype.complete = function(returnValue, error) { if(error) Zotero.debug(this._generateErrorString(error), 3); if(this.translator.length > 1) { this.translator.shift(); - this.translate(this._libraryID, this._saveAttachments); + this.translate({ + libraryID: this._libraryID, + saveAttachments: this._saveAttachments + }); return; } else { error = "No items returned from any translator"; diff --git a/chrome/content/zotero/xpcom/translation/translate_item.js b/chrome/content/zotero/xpcom/translation/translate_item.js index 66601ed965..fa6239fa74 100644 --- a/chrome/content/zotero/xpcom/translation/translate_item.js +++ b/chrome/content/zotero/xpcom/translation/translate_item.js @@ -45,6 +45,8 @@ Zotero.Translate.ItemSaver = function(options) { this._libraryID = options.libraryID; } + this._collections = options.collections || false; + // If group filesEditable==false, don't save attachments this.attachmentMode = Zotero.Libraries.isFilesEditable(this._libraryID) ? options.attachmentMode : Zotero.Translate.ItemSaver.ATTACHMENT_MODE_IGNORE; @@ -108,7 +110,11 @@ Zotero.Translate.ItemSaver.prototype = { id:item.itemID || item.id }; newItem.fromJSON(this._deleteIrrelevantFields(item)); - + + if (this._collections) { + newItem.setCollections(this._collections); + } + // save item myID = yield newItem.save(); @@ -596,6 +602,9 @@ Zotero.Translate.ItemSaver.prototype = { } else { myNote.setNote(note); } + if (this._collections) { + myNote.setCollections(this._collections); + } yield myNote.save(); return myNote; }), diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index d5b0157105..6ce8fa7da3 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -3115,11 +3115,10 @@ var ZoteroPane = new function() if (parentKey) { item.parentKey = parentKey; } - var itemID = yield item.saveTx(); - - if (!parentKey && this.itemsView && this.collectionsView.selectedTreeRow.isCollection()) { - yield this.collectionsView.selectedTreeRow.ref.addItem(itemID); + else if (this.collectionsView.selectedTreeRow.isCollection()) { + item.addToCollection(this.collectionsView.selectedTreeRow.ref.id); } + var itemID = yield item.saveTx(); yield this.selectItem(itemID); @@ -3181,13 +3180,14 @@ var ZoteroPane = new function() }); - this.createItemAndNoteFromSelectedText = function (event) { + this.createItemAndNoteFromSelectedText = Zotero.Promise.coroutine(function* (event) { var str = event.currentTarget.ownerDocument.popupNode.ownerDocument.defaultView.getSelection().toString(); var uri = event.currentTarget.ownerDocument.popupNode.ownerDocument.location.href; - var itemID = ZoteroPane.addItemFromPage(); - var {libraryID, key} = Zotero.Items.getLibraryAndKeyFromID(itemID); - ZoteroPane.newNote(false, key, str, uri) - }; + var item = yield ZoteroPane.addItemFromPage(); + if (item) { + return ZoteroPane.newNote(false, item.key, str, uri) + } + }); @@ -3316,28 +3316,33 @@ var ZoteroPane = new function() }); - this.addItemFromPage = function (itemType, saveSnapshot, row) { - return Zotero.Promise.try(function () { - if(Zotero.isConnector) { - // In connector, save page via Zotero Standalone - var doc = window.content.document; - Zotero.Connector.callMethod("saveSnapshot", {"url":doc.location.toString(), - "cookie":doc.cookie, "html":doc.documentElement.innerHTML, - "skipSnapshot": saveSnapshot === false || (saveSnapshot === true ? false : undefined)}, - function(returnValue, status) { - _showPageSaveStatus(doc.title); - }); - return; - } - - if ((row || (this.collectionsView && this.collectionsView.selection)) && !this.canEdit(row)) { - this.displayCannotEditLibraryMessage(); - return; - } - - return this.addItemFromDocument(window.content.document, itemType, saveSnapshot, row); - }.bind(this)); - } + /** + * @return {Promise|false} + */ + this.addItemFromPage = Zotero.Promise.method(function (itemType, saveSnapshot, row) { + if(Zotero.isConnector) { + // In connector, save page via Zotero Standalone + var doc = window.content.document; + Zotero.Connector.callMethod("saveSnapshot", {"url":doc.location.toString(), + "cookie":doc.cookie, "html":doc.documentElement.innerHTML, + "skipSnapshot": saveSnapshot === false || (saveSnapshot === true ? false : undefined)}, + function(returnValue, status) { + _showPageSaveStatus(doc.title); + }); + return false; + } + + if (!row && this.collectionsView && this.collectionsView.selection) { + row = this.collectionsView.selection.currentIndex; + } + + if (!this.canEdit(row)) { + this.displayCannotEditLibraryMessage(); + return false; + } + + return this.addItemFromDocument(window.content.document, itemType, saveSnapshot, row); + }); /** * Shows progress dialog for a webpage/snapshot save request @@ -3356,6 +3361,7 @@ var ZoteroPane = new function() * @param {String|Integer} [itemType='webpage'] Item type id or name * @param {Boolean} [saveSnapshot] Force saving or non-saving of a snapshot, * regardless of automaticSnapshots pref + * @return {Promise|false} */ this.addItemFromDocument = Zotero.Promise.coroutine(function* (doc, itemType, saveSnapshot, row) { _showPageSaveStatus(doc.title); @@ -3399,7 +3405,7 @@ var ZoteroPane = new function() if (row && !this.canEdit(row)) { this.displayCannotEditLibraryMessage(); - return; + return false; } if (row !== undefined) { @@ -3416,7 +3422,7 @@ var ZoteroPane = new function() if (row && !this.canEditFiles(row)) { this.displayCannotEditLibraryFilesMessage(); - return; + return false; } if (collectionTreeRow && collectionTreeRow.isCollection()) { @@ -3433,7 +3439,7 @@ var ZoteroPane = new function() }); yield this.selectItem(item.id); - return; + return false; } } @@ -3467,7 +3473,7 @@ var ZoteroPane = new function() } } - return item.id; + return item; }); diff --git a/test/tests/browserTest.js b/test/tests/browserTest.js new file mode 100644 index 0000000000..22ee6849b2 --- /dev/null +++ b/test/tests/browserTest.js @@ -0,0 +1,51 @@ +"use strict"; + +describe("Zotero_Browser", function () { + var win; + before(function* () { + win = yield loadBrowserWindow(); + }); + after(function* () { + win.close(); + }); + + it("should save webpage item to current collection", function* () { + var uri = OS.Path.join(getTestDataDirectory().path, "snapshot", "index.html"); + var deferred = Zotero.Promise.defer(); + win.addEventListener('pageshow', () => deferred.resolve()); + win.loadURI(uri); + yield deferred.promise; + + yield loadZoteroPane(win); + var collection = yield createDataObject('collection'); + + var promise = waitForItemEvent('add'); + yield win.Zotero_Browser.scrapeThisPage(); + var ids = yield promise; + var items = Zotero.Items.get(ids); + assert.lengthOf(items, 1); + assert.equal(Zotero.ItemTypes.getName(items[0].itemTypeID), 'webpage'); + assert.isTrue(collection.hasItem(items[0].id)); + }) + + it("should save journalArticle to current collection", function* () { + var uri = OS.Path.join( + getTestDataDirectory().path, "metadata", "journalArticle-single.html" + ); + var deferred = Zotero.Promise.defer(); + win.addEventListener('pageshow', () => deferred.resolve()); + win.loadURI(uri); + yield deferred.promise; + + yield loadZoteroPane(win); + var collection = yield createDataObject('collection'); + + var promise = waitForItemEvent('add'); + yield win.Zotero_Browser.scrapeThisPage(); + var ids = yield promise; + var items = Zotero.Items.get(ids); + assert.lengthOf(items, 1); + assert.equal(Zotero.ItemTypes.getName(items[0].itemTypeID), 'journalArticle'); + assert.isTrue(collection.hasItem(items[0].id)); + }) +}) diff --git a/test/tests/data/metadata/journalArticle-single.html b/test/tests/data/metadata/journalArticle-single.html new file mode 100644 index 0000000000..682ae5a0b6 --- /dev/null +++ b/test/tests/data/metadata/journalArticle-single.html @@ -0,0 +1,12 @@ + + + + +Bibliography + + +
+
Rosenzweig, Roy. 2003. “Scarcity or Abundance? Preserving the Past in a Digital Era.” The American Historical Review 108 (3): 735–62. doi:10.2307/3523084.
+ +
+ diff --git a/test/tests/zoteroPaneTest.js b/test/tests/zoteroPaneTest.js index 26968d0e4c..7f225e6889 100644 --- a/test/tests/zoteroPaneTest.js +++ b/test/tests/zoteroPaneTest.js @@ -45,6 +45,15 @@ describe("ZoteroPane", function() { assert.lengthOf(selected, 1); assert.equal(selected, noteID); }) + + it("should create a standalone note within a collection and select it", function* () { + var collection = yield createDataObject('collection'); + var noteID = yield zp.newNote(false, false, "Test"); + assert.equal(zp.collectionsView.getSelectedCollection(), collection); + var selected = zp.itemsView.getSelectedItems(true); + assert.lengthOf(selected, 1); + assert.equal(selected, noteID); + }) }) describe("#itemSelected()", function () {