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

[Christoph Heiss <c.heiss@proxmox.com>]

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