From e0bc873bce792e5567f0a7b8b877dc137c28654d Mon Sep 17 00:00:00 2001 From: Martynas Bagdonas Date: Wed, 28 Jul 2021 13:25:25 +0300 Subject: [PATCH] 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 | 4 +- chrome/content/zotero/xpcom/data/notes.js | 82 +++++++++++++++---- chrome/content/zotero/xpcom/editorInstance.js | 81 ++++++++---------- .../zotero/xpcom/storage/storageRequest.js | 6 ++ note-editor | 2 +- test/tests/zfsTest.js | 55 +++++++++++++ 6 files changed, 163 insertions(+), 67 deletions(-) diff --git a/chrome/content/zotero/bindings/noteeditor.xml b/chrome/content/zotero/bindings/noteeditor.xml index ac8985bb01..451ca0a144 100644 --- a/chrome/content/zotero/bindings/noteeditor.xml +++ b/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'); ]]> diff --git a/chrome/content/zotero/xpcom/data/notes.js b/chrome/content/zotero/xpcom/data/notes.js index d8d9c93bcb..0af1bd5128 100644 --- a/chrome/content/zotero/xpcom/data/notes.js +++ b/chrome/content/zotero/xpcom/data/notes.js @@ -32,6 +32,7 @@ Zotero.Notes = new function() { this.__defineGetter__("noteSuffix", function () { return ''; }); 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} */ 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'] diff --git a/chrome/content/zotero/xpcom/editorInstance.js b/chrome/content/zotero/xpcom/editorInstance.js index b289b3dafe..85c0384116 100644 --- a/chrome/content/zotero/xpcom/editorInstance.js +++ b/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 } }); } } } diff --git a/chrome/content/zotero/xpcom/storage/storageRequest.js b/chrome/content/zotero/xpcom/storage/storageRequest.js index d3d15eb468..886f185f3f 100644 --- a/chrome/content/zotero/xpcom/storage/storageRequest.js +++ b/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); + } } }); diff --git a/note-editor b/note-editor index 0d2f920026..f2caf42275 160000 --- a/note-editor +++ b/note-editor @@ -1 +1 @@ -Subproject commit 0d2f9200260f3c2e72487373e3ad10d7e9d1d6aa +Subproject commit f2caf422755f256af82cab5eba534bb0fd1f65a4 diff --git a/test/tests/zfsTest.js b/test/tests/zfsTest.js index afdfd2a2a6..ec721cdf30 100644 --- a/test/tests/zfsTest.js +++ b/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();