From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 41A61792C8 for ; Fri, 1 Jul 2022 16:22:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 309AEA91B for ; Fri, 1 Jul 2022 16:21:41 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 1 Jul 2022 16:21:39 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 550A943D8F; Fri, 1 Jul 2022 16:21:39 +0200 (CEST) Message-ID: <58120bd2-2d68-3748-c7b7-547c0be861cf@proxmox.com> Date: Fri, 1 Jul 2022 16:21:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Thunderbird/103.0 Content-Language: en-US To: Proxmox VE development discussion , Matthias Heiserer References: <20220629122322.816989-1-m.heiserer@proxmox.com> From: Dominik Csapak In-Reply-To: <20220629122322.816989-1-m.heiserer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.099 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Jul 2022 14:22:11 -0000 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 > --- > 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 > }, > });