[pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog

[pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog

[Timothy Nicholson]

Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
---
changes since v1:
- modified function signature
- removed unnecessary questionmark

 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) {
+	return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);
+    },
 },
 
     singleton: true,
diff --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',
diff --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

Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog

[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.

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

Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog

[Shannon Sterz]

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

Re: [pve-devel] [PATCH manager v2] fix #5787: ui: add util function for guest confirmation dialog

[Thomas Lamprecht]

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