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

[Fabian Ebner <f.ebner@proxmox.com>]

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,
> +		},