[pve-devel] [PATCH manager 1/3] ui: resource tree: fix order of types

[pve-devel] [PATCH manager 1/3] ui: resource tree: fix order of types

[Dominik Csapak]

when ordering items on the same level, we often sort first by type, but
do this via string comparison of the types.

this leads to weird results if we have e.g. lxc/node/qemu mixed in one
list, as nodes would sort in between lxc and qemu

this patch solves that issue by introducing a fixed order for types, so
we have direct control over the order when mixing types without having
to change the type names.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/tree/ResourceTree.js | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index 7b0c5dc8..640ebbf9 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -45,6 +45,18 @@ Ext.define('PVE.tree.ResourceTree', {
 
     useArrows: true,
 
+    // private
+    getTypeOrder: function(type) {
+	switch (type) {
+	    case 'lxc': return 0;
+	    case 'qemu': return 1;
+	    case 'node': return 2;
+	    case 'sdn': return 3;
+	    case 'storage': return 4;
+	    default: return 9;
+	}
+    },
+
     // private
     nodeSortFn: function(node1, node2) {
 	let me = this;
@@ -55,10 +67,9 @@ Ext.define('PVE.tree.ResourceTree', {
 	    let n2IsGuest = n2.type === 'qemu' || n2.type === 'lxc';
 	    if (me['group-guest-types'] || !n1IsGuest || !n2IsGuest) {
 		// first sort (group) by type
-		if (n1.type > n2.type) {
-		    return 1;
-		} else if (n1.type < n2.type) {
-		    return -1;
+		let res = me.getTypeOrder(n1.type) - me.getTypeOrder(n2.type);
+		if (res !== 0) {
+		    return res;
 		}
 	    }
 
-- 
2.39.5



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

[pve-devel] [PATCH manager 2/3] ui: view selector: prepare filterfn to be more efficient

[Dominik Csapak]

instead of just using the filterfn directly, return a function from
`getFilterFn`. This way, when we want to filter different things
depending on e.g. local storage values, we don't have to do that for
every element, but only once and return the finished filterFn.

while at it, rename 'filterfn' to camel case 'filterFn'.

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

diff --git a/www/manager6/form/ViewSelector.js b/www/manager6/form/ViewSelector.js
index ee16c227..f5de5c8e 100644
--- a/www/manager6/form/ViewSelector.js
+++ b/www/manager6/form/ViewSelector.js
@@ -30,12 +30,12 @@ Ext.define('PVE.form.ViewSelector', {
 		text: gettext('Pool View'),
 		groups: ['pool'],
 		// Pool View only lists VMs and Containers
-		filterfn: ({ data }) => data.type === 'qemu' || data.type === 'lxc' || data.type === 'pool',
+		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc' || data.type === 'pool',
 	    },
 	    tags: {
 		text: gettext('Tag View'),
 		groups: ['tag'],
-		filterfn: ({ data }) => data.type === 'qemu' || data.type === 'lxc',
+		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc',
 		groupRenderer: function(info) {
 		    let tag = PVE.Utils.renderTags(info.tag, PVE.UIOptions.tagOverrides);
 		    return `<span class="proxmox-tags-full">${tag}</span>`;
diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index 640ebbf9..312f958f 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -327,7 +327,7 @@ Ext.define('PVE.tree.ResourceTree', {
 
 	    // also check for name for when the tree is sorted by name
 	    let moveCheckAttrs = groups.concat(['node', 'template', 'name']);
-	    let filterfn = me.viewFilter.filterfn;
+	    let filterFn = me.viewFilter.getFilterFn ? me.viewFilter.getFilterFn() : Ext.identityFn;
 
 	    let reselect = false; // for disappeared nodes
 	    let index = pdata.dataIndex;
@@ -399,7 +399,7 @@ Ext.define('PVE.tree.ResourceTree', {
 		if (olditem) {
 		    return;
 		}
-		if (filterfn && !filterfn(item)) {
+		if (filterFn && !filterFn(item)) {
 		    return;
 		}
 		let info = Ext.apply({ leaf: true }, item.data);
-- 
2.39.5



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

[pve-devel] [PATCH manager 3/3] ui: resource tree: show nodes/storages in pool/tag view by default

[Dominik Csapak]

and make it configurable in the tree browser settings.
this makes now use of the new sorting order and the more efficient
'getFilterFn' method of the viewSelector

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
not sure if we should make each option seperate, so either
separate options for tag/pool view

or separate options for nodes/storages (sdn maybe too?)
or both (that would be 4 new options)

 www/manager6/UIOptions.js               |  1 +
 www/manager6/form/ViewSelector.js       | 16 ++++++++++++++--
 www/manager6/tree/ResourceTree.js       |  2 +-
 www/manager6/window/TreeSettingsEdit.js | 13 +++++++++++++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/www/manager6/UIOptions.js b/www/manager6/UIOptions.js
index 057c8f03..97b689b0 100644
--- a/www/manager6/UIOptions.js
+++ b/www/manager6/UIOptions.js
@@ -93,6 +93,7 @@ Ext.define('PVE.UIOptions', {
 	    'sort-field': 'vmid',
 	    'group-templates': true,
 	    'group-guest-types': true,
+	    'more-types': true,
 	};
 
 	return browserValues?.[key] ?? defaults[key];
diff --git a/www/manager6/form/ViewSelector.js b/www/manager6/form/ViewSelector.js
index f5de5c8e..8c656fc8 100644
--- a/www/manager6/form/ViewSelector.js
+++ b/www/manager6/form/ViewSelector.js
@@ -30,12 +30,24 @@ Ext.define('PVE.form.ViewSelector', {
 		text: gettext('Pool View'),
 		groups: ['pool'],
 		// Pool View only lists VMs and Containers
-		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc' || data.type === 'pool',
+		getFilterFn: function() {
+		    let types = ['qemu', 'lxc', 'pool'];
+		    if (PVE.UIOptions.getTreeSortingValue('more-types')) {
+			types.push('node', 'storage');
+		    }
+		    return ({data}) => types.indexOf(data.type) !== -1;
+		},
 	    },
 	    tags: {
 		text: gettext('Tag View'),
 		groups: ['tag'],
-		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc',
+		getFilterFn: function() {
+		    let types = ['qemu', 'lxc'];
+		    if (PVE.UIOptions.getTreeSortingValue('more-types')) {
+			types.push('node', 'storage');
+		    }
+		    return ({data}) => types.indexOf(data.type) !== -1;
+		},
 		groupRenderer: function(info) {
 		    let tag = PVE.Utils.renderTags(info.tag, PVE.UIOptions.tagOverrides);
 		    return `<span class="proxmox-tags-full">${tag}</span>`;
diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index 312f958f..0440d9c2 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -242,7 +242,7 @@ Ext.define('PVE.tree.ResourceTree', {
     saveSortingOptions: function() {
 	let me = this;
 	let changed = false;
-	for (const key of ['sort-field', 'group-templates', 'group-guest-types']) {
+	for (const key of ['sort-field', 'group-templates', 'group-guest-types', 'more-types']) {
 	    let newValue = PVE.UIOptions.getTreeSortingValue(key);
 	    if (me[key] !== newValue) {
 		me[key] = newValue;
diff --git a/www/manager6/window/TreeSettingsEdit.js b/www/manager6/window/TreeSettingsEdit.js
index b48415bc..ef2eda4a 100644
--- a/www/manager6/window/TreeSettingsEdit.js
+++ b/www/manager6/window/TreeSettingsEdit.js
@@ -55,6 +55,19 @@ Ext.define('PVE.window.TreeSettingsEdit', {
 		    value: '__default__',
 		    deleteEmpty: false,
 		},
+		{
+		    xtype: 'proxmoxKVComboBox',
+		    name: 'more-types',
+		    fieldLabel: gettext('Show Nodes/Storages in Pool/Tag View'),
+		    comboItems: [
+			['__default__', `${Proxmox.Utils.defaultText} (${gettext("Yes")})`],
+			[1, gettext('Yes')],
+			[0, gettext('No')],
+		    ],
+		    defaultValue: '__default__',
+		    value: '__default__',
+		    deleteEmpty: false,
+		},
 		{
 		    xtype: 'displayfield',
 		    userCls: 'pmx-hint',
-- 
2.39.5



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

[pve-devel] applied: [PATCH manager 1/3] ui: resource tree: fix order of types

[Thomas Lamprecht]

Am 12.11.24 um 12:45 schrieb Dominik Csapak:
> when ordering items on the same level, we often sort first by type, but
> do this via string comparison of the types.
> 
> this leads to weird results if we have e.g. lxc/node/qemu mixed in one
> list, as nodes would sort in between lxc and qemu
> 
> this patch solves that issue by introducing a fixed order for types, so
> we have direct control over the order when mixing types without having
> to change the type names.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/tree/ResourceTree.js | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
>

applied, thanks!


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

[pve-devel] applied: [PATCH manager 2/3] ui: view selector: prepare filterfn to be more efficient

[Thomas Lamprecht]

Am 12.11.24 um 12:45 schrieb Dominik Csapak:
> instead of just using the filterfn directly, return a function from
> `getFilterFn`. This way, when we want to filter different things
> depending on e.g. local storage values, we don't have to do that for
> every element, but only once and return the finished filterFn.
> 
> while at it, rename 'filterfn' to camel case 'filterFn'.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/form/ViewSelector.js | 4 ++--
>  www/manager6/tree/ResourceTree.js | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
>

applied, thanks!


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

Re: [pve-devel] [PATCH manager 3/3] ui: resource tree: show nodes/storages in pool/tag view by default

[Thomas Lamprecht]

Am 12.11.24 um 12:45 schrieb Dominik Csapak:
> and make it configurable in the tree browser settings.
> this makes now use of the new sorting order and the more efficient
> 'getFilterFn' method of the viewSelector

works OK besides some eslint warnings (see below), but what I noticed is
that we never listed storages under their assigned pools, which is IMO
a bit odd and might be something to fix separately.

Btw. having this default one for pool view might not be welcomed by all,
as some used this view to hide the nodes and that stuff for those people
that are "afraid" of them (managers and such), but no hard feelings here.

Grouping the resource without tag or pool under a collapsible node, say
"Resources without Tag/Pool" for avoiding spending time with naming, might
be a trade-off here.

Btw. having an expand-all and collapse-all inline button between view
selection and settings might be nice too for bigger setups, and probably
not that much work. I.e., somewhat similar to how the API viewer endpoint
tree has those [0], just not as header tools.

[0]: https://pve.proxmox.com/pve-docs/api-viewer/index.html


> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> not sure if we should make each option seperate, so either
> separate options for tag/pool view
> 
> or separate options for nodes/storages (sdn maybe too?)
> or both (that would be 4 new options)


If I'd do a multi-select combobox for each Pool and Tag view, but we might wait
that out to see if there is actual user demand.

> 
>  www/manager6/UIOptions.js               |  1 +
>  www/manager6/form/ViewSelector.js       | 16 ++++++++++++++--
>  www/manager6/tree/ResourceTree.js       |  2 +-
>  www/manager6/window/TreeSettingsEdit.js | 13 +++++++++++++
>  4 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/www/manager6/UIOptions.js b/www/manager6/UIOptions.js
> index 057c8f03..97b689b0 100644
> --- a/www/manager6/UIOptions.js
> +++ b/www/manager6/UIOptions.js
> @@ -93,6 +93,7 @@ Ext.define('PVE.UIOptions', {
>  	    'sort-field': 'vmid',
>  	    'group-templates': true,
>  	    'group-guest-types': true,
> +	    'more-types': true,
>  	};
>  
>  	return browserValues?.[key] ?? defaults[key];
> diff --git a/www/manager6/form/ViewSelector.js b/www/manager6/form/ViewSelector.js
> index f5de5c8e..8c656fc8 100644
> --- a/www/manager6/form/ViewSelector.js
> +++ b/www/manager6/form/ViewSelector.js
> @@ -30,12 +30,24 @@ Ext.define('PVE.form.ViewSelector', {
>  		text: gettext('Pool View'),
>  		groups: ['pool'],
>  		// Pool View only lists VMs and Containers
> -		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc' || data.type === 'pool',
> +		getFilterFn: function() {
> +		    let types = ['qemu', 'lxc', 'pool'];
> +		    if (PVE.UIOptions.getTreeSortingValue('more-types')) {
> +			types.push('node', 'storage');
> +		    }
> +		    return ({data}) => types.indexOf(data.type) !== -1;

above causes eslint to output warnings due to missing spaces for `{ data }`

> +		},
>  	    },
>  	    tags: {
>  		text: gettext('Tag View'),
>  		groups: ['tag'],
> -		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc',
> +		getFilterFn: function() {
> +		    let types = ['qemu', 'lxc'];
> +		    if (PVE.UIOptions.getTreeSortingValue('more-types')) {
> +			types.push('node', 'storage');
> +		    }
> +		    return ({data}) => types.indexOf(data.type) !== -1;

same eslint warning as above 



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

Re: [pve-devel] [PATCH manager 3/3] ui: resource tree: show nodes/storages in pool/tag view by default

[Dominik Csapak]

On 11/12/24 14:38, Thomas Lamprecht wrote:
> Am 12.11.24 um 12:45 schrieb Dominik Csapak:
>> and make it configurable in the tree browser settings.
>> this makes now use of the new sorting order and the more efficient
>> 'getFilterFn' method of the viewSelector
> 
> works OK besides some eslint warnings (see below), but what I noticed is
> that we never listed storages under their assigned pools, which is IMO
> a bit odd and might be something to fix separately.

yeah looking at this, i noticed we don't return the pools for storages
at all currently in the resources api call.

Also we can have a storage in multiple pools (in contrast to guests)
so that is a bit of a problem here. I could return the pools as comma separated
list (that would satisfy the return schema, but isn't pretty) and
use the same mechanism for duplicating storages as for the tag view.

should i go this route or should i omit this for the pool view for now
and simply add the toggle for the tag view?

> 
> Btw. having this default one for pool view might not be welcomed by all,
> as some used this view to hide the nodes and that stuff for those people
> that are "afraid" of them (managers and such), but no hard feelings here.
> 
> Grouping the resource without tag or pool under a collapsible node, say
> "Resources without Tag/Pool" for avoiding spending time with naming, might
> be a trade-off here.
> 
> Btw. having an expand-all and collapse-all inline button between view
> selection and settings might be nice too for bigger setups, and probably
> not that much work. I.e., somewhat similar to how the API viewer endpoint
> tree has those [0], just not as header tools.
> 
> [0]: https://pve.proxmox.com/pve-docs/api-viewer/index.html
> 
> 
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> not sure if we should make each option seperate, so either
>> separate options for tag/pool view
>>
>> or separate options for nodes/storages (sdn maybe too?)
>> or both (that would be 4 new options)
> 
> 
> If I'd do a multi-select combobox for each Pool and Tag view, but we might wait
> that out to see if there is actual user demand.
> 
>>
>>   www/manager6/UIOptions.js               |  1 +
>>   www/manager6/form/ViewSelector.js       | 16 ++++++++++++++--
>>   www/manager6/tree/ResourceTree.js       |  2 +-
>>   www/manager6/window/TreeSettingsEdit.js | 13 +++++++++++++
>>   4 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/www/manager6/UIOptions.js b/www/manager6/UIOptions.js
>> index 057c8f03..97b689b0 100644
>> --- a/www/manager6/UIOptions.js
>> +++ b/www/manager6/UIOptions.js
>> @@ -93,6 +93,7 @@ Ext.define('PVE.UIOptions', {
>>   	    'sort-field': 'vmid',
>>   	    'group-templates': true,
>>   	    'group-guest-types': true,
>> +	    'more-types': true,
>>   	};
>>   
>>   	return browserValues?.[key] ?? defaults[key];
>> diff --git a/www/manager6/form/ViewSelector.js b/www/manager6/form/ViewSelector.js
>> index f5de5c8e..8c656fc8 100644
>> --- a/www/manager6/form/ViewSelector.js
>> +++ b/www/manager6/form/ViewSelector.js
>> @@ -30,12 +30,24 @@ Ext.define('PVE.form.ViewSelector', {
>>   		text: gettext('Pool View'),
>>   		groups: ['pool'],
>>   		// Pool View only lists VMs and Containers
>> -		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc' || data.type === 'pool',
>> +		getFilterFn: function() {
>> +		    let types = ['qemu', 'lxc', 'pool'];
>> +		    if (PVE.UIOptions.getTreeSortingValue('more-types')) {
>> +			types.push('node', 'storage');
>> +		    }
>> +		    return ({data}) => types.indexOf(data.type) !== -1;
> 
> above causes eslint to output warnings due to missing spaces for `{ data }`
> 
>> +		},
>>   	    },
>>   	    tags: {
>>   		text: gettext('Tag View'),
>>   		groups: ['tag'],
>> -		getFilterFn: () => ({ data }) => data.type === 'qemu' || data.type === 'lxc',
>> +		getFilterFn: function() {
>> +		    let types = ['qemu', 'lxc'];
>> +		    if (PVE.UIOptions.getTreeSortingValue('more-types')) {
>> +			types.push('node', 'storage');
>> +		    }
>> +		    return ({data}) => types.indexOf(data.type) !== -1;
> 
> same eslint warning as above
> 

sorry, i have to stop testing gui patches with just 'make install'...


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