[pve-devel] [PATCH manager 1/8] add new DiskStorageSelector.js

[Thomas Lamprecht]

On 11/08/2017 10:53 AM, Dominik Csapak wrote:
> this is a wrapper for selecting a storage/disk image
> 
> it is a simple container with 4 form fields inside
> which can be reused, whenever we need to select a storage for a disk,
> image etc.
> 
> we have code similar to this four times already
> (qemu image creation, lxc mp creation, qemu cloning, qemu efidisk
> creation)
> 
> so it was time to refactor this and use the storage information from the
> backend instead of hardcoding values in the frontend
> 
> for this we need to pass the format=1 parameter to the storage api call,
> and to not load the fileselector when it is initially disabled
> 

looks , in general, good to me. two comments inline.

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  www/manager6/Makefile                    |   1 +
>  www/manager6/form/DiskStorageSelector.js | 116 +++++++++++++++++++++++++++++++
>  www/manager6/form/FileSelector.js        |   4 +-
>  www/manager6/form/StorageSelector.js     |   4 +-
>  4 files changed, 123 insertions(+), 2 deletions(-)
>  create mode 100644 www/manager6/form/DiskStorageSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index d39bb49c..52d2f117 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -52,6 +52,7 @@ JSSRC= 				                 	\
>  	form/NodeSelector.js				\
>  	form/FileSelector.js				\
>  	form/StorageSelector.js				\
> +	form/DiskStorageSelector.js			\
>  	form/BridgeSelector.js				\
>  	form/SecurityGroupSelector.js			\
>  	form/IPRefSelector.js				\
> diff --git a/www/manager6/form/DiskStorageSelector.js b/www/manager6/form/DiskStorageSelector.js
> new file mode 100644
> index 00000000..efb956cd
> --- /dev/null
> +++ b/www/manager6/form/DiskStorageSelector.js
> @@ -0,0 +1,116 @@
> +Ext.define('PVE.form.DiskStorageSelector', {
> +    extend: 'Ext.container.Container',
> +    alias: 'widget.pveDiskStorageSelector',
> +
> +    layout: 'fit',
> +    defaults: {
> +	margin: '0 0 5 0'
> +    },
> +
> +    changeStorage: function(f, value) {
> +	var me = this;
> +	var formatsel = me.getComponent('diskformat');
> +	var hdfilesel = me.getComponent('hdimage');
> +	var hdsizesel = me.getComponent('disksize');
> +
> +	// initial store load, and reset/deletion of the storage
> +	if (!value) {
> +	    hdfilesel.setDisabled(true);
> +	    hdfilesel.setVisible(false);
> +
> +	    formatsel.setDisabled(true);
> +	    return;
> +	}
> +
> +	var rec = f.store.getById(value);
> +	// if the storage is not defined, or valid,
> +	// we cannot know what to enable/disable
> +	if (!rec) {
> +	    return;
> +	}
> +
> +	var selectformat = false;
> +	if (rec.data.format) {
> +	    var format = rec.data.format[0]; // 0 is the formats, 1 the default in the backend
> +	    delete format.subvol; // we never need subvol in the gui
> +	    selectformat = (Ext.Object.getSize(format) > 1);
> +	}
> +
> +	var select = !!rec.data.select && !me.hideSelection;
> +
> +	formatsel.setDisabled(!selectformat);
> +	formatsel.setValue(selectformat ? 'qcow2' : 'raw');
> +
> +	hdfilesel.setDisabled(!select);
> +	hdfilesel.setVisible(select);
> +	if (select) {
> +	    hdfilesel.setStorage(value);
> +	}
> +
> +	hdsizesel.setDisabled(select || me.hideSize);
> +	hdsizesel.setVisible(!select && !me.hideSize);
> +    },
> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	me.items = [
> +	    {
> +		xtype: 'pveStorageSelector',
> +		itemId: 'hdstorage',
> +		name: 'hdstorage',
> +		reference: 'hdstorage',
> +		fieldLabel: me.storageLabel || gettext('Storage'),
> +		nodename: me.nodename,
> +		storageContent: me.storageContent,
> +		autoSelect: me.autoSelect,
> +		allowBlank: me.allowBlank,
> +		emptyText: me.emptyText,
> +		listeners: {
> +		    change: {
> +			fn: me.changeStorage,
> +			scope: me
> +		    }
> +		}
> +	    },
> +	    {
> +		xtype: 'pveFileSelector',
> +		name: 'hdimage',
> +		reference: 'hdimage',
> +		itemId: 'hdimage',
> +		fieldLabel: gettext('Disk image'),
> +		nodename: me.nodename,
> +		disabled: me.hideSelection || true,
> +		hidden: me.hideSelection || true

please add hideSelection as class config property above and set
the default there, else a class user hardly sees that this is not
only a internal state but actual configuration.

Also fieldLabel, autoSelect, allowBlank, ... from other entries, with
their respective defaults

> +	    },
> +	    {
> +		xtype: 'numberfield',
> +		itemId: 'disksize',
> +		reference: 'disksize',
> +		name: 'disksize',
> +		fieldLabel: gettext('Disk size') + ' (GB)',
> +		hidden: me.hideSize,
> +		disabled: me.hideSize,
> +		minValue: 0.001,
> +		maxValue: 128*1024,
> +		decimalPrecision: 3,
> +		value: '32',

off topic: maybe it would make sense to use '32.0' here, so an user
sees that floating - and thus sub gigabyte resolution - could be used here.

> +		allowBlank: false
> +	    },
> +	    {
> +		xtype: 'pveDiskFormatSelector',
> +		itemId: 'diskformat',
> +		reference: 'diskformat',
> +		name: 'diskformat',
> +		fieldLabel: gettext('Format'),
> +		nodename: me.nodename,
> +		disabled: true,
> +		hidden: me.storageContent === 'rootdir',
> +		value: 'qcow2',
> +		allowBlank: false
> +	    }
> +	];
> +
> +	me.callParent();
> +    }
> +});
> diff --git a/www/manager6/form/FileSelector.js b/www/manager6/form/FileSelector.js
> index 87a80a23..aaf37d5c 100644
> --- a/www/manager6/form/FileSelector.js
> +++ b/www/manager6/form/FileSelector.js
> @@ -76,6 +76,8 @@ Ext.define('PVE.form.FileSelector', {
>  
>          me.callParent();
>  
> -	me.setStorage(me.storage, me.nodename);
> +	if (!me.disabled) {
> +	    me.setStorage(me.storage, me.nodename);
> +	}
>      }
>  });
> diff --git a/www/manager6/form/StorageSelector.js b/www/manager6/form/StorageSelector.js
> index 78cd8b3d..184869d1 100644
> --- a/www/manager6/form/StorageSelector.js
> +++ b/www/manager6/form/StorageSelector.js
> @@ -39,7 +39,9 @@ Ext.define('PVE.form.StorageSelector', {
>  	    return;
>  	}
>  
> -	var params = {};
> +	var params = {
> +	    format: 1
> +	};
>  	var url = '/api2/json/nodes/' + me.nodename + '/storage';
>  	if (me.storageContent) {
>  	    params.content = me.storageContent;
>