Re: [pve-devel] [PATCH manager] ui: HD edit: Add multiple disks & tabs

[Dominik Csapak <d.csapak@proxmox.com>]

a few high level comments:

this patch could probably be split into more smaller (more readable)
patches.. also there seem to be some things leftover from the import
feature, but no hard feelings there.

i encountered a few bugs(some more sever, some not so), namely:
* if i remove the second to last disk while i selected it,
   the last disk does not open the panel on the right side if
   i select it
* if i add all disks (so clicking until i have scsi0-scsi31, virtio..)
   it adds ide0 multiple times at the end
   and i now get a popup that i cannot have duplicate ids.
   i'd rather expected that the add button gets disabled
* adding many disks is *very* slow (maybe because of the many storage
   api calls?), not sure if we can improve that

some comments to the overall design choices:
while it feels good, i am not so sure about the 'plain' tab style
as well as the bottom mounted buttons

for the hd edit, it should probably look more like the storage
edit window, where we have the normal tab style, and bodyPadding
set to 0

also why do you split the 'options' and the 'bandwidth' options
there, but not in the wizard? are not 'advanced' anymore
when the vm exists?

for the wizard, i'd probably move the tabs up (and leave the default 
style) and maybe put the add/remove button in tabs? though i am not
sure honestly...

as for add button, i think it would be better to have also text there,
just for clarity (so '+ Add')

what i kinda miss is the info which disk is not valid. if i add many 
disks, and one is not valid (for whatever reason), i cannot see that
immediatly and have to click through all disks and all tabs
(though i think it is not possible to have anything in the
option tab to be invalid)

otherwise i like it :)

comments to the code inline:

