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

* [pve-devel] [PATCH pve-manager v2 2/2] ui: fix #6209: create snapshots and backups from context menu
       [not found] <89366ef9-275a-46ef-b639-9b9d056f0d8e@proxmox.com>
@ 2025-10-07  9:03 ` Thomas Lamprecht
  0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2025-10-07  9:03 UTC (permalink / raw)
  To: Proxmox VE development discussion

re-sending this previous review as I forgot to CC the list when I sent
this on 2025-10-03...

Am 02.10.25 um 17:15 schrieb Nicolas Frey:
> Adds snapshot and manual backup shortcut to VM/CT right-click context
> menu.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6209
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>  www/manager6/lxc/CmdMenu.js  | 47 ++++++++++++++++++++++++++++++++++++
>  www/manager6/qemu/CmdMenu.js | 47 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
> index cd60c967..bd5fe164 100644
> --- a/www/manager6/lxc/CmdMenu.js
> +++ b/www/manager6/lxc/CmdMenu.js
> @@ -134,6 +134,40 @@ Ext.define('PVE.lxc.CmdMenu', {
>                  },
>              },
>              { xtype: 'menuseparator' },
> +            {
> +                text: gettext('Take Snapshot'),
> +                iconCls: 'fa fa-fw fa-history',
> +                itemId: 'takeSnapshotBtn',
> +                disabled: true, // default until async check finishes
> +                handler: function () {
> +                    let win = Ext.create('PVE.window.Snapshot', {

You can use:

autoShow: true,

here and avoid the need for the extra call to .show() and thus the win
variable.

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

same w.r.t. using autoShow

> +                        nodename: info.node,
> +                        vmid: info.vmid,
> +                        vmtype: info.type,
> +                        vmname: info.name,
> +                    });
> +                    win.show();
> +                },
> +            },
> +            { xtype: 'menuseparator' },
>              {
>                  text: gettext('Console'),
>                  iconCls: 'fa fa-fw fa-terminal',
> @@ -149,5 +183,18 @@ Ext.define('PVE.lxc.CmdMenu', {
>          ];
>  
>          me.callParent();
> +
> +        Proxmox.Utils.API2Request({
> +            url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
> +            params: { feature: 'snapshot' },
> +            method: 'GET',
> +            success: function (response) {
> +                let hasFeature = !!response.result.data.hasFeature;
> +                let btn = me.down('#takeSnapshotBtn');
> +                if (btn) {
> +                    btn.setDisabled(!(caps.vms['VM.Snapshot'] && hasFeature));

You could skip doing the API call if caps.vms['VM.Snapshot'] is false as optimization,
as no point in checking in that case.

And while right-click is something a user explicitly needs to do, I'm not sure
if it's really worth doing an extra API call every time the menu is opened though,
while this is mostly iterating all volumes in the config and checking the storage
plugin, it might have a noticeable delay on any non-local PVE, especially on
spotty connections.

It's a tiny bit worse UX, but as trade-off we could only check if one clicks
this menu entry and then show a message or the like. If users complain about
UX we can still think about something else. What do you think?

FYI, I can be fine with doing it your proposed way, but for these things I'd
like to have other potential options at least explored, and having some
rationale for why it's fine to do this api call on context menu open in the
encoded in the commit message would be definitively warranted.

> +                }
> +            },
> +        });
>      },
>  });
> diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
> index adf64672..7c9080c5 100644
> --- a/www/manager6/qemu/CmdMenu.js
> +++ b/www/manager6/qemu/CmdMenu.js
> @@ -169,6 +169,40 @@ Ext.define('PVE.qemu.CmdMenu', {
>                  },
>              },
>              { xtype: 'menuseparator' },
> +            {
> +                text: gettext('Take Snapshot'),
> +                iconCls: 'fa fa-fw fa-history',
> +                itemId: 'takeSnapshotBtn',
> +                disabled: true,
> +                handler: function () {
> +                    let win = 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'),
> +                        running: running,
> +                    });
> +                    win.show();

same w.r.t. using autoShow

> +                },
> +            },
> +            {
> +                text: gettext('Backup now'),
> +                iconCls: 'fa fa-fw fa-floppy-o',
> +                disabled: !caps.vms['VM.Backup'],
> +                handler: function () {
> +                    var win = Ext.create('PVE.window.Backup', {
> +                        nodename: info.node,
> +                        vmid: info.vmid,
> +                        vmtype: info.type,
> +                        vmname: info.name,
> +                    });
> +                    win.show();

same w.r.t. using autoShow

> +                },
> +            },
> +            { xtype: 'menuseparator' },
>              {
>                  text: gettext('Console'),
>                  iconCls: 'fa fa-fw fa-terminal',
> @@ -194,5 +228,18 @@ Ext.define('PVE.qemu.CmdMenu', {
>          ];
>  
>          me.callParent();
> +
> +        Proxmox.Utils.API2Request({
> +            url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`,
> +            params: { feature: 'snapshot' },
> +            method: 'GET',
> +            success: function (response) {
> +                let hasFeature = !!response.result.data.hasFeature;
> +                let btn = me.down('#takeSnapshotBtn');
> +                if (btn) {
> +                    btn.setDisabled(!(caps.vms['VM.Snapshot'] && hasFeature));
> +                }
> +            },
> +        });
>      },
>  });

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

^ permalink raw reply	[flat|nested] 2+ messages in thread