From ac672cfcc6726b9309904857dd246f5ee54b20a8 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 7 Apr 2022 12:59:06 +1000 Subject: [PATCH] DEV: Improvements to UppyUploadMixin to use ExtendableUploader (#16383) This PR brings the `UppyUploadMixin` more into line with the `ComposerUppyUpload` mixin, by extending the `ExtendableUploader` . This also adds better tracking of and events for in progress uploads in the `UppyUploadMixin` for better UI interactions, and also opens up the use of `_useUploadPlugin` for the mixin, so anything implementing `UppyUploadMixin` can add extra uppy preprocessor plugins as needed. This has been done as part of work on extracting uploads out of the chat composer. In future, we might be able to do the same for `ComposerUppyUpload`, getting rid of that mixin to standardise on `UppyUploadMixin` and have a separate `composer-uploads` component that lives alongside `composer-editor` like what we are doing in https://github.com/discourse/discourse-chat/pull/764 --- .../app/mixins/composer-upload-uppy.js | 5 +- .../app/mixins/extendable-uploader.js | 6 +- .../discourse/app/mixins/uppy-upload.js | 138 ++++++++++++++++-- config/locales/client.en.yml | 1 + 4 files changed, 131 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js index 4023ba0b7c..6dce54f500 100644 --- a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js +++ b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js @@ -234,7 +234,10 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, { if (reason === "cancel-all") { return; } - + this.appEvents.trigger( + `${this.composerEventPrefix}:upload-cancelled`, + file.id + ); file.meta.cancelled = true; this._removeInProgressUpload(file.id); this._resetUpload(file, { removePlaceholder: true }); diff --git a/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js b/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js index 9bcf1b54af..ca2a312d3b 100644 --- a/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js +++ b/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js @@ -92,7 +92,7 @@ export default Mixin.create(UploadDebugging, { }); }, - _onPreProcessComplete(callback, allCompleteCallback) { + _onPreProcessComplete(callback, allCompleteCallback = null) { this._uppyInstance.on("preprocess-complete", (file, skipped, pluginId) => { this._consoleDebug( `[${pluginId}] ${skipped ? "skipped" : "completed"} processing file ${ @@ -105,7 +105,9 @@ export default Mixin.create(UploadDebugging, { this._completePreProcessing(pluginId, (allComplete) => { if (allComplete) { this._consoleDebug("[uppy] All upload preprocessors complete!"); - allCompleteCallback(); + if (allCompleteCallback) { + allCompleteCallback(); + } } }); }); diff --git a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js index aaf6b4ec99..5cfef90d95 100644 --- a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js +++ b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js @@ -1,4 +1,6 @@ import Mixin from "@ember/object/mixin"; +import { run } from "@ember/runloop"; +import ExtendableUploader from "discourse/mixins/extendable-uploader"; import { or } from "@ember/object/computed"; import EmberObject from "@ember/object"; import { ajax } from "discourse/lib/ajax"; @@ -23,7 +25,7 @@ import bootbox from "bootbox"; export const HUGE_FILE_THRESHOLD_BYTES = 104_857_600; // 100MB -export default Mixin.create(UppyS3Multipart, { +export default Mixin.create(UppyS3Multipart, ExtendableUploader, { uploading: false, uploadProgress: 0, _uppyInstance: null, @@ -55,6 +57,10 @@ export default Mixin.create(UppyS3Multipart, { this.fileInputEventListener ); this.appEvents.off(`upload-mixin:${this.id}:add-files`, this._addFiles); + this.appEvents.off( + `upload-mixin:${this.id}:cancel-upload`, + this._cancelSingleUpload + ); this._uppyInstance?.close(); this._uppyInstance = null; }, @@ -66,6 +72,10 @@ export default Mixin.create(UppyS3Multipart, { }); this.set("allowMultipleFiles", this.fileInputEl.multiple); this.set("inProgressUploads", []); + this.appEvents.trigger( + `upload-mixin:${this.id}:in-progress-uploads`, + this.inProgressUploads + ); this._bindFileInputChange(); @@ -105,6 +115,7 @@ export default Mixin.create(UppyS3Multipart, { uploadProgress: 0, uploading: isValid && this.autoStartUploads, filesAwaitingUpload: !this.autoStartUploads, + cancellable: isValid && this.autoStartUploads, }); return isValid; }, @@ -141,8 +152,8 @@ export default Mixin.create(UppyS3Multipart, { }, }); + // droptarget is a UI plugin, only preprocessors must call _useUploadPlugin this._uppyInstance.use(DropTarget, this._uploadDropTargetOptions()); - this._uppyInstance.use(UppyChecksum, { capabilities: this.capabilities }); this._uppyInstance.on("progress", (progress) => { if (this.isDestroying || this.isDestroyed) { @@ -153,48 +164,81 @@ export default Mixin.create(UppyS3Multipart, { }); this._uppyInstance.on("upload", (data) => { + this._addNeedProcessing(data.fileIDs.length); const files = data.fileIDs.map((fileId) => this._uppyInstance.getFile(fileId) ); + this.setProperties({ + processing: true, + cancellable: false, + }); files.forEach((file) => { - this.inProgressUploads.push( + // The inProgressUploads is meant to be used to display these uploads + // in a UI, and Ember will only update the array in the UI if pushObject + // is used to notify it. + this.inProgressUploads.pushObject( EmberObject.create({ fileName: file.name, id: file.id, progress: 0, + extension: file.extension, + processing: false, }) ); + this.appEvents.trigger( + `upload-mixin:${this.id}:in-progress-uploads`, + this.inProgressUploads + ); }); }); - this._uppyInstance.on("upload-success", (file, response) => { - this._removeInProgressUpload(file.id); + this._uppyInstance.on("upload-progress", (file, progress) => { + run(() => { + if (this.isDestroying || this.isDestroyed) { + return; + } + const upload = this.inProgressUploads.find((upl) => upl.id === file.id); + if (upload) { + const percentage = Math.round( + (progress.bytesUploaded / progress.bytesTotal) * 100 + ); + upload.set("progress", percentage); + } + }); + }); + + this._uppyInstance.on("upload-success", (file, response) => { if (this.usingS3Uploads) { this.setProperties({ uploading: false, processing: true }); this._completeExternalUpload(file) .then((completeResponse) => { + this._removeInProgressUpload(file.id); + this.appEvents.trigger( + `upload-mixin:${this.id}:upload-success`, + file.name, + completeResponse + ); this.uploadDone( deepMerge(completeResponse, { file_name: file.name }) ); - if (this.inProgressUploads.length === 0) { - this._reset(); - } + this._checkInProgressUploads(); }) .catch((errResponse) => { displayErrorForUpload(errResponse, this.siteSettings, file.name); - if (this.inProgressUploads.length === 0) { - this._reset(); - } + this._checkInProgressUploads(); }); } else { - this.uploadDone( - deepMerge(response?.body || {}, { file_name: file.name }) + this._removeInProgressUpload(file.id); + const upload = response?.body || {}; + this.appEvents.trigger( + `upload-mixin:${this.id}:upload-success`, + file.name, + upload ); - if (this.inProgressUploads.length === 0) { - this._reset(); - } + this.uploadDone(deepMerge(upload, { file_name: file.name })); + this._checkInProgressUploads(); } }); @@ -204,6 +248,21 @@ export default Mixin.create(UppyS3Multipart, { this._reset(); }); + this._uppyInstance.on("file-removed", (file, reason) => { + run(() => { + // we handle the cancel-all event specifically, so no need + // to do anything here. this event is also fired when some files + // are handled by an upload handler + if (reason === "cancel-all") { + return; + } + this.appEvents.trigger( + `upload-mixin:${this.id}:upload-cancelled`, + file.id + ); + }); + }); + // TODO (martin) preventDirectS3Uploads is necessary because some of // the current upload mixin components, for example the emoji uploader, // send the upload to custom endpoints that do fancy things in the rails @@ -228,8 +287,40 @@ export default Mixin.create(UppyS3Multipart, { } } + this._uppyInstance.on("cancel-all", () => { + this.appEvents.trigger(`upload-mixin:${this.id}:uploads-cancelled`); + if (!this.isDestroyed && !this.isDestroying) { + this.set("inProgressUploads", []); + this._triggerInProgressUploadsEvent(); + } + }); + this.appEvents.on(`upload-mixin:${this.id}:add-files`, this._addFiles); + this.appEvents.on( + `upload-mixin:${this.id}:cancel-upload`, + this._cancelSingleUpload + ); this._uppyReady(); + + // It is important that the UppyChecksum preprocessor is the last one to + // be added; the preprocessors are run in order and since other preprocessors + // may modify the file (e.g. the UppyMediaOptimization one), we need to + // checksum once we are sure the file data has "settled". + this._useUploadPlugin(UppyChecksum, { capabilities: this.capabilities }); + }, + + _triggerInProgressUploadsEvent() { + this.appEvents.trigger( + `upload-mixin:${this.id}:in-progress-uploads`, + this.inProgressUploads + ); + }, + + _checkInProgressUploads() { + this._triggerInProgressUploadsEvent(); + if (this.inProgressUploads.length === 0) { + this._reset(); + } }, // This should be overridden in a child component if you need to @@ -325,6 +416,12 @@ export default Mixin.create(UppyS3Multipart, { ); }, + @bind + _cancelSingleUpload(data) { + this._uppyInstance.removeFile(data.fileId); + this._removeInProgressUpload(data.fileId); + }, + @bind _addFiles(files, opts = {}) { files = Array.isArray(files) ? files : [files]; @@ -362,6 +459,7 @@ export default Mixin.create(UppyS3Multipart, { this.setProperties({ uploading: false, processing: false, + cancellable: false, uploadProgress: 0, filesAwaitingUpload: false, }); @@ -369,10 +467,18 @@ export default Mixin.create(UppyS3Multipart, { }, _removeInProgressUpload(fileId) { + if (this.isDestroyed || this.isDestroying) { + return; + } + this.set( "inProgressUploads", this.inProgressUploads.filter((upl) => upl.id !== fileId) ); + this.appEvents.trigger( + `upload-mixin:${this.id}:in-progress-uploads`, + this.inProgressUploads + ); }, // target must be provided as a DOM element, however the diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 69f65db27c..00327d98d9 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -395,6 +395,7 @@ en: upload: "Upload" uploading: "Uploading..." + processing: "Processing..." uploading_filename: "Uploading: %{filename}..." processing_filename: "Processing: %{filename}..." clipboard: "clipboard"