[pve-devel] [PATCH manager 2/4] spice: Add enhancements form component

[Stefan Reiter]

Tried my hand at a proper code review, I like the patches in general.

On 9/13/19 3:16 PM, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
>   www/manager6/Makefile                         |  1 +
>   www/manager6/form/SpiceEnhancementSelector.js | 66 +++++++++++++++++++
>   2 files changed, 67 insertions(+)
>   create mode 100644 www/manager6/form/SpiceEnhancementSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 82e25c79..aa460c3b 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -66,6 +66,7 @@ JSSRC= 				                 	\
>   	form/CalendarEvent.js				\
>   	form/CephPoolSelector.js			\
>   	form/PermPathSelector.js			\
> +	form/SpiceEnhancementSelector.js		\
>   	dc/Tasks.js					\
>   	dc/Log.js					\
>   	panel/StatusPanel.js				\
> diff --git a/www/manager6/form/SpiceEnhancementSelector.js b/www/manager6/form/SpiceEnhancementSelector.js
> new file mode 100644
> index 00000000..5457c8d5
> --- /dev/null
> +++ b/www/manager6/form/SpiceEnhancementSelector.js
> @@ -0,0 +1,66 @@
> +Ext.define('PVE.form.SpiceEnhancementSelector', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: 'widget.pveSpiceEnhancementSelector',
> +
> +    initComponent: function() {
> +	var me = this;
> +	me.items= [
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		itemId: 'foldersharing',
> +		name: 'foldersharing',
> +		submitValue: false,
> +		fieldLabel: gettext('Folder sharing'),
> +		uncheckedValue: 0,
> +	    },
> +	    {
> +		xtype: 'proxmoxKVComboBox',
> +		itemId: 'videostreaming',
> +		name: 'videostreaming',
> +		submitValue: false,
> +		value: 'off',
> +		fieldLabel: gettext('Video streaming'),
> +		comboItems: [
> +		    ['off', 'off'],
> +		    ['all', 'all'],
> +		    ['filter', 'filter'],
> +		],
> +	    },
> +	];

Why not just set the items array on the object directly? Then you 
wouldn't have to override initComponent.

> +	me.callParent();
> +    },
> +
> +    // handle submitted values manually to work in the VM create wizard as well.
> +    // without submitValue = false the fields would be added to the config
> +    onGetValues: function() {
> +	var me = this;
> +
> +	if (me.disabled) {
> +	    return

Missing ;

> +	}
> +
> +	var values = {};
> +	var foldersharing = me.down('field[name=foldersharing]').getValue();
> +	var videostreaming= me.down('field[name=videostreaming]').getValue();

Missing space before =

> +
> +	if (videostreaming !== "off") {
> +	    values.videostreaming = videostreaming;
> +	}
> +	if (foldersharing) {
> +	    values.foldersharing = 1;
> +	}
> +	if (Ext.Object.isEmpty(values)) {
> +	    return { 'delete': 'spice_enhancements' };
> +	}
> +	var enhancements = PVE.Parser.printPropertyString(values);
> +	return { spice_enhancements: enhancements };
> +    },
> +
> +    setValues: function(values) {
> +	var enhancements = values.spice_enhancements || '';
> +	if (enhancements) {
> +	    var res = PVE.Parser.parsePropertyString(enhancements);

Same as in 3/4, foldersharing would need to be converted to a true 
boolean here (from potentially "on", "yes", ...).

> +	    this.callParent([res]);
> +	}
> +    },
> +});
>