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

[Aaron Lauterer]

On 9/16/19 2:44 PM, Stefan Reiter wrote:
> 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 ;

Thx for seeing this
> 
>> +    }
>> +
>> +    var values = {};
>> +    var foldersharing = me.down('field[name=foldersharing]').getValue();
>> +    var videostreaming= 
>> me.down('field[name=videostreaming]').getValue();
> 
> Missing space before =

Thx
> 
>> +
>> +    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", ...).

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