[pve-devel] [PATCH v3 manager 3/3] ui: vm-options: add spice enhancements

[Thomas Lamprecht]

On 10/4/19 12:09 PM, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
> removing the check if values is true and changing the parameters in
> parsePropertyString to `parsePropertyString(values || {})` as suggested
> in [0] does not work.
> parsePropertyString expects a string. Passing anything except a string
> containing a `key=value` (there is no default key for this option)
> results in warning and errors in the JS console.
> 
> Therefore I kept the old checks.

Often, if the proposed solution does not immeadiately work it may still
be good to check out if it or a similar solution can be made working
relatively easily, instead of going just straight back to the original
one.

I straight pushed the following change for the parsePropertyString into
master, which allows to use Dominiks proposal even nicer and fixes 2/3
to make it work at all (see reply there).

----8<----
diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
index 14e7af3a..9dfabfea 100644
--- a/www/manager6/Parser.js
+++ b/www/manager6/Parser.js
@@ -49,6 +49,10 @@ Ext.define('PVE.Parser', { statics: {
        var res = {},
            error;

+       if (typeof value !== 'string' || value === '') {
+           return res;
+       }
+
        Ext.Array.each(value.split(','), function(p) {
            var kv = p.split('=', 2);
            if (Ext.isDefined(kv[1])) {
--

> 
> [0]: https://pve.proxmox.com/pipermail/pve-devel/2019-September/039054.html
> 
> www/manager6/Utils.js        | 21 +++++++++++++++++++++
>  www/manager6/qemu/Options.js | 14 ++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 5cc6b674..be568070 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -334,6 +334,27 @@ Ext.define('PVE.Utils', { utilities: {
>  	}
>      },
>  
> +    render_spice_enhancements: function(values) {
> +	let disabled = Proxmox.Utils.disabledText;
> +
> +	if (!values) {
> +	    return disabled;
> +	}
> +	let props = PVE.Parser.parsePropertyString(values);
> +	if (Ext.Object.isEmpty(props)) {
> +	    return disabled;
> +	}
> +
> +	let output = [];
> +	if (PVE.Parser.parseBoolean(props.foldersharing)) {
> +	    output.push(gettext("Folder sharing enabled"));

nit: why the different capitalization, i.e., here only "*F*older sharing",
below *V*ideo *S*treaming? Also the gettext, I'd remove it and maybe do:
'Folder sharing: ' + gettext('Enabled');

> +	}
> +	if (props.videostreaming === "all" || props.videostreaming === "filter") {
> +	    output.push(gettext("Video Streaming") + ": " + props.videostreaming);

not sure about the gettext here, maybe it has more value to keep this
untranslated as this are quite specific features? reduces translation work and
maybe helps in people finding information when they search for the original
for such a specific SPICE thing?

> +	}
> +	return output.join(", ");
> +    },
> +
>      // fixme: auto-generate this
>      // for now, please keep in sync with PVE::Tools::kvmkeymaps
>      kvm_keymaps: {
> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> index e1580060..4a8e06e9 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -281,6 +281,20 @@ Ext.define('PVE.qemu.Options', {
>  		    }
>  		} : undefined
>  	    },
> +	    spice_enhancements: {
> +		header: gettext('Spice Enhancements'),
> +		defaultValue: false,
> +		renderer:  PVE.Utils.render_spice_enhancements,
> +		editor: caps.vms['VM.Config.Options'] ? {
> +		    xtype: 'proxmoxWindowEdit',
> +		    subject: gettext('Spice Enhancements'),
> +		    onlineHelp: 'qm_spice_enhancements',
> +		    items: {
> +			xtype: 'pveSpiceEnhancementSelector',
> +			name: 'spice_enhancements',
> +		    }
> +		} : undefined
> +	    },
>  	    hookscript: {
>  		header: gettext('Hookscript')
>  	    }
>