Improve embedded note image loading and deletion:

- Delete unused embedded images when note is closed.
- Load images as soon as they are downloaded.
- Introduce new notification for download event, and a test for it.
- Prevent simultaneous downloads of the same attachment.

chrome/content/zotero/bindings/noteeditor.xml

@@ -129,7 +129,7 @@
 
 					this.notify = async (event, type, ids, extraData) => {
 						if (this._editorInstance) {
-							await this._editorInstance.notify(ids);
+							await this._editorInstance.notify(event, type, ids, extraData);
 						}
 
 						if (!this.item) return;
@@ -162,7 +162,7 @@
 						this._id('links-box').refresh();
 					}
 
-					this._notifierID = Zotero.Notifier.registerObserver(this, ['item'], 'noteeditor');
+					this._notifierID = Zotero.Notifier.registerObserver(this, ['item', 'file'], 'noteeditor');
 				]]></constructor>
 
 			<property name="editorInstance" onget="return this._editorInstance"/>

chrome/content/zotero/xpcom/data/notes.js

@@ -32,6 +32,7 @@ Zotero.Notes = new function() {
 	this.__defineGetter__("noteSuffix", function () { return '</div>'; });
 	
 	this._editorInstances = [];
+	this._downloadInProgressPromise = null;
 	
 	/**
 	* Return first line (or first MAX_LENGTH characters) of note content
@@ -197,30 +198,54 @@ Zotero.Notes = new function() {
 	 * @returns {Promise<boolean>}
 	 */
 	this.ensureEmbeddedImagesAreAvailable = async function (item) {
-		var attachments = Zotero.Items.get(item.getAttachments());
-		for (let attachment of attachments) {
-			let path = await attachment.getFilePathAsync();
-			if (!path) {
-				Zotero.debug(`Image file not found for item ${attachment.key}. Trying to download`);
-				let fileSyncingEnabled = Zotero.Sync.Storage.Local.getEnabledForLibrary(item.libraryID);
-				if (!fileSyncingEnabled) {
-					Zotero.debug('File sync is disabled');
-					return false;
-				}
+		let resolvePromise = () => {};
+		if (this._downloadInProgressPromise) {
+			await this._downloadInProgressPromise;
+		}
+		else {
+			this._downloadInProgressPromise = new Promise((resolve) => {
+				resolvePromise = () => {
+					this._downloadInProgressPromise = null;
+					resolve();
+				};
+			});
+		}
+		try {
+			var attachments = Zotero.Items.get(item.getAttachments());
+			for (let attachment of attachments) {
+				let path = await attachment.getFilePathAsync();
+				if (!path) {
+					Zotero.debug(`Image file not found for item ${attachment.key}. Trying to download`);
+					let fileSyncingEnabled = Zotero.Sync.Storage.Local.getEnabledForLibrary(item.libraryID);
+					if (!fileSyncingEnabled) {
+						Zotero.debug('File sync is disabled');
+						resolvePromise();
+						return false;
+					}
 
-				try {
-					let results = await Zotero.Sync.Runner.downloadFile(attachment);
-					if (!results || !results.localChanges) {
-						Zotero.debug('Download failed');
+					try {
+						let results = await Zotero.Sync.Runner.downloadFile(attachment);
+						if (!results || !results.localChanges) {
+							Zotero.debug('Download failed');
+							resolvePromise();
+							return false;
+						}
+					}
+					catch (e) {
+						Zotero.debug(e);
+						resolvePromise();
 						return false;
 					}
 				}
-				catch (e) {
-					Zotero.debug(e);
-					return false;
-				}
 			}
 		}
+		catch (e) {
+			Zotero.debug(e);
+			resolvePromise();
+			return false;
+		}
+
+		resolvePromise();
 		return true;
 	};
 
@@ -275,6 +300,27 @@ Zotero.Notes = new function() {
 		);
 		return !index;
 	};
+	
+	this.deleteUnusedEmbeddedImages = async function (item) {
+		if (!item.isNote()) {
+			throw new Error('Item is not a note');
+		}
+		
+		let note = item.getNote();
+		let parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
+			.createInstance(Components.interfaces.nsIDOMParser);
+		let doc = parser.parseFromString(note, 'text/html');
+		
+		let keys = Array.from(doc.querySelectorAll('img[data-attachment-key]'))
+			.map(node => node.getAttribute('data-attachment-key'));
+
+		let attachments = Zotero.Items.get(item.getAttachments());
+		for (let attachment of attachments) {
+			if (!keys.includes(attachment.key)) {
+				await attachment.eraseTx();
+			}
+		}
+	};
 
 	this.hasSchemaVersion = function (note) {
 		let parser = Components.classes['@mozilla.org/xmlextras/domparser;1']

chrome/content/zotero/xpcom/editorInstance.js

@@ -65,7 +65,6 @@ class EditorInstance {
 		this._state = options.state;
 		this._disableSaving = false;
 		this._subscriptions = [];
-		this._deletedImages = {};
 		this._quickFormatWindow = null;
 		this._isAttachment = this._item.isAttachment();
 		this._citationItemsList = [];
@@ -139,13 +138,28 @@ class EditorInstance {
 		this._iframeWindow.removeEventListener('message', this._messageHandler);
 		Zotero.Notes.unregisterEditorInstance(this);
 		this.saveSync();
+		if (!this._item.isAttachment()) {
+			Zotero.Notes.deleteUnusedEmbeddedImages(this._item);
+		}
 	}
 
 	focus() {
 		this._postMessage({ action: 'focus' });
 	}
 
-	async notify(ids) {
+	async notify(event, type, ids, extraData) {
+		if (type === 'file' && event === 'download') {
+			let items = await Zotero.Items.getAsync(ids);
+			for (let item of items) {
+				if (item.isAttachment() && await item.getFilePathAsync()) {
+					let subscription = this._subscriptions.find(x => x.data.attachmentKey === item.key);
+					if (subscription) {
+						await this._feedSubscription(subscription);
+					}
+				}
+			}
+		}
+		
 		if (this._readOnly || !this._item) {
 			return;
 		}
@@ -643,13 +657,15 @@ class EditorInstance {
 					await this._save(noteData, system);
 					return;
 				}
-				case 'subscribeProvider': {
+				case 'subscribe': {
 					let { subscription } = message;
 					this._subscriptions.push(subscription);
-					await this._feedSubscription(subscription);
+					if (subscription.type === 'image') {
+						await this._feedSubscription(subscription);
+					}
 					return;
 				}
-				case 'unsubscribeProvider': {
+				case 'unsubscribe': {
 					let { id } = message;
 					this._subscriptions.splice(this._subscriptions.findIndex(s => s.id === id), 1);
 					return;
@@ -720,24 +736,6 @@ class EditorInstance {
 					}
 					return;
 				}
-				case 'syncAttachmentKeys': {
-					if (this._readOnly) {
-						return;
-					}
-					let { attachmentKeys } = message;
-					if (this._isAttachment) {
-						return;
-					}
-					let attachmentItems = this._item.getAttachments().map(id => Zotero.Items.get(id));
-					let abandonedItems = attachmentItems.filter(item => !attachmentKeys.includes(item.key));
-					for (let item of abandonedItems) {
-						// Store image data for undo. Although it stays as long
-						// as the note is opened. TODO: Find a better way
-						this._deletedImages[item.key] = await this._getDataURL(item);
-						await item.eraseTx();
-					}
-					return;
-				}
 				case 'openContextMenu': {
 					let { x, y, pos, itemGroups } = message;
 					this._openPopup(x, y, pos, itemGroups);
@@ -772,33 +770,24 @@ class EditorInstance {
 	}
 
 	async _feedSubscription(subscription) {
-		let { id, type, nodeID, data } = subscription;
+		let { id, type, data } = subscription;
 		if (type === 'image') {
 			let { attachmentKey } = data;
 			let item = Zotero.Items.getByLibraryAndKey(this._item.libraryID, attachmentKey);
-			if (!item) {
-				// TODO: Find a better way to undo image deletion,
-				//  probably just keep it in a trash until the note is closed
-				// This recreates the attachment as a completely new item
-				let dataURL = this._deletedImages[attachmentKey];
-				if (dataURL) {
-					// delete this._deletedImages[attachmentKey];
-					// TODO: Fix every repeated undo-redo cycle caching a
-					//  new image copy in memory
-					let newAttachmentKey = await this._importImage(dataURL);
-					// TODO: Inform editor about the failed to import images
-					this._postMessage({ action: 'attachImportedImage', nodeID, attachmentKey: newAttachmentKey });
+			
+			// Note: Images aren't visible in merge dialog because:
+			// - Attachments aren't downloaded at the time
+			// - We are checking if attachments belong to the current note
+			
+			if (item.parentID === this._item.id) {
+				if (await item.getFilePathAsync()) {
+					let src = await this._getDataURL(item);
+					this._postMessage({ action: 'notifySubscription', id, data: { src } });
+				}
+				else {
+					await Zotero.Notes.ensureEmbeddedImagesAreAvailable(this._item);
+					// this._postMessage({ action: 'notifySubscription', id, data: { src: 'error' } });
 				}
-			}
-			// Make sure attachment key belongs to the actual parent note,
-			// otherwise it would be a security risk.
-			// TODO: Figure out what to do with images not being
-			//  displayed in merge dialog because of this,
-			//  although another reason is because items
-			//  are synced before image attachments
-			else if(item.parentID === this._item.id) {
-				let src = await this._getDataURL(item);
-				this._postMessage({ action: 'notifyProvider', id, type, data: { src } });
 			}
 		}
 	}

chrome/content/zotero/xpcom/storage/storageRequest.js

@@ -220,6 +220,12 @@ Zotero.Sync.Storage.Request.prototype.start = Zotero.Promise.coroutine(function*
 		if (this._onStop) {
 			this._onStop.forEach(x => x());
 		}
+
+		if (this.progress == this.progressMax) {
+			var [libraryID, key] = this.name.split('/');
+			var item = Zotero.Items.getByLibraryAndKey(libraryID, key);
+			Zotero.Notifier.trigger('download', 'file', item.id);
+		}
 	}
 });
 

note-editor

@@ -1 +1 @@
-Subproject commit 0d2f9200260f3c2e72487373e3ad10d7e9d1d6aa
+Subproject commit f2caf422755f256af82cab5eba534bb0fd1f65a4

test/tests/zfsTest.js

@@ -294,6 +294,61 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 			assert.equal(library.storageVersion, library.libraryVersion);
 		})
 		
+		it("should notify when file is downloaded", async function () {
+			var { engine, client, caller } = await setup();
+			
+			var library = Zotero.Libraries.userLibrary;
+			library.libraryVersion = 5;
+			await library.saveTx();
+			library.storageDownloadNeeded = true;
+			
+			var item = new Zotero.Item("attachment");
+			item.attachmentLinkMode = 'imported_file';
+			item.attachmentPath = 'storage:test.txt';
+			var text = Zotero.Utilities.randomString();
+			item.attachmentSyncState = "to_download";
+			await item.saveTx();
+			
+			var mtime = "1441252524905";
+			var md5 = Zotero.Utilities.Internal.md5(text);
+			
+			var s3Path = `pretend-s3/${item.key}`;
+			this.httpd.registerPathHandler(
+				`/users/1/items/${item.key}/file`,
+				{
+					handle: function (request, response) {
+						response.setStatusLine(null, 302, "Found");
+						response.setHeader("Zotero-File-Modification-Time", mtime, false);
+						response.setHeader("Zotero-File-MD5", md5, false);
+						response.setHeader("Zotero-File-Compressed", "No", false);
+						response.setHeader("Location", baseURL + s3Path, false);
+					}
+				}
+			);
+			this.httpd.registerPathHandler(
+				"/" + s3Path,
+				{
+					handle: function (request, response) {
+						response.setStatusLine(null, 200, "OK");
+						response.write(text);
+					}
+				}
+			);
+
+			var deferred = Zotero.Promise.defer();
+			var observerID = Zotero.Notifier.registerObserver({
+				notify: async function (event, type, ids, extraData) {
+					if (event == 'download' && ids[0] == item.id && await item.getFilePathAsync()) {
+						deferred.resolve();
+					}
+				}
+			}, 'file', 'testFileDownload');
+			
+			await engine.start();
+			await deferred.promise;
+			Zotero.Notifier.unregisterObserver(observerID);
+		});
+		
 		it("should upload new files", function* () {
 			var { engine, client, caller } = yield setup();