[pve-devel] [PATCH manager 1/1] ui/qemu: Extend Qemu Guest agent

[Thomas Lamprecht]

On 7/5/18 7:42 PM, Stoiko Ivanov wrote:
> Change to Qemu Guest Agent now being a property_string, and including
> fstrim_cloned_disks.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>  www/manager6/Makefile                     |  1 +
>  www/manager6/Utils.js                     | 10 +++++
>  www/manager6/form/AgentFeatureSelector.js | 65 +++++++++++++++++++++++++++++++
>  www/manager6/qemu/Options.js              | 10 ++---
>  4 files changed, 80 insertions(+), 6 deletions(-)
>  create mode 100644 www/manager6/form/AgentFeatureSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 3cc6990f..28d21fa7 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -49,6 +49,7 @@ JSSRC= 				                 	\
>  	form/SnapshotSelector.js			\
>  	form/ContentTypeSelector.js			\
>  	form/HotplugFeatureSelector.js			\
> +	form/AgentFeatureSelector.js			\
>  	form/iScsiProviderSelector.js			\
>  	form/DayOfWeekSelector.js			\
>  	form/BackupModeSelector.js			\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index ad5a0a61..58f92788 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -175,6 +175,16 @@ Ext.define('PVE.Utils', { utilities: {
>  	return fa.join(', ');
>      },
>  
> +    render_qga_features: function(value) {
> +	if (!value) {
> +	    return Proxmox.Utils.defaultText + ' (' + gettext('Disabled')  + ')';
> +	} else if (value === '0') {
> +	    return gettext('Disabled');
> +	} else {
> +	    return value;
> +	}
> +    },
> +
>      render_qemu_bios: function(value) {
>  	if (!value) {
>  	    return Proxmox.Utils.defaultText + ' (SeaBIOS)';
> diff --git a/www/manager6/form/AgentFeatureSelector.js b/www/manager6/form/AgentFeatureSelector.js
> new file mode 100644
> index 00000000..cd74721b
> --- /dev/null
> +++ b/www/manager6/form/AgentFeatureSelector.js
> @@ -0,0 +1,65 @@
> +Ext.define('PVE.form.AgentFeatureSelector', {
> +    extend: 'Ext.form.CheckboxGroup',
> +    alias: 'widget.pveAgentFeatureSelector',
> +
> +    columns: 1,
> +    vertical: true,
> +    items: [
> +	{ boxLabel: gettext('Enable Qemu Agent'),    name: 'agent', inputValue: 'enabled',   submitValue: false },
> +	{ boxLabel: gettext('Run guest-trim after clone disk'),    name: 'agent', inputValue: 'fstrim_cloned_disks',   submitValue: false }

Please, no, don't! Sorry, but this looks horrible...
And it makes review hard. 

I'd normally say that there's not a single place in our JS codebase doing
this, but I just openend the Hotplug Option Editor (oh dear...) and know now where
you got that idea... I refactored it using ExtJS 'defaults' feature to group common
configs together and formatted it a bit more sanely.

Let's take a look at yours sanely formatted: 
    items: [
        {
            boxLabel: gettext('Enable Qemu Agent'),

* We try to avoid "Enable" in boolean checkboxes,
just more text (to read an translate) but no value gained.

            name: 'agent',
            inputValue: 'enabled',
            submitValue: false

name and agent could be moved into a defaults section.

        },
        {
            boxLabel: gettext('Run guest-trim after clone disk'),
            name: 'agent',
            inputValue: 'fstrim_cloned_disks',
            submitValue: false
        }
    ],


But how about just building upon a our InputPanel and do something like:

Ext.define('PVE.form.AgentFeatureSelector', {
    extend: 'Proxmox.panel.InputPanel',
    alias: 'widget.pveAgentFeatureSelector',

    column1: [
        {
            xtype: 'proxmoxcheckbox',
            boxLabel: gettext('Qemu Agent'),
            name: 'enabled'
        },
        {
            xtype: 'proxmoxcheckbox',
            boxLabel: gettext('Run guest-trim after clone disk'),
            name: 'fstrim_cloned_disks'
        }
    ],

    onGetValues: function(values) {
        var me = this;

        var agentstr = '';
        Ext.Object.each(values, function(key, value) {
            if (!Ext.isDefined(value)) {
                return; // continue
            }
            if (agentstr === '') {
                agentstr += key + '=' + value;
            } else {
                agentstr += ',' + key + '=' + value;
            }
        });

        return { agent: agentstr };
    },

    setValues: function(values) {
        var agent = values.agent || '';

        var res = {};

        if (agent) {
            Ext.Array.each(agent.split(','), function(p) {
                var kv = p.split('=');
                res[kv[0]] = kv[1];
            });
        }

        this.callParent([res]);
    }
});

This is a quick hack, but testings seems good.
What I additionally would like is to disable all following checkboxes
if the agent enable one is disabled (you cannot submit anyway).

Further the split and assemble formatsring code could be put in a general helper,
this isn't the first time I see this...

> +    ],
> +
> +    setValue: function(value) {
> +	var me = this;
> +
> +	var newVal = [];
> +
> +	var errors = false;
> +	Ext.Array.forEach(value.split(','), function(val){
> +	    if (val === '1') {
> +		newVal.push('enabled');
> +		return;
> +	    }
> +
> +	    var match_res = val.match(/^([a-z_]+)=(\S+)$/);
> +	    if (!match_res) {
> +		newVal = [];
> +		return false;
> +	    }
> +
> +	    var k = match_res[1];
> +	    if (match_res[2] == 1) {
> +		newVal.push(k);
> +	    }
> +	});
> +
> +	if (errors) {
> +	    newVal = [];
> +	}
> +	me.callParent([{ agent: newVal }]);
> +    },
> +
> +    // overide framework function to
> +    // assemble the qemu guest agent value

this got me wondering if you copied it from somewhere, as I recognized Dominiks'
comment style (early newlines, not using the 80cc) ^^

> +    getSubmitData: function() {
> +	var me = this,
> +	boxes = me.getBoxes(),
> +	data = [];
> +
> +	Ext.Array.forEach(boxes, function(box){
> +	    if (box.getValue() && box.inputValue == 'enabled') {
> +		data.push('1');
> +	    } else if (box.getValue()) {
> +		data.push(box.inputValue+'=1');
> +	    }
> +	});
> +	/* because agent is an array */
> +	/*jslint confusion: true*/
> +	if (data.length === 0) {
> +	    return { 'agent': '0' };
> +	} else {
> +	    return { 'agent': data.join(',') };
> +	}
> +    }
> +
> +});
> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> index 6f0b2511..16249fb4 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -266,17 +266,15 @@ Ext.define('PVE.qemu.Options', {
>  	    agent: {
>  		header: gettext('Qemu Agent'),
>  		defaultValue: false,
> -		renderer: Proxmox.Utils.format_boolean,
> +		renderer: PVE.Utils.render_qga_features,
>  		editor: caps.vms['VM.Config.Options'] ? {
>  		    xtype: 'proxmoxWindowEdit',
>  		    subject: gettext('Qemu Agent'),
>  		    items: {
> -			xtype: 'proxmoxcheckbox',
> +			xtype: 'pveAgentFeatureSelector',
>  			name: 'agent',
> -			uncheckedValue: 0,
> -			defaultValue: 0,
> -			deleteDefaultValue: true,
> -			fieldLabel: gettext('Enabled')
> +			value: '',
> +			allowBlank: true
>  		    }
>  		} : undefined
>  	    },
>