From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id BA47D1FF16E for ; Mon, 29 Jul 2024 14:27:13 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5C2081CB29; Mon, 29 Jul 2024 14:27:13 +0200 (CEST) Message-ID: <0e8c3f93-2953-4df1-b962-4653a1aa10ec@proxmox.com> Date: Mon, 29 Jul 2024 14:27:08 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Theodor Fumics , pve-devel@lists.proxmox.com References: <20240729114303.137022-1-theodor.fumics@gmx.net> Content-Language: en-GB, de-AT From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20240729114303.137022-1-theodor.fumics@gmx.net> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.051 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager v2] ui: pool: separate "Add Virtual Machine" menu into distinct options 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 29/07/2024 um 13:43 schrieb Theodor Fumics: > Split the "Add Virtual Machine" menu into separate options > for Virtual Machines and Containers to reduce confusion. > This change follows feedback from a user in [1], who had difficulty > finding the container option. > > [1] https://forum.proxmox.com/threads/how-to-add-containers-to-a-resource-pool.151946/ > > Signed-off-by: Theodor Fumics > --- > > Notes: > Changes from v1 -> v2 > * adjusted line to fit within 100 characters as per style guide true, but that was a side effect of changing the logical expression, which might be the more important one to highlight. > > www/manager6/grid/PoolMembers.js | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js > index 75f20cab..78ccc2a4 100644 > --- a/www/manager6/grid/PoolMembers.js > +++ b/www/manager6/grid/PoolMembers.js > @@ -1,4 +1,4 @@ > -Ext.define('PVE.pool.AddVM', { > +Ext.define('PVE.pool.AddGuest', { > extend: 'Proxmox.window.Edit', > > width: 640, > @@ -37,7 +37,7 @@ Ext.define('PVE.pool.AddVM', { > ], > filters: [ > function(item) { > - return (item.data.type === 'lxc' || item.data.type === 'qemu') &&item.data.pool !== me.pool; > + return (me.type === item.data.type) && item.data.pool !== me.pool; nit: the parenthesis are not needed and one might wonder why they are added only to the left comparison but not the right one. Either use parenthesis for both expression-parts for consistency or, and that would be IMO here better as it's quite a simple expression, use them for neither part. > }, > ], > }); > @@ -84,15 +84,11 @@ Ext.define('PVE.pool.AddVM', { > dataIndex: 'name', > flex: 1, > }, > - { > - header: gettext('Type'), > - dataIndex: 'type', > - }, > ], > }); > > Ext.apply(me, { > - subject: gettext('Virtual Machine'), > + subject: gettext(me.type === 'qemu' ? 'Virtual Machine' : 'LXC Container'), > items: [ > vmsField, > vmGrid, > @@ -228,16 +224,25 @@ Ext.define('PVE.grid.PoolMembers', { > items: [ > { > text: gettext('Virtual Machine'), Alternatively we could just rename the label to something that includes both, i.e. "Virtual Guest" or just "Guest", which is the wording we normally use when talking about both, VMs and CTs. Did you think about that and explicitly chose against doing so? If, it would be great to mention that in the commit message, having some argumentation there about such things can help to reduce back and forth and help when looking at old history to find out why things are they way they are. There are some advantages for your approach, like slightly better discoverability, but also some for keep using a combined add dialogue, like making it faster to add a set of guests that includes both CTs and VMs to a pool, or do not having to look up what if a host was set up as CT or VM, and last but not least, a bit less code. I did not check this to closely to call the shots now, but maybe take a step back and look at alternative(s) and argue whatever decision you make (this approach, the rename one, or something completely different). > - iconCls: 'pve-itype-icon-qemu', > + iconCls: 'fa fa-fw fa-desktop', > + handler: function() { > + var win = Ext.create('PVE.pool.AddGuest', { pool: me.pool, type: 'qemu' }); > + win.on('destroy', reload); > + win.show(); A nit and not directly related, but we use for modern windows the following style: Ext.create('PVE.pool.AddGuest', { autoShow: true, pool: me.pool, type: 'qemu', listeners: { listeners: { destroy: reload, // or skip the trivial reload closure and use just `() => store.reload(),` }, }, }); if you touch this many lines this clean-up could be folded in, but one also could do it with some other clean-ups upfront, both can be fine. As you're not too experienced with ExtJS it's totally fine to me to leave it as is, though, the clean-ups here won't gain us that much on code maintainability anyway. Oh, and you basically have the callback code twice, just with a different `type` value, could factor this out to a higher-order closure, like: let getAddGuestCallback = type => { return () => Ext.create('PVE.pool.AddGuest', { autoShow: true, pool: me.pool, type, listeners: { listeners: { destroy: () => store.reload(), }, }, }); }; and then use that like: handler: getAddGuestCallback('qemu'), That's not a must here for those two callbacks but can sometimes be a nice pattern to avoid repeating lots of code. > + }, > + }, > + { > + text: gettext('Container'), > + iconCls: 'fa fa-fw fa-cube', > handler: function() { > - var win = Ext.create('PVE.pool.AddVM', { pool: me.pool }); > + var win = Ext.create('PVE.pool.AddGuest', { pool: me.pool, type: 'lxc' }); > win.on('destroy', reload); > win.show(); > }, > }, > { > text: gettext('Storage'), > - iconCls: 'pve-itype-icon-storage', > + iconCls: 'fa fa-fw fa-hdd-o', > handler: function() { > var win = Ext.create('PVE.pool.AddStorage', { pool: me.pool }); > win.on('destroy', reload); > -- > 2.39.2 > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel