[pve-devel] [PATCH manager/widget-toolkit 0/2] close #3181: ui: display guest name in confirm dialogs

[pve-devel] [PATCH manager/widget-toolkit 0/2] close #3181: ui: display guest name in confirm dialogs

[Michael Köppl]

Closes #3181 [0].

The goal of this series is to extend the information displayed in the
confirmation dialogs when cloning, migrating, removing VMs or CTs by
adding the guest name in addition to the VM ID. This also unifies the
displayed information since there are multiple actions that already
include the guest name, such as reboot and shutdown [1].
This makes it easier for users to tell which VM or CT they are currently
targeting, reducing the likelihood of erroneous actions.

This affects the following actions performed on VMs/CTs:
* Remove
* Clone
* Migrate
* Snapshot
* Snapshot rollback
* Backup VM/CT from config view
* Restore VM/CT from config view

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=3181
[1] https://lore.proxmox.com/pve-devel/96d831f7-8435-412a-8205-b33f71357d0f@proxmox.com

proxmox-widget-toolkit:

Michael Köppl (1):
  close #3181: ui: add guest name to safe destroy dialog window

 src/Utils.js              | 8 ++++++--
 src/window/SafeDestroy.js | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)


pve-manager:

Michael Köppl (1):
  close #3181: ui: display guest name in confirm dialogs

 www/manager6/grid/BackupView.js   |  4 ++++
 www/manager6/lxc/CmdMenu.js       |  9 ++++++++-
 www/manager6/lxc/Config.js        | 11 +++++++++--
 www/manager6/qemu/CmdMenu.js      |  9 ++++++++-
 www/manager6/qemu/Config.js       | 11 +++++++++--
 www/manager6/tree/SnapshotTree.js |  3 +++
 www/manager6/window/Backup.js     |  3 +++
 www/manager6/window/Clone.js      |  7 ++++++-
 www/manager6/window/Migrate.js    | 10 +++++++---
 www/manager6/window/Restore.js    |  3 +++
 www/manager6/window/Snapshot.js   |  6 +++++-
 11 files changed, 65 insertions(+), 11 deletions(-)


Summary over all repositories:
  13 files changed, 73 insertions(+), 14 deletions(-)

-- 
Generated by git-murpp 0.8.0


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

[pve-devel] [PATCH widget-toolkit 1/1] close #3181: ui: add guest name to safe destroy dialog window

[Michael Köppl]

While the format_task_description function is used in other parts of the
UI, this still leaves these use cases intact. The guest name is an
optional addition in parantheses.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/Utils.js              | 8 ++++++--
 src/window/SafeDestroy.js | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/Utils.js b/src/Utils.js
index c873c85..a64d07f 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -716,7 +716,7 @@ utilities: {
 	}
     },
 
