From 27cb099c8251b902f3dc7dafc06da48da1c0cf1e Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Wed, 8 Feb 2017 14:44:21 -0500
Subject: [PATCH] Allow asynchronous item saving in import translators

This allows for imported items to be saved individually instead of being
saved in a batch after processing the entire imported file (which for
large imports would hang the UI, even if the actual saving was
asynchronous). This also fixes the progress meter during asynchronous
saves.

To take advantage of this, import translators will need to return a
promise when available (using the native Promise object) from doImport()
and wait for optional promises from item.complete().

The logic here can probably be streamlined further. (E.g., we might be
able to say that item.complete() always returns a promise.) It's
complicated by the fact that, at the moment, Promise isn't available in
child sandboxes, though this can probably be fixed.

Tests forthcoming, but they require a translator that supports this,
which needs to be committed separately.

View with -w for a cleaner diff.
---
 chrome/content/zotero/fileInterface.js        |  30 +-
 .../zotero/xpcom/translation/translate.js     | 299 +++++++++++-------
 .../xpcom/translation/translate_item.js       |   7 +-
 chrome/locale/en-US/zotero/zotero.properties  |   1 +
 4 files changed, 201 insertions(+), 136 deletions(-)

diff --git a/chrome/content/zotero/fileInterface.js b/chrome/content/zotero/fileInterface.js
index d8d5b615b..325357348 100644
--- a/chrome/content/zotero/fileInterface.js
+++ b/chrome/content/zotero/fileInterface.js
@@ -329,12 +329,21 @@ var Zotero_File_Interface = new function() {
 			importCollection.name = collectionName;
 			yield importCollection.saveTx();
 		}
-
+		
 		translation.setTranslator(translators[0]);
-		// TODO: Restore a progress meter
-		/*translation.setHandler("itemDone",  function () {
-			Zotero.updateZoteroPaneProgressMeter(translation.getProgress());
-		});*/
+		
+		// Show progress popup
+		var progressWin = new Zotero.ProgressWindow({
+			closeOnClick: false
+		});
+		progressWin.changeHeadline(Zotero.getString('fileInterface.importing'));
+		var icon = 'chrome://zotero/skin/treesource-unfiled' + (Zotero.hiDPI ? "@2x" : "") + '.png';
+		let progress = new progressWin.ItemProgress(icon, OS.Path.basename(translation.path));
+		progressWin.show();
+		
+		translation.setHandler("itemDone",  function () {
+			progress.setProgress(translation.getProgress());
+		});
 
 		yield Zotero.Promise.delay(0);
 
