From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0943C68FD2 for ; Tue, 22 Mar 2022 12:18:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F11A21D05C for ; Tue, 22 Mar 2022 12:18:34 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 9E9D91D04F for ; Tue, 22 Mar 2022 12:18:33 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4F1BA46F41 for ; Tue, 22 Mar 2022 12:18:33 +0100 (CET) Message-ID: <7055bba6-b945-9845-04fc-072d74711151@proxmox.com> Date: Tue, 22 Mar 2022 12:18:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.2 Content-Language: en-US To: pve-devel@lists.proxmox.com, Aaron Lauterer References: <20220314093509.349554-1-a.lauterer@proxmox.com> <20220314093509.349554-3-a.lauterer@proxmox.com> From: Fabian Ebner In-Reply-To: <20220314093509.349554-3-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.118 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v4 manager 2/6] ui: lxc/qemu: add disk reassign and action submenu X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Mar 2022 11:18:35 -0000 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); > };