Re: [pve-devel] [PATCH manager v3 5/6] added clipboard checkbox & combobox to DisplayEdit

[Dominik Csapak <d.csapak@proxmox.com>]

high level comments:

i'd only show the hint when the checkbox is checked
or 'novnc' is selected in the combobox

also here it shows that the options does not really fit to the display at all,
at least i would not search the setting for the console clipboard under the
virtual display hardware...
(we could just have the gui seperate on the 'options' tab, and leave the option
itself on the display option...)

comments inline

On 10/28/22 14:33, Markus Frank wrote:
> If display is set to spice the checkbox gets replaced by a combobox to
> show the available clipboard options.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   www/manager6/qemu/DisplayEdit.js | 62 +++++++++++++++++++++++++++++++-
>   1 file changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/qemu/DisplayEdit.js b/www/manager6/qemu/DisplayEdit.js
> index 9bb1763e..2cef5ad2 100644
> --- a/www/manager6/qemu/DisplayEdit.js
> +++ b/www/manager6/qemu/DisplayEdit.js
> @@ -33,22 +33,54 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
>   		    return;
>   		}
>   		let memoryfield = this.up('panel').down('field[name=memory]');
> +		let clipboardBox = this.up('panel').down('field[itemId=clipboardBox]');
> +		let clipboardDrop = this.up('panel').down('field[itemId=clipboardDrop]');
> +		let vdagentHint = this.up('panel').down('field[name=vdagentHint]');

at this point, i'd really prefer to either pull the 'panel' into a variable only once
or even rewrite the component using a controller and using references

it's not a huge performance issue, but the it makes it much less readable

also clipboardDrop could be better written as clipboardDropDown

>   		let disableMemoryField = false;
> +		let spice = false;
> +		let showClipboardAndHint = true;
>   
>   		if (val === "cirrus") {
>   		    memoryfield.setEmptyText("4");
> -		} else if (val === "std" || val.match(/^qxl\d?$/) || val === "vmware") {
> +		} else if (val === "std" || val === "vmware") {
>   		    memoryfield.setEmptyText("16");
> +		} else if (val.match(/^qxl\d?$/)) {
> +		    memoryfield.setEmptyText("16");
> +		    spice = true;
>   		} else if (val.match(/^virtio/)) {
>   		    memoryfield.setEmptyText("256");
> +		    spice = true;
>   		} else if (val.match(/^(serial\d|none)$/)) {
>   		    memoryfield.setEmptyText("N/A");
>   		    disableMemoryField = true;
> +		    showClipboardAndHint = false;
>   		} else {
>   		    console.debug("unexpected display type", val);
>   		    memoryfield.setEmptyText(Proxmox.Utils.defaultText);
>   		}
>   		memoryfield.setDisabled(disableMemoryField);
> +		vdagentHint.setVisible(showClipboardAndHint);
> +		if (showClipboardAndHint) {
> +		    // switch from Checkbox to ComboBox and vice versa
> +		    clipboardBox.setDisabled(spice);
> +		    clipboardDrop.setDisabled(!spice);
> +		    clipboardBox.setVisible(!spice);
> +		    clipboardDrop.setVisible(spice);
> +		    // reset value when changing to spice,
> +		    // so that you have to actively change to noVNC Clipboard
> +		    if (spice) {
> +			clipboardDrop.setValue('__default__');
> +		    }
> +		} else {
> +		    // reset to default
> +		    clipboardBox.setValue(false);
> +		    clipboardDrop.setValue('__default__');
> +		    // show only the disabled Checkbox
> +		    clipboardBox.setDisabled(true);
> +		    clipboardDrop.setDisabled(true);
> +		    clipboardBox.setVisible(true);
> +		    clipboardDrop.setVisible(false);
> +		}

i find that whole section a bit too complicated, it took me a few glances
to see what it's actually doing

i'd propose a different approach:

use a 'showClipboardBox' and a 'showClipboardDropDown'
variable to track which you want to show and then only do a

clipboardBox.setDisabled(!showClipboardBox);
clipboardBox.setVisible(!showClipboardBox);

etc.

>   	    },
>   	},
>       },
> @@ -60,6 +92,34 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
>   	maxValue: 512,
>   	step: 4,
>   	name: 'memory',
> +    },
> +    {
> +	xtype: 'proxmoxcheckbox',
> +	fieldLabel: gettext('noVNC Clipboard'),
> +	name: 'clipboard',
> +	itemId: 'clipboardBox',
> +    },
> +    {
> +	name: 'clipboard',
> +	itemId: 'clipboardDrop',
> +	xtype: 'proxmoxKVComboBox',
> +	value: '__default__',
> +	deleteEmpty: false,
> +	fieldLabel: gettext('Clipboard'),
> +	comboItems: [
> +	    ['__default__', 'SPICE-Clipboard'],
> +	    ['1', 'noVNC-Clipboard'],
> +	],
> +	disabled: true,
> +	hidden: true,
> +    },
> +    {
> +	itemId: 'vdagentHint',
> +	name: 'vdagentHint',
> +	xtype: 'displayfield',
> +	userCls: 'pmx-hint',
> +	value: 'Clipboard for noVNC requires spice-tools installed and ' +
> +	    'enabled in the Guest-VM.',
>       }],
>   });
>