From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 17A05D0F5
 for <pve-devel@lists.proxmox.com>; Wed, 20 Sep 2023 15:45:39 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id DD86D1659B
 for <pve-devel@lists.proxmox.com>; Wed, 20 Sep 2023 15:45:08 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pve-devel@lists.proxmox.com>; Wed, 20 Sep 2023 15:45:07 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8401D482E4;
 Wed, 20 Sep 2023 15:45:07 +0200 (CEST)
Message-ID: <a26be247-a62a-443b-9bb2-45ec244527e8@proxmox.com>
Date: Wed, 20 Sep 2023 15:45:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
 Markus Frank <m.frank@proxmox.com>
References: <20230908110604.132618-1-m.frank@proxmox.com>
 <20230908110604.132618-6-m.frank@proxmox.com>
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <20230908110604.132618-6-m.frank@proxmox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.011 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH manager v12 5/6] add clipboard checkbox to
 VM Options
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 20 Sep 2023 13:45:39 -0000

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',