[pve-devel] [PATCH manager 1/4] ui: resource map tree: make 'ok' status clearer

[pve-devel] [PATCH manager 1/4] ui: resource map tree: make 'ok' status clearer

[Dominik Csapak]

by changing into 'mapping matches host data' which indicates that the
configured values matches the host information

also for the pci and usb map selectors

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/form/PCIMapSelector.js  | 2 +-
 www/manager6/form/USBMapSelector.js  | 2 +-
 www/manager6/tree/ResourceMapTree.js | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/www/manager6/form/PCIMapSelector.js b/www/manager6/form/PCIMapSelector.js
index 4dca62ea..1bc73ec0 100644
--- a/www/manager6/form/PCIMapSelector.js
+++ b/www/manager6/form/PCIMapSelector.js
@@ -48,7 +48,7 @@ Ext.define('PVE.form.PCIMapSelector', {
 		    let me = this;
 
 		    if (!Ext.isArray(value) || !value?.length) {
-			return `<i class="fa fa-check-circle good"></i> ${gettext('Mapping OK')}`;
+			return `<i class="fa fa-check-circle good"></i> ${gettext('Mapping matches host data')}`;
 		    }
 
 		    let checks = [];
diff --git a/www/manager6/form/USBMapSelector.js b/www/manager6/form/USBMapSelector.js
index 990ef30f..6a33754a 100644
--- a/www/manager6/form/USBMapSelector.js
+++ b/www/manager6/form/USBMapSelector.js
@@ -34,7 +34,7 @@ Ext.define('PVE.form.USBMapSelector', {
 		    let me = this;
 
 		    if (!Ext.isArray(value) || !value?.length) {
-			return `<i class="fa fa-check-circle good"></i> ${gettext('Mapping OK')}`;
+			return `<i class="fa fa-check-circle good"></i> ${gettext('Mapping matches host data')}`;
 		    }
 
 		    let errors = [];
diff --git a/www/manager6/tree/ResourceMapTree.js b/www/manager6/tree/ResourceMapTree.js
index df50b63a..02717042 100644
--- a/www/manager6/tree/ResourceMapTree.js
+++ b/www/manager6/tree/ResourceMapTree.js
@@ -228,7 +228,7 @@ Ext.define('PVE.tree.ResourceMapTree', {
 	    } else {
 		let state = value ? 'good' : 'critical';
 		iconCls = PVE.Utils.get_health_icon(state, true);
-		status = value ? gettext("OK") : record.data.errmsg || Proxmox.Utils.unknownText;
+		status = value ? gettext("Mapping matches host data") : record.data.errmsg || Proxmox.Utils.unknownText;
 	    }
 	    return `<i class="fa ${iconCls}"></i> ${status}`;
 	},
-- 
2.30.2

[pve-devel] [PATCH manager 2/4] ui: pci map edit: reintroduce warnings checks

[Dominik Csapak]

they got lost in my last rebase/refactor.

the onLoadCallBack is used to check by the window if there are iommu
groups at all, and the checkIsolated function checks if the selected
ones are in a separate group (in regards to the other devices)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/form/MultiPCISelector.js |  5 ++++
 www/manager6/window/PCIMapEdit.js     | 39 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/www/manager6/form/MultiPCISelector.js b/www/manager6/form/MultiPCISelector.js
index e1ef691a..d4fb6364 100644
--- a/www/manager6/form/MultiPCISelector.js
+++ b/www/manager6/form/MultiPCISelector.js
@@ -8,6 +8,9 @@ Ext.define('PVE.form.MultiPCISelector', {
 	field: 'Ext.form.field.Field',
     },
 
+    // will be called after loading finished
+    onLoadCallBack: Ext.emptyFn,
+
     getValue: function() {
 	let me = this;
 	return me.value ?? [];
@@ -287,6 +290,8 @@ Ext.define('PVE.form.MultiPCISelector', {
 
 	me.callParent();
 
+	me.mon(me.getStore(), 'load', me.onLoadCallBack);
+
 	Proxmox.Utils.monStoreErrors(me, me.getStore(), true);
 
 	me.setNodename(nodename);
diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js
index 516678e0..cd2dbfbe 100644
--- a/www/manager6/window/PCIMapEdit.js
+++ b/www/manager6/window/PCIMapEdit.js
@@ -70,6 +70,44 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 	    me.lookup('iommu_warning').setVisible(
 		records.every((val) => val.data.iommugroup === -1),
 	    );
+
+	    let value = me.lookup('pciselector').getValue();
+	    me.checkIsolated(value);
+	},
+
+	checkIsolated: function(value) {
+	    let me = this;
+
+	    let isIsolated = function(entry) {
+		let isolated = true;
+		let parsed = PVE.Parser.parsePropertyString(entry);
+		parsed.iommugroup = parseInt(parsed.iommugroup, 10);
+		if (!parsed.iommugroup) {
+		    return isolated;
+		}
+		me.lookup('pciselector').getStore().each(({ data }) => {
+		    let isSubDevice = data.id.startsWith(parsed.path);
+		    if (data.iommugroup === parsed.iommugroup && data.id !== parsed.path && !isSubDevice) {
+			isolated = false;
+			return false;
+		    }
+		    return true;
+		});
+		return isolated;
+	    };
+
+	    let showWarning = false;
+	    if (Ext.isArray(value)) {
+		for (const entry of value) {
+		    if (!isIsolated(entry)) {
+			showWarning = true;
+			break;
+		    }
+		}
+	    } else {
+		showWarning = isIsolated(value);
+	    }
+	    me.lookup('group_warning').setVisible(showWarning);
 	},
 
 	mdevChange: function(mdevField, value) {
@@ -83,6 +121,7 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 	pciChange: function(_field, value) {
 	    let me = this;
 	    me.lookup('multiple_warning').setVisible(Ext.isArray(value) && value.length > 1);
+	    me.checkIsolated(value);
 	},
 
 	control: {
-- 
2.30.2

Re: [pve-devel] [PATCH manager 2/4] ui: pci map edit: reintroduce warnings checks

[Fiona Ebner]

Am 19.06.23 um 16:13 schrieb Dominik Csapak:
> diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js
> index 516678e0..cd2dbfbe 100644
> --- a/www/manager6/window/PCIMapEdit.js
> +++ b/www/manager6/window/PCIMapEdit.js
> @@ -70,6 +70,44 @@ Ext.define('PVE.window.PCIMapEditWindow', {
>  	    me.lookup('iommu_warning').setVisible(
>  		records.every((val) => val.data.iommugroup === -1),
>  	    );
> +
> +	    let value = me.lookup('pciselector').getValue();
> +	    me.checkIsolated(value);
> +	},
> +
> +	checkIsolated: function(value) {
> +	    let me = this;
> +
> +	    let isIsolated = function(entry) {
> +		let isolated = true;
> +		let parsed = PVE.Parser.parsePropertyString(entry);
> +		parsed.iommugroup = parseInt(parsed.iommugroup, 10);

Nit: is there a simpler way to get the selected elements directly from
the store instead of going via getValue() above and then do the parsing
here?

> +		if (!parsed.iommugroup) {
> +		    return isolated;
> +		}
> +		me.lookup('pciselector').getStore().each(({ data }) => {

Nit: Feel a bit out of place to do the lookup here every time. Maybe
pass this in the store data as an argument to the function already?

> +		    let isSubDevice = data.id.startsWith(parsed.path);
> +		    if (data.iommugroup === parsed.iommugroup && data.id !== parsed.path && !isSubDevice) {
> +			isolated = false;
> +			return false;
> +		    }
> +		    return true;
> +		});
> +		return isolated;
> +	    };
> +
> +	    let showWarning = false;
> +	    if (Ext.isArray(value)) {
> +		for (const entry of value) {
> +		    if (!isIsolated(entry)) {
> +			showWarning = true;
> +			break;
> +		    }
> +		}
> +	    } else {
> +		showWarning = isIsolated(value);
> +	    }
> +	    me.lookup('group_warning').setVisible(showWarning);
>  	},
>  

Not in this patch, but the warning is

> The selected Device is not in a seperate IOMMU group, make sure this is intended.

which has two typos:
s/Device/device/
s/seperate/separate/
and I'd use s/The/A/ because multiple ones can be selected.

And the warnings could/should use gettext().

[pve-devel] [PATCH manager 3/4] ui: pci map edit: improve new host mappings dialog

[Dominik Csapak]

by disallowing nodes to be selected where a mapping already exists
and not preselecting a node

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/window/PCIMapEdit.js | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js
index cd2dbfbe..0da2bae7 100644
--- a/www/manager6/window/PCIMapEdit.js
+++ b/www/manager6/window/PCIMapEdit.js
@@ -58,7 +58,13 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 	    let me = this;
 	    let view = me.getView();
 	    me.originalMap = [...values.map];
-	    values.map = PVE.Parser.filterPropertyStringList(values.map, (e) => e.node === view.nodename);
+	    let configuredNodes = [];
+	    values.map = PVE.Parser.filterPropertyStringList(values.map, (e) => {
+		configuredNodes.push(e.node);
+		return e.node === view.nodename;
+	    });
+
+	    me.lookup('nodeselector').disallowedNodes = configuredNodes;
 	    return values;
 	},
 
@@ -201,6 +207,8 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 		    name: 'node',
 		    editConfig: {
 			xtype: 'pveNodeSelector',
+			autoSelect: false,
+			reference: 'nodeselector',
 		    },
 		    cbind: {
 			editable: '{!nodename}',
-- 
2.30.2

Re: [pve-devel] [PATCH manager 3/4] ui: pci map edit: improve new host mappings dialog

[Fiona Ebner]

Am 19.06.23 um 16:13 schrieb Dominik Csapak:
> by disallowing nodes to be selected where a mapping already exists
> and not preselecting a node
> 

Two UX nits, not sure how hard they would be to address:
- if all nodes already have a mapping, disable the button (or after the
RFC, hide the action element)
- maybe pre-select the first valid node that has no mapping yet instead
of none?

[pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[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

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

Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[Aaron Lauterer]

I like the approach as it cleans up the overloaded tbar that has items that are 
only valid in certain contexts.

Two small nits from a UX POV:

- double clicking any PCI device should open the edit dialog for the node, 
similar to double clicking the node itself
- the Action Column should probably be further left and not on the far right 
side by default. I personally like it to be the second column from the left as 
all other columns are rather informal.


I know it is kinda late, but would it be hard to add the "Device" column from 
the PCI device selection grid to the overview as well? This way one can easily 
verify that they got the right devices by name.
But probably it is a bit harder to gather the info from the other nodes?

On 6/19/23 16:13, Dominik Csapak wrote:
> 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
> 

Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[Dominik Csapak]

On 6/20/23 11:35, Aaron Lauterer wrote:
> I like the approach as it cleans up the overloaded tbar that has items that are only valid in 
> certain contexts.
> 
> Two small nits from a UX POV:
> 
> - double clicking any PCI device should open the edit dialog for the node, similar to double 
> clicking the node itself

makes sense imo

> - the Action Column should probably be further left and not on the far right side by default. I 
> personally like it to be the second column from the left as all other columns are rather informal.

mhmm can do that, but how i refactored that seems to be a bit hacky to inject an actioncolumn at a
certain position, but technically not a problem

> 
> 
> I know it is kinda late, but would it be hard to add the "Device" column from the PCI device 
> selection grid to the overview as well? This way one can easily verify that they got the right 
> devices by name.
> But probably it is a bit harder to gather the info from the other nodes?
> 

we already query the pci list of each node, so we could extract that from there
but this only works if the user has Sys.Audit which may not be the case.
then the column would be empty

> On 6/19/23 16:13, Dominik Csapak wrote:
>> 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
>>

Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[Aaron Lauterer]

On 6/20/23 11:57, Dominik Csapak wrote:
> On 6/20/23 11:35, Aaron Lauterer wrote:
>> I like the approach as it cleans up the overloaded tbar that has items that 
>> are only valid in certain contexts.
>>
>> Two small nits from a UX POV:
>>
>> - double clicking any PCI device should open the edit dialog for the node, 
>> similar to double clicking the node itself
> 
> makes sense imo
> 
>> - the Action Column should probably be further left and not on the far right 
>> side by default. I personally like it to be the second column from the left as 
>> all other columns are rather informal.
> 
> mhmm can do that, but how i refactored that seems to be a bit hacky to inject an 
> actioncolumn at a
> certain position, but technically not a problem

But aren't we doing that already in the content view of PBS? AFAIK it is the 3rd 
column there.

> 
>>
>>
>> I know it is kinda late, but would it be hard to add the "Device" column from 
>> the PCI device selection grid to the overview as well? This way one can easily 
>> verify that they got the right devices by name.
>> But probably it is a bit harder to gather the info from the other nodes?
>>
> 
> we already query the pci list of each node, so we could extract that from there
> but this only works if the user has Sys.Audit which may not be the case.
> then the column would be empty
> 
>> On 6/19/23 16:13, Dominik Csapak wrote:
>>> 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
>>>
> 
> 

Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[Dominik Csapak]

On 6/20/23 12:18, Aaron Lauterer wrote:
> 
> 
> On 6/20/23 11:57, Dominik Csapak wrote:
>> On 6/20/23 11:35, Aaron Lauterer wrote:
>>> I like the approach as it cleans up the overloaded tbar that has items that are only valid in 
>>> certain contexts.
>>>
>>> Two small nits from a UX POV:
>>>
>>> - double clicking any PCI device should open the edit dialog for the node, similar to double 
>>> clicking the node itself
>>
>> makes sense imo
>>
>>> - the Action Column should probably be further left and not on the far right side by default. I 
>>> personally like it to be the second column from the left as all other columns are rather informal.
>>
>> mhmm can do that, but how i refactored that seems to be a bit hacky to inject an actioncolumn at a
>> certain position, but technically not a problem
> 
> But aren't we doing that already in the content view of PBS? AFAIK it is the 3rd column there.
> 
>

sorry i didn't describe it properly. ofc it's possible to have the actioncolumn at any position
it's just that the way i have structured the code makes it slightly weird to do
(the columns are defined in the subclasses, but the actioncolumn would be injected in
the base class, so i'd have to split the columns array before inserting it)

Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[Fiona Ebner]

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 <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

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?

Re: [pve-devel] [RFC PATCH manager 4/4] ui: pci mapping: rework mapping panel for better user experience

[Dominik Csapak]

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 <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
> 
> 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