From 3fc09add3a89d5db70547d942214b3ff6e35b755 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sat, 23 May 2015 04:25:47 -0400 Subject: [PATCH] Attachment fixes Change all attachment functions to take parameter objects, including a 'collections' property to assign collections. (Previously, calling code assigned collections separately, which required a nested transaction, which is no longer possible.) Fixes #723, Can't attach files by dragging --- chrome/content/zotero/xpcom/attachments.js | 136 +++++++++--------- .../zotero/xpcom/collectionTreeView.js | 41 +++--- chrome/content/zotero/xpcom/data/item.js | 9 +- chrome/content/zotero/xpcom/file.js | 3 + chrome/content/zotero/xpcom/itemTreeView.js | 45 +++--- chrome/content/zotero/zoteroPane.js | 38 ++--- test/tests/attachmentsTest.js | 47 +++++- test/tests/itemTreeViewTest.js | 34 +++++ test/tests/recognizePDFTest.js | 12 +- 9 files changed, 226 insertions(+), 139 deletions(-) diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js index 0022555174..b480457e5a 100644 --- a/chrome/content/zotero/xpcom/attachments.js +++ b/chrome/content/zotero/xpcom/attachments.js @@ -34,15 +34,29 @@ Zotero.Attachments = new function(){ /** + * @param {Object} options + * @param {nsIFile} [options.file] - File to add + * @param {Integer} [options.libraryID] + * @param {Integer[]|String[]} [options.parentItemID] - Parent item to add item to + * @param {Integer[]} [options.collections] - Collection keys or ids to add new item to * @return {Promise} */ - this.importFromFile = Zotero.Promise.coroutine(function* (file, parentItemID, libraryID) { + this.importFromFile = Zotero.Promise.coroutine(function* (options) { Zotero.debug('Importing attachment from file'); + var libraryID = options.libraryID; + var file = options.file; + var parentItemID = options.parentItemID; + var collections = options.collections; + var newName = Zotero.File.getValidFileName(file.leafName); if (!file.isFile()) { - throw ("'" + file.leafName + "' must be a file in Zotero.Attachments.importFromFile()"); + throw new Error("'" + file.leafName + "' must be a file"); + } + + if (parentItemID && collections) { + throw new Error("parentItemID and collections cannot both be provided"); } var itemID, newFile, contentType; @@ -60,11 +74,14 @@ Zotero.Attachments = new function(){ attachmentItem.setField('title', newName); attachmentItem.parentID = parentItemID; attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE; + if (collections) { + attachmentItem.setCollections(collections); + } itemID = yield attachmentItem.save(); attachmentItem = yield Zotero.Items.getAsync(itemID); // Create directory for attachment files within storage directory - var destDir = yield this.createDirectoryForItem(attachmentItem); + destDir = yield this.createDirectoryForItem(attachmentItem); // Point to copied file newFile = destDir.clone(); @@ -105,28 +122,34 @@ Zotero.Attachments = new function(){ /** + * @param {nsIFile} [options.file] + * @param {Integer[]|String[]} [options.parentItemID] - Parent item to add item to + * @param {Integer[]} [options.collections] - Collection keys or ids to add new item to * @return {Promise} */ - this.linkFromFile = Zotero.Promise.coroutine(function* (file, parentItemID) { + this.linkFromFile = Zotero.Promise.coroutine(function* (options) { Zotero.debug('Linking attachment from file'); + var file = options.file; + var parentItemID = options.parentItemID; + var collections = options.collections; + + if (parentItemID && collections) { + throw new Error("parentItemID and collections cannot both be provided"); + } + var title = file.leafName; var contentType = yield Zotero.MIME.getMIMETypeFromFile(file); - var itemID; - - return Zotero.DB.executeTransaction(function* () { - itemID = yield _addToDB({ - file: file, - title: title, - linkMode: this.LINK_MODE_LINKED_FILE, - contentType: contentType, - parentItemID: parentItemID - }); - }.bind(this)) - .then(function () { - return _postProcessFile(itemID, file, contentType); - }) - .then(() => itemID); + var itemID = yield _addToDB({ + file: file, + title: title, + linkMode: this.LINK_MODE_LINKED_FILE, + contentType: contentType, + parentItemID: parentItemID, + collections: collections + }); + yield _postProcessFile(itemID, file, contentType); + return itemID; }); @@ -202,7 +225,7 @@ Zotero.Attachments = new function(){ /** - * @param {Object} options - 'url', 'parentItemID', 'parentCollectionIDs', 'title', + * @param {Object} options - 'url', 'parentItemID', 'collections', 'title', * 'fileBaseName', 'contentType', 'cookieSandbox' * @return {Promise} - A promise for the created attachment item */ @@ -212,17 +235,14 @@ Zotero.Attachments = new function(){ var libraryID = options.libraryID; var url = options.url; var parentItemID = options.parentItemID; - var parentCollectionIDs = options.parentCollectionIDs; + var collections = options.collections; var title = options.title; var fileBaseName = options.forceFileBaseName; var contentType = options.contentType; var cookieSandbox = options.cookieSandbox; - if (parentItemID && parentCollectionIDs) { - let msg = "parentCollectionIDs is ignored when parentItemID is set in Zotero.Attachments.importFromURL()"; - Zotero.debug(msg, 2); - Components.utils.reportError(msg); - parentCollectionIDs = undefined; + if (parentItemID && collections) { + throw new Error("parentItemID and collections cannot both be provided"); } // Throw error on invalid URLs @@ -246,7 +266,7 @@ Zotero.Attachments = new function(){ document: browser.contentDocument, parentItemID: parentItemID, title: title, - parentCollectionIDs: parentCollectionIDs + collections: collections }) .then(function (attachmentItem) { Zotero.Browser.deleteHiddenBrowser(browser); @@ -329,6 +349,9 @@ Zotero.Attachments = new function(){ attachmentItem.parentID = parentItemID; attachmentItem.attachmentLinkMode = Zotero.Attachments.LINK_MODE_IMPORTED_URL; attachmentItem.attachmentContentType = contentType; + if (collections) { + attachmentItem.setCollections(collections); + } var itemID = yield attachmentItem.save(); // Create a new folder for this item in the storage directory @@ -343,15 +366,6 @@ Zotero.Attachments = new function(){ destFile, Zotero.Attachments.LINK_MODE_IMPORTED_URL ); yield attachmentItem.save(); - - // Add to collections - if (parentCollectionIDs) { - var ids = Zotero.flattenArguments(parentCollectionIDs); - for (let i=0; i} - A promise for the created attachment item */ this.importFromDocument = Zotero.Promise.coroutine(function* (options) { @@ -552,13 +555,10 @@ Zotero.Attachments = new function(){ var document = options.document; var parentItemID = options.parentItemID; var title = options.title; - var parentCollectionIDs = options.parentCollectionIDs; + var collections = options.collections; - if (parentItemID && parentCollectionIDs) { - var msg = "parentCollectionIDs is ignored when parentItemID is set in Zotero.Attachments.importFromDocument()"; - Zotero.debug(msg, 2); - Components.utils.reportError(msg); - parentCollectionIDs = undefined; + if (parentItemID && collections) { + throw new Error("parentItemID and parentCollectionIDs cannot both be provided"); } var url = document.location.href; @@ -644,6 +644,9 @@ Zotero.Attachments = new function(){ attachmentItem.attachmentLinkMode = Zotero.Attachments.LINK_MODE_IMPORTED_URL; attachmentItem.attachmentCharset = document.characterSet; attachmentItem.attachmentContentType = contentType; + if (collections) { + attachmentItem.setCollections(collections); + } var itemID = yield attachmentItem.save(); // Create a new folder for this item in the storage directory @@ -657,15 +660,6 @@ Zotero.Attachments = new function(){ destFile, Zotero.Attachments.LINK_MODE_IMPORTED_URL ); yield attachmentItem.save(); - - // Add to collections - if (parentCollectionIDs) { - let ids = Zotero.flattenArguments(parentCollectionIDs); - for (let i=0; i} - A promise for either the absolute file path of the attachment - * or false for invalid paths + * @return {string|false} - The absolute file path of the attachment, or false for invalid paths */ Zotero.Item.prototype.getFilePath = function () { if (!this.isAttachment()) { diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js index e15c96d61f..66b43f6dd6 100644 --- a/chrome/content/zotero/xpcom/file.js +++ b/chrome/content/zotero/xpcom/file.js @@ -255,6 +255,9 @@ Zotero.File = new function(){ * @return {Promise} A promise that is resolved with the contents of the source */ this.getBinaryContentsAsync = function (source, maxLength) { + if (typeof source == 'string') { + source = this.pathToFile(source); + } var deferred = Zotero.Promise.defer(); NetUtil.asyncFetch(source, function(inputStream, status) { if (!Components.isSuccessCode(status)) { diff --git a/chrome/content/zotero/xpcom/itemTreeView.js b/chrome/content/zotero/xpcom/itemTreeView.js index f72279327d..89b166e314 100644 --- a/chrome/content/zotero/xpcom/itemTreeView.js +++ b/chrome/content/zotero/xpcom/itemTreeView.js @@ -3014,8 +3014,8 @@ Zotero.ItemTreeView.prototype.drop = Zotero.Promise.coroutine(function* (row, or var parentItemID = false; var parentCollectionID = false; - var treerow = this.getRow(row); if (orient == 0) { + let treerow = this.getRow(row); parentItemID = treerow.ref.id } else if (collectionTreeRow.isCollection()) { @@ -3073,29 +3073,30 @@ Zotero.ItemTreeView.prototype.drop = Zotero.Promise.coroutine(function* (row, or // Otherwise file, so fall through } - yield Zotero.DB.executeTransaction(function* () { - if (dropEffect == 'link') { - var itemID = yield Zotero.Attachments.linkFromFile(file, parentItemID); - } - else { - var itemID = yield Zotero.Attachments.importFromFile(file, parentItemID, targetLibraryID); - // If moving, delete original file - if (dragData.dropEffect == 'move') { - try { - file.remove(false); - } - catch (e) { - Components.utils.reportError("Error deleting original file " + file.path + " after drag"); - } + if (dropEffect == 'link') { + var itemID = yield Zotero.Attachments.linkFromFile({ + file: file, + parentItemID: parentItemID, + collections: parentCollectionID ? [parentCollectionID] : undefined + }); + } + else { + var itemID = yield Zotero.Attachments.importFromFile({ + file: file, + libraryID: targetLibraryID, + parentItemID: parentItemID, + collections: parentCollectionID ? [parentCollectionID] : undefined + }); + // If moving, delete original file + if (dragData.dropEffect == 'move') { + try { + file.remove(false); + } + catch (e) { + Components.utils.reportError("Error deleting original file " + file.path + " after drag"); } } - if (parentCollectionID) { - var col = yield Zotero.Collections.getAsync(parentCollectionID); - if (col) { - yield col.addItem(itemID); - } - } - }); + } } } finally { diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index c860bc9b92..62e5f25873 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -58,7 +58,6 @@ var ZoteroPane = new function() this.clearItemsPaneMessage = clearItemsPaneMessage; this.contextPopupShowing = contextPopupShowing; this.openNoteWindow = openNoteWindow; - this.addAttachmentFromDialog = addAttachmentFromDialog; this.viewSelectedAttachment = viewSelectedAttachment; this.showAttachmentNotFoundDialog = showAttachmentNotFoundDialog; this.reportErrors = reportErrors; @@ -3227,8 +3226,7 @@ var ZoteroPane = new function() }); - function addAttachmentFromDialog(link, id) - { + this.addAttachmentFromDialog = Zotero.Promise.coroutine(function* (link, parentItemID) { if (!this.canEdit()) { this.displayCannotEditLibraryMessage(); return; @@ -3265,25 +3263,33 @@ var ZoteroPane = new function() if(fp.show() == nsIFilePicker.returnOK) { + if (!parentItemID) { + var collection = this.getSelectedCollection(); + } + var files = fp.files; while (files.hasMoreElements()){ var file = files.getNext(); file.QueryInterface(Components.interfaces.nsILocalFile); - var attachmentID; - if(link) - attachmentID = Zotero.Attachments.linkFromFile(file, id); - else - attachmentID = Zotero.Attachments.importFromFile(file, id, libraryID); - - if(attachmentID && !id) - { - var c = this.getSelectedCollection(); - if(c) - c.addItem(attachmentID); + let attachmentID; + if (link) { + attachmentID = yield Zotero.Attachments.linkFromFile({ + file: file, + parentItemID: parentItemID, + collections: collection ? [collection] : undefined + }); + } + else { + attachmentID = yield Zotero.Attachments.importFromFile({ + file: file, + libraryID: libraryID, + parentItemID: parentItemID, + collections: collection ? [collection] : undefined + }); } } } - } + }); this.addItemFromPage = function (itemType, saveSnapshot, row) { @@ -3398,7 +3404,7 @@ var ZoteroPane = new function() let item = yield Zotero.Attachments.importFromDocument({ libraryID: libraryID, document: doc, - parentCollectionIDs: [collectionID] + collections: [collectionID] }); yield this.selectItem(item.id); diff --git a/test/tests/attachmentsTest.js b/test/tests/attachmentsTest.js index ff17c42809..9137fa9136 100644 --- a/test/tests/attachmentsTest.js +++ b/test/tests/attachmentsTest.js @@ -27,7 +27,10 @@ describe("Zotero.Attachments", function() { var parentItemID = yield item.saveTx(); // Create attachment and compare content - var itemID = yield Zotero.Attachments.importFromFile(tmpFile, parentItemID); + var itemID = yield Zotero.Attachments.importFromFile({ + file: tmpFile, + parentItemID: parentItemID + }); var item = yield Zotero.Items.getAsync(itemID); var storedFile = item.getFile(); assert.equal((yield Zotero.File.getContentsAsync(storedFile)), contents); @@ -36,6 +39,43 @@ describe("Zotero.Attachments", function() { yield Zotero.Items.erase(itemID); }); + it("should create a top-level attachment from a PNG file", function* () { + var file = getTestDataDirectory(); + file.append('test.png'); + var contents = yield Zotero.File.getBinaryContentsAsync(file); + + // Create attachment and compare content + var itemID = yield Zotero.Attachments.importFromFile({ + file: file + }); + var item = yield Zotero.Items.getAsync(itemID); + var storedFile = item.getFile(); + assert.equal((yield Zotero.File.getBinaryContentsAsync(storedFile)), contents); + + // Clean up + yield Zotero.Items.erase(itemID); + }); + + it("should create a top-level attachment from a PNG file in a collection", function* () { + var file = getTestDataDirectory(); + file.append('test.png'); + var contents = yield Zotero.File.getBinaryContentsAsync(file); + + var collection = yield createDataObject('collection'); + + // Create attachment and compare content + var itemID = yield Zotero.Attachments.importFromFile({ + file: file, + collections: [collection.id] + }); + var item = yield Zotero.Items.getAsync(itemID); + var storedFile = item.getFile(); + assert.equal((yield Zotero.File.getBinaryContentsAsync(storedFile)), contents); + + // Clean up + yield Zotero.Items.erase(itemID); + }); + it("should create a child attachment from a PNG file", function* () { var file = getTestDataDirectory(); file.append('test.png'); @@ -46,7 +86,10 @@ describe("Zotero.Attachments", function() { var parentItemID = yield item.saveTx(); // Create attachment and compare content - var itemID = yield Zotero.Attachments.importFromFile(file, parentItemID); + var itemID = yield Zotero.Attachments.importFromFile({ + file: file, + parentItemID: parentItemID + }); var item = yield Zotero.Items.getAsync(itemID); var storedFile = item.getFile(); assert.equal((yield Zotero.File.getBinaryContentsAsync(storedFile)), contents); diff --git a/test/tests/itemTreeViewTest.js b/test/tests/itemTreeViewTest.js index 022ff355a1..84bc06138e 100644 --- a/test/tests/itemTreeViewTest.js +++ b/test/tests/itemTreeViewTest.js @@ -162,4 +162,38 @@ describe("Zotero.ItemTreeView", function() { assert.equal(selected[0], id); }); }) + + describe("#drop()", function () { + it("should create a top-level attachment when a file is dragged", function* () { + var file = getTestDataDirectory(); + file.append('test.png'); + + var deferred = Zotero.Promise.defer(); + itemsView.addEventListener('select', () => deferred.resolve()); + + itemsView.drop(0, -1, { + dropEffect: 'copy', + effectAllowed: 'copy', + types: { + contains: function (type) { + return type == 'application/x-moz-file'; + } + }, + mozItemCount: 1, + mozGetDataAt: function (type, i) { + if (type == 'application/x-moz-file' && i == 0) { + return file; + } + } + }) + + yield deferred.promise; + var items = itemsView.getSelectedItems(); + var path = yield items[0].getFilePathAsync(); + assert.equal( + (yield Zotero.File.getBinaryContentsAsync(path)), + (yield Zotero.File.getBinaryContentsAsync(file)) + ); + }) + }); }) diff --git a/test/tests/recognizePDFTest.js b/test/tests/recognizePDFTest.js index cca94f407e..fecdb4b879 100644 --- a/test/tests/recognizePDFTest.js +++ b/test/tests/recognizePDFTest.js @@ -19,12 +19,14 @@ describe.skip("PDF Recognition", function() { win.close(); }); - it("should recognize a PDF with a DOI", function() { + it("should recognize a PDF with a DOI", function* () { this.timeout(30000); // Import the PDF var testdir = getTestDataDirectory(); testdir.append("recognizePDF_test_DOI.pdf"); - var id = Zotero.Attachments.importFromFile(testdir); + var id = yield Zotero.Attachments.importFromFile({ + file: testdir + }); // Recognize the PDF win.ZoteroPane.selectItem(id); @@ -37,14 +39,16 @@ describe.skip("PDF Recognition", function() { }); }); - it("should recognize a PDF without a DOI", function() { + it("should recognize a PDF without a DOI", function* () { if (Zotero.noUserInput) this.skip(); // CAPTCHAs make this fail this.timeout(30000); // Import the PDF var testdir = getTestDataDirectory(); testdir.append("recognizePDF_test_GS.pdf"); - var id = Zotero.Attachments.importFromFile(testdir); + var id = yield Zotero.Attachments.importFromFile({ + file: testdir + }); // Recognize the PDF win.ZoteroPane.selectItem(id);