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 0D863697CB for ; Thu, 24 Mar 2022 11:46:11 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0B6BA2ECB1 for ; Thu, 24 Mar 2022 11:46:11 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 56EE02ECA6 for ; Thu, 24 Mar 2022 11:46:10 +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 2C50346F74 for ; Thu, 24 Mar 2022 11:46:10 +0100 (CET) Message-ID: <2fcb8f65-8b36-e425-d955-3ff5557c18b6@proxmox.com> Date: Thu, 24 Mar 2022 11:46:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: Fabian Ebner , pve-devel@lists.proxmox.com References: <20220314093509.349554-1-a.lauterer@proxmox.com> <20220314093509.349554-3-a.lauterer@proxmox.com> <7055bba6-b945-9845-04fc-072d74711151@proxmox.com> From: Aaron Lauterer In-Reply-To: <7055bba6-b945-9845-04fc-072d74711151@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.025 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: Thu, 24 Mar 2022 10:46:11 -0000 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); >> };