[pve-devel] [PATCH v3 manager 0/4] ui: lxc/qemu: add reassign for disks and volumes

[pve-devel] [PATCH v3 manager 0/4] ui: lxc/qemu: add reassign for disks and volumes

[Aaron Lauterer]

This series adds the UI to reassign a disk / volume from one guest to another.

To avoid button clutter, the Move, Reassing and Resize buttons are moved
into a new submenu called "Disk/Volume Action".

Patch 2 to 4 are optional. Patch 2 changes the labels for Move, Reassign
and Resize to remove Volume & Disk as we already have this in the
context of the submenu.

Patch 3 only changes a double negated option and patch 4 happend in the
process of working on an interface for the reassign functionality. Since
the work of modernizing this componend is done, why not use it

v3:
* change to Edit window, removing quite some boilerplate code
* create new submenu for disk/volume actions
* incorporate smaller style nits
* simplify other labels as well, removing 'Volume' and 'Disk' as the
  context gives that away already

v2: incorporated feedback I got off list, mainly
* using more modern approaches
* more arrow functions
* reducing use of predefined cbind values and using inline functions
  when possible

Aaron Lauterer (4):
  ui: lxc/qemu: add disk reassign and action submenu
  ui: lxc/qemu: disk/volume action simplify menu items
  ui: BusTypeSelector: change noVirtIO to withVirtIO
  ui: hdmove: modernize/refactor

 www/manager6/Makefile                   |   1 +
 www/manager6/form/BusTypeSelector.js    |   4 +-
 www/manager6/form/ControllerSelector.js |   4 +-
 www/manager6/lxc/Resources.js           |  66 ++++--
 www/manager6/qemu/CDEdit.js             |   2 +-
 www/manager6/qemu/CIDriveEdit.js        |   2 +-
 www/manager6/qemu/HDMove.js             | 192 ++++++++---------
 www/manager6/qemu/HDReassign.js         | 274 ++++++++++++++++++++++++
 www/manager6/qemu/HardwareView.js       |  65 +++++-
 9 files changed, 476 insertions(+), 134 deletions(-)
 create mode 100644 www/manager6/qemu/HDReassign.js

-- 
2.30.2

[pve-devel] [PATCH v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu

[Aaron Lauterer]

For the new HDReassign component, we follow the approach of HDMove to
have one componend for qemu and lxc.

To avoid button clutter, a new "Disk/Volume action" button is
introduced. It holds the Move, Reassign and Resize buttons in a submenu.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since

v2:
* switch from generic to Proxmox Edit window
* add new submenu for disk/volume specific actions
* code style improvements
* simplify some labels, removing "disk" and "volume" as the context
  already gives this away


v1: incorporated feedback I got off list

* use more modern approaches
    * arrow functions
    * autoShow
    * template strings
* reduce predefined cbind values and use arrow functions in the cbind
  directly in many cases
* some code style issues and cleanup

 www/manager6/Makefile             |   1 +
 www/manager6/lxc/Resources.js     |  62 +++++--
 www/manager6/qemu/HDReassign.js   | 274 ++++++++++++++++++++++++++++++
 www/manager6/qemu/HardwareView.js |  60 ++++++-
 4 files changed, 378 insertions(+), 19 deletions(-)
 create mode 100644 www/manager6/qemu/HDReassign.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index e6e01bd1..94a78d89 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -214,6 +214,7 @@ JSSRC= 							\
 	qemu/HDTPM.js					\
 	qemu/HDMove.js					\
 	qemu/HDResize.js				\
+	qemu/HDReassign.js				\
 	qemu/HardwareView.js				\
 	qemu/IPConfigEdit.js				\
 	qemu/KeyboardEdit.js				\
diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 15ee3c67..306a4988 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -151,7 +151,8 @@ Ext.define('PVE.lxc.RessourceView', {
 	    });
 	};
 
