* [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog
@ 2024-10-29 10:39 Timothy Nicholson
2024-10-29 12:22 ` Christoph Heiss
0 siblings, 1 reply; 4+ messages in thread
From: Timothy Nicholson @ 2024-10-29 10:39 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
---
changes since v1:
- modified function signature
- removed unnecessary questionmark
www/manager6/Utils.js | 4 ++++
| 4 ++--
www/manager6/lxc/Config.js | 8 +++++---
| 4 ++--
www/manager6/qemu/Config.js | 14 ++++++++------
www/manager6/window/GuestStop.js | 2 +-
6 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index db86fa9a..493a635f 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1960,6 +1960,10 @@ Ext.define('PVE.Utils', {
}
return languageCookie || Proxmox.defaultLang || 'en';
},
+
+ format_guest_confirmation: function(taskType, vmid, guestName) {
+ return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);
+ },
},
singleton: true,
--git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
index 76b39423..80b1d0d0 100644
--- a/www/manager6/lxc/CmdMenu.js
+++ b/www/manager6/lxc/CmdMenu.js
@@ -22,7 +22,7 @@ Ext.define('PVE.lxc.CmdMenu', {
});
};
let confirmedVMCommand = (cmd, params) => {
- let msg = Proxmox.Utils.format_task_description(`vz${cmd}`, info.vmid);
+ let msg = PVE.Utils.format_guest_confirmation(`vz${cmd}`, info.vmid, info.name);
Ext.Msg.confirm(gettext('Confirm'), msg, btn => {
if (btn === 'yes') {
vm_command(cmd, params);
@@ -108,7 +108,7 @@ Ext.define('PVE.lxc.CmdMenu', {
text: gettext('Convert to template'),
iconCls: 'fa fa-fw fa-file-o',
handler: function() {
- let msg = Proxmox.Utils.format_task_description('vztemplate', info.vmid);
+ let msg = PVE.Utils.format_guest_confirmation('vztemplate', info.vmid, info.name);
Ext.Msg.confirm(gettext('Confirm'), msg, function(btn) {
if (btn === 'yes') {
Proxmox.Utils.API2Request({
diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index d0e40fc4..a15ff815 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -20,6 +20,8 @@ Ext.define('PVE.lxc.Config', {
throw "no VM ID specified";
}
+ var vmname = vm.name;
+
var template = !!vm.template;
var running = !!vm.uptime;
@@ -59,7 +61,7 @@ Ext.define('PVE.lxc.Config', {
text: gettext('Shutdown'),
disabled: !caps.vms['VM.PowerMgmt'] || !running,
hidden: template,
- confirmMsg: Proxmox.Utils.format_task_description('vzshutdown', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('vzshutdown', vmid, vmname),
handler: function() {
vm_command('shutdown');
},
@@ -67,7 +69,7 @@ Ext.define('PVE.lxc.Config', {
items: [{
text: gettext('Reboot'),
disabled: !caps.vms['VM.PowerMgmt'],
- confirmMsg: Proxmox.Utils.format_task_description('vzreboot', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('vzreboot', vmid, vmname),
tooltip: Ext.String.format(gettext('Reboot {0}'), 'CT'),
handler: function() {
vm_command("reboot");
@@ -124,7 +126,7 @@ Ext.define('PVE.lxc.Config', {
xtype: 'pveMenuItem',
iconCls: 'fa fa-fw fa-file-o',
hidden: !caps.vms['VM.Allocate'],
- confirmMsg: Proxmox.Utils.format_task_description('vztemplate', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('vztemplate', vmid, vmname),
handler: function() {
Proxmox.Utils.API2Request({
url: base_url + '/template',
--git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
index 834577e7..95129dfc 100644
--- a/www/manager6/qemu/CmdMenu.js
+++ b/www/manager6/qemu/CmdMenu.js
@@ -23,7 +23,7 @@ Ext.define('PVE.qemu.CmdMenu', {
};
let confirmedVMCommand = (cmd, params, confirmTask) => {
let task = confirmTask || `qm${cmd}`;
- let msg = Proxmox.Utils.format_task_description(task, info.vmid);
+ let msg = PVE.Utils.format_guest_confirmation(task, info.vmid, info.name);
Ext.Msg.confirm(gettext('Confirm'), msg, btn => {
if (btn === 'yes') {
vm_command(cmd, params);
@@ -136,7 +136,7 @@ Ext.define('PVE.qemu.CmdMenu', {
iconCls: 'fa fa-fw fa-file-o',
hidden: !caps.vms['VM.Allocate'],
handler: function() {
- let msg = Proxmox.Utils.format_task_description('qmtemplate', info.vmid);
+ let msg = PVE.Utils.format_guest_confirmation('qmtemplate', info.vmid, info.name);
Ext.Msg.confirm(gettext('Confirm'), msg, btn => {
if (btn === 'yes') {
Proxmox.Utils.API2Request({
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index f28ee67b..1a9d1ba9 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -19,6 +19,8 @@ Ext.define('PVE.qemu.Config', {
throw "no VM ID specified";
}
+ var vmname = vm.name;
+
var template = !!vm.template;
var running = !!vm.uptime;
@@ -97,7 +99,7 @@ Ext.define('PVE.qemu.Config', {
xtype: 'pveMenuItem',
iconCls: 'fa fa-fw fa-file-o',
hidden: !caps.vms['VM.Allocate'],
- confirmMsg: Proxmox.Utils.format_task_description('qmtemplate', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('qmtemplate', vmid, vmname),
handler: function() {
Proxmox.Utils.API2Request({
url: base_url + '/template',
@@ -142,7 +144,7 @@ Ext.define('PVE.qemu.Config', {
text: gettext('Shutdown'),
disabled: !caps.vms['VM.PowerMgmt'] || !running,
hidden: template,
- confirmMsg: Proxmox.Utils.format_task_description('qmshutdown', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('qmshutdown', vmid, vmname),
handler: function() {
vm_command('shutdown');
},
@@ -151,7 +153,7 @@ Ext.define('PVE.qemu.Config', {
text: gettext('Reboot'),
disabled: !caps.vms['VM.PowerMgmt'],
tooltip: Ext.String.format(gettext('Shutdown, apply pending changes and reboot {0}'), 'VM'),
- confirmMsg: Proxmox.Utils.format_task_description('qmreboot', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('qmreboot', vmid, vmname),
handler: function() {
vm_command("reboot");
},
@@ -159,7 +161,7 @@ Ext.define('PVE.qemu.Config', {
}, {
text: gettext('Pause'),
disabled: !caps.vms['VM.PowerMgmt'],
- confirmMsg: Proxmox.Utils.format_task_description('qmpause', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('qmpause', vmid, vmname),
handler: function() {
vm_command("suspend");
},
@@ -167,7 +169,7 @@ Ext.define('PVE.qemu.Config', {
}, {
text: gettext('Hibernate'),
disabled: !caps.vms['VM.PowerMgmt'],
- confirmMsg: Proxmox.Utils.format_task_description('qmsuspend', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('qmsuspend', vmid, vmname),
tooltip: gettext('Suspend to disk'),
handler: function() {
vm_command("suspend", { todisk: 1 });
@@ -189,7 +191,7 @@ Ext.define('PVE.qemu.Config', {
text: gettext('Reset'),
disabled: !caps.vms['VM.PowerMgmt'],
tooltip: Ext.String.format(gettext('Reset {0} immediately'), 'VM'),
- confirmMsg: Proxmox.Utils.format_task_description('qmreset', vmid),
+ confirmMsg: PVE.Utils.format_guest_confirmation('qmreset', vmid, vmname),
handler: function() {
vm_command("reset");
},
diff --git a/www/manager6/window/GuestStop.js b/www/manager6/window/GuestStop.js
index fbd45c7a..aa020584 100644
--- a/www/manager6/window/GuestStop.js
+++ b/www/manager6/window/GuestStop.js
@@ -68,7 +68,7 @@ Ext.define('PVE.GuestStop', {
let cfg = {
title: gettext('Confirm'),
icon: Ext.Msg.WARNING,
- msg: Proxmox.Utils.format_task_description(me.taskType, me.vm.vmid),
+ msg: PVE.Utils.format_guest_confirmation(me.taskType, me.vm.vmid, me.vm.name),
buttons: Ext.Msg.YESNO,
callback: btn => me.handler(btn),
};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog
2024-10-29 10:39 [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog Timothy Nicholson
@ 2024-10-29 12:22 ` Christoph Heiss
2024-10-29 12:53 ` Shannon Sterz
2024-10-30 8:30 ` Thomas Lamprecht
0 siblings, 2 replies; 4+ messages in thread
From: Christoph Heiss @ 2024-10-29 12:22 UTC (permalink / raw)
To: Timothy Nicholson; +Cc: Proxmox VE development discussion
The patch title implies that it only adds a utility function, but the
patch itself also includes usage for it. Something like
"fix #5787: ui: include guest name in confirmation dialog"
or similar would probably be a better fit.
On Tue, Oct 29, 2024 at 11:39:56AM GMT, Timothy Nicholson wrote:
> Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
> ---
> changes since v1:
> - modified function signature
> - removed unnecessary questionmark
Link to the previous version on lore.proxmox.com is always nice to have
in the notes, FWIW. But no hard feelings, just pure convenience if one
wants to look at previous versions :^)
>
> www/manager6/Utils.js | 4 ++++
> www/manager6/lxc/CmdMenu.js | 4 ++--
> www/manager6/lxc/Config.js | 8 +++++---
> www/manager6/qemu/CmdMenu.js | 4 ++--
> www/manager6/qemu/Config.js | 14 ++++++++------
> www/manager6/window/GuestStop.js | 2 +-
> 6 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index db86fa9a..493a635f 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1960,6 +1960,10 @@ Ext.define('PVE.Utils', {
> }
> return languageCookie || Proxmox.defaultLang || 'en';
> },
> +
> + format_guest_confirmation: function(taskType, vmid, guestName) {
Maybe name it `format_guest_task_confirmation`, to really tie it to
the task functionality?
> + return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);
> + },
> },
>
> singleton: true,
> [..]
> diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
> index d0e40fc4..a15ff815 100644
> --- a/www/manager6/lxc/Config.js
> +++ b/www/manager6/lxc/Config.js
> @@ -20,6 +20,8 @@ Ext.define('PVE.lxc.Config', {
> throw "no VM ID specified";
> }
>
> + var vmname = vm.name;
> +
Any particular reason for this variable binding? Using `vm.name` below
directly would work too, if I'm not overlooking something. It's not
really shorter anyway.
In any case, please use `const` (or `let` if needed) for new variable
declaration instead of `var`.
> var template = !!vm.template;
>
> var running = !!vm.uptime;
> [..]
> diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
> index f28ee67b..1a9d1ba9 100644
> --- a/www/manager6/qemu/Config.js
> +++ b/www/manager6/qemu/Config.js
> @@ -19,6 +19,8 @@ Ext.define('PVE.qemu.Config', {
> throw "no VM ID specified";
> }
>
> + var vmname = vm.name;
> +
Same as above.
> var template = !!vm.template;
>
> var running = !!vm.uptime;
> [..]
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog
2024-10-29 12:22 ` Christoph Heiss
@ 2024-10-29 12:53 ` Shannon Sterz
2024-10-30 8:30 ` Thomas Lamprecht
1 sibling, 0 replies; 4+ messages in thread
From: Shannon Sterz @ 2024-10-29 12:53 UTC (permalink / raw)
To: Proxmox VE development discussion, Timothy Nicholson
On Tue Oct 29, 2024 at 1:22 PM CET, Christoph Heiss wrote:
> The patch title implies that it only adds a utility function, but the
> patch itself also includes usage for it. Something like
>
> "fix #5787: ui: include guest name in confirmation dialog"
>
> or similar would probably be a better fit.
>
> On Tue, Oct 29, 2024 at 11:39:56AM GMT, Timothy Nicholson wrote:
> > Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
> > ---
> > changes since v1:
> > - modified function signature
> > - removed unnecessary questionmark
>
> Link to the previous version on lore.proxmox.com is always nice to have
> in the notes, FWIW. But no hard feelings, just pure convenience if one
> wants to look at previous versions :^)
>
> >
> > www/manager6/Utils.js | 4 ++++
> > www/manager6/lxc/CmdMenu.js | 4 ++--
> > www/manager6/lxc/Config.js | 8 +++++---
> > www/manager6/qemu/CmdMenu.js | 4 ++--
> > www/manager6/qemu/Config.js | 14 ++++++++------
> > www/manager6/window/GuestStop.js | 2 +-
> > 6 files changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> > index db86fa9a..493a635f 100644
> > --- a/www/manager6/Utils.js
> > +++ b/www/manager6/Utils.js
> > @@ -1960,6 +1960,10 @@ Ext.define('PVE.Utils', {
> > }
> > return languageCookie || Proxmox.defaultLang || 'en';
> > },
> > +
> > + format_guest_confirmation: function(taskType, vmid, guestName) {
>
> Maybe name it `format_guest_task_confirmation`, to really tie it to
> the task functionality?
>
just chiming in here as a reminder that our js guidelines want you to
use camelCase for new variables/functions [1].
[1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
> > + return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);
> > + },
> > },
> >
> > singleton: true,
> > [..]
> > diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
> > index d0e40fc4..a15ff815 100644
> > --- a/www/manager6/lxc/Config.js
> > +++ b/www/manager6/lxc/Config.js
> > @@ -20,6 +20,8 @@ Ext.define('PVE.lxc.Config', {
> > throw "no VM ID specified";
> > }
> >
> > + var vmname = vm.name;
> > +
>
> Any particular reason for this variable binding? Using `vm.name` below
> directly would work too, if I'm not overlooking something. It's not
> really shorter anyway.
>
> In any case, please use `const` (or `let` if needed) for new variable
> declaration instead of `var`.
>
> > var template = !!vm.template;
> >
> > var running = !!vm.uptime;
> > [..]
> > diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
> > index f28ee67b..1a9d1ba9 100644
> > --- a/www/manager6/qemu/Config.js
> > +++ b/www/manager6/qemu/Config.js
> > @@ -19,6 +19,8 @@ Ext.define('PVE.qemu.Config', {
> > throw "no VM ID specified";
> > }
> >
> > + var vmname = vm.name;
> > +
>
> Same as above.
>
> > var template = !!vm.template;
> >
> > var running = !!vm.uptime;
> > [..]
>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog
2024-10-29 12:22 ` Christoph Heiss
2024-10-29 12:53 ` Shannon Sterz
@ 2024-10-30 8:30 ` Thomas Lamprecht
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2024-10-30 8:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Christoph Heiss, Timothy Nicholson
Am 29/10/2024 um 13:22 schrieb Christoph Heiss:
> The patch title implies that it only adds a utility function, but the
> patch itself also includes usage for it. Something like
>
> "fix #5787: ui: include guest name in confirmation dialog"
>
> or similar would probably be a better fit.
Yes, that's basically what I suggested in my v1 review:
> fix #5787: ui: display guest name in confirm dialogs
-- https://lore.proxmox.com/pve-devel/87323348-4002-463b-9d6f-48db6eac6d8b@proxmox.com/
Please take advice seriously, it does not need to be followed 1:1, and
sometimes might have been even wrong, but in such a case one should comment
on why that was the case, either in the commit message if it's for a code or
design related or in the change section of a patch for meta ones like here.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-10-30 8:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-29 10:39 [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog Timothy Nicholson
2024-10-29 12:22 ` Christoph Heiss
2024-10-29 12:53 ` Shannon Sterz
2024-10-30 8:30 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox