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

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

a few nits inline:

On 9/21/23 13:47, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   www/manager6/qemu/DisplayEdit.js |  8 +++
>   www/manager6/qemu/Options.js     | 89 ++++++++++++++++++++++++++++++++
>   2 files changed, 97 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..73d0c923 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -154,6 +154,95 @@ 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,
> +			items: [
> +			    {
> +				xtype: 'proxmoxKVComboBox',
> +				name: 'clipboard',
> +				reference: 'clipboard',
> +				itemId: 'clipboardBox',
> +				fieldLabel: gettext('Clipboard'),
> +				deleteDefaultValue: true,
> +				listeners: {
> +				    change: function(field, value) {
> +					let inputpanel = field.up("inputpanel");
> +					let vncHint = inputpanel.lookup('vncHint');
> +					let defaultHint = inputpanel.lookup('defaultHint');
> +					if (value === "__default__") {
> +					    vncHint.setVisible(false);
> +					    defaultHint.setVisible(true);
> +					} else if (value === "vnc") {
> +					    vncHint.setVisible(true);
> +					    defaultHint.setVisible(false);
> +					}

nit: since this can only have two values currently, this could be shorter by doing:

let isVnc = value === 'vnc';
inputpanel.lookup('vncHint').setVisible(isVnc);
inputpanel.lookup('defaultHint').setVisible(!isVnc);

but no hard feelings though

> +				    },
> +				},
> +				value: '__default__',
> +				comboItems: [
> +				    ['__default__', Proxmox.Utils.defaultText + ' (SPICE)'],
> +				    ['vnc', 'VNC'],
> +				],
> +			    },
> +			    {
> +				itemId: 'vncHint',
> +				name: 'vncHint',
> +				reference: 'vncHint',
> +				xtype: 'displayfield',
> +				userCls: 'pmx-hint',
> +				hidden: true,
> +				value: 'You cannot use the default SPICE clipboard' +
> +				    ' if the VNC Clipboard is selected',

nit: i'd maybe like an additional sentence here that says
the user has to install the spice-tools in the guest
(to avoid confusion)

also maybe we should pack these warnings in gettexts
(what is our current policy for that regarding long texts @thomas?)

> +			    },
> +
> +			    {
> +				itemId: 'defaultHint',
> +				name: 'defaultHint',
> +				reference: 'defaultHint',
> +				xtype: 'displayfield',
> +				userCls: 'pmx-hint',
> +				hidden: false,
> +				value: 'This option depends on your display type.' +
> +				    ' If the display type uses SPICE you are' +
> +				    ' able to use the default SPICE Clipboard',
> +			    },
> +
> +			],
> +			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',