Re: [pve-devel] [PATCH manager v14 10/12] ui: add resource mapping view for directories

["Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>]

Some comments inline


Otherwise, please consider:

Reviewed-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
Tested-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>


On 04.03.25 12:58, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v14:
> * return HTML encoded comment
> 
>   www/manager6/Makefile         |  1 +
>   www/manager6/dc/Config.js     | 10 +++++++++
>   www/manager6/dc/DirMapView.js | 42 +++++++++++++++++++++++++++++++++++
>   3 files changed, 53 insertions(+)
>   create mode 100644 www/manager6/dc/DirMapView.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 4b8677e3..57c4d377 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -189,6 +189,7 @@ JSSRC= 							\
>   	dc/RealmSyncJob.js				\
>   	dc/PCIMapView.js				\
>   	dc/USBMapView.js				\
> +	dc/DirMapView.js				\
>   	lxc/CmdMenu.js					\
>   	lxc/Config.js					\
>   	lxc/CreateWizard.js				\
> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
> index 74728c83..2958fb88 100644
> --- a/www/manager6/dc/Config.js
> +++ b/www/manager6/dc/Config.js
> @@ -329,6 +329,16 @@ Ext.define('PVE.dc.Config', {
>   			    title: gettext('USB Devices'),
>   			    flex: 1,
>   			},
> +			{
> +			    xtype: 'splitter',
> +			    collapsible: false,
> +			    performCollapse: false,
> +			},
> +			{
> +			    xtype: 'pveDcDirMapView',
> +			    title: gettext('Directories'),
> +			    flex: 1,
> +			},

This is only indirectly related to your changes, but: I noticed that the 
"Resource Mappings" tab does not have that much vertical space anymore. 
While this is fine on my (large) screen, it might be an issue with 
smaller (e.g. laptop) screens. In case we want to add more resource 
mappings in the future, it will get even worse.

It might be worth thinking whether it makes sense to split it into tabs, 
similar to how we're doing it in the PBS GUI, or the Tasks / Cluster log 
in PVE. I know that this adds a third layer of GUI (Datacenter -> 
Resource Mappings -> click correct tab), but I think it might be a bit 
more organized, and the space would be used more efficiently.

>   		    ],
>   		},
>   	    );
> diff --git a/www/manager6/dc/DirMapView.js b/www/manager6/dc/DirMapView.js
> new file mode 100644
> index 00000000..ff0ce633
> --- /dev/null
> +++ b/www/manager6/dc/DirMapView.js
> @@ -0,0 +1,42 @@
> +Ext.define('pve-resource-dir-tree', {
> +    extend: 'Ext.data.Model',
> +    idProperty: 'internalId',
> +    fields: ['type', 'text', 'path', 'id', 'description', 'digest'],
> +});
> +
> +Ext.define('PVE.dc.DirMapView', {
> +    extend: 'PVE.tree.ResourceMapTree',
> +    alias: 'widget.pveDcDirMapView',
> +
> +    editWindowClass: 'PVE.window.DirMapEditWindow',
> +    baseUrl: '/cluster/mapping/dir',
> +    mapIconCls: 'fa fa-folder',
> +    entryIdProperty: 'path',
> +
> +    store: {
> +	sorters: 'text',
> +	model: 'pve-resource-dir-tree',
> +	data: {},
> +    },
> +
> +    columns: [
> +	{
> +	    xtype: 'treecolumn',
> +	    text: gettext('ID/Node'),
> +	    dataIndex: 'text',
> +	    width: 200,
> +	},
> +	{
> +	    text: gettext('announce-submounts'),

Minor remark: while I understand why naming it 'announce-submounts' 
makes it clearer which underlying option it sets, I would usually not 
display it like this in the GUI, but by using a nice label. Although I 
also think it's hard to find a short label that is easy to understand, 
so maybe - just maybe - it would make sense to add a short tooltip which 
explains what the option does. On the other hand, I know that the user 
can always press "Help" to get to the docs that explain everything in 
detail, so I would only add a tooltip if it's possible to easily explain 
what the feature does.

> +	    dataIndex: 'announce-submounts',
> +	},
> +	{
> +	    header: gettext('Comment'),
> +	    dataIndex: 'description',
> +	    renderer: function(value, _meta, record) {
> +		return Ext.String.htmlEncode(value ?? record.data.comment);
> +	    },
> +	    flex: 1,
> +	},
> +    ],
> +});



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel