[pve-devel] [PATCH manager 3/4] spice: Add enhancements to VM Options panel

[Aaron Lauterer]

On 9/16/19 2:44 PM, Stefan Reiter wrote:
> On 9/13/19 3:16 PM, Aaron Lauterer wrote:
>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>> ---
>>   www/manager6/Utils.js        | 18 ++++++++++++++++++
>>   www/manager6/qemu/Options.js | 13 +++++++++++++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
>> index 6a489e7e..139200c3 100644
>> --- a/www/manager6/Utils.js
>> +++ b/www/manager6/Utils.js
>> @@ -334,6 +334,24 @@ Ext.define('PVE.Utils', { utilities: {
>>       }
>>       },
>> +    render_spice_enhancements: function(value) {
>> +    if (!value) {
>> +        return Proxmox.Utils.disabledText;
>> +    }
>> +    var props = PVE.Parser.parsePropertyString(value);
>> +    if (Ext.Object.isEmpty(props)) {
>> +        return Proxmox.Utils.disabledText;
>> +    }
>> +    var ret = [];
>> +    if (props.foldersharing === "1") {
> 
> I don't think '=== "1"' catches all cases here, USBEdit.js for example 
> contains a check like this:
> 
>    if (/^usb3=(1|on|true)$/.test(data[i])) {
>      ...
>    }
> 
> while our JSONSchema parser even accepts "yes" in addition to the ones 
> above.
> 
> Maybe a common Regex/helper like "parse_boolean" in JSONSchema.pm would 
> be useful in JS too?
> 
> 
>> +        ret.push("Folder sharing enabled");
> 
> These...
Yes you are right
> 
>> +    }
>> +    if (props.videostreaming === "all" || props.videostreaming === 
>> "filter") {
>> +        ret.push("Video Streaming: " + props.videostreaming);
> 
> ...need localization (gettext), since not language independent.

Here I am not sure but it does not cost much to do it.
> 
>> +    } > +    return ret.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..96eb0499 100644
>> --- a/www/manager6/qemu/Options.js
>> +++ b/www/manager6/qemu/Options.js
>> @@ -281,6 +281,19 @@ 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'),
> 
> Just as a note, SPICE enhancements currently don't have a documentation 
> available, but once they do, an "onlineHelp" would be useful here.

Once the docs are written this will be added.

> 
>> +            items: {
>> +            xtype: 'pveSpiceEnhancementSelector',
>> +            name: 'spice_enhancements',
>> +            }
>> +        } : undefined
>> +        },
> 
> Maybe disable this if VGA is not QXL (see also my note in 4/4).

I don't think this is a good idea. It is clearly named and I don't want 
to have to set the VGA to Spice first in order to change any of the 
enhancements in the options.

If I want to enable Spice with all this and I am in the options panel I 
would need to change to the hardware panel first and then come back to 
the options. If I am disabling Spice and forgot to disable the 
enhancements before setting VGA to something else I need to go back and 
temporarily enable it again.

Either way is too much of a hassle.

> 
>>           hookscript: {
>>           header: gettext('Hookscript')
>>           }
>>