[pve-devel] [PATCH pve-manager v3 0/1] ui: fix #6209: create snapshots and backups from context menu

[pve-devel] [PATCH pve-manager v3 0/1] ui: fix #6209: create snapshots and backups from context menu

[Nicolas Frey]

The original feature request [0] only specified the ability to take
snapshots. I added the manual backup shortcut as well, since
navigating through the menu for this action felt unnecessarily
cumbersome when performing just a quick backup.

Changes since v2:
* rebased
* use autoShow on snapshot and backup windows
* improve UX and account for slow connections by only making API
  calls for snapshot feature on button click

changes since v1, thanks @Fiona Ebner
* clarified commit message
* addressed several issues in the added menu options, namely:
  * correctly handle disabled state with async
  * fixed negation logic and cleaned up minor nits

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=6209

Nicolas Frey (1):
  ui: fix #6209: create snapshots and backups from context menu

 www/manager6/lxc/CmdMenu.js  | 46 ++++++++++++++++++++++++++++++++++++
 www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

-- 
2.47.3


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Nicolas Frey]

Adds snapshot and manual backup shortcut to VM/CT right-click context
menu.

On the previous iteration (v2), the snapshot feature availablity was
checked on menu load. It is now called only when the snapshot button
is clicked, preventing delays in loading the context menu. Testing
with simulated latency (with setTimeout) showed better UX by
displaying an error on click rather than preloading the check.

Previously, users couldn't distinguish between a slow API and the
feature being disabled. The explicit check provides clear feedback.
The 'disabled' property remains bound to VM.Snapshot to disable
the option when snapshots aren't allowed anyway.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6209
Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
 www/manager6/lxc/CmdMenu.js  | 46 ++++++++++++++++++++++++++++++++++++
 www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
index cd60c967..f0e8f700 100644
--- a/www/manager6/lxc/CmdMenu.js
+++ b/www/manager6/lxc/CmdMenu.js
@@ -134,6 +134,52 @@ Ext.define('PVE.lxc.CmdMenu', {
                 },
             },
             { xtype: 'menuseparator' },
