Re: [pve-devel] [PATCH manager v12 5/6] add clipboard checkbox to VM Options

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

Rest of the series looks fine to me (and tested ok), but here i have some comments/nits inline:

On 9/8/23 13:06, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   www/manager6/qemu/DisplayEdit.js |  8 +++++
>   www/manager6/qemu/Options.js     | 52 ++++++++++++++++++++++++++++++++
>   2 files changed, 60 insertions(+)
> 
> diff --git a/www/manager6/qemu/DisplayEdit.js b/www/manager6/qemu/DisplayEdit.js
> index 9bb1763e..d7cd51a9 100644
> --- a/www/manager6/qemu/DisplayEdit.js
> +++ b/www/manager6/qemu/DisplayEdit.js
> @@ -4,6 +4,9 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
>       onlineHelp: 'qm_display',
>   
>       onGetValues: function(values) {
> +	if (typeof this.originalConfig.clipboard !== 'undefined') {
> +	    values.clipboard = this.originalConfig.clipboard;
> +	}
>   	let ret = PVE.Parser.printPropertyString(values, 'type');
>   	if (ret === '') {
>   	    return { 'delete': 'vga' };
> @@ -11,6 +14,11 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
>   	return { vga: ret };
>       },
>   
> +    onSetValues: function(values) {
> +	this.originalConfig = values;
> +	return values;
> +    },
> +
>       items: [{
>   	name: 'type',
>   	xtype: 'proxmoxKVComboBox',
> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> index 7b112400..7b8283c6 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -154,6 +154,58 @@ Ext.define('PVE.qemu.Options', {
>   		    },
>   		} : undefined,
>   	    },
> +	    vga: {
> +		header: gettext('Clipboard'),
> +		defaultValue: false,
> +		renderer: function(value) {
> +		    let vga = PVE.Parser.parsePropertyString(value, 'type');
> +		    return vga.clipboard ? vga.clipboard.toUpperCase() : "auto (SPICE)";
> +		},
> +		editor: caps.vms['VM.Config.HWType'] ? {
> +		    xtype: 'proxmoxWindowEdit',
> +		    subject: gettext('Clipboard'),
> +		    onlineHelp: 'qm_display',
> +		    items: {
> +			xtype: 'pveDisplayInputPanel',
> +			items: [
> +			    {
> +				xtype: 'proxmoxKVComboBox',
> +				name: 'clipboard',
> +				itemId: 'clipboardBox',
> +				fieldLabel: gettext('Clipboard'),
> +				deleteDefaultValue: true,
> +				value: '__default__',
> +				comboItems: [
> +				    ['__default__', 'auto (SPICE)'],

nit: not sure if we really want to use 'auto (SPICE)' (@thomas) ?
wouldn't `${defaulttext} (SPICE)` fit our scheme better ?

> +				    ['vnc', 'VNC'],
> +				],
> +			    },
> +			    {
> +				itemId: 'vdagentHint',
> +				name: 'vdagentHint',
> +				xtype: 'displayfield',
> +				userCls: 'pmx-hint',
> +				value: 'The SPICE Clipboard stops working when' +
> +				    ' you are using the VNC Clipboard, as both' +
> +				    ' rely on the same SPICE vdagent.',
> +			    },

two nits here:
1. we may want to show the hint only when VNC is enabled
2. I'd remove the vdagent reference, since that is only an implementation detail
    so i'd write something like this:

----
Only one of either the SPICE or VNC clipboard can work at a time.
----
?

(also i'd put it in a gettext)

> +			],
> +			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');
> +			    return { vga: ret };
> +			},

not a nit:

this is missing the check if ret === '' (which then must send the delete parameter)
otherwise you can get the empty string into the config

> +			onSetValues: function(values) {
> +			    this.originalConfig = PVE.Parser.parsePropertyString(values.vga, 'type');
> +			    return this.originalConfig;
> +			},
> +		    },
> +		} : undefined,
> +	    },
>   	    hotplug: {
>   		header: gettext('Hotplug'),
>   		defaultValue: 'disk,network,usb',