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 CA5F7A2C79 for ; Tue, 20 Jun 2023 15:25:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A124335C32 for ; Tue, 20 Jun 2023 15:25:12 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 20 Jun 2023 15:25:12 +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 A86C741CE5 for ; Tue, 20 Jun 2023 15:25:11 +0200 (CEST) Message-ID: <9423a607-64f8-0a29-b29e-dc1bdc4fa73c@proxmox.com> Date: Tue, 20 Jun 2023 15:25:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Content-Language: en-US To: Proxmox VE development discussion , Dominik Csapak References: <20230619141307.119430-1-d.csapak@proxmox.com> <20230619141307.119430-4-d.csapak@proxmox.com> From: Fiona Ebner In-Reply-To: <20230619141307.119430-4-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.005 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 NICE_REPLY_A -0.102 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 - 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 13:25:42 -0000 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. > 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. > * 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. The full grid might become quite big/confusing and involve lots of scrolling or how would the grouping by node be done? 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? Would the full grid and tabs approach even be feasible with many nodes or require too many API calls? > > 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 '!'. > + 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 > 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?