From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id E4FB31FF09B for ; Mon, 31 Aug 2026 10:03:59 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 32955212E0; Mon, 31 Aug 2026 10:03:59 +0200 (CEST) Message-ID: Date: Mon, 31 Aug 2026 10:03:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH manager v2] fix #7896: ui: qemu: show effective machine version for Windows guests To: Jakob Klocker , pve-devel@lists.proxmox.com References: <20260824130155.249596-1-j.klocker@proxmox.com> <382da81b-b48b-4047-9fb0-a9d355573902@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788163422222 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.684 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: F7QJNGP5MYUU6VMV5WRPAY5ZJXO3QXHO X-Message-ID-Hash: F7QJNGP5MYUU6VMV5WRPAY5ZJXO3QXHO X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 8/31/26 9:11 AM, Jakob Klocker wrote: > On Wed Aug 26, 2026 at 10:36 AM CEST, Dominik Csapak wrote: >> one comment inline >> >> On 8/24/26 3:01 PM, Jakob Klocker wrote: >> [snip] >>> q35: (get) => get('type') === 'q35', >>> @@ -102,10 +104,21 @@ Ext.define('PVE.qemu.MachineInputPanel', { >>> } >>> >>> if (me.isWindows) { >>> - if (values.machine === '__default__') { >>> - values.version = 'pc-i440fx-5.1'; >>> - } else if (values.machine === 'q35') { >>> - values.version = 'pc-q35-5.1'; >>> + if (values.machine === '__default__' || values.machine === 'q35') { >>> + let effective = PVE.Utils.qemu_implicit_machine_version( >>> + values.machine, >>> + values.creationQemu, >>> + values.arch, >>> + ); >>> + me.getViewModel().set({ >>> + effectiveVersionLabel: effective + ' ' + gettext('(implicit)'), >>> + implicitVersion: true, >>> + }); >>> + me.setAdvancedVisible(true); >> >> >>> + // flush the binding, then re-baseline the reset value to it >>> + // otherwise the Reset button reverts to the field's empty initial value >>> + me.getViewModel().notify(); >>> + me.lookup('effectiveVersion').resetOriginalValue(); >> >> under which circumstances is this necessary? >> >> even with those two statements deleted, i can't trigger it to have >> the reset button enabled? >> >> in general, we probably shouldn't mess with the 'resetOriginalValue' too >> much, but if we do, having the steps where this is necessary >> would be good (does not have to be a comment, in the commit message is >> fine for me) > Than for taking a look at the patch. > > I'm not quite sure what you mean with 'reset button enabled'. The reset > button enables just like in other UI forms; in the Machine case that's > when you select a different value in the dropdowns (e.g. change > `Machine`, `Version` or `vIOMMU`). I'm talking about the > `Reset from Data` button on the top right. > > The `resetOriginalValue` is necessary because the > `effectiveVersionLabel` value is empty on page load, and only gets set > when `setValues` is called. Since `setValues` is not called on a reset, > this would display an empty current version on a reset. Therefore I > replace the field's empty initial value with the actual current version > on page load, so a reset reverts to the correct value instead of an > empty one. > > I thought adding the comment above the code, saying that on reset the > value is empty made this clear -- if not, I can mention why exactly I > used this in the commit message as well. ok i misunderstood what you meant from the comment, i get it now and could reproduce the misbehavior with the lines deleted. (i simply looked for a different thing) there is a much easier method instead of manipulating the value itself and notifying the viewmodel: since both the effectiveVersion field and the hint are only modified once, we don't need to use the viewmodel here at all? just have the field value set via the normal 'value' object but set 'submitValue: false' so it does not get submitted on clicking ok. (the setValue parent handler then calls resetOriginal value anyway) same for the hint, we can show/hide it in the setValue method, but don't have to use the viewmodel for it. In general the viewmodel is only really helpful if we change their values dynamically while the user has the editor open which isn't the case for the 'setValue' method (this is only called once on opening)