-    format_task_description: function(type, id) {
+    format_task_description: function(type, id, name) {
 	let farray = Proxmox.Utils.task_desc_table[type];
 	let text;
 	if (!farray) {
@@ -731,7 +731,11 @@ utilities: {
 	let prefix = farray[0];
 	text = farray[1];
 	if (prefix && id !== undefined) {
-	    return prefix + ' ' + id + ' - ' + text;
+	    let fullText = prefix + ' ' + id;
+	    if (name) {
+		fullText += " (" + name + ")";
+	    }
+	    return fullText + " - " + text;
 	}
 	return text;
     },
diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js
index c058465..d980a52 100644
--- a/src/window/SafeDestroy.js
+++ b/src/window/SafeDestroy.js
@@ -187,9 +187,10 @@ Ext.define('Proxmox.window.SafeDestroy', {
 	}
 
 	let taskName = me.getTaskName();
+	const itemName = me.getItem().name;
 	if (Ext.isDefined(taskName)) {
 	    me.lookupReference('messageCmp').setHtml(
-		Proxmox.Utils.format_task_description(taskName, itemId),
+		Proxmox.Utils.format_task_description(taskName, itemId, itemName),
 	    );
 	} else {
 	    throw "no task name specified";
-- 
2.39.5



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

[pve-devel] [PATCH manager 1/1] close #3181: ui: display guest name in confirm dialogs

[Michael Köppl]

The confirmation dialogs of the following actions are affected by this
change:
* Remove
* Clone
* Migrate
* Snapshot
* Snapshot rollback
* Backup VM/CT from config view
* Restore VM/CT from config view

The VM/CT name is considered optional in all handled cases. If it is
undefined, the parentheses and the guest name simply will not be
displayed in the dialog window. No exceptions are thrown in case of an
undefined guest name because it only extends the information displayed
to the user and is not essential for performing any of the actions
above.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/grid/BackupView.js   |  4 ++++
 www/manager6/lxc/CmdMenu.js       |  9 ++++++++-
 www/manager6/lxc/Config.js        | 11 +++++++++--
 www/manager6/qemu/CmdMenu.js      |  9 ++++++++-
 www/manager6/qemu/Config.js       | 11 +++++++++--
 www/manager6/tree/SnapshotTree.js |  3 +++
 www/manager6/window/Backup.js     |  3 +++
 www/manager6/window/Clone.js      |  7 ++++++-
 www/manager6/window/Migrate.js    | 10 +++++++---
 www/manager6/window/Restore.js    |  3 +++
 www/manager6/window/Snapshot.js   |  6 +++++-
 11 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index e71d1c88..99f2a49b 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -39,6 +39,8 @@ Ext.define('PVE.grid.BackupView', {
 	    throw "unsupported VM type '" + vmtype + "'";
 	}
 
+	let vmname = me.pveSelNode.data.name;
+
 	var searchFilter = {
 	    property: 'volid',
 	    value: '',
@@ -167,6 +169,7 @@ Ext.define('PVE.grid.BackupView', {
 		    nodename: nodename,
 		    vmid: vmid,
 		    vmtype: vmtype,
+		    vmname: vmname,
 		    storage: storagesel.getValue(),
 		    listeners: {
 			close: function() {
@@ -189,6 +192,7 @@ Ext.define('PVE.grid.BackupView', {
 		let win = Ext.create('PVE.window.Restore', {
 		    nodename: nodename,
 		    vmid: vmid,
+		    vmname: vmname,
 		    volid: rec.data.volid,
 		    volidText: PVE.Utils.render_storage_content(rec.data.volid, {}, rec),
 		    vmtype: vmtype,
diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
index e30989a6..e4dadd2f 100644
--- a/www/manager6/lxc/CmdMenu.js
+++ b/www/manager6/lxc/CmdMenu.js
@@ -89,7 +89,13 @@ Ext.define('PVE.lxc.CmdMenu', {
 		text: gettext('Clone'),
 		iconCls: 'fa fa-fw fa-clone',
 		hidden: !caps.vms['VM.Clone'],
-		handler: () => PVE.window.Clone.wrap(info.node, info.vmid, me.isTemplate, 'lxc'),
+		handler: () => PVE.window.Clone.wrap(
+		    info.node,
+		    info.vmid,
+		    info.name,
+		    me.isTemplate,
+		    'lxc',
+		),
 	    },
 	    {
 		text: gettext('Migrate'),
@@ -100,6 +106,7 @@ Ext.define('PVE.lxc.CmdMenu', {
 			vmtype: 'lxc',
 			nodename: info.node,
 			vmid: info.vmid,
+			vmname: info.name,
 			autoShow: true,
 		    });
 		},
diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index a7191fa2..9e33ca56 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -100,6 +100,7 @@ Ext.define('PVE.lxc.Config', {
 		    vmtype: 'lxc',
 		    nodename: nodename,
 		    vmid: vmid,
+		    vmname: vm.name,
 		});
 		win.show();
 	    },
@@ -115,7 +116,13 @@ Ext.define('PVE.lxc.Config', {
 		    iconCls: 'fa fa-fw fa-clone',
 		    hidden: !caps.vms['VM.Clone'],
 		    handler: function() {
-			PVE.window.Clone.wrap(nodename, vmid, template, 'lxc');
+			PVE.window.Clone.wrap(
+			    nodename,
+			    vmid,
+			    vm.name,
+			    template,
+			    'lxc',
+			);
 		    },
 		},
 		{
@@ -156,7 +163,7 @@ Ext.define('PVE.lxc.Config', {
 		    handler: function() {
 			Ext.create('PVE.window.SafeDestroyGuest', {
 			    url: base_url,
-			    item: { type: 'CT', id: vmid },
+			    item: { type: 'CT', id: vmid, name: vm.name },
 			    taskName: 'vzdestroy',
 			}).show();
 		    },
diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
index 7a4e0a0e..e7dd6782 100644
--- a/www/manager6/qemu/CmdMenu.js
+++ b/www/manager6/qemu/CmdMenu.js
@@ -121,6 +121,7 @@ Ext.define('PVE.qemu.CmdMenu', {
 			vmtype: 'qemu',
 			nodename: info.node,
 			vmid: info.vmid,
+			vmname: info.name,
 			autoShow: true,
 		    });
 		},
@@ -129,7 +130,13 @@ Ext.define('PVE.qemu.CmdMenu', {
 		text: gettext('Clone'),
 		iconCls: 'fa fa-fw fa-clone',
 		hidden: !caps.vms['VM.Clone'],
-		handler: () => PVE.window.Clone.wrap(info.node, info.vmid, me.isTemplate, 'qemu'),
+		handler: () => PVE.window.Clone.wrap(
+		    info.node,
+		    info.vmid,
+		    info.name,
+		    me.isTemplate,
+		    'qemu',
+		),
 	    },
 	    {
 		text: gettext('Convert to template'),
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 48eb753e..c4f904e8 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -73,6 +73,7 @@ Ext.define('PVE.qemu.Config', {
 		    vmtype: 'qemu',
 		    nodename: nodename,
 		    vmid: vmid,
+		    vmname: vm.name,
 		});
 		win.show();
 	    },
@@ -88,7 +89,13 @@ Ext.define('PVE.qemu.Config', {
 		    iconCls: 'fa fa-fw fa-clone',
 		    hidden: !caps.vms['VM.Clone'],
 		    handler: function() {
-			PVE.window.Clone.wrap(nodename, vmid, template, 'qemu');
+			PVE.window.Clone.wrap(
+			    nodename,
+			    vmid,
+			    vm.name,
+			    template,
+			    'qemu',
+			);
 		    },
 		},
 		{
@@ -128,7 +135,7 @@ Ext.define('PVE.qemu.Config', {
 		    handler: function() {
 			Ext.create('PVE.window.SafeDestroyGuest', {
 			    url: base_url,
-			    item: { type: 'VM', id: vmid },
+			    item: { type: 'VM', id: vmid, name: vm.name },
 			    taskName: 'qmdestroy',
 			}).show();
 		    },
diff --git a/www/manager6/tree/SnapshotTree.js b/www/manager6/tree/SnapshotTree.js
index 97268072..835245c1 100644
--- a/www/manager6/tree/SnapshotTree.js
+++ b/www/manager6/tree/SnapshotTree.js
@@ -50,6 +50,7 @@ Ext.define('PVE.guest.SnapshotTree', {
 	    let win = Ext.create('PVE.window.Snapshot', {
 		nodename: vm.get('nodename'),
 		vmid: vm.get('vmid'),
+		vmname: vm.get('vmname'),
 		viewonly: !vm.get('snapshotAllowed'),
 		type: vm.get('type'),
 		isCreate: !edit,
@@ -213,6 +214,8 @@ Ext.define('PVE.guest.SnapshotTree', {
 	    }
 	    vm.set('vmid', view.pveSelNode.data.vmid);
 
+	    vm.set('vmname', view.pveSelNode.data.name);
+
 	    let caps = Ext.state.Manager.get('GuiCap');
 	    vm.set('snapshotAllowed', !!caps.vms['VM.Snapshot']);
 	    vm.set('rollbackAllowed', !!caps.vms['VM.Snapshot.Rollback']);
diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 4418a9c7..2b0ab153 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -331,6 +331,9 @@ Ext.define('PVE.window.Backup', {
 	var title = gettext('Backup') + " " +
 	    (me.vmtype === 'lxc' ? "CT" : "VM") +
 	    " " + me.vmid;
+	if (me.vmname) {
+	    title += " (" + me.vmname + ")";
+	}
 
 	Ext.apply(me, {
 	    title: title,
diff --git a/www/manager6/window/Clone.js b/www/manager6/window/Clone.js
index 2d3f2678..b139cb46 100644
--- a/www/manager6/window/Clone.js
+++ b/www/manager6/window/Clone.js
@@ -21,7 +21,7 @@ Ext.define('PVE.window.Clone', {
 
     statics: {
 	// display a snapshot selector only if needed
-	wrap: function(nodename, vmid, isTemplate, guestType) {
+	wrap: function(nodename, vmid, vmname, isTemplate, guestType) {
 	    Proxmox.Utils.API2Request({
 		url: '/nodes/' + nodename + '/' + guestType + '/' + vmid +'/snapshot',
 		failure: function(response, opts) {
@@ -36,6 +36,7 @@ Ext.define('PVE.window.Clone', {
 			nodename: nodename,
 			guestType: guestType,
 			vmid: vmid,
+			vmname: vmname,
 			isTemplate: isTemplate,
 			hasSnapshots: hasSnapshots,
 		    }).show();
@@ -155,7 +156,11 @@ Ext.define('PVE.window.Clone', {
 	if (me.isTemplate) {
 	    titletext += ' Template';
 	}
+
 	me.title = "Clone " + titletext + " " + me.vmid;
+	if (me.vmname) {
+	    me.title += " (" + me.vmname + ")";
+	}
 
 	var col1 = [];
 	var col2 = [];
diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
index 78d03921..69792af8 100644
--- a/www/manager6/window/Migrate.js
+++ b/www/manager6/window/Migrate.js
@@ -4,6 +4,7 @@ Ext.define('PVE.window.Migrate', {
     vmtype: undefined,
     nodename: undefined,
     vmid: undefined,
+    vmname: undefined,
     maxHeight: 450,
 
     viewModel: {
@@ -92,9 +93,12 @@ Ext.define('PVE.window.Migrate', {
 	    }
 	    vm.set('vmtype', view.vmtype);
 
-	    view.setTitle(
-		Ext.String.format('{0} {1} {2}', gettext('Migrate'), vm.get(view.vmtype).commonName, view.vmid),
-	    );
+	    let title = Ext.String.format('{0} {1} {2}', gettext('Migrate'), vm.get(view.vmtype).commonName, view.vmid);
+	    if (view.vmname) {
+		title += Ext.String.format(" ({0})", view.vmname);
+	    }
+	    view.setTitle(title);
+
 	    me.lookup('proxmoxHelpButton').setHelpConfig({
 		onlineHelp: vm.get(view.vmtype).onlineHelp,
 	    });
diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
index 6efe1313..d98f3e53 100644
--- a/www/manager6/window/Restore.js
+++ b/www/manager6/window/Restore.js
@@ -357,6 +357,9 @@ Ext.define('PVE.window.Restore', {
 	let title = gettext('Restore') + ": " + (me.vmtype === 'lxc' ? 'CT' : 'VM');
 	if (me.vmid) {
 	    title = `${gettext('Overwrite')} ${title} ${me.vmid}`;
+	    if (me.vmname) {
+		title += " (" + me.vmname + ")";
+	    }
 	}
 
 	Ext.apply(me, {
diff --git a/www/manager6/window/Snapshot.js b/www/manager6/window/Snapshot.js
index 426be7cc..c1816e69 100644
--- a/www/manager6/window/Snapshot.js
+++ b/www/manager6/window/Snapshot.js
@@ -137,7 +137,11 @@ Ext.define('PVE.window.Snapshot', {
 
 	let subject;
 	if (me.isCreate) {
-	    subject = (me.type === 'qemu' ? 'VM' : 'CT') + me.vmid + ' ' + gettext('Snapshot');
+	    subject = (me.type === 'qemu' ? 'VM' : 'CT') + me.vmid;
+	    if (me.vmname) {
+		subject += " (" + me.vmname + ")";
+	    }
+	    subject += ' ' + gettext('Snapshot');
 	    me.method = 'POST';
 	    me.showTaskViewer = true;
 	} else {
-- 
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 widget-toolkit 1/1] close #3181: ui: add guest name to safe destroy dialog window

[Thomas Lamprecht]

Am 25.03.25 um 16:01 schrieb Michael Köppl:
> While the format_task_description function is used in other parts of the
> UI, this still leaves these use cases intact. The guest name is an
> optional addition in parantheses.

s/parantheses/parentheses/

> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/Utils.js              | 8 ++++++--
>  src/window/SafeDestroy.js | 3 ++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index c873c85..a64d07f 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -716,7 +716,7 @@ utilities: {
>  	}
>      },
>  
> -    format_task_description: function(type, id) {
> +    format_task_description: function(type, id, name) {

FYI in pve-manager's Utils we got the following in formatGuestTaskConfirmation
already:

return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);

And that seems to do the same thing and comes from a not so old commit
31965684c ("fix #5787: ui: display guest name in confirmation dialog")
https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=31965684c

Could make sense to use the same approach here and also to reference that
commit in the commit message for posterity's sake.


>  	let farray = Proxmox.Utils.task_desc_table[type];
>  	let text;
>  	if (!farray) {
> @@ -731,7 +731,11 @@ utilities: {
>  	let prefix = farray[0];
>  	text = farray[1];
>  	if (prefix && id !== undefined) {
> -	    return prefix + ' ' + id + ' - ' + text;
> +	    let fullText = prefix + ' ' + id;
> +	    if (name) {
> +		fullText += " (" + name + ")";
> +	    }
> +	    return fullText + " - " + text;

small nit that I already had written before noticing above, so mostly relevant
if we keep this in the first place (or similar changes in the future):

I'd slightly favor directly returning and using template strings [0] for new
interpolation of variables with strings.

And FWIW, it might be a nice small polishing if we allow the caller to
determine if the VMID should be put in parentheses, e.g. for the case where the
user configured the resource tree to use the name as sort key, OTOH., that might
be a relatively big amount of complexity for little gain – just mentioning it
for the sake of completeness.

A simple variant without that extra polishing could look like:

if (prefix && id !== undefined) {
    let fullId = name ? `${id} (${name}) : id;
    return `${prefix} ${fullId} - ${text}`;
}

Ore more verbose:

if (prefix && id !== undefined) {
    let fullId = name ? `${id} (${name}) : id;
    if (name) {
        return `${prefix} ${id} (${name}) - ${text}`;
    } else {
        return `${prefix} ${id} - ${text}`;
    }    
}

but no hard feelings in any way.

[0]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

>  	}
>  	return text;
>      },
> diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js
> index c058465..d980a52 100644
> --- a/src/window/SafeDestroy.js
> +++ b/src/window/SafeDestroy.js
> @@ -187,9 +187,10 @@ Ext.define('Proxmox.window.SafeDestroy', {
>  	}
>  
>  	let taskName = me.getTaskName();
> +	const itemName = me.getItem().name;
>  	if (Ext.isDefined(taskName)) {
>  	    me.lookupReference('messageCmp').setHtml(
> -		Proxmox.Utils.format_task_description(taskName, itemId),
> +		Proxmox.Utils.format_task_description(taskName, itemId, itemName),
>  	    );
>  	} else {
>  	    throw "no task name specified";



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

Re: [pve-devel] [PATCH widget-toolkit 1/1] close #3181: ui: add guest name to safe destroy dialog window

[Michael Köppl]

On 3/25/25 19:27, Thomas Lamprecht wrote:
> Am 25.03.25 um 16:01 schrieb Michael Köppl:
>> While the format_task_description function is used in other parts of the
>> UI, this still leaves these use cases intact. The guest name is an
>> optional addition in parantheses.
> 
> s/parantheses/parentheses/

This is embarrassing...

> FYI in pve-manager's Utils we got the following in formatGuestTaskConfirmation
> already:
> 
> return Proxmox.Utils.format_task_description(taskType, `${vmid} (${guestName})`);
> 
> And that seems to do the same thing and comes from a not so old commit
> 31965684c ("fix #5787: ui: display guest name in confirmation dialog")
> https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=31965684c
> 
> Could make sense to use the same approach here and also to reference that
> commit in the commit message for posterity's sake.

Thanks for your suggestion. I think it makes more sense to adopt this 
approach than change format_task_description. I'll send a v2 that just 
adds the guest name as part of the id input. However, the current 
implementation of formatGuestTaskConfirmation prints the parentheses 
even if the guest name is undefined. I suppose it mostly feels a bit 
unpolished if the confirmation dialog displays "VM 100 () - Remove" 
(this occurred to me only right after rebooting a node). Would it make 
sense to also update the implementation of formatGuestTaskConfirmation 
to only conditionally display the parentheses as part of a v2 and make 
it consistent across all confirmation dialogs, or is it better to 
consider this a separate patch?

> And FWIW, it might be a nice small polishing if we allow the caller to
> determine if the VMID should be put in parentheses, e.g. for the case where the
> user configured the resource tree to use the name as sort key, OTOH., that might
> be a relatively big amount of complexity for little gain – just mentioning it
> for the sake of completeness.

Yes, I already pondered a bit on which way around it makes more sense 
(ID (name) or name (ID)) or if the user should even be able to choose. I 
think doing it dependent on the sort key is a good idea, but it would 
add complexity to the code, as you said. I'll check it out and, if I 
find a sensible solution, send a separate patch so the added complexity 
can be properly considered.

Thank you for your feedback!


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

Re: [pve-devel] [PATCH widget-toolkit 1/1] close #3181: ui: add guest name to safe destroy dialog window

[Michael Köppl]

On 3/28/25 13:03, Michael Köppl wrote:
> On 3/25/25 19:27, Thomas Lamprecht wrote:
>> Am 25.03.25 um 16:01 schrieb Michael Köppl:
>>> While the format_task_description function is used in other parts of the
>>> UI, this still leaves these use cases intact. The guest name is an
>>> optional addition in parantheses.
>>
>> s/parantheses/parentheses/
> 
> This is embarrassing...
> 
>> FYI in pve-manager's Utils we got the following in 
>> formatGuestTaskConfirmation
>> already:
>>
>> return Proxmox.Utils.format_task_description(taskType, `${vmid} 
>> (${guestName})`);
>>
>> And that seems to do the same thing and comes from a not so old commit
>> 31965684c ("fix #5787: ui: display guest name in confirmation dialog")
>> https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=31965684c
>>
>> Could make sense to use the same approach here and also to reference that
>> commit in the commit message for posterity's sake.
> 
> Thanks for your suggestion. I think it makes more sense to adopt this 
> approach than change format_task_description. I'll send a v2 that just 
> adds the guest name as part of the id input. However, the current 
> implementation of formatGuestTaskConfirmation prints the parentheses 
> even if the guest name is undefined. I suppose it mostly feels a bit 
> unpolished if the confirmation dialog displays "VM 100 () - 
> Remove" (this occurred to me only right after rebooting a node). Would 
> it make sense to also update the implementation of 
> formatGuestTaskConfirmation to only conditionally display the 
> parentheses as part of a v2 and make it consistent across all 
> confirmation dialogs, or is it better to consider this a separate patch?
> 
>> And FWIW, it might be a nice small polishing if we allow the caller to
>> determine if the VMID should be put in parentheses, e.g. for the case 
>> where the
>> user configured the resource tree to use the name as sort key, OTOH., 
>> that might
>> be a relatively big amount of complexity for little gain – just 
>> mentioning it
>> for the sake of completeness.
> 
> Yes, I already pondered a bit on which way around it makes more sense 
> (ID (name) or name (ID)) or if the user should even be able to choose. I 
> think doing it dependent on the sort key is a good idea, but it would 
> add complexity to the code, as you said. I'll check it out and, if I 
> find a sensible solution, send a separate patch so the added complexity 
> can be properly considered.
> 
> Thank you for your feedback!
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Superseded by 
https://lore.proxmox.com/pve-devel/20250331133154.148713-1-m.koeppl@proxmox.com


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