Re: [PATCH manager v2] fix #7896: ui: qemu: show effective machine version for Windows guests

["Jakob Klocker" <j.klocker@proxmox.com>]

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.