[pve-devel] [PATCH manager v2] ui: pool: separate "Add Virtual Machine" menu into distinct options

[Thomas Lamprecht]

Am 29/07/2024 um 13:43 schrieb Theodor Fumics:
> Split the "Add Virtual Machine" menu into separate options
> for Virtual Machines and Containers to reduce confusion.
> This change follows feedback from a user in [1], who had difficulty
> finding the container option.
> 
> [1] https://forum.proxmox.com/threads/how-to-add-containers-to-a-resource-pool.151946/
> 
> Signed-off-by: Theodor Fumics <theodor.fumics at gmx.net>
> ---
> 
> Notes:
>     Changes from v1 -> v2
>     * adjusted line to fit within 100 characters as per style guide

true, but that was a side effect of changing the logical expression, which
might be the more important one to highlight.

> 
>  www/manager6/grid/PoolMembers.js | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
> index 75f20cab..78ccc2a4 100644
> --- a/www/manager6/grid/PoolMembers.js
> +++ b/www/manager6/grid/PoolMembers.js
> @@ -1,4 +1,4 @@
> -Ext.define('PVE.pool.AddVM', {
> +Ext.define('PVE.pool.AddGuest', {
>      extend: 'Proxmox.window.Edit',
> 
>      width: 640,
> @@ -37,7 +37,7 @@ Ext.define('PVE.pool.AddVM', {
>  	    ],
>  	    filters: [
>  		function(item) {
> -		    return (item.data.type === 'lxc' || item.data.type === 'qemu') &&item.data.pool !== me.pool;
> +		    return (me.type === item.data.type) && item.data.pool !== me.pool;

nit: the parenthesis are not needed and one might wonder why they are added
only to the left comparison but not the right one.

Either use parenthesis for both expression-parts for consistency or, and that
would be IMO here better as it's quite a simple expression, use them for neither
part.

>  		},
>  	    ],
>  	});
> @@ -84,15 +84,11 @@ Ext.define('PVE.pool.AddVM', {
>  		    dataIndex: 'name',
>  		    flex: 1,
>  		},
> -		{
> -		    header: gettext('Type'),
> -		    dataIndex: 'type',
> -		},
>  	    ],
>  	});
> 
>  	Ext.apply(me, {
> -	    subject: gettext('Virtual Machine'),
> +	    subject: gettext(me.type === 'qemu' ? 'Virtual Machine' : 'LXC Container'),
>  	    items: [
>  		vmsField,
>  		vmGrid,
> @@ -228,16 +224,25 @@ Ext.define('PVE.grid.PoolMembers', {
>  			items: [
>  			    {
>  				text: gettext('Virtual Machine'),

Alternatively we could just rename the label to something that includes both,
i.e. "Virtual Guest" or just "Guest", which is the wording we normally use
when talking about both, VMs and CTs.

Did you think about that and explicitly chose against doing so?
If, it would be great to mention that in the commit message, having some
argumentation there about such things can help to reduce back and forth
and help when looking at old history to find out why things are they way
they are.

There are some advantages for your approach, like slightly better discoverability,
but also some for keep using a combined add dialogue, like making it faster to add
a set of guests that includes both CTs and VMs to a pool, or do not having to look
up what if a host was set up as CT or VM, and last but not least, a bit less code.

I did not check this to closely to call the shots now, but maybe take a step
back and look at alternative(s) and argue whatever decision you make (this approach,
the rename one, or something completely different).

> -				iconCls: 'pve-itype-icon-qemu',
> +				iconCls: 'fa fa-fw fa-desktop',
> +				handler: function() {
> +				    var win = Ext.create('PVE.pool.AddGuest', { pool: me.pool, type: 'qemu' });
> +				    win.on('destroy', reload);
> +				    win.show();

A nit and not directly related, but we use for modern windows the following style:

Ext.create('PVE.pool.AddGuest', {
    autoShow: true,
    pool: me.pool,
    type: 'qemu',
    listeners: {
        listeners: {
            destroy: reload, // or skip the trivial reload closure and use just `() => store.reload(),`
        },
    },
});


if you touch this many lines this clean-up could be folded in, but one also could
do it with some other clean-ups upfront, both can be fine.
As you're not too experienced with ExtJS it's totally fine to me to leave it as is,
though, the clean-ups here won't gain us that much on code maintainability anyway.

Oh, and you basically have the callback code twice, just with a different `type`
value, could factor this out to a higher-order closure, like:

let getAddGuestCallback = type => {
    return () => Ext.create('PVE.pool.AddGuest', {
        autoShow: true,
        pool: me.pool,
        type,
        listeners: {
            listeners: {
                destroy: () => store.reload(),
            },
        },
    });
};

and then use that like:

    handler: getAddGuestCallback('qemu'),

That's not a must here for those two callbacks but can sometimes be a nice pattern
to avoid repeating lots of code.

> +				},
> +			    },
> +			    {
> +				text: gettext('Container'),
> +				iconCls: 'fa fa-fw fa-cube',
>  				handler: function() {
> -				    var win = Ext.create('PVE.pool.AddVM', { pool: me.pool });
> +				    var win = Ext.create('PVE.pool.AddGuest', { pool: me.pool, type: 'lxc' });
>  				    win.on('destroy', reload);
>  				    win.show();
>  				},
>  			    },
>  			    {
>  				text: gettext('Storage'),
> -				iconCls: 'pve-itype-icon-storage',
> +				iconCls: 'fa fa-fw fa-hdd-o',
>  				handler: function() {
>  				    var win = Ext.create('PVE.pool.AddStorage', { pool: me.pool });
>  				    win.on('destroy', reload);
> --
> 2.39.2
> 
>