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