[pve-devel] [PATCH manager v2 4/4] spice: Add enhancements to VM Creation wizard

[Thomas Lamprecht]

On 18.09.19 12:22, Dominik Csapak wrote:
> lgtm, i find it understandable that the 'spice enhancments' are disabled
> because spice is not selected, but maybe someone else has a comment on that?
> 

IMO this is highly confusing and bad UX... The fields relation is
not visible, also it looks like a checkbox misses "Spice enhancements"
as it's really not clear that this should be some sort of "option group
heading"

> On 9/17/19 11:35 AM, Aaron Lauterer wrote:
>> For a cleaner UI the SCSI Controller (pveScsiHwSelector) is moved to the
>> left column below the VGA selector. The new Spice enhancements
>> components is placed in the right column and enabled if qxl/spice is
>> selected in the VGA selector.
>>

1. I'd omit the extra "Spice enhancements:" and just include "SPICE" (note here:
   keep the uppercase consistent, the display selector uses SPICE and is before your
   patch, try to orient on such things) in the settings, like "SPICE folder sharing"
2. Please keep the SCSI controller where it is, no reason or improvement to move it
   just breaks muscle memory of existing users. We're not a super market, random movement
   of fields to make people look for them and thus find new "buttons to buy" should thus
   be rather avoided ;)
3. It's a bit strange that here the spice enhancements are with the display settings but
   then, after VM creation the display settings are in the HW tab while this is in the
   Options. While it could fit there for sure, it makes one search for this.. But OK,
   that's maybe a bit hard to solve right, more confusing is the fact that I cannot change
   those in the Wizard on creation if SPICE is not selected, but I can do so afterwards in
   the options edit, also a semantic difference, which may not be to ideal for UX.
   A simple hint, like we do for OVMF/EFI Disk could be enough.
4. As others mentioned, the enhancements may also make sense without a QXL display, while
   our code path doesn't really allow this separation as is this should at least not be 
   made hard for the future. I mean, if I think about this it seems to fit better with
   QEMU Agent, as Guest Integration Enhancement, than with the display itself?

Maybe just move this to advanced here and show it always?

The, "SPICE dual monitor", "three monitors", .. could also be integrated in such settings?
I.e., a simple spinner with monitor count and a single SPICE in the display combo box.

As an additional spice enhancement convenience feature one could add a checkbox, or the
like, here with which a spice USB port gets added from the wizard?
But the latter two should be additional patches, if done.

>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>> ---
>>   www/manager6/qemu/SystemEdit.js | 34 ++++++++++++++++++++++++---------
>>   1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/www/manager6/qemu/SystemEdit.js b/www/manager6/qemu/SystemEdit.js
>> index 846baa73..dac0cc04 100644
>> --- a/www/manager6/qemu/SystemEdit.js
>> +++ b/www/manager6/qemu/SystemEdit.js
>> @@ -79,7 +79,26 @@ Ext.define('PVE.qemu.SystemInputPanel', {
>>           deleteEmpty: false,
>>           fieldLabel: gettext('Graphic card'),
>>           name: 'vga',
>> -        comboItems: PVE.Utils.kvm_vga_driver_array()
>> +        comboItems: PVE.Utils.kvm_vga_driver_array(),
>> +        listeners: {
>> +        change: function(f, value, old) {
>> +            var sef = this.up('pveQemuSystemPanel').down('pveSpiceEnhancementSelector');

what's "sef" ? use variable names with a bit better, more descriptive, names.
"spice_enhancements" works better than the abbreviation for Spice Enhancement Field, IMO.

>> +            if (/^(qxl)(\d?)$/.test(value)) {
>> +            sef.setDisabled(false);
>> +            } else {
>> +            sef.setDisabled(true);
>> +            }
>> +        }
>> +        }
>> +    },
>> +    {
>> +        xtype: 'pveScsiHwSelector',
>> +        name: 'scsihw',
>> +        value: '__default__',
>> +        bind: {
>> +        value: '{current.scsihw}'
>> +        },
>> +        fieldLabel: gettext('SCSI Controller')
>>       },
>>       {
>>           xtype: 'proxmoxcheckbox',
>> @@ -88,18 +107,15 @@ Ext.define('PVE.qemu.SystemInputPanel', {
>>           defaultValue: 0,
>>           deleteDefaultValue: true,
>>           fieldLabel: gettext('Qemu Agent')
>> -    }
>> +    },
>>       ],
>>         column2: [
>>       {
>> -        xtype: 'pveScsiHwSelector',
>> -        name: 'scsihw',
>> -        value: '__default__',
>> -        bind: {
>> -        value: '{current.scsihw}'
>> -        },
>> -        fieldLabel: gettext('SCSI Controller')
>> +        xtype: 'pveSpiceEnhancementSelector',
>> +        name: 'spice_enhancements',
>> +        insideWizard: true,
>> +        disabled: true,
>>       }
>>       ],
>>