@@ -352,7 +361,6 @@ var Zotero_File_Interface = new function() {
 		
 		// Show popup on completion
 		var numItems = translation.newItems.length;
-		var progressWin = new Zotero.ProgressWindow();
 		progressWin.changeHeadline(Zotero.getString('fileInterface.importComplete'));
 		if (numItems == 1) {
 			var icon = translation.newItems[0].getImageSrc();
@@ -360,10 +368,12 @@ var Zotero_File_Interface = new function() {
 		else {
 			var icon = 'chrome://zotero/skin/treesource-unfiled' + (Zotero.hiDPI ? "@2x" : "") + '.png';
 		}
-		var title = Zotero.getString(`fileInterface.itemsWereImported`, numItems, numItems);
-		progressWin.addLines(title, icon)
-		progressWin.show();
-		progressWin.startCloseTimer();
+		var text = Zotero.getString(`fileInterface.itemsWereImported`, numItems, numItems);
+		progress.setIcon(icon);
+		progress.setText(text);
+		// For synchronous translators, which don't update progress
+		progress.setProgress(100);
+		progressWin.startCloseTimer(5000);
 	});
 	
 	/*
diff --git a/chrome/content/zotero/xpcom/translation/translate.js b/chrome/content/zotero/xpcom/translation/translate.js
index cf38c6add..73710205d 100644
--- a/chrome/content/zotero/xpcom/translation/translate.js
+++ b/chrome/content/zotero/xpcom/translation/translate.js
@@ -80,129 +80,165 @@ Zotero.Translate.Sandbox = {
 		 * @param {Zotero.Translate} translate
 		 * @param {SandboxItem} An item created using the Zotero.Item class from the sandbox
 		 */
-		"_itemDone":function(translate, item) {
-			//Zotero.debug("Translate: Saving item");
-			
-			// warn if itemDone called after translation completed
-			if(translate._complete) {
-				Zotero.debug("Translate: WARNING: Zotero.Item#complete() called after Zotero.done(); please fix your code", 2);
-			}
+		_itemDone: function (translate, item) {
+			var run = function (resolve) {
+				Zotero.debug("Translate: Saving item");
 				
-			const allowedObjects = ["complete", "attachments", "seeAlso", "creators", "tags", "notes"];
-			
-			// Create a new object here, so that we strip the "complete" property
-			var newItem = {};
-			var oldItem = item;
-			for(var i in item) {
-				var val = item[i];
-				if(i === "complete" || (!val && val !== 0)) continue;
-				
-				var type = typeof val;
-				var isObject = type === "object" || type === "xml" || type === "function",
-					shouldBeObject = allowedObjects.indexOf(i) !== -1;
-				if(isObject && !shouldBeObject) {
-					// Convert things that shouldn't be objects to objects
-					translate._debug("Translate: WARNING: typeof "+i+" is "+type+"; converting to string");
-					newItem[i] = val.toString();
-				} else if(shouldBeObject && !isObject) {
-					translate._debug("Translate: WARNING: typeof "+i+" is "+type+"; converting to array");
-					newItem[i] = [val];
-				} else if(type === "string") {
-					// trim strings
-					newItem[i] = val.trim();
-				} else {
-					newItem[i] = val;
+				// warn if itemDone called after translation completed
+				if(translate._complete) {
+					Zotero.debug("Translate: WARNING: Zotero.Item#complete() called after Zotero.done(); please fix your code", 2);
 				}
-			}
-			item = newItem;
-
-			// Clean empty creators
-			if (item.creators) {
-				for (var i=0; i<item.creators.length; i++) {
-					var creator = item.creators[i];
-					if (!creator.firstName && !creator.lastName) {
-						item.creators.splice(i, 1);
-						i--;
+					
+				const allowedObjects = ["complete", "attachments", "seeAlso", "creators", "tags", "notes"];
+				
+				// Create a new object here, so that we strip the "complete" property
+				var newItem = {};
+				var oldItem = item;
+				for(var i in item) {
+					var val = item[i];
+					if(i === "complete" || (!val && val !== 0)) continue;
+					
+					var type = typeof val;
+					var isObject = type === "object" || type === "xml" || type === "function",
+						shouldBeObject = allowedObjects.indexOf(i) !== -1;
+					if(isObject && !shouldBeObject) {
+						// Convert things that shouldn't be objects to objects
+						translate._debug("Translate: WARNING: typeof "+i+" is "+type+"; converting to string");
+						newItem[i] = val.toString();
+					} else if(shouldBeObject && !isObject) {
+						translate._debug("Translate: WARNING: typeof "+i+" is "+type+"; converting to array");
+						newItem[i] = [val];
+					} else if(type === "string") {
+						// trim strings
+						newItem[i] = val.trim();
+					} else {
+						newItem[i] = val;
 					}
 				}
-			}
-
-			// If we're not in a child translator, canonicalize tags
-			if (!translate._parentTranslator) {
-				if(item.tags) item.tags = translate._cleanTags(item.tags);
-			}
-			
-			// if we're not supposed to save the item or we're in a child translator,
-			// just return the item array
-			if(translate._libraryID === false || translate._parentTranslator) {
-				translate.newItems.push(item);
-				if(translate._parentTranslator && Zotero.isFx && !Zotero.isBookmarklet) {
-					// Copy object so it is accessible to child translator
-					item = translate._sandboxManager.copyObject(item);
-					item.complete = oldItem.complete;
+				item = newItem;
+	
+				// Clean empty creators
+				if (item.creators) {
+					for (var i=0; i<item.creators.length; i++) {
+						var creator = item.creators[i];
+						if (!creator.firstName && !creator.lastName) {
+							item.creators.splice(i, 1);
+							i--;
+						}
+					}
 				}
-				translate._runHandler("itemDone", item, item);
+	
+				// If we're not in a child translator, canonicalize tags
+				if (!translate._parentTranslator) {
+					if(item.tags) item.tags = translate._cleanTags(item.tags);
+				}
+				
+				// if we're not supposed to save the item or we're in a child translator,
+				// just return the item array
+				if(translate._libraryID === false || translate._parentTranslator) {
+					translate.newItems.push(item);
+					if(translate._parentTranslator && Zotero.isFx && !Zotero.isBookmarklet) {
+						// Copy object so it is accessible to child translator
+						item = translate._sandboxManager.copyObject(item);
+						item.complete = oldItem.complete;
+					}
+					let maybePromise = translate._runHandler("itemDone", item, item);
+					// DEBUG: Is this ever necessary?
+					if (maybePromise) {
+						return resolve ? maybePromise.then(resolve) : maybePromise;
+					}
+					if (resolve) {
+						resolve();
+					}
+					return;
+				}
+				
+				// We use this within the connector to keep track of items as they are saved
+				if(!item.id) item.id = Zotero.Utilities.randomString();
+				
+				if(item.attachments) {
+					var attachments = item.attachments;
+					for(var j=0; j<attachments.length; j++) {
+						var attachment = attachments[j];
+	
+						// Don't save documents as documents in connector, since we can't pass them around
+						if(Zotero.isConnector && attachment.document) {
+							attachment.url = attachment.document.documentURI || attachment.document.URL;
+							attachment.mimeType = "text/html";
+							delete attachment.document;
+						}
+	
+						// If we're not in a child translator, canonicalize tags
+						if (!translate._parentTranslator) {
+							if(attachment.tags !== undefined) attachment.tags = translate._cleanTags(attachment.tags);
+						}
+					}
+				}
+	
+				if(item.notes) {
+					var notes = item.notes;
+					for(var j=0; j<notes.length; j++) {
+						var note = notes[j];
+						if(!note) {
+							notes.splice(j--, 1);
+						} else if(typeof(note) != "object") {
+							// Convert to object
+							notes[j] = {"note":note.toString()}
+						}
+						// If we're not in a child translator, canonicalize tags
+						if (!translate._parentTranslator) {
+							if(note.tags !== undefined) note.tags = translate._cleanTags(note.tags);
+						}
+					}
+				}
+	
+				if (item.version) {
+					translate._debug("Translate: item.version is deprecated; set item.versionNumber instead");
+					item.versionNumber = item.version;
+				}
+	
+				if (item.accessDate) {
+					if (Zotero.Date.isSQLDateTime(item.accessDate)) {
+						translate._debug("Translate: Passing accessDate as SQL is deprecated; pass an ISO 8601 date instead");
+						item.accessDate = Zotero.Date.sqlToISO8601(item.accessDate);
+					}
+				}
+			
+				// Fire itemSaving event
+				translate._runHandler("itemSaving", item);
+				
+				translate._savingItems++;
+				
+				// For synchronous import (when Promise isn't available in the sandbox) and web
+				// translators, we queue saves
+				if (!resolve || translate instanceof Zotero.Translate.Web) {
+					Zotero.debug("Translate: Saving via queue");
+					translate.saveQueue.push(item);
+					if (resolve) {
+						resolve();
+					}
+				}
+				// For async import, save items immediately
+				else {
+					Zotero.debug("Translate: Saving now");
+					translate._saveItems([item]).then(resolve);
+				}
+			};
+			
+			if (!translate._sandboxManager.sandbox.Promise) {
+				Zotero.debug("Translate: Promise not available in sandbox in _itemDone()");
+				run();
 				return;
 			}
 			
-			// We use this within the connector to keep track of items as they are saved
-			if(!item.id) item.id = Zotero.Utilities.randomString();
-			
-			if(item.attachments) {
-				var attachments = item.attachments;
-				for(var j=0; j<attachments.length; j++) {
-					var attachment = attachments[j];
-
-					// Don't save documents as documents in connector, since we can't pass them around
-					if(Zotero.isConnector && attachment.document) {
-						attachment.url = attachment.document.documentURI || attachment.document.URL;
-						attachment.mimeType = "text/html";
-						delete attachment.document;
-					}
-
-					// If we're not in a child translator, canonicalize tags
-					if (!translate._parentTranslator) {
-						if(attachment.tags !== undefined) attachment.tags = translate._cleanTags(attachment.tags);
-					}
+			return new translate._sandboxManager.sandbox.Promise(function (resolve, reject) {
+				try {
+					run(resolve);
 				}
-			}
-
-			if(item.notes) {
-				var notes = item.notes;
-				for(var j=0; j<notes.length; j++) {
-					var note = notes[j];
-					if(!note) {
-						notes.splice(j--, 1);
-					} else if(typeof(note) != "object") {
-						// Convert to object
-						notes[j] = {"note":note.toString()}
-					}
-					// If we're not in a child translator, canonicalize tags
-					if (!translate._parentTranslator) {
-						if(note.tags !== undefined) note.tags = translate._cleanTags(note.tags);
-					}
+				catch (e) {
+					reject(e);
 				}
-			}
-
-			if (item.version) {
-				translate._debug("Translate: item.version is deprecated; set item.versionNumber instead");
-				item.versionNumber = item.version;
-			}
-
-			if (item.accessDate) {
-				if (Zotero.Date.isSQLDateTime(item.accessDate)) {
-					translate._debug("Translate: Passing accessDate as SQL is deprecated; pass an ISO 8601 date instead");
-					item.accessDate = Zotero.Date.sqlToISO8601(item.accessDate);
-				}
-			}
-		
-			// Fire itemSaving event
-			translate._runHandler("itemSaving", item);
-			
-			// TODO: This used to only be used for some modes. Since it's now used for everything with
-			// async saving, there's probably a bunch of code for the non-queued mode that can be removed.
-			translate.saveQueue.push(item);
-			translate._savingItems++;
+			});
 		},
 		
 		/**
@@ -1320,7 +1356,23 @@ Zotero.Translate.Base.prototype = {
 
 			// translate
 			try {
-				Function.prototype.apply.call(this._sandboxManager.sandbox["do" + this._entryFunctionSuffix], null, this._getParameters());
+				let maybePromise = Function.prototype.apply.call(
+					this._sandboxManager.sandbox["do" + this._entryFunctionSuffix],
+					null,
+					this._getParameters()
+				);
+				// doImport can return a promise to allow for incremental saves (via promise-returning
+				// item.complete() calls)
+				if (maybePromise) {
+					maybePromise
+					.then(
+						() => this.decrementAsyncProcesses("Zotero.Translate#translate()")
+					)
+					.catch((e) => {
+						this.complete(false, e)
+					});
+					return;
+				}
 			} catch (e) {
 				this.complete(false, e);
 				return false;
@@ -1579,11 +1631,14 @@ Zotero.Translate.Base.prototype = {
 				}
 			}
 			
-			// Trigger itemDone events
+			// Trigger itemDone events, waiting for them if they return promises
+			var maybePromises = [];
 			for(var i=0, nItems = items.length; i<nItems; i++) {
-				this._runHandler("itemDone", newItems[i], items[i]);
+				maybePromises.push(this._runHandler("itemDone", newItems[i], items[i]));
 			}
-			
+			return Zotero.Promise.all(maybePromises).then(() => newItems);
+		}.bind(this))
+		.then(function (newItems) {
 			// Specify that itemDone event was dispatched, so that we don't defer
 			// attachmentProgress notifications anymore
 			itemDoneEventsDispatched = true;
@@ -1612,10 +1667,12 @@ Zotero.Translate.Base.prototype = {
 		if(!this._savingItems && !this._savingAttachments.length && (!this._currentState || this._waitingForSave)) {
 			if(this.newCollections && this._itemSaver.saveCollections) {
 				var me = this;
-				this._itemSaver.saveCollections(this.newCollections).then(function (newCollections) {
+				this._itemSaver.saveCollections(this.newCollections)
+				.then(function (newCollections) {
 					me.newCollections = newCollections;
 					me._runHandler("done", true);
-				}, function (err) {
+				})
+				.catch(function (err) {
 					me._runHandler("error", err);
 					me._runHandler("done", false);
 				});
@@ -1767,10 +1824,10 @@ Zotero.Translate.Base.prototype = {
 		
 		if(this instanceof Zotero.Translate.Export || this instanceof Zotero.Translate.Import) {
 			src += "Zotero.Collection = function () {};"+
-			"Zotero.Collection.prototype.complete = function() { Zotero._collectionDone(this); };";
+			"Zotero.Collection.prototype.complete = function() { return Zotero._collectionDone(this); };";
 		}
 		
-		src += "Zotero.Item.prototype.complete = function() { Zotero._itemDone(this); }";
+		src += "Zotero.Item.prototype.complete = function() { return Zotero._itemDone(this); }";
 
 		this._sandboxManager.eval(src);
 		this._sandboxManager.importObject(this.Sandbox, this);
diff --git a/chrome/content/zotero/xpcom/translation/translate_item.js b/chrome/content/zotero/xpcom/translation/translate_item.js
index a1cd61a16..cfc600064 100644
--- a/chrome/content/zotero/xpcom/translation/translate_item.js
+++ b/chrome/content/zotero/xpcom/translation/translate_item.js
@@ -158,18 +158,15 @@ Zotero.Translate.ItemSaver.prototype = {
 			}
 		}.bind(this));
 
-		// Handle standalone attachments outside of the transaction, because they can involve downloading
+		// Handle attachments outside of the transaction, because they can involve downloading
 		for (let item of standaloneAttachments) {
 			let newItem = yield this._saveAttachment(item, null, attachmentCallback);
 			if (newItem) newItems.push(newItem);
 		}
-		// Save child attachments afterwards, since we want to signal completion as soon as the main
-		// items are saved
-		var promise = Zotero.Promise.delay(1);
 		for (let a of childAttachments) {
 			// Workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=449811 (fixed in Fx51?)
 			let [item, parentItemID] = a;
-			promise = promise.then(() => this._saveAttachment(item, parentItemID, attachmentCallback));
+			yield this._saveAttachment(item, parentItemID, attachmentCallback);
 		}
 		
 		return newItems;
diff --git a/chrome/locale/en-US/zotero/zotero.properties b/chrome/locale/en-US/zotero/zotero.properties
index bb5f6fe1a..dc77cd09a 100644
--- a/chrome/locale/en-US/zotero/zotero.properties
+++ b/chrome/locale/en-US/zotero/zotero.properties
@@ -655,6 +655,7 @@ zotero.preferences.advanced.debug.error = An error occurred sending debug output
 dragAndDrop.existingFiles = The following files already existed in the destination directory and were not copied:
 dragAndDrop.filesNotFound = The following files were not found and could not be copied:
 
+fileInterface.importing = Importing…
 fileInterface.importComplete = Import Complete
 fileInterface.itemsWereImported = %1$S item was imported;%1$S items were imported
 fileInterface.itemsExported		= Exporting items…