On 6/16/21 13:34, Dominic Jäger wrote:
> Enable adding multiple disks in VM create wizard.
> This is a first step for future import features.
> 
> Split disk edit panel into multiple tabbed panels to make it less cluttered.
> This affects the create wizard & the HD edit windows in the VM hardware view.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
>   www/manager6/Makefile                         |   6 +-
>   www/manager6/form/ControllerSelector.js       |  20 +
>   www/manager6/qemu/CDEdit.js                   |   3 -
>   www/manager6/qemu/CreateWizard.js             |  16 +-
>   www/manager6/qemu/HDEdit.js                   | 409 ------------------
>   www/manager6/qemu/HardwareView.js             |   9 +-
>   www/manager6/qemu/OSTypeEdit.js               |  12 +-
>   .../qemu/disk/DiskBandwidthOptions.js         | 192 ++++++++
>   www/manager6/qemu/disk/DiskBasicOptions.js    | 153 +++++++
>   www/manager6/qemu/disk/DiskCollection.js      | 282 ++++++++++++
>   www/manager6/qemu/disk/DiskData.js            | 241 +++++++++++
>   www/manager6/qemu/disk/HardDisk.js            | 215 +++++++++
>   www/manager6/window/Wizard.js                 |   2 +
>   13 files changed, 1139 insertions(+), 421 deletions(-)
>   delete mode 100644 www/manager6/qemu/HDEdit.js
>   create mode 100644 www/manager6/qemu/disk/DiskBandwidthOptions.js
>   create mode 100644 www/manager6/qemu/disk/DiskBasicOptions.js
>   create mode 100644 www/manager6/qemu/disk/DiskCollection.js
>   create mode 100644 www/manager6/qemu/disk/DiskData.js
>   create mode 100644 www/manager6/qemu/disk/HardDisk.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 6776d4ce..95f03d88 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -198,7 +198,11 @@ JSSRC= 							\
>   	qemu/Config.js					\
>   	qemu/CreateWizard.js				\
>   	qemu/DisplayEdit.js				\
> -	qemu/HDEdit.js					\
> +	qemu/disk/DiskCollection.js			\
> +	qemu/disk/HardDisk.js				\
> +	qemu/disk/DiskData.js				\
> +	qemu/disk/DiskBasicOptions.js			\
> +	qemu/disk/DiskBandwidthOptions.js		\
>   	qemu/HDEfi.js					\
>   	qemu/HDMove.js					\
>   	qemu/HDResize.js				\
> diff --git a/www/manager6/form/ControllerSelector.js b/www/manager6/form/ControllerSelector.js
> index daca2432..85f66956 100644
> --- a/www/manager6/form/ControllerSelector.js
> +++ b/www/manager6/form/ControllerSelector.js
> @@ -72,6 +72,26 @@ Ext.define('PVE.form.ControllerSelector', {
>   	deviceid.validate();
>       },
>   
> +    deleteFromVMConfig: function(key) {
> +	const me = this;
> +	delete me.vmconfig[key];
> +    },
> +
> +    getValues: function() {
> +	return this.query('field').map(x => x.getValue());
> +    },
> +
> +    getValuesAsString: function() {
> +	return this.getValues().join('');
> +    },

nit: i am not sure this warrants it's own function?
or at least give it a better name, like 'getDiskId' ?
(also, are we sure 'this.query' has a stable order?
i guess so, but would be good to verify, or make the
order explicit)

> +
> +    setValue: function(value) {
> +	const regex = /([a-z]+)(\d+)/;
> +	const [_, controller, deviceid] = regex.exec(value);
> +	this.down('field[name=controller]').setValue(controller);
> +	this.down('field[name=deviceid]').setValue(deviceid);
> +    },
> +
>       initComponent: function() {
>   	var me = this;
>   
> diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
> index 72c01037..27092d32 100644
> --- a/www/manager6/qemu/CDEdit.js
> +++ b/www/manager6/qemu/CDEdit.js
> @@ -84,9 +84,6 @@ Ext.define('PVE.qemu.CDInputPanel', {
>   	    checked: true,
>   	    listeners: {
>   		change: function(f, value) {
> -		    if (!me.rendered) {
> -			return;
> -		    }

nit: seems like an unrelated change..

>   		    me.down('field[name=cdstorage]').setDisabled(!value);
>   		    var cdImageField = me.down('field[name=cdimage]');
>   		    cdImageField.setDisabled(!value);
> diff --git a/www/manager6/qemu/CreateWizard.js b/www/manager6/qemu/CreateWizard.js
> index d4535c9d..2405b6f7 100644
> --- a/www/manager6/qemu/CreateWizard.js
> +++ b/www/manager6/qemu/CreateWizard.js
> @@ -154,7 +154,7 @@ Ext.define('PVE.qemu.CreateWizard', {
>   	    insideWizard: true,
>   	},
>   	{
> -	    xtype: 'pveQemuHDInputPanel',
> +	    xtype: 'pveQemuDiskCollection',
>   	    bind: {
>   		nodename: '{nodename}',
>   	    },
> @@ -251,6 +251,20 @@ Ext.define('PVE.qemu.CreateWizard', {
>   	    },
>   	},
>       ],
> +
> +    getValues: function() {
> +	let values = this.callParent();
> +	for (const [key, value] of Object.entries(values)) {
> +	    const re = /ide\d+|sata\d+|virtio\d+|scsi\d+|import_sources/;
> +	    if (key.match(re) && Array.isArray(value)) {
> +		    // Collected from different panels => array
> +		    // But API & some GUI functions expect not array
> +		    const sep = key === 'import_sources' ? '\0' : ',';
> +		    values[key] = value.join(sep);
> +	    }
> +	}
> +	return values;
> +    },
>   });
>   
>   
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> deleted file mode 100644
> index 95a98b0b..00000000
> --- a/www/manager6/qemu/HDEdit.js
> +++ /dev/null
> @@ -1,409 +0,0 @@
> -/* 'change' property is assigned a string and then a function */
> -Ext.define('PVE.qemu.HDInputPanel', {
> -    extend: 'Proxmox.panel.InputPanel',
> -    alias: 'widget.pveQemuHDInputPanel',
> -    onlineHelp: 'qm_hard_disk',
> -
> -    insideWizard: false,
> -
> -    unused: false, // ADD usused disk imaged
> -
> -    vmconfig: {}, // used to select usused disks
> -
> -    viewModel: {},
> -
> -    controller: {
> -
> -	xclass: 'Ext.app.ViewController',
> -
> -	onControllerChange: function(field) {
> -	    var value = field.getValue();
> -
> -	    var allowIOthread = value.match(/^(virtio|scsi)/);
> -	    this.lookup('iothread').setDisabled(!allowIOthread);
> -	    if (!allowIOthread) {
> -		this.lookup('iothread').setValue(false);
> -	    }
> -
> -	    var virtio = value.match(/^virtio/);
> -	    this.lookup('ssd').setDisabled(virtio);
> -	    if (virtio) {
> -		this.lookup('ssd').setValue(false);
> -	    }
> -
> -	    this.lookup('scsiController').setVisible(value.match(/^scsi/));
> -	},
> -
> -	control: {
> -	    'field[name=controller]': {
> -		change: 'onControllerChange',
> -		afterrender: 'onControllerChange',
> -	    },
> -	    'field[name=iothread]': {
> -		change: function(f, value) {
> -		    if (!this.getView().insideWizard) {
> -			return;
> -		    }
> -		    var vmScsiType = value ? 'virtio-scsi-single': 'virtio-scsi-pci';
> -		    this.lookupReference('scsiController').setValue(vmScsiType);
> -		},
> -	    },
> -	},
> -
> -	init: function(view) {
> -	    var vm = this.getViewModel();
> -	    if (view.isCreate) {
> -		vm.set('isIncludedInBackup', true);
> -	    }
> -	},
> -    },
> -
> -    onGetValues: function(values) {
> -	var me = this;
> -
> -	var params = {};
> -	var confid = me.confid || values.controller + values.deviceid;
> -
> -	if (me.unused) {
> -	    me.drive.file = me.vmconfig[values.unusedId];
> -	    confid = values.controller + values.deviceid;
> -	} else if (me.isCreate) {
> -	    if (values.hdimage) {
> -		me.drive.file = values.hdimage;
> -	    } else {
> -		me.drive.file = values.hdstorage + ":" + values.disksize;
> -	    }
> -	    me.drive.format = values.diskformat;
> -	}
> -
> -	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> -	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> -	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> -
> -        var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> -        Ext.Array.each(names, function(name) {
> -            var burst_name = name + '_max';
> -	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> -	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> -        });
> -
> -
> -	params[confid] = PVE.Parser.printQemuDrive(me.drive);
> -
> -	return params;
> -    },
> -
> -    setVMConfig: function(vmconfig) {
> -	var me = this;
> -
> -	me.vmconfig = vmconfig;
> -
> -	if (me.bussel) {
> -	    me.bussel.setVMConfig(vmconfig);
> -	    me.scsiController.setValue(vmconfig.scsihw);
> -	}
> -	if (me.unusedDisks) {
> -	    var disklist = [];
> -	    Ext.Object.each(vmconfig, function(key, value) {
> -		if (key.match(/^unused\d+$/)) {
> -		    disklist.push([key, value]);
> -		}
> -	    });
> -	    me.unusedDisks.store.loadData(disklist);
> -	    me.unusedDisks.setValue(me.confid);
> -	}
> -    },
> -
> -    setDrive: function(drive) {
> -	var me = this;
> -
> -	me.drive = drive;
> -
> -	var values = {};
> -	var match = drive.file.match(/^([^:]+):/);
> -	if (match) {
> -	    values.hdstorage = match[1];
> -	}
> -
> -	values.hdimage = drive.file;
> -	values.backup = PVE.Parser.parseBoolean(drive.backup, 1);
> -	values.noreplicate = !PVE.Parser.parseBoolean(drive.replicate, 1);
> -	values.diskformat = drive.format || 'raw';
> -	values.cache = drive.cache || '__default__';
> -	values.discard = drive.discard === 'on';
> -	values.ssd = PVE.Parser.parseBoolean(drive.ssd);
> -	values.iothread = PVE.Parser.parseBoolean(drive.iothread);
> -
> -	values.mbps_rd = drive.mbps_rd;
> -	values.mbps_wr = drive.mbps_wr;
> -	values.iops_rd = drive.iops_rd;
> -	values.iops_wr = drive.iops_wr;
> -	values.mbps_rd_max = drive.mbps_rd_max;
> -	values.mbps_wr_max = drive.mbps_wr_max;
> -	values.iops_rd_max = drive.iops_rd_max;
> -	values.iops_wr_max = drive.iops_wr_max;
> -
> -	me.setValues(values);
> -    },
> -
> -    setNodename: function(nodename) {
> -	var me = this;
> -	me.down('#hdstorage').setNodename(nodename);
> -	me.down('#hdimage').setStorage(undefined, nodename);
> -    },
> -
> -    initComponent: function() {
> -	var me = this;
> -
> -	var labelWidth = 140;
> -
> -	me.drive = {};
> -
> -	me.column1 = [];
> -	me.column2 = [];
> -
> -	me.advancedColumn1 = [];
> -	me.advancedColumn2 = [];
> -
> -	if (!me.confid || me.unused) {
> -	    me.bussel = Ext.create('PVE.form.ControllerSelector', {
> -		vmconfig: me.insideWizard ? { ide2: 'cdrom' } : {},
> -	    });
> -	    me.column1.push(me.bussel);
> -
> -	    me.scsiController = Ext.create('Ext.form.field.Display', {
> -		fieldLabel: gettext('SCSI Controller'),
> -		reference: 'scsiController',
> -		bind: me.insideWizard ? {
> -		    value: '{current.scsihw}',
> -		} : undefined,
> -		renderer: PVE.Utils.render_scsihw,
> -		submitValue: false,
> -		hidden: true,
> -	    });
> -	    me.column1.push(me.scsiController);
> -	}
> -
> -	if (me.unused) {
> -	    me.unusedDisks = Ext.create('Proxmox.form.KVComboBox', {
> -		name: 'unusedId',
> -		fieldLabel: gettext('Disk image'),
> -		matchFieldWidth: false,
> -		listConfig: {
> -		    width: 350,
> -		},
> -		data: [],
> -		allowBlank: false,
> -	    });
> -	    me.column1.push(me.unusedDisks);
> -	} else if (me.isCreate) {
> -	    me.column1.push({
> -		xtype: 'pveDiskStorageSelector',
> -		storageContent: 'images',
> -		name: 'disk',
> -		nodename: me.nodename,
> -		autoSelect: me.insideWizard,
> -	    });
> -	} else {
> -	    me.column1.push({
> -		xtype: 'textfield',
> -		disabled: true,
> -		submitValue: false,
> -		fieldLabel: gettext('Disk image'),
> -                name: 'hdimage',
> -	    });
> -	}
> -
> -	me.column2.push(
> -	    {
> -		xtype: 'CacheTypeSelector',
> -		name: 'cache',
> -		value: '__default__',
> -		fieldLabel: gettext('Cache'),
> -	    },
> -	    {
> -		xtype: 'proxmoxcheckbox',
> -		fieldLabel: gettext('Discard'),
> -		reference: 'discard',
> -		name: 'discard',
> -	    },
> -	);
> -
> -	me.advancedColumn1.push(
> -	    {
> -		xtype: 'proxmoxcheckbox',
> -		disabled: me.confid && me.confid.match(/^virtio/),
> -		fieldLabel: gettext('SSD emulation'),
> -		labelWidth: labelWidth,
> -		name: 'ssd',
> -		reference: 'ssd',
> -	    },
> -	    {
> -		xtype: 'proxmoxcheckbox',
> -		disabled: me.confid && !me.confid.match(/^(virtio|scsi)/),
> -		fieldLabel: 'IO thread',
> -		labelWidth: labelWidth,
> -		reference: 'iothread',
> -		name: 'iothread',
> -	    },
> -	    {
> -		xtype: 'numberfield',
> -		name: 'mbps_rd',
> -		minValue: 1,
> -		step: 1,
> -		fieldLabel: gettext('Read limit') + ' (MB/s)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('unlimited'),
> -	    },
> -	    {
> -		xtype: 'numberfield',
> -		name: 'mbps_wr',
> -		minValue: 1,
> -		step: 1,
> -		fieldLabel: gettext('Write limit') + ' (MB/s)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('unlimited'),
> -	    },
> -	    {
> -		xtype: 'proxmoxintegerfield',
> -		name: 'iops_rd',
> -		minValue: 10,
> -		step: 10,
> -		fieldLabel: gettext('Read limit') + ' (ops/s)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('unlimited'),
> -	    },
> -	    {
> -		xtype: 'proxmoxintegerfield',
> -		name: 'iops_wr',
> -		minValue: 10,
> -		step: 10,
> -		fieldLabel: gettext('Write limit') + ' (ops/s)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('unlimited'),
> -	    },
> -	);
> -
> -	me.advancedColumn2.push(
> -	    {
> -		xtype: 'proxmoxcheckbox',
> -		fieldLabel: gettext('Backup'),
> -		autoEl: {
> -		    tag: 'div',
> -		    'data-qtip': gettext('Include volume in backup job'),
> -		},
> -		labelWidth: labelWidth,
> -		name: 'backup',
> -		bind: {
> -		    value: '{isIncludedInBackup}',
> -		},
> -	    },
> -	    {
> -		xtype: 'proxmoxcheckbox',
> -		fieldLabel: gettext('Skip replication'),
> -		labelWidth: labelWidth,
> -		name: 'noreplicate',
> -	    },
> -	    {
> -		xtype: 'numberfield',
> -		name: 'mbps_rd_max',
> -		minValue: 1,
> -		step: 1,
> -		fieldLabel: gettext('Read max burst') + ' (MB)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('default'),
> -	    },
> -	    {
> -		xtype: 'numberfield',
> -		name: 'mbps_wr_max',
> -		minValue: 1,
> -		step: 1,
> -		fieldLabel: gettext('Write max burst') + ' (MB)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('default'),
> -	    },
> -	    {
> -		xtype: 'proxmoxintegerfield',
> -		name: 'iops_rd_max',
> -		minValue: 10,
> -		step: 10,
> -		fieldLabel: gettext('Read max burst') + ' (ops)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('default'),
> -	    },
> -	    {
> -		xtype: 'proxmoxintegerfield',
> -		name: 'iops_wr_max',
> -		minValue: 10,
> -		step: 10,
> -		fieldLabel: gettext('Write max burst') + ' (ops)',
> -		labelWidth: labelWidth,
> -		emptyText: gettext('default'),
> -	    },
> -	);
> -
> -	me.callParent();
> -    },
> -});
> -
> -Ext.define('PVE.qemu.HDEdit', {
> -    extend: 'Proxmox.window.Edit',
> -
> -    isAdd: true,
> -
> -    backgroundDelay: 5,
> -
> -    initComponent: function() {
> -	var me = this;
> -
> -	var nodename = me.pveSelNode.data.node;
> -	if (!nodename) {
> -	    throw "no node name specified";
> -	}
> -
> -	var unused = me.confid && me.confid.match(/^unused\d+$/);
> -
> -	me.isCreate = me.confid ? unused : true;
> -
> -	var ipanel = Ext.create('PVE.qemu.HDInputPanel', {
> -	    confid: me.confid,
> -	    nodename: nodename,
> -	    unused: unused,
> -	    isCreate: me.isCreate,
> -	});
> -
> -	if (unused) {
> -	    me.subject = gettext('Unused Disk');
> -	} else if (me.isCreate) {
> -            me.subject = gettext('Hard Disk');
> -	} else {
> -           me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
> -	}
> -
> -	me.items = [ipanel];
> -
> -	me.callParent();
> -	/* 'data' is assigned an empty array in same file, and here we
> -	 * use it like an object
> -	 */
> -	me.load({
> -	    success: function(response, options) {
> -		ipanel.setVMConfig(response.result.data);
> -		if (me.confid) {
> -		    var value = response.result.data[me.confid];
> -		    var drive = PVE.Parser.parseQemuDrive(me.confid, value);
> -		    if (!drive) {
> -			Ext.Msg.alert(gettext('Error'), 'Unable to parse drive options');
> -			me.close();
> -			return;
> -		    }
> -		    ipanel.setDrive(drive);
> -		    me.isValid(); // trigger validation
> -		}
> -	    },
> -	});
> -    },
> -});
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 200e3c28..5126fab8 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -220,7 +220,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    rows[confid] = {
>   		group: 10,
>   		iconCls: 'hdd-o',
> -		editor: 'PVE.qemu.HDEdit',
> +		editor: 'PVE.qemu.HardDiskWindow',
>   		isOnStorageBus: true,
>   		header: gettext('Hard Disk') + ' (' + confid +')',
>   		cdheader: gettext('CD/DVD Drive') + ' (' + confid +')',
> @@ -290,7 +290,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		order: i,
>   		iconCls: 'hdd-o',
>   		del_extra_msg: gettext('This will permanently erase all data.'),
> -		editor: caps.vms['VM.Config.Disk'] ? 'PVE.qemu.HDEdit' : undefined,
> +		editor: caps.vms['VM.Config.Disk'] ? 'PVE.qemu.HardDiskWindow' : undefined,
>   		header: gettext('Unused Disk') + ' ' + i.toString(),
>   	    };
>   	}
> @@ -630,9 +630,10 @@ Ext.define('PVE.qemu.HardwareView', {
>   				iconCls: 'fa fa-fw fa-hdd-o black',
>   				disabled: !caps.vms['VM.Config.Disk'],
>   				handler: function() {
> -				    let win = Ext.create('PVE.qemu.HDEdit', {
> +				    let win = Ext.create('PVE.qemu.HardDiskWindow', {
>   					url: '/api2/extjs/' + baseurl,
> -					pveSelNode: me.pveSelNode,
> +					nodename: me.pveSelNode.data.node,
> +					isCreate: true,
>   				    });
>   				    win.on('destroy', me.reload, me);
>   				    win.show();
> diff --git a/www/manager6/qemu/OSTypeEdit.js b/www/manager6/qemu/OSTypeEdit.js
> index 438d7c6b..641d9394 100644
> --- a/www/manager6/qemu/OSTypeEdit.js
> +++ b/www/manager6/qemu/OSTypeEdit.js
> @@ -3,6 +3,7 @@ Ext.define('PVE.qemu.OSTypeInputPanel', {
>       alias: 'widget.pveQemuOSTypePanel',
>       onlineHelp: 'qm_os_settings',
>       insideWizard: false,
> +    ignoreDisks: false,

is this also leftover from the import?
this is not set anywhere to true
and only read once (in the next hunk)

>   
>       controller: {
>   	xclass: 'Ext.app.ViewController',
> @@ -20,13 +21,18 @@ Ext.define('PVE.qemu.OSTypeInputPanel', {
>   	},
>   	onOSTypeChange: function(field) {
>   	    var me = this, ostype = field.getValue();
> -	    if (!me.getView().insideWizard) {
> +	    const view = me.getView();
> +	    if (!view.insideWizard) {
>   		return;
>   	    }
>   	    var targetValues = PVE.qemu.OSDefaults.getDefaults(ostype);
> -
> -	    me.setWidget('pveBusSelector', targetValues.busType);
> +	    if (!view.ignoreDisks) {
> +		const ids = Ext.ComponentQuery.query('pveBusSelector')
> +		    .reduce((acc, cur) => acc.concat(cur.id), []);
> +		ids.forEach(i => me.setWidget(`#${i}`, targetValues.busType));
is that not simply a 'query('').map((rec) => rec.id)' ?
no need to copy the whole array each time

also we simply do one thing per id so the whole thing could be a 
'forEach' simply?

query('').forEach((rec) => me.setWidget(....rec.id...)

?


> +	    }
>   	    me.setWidget('pveNetworkCardSelector', targetValues.networkCard);
> +	    me.setWidget('pveQemuBiosSelector', targetValues.bios);

nit: also unrelated change (also we do not have any 'bios' versions in
the osdefaults yet)

>   	    var scsihw = targetValues.scsihw || '__default__';
>   	    this.getViewModel().set('current.scsihw', scsihw);
>   	},
> diff --git a/www/manager6/qemu/disk/DiskBandwidthOptions.js b/www/manager6/qemu/disk/DiskBandwidthOptions.js
> new file mode 100644
> index 00000000..58e08f59
> --- /dev/null
> +++ b/www/manager6/qemu/disk/DiskBandwidthOptions.js
> @@ -0,0 +1,192 @@
> +/* 'change' property is assigned a string and then a function */
> +Ext.define('PVE.qemu.DiskBandwidthOptions', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pveQemuDiskBandwidthOptions',
> +    onlineHelp: 'qm_hard_disk',
> +
> +    insideWizard: false,
> +
> +    unused: false, // ADD usused disk imaged
> +
> +    padding: '10 10 10 10',
> +
> +    vmconfig: {}, // used to select usused disks
> +
> +    viewModel: {},

why an empty viewmodel ?

> +
> +    /**
> +     * All radiofields in pveQemuDiskCollection have the same scope
> +     * Make name of radiofields unique for each disk panel
> +     */
> +    getRadioName() {
> +	return 'radio_' + this.id;
> +    },

leftover? i cannot see why this should be here?

> +
> +    onGetValues: function(values) {
> +	const me = this;
> +
> +	let params = {};
> +
> +	const confid = me.up('pveQemuHardDisk').down('pveQemuDiskData').getConfid();

i really do not like that this panel determines where it is used...
could we do this differently? by bubbling the values up
and combine them only at the top level ?

or give the panels a reference to the drive, so they can set the 
properties on that

also i am not really clear on how this works now, since
there are 3 panels that all want to modify the same drive?

> +
> +	const names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +	Ext.Array.each(names, function(name) {
> +	    let burst_name = name + '_max';
> +	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> +	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> +	});
> +
> +	me.drive.file = 'dummy';
> +	// no values => no comma
> +	params[confid] = PVE.Parser.printQemuDrive(me.drive).replace(/^dummy,?/, "");

this seems wrong? is this because somewhere else we simply concatenate 
the drive? thats another reason why i do not like this...

> +
> +	return params;
> +    },
> +
> +    setVMConfig: function(vmconfig) {
> +	let me = this;
> +
> +	me.vmconfig = vmconfig;
> +
> +	if (me.bussel) {
> +	    me.bussel.setVMConfig(vmconfig);
> +	    me.scsiController.setValue(vmconfig.scsihw);
> +	}
> +	if (me.unusedDisks) {
> +	    let disklist = [];
> +	    Ext.Object.each(vmconfig, function(key, value) {
> +		if (key.match(/^unused\d+$/)) {
> +		    disklist.push([key, value]);
> +		}
> +	    });
> +	    me.unusedDisks.store.loadData(disklist);
> +	    me.unusedDisks.setValue(me.confid);
> +	}
> +    },
> +
> +    setDrive: function(drive) {
> +	let me = this;
> +
> +	me.drive = {};
> +	[
> +	    'interface',
> +	    'index',
> +	    'mbps_rd',
> +	    'mbps_wr',
> +	    'iops_rd',
> +	    'iops_wr',
> +	    'mbps_rd_max',
> +	    'mbps_wr_max',
> +	    'iops_rd_max',
> +	    'iops_wr_max',
> +	].forEach(o => { me.drive[o] = drive[o]; });
> +
> +	let values = {};
> +	values.mbps_rd = drive.mbps_rd;
> +	values.mbps_wr = drive.mbps_wr;
> +	values.iops_rd = drive.iops_rd;
> +	values.iops_wr = drive.iops_wr;
> +	values.mbps_rd_max = drive.mbps_rd_max;
> +	values.mbps_wr_max = drive.mbps_wr_max;
> +	values.iops_rd_max = drive.iops_rd_max;
> +	values.iops_wr_max = drive.iops_wr_max;
> +
> +	me.setValues(values);

would me.setValues(me.drive) not work?
if a field does not exists for a property, it's simply
not set anywhere right?

> +    },
> +
> +
> +    setNodename: function(nodename) {
> +	// nothing
> +    },

if it does nothing, is it necessary to have?

> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	let labelWidth = 140;
> +
> +	me.drive = {};
> +
> +	me.column1 = [];
> +	me.column2 = [];
> +
> +	me.column1.push(
> +	    {
> +		xtype: 'numberfield',
> +		name: 'mbps_rd',
> +		minValue: 1,
> +		step: 1,
> +		fieldLabel: gettext('Read limit') + ' (MB/s)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('unlimited'),
> +	    },
> +	    {
> +		xtype: 'numberfield',
> +		name: 'mbps_wr',
> +		minValue: 1,
> +		step: 1,
> +		fieldLabel: gettext('Write limit') + ' (MB/s)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('unlimited'),
> +	    },
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		name: 'iops_rd',
> +		minValue: 10,
> +		step: 10,
> +		fieldLabel: gettext('Read limit') + ' (ops/s)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('unlimited'),
> +	    },
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		name: 'iops_wr',
> +		minValue: 10,
> +		step: 10,
> +		fieldLabel: gettext('Write limit') + ' (ops/s)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('unlimited'),
> +	    },
> +	);
> +
> +	me.column2.push(
> +	    {
> +		xtype: 'numberfield',
> +		name: 'mbps_rd_max',
> +		minValue: 1,
> +		step: 1,
> +		fieldLabel: gettext('Read max burst') + ' (MB)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('default'),
> +	    },
> +	    {
> +		xtype: 'numberfield',
> +		name: 'mbps_wr_max',
> +		minValue: 1,
> +		step: 1,
> +		fieldLabel: gettext('Write max burst') + ' (MB)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('default'),
> +	    },
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		name: 'iops_rd_max',
> +		minValue: 10,
> +		step: 10,
> +		fieldLabel: gettext('Read max burst') + ' (ops)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('default'),
> +	    },
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		name: 'iops_wr_max',
> +		minValue: 10,
> +		step: 10,
> +		fieldLabel: gettext('Write max burst') + ' (ops)',
> +		labelWidth: labelWidth,
> +		emptyText: gettext('default'),
> +	    },
> +	);

i'd rather have those statically in the class if we can

> +
> +	me.callParent();
> +    },
> +});
> diff --git a/www/manager6/qemu/disk/DiskBasicOptions.js b/www/manager6/qemu/disk/DiskBasicOptions.js
> new file mode 100644
> index 00000000..d582b1df
> --- /dev/null
> +++ b/www/manager6/qemu/disk/DiskBasicOptions.js
> @@ -0,0 +1,153 @@
> +/* 'change' property is assigned a string and then a function */
> +Ext.define('PVE.qemu.DiskBasicOptions', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pveQemuDiskBasicOptions',
> +    onlineHelp: 'qm_hard_disk',
> +
> +    insideWizard: false,
> +
> +    unused: false, // ADD usused disk imaged
> +
> +    padding: '10 10 10 10',
> +
> +    vmconfig: {}, // used to select usused disks
> +
> +    viewModel: {},

same as in BandwidthSelector

> +
> +    /**
> +     * All radiofields in pveQemuDiskCollection have the same scope
> +     * Make name of radiofields unique for each disk panel
> +     */
> +    getRadioName() {
> +	return 'radio_' + this.id;
> +    },


same as in BandwidthSelector

> +
> +    onGetValues: function(values) {
> +	const me = this;
> +
> +	let params = {};
> +
> +	const confid = me.up('pveQemuHardDisk').down('pveQemuDiskData').getConfid();

same as in BandwidthSelector

> +
> +	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> +	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> +	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> +	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> +
> +	me.drive.file = 'dummy';
> +	// no values => no comma
> +	params[confid] = PVE.Parser.printQemuDrive(me.drive).replace(/^dummy,?/, "");

same as in BandwidthSelector

> +
> +	return params;
> +    },
> +
> +    setVMConfig: function(vmconfig) {
> +	let me = this;
> +
> +	me.vmconfig = vmconfig;
> +
> +	if (me.bussel) {
> +	    me.bussel.setVMConfig(vmconfig);
> +	    me.scsiController.setValue(vmconfig.scsihw);
> +	}
> +	if (me.unusedDisks) {
> +	    let disklist = [];
> +	    Ext.Object.each(vmconfig, function(key, value) {
> +		if (key.match(/^unused\d+$/)) {
> +		    disklist.push([key, value]);
> +		}
> +	    });
> +	    me.unusedDisks.store.loadData(disklist);
> +	    me.unusedDisks.setValue(me.confid);
> +	}
> +    },
> +
> +    setDrive: function(drive) {
> +	let me = this;
> +
> +	me.drive = {};
> +
> +	[
> +	    'interface',
> +	    'index',
> +	    'backup',
> +	    'replicate',
> +	    'ssd',
> +	    'iothread',
> +	].forEach(o => { me.drive[o] = drive[o]; });
> +
> +	let values = {};
> +	values.backup = PVE.Parser.parseBoolean(drive.backup, 1);
> +	values.noreplicate = !PVE.Parser.parseBoolean(drive.replicate, 1);
> +	values.ssd = PVE.Parser.parseBoolean(drive.ssd);
> +	values.iothread = PVE.Parser.parseBoolean(drive.iothread);
> +
> +	me.setValues(values);

same as in BandwidthSelector

> +    },
> +
> +
> +    setNodename: function(nodename) {
> +	// nothing
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	let labelWidth = 140;
> +
> +	me.drive = {};
> +
> +	me.column1 = [];
> +	me.column2 = [];
> +
> +	me.column1.push(
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		disabled: me.confid && me.confid.match(/^virtio/),
> +		fieldLabel: gettext('SSD emulation'),
> +		labelWidth: labelWidth,
> +		name: 'ssd',
> +		reference: 'ssd',
> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		disabled: me.confid && !me.confid.match(/^(virtio|scsi)/),
> +		fieldLabel: 'IO thread',
> +		labelWidth: labelWidth,
> +		reference: 'iothread',
> +		name: 'iothread',
> +		listeners: {
> +		    change: function(f, value) {
> +			const disk = f.up('pveQemuHardDisk');
> +			if (disk.insideWizard) {
> +			    const vmScsiType = value ? 'virtio-scsi-single' : 'virtio-scsi-pci';
> +			    disk.down('field[name=scsiController]').setValue(vmScsiType);
> +			}
> +		    },
> +		},
> +	    },
> +	);
> +
> +	me.column2.push(
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Backup'),
> +		autoEl: {
> +		    tag: 'div',
> +		    'data-qtip': gettext('Include volume in backup job'),
> +		},
> +		labelWidth: labelWidth,
> +		name: 'backup',
> +		value: me.isCreate,
> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Skip replication'),
> +		labelWidth: labelWidth,
> +		name: 'noreplicate',
> +	    },
> +	);

same as in BandwidthSelector

> +
> +	me.callParent();
> +    },
> +});
> diff --git a/www/manager6/qemu/disk/DiskCollection.js b/www/manager6/qemu/disk/DiskCollection.js
> new file mode 100644
> index 00000000..79815244
> --- /dev/null
> +++ b/www/manager6/qemu/disk/DiskCollection.js
> @@ -0,0 +1,282 @@
> +Ext.define('PVE.qemu.DiskCollection', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pveQemuDiskCollection',
> +
> +    insideWizard: false,
> +
> +    isCreate: false,
> +
> +    hiddenDisks: [],
> +
> +    leftColumnRatio: 0.25,
> +
> +    column1: [
> +	{
> +	    // Adding to the panelContainer below automatically adds
> +	    // items to the store
> +	    xtype: 'gridpanel',
> +	    scrollable: true,
> +	    store: {
> +		xtype: 'store',
> +		storeId: 'diskstorage',
> +		// Use the panel as id
> +		// Panels have are objects and therefore unique
> +		// E.g. while adding new panels 'device' is ambiguous
> +		fields: ['device', 'panel'],

i mean, technically it works to have the panel reference in a store,
but why don't we want to store some id? let it autogenerate
and reference it?

> +		removeByPanel: function(panel) {
> +		    const recordIndex = this.findBy(record => record.data.panel === panel);
> +		    this.removeAt(recordIndex);
> +		    return recordIndex;
> +		} > +		getLast: function() {
> +		    const last = this.getCount() - 1;
> +		    return this.getAt(last);
> +		},

stores already have a 'last' function?
https://docs.sencha.com/extjs/6.0.1/classic/Ext.data.Store.html#method-last

> +		listeners: {
> +		    remove: function(store, records, index, isMove, eOpts) {
> +			const view = Ext.ComponentQuery.query('pveQemuDiskCollection').shift();

shift seems unnecessary, since it shifts all elements one down
but since we do not use the rest of the elements anyway
you can do [0] ?

> +			records.forEach(r => {
> +			    view.removePanel(r.get('panel'));
> +			    view.deleteFromVMConfig(r.get('device'));
> +			});
> +		    },
> +		},
> +	    },
> +	    enableColumnMove: false,
> +	    enableColumnResize: false,
> +	    enableColumnHide: false,
> +	    columns: [
> +		{
> +		    text: gettext('Device'),
> +		    dataIndex: 'device',
> +		    flex: 4,
> +		    menuDisabled: true,
> +		},
> +		{
> +		    flex: 1,
> +		    xtype: 'actioncolumn',
> +		    align: 'center',
> +		    menuDisabled: true,
> +		    items: [
> +			{
> +			    iconCls: 'x-fa fa-trash critical',
> +			    tooltip: 'Delete',
> +			    handler: function(tableview, rowIndex, colIndex, item, event, record) {
> +				Ext.getStore('diskstorage').remove(record);
> +			    },
> +			},
> +		    ],
> +		},
> +	    ],
> +	    listeners: {
> +		select: function(_, record) {
> +		    this.up('pveQemuDiskCollection')
> +			.down('#panelContainer')
> +			.setActiveItem(record.data.panel);
> +		},
> +	    },
> +	    anchor: '100% 90%',
> +	    selectLast: function() {
> +		this.setSelection(this.store.getLast());
> +	    },
> +	    dockedItems: [
> +		{
> +		    xtype: 'toolbar',
> +		    dock: 'bottom',
> +		    ui: 'footer',
> +		    style: {
> +			backgroundColor: 'transparent',
> +		    },
> +		    layout: {
> +			pack: 'center',
> +		    },
> +		    items: [
> +			{
> +			    iconCls: 'fa fa-plus-circle',
> +			    itemId: 'addDisk',
> +			    minWidth: '60',
> +			    handler: function(button) {
> +				button.up('pveQemuDiskCollection').addDisk();
> +			    },
> +			},
> +		    ],
> +		},
> +	    ],
> +	},
> +    ],
> +    column2: [
> +	{
> +	    itemId: 'panelContainer',
> +	    xtype: 'container',
> +	    layout: 'card',
> +	    items: [],
> +	    listeners: {
> +		beforeRender: function() {
> +		    // Initial disk if none have been added by manifest yet
> +		    if (this.items.items.length === 0) {
> +			this.addDisk();
> +		    }

i guess leftover from import?

> +		},
> +		add: function(container, newPanel) {
> +		    const store = Ext.getStore('diskstorage');
> +		    store.add({ device: newPanel.getDevice(), panel: newPanel });
> +		    container.setActiveItem(newPanel);
> +		},
> +	    },
> +	    defaultItem: {
> +		xtype: 'pveQemuHardDisk',
> +		bind: {
> +		    nodename: '{nodename}',
> +		},
> +		listeners: {
> +		    // newPanel ... cloned + added defaultItem
> +		    added: function(newPanel) {
> +			Ext.Array.each(newPanel.down('pveControllerSelector').query('field'),
> +			    function(field) {
> +				//the fields don't exist earlier
> +				field.on('change', function() {
> +				    const store = Ext.getStore('diskstorage');
> +
> +				    // find by panel object because it is unique
> +				    const recordIndex = store.findBy(record =>
> +					record.data.panel === field.up('pveQemuHardDisk'),
> +				    );
> +				    const controllerSelector = field.up('pveControllerSelector');
> +				    const newControllerAndId = controllerSelector.getValuesAsString();
> +				    store.getAt(recordIndex).set('device', newControllerAndId);
> +				});
> +			    },
> +			);
> +			const wizard = this.up('pveQemuCreateWizard');
> +			Ext.Array.each(this.query('field'), function(field) {
> +			    field.on('change', wizard.validcheck);
> +			    field.on('validitychange', wizard.validcheck);
> +			});

any specific reason why this is an event of the added panel, and
not a normal function that is called after adding a new panel?

that way it would be way less confusing when it will called with
what parameter

also i do not like the whole down/up handling. it is ok
if used sparingly, but if the lower components often require
the upper elements, maybe it is better to give them some kind
of reference or callbacks. this way the component does not have
to know in which context it is used

> +		    },
> +		},
> +		validator: function() {
> +		    let valid = true;
> +		    const fields = this.query('field, fieldcontainer');
> +		    Ext.Array.each(fields, function(field) {
> +			if (Ext.isFunction(field.isValid) && !field.isValid()) {
> +				field.isValid();
> +			    valid = false;

i dont get it, we check if isValid is a function, call it if it is not,
call it again? and set valid to false

also indentation

> +			}
> +		    });
> +		    return valid;
> +		},
> +	    },
> +
> +	    // device ... device that the new disk should be assigned to, e.g. ide0, sata2
> +	    // path ... content of the textfield with source path
> +	    addDisk(device, path) {
> +		const initialValues = this.up('window').getValues();
> +		const item = Ext.clone(this.defaultItem);
> +		item.insideWizard = this.insideWizard;
> +		item.isCreate = this.isCreate;
> +		const added = this.add(item);

would not easier to use 'Ext.apply' ?

let foo = Ext.apply( { somedefaults: 'bar' }, this.defaultItem);
?

> +		// values in the storage will be updated by listeners
> +		if (path) {
> +		    // Need to explicitly deactivate when not rendered
> +		    added.down('radiofield[inputValue=empty]').setValue(false);
> +		    added.down('radiofield[inputValue=path]').setValue(true);
> +		    added.down('textfield[name=sourcePath]').setValue(path);
> +		}
> +
> +		const selector = added.down('pveControllerSelector');
> +		if (device) {
> +		    selector.setValue(device);
> +		} else {
> +		    selector.setVMConfig(initialValues);
> +		}

should that not be handled by the creation of the 'defaultItem'?

i'd rather like the 'defaultItem' to be its own class
that gets device/path as configuration options
and does the correct thing

this coupling outside of the class is very brittle
since what if we decide to remove a textfield or the controller selector?

> +
> +		return added;
> +	    },
> +	    removePanel: function(panelId) {
> +		this.remove(panelId, true);
> +	    },
> +	},
> +    ],
> +
> +    addDisk: function(device, path) {
> +	this.down('#panelContainer').addDisk(device, path);
> +	this.down('gridpanel').selectLast();
> +    },
> +
> +    removePanel: function(panelId) {
> +	this.down('#panelContainer').removePanel(panelId);
> +    },
> +
> +    beforeRender: function() {
> +	const me = this;
> +	const leftColumnPanel = me.items.get(0).items.get(0); // not the gridpanel
> +	leftColumnPanel.setFlex(me.leftColumnRatio);
> +	// any other panel because this has no height yet
> +	const panelHeight = me.up('tabpanel').items.get(0).getHeight();
> +	me.down('gridpanel').setHeight(panelHeight);
> +    },
> +
> +    setNodename: function(nodename) {
> +	this.nodename = nodename;
> +	this.query('pveQemuHardDisk').forEach(p => p.setNodename(nodename));
> +    },
> +
> +    listeners: {
> +	afterrender: function() {
> +	    const store = Ext.getStore('diskstorage');
> +	    const first = store.getAt(0);
> +	    if (first) {
> +		this.down('gridpanel').setSelection(first);
> +	    }
> +	},
> +    },
> +
> +    // values ... is optional

great that it's optional, but what does it ? ;)

> +    hasDuplicateDevices: function(values) {
> +	if (!values) {
> +	    values = this.up('form').getValues();
> +	}
> +	if (!Array.isArray(values.controller)) {
> +	    return false;
> +	}
> +	for (let i = 0; i < values.controller.length - 1; i++) {
> +	    for (let j = i+1; j < values.controller.length; j++) {
> +		if (
> +		    values.controller[i] === values.controller[j] &&
> +		    values.deviceid[i] === values.deviceid[j]
> +		) {
> +		    return true;
> +		}
> +	    }
> +	}
> +	return false;
> +    },
> +
> +    onGetValues: function(values) {
> +	if (this.hasDuplicateDevices(values)) {
> +	    Ext.Msg.alert(gettext('Error'), 'Equal target devices are forbidden. Make all unique!');
> +	}
> +	// Each child panel has sufficient onGetValues() => Return nothing
> +    },
> +
> +    validator: function() {
> +	const me = this;
> +	const panels = me.down('#panelContainer').items.getRange();
> +	return panels.every(panel => panel.validator()) && !me.hasDuplicateDevices();
> +    },
> +
> +    initComponent: function() {
> +	this.callParent();
> +	this.down('tableview').markDirty = false;

why set this here and not statically in the class?
(i think with 'viewConfig')


> +	this.down('#panelContainer').insideWizard = this.insideWizard;
> +	this.down('#panelContainer').isCreate = this.isCreate;

at this point, i'd suggest using let me = this; etc..

> +    },
> +
> +    deleteFromVMConfig: function(key) {
> +	this.query('pveQemuHardDisk').forEach(p => p.deleteFromVMConfig(key));
well that was a rollercoaster following this call

remove action click
store removal
store event remove
foreach record -> view.deleteFromVMConfig (this)
foreach qemuHardDisk -> deleteFromVMConfig
qemuHarddisk items each -> deletefromvmconfig
optionspanel -> up / down -> get datapanel
datapanel-> deleteFromVMConfig
bussel.deleteFromVMConfig, which deletes the actual config from the 
vmconfig....

sorry but this is simply too contrived...
why not having a (per wizard) vmconfig component/class/utility
that you call directly? no up/down

this could even emit events that the other components listen to,
then you'd have something like:

wizard.removeFromVMConfig('somekey')
-> vmDataClass.remove('somekey') -> sendsEvent (e.g. 'keyremoved')
all panels can subscribe to that...
(there are 'globalevents' that can be fired/listened to, see
the helpbutton for example)


> +    },
> +
> +    setVMConfig: function(vmconfig) {
> +	this.query('pveQemuHardDisk').forEach(p => p.setVMConfig(vmconfig));
> +    },
> +});
> diff --git a/www/manager6/qemu/disk/DiskData.js b/www/manager6/qemu/disk/DiskData.js
> new file mode 100644
> index 00000000..ff1bc163
> --- /dev/null
> +++ b/www/manager6/qemu/disk/DiskData.js
> @@ -0,0 +1,241 @@
> +/* 'change' property is assigned a string and then a function */
> +Ext.define('PVE.qemu.DiskData', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pveQemuDiskData',
> +    onlineHelp: 'qm_hard_disk',
> +
> +    insideWizard: false,
> +
> +    unused: false,
> +
> +    padding: '10 10 10 10',
> +
> +    vmconfig: {}, // used to select usused disks
> +
> +    viewModel: {},

same as in BandwidthSelector

> +
> +    /**
> +     * All radiofields in pveQemuDiskCollection have the same scope
> +     * Make name of radiofields unique for each disk
> +     */
> +    getRadioName() {
> +	return 'radio_' + this.id;
> +    },

same as in BandwidthSelector

> +
> +    getConfid() {
> +	const me = this;
> +	if (me.confid) {
> +	    return me.confid; // When editing disks
> +	}
> +	// In wizard
> +	const pairs = Object.entries(me.getValues());
> +	const confidArray = pairs.filter(([key, _]) => key !== "import_sources");
> +	// confidArray contains 1 array of length 2,
> +	// e.g. confidArray = [["sata1", "local:-1,format=qcow2"]]
> +	const confid = confidArray.shift().shift();
> +	return confid;
> +    },
> +
> +    onGetValues: function(values) {
> +	let me = this;
> +
> +	let params = {};
> +	let confid = me.confid || values.controller + values.deviceid;
> +
> +	const isImport = values.sourceVolid || values.sourcePath;
> +	if (me.unused) {
> +	    me.drive.file = me.vmconfig[values.unusedId];
> +	    confid = values.controller + values.deviceid;
> +	} else if (me.isCreate) {
> +	    if (values.hdimage) {
> +		me.drive.file = values.hdimage;
> +	    } else if (isImport) {
> +		me.drive.file = `${values.hdstorage}:-1`;
> +	    } else {
> +		me.drive.file = values.hdstorage + ":" + values.disksize;
> +	    }
> +	    me.drive.format = values.diskformat;
> +	}
> +
> +	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> +	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> +
> +	if (isImport) {
> +	    // exactly 1 of sourceVolid and sourcePath must be defined
> +	    params.import_sources = `${confid}=${isImport}`;
> +	}
> +
> +	params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +
> +	return params;
> +    },
> +
> +    setVMConfig: function(vmconfig) {
> +	let me = this;
> +
> +	me.vmconfig = vmconfig;
> +
> +	if (me.bussel) {
> +	    me.bussel.setVMConfig(vmconfig);
> +	    me.scsiController.setValue(vmconfig.scsihw);
> +	}
> +	if (me.unusedDisks) {
> +	    let disklist = [];
> +	    Ext.Object.each(vmconfig, function(key, value) {
> +		if (key.match(/^unused\d+$/)) {
> +		    disklist.push([key, value]);
> +		}
> +	    });
> +	    me.unusedDisks.store.loadData(disklist);
> +	    me.unusedDisks.setValue(me.confid);
> +	}
> +    },
> +
> +    deleteFromVMConfig: function(key) {
> +	const me = this;
> +	if (me.bussel) {
> +	    me.bussel.deleteFromVMConfig(key);
> +	}
> +    },
> +
> +    setDrive: function(drive) {
> +	let me = this;
> +
> +	me.drive = {};
> +	[
> +	    'interface',
> +	    'index',
> +	    'file',
> +	    'format',
> +	    'cache',
> +	    'discard',
> +	].forEach(o => { me.drive[o] = drive[o]; });
> +
> +	let values = {};
> +	let match = drive.file.match(/^([^:]+):/);
> +	if (match) {
> +	    values.hdstorage = match[1];
> +	}
> +
> +	values.hdimage = drive.file;
> +	values.diskformat = drive.format || 'raw';
> +	values.cache = drive.cache || '__default__';
> +	values.discard = drive.discard === 'on';
> +
> +	me.setValues(values);
> +    },
> +
> +    getDevice: function() {
> +	return this.bussel.getValuesAsString();
> +    },
> +
> +    setNodename: function(nodename) {
> +	const me = this;
> +	const hdstorage = me.down('#hdstorage');
> +	if (hdstorage) { // iff me.isCreate
> +	    hdstorage.setNodename(nodename);
> +	}
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +
> +	me.drive = {};
> +
> +	me.column1 = [];
> +	me.column2 = [];
> +
> +	const nodename = me.getViewModel().get('nodename');
> +
> +	if (!me.confid || me.unused) {
> +	    const controllerColumn = me.column2;
> +	    me.scsiController = Ext.create('Ext.form.field.Display', {
> +		    fieldLabel: gettext('SCSI Controller'),
> +		    reference: 'scsiController',
> +		    name: 'scsiController',
> +		    bind: me.insideWizard ? {
> +			value: '{current.scsihw}',
> +		    } : undefined,
> +		    renderer: PVE.Utils.render_scsihw,
> +		    submitValue: false,
> +		    hidden: true,
> +	    });
> +
> +	     me.bussel = Ext.create('PVE.form.ControllerSelector', {
> +		itemId: 'bussel',
> +		vmconfig: me.insideWizard ? { ide2: 'cdrom' } : {},
> +	    });
> +
> +	    me.bussel.down('field[name=controller]').addListener('change', function(_, newValue) {
> +		const allowIOthread = newValue.match(/^(virtio|scsi)/);
> +		const iothreadField = me.up('pveQemuHardDisk').down('field[name=iothread]');
> +		iothreadField.setDisabled(!allowIOthread);
> +		if (!allowIOthread) {
> +		    iothreadField.setValue(false);
> +		}
> +
> +		const virtio = newValue.match(/^virtio/);
> +		const ssdField = me.up('pveQemuHardDisk').down('field[name=ssd]');
> +		ssdField.setDisabled(virtio);
> +		if (virtio) {
> +		    ssdField.setValue(false);
> +		}
> +
> +		me.scsiController.setVisible(newValue.match(/^scsi/));
> +	    });
> +
> +	    controllerColumn.push(me.bussel);
> +	    controllerColumn.push(me.scsiController);
> +	}
> +
> +	if (me.unused) {
> +	    me.unusedDisks = Ext.create('Proxmox.form.KVComboBox', {
> +		name: 'unusedId',
> +		fieldLabel: gettext('Disk image'),
> +		matchFieldWidth: false,
> +		listConfig: {
> +		    width: 350,
> +		},
> +		data: [],
> +		allowBlank: false,
> +	    });
> +	    me.column1.push(me.unusedDisks);
> +	} else if (me.isCreate) {
> +	    let selector = {
> +		xtype: 'pveDiskStorageSelector',
> +		storageContent: 'images',
> +		name: 'disk',
> +		nodename: nodename,
> +		autoSelect: me.insideWizard,
> +	    };
> +	    selector.storageLabel = gettext('Storage');
> +	    me.column1.push(selector);
> +	} else {
> +	    me.column1.push({
> +		xtype: 'textfield',
> +		disabled: true,
> +		submitValue: false,
> +		fieldLabel: gettext('Disk image'),
> +		name: 'hdimage',
> +	    });
> +	}
> +
> +	me.column2.push(
> +	    {
> +		xtype: 'CacheTypeSelector',
> +		name: 'cache',
> +		value: '__default__',
> +		fieldLabel: gettext('Cache'),
> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Discard'),
> +		reference: 'discard',
> +		name: 'discard',
> +	    },
> +	);
> +
> +	me.callParent();
> +    },
> +});
> diff --git a/www/manager6/qemu/disk/HardDisk.js b/www/manager6/qemu/disk/HardDisk.js
> new file mode 100644
> index 00000000..3e352af8
> --- /dev/null
> +++ b/www/manager6/qemu/disk/HardDisk.js
> @@ -0,0 +1,215 @@
> +/* 'change' property is assigned a string and then a function */
> +Ext.define('PVE.qemu.HardDisk', {
> +    extend: 'Ext.tab.Panel',
> +    alias: 'widget.pveQemuHardDisk',
> +    onlineHelp: 'qm_hard_disk',
> +
> +    plain: true,
> +
> +    bind: {
> +	nodename: '{nodename}',
> +    },

should that not be defined where it is used?

> +
> +    insideWizard: false,
> +
> +    isCreate: false,
> +
> +    setNodename: function(nodename) {
> +	this.nodename = nodename;
> +	this.items.each(panel => panel.setNodename(nodename));
> +    },
> +
> +    setDrive: function(drive) {
> +	this.items.each(i => i.setDrive(drive));
> +    },
> +
> +    getDevice: function() {
> +	return this.down('pveQemuDiskData').getDevice();
> +    },
> +
> +    items: [
> +	{
> +	    title: gettext('Data'),
> +	    xtype: 'pveQemuDiskData',
> +	    bind: {
> +		nodename: '{nodename}',
> +	    },
> +	},
> +    ],
> +
> +    beforeRender: function() {
> +	const me = this;
> +	const tabPosition = me.insideWizard ? 'bottom' : 'top';
> +	me.setTabPosition(tabPosition);

why here and not in initComponent?

> +	// any other panel because this has no height yet
> +	if (me.insideWizard) {
> +	    const panelHeight = me.up('tabpanel').items.get(0).getHeight();
> +	    me.setHeight(panelHeight);
> +	}
> +    },
> +
> +    initComponent: function() {
> +	const me = this;
> +
> +	let diskData = me.items[0];
> +	diskData.confid = me.confid;
> +	diskData.isCreate = me.isCreate;
> +	diskData.insideWizard = me.insideWizard > +
> +	const basicOptions = {
> +	    xtype: 'pveQemuDiskBasicOptions',
> +	    isCreate: me.isCreate,
> +	    confid: me.confid,
> +	    insideWizard: me.insideWizard,
> +	    bind: {
> +		nodename: '{nodename}',
> +	    },
> +	};
> +	const bandwidthOptions = {
> +	    xtype: 'pveQemuDiskBandwidthOptions',
> +	    isCreate: me.isCreate,
> +	    insideWizard: me.insideWizard,
> +	    bind: {
> +		nodename: '{nodename}',
> +	    },
> +	};
> +
> +	if (me.insideWizard) {
> +	    me.items = me.items.concat([
> +		{
> +		    title: gettext('Options'),
> +		    xtype: 'panel',
> +		    layout: {
> +			type: 'vbox',
> +		    },
> +		    defaults: {
> +			width: '100%',
> +			margin: '0 0 10 0',
> +		    },
> +		    items: [
> +			basicOptions,
> +			{
> +				xtype: 'box',
> +				autoEl: { tag: 'hr' },
> +			},
> +			bandwidthOptions,
> +		    ],
> +		    setNodename: function(nodename) {
> +			this.nodename = nodename;
> +			if (basicOptions.setNodename) { // added after initialization
> +			    basicOptions.setNodename(nodename);
> +			    bandwidthOptions.setNodename(nodename);
> +			}
> +		    },
> +		    setVMConfig: function(vmconfig) {
> +			this.down('pveQemuDiskBasicOptions').setVMConfig(vmconfig);
> +			this.down('pveQemuDiskBandwidthOptions').setVMConfig(vmconfig);
> +		    },
> +		    deleteFromVMConfig: function(key) {
> +			const panel = this.up('pveQemuHardDisk').down('pveQemuDiskData');
> +			if (panel) {
> +			    panel.deleteFromVMConfig(key);
> +			}
> +		    },
> +		    setDrive: function(drive) {
> +			this.down('pveQemuDiskBasicOptions').setDrive(drive);
> +			this.down('pveQemuDiskBandwidthOptions').setDrive(drive);
> +		    },
> +		},
> +	    ]);
> +	} else {
> +	    basicOptions.title = gettext('Options');
> +	    bandwidthOptions.title = gettext('Bandwidth');
> +	    me.items = me.items.concat([basicOptions, bandwidthOptions]);
> +	}
> +
> +	me.callParent();
> +    },
> +
> +    setVMConfig: function(vmconfig) {
> +	this.items.each(panel => panel.setVMConfig(vmconfig));
> +    },
> +    deleteFromVMConfig: function(key) {
> +	this.items.each(panel => panel.deleteFromVMConfig(key));
> +    },
> +});
> +
> +Ext.define('PVE.qemu.HardDiskWindow', {
> +    extend: 'Proxmox.window.Edit',
> +
> +    isAdd: true,
> +
> +    backgroundDelay: 5,
> +
> +    setNodename: function(nodename) {
> +	this.nodename = nodename;
> +	this.down('pveQemuHDTabpanel').setNodename(nodename);
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	const selnode = me.pveSelNode && me.pveSelNode.data && me.pveSelNode.data.node;
> +	if (selnode && !me.nodename) {
> +	    me.nodename = selnode;
> +	}
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +
> +	const unused = me.confid && me.confid.match(/^unused\d+$/);
> +
> +	me.isCreate = me.confid ? unused : true;
> +
> +	let ipanel = Ext.create('PVE.qemu.HardDisk', {
> +	    confid: me.confid,
> +	    unused: unused,
> +	    isCreate: me.isCreate,
> +	});
> +	ipanel.setNodename(me.nodename);
> +
> +	if (unused) {
> +	    me.subject = gettext('Unused Disk');
> +	} else if (me.isCreate) {
> +	    me.subject = gettext('Hard Disk');
> +	} else {
> +	    me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
> +	}
> +
> +	me.items = [ipanel];
> +
> +	me.callParent();
> +	/* 'data' is assigned an empty array in same file, and here we
> +	 * use it like an object
> +	 */
> +	me.load({
> +	    success: function(response, options) {
> +		ipanel.setVMConfig(response.result.data);
> +		if (me.confid) {
> +		    let value = response.result.data[me.confid];
> +		    let drive = PVE.Parser.parseQemuDrive(me.confid, value);
> +		    if (!drive) {
> +			Ext.Msg.alert(gettext('Error'), 'Unable to parse drive options');
> +			me.close();
> +			return;
> +		    }
> +		    ipanel.setDrive(drive);
> +		    me.isValid(); // trigger validation
> +		}
> +	    },
> +	});
> +    },
> +    getValues: function() {
> +	let values = this.callParent();
> +	for (const [key, value] of Object.entries(values)) {
> +	    const re = /ide\d+|sata\d+|virtio\d+|scsi\d+|import_sources/;
> +	    if (key.match(re) && Array.isArray(value)) {
> +		// Collected from different panels => array
> +		// But API & some GUI functions expect not array
> +		const sep = key === 'import_sources' ? '\0' : ','; // for API
> +		values[key] = value.join(sep);
> +	    }
> +	}
> +	return values;
> +    },
> +});
> diff --git a/www/manager6/window/Wizard.js b/www/manager6/window/Wizard.js
> index 47d60b8e..de935fd0 100644
> --- a/www/manager6/window/Wizard.js
> +++ b/www/manager6/window/Wizard.js
> @@ -245,6 +245,8 @@ Ext.define('PVE.window.Wizard', {
>   	    };
>   	    field.on('change', validcheck);
>   	    field.on('validitychange', validcheck);
> +	    // Make available for fields that get added later
> +	    me.validcheck = validcheck;
>   	});
>       },
>   });
>