From ff798d332bdde9cf92d4a9fd490f9138c2f9022b Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Wed, 4 Oct 2017 21:35:59 -0400
Subject: [PATCH] Avoid double item save when adding attachment

---
 chrome/content/zotero/xpcom/attachments.js | 60 +++++++++-------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js
index 82c548075..9d51000af 100644
--- a/chrome/content/zotero/xpcom/attachments.js
+++ b/chrome/content/zotero/xpcom/attachments.js
@@ -170,6 +170,7 @@ Zotero.Attachments = new function(){
 		Zotero.debug('Importing snapshot from file');
 		
 		var file = Zotero.File.pathToFile(options.file);
+		var fileName = file.leafName;
 		var url = options.url;
 		var title = options.title;
 		var contentType = options.contentType;
@@ -193,6 +194,7 @@ Zotero.Attachments = new function(){
 				attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL;
 				attachmentItem.attachmentContentType = contentType;
 				attachmentItem.attachmentCharset = charset;
+				attachmentItem.attachmentPath = 'storage:' + fileName;
 				
 				// DEBUG: this should probably insert access date too so as to
 				// create a proper item, but at the moment this is only called by
@@ -207,9 +209,6 @@ Zotero.Attachments = new function(){
 				// Point to copied file
 				newFile = destDir.clone();
 				newFile.append(file.leafName);
-				
-				attachmentItem.attachmentPath = newFile.path;
-				yield attachmentItem.save();
 			}.bind(this));
 			yield _postProcessFile(attachmentItem, newFile, contentType, charset);
 		}
@@ -323,9 +322,8 @@ Zotero.Attachments = new function(){
 			// Create a temporary directory to save to within the storage directory.
 			// We don't use the normal temp directory because people might have 'storage'
 			// symlinked to another volume, which makes moving complicated.
-			var tmpDir = yield this.createTemporaryStorageDirectory();
-			var tmpFile = tmpDir.clone();
-			tmpFile.append(fileName);
+			var tmpDir = (yield this.createTemporaryStorageDirectory()).path;
+			var tmpFile = OS.Path.join(tmpDir, fileName);
 			
 			// Save to temp dir
 			var deferred = Zotero.Promise.defer();
@@ -373,25 +371,20 @@ Zotero.Attachments = new function(){
 					if (collections) {
 						attachmentItem.setCollections(collections);
 					}
+					attachmentItem.attachmentPath = 'storage:' + fileName;
 					var itemID = yield attachmentItem.save(saveOptions);
-
-					// Create a new folder for this item in the storage directory
-					destDir = this.getStorageDirectory(attachmentItem);
-					yield OS.File.move(tmpDir.path, destDir.path);
-					var destFile = destDir.clone();
-					destFile.append(fileName);
-
-					// Refetch item to update path
-					attachmentItem.attachmentPath = destFile.path;
-					yield attachmentItem.save(saveOptions);
+					
+					// DEBUG: Does this fail if 'storage' is symlinked to another drive?
+					destDir = this.getStorageDirectory(attachmentItem).path;
+					yield OS.File.move(tmpDir, destDir);
 				}.bind(this));
 			} catch (e) {
 				try {
-					if (tmpDir && tmpDir.exists()) {
-						tmpDir.remove(true);
+					if (tmpDir) {
+						yield OS.File.removeDir(tmpDir, { ignoreAbsent: true });
 					}
-					if (destDir && destDir.exists()) {
-						destDir.remove(true);
+					if (destDir) {
+						yield OS.File.removeDir(destDir, { ignoreAbsent: true });
 					}
 				}
 				catch (e) {
@@ -580,11 +573,10 @@ Zotero.Attachments = new function(){
 			contentType = "application/pdf";
 		}
 		
-		var tmpDir = yield this.createTemporaryStorageDirectory();
+		var tmpDir = (yield this.createTemporaryStorageDirectory()).path;
 		try {
-			var tmpFile = tmpDir.clone();
 			var fileName = Zotero.File.truncateFileName(_getFileNameFromURL(url, contentType), 100);
-			tmpFile.append(fileName);
+			var tmpFile = OS.Path.join(tmpDir, fileName);
 			
 			// If we're using the title from the document, make some adjustments
 			if (!options.title) {
@@ -601,7 +593,7 @@ Zotero.Attachments = new function(){
 			
 			if (contentType === 'text/html' || contentType === 'application/xhtml+xml') {
 				Zotero.debug('Saving document with saveDocument()');
-				yield Zotero.Utilities.Internal.saveDocument(document, tmpFile.path);
+				yield Zotero.Utilities.Internal.saveDocument(document, tmpFile);
 			}
 			else {
 				Zotero.debug("Saving file with saveURI()");
@@ -643,16 +635,12 @@ Zotero.Attachments = new function(){
 				if (collections && collections.length) {
 					attachmentItem.setCollections(collections);
 				}
+				attachmentItem.attachmentPath = 'storage:' + fileName;
 				var itemID = yield attachmentItem.save();
 				
-				// Create a new folder for this item in the storage directory
-				destDir = this.getStorageDirectory(attachmentItem);
-				yield OS.File.move(tmpDir.path, destDir.path);
-				var destFile = destDir.clone();
-				destFile.append(fileName);
-				
-				attachmentItem.attachmentPath = destFile.path;
-				yield attachmentItem.save();
+				// DEBUG: Does this fail if 'storage' is symlinked to another drive?
+				destDir = this.getStorageDirectory(attachmentItem).path;
+				yield OS.File.move(tmpDir, destDir);
 			}.bind(this));
 		}
 		catch (e) {
@@ -660,11 +648,11 @@ Zotero.Attachments = new function(){
 			
 			// Clean up
 			try {
-				if (tmpDir && tmpDir.exists()) {
-					tmpDir.remove(true);
+				if (tmpDir) {
+					yield OS.File.removeDir(tmpDir, { ignoreAbsent: true });
 				}
-				if (destDir && destDir.exists()) {
-					destDir.remove(true);
+				if (destDir) {
+					yield OS.File.removeDir(destDir, { ignoreAbsent: true });
 				}
 			}
 			catch (e) {