[pve-devel] [PATCH manager 1/1] Add 'type' option to AgentFeatureSelector

[Thomas Lamprecht]

On 11/18/19 7:46 AM, Matt Dunwoodie wrote:
> This adds an extra field to the AgentFeatureSelector that reflects the
> change in qemu-server.
> 

thanks again for your contribution, a few comments inline.

> Signed-off-by: Matt Dunwoodie <ncon at noconroy.net>
> ---
>  www/manager6/Utils.js                     | 16 +++++++++++++---
>  www/manager6/form/AgentFeatureSelector.js | 14 ++++++++++++--
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 47f9d297..6c4f4bed 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -276,10 +276,20 @@ Ext.define('PVE.Utils', { utilities: {
>  	    var keystring = '' ;
>  	    agentstring += ', ' + key + ': ';
>  
> -	    if (PVE.Parser.parseBoolean(value)) {
> -		agentstring += Proxmox.Utils.enabledText;
> +	    if (key === 'type') {
> +		if (value === 'virtio') {
> +			agentstring += 'Virtio';
> +		} else if (value === 'isa') {
> +			agentstring += 'ISA';
> +		} else {
> +			agentstring += 'Unknown';
> +		}

hmm, I'd prefer a map here, something like:

let map = {
    virtio: "VirtIO",
    isa: "ISA",
};
agentstring += map[value] || Proxmox.Utils.unknownText;

(note also the gettext'd translated Proxmox.Utils.unknownText)

>  	    } else {
> -		agentstring += Proxmox.Utils.disabledText;
> +		if (PVE.Parser.parseBoolean(value)) {
> +		    agentstring += Proxmox.Utils.enabledText;
> +		} else {
> +		    agentstring += Proxmox.Utils.disabledText;
> +		}
>  	    }
>  	});
>  
> diff --git a/www/manager6/form/AgentFeatureSelector.js b/www/manager6/form/AgentFeatureSelector.js
> index 44ef2e57..794f179c 100644
> --- a/www/manager6/form/AgentFeatureSelector.js
> +++ b/www/manager6/form/AgentFeatureSelector.js
> @@ -7,20 +7,30 @@ Ext.define('PVE.form.AgentFeatureSelector', {
>      items: [
>  	{
>  	    xtype: 'proxmoxcheckbox',
> -	    boxLabel: Ext.String.format(gettext('Use {0}'), 'QEMU Guest Agent'),
> +	    fieldLabel: Ext.String.format(gettext('Use {0}'), 'QEMU Guest Agent'),
>  	    name: 'enabled',
>  	    reference: 'enabled',
>  	    uncheckedValue: 0,
>  	},
>  	{
>  	    xtype: 'proxmoxcheckbox',
> -	    boxLabel: gettext('Run guest-trim after clone disk'),
> +	    fieldLabel: gettext('Run guest-trim after clone disk'),
>  	    name: 'fstrim_cloned_disks',
>  	    bind: {
>  		disabled: '{!enabled.checked}',
>  	    },
>  	    disabled: true
>  	},

I'd rather keep those as boxLabel, they were made as such due to they
lengthier text, it looks a bit crammed as fieldLabel.


> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    name: 'type',
> +	    value: 'virtio',

if you do:
value: '__default__',
deleteEmpty: false,

and...

> +	    fieldLabel: 'Type',
> +	    comboItems: [

..here add:
             ['__default__', Proxmox.Utils.defaultText + " (VirtIO)"],

we would get the semantics we have in a lot of places, and avoid setting
the default in the configuration (saves space)

> +		['virtio', 'virtio'],
> +		['isa', 'isa'],
> +	    ],
> +	},

as this is a bit of a special case, I'd probably move it from "items" to
an "advancedItems" definition.

>  	{
>  	    xtype: 'displayfield',
>  	    userCls: 'pmx-hint',
>