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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A939C6932D for ; Fri, 11 Mar 2022 15:38:43 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 936D4261E5 for ; Fri, 11 Mar 2022 15:38:13 +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 8F9C7261DA for ; Fri, 11 Mar 2022 15:38:12 +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 64EAF42733 for ; Fri, 11 Mar 2022 15:38:12 +0100 (CET) Message-ID: <1da390da-b037-f9e0-3de2-505c0b89e5d9@proxmox.com> Date: Fri, 11 Mar 2022 15:38:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 From: Aaron Lauterer To: Fabian Ebner , pve-devel@lists.proxmox.com References: <20220307100722.257128-1-a.lauterer@proxmox.com> <20220307100722.257128-2-a.lauterer@proxmox.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.019 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 v3 manager 1/4] 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: Fri, 11 Mar 2022 14:38:43 -0000 On 3/10/22 11:49, Fabian Ebner wrote: > Am 07.03.22 um 11:07 schrieb Aaron Lauterer: >> For the new HDReassign component, we follow the approach of HDMove to >> have one componend for qemu and lxc. >> >> To avoid button clutter, a new "Disk/Volume action" button is >> introduced. It holds the Move, Reassign and Resize buttons in a submenu. >> > > Some small general things I noticed: > * Since re-assign to/from templates is not possible, we could filter > those out in the selection/disable the action (assuming the information > whether it's a template or not is readily available). good point, definitely doable > * For such drop-down menus we mostly (always?) have icons too. Yes, but since the existing buttons did not have any icons and finding matching and not yet (too widely) used icons in font-awesome is getting tricky, I opted for a menu without icons. I'd rather have no icons instead of poorly fitting ones. But definitely something that can be discussed and changed quite easily. > * The move disk and reassign windows seem to have a bit much padding. thanks for catching that! I'll investigate where that happens. [...] >> diff --git a/www/manager6/Makefile b/www/manager6/Makefile >> index e6e01bd1..94a78d89 100644 >> --- a/www/manager6/Makefile >> +++ b/www/manager6/Makefile >> @@ -214,6 +214,7 @@ JSSRC= \ >> qemu/HDTPM.js \ >> qemu/HDMove.js \ >> qemu/HDResize.js \ >> + qemu/HDReassign.js \ > > Nit: not ordered alphabetically yep, will fix it > >> qemu/HardwareView.js \ >> qemu/IPConfigEdit.js \ >> qemu/KeyboardEdit.js \ >> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js >> index 15ee3c67..306a4988 100644 >> --- a/www/manager6/lxc/Resources.js >> +++ b/www/manager6/lxc/Resources.js >> @@ -151,7 +151,8 @@ Ext.define('PVE.lxc.RessourceView', { >> }); >> }; >> >> - var run_move = function(b, e, rec) { >> + var run_move = function() { >> + let rec = me.selModel.getSelection()[0]; > > Style nit: could switch to let instead of var. yeah, I tried to keep it similar to the surrounding, but we could definitely switch it over to let > [...] >> >> - var resize_btn = new Proxmox.button.Button({ >> + var resize_menuitem = new Ext.menu.Item({ > > Same here. > >> text: gettext('Resize disk'), >> selModel: me.selModel, >> disabled: true, > > ----8<---- > >> @@ -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); >> + reassign_menuitem.setDisabled(isRootFS); >> + >> + remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending); >> + resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk); >> + move_menuitem.setDisabled(!isDisk || !diskCap); > > Nothing new, but could use '|| isUnusedDisk' too: > cannot move volume 'unused0', only configured volumes can be moved to > another storage I kept it as is because, and maybe that was also considered originally, getting an error message at some point, explaining why it does not work, helps more than having the button disabled without knowing why. [...] >> + cbindData: function() { >> + let me = this; >> + return { >> + vmid: me.vmid, >> + disk: me.disk, >> + isQemu: me.type === 'qemu', >> + nodename: me.nodename, >> + url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`, >> + }; >> + }, >> + >> + cbind: { >> + title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'), >> + submitText: get => get('title'), >> + qemu: '{isQemu}', >> + url: '{url}', >> + }, >> + >> + submitUrl: function(url, values) { >> + url += this.qemu ? 'move_disk' : 'move_volume'; > > Why not already construct the full URL above? I'll see how to do that. First tests show that if I construct the submitUrl directly, I need to set the 'url' at some point, even if not used because the edit window does check its existence. [...] >> + onTargetVMChange: function(f, vmid) { >> + let me = this; >> + let view = me.getView(); >> + let diskSelector = view.lookup('diskSelector'); >> + if (!vmid) { >> + diskSelector.setVMConfig(null); >> + me.VMConfig = null; >> + return; >> + } >> + >> + let type = view.qemu ? 'qemu' : 'lxc'; >> + >> + let url = `/nodes/${view.nodename}/${type}/${vmid}/config`; >> + Proxmox.Utils.API2Request({ >> + url: url, >> + method: 'GET', >> + failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus), >> + success: function(response, options) { >> + if (view.qemu) { >> + diskSelector.setVMConfig(response.result.data); >> + diskSelector.setDisabled(false); >> + } else { >> + let mpIdSelector = view.lookup('mpIdSelector'); >> + let mpType = view.lookup('mpType'); >> + >> + view.VMConfig = response.result.data; >> + >> + PVE.Utils.forEachMP((bus, i) => { > > Feels hacky and in fact only works in all cases, because the number of > max unused and max mp is the same. The iterator only iterates over mount > points when called like this and you ignore the bus. Using a new helper > akin to PVE.Utils.nextFreeDisk or simply inlining the required loop > here, would be more robust. Yep, going for a PVE.Utils.nextFreeMP implementation > >> + let name = view.getViewModel().get('mpType') + i.toString(); >> + if (!Ext.isDefined(view.VMConfig[name])) { >> + mpIdSelector.setValue(i); >> + return false; >> + } >> + return undefined; >> + }); >> + >> + mpType.setDisabled(false); >> + mpIdSelector.setDisabled(false); >> + mpIdSelector.validate(); >> + } >> + }, >> + }); >> + }, >> + }, >> + >> + items: [ >> + { >> + xtype: 'form', >> + reference: 'moveFormPanel', >> + bodyPadding: 10, >> + border: false, >> + fieldDefaults: { >> + labelWidth: 100, >> + anchor: '100%', >> + }, >> + items: [ >> + { >> + xtype: 'displayfield', >> + name: 'sourceDisk', >> + fieldLabel: gettext('Source'), >> + cbind: { >> + name: get => get('isQemu') ? 'disk' : 'volume', >> + value: '{disk}', >> + }, >> + vtype: 'StorageId', > > But it's not a storage ID. yeah, bad copy pasta and AFAICT not even needed here > >> + allowBlank: false, >> + }, >> + { >> + xtype: 'vmComboSelector', >> + reference: 'targetVMID', >> + name: 'targetVmid', >> + allowBlank: false, >> + fieldLabel: gettext('Target'), >> + bind: { >> + value: '{targetVMID}', >> + }, >> + store: { >> + model: 'PVEResources', >> + autoLoad: true, >> + sorters: 'vmid', >> + cbind: {}, // for nested cbinds >> + filters: [ >> + { >> + property: 'type', >> + cbind: { >> + value: get => get('isQemu') ? /qemu/ : /lxc/, > > Nit: the type can be matched exaclty, or was there a reason for using a > regex? IIRC not really, will change it [...] >> + { >> + xtype: 'proxmoxintegerfield', >> + name: 'mpId', >> + reference: 'mpIdSelector', >> + minValue: 0, >> + flex: 1, >> + allowBlank: false, >> + validateOnChange: true, >> + disabled: true, >> + bind: { >> + maxValue: '{mpMaxCount}', >> + }, >> + validator: function(value) { >> + let view = this.up('window'); >> + let type = view.getViewModel().get('mpType'); >> + if (Ext.isDefined(view.VMConfig[type+value])) { > > Style nit: could also use `${type}${value}` good point