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

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

some comments inline

On 11/15/21 16:02, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> changes since 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       | 239 +++++++++++++++++-------------
>   www/manager6/qemu/HardwareView.js |   1 +
>   2 files changed, 134 insertions(+), 106 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
> index 181b7bdc..210e6ce8 100644
> --- a/www/manager6/qemu/HDMove.js
> +++ b/www/manager6/qemu/HDMove.js
> @@ -1,48 +1,147 @@
>   Ext.define('PVE.window.HDMove', {
>       extend: 'Ext.window.Window',
> +    mixins: ['Proxmox.Mixin.CBind'],
>   
>       resizable: false,
> +    modal: true,
> +    width: 350,
> +    border: false,
> +    layout: 'fit',
> +
> +    cbindData: function() {
> +	let me = this;
> +	return {
> +	    isQemu: me.type === 'qemu',
> +	    disk: me.disk,
> +	    nodename: me.nodename,
> +	};
> +    },
>   
> +    cbind: {
> +	title: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
> +    },
>   
> -    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;
> -
> -	if (format && qemu) {
> -	    params.format = format;
> -	}
> -
> -	if (delete_disk) {
> -	    params.delete = 1;
> -	}
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	move_disk: function(disk, storage, format, delete_disk) {

as with the other patch, this should probably be 'moveDisk'

> +	    let me = this;
> +	    let view = me.getView();
> +	    let qemu = view.type === 'qemu';
> +	    let params = {
> +		storage: storage,
> +	    };
> +	    params[qemu ? 'disk':'volume'] = disk;
> +
> +	    if (format && qemu) {
> +		params.format = format;
> +	    }
> +
> +	    if (delete_disk) {
> +		params.delete = 1;
> +	    }
> +
> +	    let url = `/nodes/${view.nodename}/${view.type}/${view.vmid}/`;
> +	    url += qemu ? 'move_disk' : 'move_volume';
> +
> +	    Proxmox.Utils.API2Request({
> +		params: params,
> +		url: url,
> +		waitMsgTarget: me.getView(),
> +		method: 'POST',
> +		failure: function(response, opts) {
> +		    Ext.Msg.alert('Error', response.htmlStatus);
> +		},
> +		success: function(response, options) {
> +		    let upid = response.result.data;
> +		    Ext.create('Proxmox.window.TaskViewer', {
> +			upid: upid,
> +			autoShow: true,
> +			listeners: {
> +			    destroy: () => view.close(),
> +			},
> +		    });
> +		},
> +	    });
> +	},
> +
> +	onMoveClick: function() {
> +		let me = this;
> +		let view = me.getView();
> +		let form = me.lookup('moveFormPanel').getForm();
> +		if (form.isValid()) {
> +		    let values = form.getValues();
> +		    me.move_disk(view.disk, values.hdstorage, values.diskformat,
> +				 values.deleteDisk);
> +		}
> +	},
>   
> -	var url = '/nodes/' + me.nodename + '/' + me.type + '/' + me.vmid + '/';
> -	url += qemu ? 'move_disk' : 'move_volume';
> +	validateForm: function(fp, isValid) {

imho the name is confusing, since it does not actually
validate the form, but is called after the form was
(succesfully or not) validated

> +	    this.getView().lookup('submitButton').setDisabled(!isValid);
> +	},
> +    },
>   
> -	Proxmox.Utils.API2Request({
> -	    params: params,
> -	    url: url,
> -	    waitMsgTarget: me,
> -	    method: 'POST',
> -	    failure: function(response, opts) {
> -		Ext.Msg.alert('Error', response.htmlStatus);
> +    buttons: [
> +	{
> +	    xtype: 'button',
> +	    reference: 'submitButton',
> +	    cbind: {
> +		text: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
>   	    },
> -	    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(); });
> +	    handler: 'onMoveClick',
> +	    disabled: true,
> +	},
> +    ],
> +
> +    items: [
> +	{
> +	    xtype: 'form',
> +	    reference: 'moveFormPanel',
> +	    bodyPadding: 10,
> +	    border: false,
> +	    fieldDefaults: {
> +		labelWidth: 100,
> +		anchor: '100%',
>   	    },
> -	});
> -    },
> +	    listeners: {
> +		validitychange: 'validateForm',
> +	    },
> +	    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 +152,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 b76814e8..54946651 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -415,6 +415,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		disk: rec.data.key,
>   		nodename: nodename,
>   		vmid: vmid,
> +		type: 'qemu',
>   	    });
>   
>   	    win.show();