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

[Aaron Lauterer]

On 9/20/19 9:00 AM, Thomas Lamprecht wrote:
> 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"

Yep, sounds good.

> 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 ;)

Okay, I am going to try how we could do this in another way, without 
destroying old muscle memory.

> 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.

The same with regards to placement is valid for the qemu agent. In the 
creation wizard it is in the System tab, close to the setting that can 
enable these features.

Once the VM is created I expect settings like these in the options panel 
and not the hardware panel.
Having all the SPICE enhancement settings in the display / hardware 
settings would surely confuse me more.

I will add a hint in the options -> spice dialog that spice needs to be 
enabled for the settings to have any effect if it is not the selected 
graphics card.

> 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?

So we could just add the two new settings (folder sharing, video 
streaming) in the same plain way as the qemu agent and if a user enables 
either of these two options and spice is not selected show the same hint 
that they will only have an effect if spice is selected as graphics card.

Other possibilities would be to group some options in fieldsets or 
something similar. In the end this comes down to taste, how these items 
should be structured and how simple the UI should be kept.

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

Move these items to the advanced area and have it always enabled in the 
system tab? I don't like that idea as it would break the UI's behavior 
in a much harder way.
> 
> 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.

These two ideas sound good for me, especially when thinking about making 
these settings easily accessible during the wizard and thus reducing the 
needed interactions after the wizard is done.

For the future, if we go down the road of having more of these settings 
in the wizard grouping them in some way might be a good idea.

What about making these additional settings (number of monitors, spice 
USB port, etc.) only visible if spice is selected as graphics card? This 
would help to keep the clutter to a minimum.
They should not go to the advanced area IMHO because once spice is 
selected some of these settings are essential and the others we don't 
want to hide from the user because then we could skip adding them to the 
wizard in the first place.

Another thing that just hit me is that we will need an additional hint 
to tell users that folder sharing is only supported in the Linux version 
of virt-viewer to not get any Windows desktop users all excited for nothing.

> 
>>> 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.

Okay

> 
>>> +            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,
>>>        }
>>>        ],
>>>   
>