-	var run_move = function(b, e, rec) {
+	var run_move = function() {
+	    let rec = me.selModel.getSelection()[0];
 	    if (!rec) {
 		return;
 	    }
@@ -168,6 +169,24 @@ Ext.define('PVE.lxc.RessourceView', {
 	    win.on('destroy', me.reload, me);
 	};
 
+	let run_reassign = function() {
+	    let rec = me.selModel.getSelection()[0];
+	    if (!rec) {
+		return;
+	    }
+
+	    Ext.create('PVE.window.HDReassign', {
+		disk: rec.data.key,
+		nodename: nodename,
+		autoShow: true,
+		vmid: vmid,
+		type: 'lxc',
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	};
+
 	var edit_btn = new Proxmox.button.Button({
 	    text: gettext('Edit'),
 	    selModel: me.selModel,
@@ -182,7 +201,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	    handler: function() { me.run_editor(); },
 	});
 
-	var resize_btn = new Proxmox.button.Button({
+	var resize_menuitem = new Ext.menu.Item({
 	    text: gettext('Resize disk'),
 	    selModel: me.selModel,
 	    disabled: true,
@@ -227,14 +246,34 @@ Ext.define('PVE.lxc.RessourceView', {
 	    },
 	});
 
-	var move_btn = new Proxmox.button.Button({
+	let reassign_menuitem = new Ext.menu.Item({
+	    text: gettext('Reassign Volume'),
+	    tooltip: gettext('Reassign volume to another CT'),
+	    handler: run_reassign,
+	    reference: 'reassing_item',
+	    disabled: true,
+	});
+
+	let move_menuitem = new Ext.menu.Item({
 	    text: gettext('Move Volume'),
 	    selModel: me.selModel,
 	    disabled: true,
-	    dangerous: true,
 	    handler: run_move,
 	});
 
+	let volumeaction_btn = new Proxmox.button.Button({
+	    text: gettext('Volume Action'),
+	    disabled: true,
+	    menu: {
+		plain: true,
+		items: [
+		    move_menuitem,
+		    reassign_menuitem,
+		    resize_menuitem,
+		],
+	    },
+	});
+
 	var revert_btn = new PVE.button.PendingRevert();
 
 	var set_button_status = function() {
@@ -243,7 +282,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	    if (!rec) {
 		edit_btn.disable();
 		remove_btn.disable();
-		resize_btn.disable();
+		volumeaction_btn.disable();
 		revert_btn.disable();
 		return;
 	    }
@@ -253,6 +292,7 @@ Ext.define('PVE.lxc.RessourceView', {
 
 	    var pending = rec.data.delete || me.hasPendingChanges(key);
 	    var isDisk = rowdef.tdCls === 'pve-itype-icon-storage';
+	    let isRootFS = rec.data.key === 'rootfs';
 	    var isUnusedDisk = key.match(/^unused\d+/);
 	    var isUsedDisk = isDisk && !isUnusedDisk;
 
@@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', {
 	    }
 	    edit_btn.setDisabled(noedit);
 
-	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
-	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
-	    move_btn.setDisabled(!isDisk || !diskCap);
+	    volumeaction_btn.setDisabled(!isDisk);
+	    reassign_menuitem.setDisabled(isRootFS);
+
+	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
+	    resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk);
+	    move_menuitem.setDisabled(!isDisk || !diskCap);
 	    revert_btn.setDisabled(!pending);
 
 	    remove_btn.setText(isUsedDisk ? remove_btn.altText : remove_btn.defaultText);
@@ -327,8 +370,7 @@ Ext.define('PVE.lxc.RessourceView', {
 		},
 		edit_btn,
 		remove_btn,
-		resize_btn,
-		move_btn,
+		volumeaction_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
new file mode 100644
index 00000000..48bc0b1f
--- /dev/null
+++ b/www/manager6/qemu/HDReassign.js
@@ -0,0 +1,274 @@
+Ext.define('PVE.window.HDReassign', {
+    extend: 'Proxmox.window.Edit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    resizable: false,
+    modal: true,
+    width: 350,
+    border: false,
+    layout: 'fit',
+    showReset: false,
+    showProgress: true,
+    method: 'POST',
+
+    viewModel: {
+	data: {
+	    mpType: '',
+	},
+	formulas: {
+	    mpMaxCount: get => get('mpType') === 'mp'
+		? PVE.Utils.mp_counts.mps - 1
+		: PVE.Utils.mp_counts.unused - 1,
+	},
+    },
+
+    cbindData: function() {
+	let me = this;
+	return {
+	    vmid: me.vmid,
+	    disk: me.disk,
+	    isQemu: me.type === 'qemu',
+	    nodename: me.nodename,
+	    url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
+	};
+    },
+
+    cbind: {
+	title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
+	submitText: get => get('title'),
+	qemu: '{isQemu}',
+	url: '{url}',
+    },
+
+    submitUrl: function(url, values) {
+	url += this.qemu ? 'move_disk' : 'move_volume';
+	return url;
+    },
+
+    getValues: function() {
+	let me = this;
+	let values = me.formPanel.getForm().getValues();
+
+	let params = {
+	    vmid: me.vmid,
+	    'target-vmid': values.targetVmid,
+	};
+
+	params[me.qemu ? 'disk' : 'volume'] = me.disk;
+
+	if (me.qemu) {
+	    params['target-disk'] = `${values.controller}${values.deviceid}`;
+	} else {
+	    params['target-volume'] = `${values.mpType}${values.mpId}`;
+	}
+	return params;
+    },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	initViewModel: function(model) {
+	    let view = this.getView();
+	    let mpTypeValue = view.disk.match(/^unused\d+/) ? 'unused' : 'mp';
+	    model.set('mpType', mpTypeValue);
+	},
+
+	onMpTypeChange: function(value) {
+	    this.getView().getViewModel().set('mpType', value.getValue());
+	    this.getView().lookup('mpIdSelector').validate();
+	},
+
+	onTargetVMChange: function(f, vmid) {
+	    let me = this;
+	    let view = me.getView();
+	    let diskSelector = view.lookup('diskSelector');
+	    if (!vmid) {
+		diskSelector.setVMConfig(null);
+		me.VMConfig = null;
+		return;
+	    }
+
+	    let type = view.qemu ? 'qemu' : 'lxc';
+
+	    let url = `/nodes/${view.nodename}/${type}/${vmid}/config`;
+	    Proxmox.Utils.API2Request({
+		url: url,
+		method: 'GET',
+		failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+		success: function(response, options) {
+		    if (view.qemu) {
+			diskSelector.setVMConfig(response.result.data);
+			diskSelector.setDisabled(false);
+		    } else {
+			let mpIdSelector = view.lookup('mpIdSelector');
+			let mpType = view.lookup('mpType');
+
+			view.VMConfig = response.result.data;
+
+			PVE.Utils.forEachMP((bus, i) => {
+			    let name = view.getViewModel().get('mpType') + i.toString();
+			    if (!Ext.isDefined(view.VMConfig[name])) {
+				mpIdSelector.setValue(i);
+				return false;
+			    }
+			    return undefined;
+			});
+
+			mpType.setDisabled(false);
+			mpIdSelector.setDisabled(false);
+			mpIdSelector.validate();
+		    }
+		},
+	    });
+	},
+    },
+
+    items: [
+	{
+	    xtype: 'form',
+	    reference: 'moveFormPanel',
+	    bodyPadding: 10,
+	    border: false,
+	    fieldDefaults: {
+		labelWidth: 100,
+		anchor: '100%',
+	    },
+	    items: [
+		{
+		    xtype: 'displayfield',
+		    name: 'sourceDisk',
+		    fieldLabel: gettext('Source'),
+		    cbind: {
+			name: get => get('isQemu') ? 'disk' : 'volume',
+			value: '{disk}',
+		    },
+		    vtype: 'StorageId',
+		    allowBlank: false,
+		},
+		{
+		    xtype: 'vmComboSelector',
+		    reference: 'targetVMID',
+		    name: 'targetVmid',
+		    allowBlank: false,
+		    fieldLabel: gettext('Target'),
+		    bind: {
+			value: '{targetVMID}',
+		    },
+		    store: {
+			model: 'PVEResources',
+			autoLoad: true,
+			sorters: 'vmid',
+			cbind: {}, // for nested cbinds
+			filters: [
+			    {
+				property: 'type',
+				cbind: {
+				    value: get => get('isQemu') ? /qemu/ : /lxc/,
+				},
+			    },
+			    {
+				property: 'node',
+				cbind: {
+				    value: '{nodename}',
+				},
+			    },
+			    {
+				property: 'vmid',
+				operator: '!=',
+				cbind: {
+				    value: '{vmid}',
+				},
+			    },
+			],
+		    },
+		    listeners: { change: 'onTargetVMChange' },
+		},
+		{
+		    xtype: 'pveControllerSelector',
+		    reference: 'diskSelector',
+		    withUnused: true,
+		    disabled: true,
+		    cbind: {
+			hidden: '{!isQemu}',
+		    },
+		},
+		{
+		    xtype: 'container',
+		    layout: 'hbox',
+		    cbind: {
+			hidden: '{isQemu}',
+			disabled: '{isQemu}',
+		    },
+		    items: [
+			{
+			    xtype: 'pmxDisplayEditField',
+			    cbind: {
+				editable: get => !get('disk').match(/^unused\d+/),
+				value: get => get('disk').match(/^unused\d+/) ? 'unused' : 'mp',
+			    },
+			    disabled: true,
+			    name: 'mpType',
+			    reference: 'mpType',
+			    fieldLabel: gettext('Add as'),
+			    submitValue: true,
+			    flex: 4,
+			    editConfig: {
+				xtype: 'proxmoxKVComboBox',
+				name: 'mpTypeCombo',
+				reference: 'mpTypeCombo',
+				deleteEmpty: false,
+				cbind: {
+				    hidden: '{isQemu}',
+				},
+				comboItems: [
+				    ['mp', gettext('Mount Point')],
+				    ['unused', gettext('Unused')],
+				],
+				listeners: { change: 'onMpTypeChange' },
+			    },
+			},
+			{
+			    xtype: 'proxmoxintegerfield',
+			    name: 'mpId',
+			    reference: 'mpIdSelector',
+			    minValue: 0,
+			    flex: 1,
+			    allowBlank: false,
+			    validateOnChange: true,
+			    disabled: true,
+			    bind: {
+				maxValue: '{mpMaxCount}',
+			    },
+			    validator: function(value) {
+				let view = this.up('window');
+				let type = view.getViewModel().get('mpType');
+				if (Ext.isDefined(view.VMConfig[type+value])) {
+				    return "Mount point is already in use.";
+				}
+				return true;
+			    },
+			},
+		    ],
+		},
+	    ],
+	},
+    ],
+
+    initComponent: function() {
+	let me = this;
+
+	if (!me.nodename) {
+	    throw "no node name specified";
+	}
+
+	if (!me.vmid) {
+	    throw "no VM ID specified";
+	}
+
+	if (!me.type) {
+	    throw "no type specified";
+	}
+
+	me.callParent();
+    },
+});
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 6cea4287..1f19269c 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -417,6 +417,24 @@ Ext.define('PVE.qemu.HardwareView', {
 	    win.on('destroy', me.reload, me);
 	};
 
+	let run_reassign = function() {
+	    let rec = sm.getSelection()[0];
+	    if (!rec) {
+		return;
+	    }
+
+	    Ext.create('PVE.window.HDReassign', {
+		disk: rec.data.key,
+		nodename: nodename,
+		autoShow: true,
+		vmid: vmid,
+		type: 'qemu',
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	};
+
 	var edit_btn = new Proxmox.button.Button({
 	    text: gettext('Edit'),
 	    selModel: sm,
@@ -424,20 +442,40 @@ Ext.define('PVE.qemu.HardwareView', {
 	    handler: run_editor,
         });
 
-	var resize_btn = new Proxmox.button.Button({
+	var resize_menuitem = new Ext.menu.Item({
 	    text: gettext('Resize disk'),
 	    selModel: sm,
 	    disabled: true,
 	    handler: run_resize,
 	});
 
-	var move_btn = new Proxmox.button.Button({
+	let move_menuitem = new Ext.menu.Item({
 	    text: gettext('Move disk'),
 	    selModel: sm,
-	    disabled: true,
 	    handler: run_move,
 	});
 
+	let reassign_menuitem = new Ext.menu.Item({
+	    text: gettext('Reassign Disk'),
+	    tooltip: gettext('Reassign disk to another VM'),
+	    handler: run_reassign,
+	    selModel: sm,
+	});
+
+	let diskaction_btn = new Proxmox.button.Button({
+	    text: gettext('Disk Action'),
+	    disabled: true,
+	    menu: {
+		plain: true,
+		items: [
+		    move_menuitem,
+		    reassign_menuitem,
+		    resize_menuitem,
+		],
+	    },
+	});
+
+
 	var remove_btn = new Proxmox.button.Button({
 	    text: gettext('Remove'),
 	    defaultText: gettext('Remove'),
@@ -573,8 +611,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    if (!rec) {
 		remove_btn.disable();
 		edit_btn.disable();
-		resize_btn.disable();
-		move_btn.disable();
+		diskaction_btn.disable();
 		revert_btn.disable();
 		return;
 	    }
@@ -611,9 +648,15 @@ Ext.define('PVE.qemu.HardwareView', {
 	        (isDisk && !diskCap),
 	    );
 
-	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
+	    resize_menuitem.setDisabled(pending || !isUsedDisk || !diskCap);
+	    reassign_menuitem.setDisabled(pending || (isEfi || tpmMoveable));
 
-	    move_btn.setDisabled(pending || !(isUsedDisk || isEfi || tpmMoveable) || !diskCap);
+	    diskaction_btn.setDisabled(
+		pending ||
+		isCloudInit ||
+		!(isDisk || isEfi || tpmMoveable) ||
+		!diskCap,
+	    );
 
 	    revert_btn.setDisabled(!pending);
 	};
@@ -775,8 +818,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		},
 		remove_btn,
 		edit_btn,
-		resize_btn,
-		move_btn,
+		diskaction_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
-- 
2.30.2

[pve-devel] [PATCH v3 manager 2/4] ui: lxc/qemu: disk/volume action simplify menu items

[Aaron Lauterer]

We already know that we are acting upon a disk / volume due to the
submenu we are in.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/lxc/Resources.js     | 6 +++---
 www/manager6/qemu/HardwareView.js | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 306a4988..a5df9182 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -202,7 +202,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	});
 
 	var resize_menuitem = new Ext.menu.Item({
-	    text: gettext('Resize disk'),
+	    text: gettext('Resize'),
 	    selModel: me.selModel,
 	    disabled: true,
 	    handler: run_resize,
@@ -247,7 +247,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	});
 
 	let reassign_menuitem = new Ext.menu.Item({
-	    text: gettext('Reassign Volume'),
+	    text: gettext('Reassign'),
 	    tooltip: gettext('Reassign volume to another CT'),
 	    handler: run_reassign,
 	    reference: 'reassing_item',
@@ -255,7 +255,7 @@ Ext.define('PVE.lxc.RessourceView', {
 	});
 
 	let move_menuitem = new Ext.menu.Item({
-	    text: gettext('Move Volume'),
+	    text: gettext('Move'),
 	    selModel: me.selModel,
 	    disabled: true,
 	    handler: run_move,
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 1f19269c..21f67cd0 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -443,20 +443,20 @@ Ext.define('PVE.qemu.HardwareView', {
         });
 
 	var resize_menuitem = new Ext.menu.Item({
-	    text: gettext('Resize disk'),
+	    text: gettext('Resize'),
 	    selModel: sm,
 	    disabled: true,
 	    handler: run_resize,
 	});
 
 	let move_menuitem = new Ext.menu.Item({
-	    text: gettext('Move disk'),
+	    text: gettext('Move'),
 	    selModel: sm,
 	    handler: run_move,
 	});
 
 	let reassign_menuitem = new Ext.menu.Item({
-	    text: gettext('Reassign Disk'),
+	    text: gettext('Reassign'),
 	    tooltip: gettext('Reassign disk to another VM'),
 	    handler: run_reassign,
 	    selModel: sm,
-- 
2.30.2

[pve-devel] [PATCH v3 manager 3/4] ui: BusTypeSelector: change noVirtIO to withVirtIO

[Aaron Lauterer]

Double negated properties make it harder than necessary to parse
conditions.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/form/BusTypeSelector.js    | 4 ++--
 www/manager6/form/ControllerSelector.js | 4 ++--
 www/manager6/qemu/CDEdit.js             | 2 +-
 www/manager6/qemu/CIDriveEdit.js        | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/www/manager6/form/BusTypeSelector.js b/www/manager6/form/BusTypeSelector.js
index a420e56f..0f040229 100644
--- a/www/manager6/form/BusTypeSelector.js
+++ b/www/manager6/form/BusTypeSelector.js
@@ -2,7 +2,7 @@ Ext.define('PVE.form.BusTypeSelector', {
     extend: 'Proxmox.form.KVComboBox',
     alias: 'widget.pveBusSelector',
 
-    noVirtIO: false,
+    withVirtIO: true,
     withUnused: false,
 
     initComponent: function() {
@@ -10,7 +10,7 @@ Ext.define('PVE.form.BusTypeSelector', {
 
 	me.comboItems = [['ide', 'IDE'], ['sata', 'SATA']];
 
-	if (!me.noVirtIO) {
+	if (me.withVirtIO) {
 	    me.comboItems.push(['virtio', 'VirtIO Block']);
 	}
 
diff --git a/www/manager6/form/ControllerSelector.js b/www/manager6/form/ControllerSelector.js
index 798dc4b2..d84c49d6 100644
--- a/www/manager6/form/ControllerSelector.js
+++ b/www/manager6/form/ControllerSelector.js
@@ -2,7 +2,7 @@ Ext.define('PVE.form.ControllerSelector', {
     extend: 'Ext.form.FieldContainer',
     alias: 'widget.pveControllerSelector',
 
-    noVirtIO: false,
+    withVirtIO: true,
     withUnused: false,
 
     vmconfig: {}, // used to check for existing devices
@@ -73,7 +73,7 @@ Ext.define('PVE.form.ControllerSelector', {
 		    name: 'controller',
 		    itemId: 'controller',
 		    value: PVE.qemu.OSDefaults.generic.busType,
-		    noVirtIO: me.noVirtIO,
+		    withVirtIO: me.withVirtIO,
 		    withUnused: me.withUnused,
 		    allowBlank: false,
 		    flex: 2,
diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
index 72c01037..fc7a59cc 100644
--- a/www/manager6/qemu/CDEdit.js
+++ b/www/manager6/qemu/CDEdit.js
@@ -71,7 +71,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 
 	if (!me.confid) {
 	    me.bussel = Ext.create('PVE.form.ControllerSelector', {
-		noVirtIO: true,
+		withVirtIO: false,
 	    });
 	    items.push(me.bussel);
 	}
diff --git a/www/manager6/qemu/CIDriveEdit.js b/www/manager6/qemu/CIDriveEdit.js
index 754b8353..a9ca8bf1 100644
--- a/www/manager6/qemu/CIDriveEdit.js
+++ b/www/manager6/qemu/CIDriveEdit.js
@@ -36,7 +36,7 @@ Ext.define('PVE.qemu.CIDriveInputPanel', {
 	me.items = [
 	    {
 		xtype: 'pveControllerSelector',
-		noVirtIO: true,
+		withVirtIO: false,
 		itemId: 'drive',
 		fieldLabel: gettext('CloudInit Drive'),
 		name: 'drive',
-- 
2.30.2

[pve-devel] [PATCH v3 manager 4/4] ui: hdmove: modernize/refactor

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since

v2:
* switch from generic window to proxmox edit

v1: much of the feedback to the HDReassign.js from the
first patch has been incorporated here as well.

* reducing predefined cbind values for more arrow functions
* using more arrow functions in general
* template strings
 www/manager6/qemu/HDMove.js       | 192 ++++++++++++++----------------
 www/manager6/qemu/HardwareView.js |   1 +
 2 files changed, 88 insertions(+), 105 deletions(-)

diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
index 181b7bdc..3e94479c 100644
--- a/www/manager6/qemu/HDMove.js
+++ b/www/manager6/qemu/HDMove.js
@@ -1,48 +1,102 @@
 Ext.define('PVE.window.HDMove', {
-    extend: 'Ext.window.Window',
+    extend: 'Proxmox.window.Edit',
+    mixins: ['Proxmox.Mixin.CBind'],
 
     resizable: false,
+    modal: true,
+    width: 350,
+    border: false,
+    layout: 'fit',
+    showReset: false,
+    showProgress: true,
+    method: 'POST',
+
+    cbindData: function() {
+	let me = this;
+	return {
+	    disk: me.disk,
+	    isQemu: me.type === 'qemu',
+	    nodename: me.nodename,
+	    url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
+	};
+    },
+
+    cbind: {
+	title: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
+	submitText: get => get('title'),
+	qemu: '{isQemu}',
+	url: '{url}',
+    },
+
+    submitUrl: function(url, values) {
+	url += this.qemu ? 'move_disk' : 'move_volume';
+	return url;
+    },
 
+    getValues: function() {
+	let me = this;
+	let values = me.formPanel.getForm().getValues();
 
-    move_disk: function(disk, storage, format, delete_disk) {
-	var me = this;
-	var qemu = me.type === 'qemu';
-	var params = {};
-	params.storage = storage;
-	params[qemu ? 'disk':'volume'] = disk;
+	let params = {
+	    storage: values.hdstorage,
+	};
+	params[me.qemu ? 'disk':'volume'] = me.disk;
 
-	if (format && qemu) {
-	    params.format = format;
+	if (values.diskformat && me.qemu) {
+	    params.format = values.diskformat;
 	}
 
-	if (delete_disk) {
+	if (values.deleteDisk) {
 	    params.delete = 1;
 	}
-
-	var url = '/nodes/' + me.nodename + '/' + me.type + '/' + me.vmid + '/';
-	url += qemu ? 'move_disk' : 'move_volume';
-
-	Proxmox.Utils.API2Request({
-	    params: params,
-	    url: url,
-	    waitMsgTarget: me,
-	    method: 'POST',
-	    failure: function(response, opts) {
-		Ext.Msg.alert('Error', response.htmlStatus);
-	    },
-	    success: function(response, options) {
-		var upid = response.result.data;
-		var win = Ext.create('Proxmox.window.TaskViewer', {
-		    upid: upid,
-		});
-		win.show();
-		win.on('destroy', function() { me.close(); });
-	    },
-	});
+	return params;
     },
+    items: [
+	{
+	    xtype: 'form',
+	    reference: 'moveFormPanel',
+	    bodyPadding: 10,
+	    border: false,
+	    fieldDefaults: {
+		labelWidth: 100,
+		anchor: '100%',
+	    },
+	    items: [
+		{
+		    xtype: 'displayfield',
+		    cbind: {
+			name: get => get('isQemu') ? 'disk' : 'volume',
+			fieldLabel: get => get('isQemu')
+			    ? gettext('Disk')
+			    : gettext('Mount Point'),
+			value: '{disk}',
+		    },
+		    vtype: 'StorageId',
+		    allowBlank: false,
+		},
+		{
+		    xtype: 'pveDiskStorageSelector',
+		    storageLabel: gettext('Target Storage'),
+		    cbind: {
+			nodename: '{nodename}',
+			storageContent: get => get('isQemu') ? 'images' : 'rootdir',
+			hideFormat: get => get('disk') === 'tpmstate0',
+		    },
+		    hideSize: true,
+		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    fieldLabel: gettext('Delete source'),
+		    name: 'deleteDisk',
+		    uncheckedValue: 0,
+		    checked: false,
+		},
+	    ],
+	},
+    ],
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.nodename) {
 	    throw "no node name specified";
@@ -53,81 +107,9 @@ Ext.define('PVE.window.HDMove', {
 	}
 
 	if (!me.type) {
-	    me.type = 'qemu';
+	    throw "no type specified";
 	}
 
-	var qemu = me.type === 'qemu';
-
-        var items = [
-            {
-                xtype: 'displayfield',
-                name: qemu ? 'disk' : 'volume',
-                value: me.disk,
-                fieldLabel: qemu ? gettext('Disk') : gettext('Mount Point'),
-                vtype: 'StorageId',
-                allowBlank: false,
-            },
-        ];
-
-	items.push({
-	    xtype: 'pveDiskStorageSelector',
-	    storageLabel: gettext('Target Storage'),
-	    nodename: me.nodename,
-	    storageContent: qemu ? 'images' : 'rootdir',
-	    hideSize: true,
-	    hideFormat: me.disk === 'tpmstate0',
-	});
-
-	items.push({
-	    xtype: 'proxmoxcheckbox',
-	    fieldLabel: gettext('Delete source'),
-	    name: 'deleteDisk',
-	    uncheckedValue: 0,
-	    checked: false,
-	});
-
-	me.formPanel = Ext.create('Ext.form.Panel', {
-	    bodyPadding: 10,
-	    border: false,
-	    fieldDefaults: {
-		labelWidth: 100,
-		anchor: '100%',
-	    },
-	    items: items,
-	});
-
-	var form = me.formPanel.getForm();
-
-	var submitBtn;
-
-	me.title = qemu ? gettext("Move disk") : gettext('Move Volume');
-	submitBtn = Ext.create('Ext.Button', {
-	    text: me.title,
-	    handler: function() {
-		if (form.isValid()) {
-		    var values = form.getValues();
-		    me.move_disk(me.disk, values.hdstorage, values.diskformat,
-				 values.deleteDisk);
-		}
-	    },
-	});
-
-	Ext.apply(me, {
-	    modal: true,
-	    width: 350,
-	    border: false,
-	    layout: 'fit',
-	    buttons: [submitBtn],
-	    items: [me.formPanel],
-	});
-
-
 	me.callParent();
-
-	me.mon(me.formPanel, 'validitychange', function(fp, isValid) {
-	    submitBtn.setDisabled(!isValid);
-	});
-
-	me.formPanel.isValid();
     },
 });
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 21f67cd0..cf1a67cd 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -410,6 +410,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		disk: rec.data.key,
 		nodename: nodename,
 		vmid: vmid,
+		type: 'qemu',
 	    });
 
 	    win.show();
-- 
2.30.2

Re: [pve-devel] [PATCH v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu

[Fabian Ebner]

Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
> For the new HDReassign component, we follow the approach of HDMove to
> have one componend for qemu and lxc.
> 
> To avoid button clutter, a new "Disk/Volume action" button is
> introduced. It holds the Move, Reassign and Resize buttons in a submenu.
> 

Some small general things I noticed:
* Since re-assign to/from templates is not possible, we could filter
those out in the selection/disable the action (assuming the information
whether it's a template or not is readily available).
* For such drop-down menus we mostly (always?) have icons too.
* The move disk and reassign windows seem to have a bit much padding.

> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since
> 
> v2:
> * switch from generic to Proxmox Edit window
> * add new submenu for disk/volume specific actions
> * code style improvements
> * simplify some labels, removing "disk" and "volume" as the context
>   already gives this away
> 
> 
> v1: incorporated feedback I got off list
> 
> * use more modern approaches
>     * arrow functions
>     * autoShow
>     * template strings
> * reduce predefined cbind values and use arrow functions in the cbind
>   directly in many cases
> * some code style issues and cleanup
> 
>  www/manager6/Makefile             |   1 +
>  www/manager6/lxc/Resources.js     |  62 +++++--
>  www/manager6/qemu/HDReassign.js   | 274 ++++++++++++++++++++++++++++++
>  www/manager6/qemu/HardwareView.js |  60 ++++++-
>  4 files changed, 378 insertions(+), 19 deletions(-)
>  create mode 100644 www/manager6/qemu/HDReassign.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index e6e01bd1..94a78d89 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -214,6 +214,7 @@ JSSRC= 							\
>  	qemu/HDTPM.js					\
>  	qemu/HDMove.js					\
>  	qemu/HDResize.js				\
> +	qemu/HDReassign.js				\

Nit: not ordered alphabetically

>  	qemu/HardwareView.js				\
>  	qemu/IPConfigEdit.js				\
>  	qemu/KeyboardEdit.js				\
> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
> index 15ee3c67..306a4988 100644
> --- a/www/manager6/lxc/Resources.js
> +++ b/www/manager6/lxc/Resources.js
> @@ -151,7 +151,8 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    });
>  	};
>  
> -	var run_move = function(b, e, rec) {
> +	var run_move = function() {
> +	    let rec = me.selModel.getSelection()[0];

Style nit: could switch to let instead of var.

>  	    if (!rec) {
>  		return;
>  	    }
> @@ -168,6 +169,24 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    win.on('destroy', me.reload, me);
>  	};
>  
> +	let run_reassign = function() {
> +	    let rec = me.selModel.getSelection()[0];
> +	    if (!rec) {
> +		return;
> +	    }
> +
> +	    Ext.create('PVE.window.HDReassign', {
> +		disk: rec.data.key,
> +		nodename: nodename,
> +		autoShow: true,
> +		vmid: vmid,
> +		type: 'lxc',
> +		listeners: {
> +		    destroy: () => me.reload(),
> +		},
> +	    });
> +	};
> +
>  	var edit_btn = new Proxmox.button.Button({
>  	    text: gettext('Edit'),
>  	    selModel: me.selModel,
> @@ -182,7 +201,7 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    handler: function() { me.run_editor(); },
>  	});
>  
> -	var resize_btn = new Proxmox.button.Button({
> +	var resize_menuitem = new Ext.menu.Item({

Same here.

>  	    text: gettext('Resize disk'),
>  	    selModel: me.selModel,
>  	    disabled: true,

----8<----

> @@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    }
>  	    edit_btn.setDisabled(noedit);
>  
> -	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
> -	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> -	    move_btn.setDisabled(!isDisk || !diskCap);
> +	    volumeaction_btn.setDisabled(!isDisk);
> +	    reassign_menuitem.setDisabled(isRootFS);
> +
> +	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
> +	    resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> +	    move_menuitem.setDisabled(!isDisk || !diskCap);

Nothing new, but could use '|| isUnusedDisk' too:
cannot move volume 'unused0', only configured volumes can be moved to
another storage

>  	    revert_btn.setDisabled(!pending);
>  
>  	    remove_btn.setText(isUsedDisk ? remove_btn.altText : remove_btn.defaultText);
> @@ -327,8 +370,7 @@ Ext.define('PVE.lxc.RessourceView', {
>  		},
>  		edit_btn,
>  		remove_btn,
> -		resize_btn,
> -		move_btn,
> +		volumeaction_btn,
>  		revert_btn,
>  	    ],
>  	    rows: rows,
> diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
> new file mode 100644
> index 00000000..48bc0b1f
> --- /dev/null
> +++ b/www/manager6/qemu/HDReassign.js
> @@ -0,0 +1,274 @@
> +Ext.define('PVE.window.HDReassign', {
> +    extend: 'Proxmox.window.Edit',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    resizable: false,
> +    modal: true,
> +    width: 350,
> +    border: false,
> +    layout: 'fit',
> +    showReset: false,
> +    showProgress: true,
> +    method: 'POST',
> +
> +    viewModel: {
> +	data: {
> +	    mpType: '',
> +	},
> +	formulas: {
> +	    mpMaxCount: get => get('mpType') === 'mp'
> +		? PVE.Utils.mp_counts.mps - 1
> +		: PVE.Utils.mp_counts.unused - 1,
> +	},
> +    },
> +
> +    cbindData: function() {
> +	let me = this;
> +	return {
> +	    vmid: me.vmid,
> +	    disk: me.disk,
> +	    isQemu: me.type === 'qemu',
> +	    nodename: me.nodename,
> +	    url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
> +	};
> +    },
> +
> +    cbind: {
> +	title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
> +	submitText: get => get('title'),
> +	qemu: '{isQemu}',
> +	url: '{url}',
> +    },
> +
> +    submitUrl: function(url, values) {
> +	url += this.qemu ? 'move_disk' : 'move_volume';

Why not already construct the full URL above?

> +	return url;
> +    },
> +
> +    getValues: function() {
> +	let me = this;
> +	let values = me.formPanel.getForm().getValues();
> +
> +	let params = {
> +	    vmid: me.vmid,
> +	    'target-vmid': values.targetVmid,
> +	};
> +
> +	params[me.qemu ? 'disk' : 'volume'] = me.disk;
> +
> +	if (me.qemu) {
> +	    params['target-disk'] = `${values.controller}${values.deviceid}`;
> +	} else {
> +	    params['target-volume'] = `${values.mpType}${values.mpId}`;
> +	}
> +	return params;
> +    },
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	initViewModel: function(model) {
> +	    let view = this.getView();
> +	    let mpTypeValue = view.disk.match(/^unused\d+/) ? 'unused' : 'mp';
> +	    model.set('mpType', mpTypeValue);
> +	},
> +
> +	onMpTypeChange: function(value) {
> +	    this.getView().getViewModel().set('mpType', value.getValue());
> +	    this.getView().lookup('mpIdSelector').validate();
> +	},
> +
> +	onTargetVMChange: function(f, vmid) {
> +	    let me = this;
> +	    let view = me.getView();
> +	    let diskSelector = view.lookup('diskSelector');
> +	    if (!vmid) {
> +		diskSelector.setVMConfig(null);
> +		me.VMConfig = null;
> +		return;
> +	    }
> +
> +	    let type = view.qemu ? 'qemu' : 'lxc';
> +
> +	    let url = `/nodes/${view.nodename}/${type}/${vmid}/config`;
> +	    Proxmox.Utils.API2Request({
> +		url: url,
> +		method: 'GET',
> +		failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> +		success: function(response, options) {
> +		    if (view.qemu) {
> +			diskSelector.setVMConfig(response.result.data);
> +			diskSelector.setDisabled(false);
> +		    } else {
> +			let mpIdSelector = view.lookup('mpIdSelector');
> +			let mpType = view.lookup('mpType');
> +
> +			view.VMConfig = response.result.data;
> +
> +			PVE.Utils.forEachMP((bus, i) => {

Feels hacky and in fact only works in all cases, because the number of
max unused and max mp is the same. The iterator only iterates over mount
points when called like this and you ignore the bus. Using a new helper
akin to PVE.Utils.nextFreeDisk or simply inlining the required loop
here, would be more robust.

> +			    let name = view.getViewModel().get('mpType') + i.toString();
> +			    if (!Ext.isDefined(view.VMConfig[name])) {
> +				mpIdSelector.setValue(i);
> +				return false;
> +			    }
> +			    return undefined;
> +			});
> +
> +			mpType.setDisabled(false);
> +			mpIdSelector.setDisabled(false);
> +			mpIdSelector.validate();
> +		    }
> +		},
> +	    });
> +	},
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'form',
> +	    reference: 'moveFormPanel',
> +	    bodyPadding: 10,
> +	    border: false,
> +	    fieldDefaults: {
> +		labelWidth: 100,
> +		anchor: '100%',
> +	    },
> +	    items: [
> +		{
> +		    xtype: 'displayfield',
> +		    name: 'sourceDisk',
> +		    fieldLabel: gettext('Source'),
> +		    cbind: {
> +			name: get => get('isQemu') ? 'disk' : 'volume',
> +			value: '{disk}',
> +		    },
> +		    vtype: 'StorageId',

But it's not a storage ID.

> +		    allowBlank: false,
> +		},
> +		{
> +		    xtype: 'vmComboSelector',
> +		    reference: 'targetVMID',
> +		    name: 'targetVmid',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('Target'),
> +		    bind: {
> +			value: '{targetVMID}',
> +		    },
> +		    store: {
> +			model: 'PVEResources',
> +			autoLoad: true,
> +			sorters: 'vmid',
> +			cbind: {}, // for nested cbinds
> +			filters: [
> +			    {
> +				property: 'type',
> +				cbind: {
> +				    value: get => get('isQemu') ? /qemu/ : /lxc/,

Nit: the type can be matched exaclty, or was there a reason for using a
regex?

> +				},
> +			    },
> +			    {
> +				property: 'node',
> +				cbind: {
> +				    value: '{nodename}',
> +				},
> +			    },
> +			    {
> +				property: 'vmid',
> +				operator: '!=',
> +				cbind: {
> +				    value: '{vmid}',
> +				},
> +			    },
> +			],
> +		    },
> +		    listeners: { change: 'onTargetVMChange' },
> +		},
> +		{
> +		    xtype: 'pveControllerSelector',
> +		    reference: 'diskSelector',
> +		    withUnused: true,
> +		    disabled: true,
> +		    cbind: {
> +			hidden: '{!isQemu}',
> +		    },
> +		},
> +		{
> +		    xtype: 'container',
> +		    layout: 'hbox',
> +		    cbind: {
> +			hidden: '{isQemu}',
> +			disabled: '{isQemu}',
> +		    },
> +		    items: [
> +			{
> +			    xtype: 'pmxDisplayEditField',
> +			    cbind: {
> +				editable: get => !get('disk').match(/^unused\d+/),
> +				value: get => get('disk').match(/^unused\d+/) ? 'unused' : 'mp',
> +			    },
> +			    disabled: true,
> +			    name: 'mpType',
> +			    reference: 'mpType',
> +			    fieldLabel: gettext('Add as'),
> +			    submitValue: true,
> +			    flex: 4,
> +			    editConfig: {
> +				xtype: 'proxmoxKVComboBox',
> +				name: 'mpTypeCombo',
> +				reference: 'mpTypeCombo',
> +				deleteEmpty: false,
> +				cbind: {
> +				    hidden: '{isQemu}',
> +				},
> +				comboItems: [
> +				    ['mp', gettext('Mount Point')],
> +				    ['unused', gettext('Unused')],
> +				],
> +				listeners: { change: 'onMpTypeChange' },
> +			    },
> +			},
> +			{
> +			    xtype: 'proxmoxintegerfield',
> +			    name: 'mpId',
> +			    reference: 'mpIdSelector',
> +			    minValue: 0,
> +			    flex: 1,
> +			    allowBlank: false,
> +			    validateOnChange: true,
> +			    disabled: true,
> +			    bind: {
> +				maxValue: '{mpMaxCount}',
> +			    },
> +			    validator: function(value) {
> +				let view = this.up('window');
> +				let type = view.getViewModel().get('mpType');
> +				if (Ext.isDefined(view.VMConfig[type+value])) {

Style nit: could also use `${type}${value}`

> +				    return "Mount point is already in use.";
> +				}
> +				return true;
> +			    },
> +			},
> +		    ],
> +		},
> +	    ],
> +	},
> +    ],
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +
> +	if (!me.vmid) {
> +	    throw "no VM ID specified";
> +	}
> +
> +	if (!me.type) {
> +	    throw "no type specified";
> +	}
> +
> +	me.callParent();
> +    },
> +});

----8<----

Re: [pve-devel] [PATCH v3 manager 4/4] ui: hdmove: modernize/refactor

[Fabian Ebner]

Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since
> 
> v2:
> * switch from generic window to proxmox edit
> 
> v1: much of the feedback to the HDReassign.js from the
> first patch has been incorporated here as well.
> 
> * reducing predefined cbind values for more arrow functions
> * using more arrow functions in general
> * template strings
>  www/manager6/qemu/HDMove.js       | 192 ++++++++++++++----------------
>  www/manager6/qemu/HardwareView.js |   1 +
>  2 files changed, 88 insertions(+), 105 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
> index 181b7bdc..3e94479c 100644
> --- a/www/manager6/qemu/HDMove.js
> +++ b/www/manager6/qemu/HDMove.js
> @@ -1,48 +1,102 @@
>  Ext.define('PVE.window.HDMove', {
> -    extend: 'Ext.window.Window',
> +    extend: 'Proxmox.window.Edit',
> +    mixins: ['Proxmox.Mixin.CBind'],
>  
>      resizable: false,
> +    modal: true,
> +    width: 350,
> +    border: false,
> +    layout: 'fit',
> +    showReset: false,
> +    showProgress: true,
> +    method: 'POST',
> +
> +    cbindData: function() {
> +	let me = this;
> +	return {
> +	    disk: me.disk,
> +	    isQemu: me.type === 'qemu',
> +	    nodename: me.nodename,
> +	    url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
> +	};
> +    },
> +
> +    cbind: {
> +	title: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
> +	submitText: get => get('title'),
> +	qemu: '{isQemu}',
> +	url: '{url}',
> +    },
> +
> +    submitUrl: function(url, values) {
> +	url += this.qemu ? 'move_disk' : 'move_volume';

Same as in patch #1: url could be constructed above in one go.

> +	return url;
> +    },
>  
> +    getValues: function() {
> +	let me = this;
> +	let values = me.formPanel.getForm().getValues();
>  
> -    move_disk: function(disk, storage, format, delete_disk) {
> -	var me = this;
> -	var qemu = me.type === 'qemu';
> -	var params = {};
> -	params.storage = storage;
> -	params[qemu ? 'disk':'volume'] = disk;
> +	let params = {
> +	    storage: values.hdstorage,
> +	};
> +	params[me.qemu ? 'disk':'volume'] = me.disk;

Style nit: missing spaces around the colon

>  
> -	if (format && qemu) {
> -	    params.format = format;
> +	if (values.diskformat && me.qemu) {
> +	    params.format = values.diskformat;
>  	}
>  
> -	if (delete_disk) {
> +	if (values.deleteDisk) {
>  	    params.delete = 1;
>  	}
> -
> -	var url = '/nodes/' + me.nodename + '/' + me.type + '/' + me.vmid + '/';
> -	url += qemu ? 'move_disk' : 'move_volume';
> -
> -	Proxmox.Utils.API2Request({
> -	    params: params,
> -	    url: url,
> -	    waitMsgTarget: me,
> -	    method: 'POST',
> -	    failure: function(response, opts) {
> -		Ext.Msg.alert('Error', response.htmlStatus);
> -	    },
> -	    success: function(response, options) {
> -		var upid = response.result.data;
> -		var win = Ext.create('Proxmox.window.TaskViewer', {
> -		    upid: upid,
> -		});
> -		win.show();
> -		win.on('destroy', function() { me.close(); });
> -	    },
> -	});
> +	return params;
>      },

Style nit: missing blank line

> +    items: [
> +	{
> +	    xtype: 'form',
> +	    reference: 'moveFormPanel',
> +	    bodyPadding: 10,
> +	    border: false,
> +	    fieldDefaults: {
> +		labelWidth: 100,
> +		anchor: '100%',
> +	    },
> +	    items: [
> +		{
> +		    xtype: 'displayfield',
> +		    cbind: {
> +			name: get => get('isQemu') ? 'disk' : 'volume',
> +			fieldLabel: get => get('isQemu')
> +			    ? gettext('Disk')
> +			    : gettext('Mount Point'),

Style nit: (exactly) fits in one line

> +			value: '{disk}',
> +		    },
> +		    vtype: 'StorageId',

Not a storage ID either, but here it's pre-existing. There is a
'ConfigId' vtype, but that's also not entirely correct, since this is a
config key. The field is not editable anyways, so I think we can just
drop it.

> +		    allowBlank: false,
> +		},

Re: [pve-devel] [PATCH v3 manager 3/4] ui: BusTypeSelector: change noVirtIO to withVirtIO

[Fabian Ebner]

Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
> Double negated properties make it harder than necessary to parse
> conditions.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  www/manager6/form/BusTypeSelector.js    | 4 ++--
>  www/manager6/form/ControllerSelector.js | 4 ++--
>  www/manager6/qemu/CDEdit.js             | 2 +-
>  www/manager6/qemu/CIDriveEdit.js        | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

Re: [pve-devel] [PATCH v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu

[Aaron Lauterer]

On 3/10/22 11:49, Fabian Ebner wrote:
> Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
>> For the new HDReassign component, we follow the approach of HDMove to
>> have one componend for qemu and lxc.
>>
>> To avoid button clutter, a new "Disk/Volume action" button is
>> introduced. It holds the Move, Reassign and Resize buttons in a submenu.
>>
> 
> Some small general things I noticed:
> * Since re-assign to/from templates is not possible, we could filter
> those out in the selection/disable the action (assuming the information
> whether it's a template or not is readily available).

good point, definitely doable

> * For such drop-down menus we mostly (always?) have icons too.

Yes, but since the existing buttons did not have any icons and finding matching and not yet (too widely) used icons in font-awesome is getting tricky, I opted for a menu without icons.
I'd rather have no icons instead of poorly fitting ones. But definitely something that can be discussed and changed quite easily.


> * The move disk and reassign windows seem to have a bit much padding.

thanks for catching that! I'll investigate where that happens.

[...]

>> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
>> index e6e01bd1..94a78d89 100644
>> --- a/www/manager6/Makefile
>> +++ b/www/manager6/Makefile
>> @@ -214,6 +214,7 @@ JSSRC= 							\
>>   	qemu/HDTPM.js					\
>>   	qemu/HDMove.js					\
>>   	qemu/HDResize.js				\
>> +	qemu/HDReassign.js				\
> 
> Nit: not ordered alphabetically

yep, will fix it

> 
>>   	qemu/HardwareView.js				\
>>   	qemu/IPConfigEdit.js				\
>>   	qemu/KeyboardEdit.js				\
>> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
>> index 15ee3c67..306a4988 100644
>> --- a/www/manager6/lxc/Resources.js
>> +++ b/www/manager6/lxc/Resources.js
>> @@ -151,7 +151,8 @@ Ext.define('PVE.lxc.RessourceView', {
>>   	    });
>>   	};
>>   
>> -	var run_move = function(b, e, rec) {
>> +	var run_move = function() {
>> +	    let rec = me.selModel.getSelection()[0];
> 
> Style nit: could switch to let instead of var.

yeah, I tried to keep it similar to the surrounding, but we could definitely switch it over to let

> 
[...]
>>   
>> -	var resize_btn = new Proxmox.button.Button({
>> +	var resize_menuitem = new Ext.menu.Item({
> 
> Same here.
> 
>>   	    text: gettext('Resize disk'),
>>   	    selModel: me.selModel,
>>   	    disabled: true,
> 
> ----8<----
> 
>> @@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', {
>>   	    }
>>   	    edit_btn.setDisabled(noedit);
>>   
>> -	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
>> -	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
>> -	    move_btn.setDisabled(!isDisk || !diskCap);
>> +	    volumeaction_btn.setDisabled(!isDisk);
>> +	    reassign_menuitem.setDisabled(isRootFS);
>> +
>> +	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
>> +	    resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk);
>> +	    move_menuitem.setDisabled(!isDisk || !diskCap);
> 
> Nothing new, but could use '|| isUnusedDisk' too:
> cannot move volume 'unused0', only configured volumes can be moved to
> another storage

I kept it as is because, and maybe that was also considered originally, getting an error message at some point, explaining why it does not work, helps more than having the button disabled without knowing why.

[...]
>> +    cbindData: function() {
>> +	let me = this;
>> +	return {
>> +	    vmid: me.vmid,
>> +	    disk: me.disk,
>> +	    isQemu: me.type === 'qemu',
>> +	    nodename: me.nodename,
>> +	    url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
>> +	};
>> +    },
>> +
>> +    cbind: {
>> +	title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
>> +	submitText: get => get('title'),
>> +	qemu: '{isQemu}',
>> +	url: '{url}',
>> +    },
>> +
>> +    submitUrl: function(url, values) {
>> +	url += this.qemu ? 'move_disk' : 'move_volume';
> 
> Why not already construct the full URL above?

I'll see how to do that. First tests show that if I construct the submitUrl directly, I need to set the 'url' at some point, even if not used because the edit window does check its existence.

[...]

>> +	onTargetVMChange: function(f, vmid) {
>> +	    let me = this;
>> +	    let view = me.getView();
>> +	    let diskSelector = view.lookup('diskSelector');
>> +	    if (!vmid) {
>> +		diskSelector.setVMConfig(null);
>> +		me.VMConfig = null;
>> +		return;
>> +	    }
>> +
>> +	    let type = view.qemu ? 'qemu' : 'lxc';
>> +
>> +	    let url = `/nodes/${view.nodename}/${type}/${vmid}/config`;
>> +	    Proxmox.Utils.API2Request({
>> +		url: url,
>> +		method: 'GET',
>> +		failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
>> +		success: function(response, options) {
>> +		    if (view.qemu) {
>> +			diskSelector.setVMConfig(response.result.data);
>> +			diskSelector.setDisabled(false);
>> +		    } else {
>> +			let mpIdSelector = view.lookup('mpIdSelector');
>> +			let mpType = view.lookup('mpType');
>> +
>> +			view.VMConfig = response.result.data;
>> +
>> +			PVE.Utils.forEachMP((bus, i) => {
> 
> Feels hacky and in fact only works in all cases, because the number of
> max unused and max mp is the same. The iterator only iterates over mount
> points when called like this and you ignore the bus. Using a new helper
> akin to PVE.Utils.nextFreeDisk or simply inlining the required loop
> here, would be more robust.

Yep, going for a PVE.Utils.nextFreeMP implementation

> 
>> +			    let name = view.getViewModel().get('mpType') + i.toString();
>> +			    if (!Ext.isDefined(view.VMConfig[name])) {
>> +				mpIdSelector.setValue(i);
>> +				return false;
>> +			    }
>> +			    return undefined;
>> +			});
>> +
>> +			mpType.setDisabled(false);
>> +			mpIdSelector.setDisabled(false);
>> +			mpIdSelector.validate();
>> +		    }
>> +		},
>> +	    });
>> +	},
>> +    },
>> +
>> +    items: [
>> +	{
>> +	    xtype: 'form',
>> +	    reference: 'moveFormPanel',
>> +	    bodyPadding: 10,
>> +	    border: false,
>> +	    fieldDefaults: {
>> +		labelWidth: 100,
>> +		anchor: '100%',
>> +	    },
>> +	    items: [
>> +		{
>> +		    xtype: 'displayfield',
>> +		    name: 'sourceDisk',
>> +		    fieldLabel: gettext('Source'),
>> +		    cbind: {
>> +			name: get => get('isQemu') ? 'disk' : 'volume',
>> +			value: '{disk}',
>> +		    },
>> +		    vtype: 'StorageId',
> 
> But it's not a storage ID.

yeah, bad copy pasta and AFAICT not even needed here

> 
>> +		    allowBlank: false,
>> +		},
>> +		{
>> +		    xtype: 'vmComboSelector',
>> +		    reference: 'targetVMID',
>> +		    name: 'targetVmid',
>> +		    allowBlank: false,
>> +		    fieldLabel: gettext('Target'),
>> +		    bind: {
>> +			value: '{targetVMID}',
>> +		    },
>> +		    store: {
>> +			model: 'PVEResources',
>> +			autoLoad: true,
>> +			sorters: 'vmid',
>> +			cbind: {}, // for nested cbinds
>> +			filters: [
>> +			    {
>> +				property: 'type',
>> +				cbind: {
>> +				    value: get => get('isQemu') ? /qemu/ : /lxc/,
> 
> Nit: the type can be matched exaclty, or was there a reason for using a
> regex?

IIRC not really, will change it

[...]

>> +			{
>> +			    xtype: 'proxmoxintegerfield',
>> +			    name: 'mpId',
>> +			    reference: 'mpIdSelector',
>> +			    minValue: 0,
>> +			    flex: 1,
>> +			    allowBlank: false,
>> +			    validateOnChange: true,
>> +			    disabled: true,
>> +			    bind: {
>> +				maxValue: '{mpMaxCount}',
>> +			    },
>> +			    validator: function(value) {
>> +				let view = this.up('window');
>> +				let type = view.getViewModel().get('mpType');
>> +				if (Ext.isDefined(view.VMConfig[type+value])) {
> 
> Style nit: could also use `${type}${value}`

good point

Re: [pve-devel] [PATCH v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu

[Fabian Ebner]

Am 11.03.22 um 15:38 schrieb Aaron Lauterer:
> On 3/10/22 11:49, Fabian Ebner wrote:
>> Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
>>> +    cbindData: function() {
>>> +    let me = this;
>>> +    return {
>>> +        vmid: me.vmid,
>>> +        disk: me.disk,
>>> +        isQemu: me.type === 'qemu',
>>> +        nodename: me.nodename,
>>> +        url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
>>> +    };
>>> +    },
>>> +
>>> +    cbind: {
>>> +    title: get => get('isQemu') ? gettext('Reassign disk') :
>>> gettext('Reassign volume'),
>>> +    submitText: get => get('title'),
>>> +    qemu: '{isQemu}',
>>> +    url: '{url}',
>>> +    },
>>> +
>>> +    submitUrl: function(url, values) {
>>> +    url += this.qemu ? 'move_disk' : 'move_volume';
>>
>> Why not already construct the full URL above?
> 
> I'll see how to do that. First tests show that if I construct the
> submitUrl directly, I need to set the 'url' at some point, even if not
> used because the edit window does check its existence.
> 

Sorry, by "above" I meant in the cbindData function, not directly above.

Re: [pve-devel] [PATCH v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu

[Aaron Lauterer]

On 3/14/22 09:18, Fabian Ebner wrote:
> Am 11.03.22 um 15:38 schrieb Aaron Lauterer:
>> On 3/10/22 11:49, Fabian Ebner wrote:
>>> Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
>>>> +    cbindData: function() {
>>>> +    let me = this;
>>>> +    return {
>>>> +        vmid: me.vmid,
>>>> +        disk: me.disk,
>>>> +        isQemu: me.type === 'qemu',
>>>> +        nodename: me.nodename,
>>>> +        url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
>>>> +    };
>>>> +    },
>>>> +
>>>> +    cbind: {
>>>> +    title: get => get('isQemu') ? gettext('Reassign disk') :
>>>> gettext('Reassign volume'),
>>>> +    submitText: get => get('title'),
>>>> +    qemu: '{isQemu}',
>>>> +    url: '{url}',
>>>> +    },
>>>> +
>>>> +    submitUrl: function(url, values) {
>>>> +    url += this.qemu ? 'move_disk' : 'move_volume';
>>>
>>> Why not already construct the full URL above?
>>
>> I'll see how to do that. First tests show that if I construct the
>> submitUrl directly, I need to set the 'url' at some point, even if not
>> used because the edit window does check its existence.
>>
> 
> Sorry, by "above" I meant in the cbindData function, not directly above.

Yeah. In the meantime I found out, that I can also just use the `url` as it will be used if there is no `submitUrl`.