* [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files
@ 2022-06-29 12:23 Matthias Heiserer
2022-07-01 14:21 ` Dominik Csapak
0 siblings, 1 reply; 4+ messages in thread
From: Matthias Heiserer @ 2022-06-29 12:23 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files
2022-06-29 12:23 [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files Matthias Heiserer
@ 2022-07-01 14:21 ` Dominik Csapak
2022-07-05 6:50 ` Thomas Lamprecht
2022-07-07 12:37 ` Matthias Heiserer
0 siblings, 2 replies; 4+ messages in thread
From: Dominik Csapak @ 2022-07-01 14:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Matthias Heiserer
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
> },
> });
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files
2022-07-01 14:21 ` Dominik Csapak
@ 2022-07-05 6:50 ` Thomas Lamprecht
2022-07-07 12:37 ` Matthias Heiserer
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2022-07-05 6:50 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak, Matthias Heiserer
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:
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files
2022-07-01 14:21 ` Dominik Csapak
2022-07-05 6:50 ` Thomas Lamprecht
@ 2022-07-07 12:37 ` Matthias Heiserer
1 sibling, 0 replies; 4+ messages in thread
From: Matthias Heiserer @ 2022-07-07 12:37 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
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.
[...]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-07 12:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-29 12:23 [pve-devel] [RFC manager] fix #3248: GUI: storage: upload multiple files Matthias Heiserer
2022-07-01 14:21 ` Dominik Csapak
2022-07-05 6:50 ` Thomas Lamprecht
2022-07-07 12:37 ` Matthias Heiserer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox