Re: [pve-devel] [PATCH manager v15 1/2] add clipboard comboBox to VM Options

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

subject should be more in the style of:

ui: qemu: add clipboard selector to options

Am 21/11/2023 um 13:39 schrieb Markus Frank:
> For SPICE and VNC, a different message is displayed.
> 

possibly reference the backend work here,

> Save config in DisplayEdit so that the clipboard setting persist.

this sentence is not really that self-explaining, maybe extend
on the reasoning here.

> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> Tested-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> v15:
> * changed style of line break in vncHint field
> 
>  www/manager6/qemu/DisplayEdit.js |  8 ++++
>  www/manager6/qemu/Options.js     | 80 ++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 


> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> index 7b112400..53d0beac 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -154,6 +154,86 @@ Ext.define('PVE.qemu.Options', {
>  		    },
>  		} : undefined,
>  	    },
> +	    vga: {
> +		header: gettext('Clipboard'),
> +		defaultValue: false,
> +		renderer: function(value) {
> +		    let vga = PVE.Parser.parsePropertyString(value, 'type');
> +		    if (vga.clipboard) {
> +			return vga.clipboard.toUpperCase();
> +		    } else {
> +			return Proxmox.Utils.defaultText + ' (SPICE)';
> +		    }
> +		},
> +		editor: caps.vms['VM.Config.HWType'] ? {
> +		    xtype: 'proxmoxWindowEdit',
> +		    subject: gettext('Clipboard'),
> +		    onlineHelp: 'qm_display',
> +		    items: {
> +			xtype: 'pveDisplayInputPanel',
> +			referenceHolder: true,

It might be avoidable to make this a referenceHolder by using a viewModel,
see below.

> +			items: [
> +			    {
> +				xtype: 'proxmoxKVComboBox',
> +				name: 'clipboard',
> +				reference: 'clipboard',

do we really need a reference here? I see no use of it (i.e., through lookup)

> +				itemId: 'clipboardBox',

the itemId seems also unused, and those have to be globally unique, so best
to be avoided if unsure.


> +				fieldLabel: gettext('Clipboard'),
> +				deleteDefaultValue: true,
> +				listeners: {
> +				    change: function(field, value) {
> +					let inputpanel = field.up("inputpanel");
> +					let isVnc = value === 'vnc';
> +					inputpanel.lookup('vncHint').setVisible(isVnc);
> +					inputpanel.lookup('defaultHint').setVisible(!isVnc);
> +				    },

a viewModel would be nicer to do this, e.g at the input panel level add:

viewModel: {
    data: {
        clipboard: '__default__',
    },
    formulas: {
        isVnc: get => get('clipboard') === 'vnc',
    },
},

Then in the clipboard field bind the value to the viewModel:

bind: {
    value: {clipboard},
},

and in the hint fields bind the visibility state to the formula:

bind: {
    hidden: {isVNC},
    // or reverse: hidden: {!isVNC},
},


> +				},
> +				value: '__default__',
> +				comboItems: [
> +				    ['__default__', Proxmox.Utils.defaultText + ' (SPICE)'],
> +				    ['vnc', 'VNC'],
> +				],
> +			    },
> +			    {
> +				itemId: 'vncHint',
> +				name: 'vncHint',
> +				reference: 'vncHint',

view the viewModel you would avoid the explicit references and let ExtJS built-in
logic handle this.

> +				xtype: 'displayfield',

the xtype should be the first property in item object definitions. immediately
followed by the name.

> +				userCls: 'pmx-hint',
> +				hidden: true,
> +				value: gettext('You cannot use the default SPICE clipboard if the VNC Clipboard is selected.') + ' ' +
> +				    gettext('VNC Clipboard requires spice-tools installed in the Guest-VM.'),
> +			    },
> +			    {
> +				itemId: 'defaultHint',
> +				name: 'defaultHint',
> +				reference: 'defaultHint',

same here w.r.t. reference avoidance by using a viewModel.

> +				xtype: 'displayfield',

same here with xtype definition having to move up.

> +				userCls: 'pmx-hint',
> +				hidden: false,
> +				value: gettext('This option depends on your display type.') + ' ' +
> +				    gettext('If the display type uses SPICE you are able to use the default SPICE Clipboard.'),

Those hints are IMO a good indicator that it would be more fitting to have in
the advanced options of the display hardware.

As while it might be slightly more discoverable here, having to jump between
hardware and options to bring this in sync is  a bit of a nuisance, and docs
can improve discoverability wherever it is.


That said, I have no hard feelings for this, there are some arguments for either,
I'd say just decide on your own.

> +			    },
> +			],
> +			onGetValues: function(values) {
> +			    values = Ext.apply(this.originalConfig, values);
> +			    if (values.delete === "clipboard") {
> +				delete values.clipboard;
> +				delete values.delete;
> +			    }
> +			    let ret = PVE.Parser.printPropertyString(values, 'type');
> +			    if (ret === "") {
> +				return { 'delete': "vga" };
> +			    }
> +			    return { vga: ret };
> +			},
> +			onSetValues: function(values) {
> +			    this.originalConfig = PVE.Parser.parsePropertyString(values.vga, 'type');
> +			    return this.originalConfig;
> +			},
> +		    },
> +		} : undefined,
> +	    },
>  	    hotplug: {
>  		header: gettext('Hotplug'),
>  		defaultValue: 'disk,network,usb',