From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
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 82708A2876
 for <pve-devel@lists.proxmox.com>; Mon, 19 Jun 2023 16:13:40 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 5DFC02A75A
 for <pve-devel@lists.proxmox.com>; Mon, 19 Jun 2023 16:13:10 +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 <pve-devel@lists.proxmox.com>; Mon, 19 Jun 2023 16:13:09 +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 E43DE46879
 for <pve-devel@lists.proxmox.com>; Mon, 19 Jun 2023 16:13:08 +0200 (CEST)
From: Dominik Csapak <d.csapak@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Mon, 19 Jun 2023 16:13:07 +0200
Message-Id: <20230619141307.119430-4-d.csapak@proxmox.com>
X-Mailer: git-send-email 2.30.2
In-Reply-To: <20230619141307.119430-1-d.csapak@proxmox.com>
References: <20230619141307.119430-1-d.csapak@proxmox.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.016 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
 T_SCC_BODY_TEXT_LINE    -0.01 -
Subject: [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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 19 Jun 2023 14:13:40 -0000

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

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
* 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 <d.csapak@proxmox.com>
---
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

 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') {
+		return;
+	    }
+
+	    me.openMapEditWindow(rec.data.name);
 	},
 
-	edit: function(includeNodename = true) {
+	editDblClick: function() {
 	    let me = this;
 	    let view = me.getView();
 	    let selection = view.getSelection();
-	    if (!selection || !selection.length) {
+	    if (!selection || selection.length < 1) {
 		return;
 	    }
-	    let rec = selection[0];
-	    if (!view.canConfigure || (rec.data.type === 'entry' && includeNodename)) {
+
+	    me.edit(selection[0]);
+	},
+
+	editAction: function(_grid, _rI, _cI, _item, _e, rec) {
+	    this.edit(rec);
+	},
+
+	edit: function(rec) {
+	    let me = this;
+	    if (rec.data.type === 'map') {
 		return;
 	    }
 
+	    me.openMapEditWindow(rec.data.name, rec.data.node, rec.data.type === 'entry');
+	},
+
+	openMapEditWindow: function(name, nodename, entryOnly) {
+	    let me = this;
+	    let view = me.getView();
+
 	    Ext.create(view.editWindowClass, {
-		url: `${view.baseUrl}/${rec.data.name}`,
+		url: `${view.baseUrl}/${name}`,
 		autoShow: true,
 		autoLoad: true,
-		nodename: includeNodename ? rec.data.node : undefined,
-		name: rec.data.name,
+		entryOnly,
+		nodename,
+		name,
 		listeners: {
 		    destroy: () => me.load(),
 		},
 	    });
 	},
 
-	remove: function() {
+	remove: function(_grid, _rI, _cI, _item, _e, rec) {
 	    let me = this;
+	    let msg, id;
 	    let view = me.getView();
-	    let selection = view.getSelection();
-	    if (!selection || !selection.length) {
-		return;
+	    let confirmMsg;
+	    switch (rec.data.type) {
+		case 'entry':
+		    msg = gettext("Are you sure you want to remove '{0}'");
+		    confirmMsg = Ext.String.format(msg, rec.data.name);
+		    break;
+		case 'node':
+		    msg = gettext("Are you sure you want to remove '{0}' entries for '{1}'");
+		    confirmMsg = Ext.String.format(msg, rec.data.node, rec.data.name);
+		    break;
+		case 'map':
+		    msg = gettext("Are you sure you want to remove '{0}' on '{1}' for '{2}'");
+		    id = rec.data[view.entryIdProperty];
+		    confirmMsg = Ext.String.format(msg, id, rec.data.node, rec.data.name);
+		    break;
+		default:
+		    throw "invalid type";
 	    }
+	    Ext.Msg.confirm(gettext('Confirm'), confirmMsg, function(btn) {
+		if (btn === 'yes') {
+		    me.executeRemove(rec.data);
+		}
+	    });
+	},
+
+	executeRemove: function(data) {
+	    let me = this;
+	    let view = me.getView();
 
-	    let data = selection[0].data;
 	    let url = `${view.baseUrl}/${data.name}`;
 	    let method = 'PUT';
 	    let params = {
@@ -254,63 +299,56 @@ Ext.define('PVE.tree.ResourceMapTree', {
 
     tbar: [
 	{
-	    text: gettext('Add mapping'),
+	    text: gettext('Add'),
 	    handler: 'addMapping',
 	    cbind: {
 		disabled: '{!canConfigure}',
 	    },
 	},
-	{
-	    xtype: 'proxmoxButton',
-	    text: gettext('New Host mapping'),
-	    disabled: true,
-	    parentXType: 'treepanel',
-	    enableFn: function(_rec) {
-		return this.up('treepanel').canConfigure;
-	    },
-	    handler: 'addHost',
-	},
-	{
-	    xtype: 'proxmoxButton',
-	    text: gettext('Edit'),
-	    disabled: true,
-	    parentXType: 'treepanel',
-	    enableFn: function(rec) {
-		return rec && rec.data.type !== 'entry' && this.up('treepanel').canConfigure;
-	    },
-	    handler: 'edit',
-	},
-	{
-	    xtype: 'proxmoxButton',
-	    parentXType: 'treepanel',
-	    handler: 'remove',
-	    disabled: true,
-	    text: gettext('Remove'),
-	    enableFn: function(rec) {
-		return rec && this.up('treepanel').canConfigure;
-	    },
-	    confirmMsg: function(rec) {
-		let msg, id;
-		let view = this.up('treepanel');
-		switch (rec.data.type) {
-		    case 'entry':
-			msg = gettext("Are you sure you want to remove '{0}'");
-			return Ext.String.format(msg, rec.data.name);
-		    case 'node':
-			msg = gettext("Are you sure you want to remove '{0}' entries for '{1}'");
-			return Ext.String.format(msg, rec.data.node, rec.data.name);
-		    case 'map':
-			msg = gettext("Are you sure you want to remove '{0}' on '{1}' for '{2}'");
-			id = rec.data[view.entryIdProperty];
-			return Ext.String.format(msg, id, rec.data.node, rec.data.name);
-		    default:
-			throw "invalid type";
-		}
-	    },
-	},
     ],
 
     listeners: {
-	itemdblclick: 'edit',
+	itemdblclick: 'editDblClick',
+    },
+
+    initComponent: function() {
+	let me = this;
+
+	let columns = [...me.columns];
+	columns.push({
+	    xtype: 'actioncolumn',
+	    text: gettext('Actions'),
+	    width: 80,
+	    items: [
+		{
+		    getTip: (v, m, { data }) =>
+			Ext.String.format(gettext("Add new host mapping for '{0}'"), data.name),
+		    getClass: (v, m, { data }) => data.type === 'entry' ? 'fa fa-plus-circle' : 'pmx-hidden',
+		    isActionDisabled: (v, r, c, i, rec) => rec.data.type !== 'entry',
+		    handler: 'add',
+		},
+		{
+		    iconCls: 'fa fa-pencil',
+		    getTip: (v, m, { data }) => data.type === 'entry'
+			? Ext.String.format(gettext("Edit Mapping '{0}'"), data.name)
+			: Ext.String.format(gettext("Edit Mapping '{0}' for '{1}'"), data.name, data.node),
+		    getClass: (v, m, { data }) => data.type !== 'map' ? 'fa fa-pencil' : 'pmx-hidden',
+		    isActionDisabled: (v, r, c, i, rec) => rec.data.type === 'map',
+		    handler: 'editAction',
+		},
+		{
+		    iconCls: 'fa fa-trash-o',
+		    getTip: (v, m, { data }) => data.type === 'entry'
+			? Ext.String.format(gettext("Remove '{0}'"), data.name)
+			: data.type === 'node'
+			    ? Ext.String.format(gettext("Remove mapping for '{0}'"), data.node)
+			    : Ext.String.format(gettext("Remove mapping '{0}'"), data.path),
+		    handler: 'remove',
+		},
+	    ],
+	});
+	me.columns = columns;
+
+	me.callParent();
     },
 });
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,
@@ -201,36 +205,42 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 		    allowBlank: false,
 		},
 		{
-		    xtype: 'pmxDisplayEditField',
+		    xtype: 'displayfield',
 		    fieldLabel: gettext('Mapping on Node'),
 		    labelWidth: 120,
 		    name: 'node',
-		    editConfig: {
-			xtype: 'pveNodeSelector',
-			autoSelect: false,
-			reference: 'nodeselector',
-		    },
 		    cbind: {
-			editable: '{!nodename}',
 			value: '{nodename}',
+			disabled: '{hideNode}',
+			hidden: '{hideNode}',
+		    },
+		    allowBlank: false,
+		},
+		{
+		    xtype: 'pveNodeSelector',
+		    reference: 'nodeselector',
+		    fieldLabel: gettext('Mapping on Node'),
+		    labelWidth: 120,
+		    name: 'node',
+		    autoSelect: false,
+		    cbind: {
+			disabled: '{hideNodeSelector}',
+			hidden: '{hideNodeSelector}',
 		    },
 		    allowBlank: false,
 		},
 	    ],
 
 	    column2: [
-		{
-		    // as spacer
-		    xtype: 'displayfield',
-		},
 		{
 		    xtype: 'proxmoxcheckbox',
-		    fieldLabel: gettext('Mediated Devices'),
-		    labelWidth: 120,
+		    fieldLabel: gettext('Use with Mediated Devices'),
+		    labelWidth: 200,
 		    reference: 'mdev',
 		    name: 'mdev',
 		    cbind: {
 			deleteEmpty: '{!isCreate}',
+			disabled: '{hideComment}',
 		    },
 		},
 	    ],
@@ -245,6 +255,8 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 		    name: 'map',
 		    cbind: {
 			nodename: '{nodename}',
+			disabled: '{hideMapping}',
+			hidden: '{hideMapping}',
 		    },
 		    allowBlank: false,
 		    onLoadCallBack: 'checkIommu',
@@ -258,6 +270,8 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 		    name: 'description',
 		    cbind: {
 			deleteEmpty: '{!isCreate}',
+			disabled: '{hideComment}',
+			hidden: '{hideComment}',
 		    },
 		},
 	    ],
-- 
2.30.2