[pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files

[pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files

[Matthias Heiserer]

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@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,
+		});
+	    }
+	},
+
+	currentUploadIndex: 1,
+	startUpload: function() {
+	    const me = this;
+	    const view = me.getView();
+	    const grid = me.lookup('grid');
+	    view.taskDone();
+
+	    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;
+	    }
+
+	    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?.();
 			    },
 			},
 		    });
+		} 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);
+	    };
 
 	    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));
+		},
+	    },
+	    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);
+			    },
+			},
+			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);
+				}
+			    },
+			},
+		    },
 		},
 		{
-		    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);
+		    },
 		},
 		{
-		    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, "");
+		    },
+		    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);
+	});
     },
 });
-- 
2.30.2

Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files

[Dominik Csapak]

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@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

>       },
>   });

Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files

[Thomas Lamprecht]

On 01/07/2022 16:21, Dominik Csapak wrote:
> 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

Storage and network links that are fast enough to make a GiB+ ISO file upload
flicker may not be that rare but I'd think that it isn't the norm too.

Maybe add a wrapper with at the top having a progress bar with completed/total
uploads as progress indicator and a (maybe collapsible) panel below, with the
current task log and nothing else, for when the upload(s) actually get
processed/submitted.

> 
> 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

+1, makes not much sense IMO too.

> 
> 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

Yeah making it more wizard like (i.e., splitting up steps into separate
"view units") is often a good way to reduce complexity for both the devs
and the users.

> 
> also we generally don't use 'abort' but 'Cancel'
> 
> remaining comments inline:
> 

Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files

[Matthias Heiserer]

On 01.07.2022 16:21, Dominik Csapak wrote:
> 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
Same iso meaning two files with same name? afaic that's the only thing 
the browser allows us to see.

> 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:
Good idea, will change it in v2

[...]
>> +    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?
> 
to update the view, so that uploaded files are shown in the background :)
but you're right, it should be called when the upload is actually finished.
> 
>> +        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?Sounds good.

[...]