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

[Aaron Lauterer <a.lauterer@proxmox.com>]

Thanks for the nits. Especially regarding the button (de)activation logic.

I can send a follow-up patch or a new version of the whole series, or just this patch.

Whatever is preferred :)

On 3/22/22 12:18, Fabian Ebner wrote:
> 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);
>>   	};