+            {
+                text: gettext('Take Snapshot'),
+                iconCls: 'fa fa-fw fa-history',
+                disabled: !caps.vms['VM.Snapshot'],
+                handler: function () {
+                    Proxmox.Utils.API2Request({
+                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
+                        params: { feature: 'snapshot' },
+                        method: 'GET',
+                        success: function (response) {
+                            if (!response.result.data.hasFeature) {
+                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')
+                                return;
+                            }
+                            Ext.create('PVE.window.Snapshot', {
+                                nodename: info.node,
+                                vmid: info.vmid,
+                                vmname: info.name,
+                                viewonly: false,
+                                type: info.type,
+                                isCreate: true,
+                                submitText: gettext('Take Snapshot'),
+                                autoShow: true,
+                                running: running,
+                            });
+                        },
+                        failure: (response, opts) =>
+                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+                    });
+                },
+            },
+            {
+                text: gettext('Backup now'),
+                iconCls: 'fa fa-fw fa-floppy-o',
+                disabled: !caps.vms['VM.Backup'],
+                handler: function () {
+                    Ext.create('PVE.window.Backup', {
+                        nodename: info.node,
+                        vmid: info.vmid,
+                        vmtype: info.type,
+                        vmname: info.name,
+                        autoShow: true,
+                    });
+                },
+            },
+            { xtype: 'menuseparator' },
             {
                 text: gettext('Console'),
                 iconCls: 'fa fa-fw fa-terminal',
diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
index adf64672..3e51be8f 100644
--- a/www/manager6/qemu/CmdMenu.js
+++ b/www/manager6/qemu/CmdMenu.js
@@ -169,6 +169,52 @@ Ext.define('PVE.qemu.CmdMenu', {
                 },
             },
             { xtype: 'menuseparator' },
+            {
+                text: gettext('Take Snapshot'),
+                iconCls: 'fa fa-fw fa-history',
+                disabled: !caps.vms['VM.Snapshot'],
+                handler: function () {
+                    Proxmox.Utils.API2Request({
+                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
+                        params: { feature: 'snapshot' },
+                        method: 'GET',
+                        success: function (response) {
+                            if (!response.result.data.hasFeature) {
+                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')
+                                return;
+                            }
+                            Ext.create('PVE.window.Snapshot', {
+                                nodename: info.node,
+                                vmid: info.vmid,
+                                vmname: info.name,
+                                viewonly: false,
+                                type: info.type,
+                                isCreate: true,
+                                submitText: gettext('Take Snapshot'),
+                                autoShow: true,
+                                running: running,
+                            });
+                        },
+                        failure: (response, opts) =>
+                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+                    });
+                },
+            },
+            {
+                text: gettext('Backup now'),
+                iconCls: 'fa fa-fw fa-floppy-o',
+                disabled: !caps.vms['VM.Backup'],
+                handler: function () {
+                    Ext.create('PVE.window.Backup', {
+                        nodename: info.node,
+                        vmid: info.vmid,
+                        vmtype: info.type,
+                        vmname: info.name,
+                        autoShow: true,
+                    });
+                },
+            },
+            { xtype: 'menuseparator' },
             {
                 text: gettext('Console'),
                 iconCls: 'fa fa-fw fa-terminal',
-- 
2.47.3


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Shannon Sterz]

some small comments in-line

On Mon Oct 6, 2025 at 3:33 PM CEST, Nicolas Frey wrote:
> Adds snapshot and manual backup shortcut to VM/CT right-click context
> menu.
>
> On the previous iteration (v2), the snapshot feature availablity was
> checked on menu load. It is now called only when the snapshot button
> is clicked, preventing delays in loading the context menu. Testing
> with simulated latency (with setTimeout) showed better UX by
> displaying an error on click rather than preloading the check.
>
> Previously, users couldn't distinguish between a slow API and the
> feature being disabled. The explicit check provides clear feedback.
> The 'disabled' property remains bound to VM.Snapshot to disable
> the option when snapshots aren't allowed anyway.
>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6209
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>  www/manager6/lxc/CmdMenu.js  | 46 ++++++++++++++++++++++++++++++++++++
>  www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
>
> diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
> index cd60c967..f0e8f700 100644
> --- a/www/manager6/lxc/CmdMenu.js
> +++ b/www/manager6/lxc/CmdMenu.js
> @@ -134,6 +134,52 @@ Ext.define('PVE.lxc.CmdMenu', {
>                  },
>              },
>              { xtype: 'menuseparator' },
> +            {
> +                text: gettext('Take Snapshot'),
> +                iconCls: 'fa fa-fw fa-history',
> +                disabled: !caps.vms['VM.Snapshot'],
> +                handler: function () {
> +                    Proxmox.Utils.API2Request({
> +                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
> +                        params: { feature: 'snapshot' },
> +                        method: 'GET',
> +                        success: function (response) {

nit: you are mixing arrow style functions with anonymous functions here,
imo this should also just be:

(response) => {
    [..]
}

> +                            if (!response.result.data.hasFeature) {
> +                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')

nit: did you run `make tidy`? for me it cleans the line above up to:

                                Ext.Msg.alert(
                                    gettext('Error'),
                                    'Cannot take snapshot: Feature not enabled.',
                                );

> +                                return;
> +                            }
> +                            Ext.create('PVE.window.Snapshot', {
> +                                nodename: info.node,
> +                                vmid: info.vmid,
> +                                vmname: info.name,
> +                                viewonly: false,
> +                                type: info.type,
> +                                isCreate: true,
> +                                submitText: gettext('Take Snapshot'),
> +                                autoShow: true,
> +                                running: running,
> +                            });
> +                        },
> +                        failure: (response, opts) =>
> +                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> +                    });
> +                },
> +            },
> +            {
> +                text: gettext('Backup now'),
> +                iconCls: 'fa fa-fw fa-floppy-o',
> +                disabled: !caps.vms['VM.Backup'],
> +                handler: function () {

see comment about arrow function above

> +                    Ext.create('PVE.window.Backup', {
> +                        nodename: info.node,
> +                        vmid: info.vmid,
> +                        vmtype: info.type,
> +                        vmname: info.name,
> +                        autoShow: true,
> +                    });
> +                },
> +            },
> +            { xtype: 'menuseparator' },
>              {
>                  text: gettext('Console'),
>                  iconCls: 'fa fa-fw fa-terminal',
> diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
> index adf64672..3e51be8f 100644
> --- a/www/manager6/qemu/CmdMenu.js
> +++ b/www/manager6/qemu/CmdMenu.js
> @@ -169,6 +169,52 @@ Ext.define('PVE.qemu.CmdMenu', {
>                  },
>              },
>              { xtype: 'menuseparator' },
> +            {
> +                text: gettext('Take Snapshot'),
> +                iconCls: 'fa fa-fw fa-history',
> +                disabled: !caps.vms['VM.Snapshot'],
> +                handler: function () {
> +                    Proxmox.Utils.API2Request({
> +                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
> +                        params: { feature: 'snapshot' },
> +                        method: 'GET',
> +                        success: function (response) {

see comment about arrow function above

> +                            if (!response.result.data.hasFeature) {
> +                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')

see comment about `make tidy` above

> +                                return;
> +                            }
> +                            Ext.create('PVE.window.Snapshot', {
> +                                nodename: info.node,
> +                                vmid: info.vmid,
> +                                vmname: info.name,
> +                                viewonly: false,
> +                                type: info.type,
> +                                isCreate: true,
> +                                submitText: gettext('Take Snapshot'),
> +                                autoShow: true,
> +                                running: running,
> +                            });
> +                        },
> +                        failure: (response, opts) =>
> +                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> +                    });
> +                },
> +            },
> +            {
> +                text: gettext('Backup now'),
> +                iconCls: 'fa fa-fw fa-floppy-o',
> +                disabled: !caps.vms['VM.Backup'],
> +                handler: function () {

see comment about arrow function above

> +                    Ext.create('PVE.window.Backup', {
> +                        nodename: info.node,
> +                        vmid: info.vmid,
> +                        vmtype: info.type,
> +                        vmname: info.name,
> +                        autoShow: true,
> +                    });
> +                },
> +            },
> +            { xtype: 'menuseparator' },
>              {
>                  text: gettext('Console'),
>                  iconCls: 'fa fa-fw fa-terminal',

one small thought: have you explored whether querying the features of
the guest when opening the context menu and graying out the snapshot
option is viable? imo that would be a nicer user experience, but the
overhead of querying the backend there might be too much.

other than that:

Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Shannon Sterz]

On Tue Oct 7, 2025 at 10:42 AM CEST, Shannon Sterz wrote:
> some small comments in-line
>
> On Mon Oct 6, 2025 at 3:33 PM CEST, Nicolas Frey wrote:
>> Adds snapshot and manual backup shortcut to VM/CT right-click context
>> menu.
>>
>> On the previous iteration (v2), the snapshot feature availablity was
>> checked on menu load. It is now called only when the snapshot button
>> is clicked, preventing delays in loading the context menu. Testing
>> with simulated latency (with setTimeout) showed better UX by
>> displaying an error on click rather than preloading the check.
>>
>> Previously, users couldn't distinguish between a slow API and the
>> feature being disabled. The explicit check provides clear feedback.
>> The 'disabled' property remains bound to VM.Snapshot to disable
>> the option when snapshots aren't allowed anyway.
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6209
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>>  www/manager6/lxc/CmdMenu.js  | 46 ++++++++++++++++++++++++++++++++++++
>>  www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
>> index cd60c967..f0e8f700 100644
>> --- a/www/manager6/lxc/CmdMenu.js
>> +++ b/www/manager6/lxc/CmdMenu.js
>> @@ -134,6 +134,52 @@ Ext.define('PVE.lxc.CmdMenu', {
>>                  },
>>              },
>>              { xtype: 'menuseparator' },
>> +            {
>> +                text: gettext('Take Snapshot'),
>> +                iconCls: 'fa fa-fw fa-history',
>> +                disabled: !caps.vms['VM.Snapshot'],
>> +                handler: function () {
>> +                    Proxmox.Utils.API2Request({
>> +                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
>> +                        params: { feature: 'snapshot' },
>> +                        method: 'GET',
>> +                        success: function (response) {
>
> nit: you are mixing arrow style functions with anonymous functions here,
> imo this should also just be:
>
> (response) => {
>     [..]
> }
>
>> +                            if (!response.result.data.hasFeature) {
>> +                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')
>
> nit: did you run `make tidy`? for me it cleans the line above up to:
>
>                                 Ext.Msg.alert(
>                                     gettext('Error'),
>                                     'Cannot take snapshot: Feature not enabled.',
>                                 );
>
>> +                                return;
>> +                            }
>> +                            Ext.create('PVE.window.Snapshot', {
>> +                                nodename: info.node,
>> +                                vmid: info.vmid,
>> +                                vmname: info.name,
>> +                                viewonly: false,
>> +                                type: info.type,
>> +                                isCreate: true,
>> +                                submitText: gettext('Take Snapshot'),
>> +                                autoShow: true,
>> +                                running: running,
>> +                            });
>> +                        },
>> +                        failure: (response, opts) =>
>> +                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
>> +                    });
>> +                },
>> +            },
>> +            {
>> +                text: gettext('Backup now'),
>> +                iconCls: 'fa fa-fw fa-floppy-o',
>> +                disabled: !caps.vms['VM.Backup'],
>> +                handler: function () {
>
> see comment about arrow function above
>
>> +                    Ext.create('PVE.window.Backup', {
>> +                        nodename: info.node,
>> +                        vmid: info.vmid,
>> +                        vmtype: info.type,
>> +                        vmname: info.name,
>> +                        autoShow: true,
>> +                    });
>> +                },
>> +            },
>> +            { xtype: 'menuseparator' },
>>              {
>>                  text: gettext('Console'),
>>                  iconCls: 'fa fa-fw fa-terminal',
>> diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
>> index adf64672..3e51be8f 100644
>> --- a/www/manager6/qemu/CmdMenu.js
>> +++ b/www/manager6/qemu/CmdMenu.js
>> @@ -169,6 +169,52 @@ Ext.define('PVE.qemu.CmdMenu', {
>>                  },
>>              },
>>              { xtype: 'menuseparator' },
>> +            {
>> +                text: gettext('Take Snapshot'),
>> +                iconCls: 'fa fa-fw fa-history',
>> +                disabled: !caps.vms['VM.Snapshot'],
>> +                handler: function () {
>> +                    Proxmox.Utils.API2Request({
>> +                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
>> +                        params: { feature: 'snapshot' },
>> +                        method: 'GET',
>> +                        success: function (response) {
>
> see comment about arrow function above
>
>> +                            if (!response.result.data.hasFeature) {
>> +                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')
>
> see comment about `make tidy` above
>
>> +                                return;
>> +                            }
>> +                            Ext.create('PVE.window.Snapshot', {
>> +                                nodename: info.node,
>> +                                vmid: info.vmid,
>> +                                vmname: info.name,
>> +                                viewonly: false,
>> +                                type: info.type,
>> +                                isCreate: true,
>> +                                submitText: gettext('Take Snapshot'),
>> +                                autoShow: true,
>> +                                running: running,
>> +                            });
>> +                        },
>> +                        failure: (response, opts) =>
>> +                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
>> +                    });
>> +                },
>> +            },
>> +            {
>> +                text: gettext('Backup now'),
>> +                iconCls: 'fa fa-fw fa-floppy-o',
>> +                disabled: !caps.vms['VM.Backup'],
>> +                handler: function () {
>
> see comment about arrow function above
>
>> +                    Ext.create('PVE.window.Backup', {
>> +                        nodename: info.node,
>> +                        vmid: info.vmid,
>> +                        vmtype: info.type,
>> +                        vmname: info.name,
>> +                        autoShow: true,
>> +                    });
>> +                },
>> +            },
>> +            { xtype: 'menuseparator' },
>>              {
>>                  text: gettext('Console'),
>>                  iconCls: 'fa fa-fw fa-terminal',
>
> one small thought: have you explored whether querying the features of
> the guest when opening the context menu and graying out the snapshot
> option is viable? imo that would be a nicer user experience, but the
> overhead of querying the backend there might be too much.

just saw thomas' review of v2 of this series, so nevermind that. sorry
for the noise.

> other than that:
>
> Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Nicolas Frey]

thanks for the review!

On 10/7/25 10:42 AM, Shannon Sterz wrote:
> some small comments in-line
> 
> On Mon Oct 6, 2025 at 3:33 PM CEST, Nicolas Frey wrote:
>> Adds snapshot and manual backup shortcut to VM/CT right-click context
>> menu.
>>
>> On the previous iteration (v2), the snapshot feature availablity was
>> checked on menu load. It is now called only when the snapshot button
>> is clicked, preventing delays in loading the context menu. Testing
>> with simulated latency (with setTimeout) showed better UX by
>> displaying an error on click rather than preloading the check.
>>
>> Previously, users couldn't distinguish between a slow API and the
>> feature being disabled. The explicit check provides clear feedback.
>> The 'disabled' property remains bound to VM.Snapshot to disable
>> the option when snapshots aren't allowed anyway.
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6209
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>>  www/manager6/lxc/CmdMenu.js  | 46 ++++++++++++++++++++++++++++++++++++
>>  www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
>> index cd60c967..f0e8f700 100644
>> --- a/www/manager6/lxc/CmdMenu.js
>> +++ b/www/manager6/lxc/CmdMenu.js
>> @@ -134,6 +134,52 @@ Ext.define('PVE.lxc.CmdMenu', {
>>                  },
>>              },
>>              { xtype: 'menuseparator' },
>> +            {
>> +                text: gettext('Take Snapshot'),
>> +                iconCls: 'fa fa-fw fa-history',
>> +                disabled: !caps.vms['VM.Snapshot'],
>> +                handler: function () {
>> +                    Proxmox.Utils.API2Request({
>> +                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
>> +                        params: { feature: 'snapshot' },
>> +                        method: 'GET',
>> +                        success: function (response) {
> 
> nit: you are mixing arrow style functions with anonymous functions here,
> imo this should also just be:
> 
> (response) => {
>     [..]
> }

will change them all to anonymous functions in a v4

> 
>> +                            if (!response.result.data.hasFeature) {
>> +                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')
> 
> nit: did you run `make tidy`? for me it cleans the line above up to:

forgot to do so, thanks for pointing that out!

> 
>                                 Ext.Msg.alert(
>                                     gettext('Error'),
>                                     'Cannot take snapshot: Feature not enabled.',
>                                 );
> 
>> +                                return;
>> +                            }
>> +                            Ext.create('PVE.window.Snapshot', {
>> +                                nodename: info.node,
>> +                                vmid: info.vmid,
>> +                                vmname: info.name,
>> +                                viewonly: false,
>> +                                type: info.type,
>> +                                isCreate: true,
>> +                                submitText: gettext('Take Snapshot'),
>> +                                autoShow: true,
>> +                                running: running,
>> +                            });
>> +                        },
>> +                        failure: (response, opts) =>
>> +                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
>> +                    });
>> +                },
>> +            },
>> +            {
>> +                text: gettext('Backup now'),
>> +                iconCls: 'fa fa-fw fa-floppy-o',
>> +                disabled: !caps.vms['VM.Backup'],
>> +                handler: function () {
> 
> see comment about arrow function above
> 
>> +                    Ext.create('PVE.window.Backup', {
>> +                        nodename: info.node,
>> +                        vmid: info.vmid,
>> +                        vmtype: info.type,
>> +                        vmname: info.name,
>> +                        autoShow: true,
>> +                    });
>> +                },
>> +            },
>> +            { xtype: 'menuseparator' },
>>              {
>>                  text: gettext('Console'),
>>                  iconCls: 'fa fa-fw fa-terminal',
>> diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
>> index adf64672..3e51be8f 100644
>> --- a/www/manager6/qemu/CmdMenu.js
>> +++ b/www/manager6/qemu/CmdMenu.js
>> @@ -169,6 +169,52 @@ Ext.define('PVE.qemu.CmdMenu', {
>>                  },
>>              },
>>              { xtype: 'menuseparator' },
>> +            {
>> +                text: gettext('Take Snapshot'),
>> +                iconCls: 'fa fa-fw fa-history',
>> +                disabled: !caps.vms['VM.Snapshot'],
>> +                handler: function () {
>> +                    Proxmox.Utils.API2Request({
>> +                        url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
>> +                        params: { feature: 'snapshot' },
>> +                        method: 'GET',
>> +                        success: function (response) {
> 
> see comment about arrow function above
> 
>> +                            if (!response.result.data.hasFeature) {
>> +                                Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.')
> 
> see comment about `make tidy` above
> 
>> +                                return;
>> +                            }
>> +                            Ext.create('PVE.window.Snapshot', {
>> +                                nodename: info.node,
>> +                                vmid: info.vmid,
>> +                                vmname: info.name,
>> +                                viewonly: false,
>> +                                type: info.type,
>> +                                isCreate: true,
>> +                                submitText: gettext('Take Snapshot'),
>> +                                autoShow: true,
>> +                                running: running,
>> +                            });
>> +                        },
>> +                        failure: (response, opts) =>
>> +                            Ext.Msg.alert(gettext('Error'), response.htmlStatus),
>> +                    });
>> +                },
>> +            },
>> +            {
>> +                text: gettext('Backup now'),
>> +                iconCls: 'fa fa-fw fa-floppy-o',
>> +                disabled: !caps.vms['VM.Backup'],
>> +                handler: function () {
> 
> see comment about arrow function above
> 
>> +                    Ext.create('PVE.window.Backup', {
>> +                        nodename: info.node,
>> +                        vmid: info.vmid,
>> +                        vmtype: info.type,
>> +                        vmname: info.name,
>> +                        autoShow: true,
>> +                    });
>> +                },
>> +            },
>> +            { xtype: 'menuseparator' },
>>              {
>>                  text: gettext('Console'),
>>                  iconCls: 'fa fa-fw fa-terminal',
> 
> one small thought: have you explored whether querying the features of
> the guest when opening the context menu and graying out the snapshot
> option is viable? imo that would be a nicer user experience, but the
> overhead of querying the backend there might be too much.
that was discussed (accidentally) off list but now has been re-sent by
Thomas [0]. That was the previous approach and it did seem like quite
a bit of overhead for just the context menu. IMO it is also better
experience to not have to wait until a button is enabled (as would be
the case with very high latency). I would rather wait/get an error
*after* clicking.

[0]
https://lore.proxmox.com/pve-devel/9563cc43-f8ad-4b33-b8ea-a22d22b3405e@proxmox.com/T/#u

> 
> other than that:
> 
> Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Thomas Lamprecht]

Am 07.10.25 um 10:42 schrieb Shannon Sterz:
> one small thought: have you explored whether querying the features of
> the guest when opening the context menu and graying out the snapshot
> option is viable? imo that would be a nicer user experience, but the
> overhead of querying the backend there might be too much.

FWIW, Nicolas did that in v1/v2, but I wondered w.r.t. slow backend
or spotty and/or high-latency connection making this odd to use.
But I just found that my reply was not CC'ing the list, so I just
re-send it for the record:
https://lore.proxmox.com/pve-devel/9563cc43-f8ad-4b33-b8ea-a22d22b3405e@proxmox.com/

That said, given that you suggested the other way around for UX, it
might be indeed better to keep it that way, or at least actually try
how that way works with a slow backend (developer tools can simulate
slow network (the chromium based ones are a bit more powerful in that
regard IIRC), or alternatively use a traffic control (tc) netem qdisc
that adds latency [0] can be added on the PVE server.
Because as mentioned, it could be fine your previous way and the one
Shannon also would prefer, I rather wanted to avoid that we just take
our rather perfect fast and sub-milliseconds lab environments as sole
base for UX decisions.

[0]: https://manpages.debian.org/trixie/iproute2/tc-netem.8.en.html
     e.g. something like: tc qdisc add dev eth0 root netem delay 3000ms


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Nicolas Frey]

On 10/7/25 11:16 AM, Thomas Lamprecht wrote:
> Am 07.10.25 um 10:42 schrieb Shannon Sterz:
>> one small thought: have you explored whether querying the features of
>> the guest when opening the context menu and graying out the snapshot
>> option is viable? imo that would be a nicer user experience, but the
>> overhead of querying the backend there might be too much.
> 
> FWIW, Nicolas did that in v1/v2, but I wondered w.r.t. slow backend
> or spotty and/or high-latency connection making this odd to use.
> But I just found that my reply was not CC'ing the list, so I just
> re-send it for the record:
> https://lore.proxmox.com/pve-devel/9563cc43-f8ad-4b33-b8ea-a22d22b3405e@proxmox.com/
> 
> That said, given that you suggested the other way around for UX, it
> might be indeed better to keep it that way, or at least actually try
> how that way works with a slow backend (developer tools can simulate
> slow network (the chromium based ones are a bit more powerful in that
> regard IIRC), or alternatively use a traffic control (tc) netem qdisc
> that adds latency [0] can be added on the PVE server.
> Because as mentioned, it could be fine your previous way and the one
> Shannon also would prefer, I rather wanted to avoid that we just take
> our rather perfect fast and sub-milliseconds lab environments as sole
> base for UX decisions.
> 
> [0]: https://manpages.debian.org/trixie/iproute2/tc-netem.8.en.html
>      e.g. something like: tc qdisc add dev eth0 root netem delay 3000ms


I used some variations of added network throttle using chrome dev
tools and traffic control and noticed that the disabling of the button
is starting to get really noticable in the 1-1.5 second range (if you
don't have sniper level mouse accuracy). Anything below that is
negotiable, but if the request stalls even a bit above that range it
could become an annoyance (e.g. being just about too early and having
to click a second time or simply the long wait). Though similarly, you
still have to wait for the window to appear if you clicked it in the
new version, and there may also be an error if snapshots aren't
supported on the guest.

As a note, the original implementation (v1/v2) matches the behaviour
found in the Snapshots tab, which has the button disabled until
querying snapshot features is complete. This also shows that the
`current guest does not support taking new snapshots` until fully
loaded. IMO parity between them is essential, as to not have two
different approaches which might confuse users.

I cannot tell which approach is better here, but it might be a sign
that Shannon had also suggested the other way...


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Thomas Lamprecht]

Am 07.10.25 um 12:34 schrieb Nicolas Frey:
> I used some variations of added network throttle using chrome dev
> tools and traffic control and noticed that the disabling of the button
> is starting to get really noticable in the 1-1.5 second range (if you
> don't have sniper level mouse accuracy). Anything below that is
> negotiable, but if the request stalls even a bit above that range it
> could become an annoyance (e.g. being just about too early and having
> to click a second time or simply the long wait). Though similarly, you
> still have to wait for the window to appear if you clicked it in the
> new version, and there may also be an error if snapshots aren't
> supported on the guest.

Thanks for checking so closely and reporting back!

> As a note, the original implementation (v1/v2) matches the behaviour
> found in the Snapshots tab, which has the button disabled until
> querying snapshot features is complete. This also shows that the
> `current guest does not support taking new snapshots` until fully
> loaded. IMO parity between them is essential, as to not have two
> different approaches which might confuse users.
> 
> I cannot tell which approach is better here, but it might be a sign
> that Shannon had also suggested the other way...

Yeah, that, and this being basically pre-existing behavior in the 
Snapshots tab, like you mentioned, is probably enough weight to tip the
scale towards your original approach.

Thanks for trying this out though, sometimes some experimentation is
needed, and even though it's only a small UX thing, these things tend to
get copied and thus leak often quite fast in other parts of the UI or
even UIs for our other projects, so it  can be worth to look more
closely once in a while here.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager v3 0/1] ui: fix #6209: create snapshots and backups from context menu

[Nicolas Frey]

Superseded-by: https://lore.proxmox.com/pve-devel/20251007121352.102236-1-n.frey@proxmox.com/T/#u

On 10/6/25 3:32 PM, Nicolas Frey wrote:
> The original feature request [0] only specified the ability to take
> snapshots. I added the manual backup shortcut as well, since
> navigating through the menu for this action felt unnecessarily
> cumbersome when performing just a quick backup.
> 
> Changes since v2:
> * rebased
> * use autoShow on snapshot and backup windows
> * improve UX and account for slow connections by only making API
>   calls for snapshot feature on button click
> 
> changes since v1, thanks @Fiona Ebner
> * clarified commit message
> * addressed several issues in the added menu options, namely:
>   * correctly handle disabled state with async
>   * fixed negation logic and cleaned up minor nits
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=6209
> 
> Nicolas Frey (1):
>   ui: fix #6209: create snapshots and backups from context menu
> 
>  www/manager6/lxc/CmdMenu.js  | 46 ++++++++++++++++++++++++++++++++++++
>  www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel