commit 3fc09add3a89d5db70547d942214b3ff6e35b755
parent 6e2d1f683ab64fac0275b74cfc6395d3ea219fb9
Author: Dan Stillman <dstillman@zotero.org>
Date: Sat, 23 May 2015 04:25:47 -0400
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
Diffstat:
9 files changed, 224 insertions(+), 137 deletions(-)
diff --git 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<Zotero.Item>} - 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<ids.length; i++) {
- let col = yield Zotero.Collections.getAsync(ids[i]);
- yield col.addItem(itemID);
- }
- }
}.bind(this))
.catch(function (e) {
Zotero.debug(e, 1);
@@ -482,7 +496,7 @@ Zotero.Attachments = new function(){
/**
* TODO: what if called on file:// document?
*
- * @param {Object} options - 'document', 'parentItemID', 'parentCollectionIDs'
+ * @param {Object} options - 'document', 'parentItemID', 'collections'
* @return {Promise}
*/
this.linkFromDocument = Zotero.Promise.coroutine(function* (options) {
@@ -490,13 +504,10 @@ Zotero.Attachments = new function(){
var document = options.document;
var parentItemID = options.parentItemID;
- var parentCollectionIDs = options.parentCollectionIDs;
+ var collections = options.collections;
- if (parentItemID && parentCollectionIDs) {
- let msg = "parentCollectionIDs is ignored when parentItemID is set in Zotero.Attachments.linkFromDocument()";
- Zotero.debug(msg, 2);
- Components.utils.reportError(msg);
- parentCollectionIDs = undefined;
+ if (parentItemID && collections) {
+ throw new Error("parentItemID and collections cannot both be provided");
}
var url = document.location.href;
@@ -511,17 +522,9 @@ Zotero.Attachments = new function(){
linkMode: this.LINK_MODE_LINKED_URL,
contentType: contentType,
charset: document.characterSet,
- parentItemID: parentItemID
+ parentItemID: parentItemID,
+ collections: collections
});
-
- // Add to collections
- if (parentCollectionIDs) {
- var ids = Zotero.flattenArguments(parentCollectionIDs);
- for (let i=0; i<ids.length; i++) {
- let col = yield Zotero.Collections.getAsync(id);
- yield col.addItem(itemID);
- }
- }
}.bind(this));
// Run the indexer asynchronously
@@ -542,7 +545,7 @@ Zotero.Attachments = new function(){
/**
* Save a snapshot -- uses synchronous WebPageDump or asynchronous saveURI()
*
- * @param {Object} options - 'libraryID', 'document', 'parentItemID', 'forceTitle', 'parentCollectionIDs'
+ * @param {Object} options - 'libraryID', 'document', 'parentItemID', 'forceTitle', 'collections'
* @return {Promise<Zotero.Item>} - 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<ids.length; i++) {
- let col = yield Zotero.Collections.getAsync(ids[i]);
- yield col.addItem(itemID);
- }
- }
}.bind(this))
.catch(function (e) {
Zotero.debug(e, 1);
@@ -1286,6 +1280,7 @@ Zotero.Attachments = new function(){
var contentType = options.contentType;
var charset = options.charset;
var parentItemID = options.parentItemID;
+ var collections = options.collections;
return Zotero.DB.executeTransaction(function* () {
var attachmentItem = new Zotero.Item('attachment');
@@ -1312,6 +1307,9 @@ Zotero.Attachments = new function(){
attachmentItem.attachmentLinkMode = linkMode;
attachmentItem.attachmentContentType = contentType;
attachmentItem.attachmentCharset = charset;
+ if (collections) {
+ attachmentItem.setCollections(collections);
+ }
yield attachmentItem.save();
return attachmentItem.id;
diff --git a/chrome/content/zotero/xpcom/collectionTreeView.js b/chrome/content/zotero/xpcom/collectionTreeView.js
@@ -2007,29 +2007,28 @@ Zotero.CollectionTreeView.prototype.drop = Zotero.Promise.coroutine(function* (r
// Otherwise file, so fall through
}
- yield Zotero.DB.executeTransaction(function* () {
- if (dropEffect == 'link') {
- var itemID = yield Zotero.Attachments.linkFromFile(file);
- }
- else {
- var itemID = yield Zotero.Attachments.importFromFile(file, false, 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,
+ collections: [parentCollectionID]
+ });
+ }
+ else {
+ var itemID = yield Zotero.Attachments.importFromFile({
+ file: file,
+ libraryID: targetLibraryID,
+ collections: [parentCollectionID]
+ });
+ // If moving, delete original file
+ if (dragData.dropEffect == 'move') {
+ try {
+ file.remove(false);
}
- }
- if (parentCollectionID) {
- var col = yield Zotero.Collections.getAsync(parentCollectionID);
- if (col) {
- col.addItem(itemID);
+ catch (e) {
+ Components.utils.reportError("Error deleting original file " + file.path + " after drag");
}
}
- });
+ }
}
}
finally {
diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
@@ -2135,7 +2135,7 @@ Zotero.Item.prototype.getFile = function () {
try {
var file = Components.classes["@mozilla.org/file/local;1"].
createInstance(Components.interfaces.nsILocalFile);
- file.persistentDescriptor = row.path;
+ file.persistentDescriptor = path;
}
catch (e) {
Zotero.debug('Invalid persistent descriptor', 2);
@@ -2153,7 +2153,7 @@ Zotero.Item.prototype.getFile = function () {
createInstance(Components.interfaces.nsILocalFile);
try {
- file.persistentDescriptor = row.path;
+ file.persistentDescriptor = path;
}
catch (e) {
// See if this is an old relative path (deprecated)
@@ -2161,7 +2161,7 @@ Zotero.Item.prototype.getFile = function () {
try {
var refDir = (row.linkMode == this.LINK_MODE_LINKED_FILE)
? Zotero.getZoteroDirectory() : Zotero.getStorageDirectory();
- file.setRelativeDescriptor(refDir, row.path);
+ file.setRelativeDescriptor(refDir, path);
}
catch (e) {
Zotero.debug('Invalid relative descriptor', 2);
@@ -2177,8 +2177,7 @@ Zotero.Item.prototype.getFile = function () {
/**
* Get the absolute file path for the attachment
*
- * @return {Promise<string|false>} - 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
@@ -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
@@ -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);
}
- }
- if (parentCollectionID) {
- var col = yield Zotero.Collections.getAsync(parentCollectionID);
- if (col) {
- yield col.addItem(itemID);
+ catch (e) {
+ Components.utils.reportError("Error deleting original file " + file.path + " after drag");
}
}
- });
+ }
}
}
finally {
diff --git 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
@@ -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
@@ -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
@@ -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);
