Re: [pve-devel] [PATCH v4 manager 2/6] ui: lxc/qemu: add disk reassign and action submenu

[Fabian Ebner <f.ebner@proxmox.com>]

Am 14.03.22 um 10:35 schrieb Aaron Lauterer:
> @@ -227,14 +246,34 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    },
>  	});
>  
> -	var move_btn = new Proxmox.button.Button({
> +	let reassign_menuitem = new Ext.menu.Item({
> +	    text: gettext('Reassign Volume'),
> +	    tooltip: gettext('Reassign volume to another CT'),


Nit: if there is a tooltip for reassign maybe there should also be one
for move? Otherwise, it feels inconsistent from the perspective of a new
user ;) Same for VMs.

> +	    handler: run_reassign,
> +	    reference: 'reassing_item',
> +	    disabled: true,
> +	});
> +
> +	let move_menuitem = new Ext.menu.Item({
>  	    text: gettext('Move Volume'),
>  	    selModel: me.selModel,
>  	    disabled: true,
> -	    dangerous: true,
>  	    handler: run_move,
>  	});
>  

(...)

> @@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    }
>  	    edit_btn.setDisabled(noedit);
>  
> -	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
> -	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> -	    move_btn.setDisabled(!isDisk || !diskCap);
> +	    volumeaction_btn.setDisabled(!isDisk);

Shouldn't this also check for diskCap?

> +	    reassign_menuitem.setDisabled(isRootFS);
> +
> +	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
> +	    resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> +	    move_menuitem.setDisabled(!isDisk || !diskCap);

Nit: please group the menu and menuitems together. And since the whole
menu is disabled if !isDisk, you don't need to repeat that for the
menuitems (like you already don't for reassign ;)).

>  	    revert_btn.setDisabled(!pending);
>  
>  	    remove_btn.setText(isUsedDisk ? remove_btn.altText : remove_btn.defaultText);

(...)

> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 6cea4287..af84fb3f 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -400,8 +400,8 @@ Ext.define('PVE.qemu.HardwareView', {
>  	    win.on('destroy', me.reload, me);
>  	};
>  
> -	var run_move = function() {
> -	    var rec = sm.getSelection()[0];
> +	let run_move = function() {
> +	    let rec = sm.getSelection()[0];
>  	    if (!rec) {
>  		return;
>  	    }

Nit: While it is an improvement, it doesn't actually interact with
anything else in the patch, and hence doesn't really belong here.

(...)

> @@ -611,9 +648,15 @@ Ext.define('PVE.qemu.HardwareView', {
>  	        (isDisk && !diskCap),
>  	    );
>  
> -	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
> +	    resize_menuitem.setDisabled(pending || !isUsedDisk || !diskCap);
> +	    reassign_menuitem.setDisabled(pending || (isEfi || tpmMoveable));
>  
> -	    move_btn.setDisabled(pending || !(isUsedDisk || isEfi || tpmMoveable) || !diskCap);
> +	    diskaction_btn.setDisabled(
> +		pending ||
> +		isCloudInit ||
> +		!(isDisk || isEfi || tpmMoveable) ||
> +		!diskCap,
> +	    );

Here you are using the fact that "move action disabled" implies "resize
and reassign action disabled". Might be worth giving the following a
shot hoping it's cleaner:

if pending || !diskCap || other common criteria
  disable menu
else
  enable menu
  disable resize based on resize-specific criteria (here the common
criteria don't need to be repeated)
  disable reassign based on reassign-specific criteria
  disable move based on move-specific criteria

and if the common criteria are collected correctly, there should never
be a case where all entries are disabled but the menu isn't.

>  
>  	    revert_btn.setDisabled(!pending);
>  	};