[pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files
Dominik Csapak
d.csapak at proxmox.com
Fri Jul 1 16:21:37 CEST 2022
a few high level comments:
(mentioned it already offline): the window is imho too big. we try to make it functional
at 1280x720 and for that the window is too tall. maybe make it automatically smaller
in that case?
even with the big window, the columns are too narrow to be really functional.
filename/progress/etc were all cut off here with my test isos
(standard linux distro isos)
the popping in&out of the task viewer is a bit irritating, since with fast
storage, i don't get a chance to really read the task log before
it closes on me.. i think it'd be better if we'd just poll the task
in the background and e.g. add a spinner to the progress bar
with: 'copying & verifying' (or similar) and only open the task
log in case of an error
i can add the same isos multiple times. does that make sense?
i know i can use different target names for them, but what would
that be good for? imho preventing the user from uploading
the same iso multiple times would be good
i can add isos while there are still ones uploading, was this intentional?
i think what could make the whole thing a bit better in general is
by having the selecting and uploading part split into two windows:
a select window where you select isos/enter checksums etc.
and when clicking start upload, open a different window with only
the names & progress.
that way the window is not that overloaded and can be smaller
without losing any functionality
also we generally don't use 'abort' but 'Cancel'
remaining comments inline:
On 6/29/22 14:23, Matthias Heiserer wrote:
> Allows queueing multiple files for upload to the storage, which wasn't
> possible with the old upload window.
>
> Signed-off-by: Matthias Heiserer <m.heiserer at proxmox.com>
> ---
> www/manager6/window/UploadToStorage.js | 393 +++++++++++++------------
> 1 file changed, 210 insertions(+), 183 deletions(-)
>
> diff --git a/www/manager6/window/UploadToStorage.js b/www/manager6/window/UploadToStorage.js
> index 0de6d89d..bf656164 100644
> --- a/www/manager6/window/UploadToStorage.js
> +++ b/www/manager6/window/UploadToStorage.js
> @@ -1,3 +1,13 @@
> +Ext.define('pve-multiupload', {
> + extend: 'Ext.data.Model',
> + fields: [
> + 'file', 'filename', 'progressWidget', 'hashsum', 'hashWidget',
> + 'xhr', 'mimetype', 'size',
> + {
> + name: 'hash', defaultValue: '__default__',
> + },
> + ],
> +});
> Ext.define('PVE.window.UploadToStorage', {
> extend: 'Ext.window.Window',
> alias: 'widget.pveStorageUpload',
> @@ -27,93 +37,102 @@ Ext.define('PVE.window.UploadToStorage', {
>
> viewModel: {
> data: {
> - size: '-',
> - mimetype: '-',
> - filename: '',
> + hasFiles: false,
> + uploadInProgress: false,
> },
> },
> -
> controller: {
> - submit: function(button) {
> - const view = this.getView();
> - const form = this.lookup('formPanel').getForm();
> - const abortBtn = this.lookup('abortBtn');
> - const pbar = this.lookup('progressBar');
> -
> - const updateProgress = function(per, bytes) {
> - let text = (per * 100).toFixed(2) + '%';
> - if (bytes) {
> - text += " (" + Proxmox.Utils.format_size(bytes) + ')';
> - }
> - pbar.updateProgress(per, text);
> - };
>
> + addFile: function(input) {
> + let me = this;
> + let grid = me.lookup('grid');
> + for (const file of input.fileInputEl.dom.files) {
> + grid.store.add({
> + file: file,
> + filename: file.name,
> + mimetype: Proxmox.Utils.format_size(file.size),
> + size: file.type,
seems backwards? 'mimetype: ...(file.size)' and 'size: file.type' ?
> + });
> + }
> + },
> +
> + currentUploadIndex: 1,
> + startUpload: function() {
> + const me = this;
> + const view = me.getView();
> + const grid = me.lookup('grid');
> + view.taskDone();
> +
what's that doing here?
why do you call 'taskDone' at the beginning of the upload?
> + const last = grid.store.last();
> + if (!last) {
> + me.getViewModel().set('uploadInProgress', false);
> + return;
> + }
> + const endId = parseInt(last.id.replace('pve-multiupload-', ''), 10);
> + let record;
> + while (!record && me.currentUploadIndex <= endId) {
> + record = grid.store.getById(`pve-multiupload-${me.currentUploadIndex++}`);
> + }
> +
> + if (!record) {
> + me.getViewModel().set('uploadInProgress', false);
> + return;
> + }
AFAICS, you try to get the record with lowest id higher than currentUploadIndex ?
you rely here on extjs generated id, but you could handle that yourself by
e.g. having a list with the added files (and use e.g. the filename as id)
then you have simple lookup like
let filename = me.uploadList[currentUploadIndex];
let record = grid.getStore().getById(filename);
does that make sense?
> +
> + const data = record.data;
> const fd = new FormData();
> -
> - button.setDisabled(true);
> - abortBtn.setDisabled(false);
> -
> fd.append("content", view.content);
> -
> - const fileField = form.findField('file');
> - const file = fileField.fileInputEl.dom.files[0];
> - fileField.setDisabled(true);
> -
> - const filenameField = form.findField('filename');
> - const filename = filenameField.getValue();
> - filenameField.setDisabled(true);
> -
> - const algorithmField = form.findField('checksum-algorithm');
> - algorithmField.setDisabled(true);
> - if (algorithmField.getValue() !== '__default__') {
> - fd.append("checksum-algorithm", algorithmField.getValue());
> -
> - const checksumField = form.findField('checksum');
> - fd.append("checksum", checksumField.getValue()?.trim());
> - checksumField.setDisabled(true);
> + if (data.hash !== '__default__') {
> + fd.append("checksum-algorithm", data.hash);
> + fd.append("checksum", data.hashsum.trim());
> }
> + fd.append("filename", data.file, data.filename);
>
> - fd.append("filename", file, filename);
> -
> - pbar.setVisible(true);
> - updateProgress(0);
> -
> - const xhr = new XMLHttpRequest();
> - view.xhr = xhr;
>
> + const xhr = data.xhr = new XMLHttpRequest();
> xhr.addEventListener("load", function(e) {
> if (xhr.status === 200) {
> - view.hide();
> -
> const result = JSON.parse(xhr.response);
> const upid = result.data;
> Ext.create('Proxmox.window.TaskViewer', {
> autoShow: true,
> upid: upid,
> - taskDone: view.taskDone,
> + taskDone: function(success) {
> + if (success) {
> + this.close();
> + } else {
> + const widget = record.get('progressWidget');
> + widget.updateProgress(0, "ERROR");
> + widget.setStyle('background-color', 'red');
> + }
> + },
> listeners: {
> destroy: function() {
> - view.close();
> + me?.startUpload?.();
why the '?' here? me should be well defined, as should 'startUpload' ?
> },
> },
> });
> + } else {
> + const widget = record.get('progressWidget');
> + widget.updateProgress(0, `ERROR: ${xhr.status}`);
> + widget.setStyle('background-color', 'red');
> + me.getViewModel().set('uploadInProgress', false);
> + }
> + });
>
> - return;
> + const updateProgress = function(per, bytes) {
> + let text = (per * 100).toFixed(2) + '%';
> + if (bytes) {
> + text += " (" + Proxmox.Utils.format_size(bytes) + ')';
> }
> - const err = Ext.htmlEncode(xhr.statusText);
> - let msg = `${gettext('Error')} ${xhr.status.toString()}: ${err}`;
> - if (xhr.responseText !== "") {
> - const result = Ext.decode(xhr.responseText);
> - result.message = msg;
> - msg = Proxmox.Utils.extractRequestError(result, true);
> - }
> - Ext.Msg.alert(gettext('Error'), msg, btn => view.close());
> - }, false);
> + let widget = record.get('progressWidget');
> + widget?.updateProgress(per, text);
here you could omit the intermediate variable:
record.get('progressWidget')?.updateProgress(...);
> + };
>
> xhr.addEventListener("error", function(e) {
> const err = e.target.status.toString();
> const msg = `Error '${err}' occurred while receiving the document.`;
> - Ext.Msg.alert(gettext('Error'), msg, btn => view.close());
> + Ext.Msg.alert(gettext('Error'), msg, _ => view.close());
> });
>
> xhr.upload.addEventListener("progress", function(evt) {
> @@ -123,173 +142,181 @@ Ext.define('PVE.window.UploadToStorage', {
> }
> }, false);
>
> + me.getViewModel().set('uploadInProgress', true);
> xhr.open("POST", `/api2/json${view.url}`, true);
> xhr.send(fd);
> },
>
> - validitychange: function(f, valid) {
> - const submitBtn = this.lookup('submitBtn');
> - submitBtn.setDisabled(!valid);
> - },
> -
> - fileChange: function(input) {
> - const vm = this.getViewModel();
> - const name = input.value.replace(/^.*(\/|\\)/, '');
> - const fileInput = input.fileInputEl.dom;
> - vm.set('filename', name);
> - vm.set('size', (fileInput.files[0] && Proxmox.Utils.format_size(fileInput.files[0].size)) || '-');
> - vm.set('mimetype', (fileInput.files[0] && fileInput.files[0].type) || '-');
> - },
> -
> - hashChange: function(field, value) {
> - const checksum = this.lookup('downloadUrlChecksum');
> - if (value === '__default__') {
> - checksum.setDisabled(true);
> - checksum.setValue("");
> - } else {
> - checksum.setDisabled(false);
> - }
> + removeFile: function(_view, _rowIndex, _colIndex, _item, _event, record) {
> + let me = this;
> + me.lookup('grid').store.remove(record);
> + me.getViewModel().set('uploadInProgress', false);
> },
> },
>
> items: [
> {
> - xtype: 'form',
> - reference: 'formPanel',
> - method: 'POST',
> - waitMsgTarget: true,
> - bodyPadding: 10,
> - border: false,
> - width: 400,
> - fieldDefaults: {
> - labelWidth: 100,
> - anchor: '100%',
> - },
> - items: [
> - {
> - xtype: 'filefield',
> - name: 'file',
> - buttonText: gettext('Select File'),
> - allowBlank: false,
> - fieldLabel: gettext('File'),
> - cbind: {
> - accept: '{extensions}',
> - },
> - listeners: {
> - change: 'fileChange',
> + xtype: 'grid',
> + reference: 'grid',
> + height: 700,
> + width: 1100,
> + store: {
> + listeners: {
> + remove: function(_store, records) {
> + records.forEach(record => {
> + record.get('xhr')?.abort();
> + record.progressWidget = null;
> + record.hashWidget = null;
> + });
> },
> },
> + model: 'pve-multiupload',
> + },
> + listeners: {
> + beforedestroy: function(grid) {
> + grid.store.each(record => grid.store.remove(record));
> + },
> + },
a small comment above that it's done to break the cyclic reference
would be nice. this is something that can be easily overlooked and
does not happen to often in our code, so having a comment
prevents someone from removing it after determining that it's not
necessary (without knowing it's purpose)
> + columns: [
> {
> - xtype: 'textfield',
> - name: 'filename',
> - allowBlank: false,
> - fieldLabel: gettext('File name'),
> - bind: {
> - value: '{filename}',
> - },
> - cbind: {
> - regex: '{filenameRegex}',
> - },
> - regexText: gettext('Wrong file extension'),
> + header: gettext('Source Name'),
> + dataIndex: 'file',
> + renderer: file => file.name,
> + flex: 2,
> },
> {
> - xtype: 'displayfield',
> - name: 'size',
> - fieldLabel: gettext('File size'),
> - bind: {
> - value: '{size}',
> + header: gettext('File Name'),
> + dataIndex: 'filename',
> + flex: 2,
> + xtype: 'widgetcolumn',
> + widget: {
> + xtype: 'textfield',
> + listeners: {
> + change: function(widget, newValue, oldValue) {
> + const record = widget.getWidgetRecord();
> + record.set('filename', newValue);
> + },
we already have a controller, could these functions live there?
the main advantage of having a controller in the first place is that
we nicely can seperate the behaviour from the layout/configuration
> + },
> + cbind: {
> + regex: '{filenameRegex}',
> + },
> + regexText: gettext('Wrong file extension'),
> },
> },
> {
> - xtype: 'displayfield',
> - name: 'mimetype',
> - fieldLabel: gettext('MIME type'),
> - bind: {
> - value: '{mimetype}',
> - },
> + header: gettext('File size'),
> + dataIndex: 'size',
> },
> {
> - xtype: 'pveHashAlgorithmSelector',
> - name: 'checksum-algorithm',
> - fieldLabel: gettext('Hash algorithm'),
> - allowBlank: true,
> - hasNoneOption: true,
> - value: '__default__',
> - listeners: {
> - change: 'hashChange',
> - },
> + header: gettext('MIME type'),
> + dataIndex: 'mimetype',
> },
> {
> - xtype: 'textfield',
> - name: 'checksum',
> - fieldLabel: gettext('Checksum'),
> - allowBlank: false,
> - disabled: true,
> - emptyText: gettext('none'),
> - reference: 'downloadUrlChecksum',
> + header: gettext('Hash'),
> + dataIndex: 'hash',
> + flex: 2,
> + xtype: 'widgetcolumn',
> + widget: {
> + xtype: 'pveHashAlgorithmSelector',
> + listeners: {
> + change: function(widget, newValue, oldValue) {
> + const record = widget.getWidgetRecord();
> + record.set('hash', newValue);
> + let hashWidget = record.get('hashWidget');
> + if (newValue === '__default__') {
> + hashWidget?.setDisabled(true);
> + hashWidget?.setValue('');
> + } else {
> + hashWidget?.setDisabled(false);
> + }
> + },
> + },
same as above
> + },
> },
> {
> - xtype: 'progressbar',
> - text: 'Ready',
> - hidden: true,
> - reference: 'progressBar',
> + header: gettext('Hash Value'),
> + dataIndex: 'hashsum',
> + renderer: data => data || 'None',
> + flex: 4,
> + xtype: 'widgetcolumn',
> + widget: {
> + xtype: 'textfield',
> + disabled: true,
> + listeners: {
> + change: function(widget, newValue, oldValue) {
> + const record = widget.getWidgetRecord();
> + record.set('hashsum', newValue);
> + },
> + },
> + },
> + onWidgetAttach: function(col, widget, record) {
> + record.set('hashWidget', widget);
> + },
same as above
> },
> {
> - xtype: 'hiddenfield',
> - name: 'content',
> - cbind: {
> - value: '{content}',
> + header: gettext('Progress Bar'),
> + xtype: 'widgetcolumn',
> + widget: {
> + xtype: 'progressbar',
> },
> + onWidgetAttach: function(col, widget, rec) {
> + rec.set('progressWidget', widget);
> + widget.updateProgress(0, "");
> + },
same as above
> + flex: 2,
> + },
> + {
> + xtype: 'actioncolumn',
> + items: [{
> + iconCls: 'fa critical fa-trash-o',
> + handler: 'removeFile',
> + }],
> + flex: 0.5,
> },
> ],
> - listeners: {
> - validitychange: 'validitychange',
> - },
> },
> ],
>
> buttons: [
> + {
> + xtype: 'filefield',
> + name: 'file',
> + buttonText: gettext('Add File'),
> + allowBlank: false,
> + hideLabel: true,
> + fieldStyle: 'display: none;',
> + cbind: {
> + accept: '{extensions}',
> + },
> + listeners: {
> + change: 'addFile',
> + render: function(filefield) {
> + filefield.fileInputEl.dom.multiple = true;
> + },
> + },
> + },
> {
> xtype: 'button',
> text: gettext('Abort'),
> - reference: 'abortBtn',
> - disabled: true,
> handler: function() {
> const me = this;
> me.up('pveStorageUpload').close();
> },
> },
> {
> - text: gettext('Upload'),
> - reference: 'submitBtn',
> - disabled: true,
> - handler: 'submit',
> + text: gettext('Start upload'),
> + handler: 'startUpload',
> + bind: {
> + disabled: '{!hasFiles || uploadInProgress}',
> + },
> },
> ],
>
> - listeners: {
> - close: function() {
> - const me = this;
> - if (me.xhr) {
> - me.xhr.abort();
> - delete me.xhr;
> - }
> - },
> - },
> -
> initComponent: function() {
> - const me = this;
> -
> - if (!me.nodename) {
> - throw "no node name specified";
> - }
> - if (!me.storage) {
> - throw "no storage ID specified";
> - }
> - if (!me.acceptedExtensions[me.content]) {
> - throw "content type not supported";
> - }
> -
> - me.callParent();
> + let me = this;
> + me.callParent();
> + me.lookup('grid').store.on('datachanged', function(store) {
> + me.getViewModel().set('hasFiles', store.count() > 0);
> + });
i'd probably put this in the 'init' function of the controller, just
so that we can avoid having an 'initComponent' at all
> },
> });
More information about the pve-devel
mailing list