Re: [PATCH pve-manager 5/6] ui: qemu: options: add editor for reboot behavior

[David Riley <d.riley@proxmox.com>]

see comments inline

On 8/13/26 1:27 PM, Jakob Klocker wrote:
> The `reboot` option is now a property string with an added `powercycle`
> sub-property, so expose both in the VM options panel: one checkbox to
> allow reboots at all, and one to stop and start the VM on a
> guest-initated reboot instead of resetting it in place, which makes
> pending changes take effect.
>
> Signed-off-by: Jakob Klocker<j.klocker@proxmox.com>
> ---
>   www/manager6/Makefile                      |  1 +
>   www/manager6/Utils.js                      | 17 +++++++
>   www/manager6/form/RebootFeatureSelector.js | 59 ++++++++++++++++++++++
>   www/manager6/qemu/Options.js               | 14 +++++
>   4 files changed, 91 insertions(+)
>   create mode 100644 www/manager6/form/RebootFeatureSelector.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index eb0e9d9c..40f71b8c 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -67,6 +67,7 @@ JSSRC= 							\
>   	form/PrivilegesSelector.js			\
>   	form/QemuBiosSelector.js			\
>   	form/QemuMachineSelector.js			\
> +	form/RebootFeatureSelector.js			\
>   	form/RecordSearchField.js			\
>   	form/SDNControllerSelector.js			\
>   	form/SDNZoneSelector.js				\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 040b5ae0..f10c26d5 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1160,6 +1160,23 @@ Ext.define('PVE.Utils', {
>               return Ext.Date.format(new Date(value * 1000), 'l d F Y H:i:s');
>           },
>   
> +        render_reboot: function (value) {

nit: this should be in camelCase see style guide [0]

[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

> +            let normal = gettext('Reboot normally');
> +
> +            if (value === undefined || value === '') {

nit: here you are just catching undefined values but not 'null' values.
Consider using 'Ext.isEmpty(value)' here as this would cover 'null',
'undefined' as well as the empty string check, see [1].

[1] https://docs.sencha.com/extjs/7.0.0/classic/Ext.html#method-isEmpty

> +                return `${Proxmox.Utils.defaultText} (${normal})`;
> +            }
> +
> +            let props = PVE.Parser.parsePropertyString(value, 'enabled');
> +            if (!PVE.Parser.parseBoolean(props.enabled, 1)) {
> +                return gettext('Shut down instead of rebooting');
> +            }
> +            if (PVE.Parser.parseBoolean(props.powercycle, 0)) {
> +                return gettext('Stop and start the VM on guest reboot');
> +            }
> +            return normal;
> +        },
> +
>           // render a timestamp or pending
>           render_next_event: function (value) {
>               if (!value) {
> diff --git a/www/manager6/form/RebootFeatureSelector.js b/www/manager6/form/RebootFeatureSelector.js
> new file mode 100644
> index 00000000..88e93da3
> --- /dev/null
> +++ b/www/manager6/form/RebootFeatureSelector.js
> @@ -0,0 +1,59 @@
> +Ext.define('PVE.form.RebootFeatureSelector', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    alias: ['widget.pveRebootFeatureSelector'],
> +
> +    viewModel: {},
> +
> +    items: [
> +        {
> +            xtype: 'proxmoxcheckbox',
> +            boxLabel: gettext('Allow reboot'),
> +            name: 'enabled',
> +            reference: 'enabled',
> +            uncheckedValue: 0,
> +        },
> +        {
> +            xtype: 'proxmoxcheckbox',
> +            boxLabel: gettext('Stop and start the VM on guest reboot (applies pending changes)'),
> +            name: 'powercycle',
> +            uncheckedValue: 0,
> +            bind: {
> +                disabled: '{!enabled.checked}',
> +            },
> +            disabled: true,
> +        },
> +        {
> +            xtype: 'displayfield',
> +            userCls: 'pmx-hint',
> +            value: gettext('The VM is shut down instead of rebooted and stays off.'),
> +            bind: {
> +                hidden: '{enabled.checked}',
> +            },
> +        },
> +    ],
> +
> +    onGetValues: function (values) {
> +        let enabled = !Ext.isDefined(values.enabled) || String(values.enabled) === '1';

Do we need the !Ext.isDefined check here? Is it not guaranteed that this will always
hold a value, because you have set 'uncheckedValue: 0' on the proxmoxcheckbox?

> +        let powercycle = String(values.powercycle) === '1';
> +
> +        // equivalent to the default, so don't write the option at all
> +        if (enabled && !powercycle) {
> +            return { delete: 'reboot' };
> +        }
> +
> +        let props = { enabled: enabled ? 1 : 0 };
> +        if (powercycle) {
> +            props.powercycle = 1;
> +        }
> +
> +        return { reboot: PVE.Parser.printPropertyString(props, 'enabled') };
> +    },
> +
> +    setValues: function (values) {
> +        let res = PVE.Parser.parsePropertyString(values.reboot, 'enabled');
> +        if (!Ext.isDefined(res.enabled)) {
> +            res.enabled = 1;
> +        }
> +        this.callParent([res]);
> +    },
> +});
> diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
> index 8a4721a0..4fd5d1bb 100644
> --- a/www/manager6/qemu/Options.js
> +++ b/www/manager6/qemu/Options.js
> @@ -84,6 +84,20 @@ Ext.define('PVE.qemu.Options', {
>                             }
>                           : undefined,
>               },
> +            reboot: {
> +                header: gettext('Reboot behavior'),
> +                defaultValue: '',
> +                renderer: PVE.Utils.render_reboot,
> +                editor: caps.vms['VM.Config.Options']
> +                    ? {
> +                          xtype: 'proxmoxWindowEdit',
> +                          subject: gettext('Reboot behavior'),
> +                          onlineHelp: 'qm_reboot_behavior',
> +                          width: 350,
> +                          items: { xtype: 'pveRebootFeatureSelector', name: 'reboot' },

nit: is the name here actually needed?

> +                      }
> +                    : undefined,
> +            },
>               ostype: {
>                   header: gettext('OS Type'),
>                   editor: caps.vms['VM.Config.Options']