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 41442A2D25 for ; Tue, 20 Jun 2023 16:13:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 216D4366A3 for ; Tue, 20 Jun 2023 16:13:20 +0200 (CEST) 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 for ; Tue, 20 Jun 2023 16:13:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C02DC41CF8; Tue, 20 Jun 2023 16:13:18 +0200 (CEST) Message-ID: Date: Tue, 20 Jun 2023 16:13:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Fiona Ebner , Proxmox VE development discussion References: <20230619141307.119430-1-d.csapak@proxmox.com> <20230619141307.119430-4-d.csapak@proxmox.com> <9423a607-64f8-0a29-b29e-dc1bdc4fa73c@proxmox.com> From: Dominik Csapak In-Reply-To: <9423a607-64f8-0a29-b29e-dc1bdc4fa73c@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.384 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_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [me.name] Subject: Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience 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, 20 Jun 2023 14:13:20 -0000 thanks for the review! i agree with all of the points of the answers in your other mails some comments here: On 6/20/23 15:25, Fiona Ebner wrote: > Am 19.06.23 um 16:13 schrieb Dominik Csapak: >> by removing the confusing buttons in the toolbar and adding them as >> actions in an actioncolumn. There a only relevant actions are visible >> and get a more expressive tooltip > > I agree with Aaron that the actioncolumn is too far right at the moment. yes makes sense to me too > >> with this, we now differentiate between 4 modes of the edit window: >> * create a new mapping altogether >> - shows all fields >> * edit existing mapping on top level >> - show only 'global' fields (comment+mdev), so no mappings > > This one feels slightly surprising to me from a user perspective as I > can't edit the actual mapping here. But it is cleaner and I guess one > could argue in the opposite direction too. > the question would be "which mapping" if there is more than one, so i left it out, this way the config of the overall entry is a bit seperated from the mappings themselves >> * add new host mapping >> - shows nodeselector, mapping and mdev, but mdev is disabled >> (informational only) >> * edit existing host mapping >> - show selected node (displayfield) mdev and mappings, but only >> mappings are editable >> >> we have to split the nodeselector into two fields, since the disabling >> cbind does not pass through to the editconfig (and thus makes the form >> invalid if we try that) >> >> Signed-off-by: Dominik Csapak >> --- >> this is not intended to be applied as is, rather i'd like some feedback >> on the approach (@thomas, @aaron ?) so that if we want to do it this way >> i can also do it for the usb mappings >> >> the other approach mentioned off-list can still be done >> (having a full grid with all mappings regardless of the node) >> maybe only for usb devices (there it makes imho more sense) but then >> we'd have two interfaces for the mappings instead of one > > It does involve a bit of clicking when it's only possible to add one > node entry at a time, but I'm not generally opposed to the current RFC. > I can image the action column takes a bit of getting used to as a > Proxmox VE user, because we don't really have those there yet. we have it in pbs and pmg, but yes it's a new pattern for pve i however find it less confusing than the toolbar buttons > > The full grid might become quite big/confusing and involve lots of > scrolling or how would the grouping by node be done? grouping/treestyle in such grids is always a bit tricky i am currently working on this approach for the usb mapping (as a demo of the other solution) but am running into quite a few weird extjs related thing regarding widget columns and field event handlers :( i'll see if i can send it tomorrow before lunch > > Maybe a third alternative would be to have a tab for each node and show > basic meta-info like how many devices are already selected on that node > and a warn/error indicator if that node is affected? mhmm that could be done, it's also a pattern we don't currently have: a dynamic amount of tabs for each node this could also become complex/confusing in a cluster with many nodes, especially the hint in the tab title would probably not be visible for all if there are many i'll think about it > > Would the full grid and tabs approach even be feasible with many nodes > or require too many API calls? depends how we implement it, in my naive grid solution from above, the api calls would be only done when a user selects a node i.e it would look like: ------------------------------------------------------------- | node | device | port | | ------------------------------------------------------------- | [nodeselector] | [usbselector] | [usbselector] | [X] | | | | | | | | | | | ------------------------------------------------------------- [add] for pci it would be probably only one drop down + 'all functions' checkbox the dropdowns only make an api call for the listing when the nodeselector in the line changes its value if we make more than one tab, we could defer the api call when the user changes to the tab > >> >> www/manager6/tree/ResourceMapTree.js | 166 ++++++++++++++++----------- >> www/manager6/window/PCIMapEdit.js | 42 ++++--- >> 2 files changed, 130 insertions(+), 78 deletions(-) >> >> diff --git a/www/manager6/tree/ResourceMapTree.js b/www/manager6/tree/ResourceMapTree.js >> index 02717042..cd24923e 100644 >> --- a/www/manager6/tree/ResourceMapTree.js >> +++ b/www/manager6/tree/ResourceMapTree.js >> @@ -49,44 +49,89 @@ Ext.define('PVE.tree.ResourceMapTree', { >> }); >> }, >> >> - addHost: function() { >> + add: function(_grid, _rI, _cI, _item, _e, rec) { >> let me = this; >> - me.edit(false); >> + if (!rec.data.type === 'entry') { > > AFAICT, this always evaluates to false, because of the misplaced '!'. oops ;) > >> + return; >> + } >> + >> + me.openMapEditWindow(rec.data.name); >> }, >> > > (...) > >> @@ -254,63 +299,56 @@ Ext.define('PVE.tree.ResourceMapTree', { >> >> tbar: [ >> { >> - text: gettext('Add mapping'), >> + text: gettext('Add'), > > IMHO, Add mapping was/is better OK > >> handler: 'addMapping', >> cbind: { >> disabled: '{!canConfigure}', >> }, >> }, > > (...) > >> diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js >> index 0da2bae7..a0b42758 100644 >> --- a/www/manager6/window/PCIMapEdit.js >> +++ b/www/manager6/window/PCIMapEdit.js >> @@ -13,8 +13,12 @@ Ext.define('PVE.window.PCIMapEditWindow', { >> >> cbindData: function(initialConfig) { >> let me = this; >> - me.isCreate = !me.name || !me.nodename; >> + me.isCreate = (!me.name || !me.nodename) && !me.entryOnly; >> me.method = me.name ? 'PUT' : 'POST'; >> + me.hideMapping = !!me.entryOnly; >> + me.hideComment = me.name && !me.entryOnly; >> + me.hideNodeSelector = me.nodename || me.entryOnly; >> + me.hideNode = !me.nodename || !me.hideNodeSelector; >> return { >> name: me.name, >> nodename: me.nodename, > > Nit: Is it even necessary to return these two as they are already > persistent properties? probably not, i guess this was leftover from some previous version