[pve-devel] [PATCH cluster/manager] fix #1408: ui: make tree sorting configurable

[pve-devel] [PATCH cluster/manager] fix #1408: ui: make tree sorting configurable

[Dominik Csapak]

this series allows configuring the sorting of the resource tree

options are the sort-field, if guest types are grouped and if templates
are grouped seperately

it's configurable in the datacenter config and the browser local storage
(latter takes precedence)

not completely sure if the displaying/selecting the default is really
optimal by just showing 'Default'. (more in the comment of the specific
patch 2/3 in pve-manager)

pve-cluster:

Dominik Csapak (1):
  datacenter config: add options for sorting the gui tree

 data/PVE/DataCenterConfig.pm | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

pve-manager:

Dominik Csapak (3):
  ui: Utils: refactor refreshing the the resource store/tree
  ui: add TreeSortingEdit window
  fix #1408: ui: ResourceTree: sort the tree according to tree-sorting
    options

 www/manager6/Makefile                  |   1 +
 www/manager6/Utils.js                  |  17 +++-
 www/manager6/Workspace.js              |  27 +++++-
 www/manager6/dc/OptionView.js          |  11 +++
 www/manager6/tree/ResourceTree.js      |  54 +++++++++---
 www/manager6/window/TreeSortingEdit.js | 113 +++++++++++++++++++++++++
 6 files changed, 208 insertions(+), 15 deletions(-)
 create mode 100644 www/manager6/window/TreeSortingEdit.js

-- 
2.30.2

[pve-devel] [PATCH cluster 1/1] datacenter config: add options for sorting the gui tree

[Dominik Csapak]

namely the 'sort-field' (either vmid or name), 'group-templates' (if the
guest templates should be grouped seperately) and 'group-guest-types'
(if the guest types should be grouped or not)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 data/PVE/DataCenterConfig.pm | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/data/PVE/DataCenterConfig.pm b/data/PVE/DataCenterConfig.pm
index 7a24870..36e0ea6 100644
--- a/data/PVE/DataCenterConfig.pm
+++ b/data/PVE/DataCenterConfig.pm
@@ -219,6 +219,28 @@ my $user_tag_privs_format = {
     },
 };
 
+my $tree_sorting_format = {
+    'sort-field' => {
+	optional => 1,
+	type => 'string',
+	enum => ['vmid', 'name'],
+	default => 'vmid',
+	description => "Controls the field to use as sort order and primary display.",
+    },
+    'group-templates' => {
+	optional => 1,
+	type => 'boolean',
+	default => 1,
+	description => "If enabled, groups the guest templates seperately from regular guests.",
+    },
+    'group-guest-types' => {
+	optional => 1,
+	type => 'boolean',
+	default => 1,
+	description => "If enabled, groups the guests by type.",
+    },
+};
+
 my $datacenter_schema = {
     type => "object",
     additionalProperties => 0,
@@ -374,6 +396,12 @@ my $datacenter_schema = {
 	    pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
 	    typetext => "<tag>[;<tag>...]",
 	},
+	'tree-sorting' => {
+	    optional => 1,
+	    type => 'string',
+	    description => "Tree sorting and grouping options",
+	    format => $tree_sorting_format,
+	},
     },
 };
 
@@ -442,6 +470,10 @@ sub parse_datacenter_config {
 	$res->{'registered-tags'} = [split(';', $admin_tags)];
     }
 
+    if (my $tree_sorting = $res->{'tree-sorting'}) {
+	$res->{'tree-sorting'} = parse_property_string($tree_sorting_format, $tree_sorting);
+    }
+
     # for backwards compatibility only, new migration property has precedence
     if (defined($res->{migration_unsecure})) {
 	if (defined($res->{migration}->{type})) {
@@ -524,6 +556,11 @@ sub write_datacenter_config {
 	$cfg->{'registered-tags'} = join(';', sort $admin_tags->@*);
     }
 
+    if (ref(my $tree_sorting = $cfg->{'tree-sorting'})) {
+	$cfg->{'tree-sorting'} =
+	    PVE::JSONSchema::print_property_string($tree_sorting, $tree_sorting_format);
+    }
+
     my $comment = '';
     # add description as comment to top of file
     my $description = $cfg->{description} || '';
-- 
2.30.2

[pve-devel] [PATCH manager 1/3] ui: Utils: refactor refreshing the the resource store/tree

[Dominik Csapak]

we'll need it elsewhere too, and it was rather hidden in the
updateTagSettings call.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/Utils.js         | 3 +++
 www/manager6/dc/OptionView.js | 1 +
 2 files changed, 4 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index fcc668c3a..ee9e4bd5d 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1871,6 +1871,7 @@ Ext.define('PVE.Utils', {
 
 		PVE.Utils.updateTagList(PVE.UIOptions['allowed-tags']);
 		PVE.Utils.updateTagSettings(PVE.UIOptions?.['tag-style']);
+		PVE.Utils.fireUIUpdate();
 	    },
 	});
     },
@@ -1920,7 +1921,9 @@ Ext.define('PVE.Utils', {
 	}
 
 	Ext.ComponentQuery.query('pveResourceTree')[0].setUserCls(`proxmox-tags-${shape}`);
+    },
 
+    fireUIUpdate: function() {
 	if (!PVE.data.ResourceStore.isLoading() && PVE.data.ResourceStore.isLoaded()) {
 	    PVE.data.ResourceStore.fireEvent('load');
 	}
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index 9c803beef..2e4f76263 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -566,6 +566,7 @@ Ext.define('PVE.dc.OptionView', {
 
 	    PVE.UIOptions['tag-style'] = store.getById('tag-style')?.data?.value;
 	    PVE.Utils.updateTagSettings(PVE.UIOptions['tag-style']);
+	    PVE.Utils.fireUIUpdate();
 	});
 
 	me.on('activate', me.rstore.startUpdate);
-- 
2.30.2

Re: [pve-devel] [PATCH manager 1/3] ui: Utils: refactor refreshing the the resource store/tree

[Thomas Lamprecht]

Am 01/02/2023 um 16:49 schrieb Dominik Csapak:
> we'll need it elsewhere too, and it was rather hidden in the
> updateTagSettings call.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/Utils.js         | 3 +++
>  www/manager6/dc/OptionView.js | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index fcc668c3a..ee9e4bd5d 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1871,6 +1871,7 @@ Ext.define('PVE.Utils', {
>  
>  		PVE.Utils.updateTagList(PVE.UIOptions['allowed-tags']);
>  		PVE.Utils.updateTagSettings(PVE.UIOptions?.['tag-style']);
> +		PVE.Utils.fireUIUpdate();
>  	    },
>  	});
>      },
> @@ -1920,7 +1921,9 @@ Ext.define('PVE.Utils', {
>  	}
>  
>  	Ext.ComponentQuery.query('pveResourceTree')[0].setUserCls(`proxmox-tags-${shape}`);
> +    },
>  
> +    fireUIUpdate: function() {

naming is off for this as it uses the action desired (but that's not future proof)
instead of using the actual event being fired, e.g., do s/fireUIUpdate/fireUIConfigChange/

btw, utils _really_ is a convoluted mess that needs some basic cleanup rather sooner
than later...

>  	if (!PVE.data.ResourceStore.isLoading() && PVE.data.ResourceStore.isLoaded()) {
>  	    PVE.data.ResourceStore.fireEvent('load');
>  	}
> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
> index 9c803beef..2e4f76263 100644
> --- a/www/manager6/dc/OptionView.js
> +++ b/www/manager6/dc/OptionView.js
> @@ -566,6 +566,7 @@ Ext.define('PVE.dc.OptionView', {
>  
>  	    PVE.UIOptions['tag-style'] = store.getById('tag-style')?.data?.value;
>  	    PVE.Utils.updateTagSettings(PVE.UIOptions['tag-style']);
> +	    PVE.Utils.fireUIUpdate();
>  	});
>  
>  	me.on('activate', me.rstore.startUpdate);

[pve-devel] [PATCH manager 2/3] ui: add TreeSortingEdit window

[Dominik Csapak]

in cluster options (for datacenter.cfg) and besides the view selector
(for the browser local storage)

the edit window is the same for bot but either makes an api call or
saves the resulting values into the browser local storage.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
not sure about just showing 'Default' for the options in both cases.
I could make the text different for the datacenter options (there it's
fixed) and make the text dynamic for the browser local storage, but it
does not seem worth it really...

could also always show the inherent defaults, but that would be wrong
for the local storage when something is set in the datacenter config...

 www/manager6/Makefile                  |   1 +
 www/manager6/Utils.js                  |   2 +-
 www/manager6/Workspace.js              |  27 +++++-
 www/manager6/dc/OptionView.js          |  10 +++
 www/manager6/window/TreeSortingEdit.js | 113 +++++++++++++++++++++++++
 5 files changed, 150 insertions(+), 3 deletions(-)
 create mode 100644 www/manager6/window/TreeSortingEdit.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 05afeda40..157d4da52 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -119,6 +119,7 @@ JSSRC= 							\
 	window/ScheduleSimulator.js			\
 	window/Wizard.js				\
 	window/GuestDiskReassign.js				\
+	window/TreeSortingEdit.js			\
 	ha/Fencing.js					\
 	ha/GroupEdit.js					\
 	ha/GroupSelector.js				\
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index ee9e4bd5d..0c57e0844 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1865,7 +1865,7 @@ Ext.define('PVE.Utils', {
 		PVE.UIOptions = {
 		    'allowed-tags': [],
 		};
-		for (const option of ['allowed-tags', 'console', 'tag-style']) {
+		for (const option of ['allowed-tags', 'console', 'tag-style', 'tree-sorting']) {
 		    PVE.UIOptions[option] = response?.result?.data?.[option];
 		}
 
diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
index d0f7ff760..1963dc3f2 100644
--- a/www/manager6/Workspace.js
+++ b/www/manager6/Workspace.js
@@ -223,7 +223,9 @@ Ext.define('PVE.StdWorkspace', {
 	let appState = Ext.create('PVE.StateProvider');
 	Ext.state.Manager.setProvider(appState);
 
-	let selview = Ext.create('PVE.form.ViewSelector');
+	let selview = Ext.create('PVE.form.ViewSelector', {
+	    flex: 1,
+	});
 
 	let rtree = Ext.createWidget('pveResourceTree', {
 	    viewFilter: selview.getViewFilter(),
@@ -449,7 +451,28 @@ Ext.define('PVE.StdWorkspace', {
 		    margin: '0 0 0 5',
 		    split: true,
 		    width: 300,
-		    items: [selview, rtree],
+		    items: [
+			{
+			    xtype: 'container',
+			    layout: 'hbox',
+			    padding: '0 0 5 0',
+			    items: [
+				selview,
+				{
+				    xtype: 'button',
+				    iconCls: 'fa fa-fw fa-gear',
+				    handler: () => {
+					Ext.create('PVE.window.TreeSortingEdit', {
+					    autoShow: true,
+					    browserSettings: true,
+					    apiCallDone: () => PVE.Utils.fireUIUpdate(),
+					});
+				    },
+				},
+			    ],
+			},
+			rtree,
+		    ],
 		    listeners: {
 			resize: function(panel, width, height) {
 			    var viewWidth = me.getSize().width;
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index 2e4f76263..0f5eaa680 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -529,6 +529,15 @@ Ext.define('PVE.dc.OptionView', {
 	    },
 	};
 
+	me.rows['tree-sorting'] = {
+	    required: true,
+	    renderer: (value) => value ? PVE.Parser.printPropertyString(value) : gettext('No sorting overrides'),
+	    header: gettext('Tree Sorting'),
+	    editor: {
+		xtype: 'pveTreeSortingEdit',
+	    },
+	};
+
 	me.selModel = Ext.create('Ext.selection.RowModel', {});
 
 	Ext.apply(me, {
@@ -565,6 +574,7 @@ Ext.define('PVE.dc.OptionView', {
 	    }
 
 	    PVE.UIOptions['tag-style'] = store.getById('tag-style')?.data?.value;
+	    PVE.UIOptions['tree-sorting'] = store.getById('tree-sorting')?.data?.value;
 	    PVE.Utils.updateTagSettings(PVE.UIOptions['tag-style']);
 	    PVE.Utils.fireUIUpdate();
 	});
diff --git a/www/manager6/window/TreeSortingEdit.js b/www/manager6/window/TreeSortingEdit.js
new file mode 100644
index 000000000..eff7f3f21
--- /dev/null
+++ b/www/manager6/window/TreeSortingEdit.js
@@ -0,0 +1,113 @@
+Ext.define('PVE.window.TreeSortingEdit', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pveTreeSortingEdit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    title: gettext('Tree Sorting'),
+
+    isCreate: false,
+
+    url: '/cluster/options',
+
+    browserSettings: false,
+
+    items: [
+	{
+	    xtype: 'inputpanel',
+	    cbind: {},
+	    onSetValues: (values) => values?.['tree-sorting'] ?? {},
+	    onGetValues: function(values) {
+		if (this.up('window').browserSettings) {
+		    let sp = Ext.state.Manager.getProvider();
+		    let state = values ? values : null;
+		    sp.set('pve-tree-sorting', state);
+		    return {};
+		}
+		PVE.Utils.delete_if_default(values, 'group-templates', "1");
+		PVE.Utils.delete_if_default(values, 'group-guest-types', "1");
+		// no delete in property strings
+		delete values.delete;
+
+		let value = PVE.Parser.printPropertyString(values);
+
+		if (value === '') {
+		    return { 'delete': 'tree-sorting' };
+		}
+		return { 'tree-sorting': value };
+	    },
+	    items: [
+		{
+		    name: 'sort-field',
+		    xtype: 'proxmoxKVComboBox',
+		    fieldLabel: gettext('Sort Field'),
+		    labelWidth: 120,
+		    comboItems: [
+			['__default__', Proxmox.Utils.defaultText],
+			['vmid', gettext('VMID')],
+			['name', gettext('Name')],
+		    ],
+		    defaultValue: '__default__',
+		    value: '__default__',
+		    deleteEmpty: false,
+		},
+		{
+		    name: 'group-templates',
+		    xtype: 'booleanfield',
+		    fieldLabel: gettext('Group Templates'),
+		    defaultValue: '__default__',
+		    value: '__default__',
+		    deleteEmpty: false,
+		    labelWidth: 120,
+		},
+		{
+		    name: 'group-guest-types',
+		    xtype: 'booleanfield',
+		    fieldLabel: gettext('Group Types'),
+		    defaultValue: '__default__',
+		    value: '__default__',
+		    deleteEmpty: false,
+		    labelWidth: 120,
+		},
+		{
+		    xtype: 'displayfield',
+		    userCls: 'pmx-hint',
+		    value: gettext('Settings are saved in the browser local storage'),
+		    hidden: true,
+		    cbind: {
+			hidden: '{!browserSettings}',
+		    },
+		}
+	    ],
+	},
+    ],
+
+    submit: function() {
+	let me = this;
+
+	if (me.browserSettings) {
+	    me.down('inputpanel').getValues();
+	    me.apiCallDone();
+	    me.close();
+	} else {
+	    me.callParent();
+	}
+    },
+
+    initComponent: function() {
+	let me = this;
+
+	me.callParent();
+
+	me.load({
+	    success: function(response) {
+		let values = response?.result?.data?.['tree-sorting'] ?? {};
+		if (me.browserSettings) {
+		    let sp = Ext.state.Manager.getProvider();
+		    values = sp.get('pve-tree-sorting');
+		}
+		me.down('inputpanel').setValues({ 'tree-sorting': values });
+	    },
+	});
+    },
+
+});
-- 
2.30.2

Re: [pve-devel] [PATCH manager 2/3] ui: add TreeSortingEdit window

[Thomas Lamprecht]

looks like going in the right direction and feasible, some comments inline

high level: subject is using module name 1:1, as mentioned a few times please
avoid that, for ~ 99.9% cases it's not useful, for sure not for d/changelog but
not really for other devs in `git log` (sorting trees can mean widely different
things), maybe:

ui: add edit window for changing resource tree sort behavior

And for the module/file name I maybe wouldn't imply that this is for sorting
only, I could immagine that we expose other knobs for the tree behavior or
visuals in the future (e.g., add more font-weight to names, or 

Am 01/02/2023 um 16:49 schrieb Dominik Csapak:
> in cluster options (for datacenter.cfg) and besides the view selector
> (for the browser local storage)

I'd rather do it browser-local storage only, that way we can play a bit
around possbly adding or changing knob (names) and once stable for a time
and users are happy we can commit to adding this to datacenter config w

> 
> the edit window is the same for bot but either makes an api call or
> saves the resulting values into the browser local storage.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> not sure about just showing 'Default' for the options in both cases.
> I could make the text different for the datacenter options (there it's
> fixed) and make the text dynamic for the browser local storage, but it
> does not seem worth it really...
> 
> could also always show the inherent defaults, but that would be wrong
> for the local storage when something is set in the datacenter config...
> 
>  www/manager6/Makefile                  |   1 +
>  www/manager6/Utils.js                  |   2 +-
>  www/manager6/Workspace.js              |  27 +++++-
>  www/manager6/dc/OptionView.js          |  10 +++
>  www/manager6/window/TreeSortingEdit.js | 113 +++++++++++++++++++++++++
>  5 files changed, 150 insertions(+), 3 deletions(-)
>  create mode 100644 www/manager6/window/TreeSortingEdit.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 05afeda40..157d4da52 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -119,6 +119,7 @@ JSSRC= 							\
>  	window/ScheduleSimulator.js			\
>  	window/Wizard.js				\
>  	window/GuestDiskReassign.js				\
> +	window/TreeSortingEdit.js			\
>  	ha/Fencing.js					\
>  	ha/GroupEdit.js					\
>  	ha/GroupSelector.js				\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index ee9e4bd5d..0c57e0844 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1865,7 +1865,7 @@ Ext.define('PVE.Utils', {
>  		PVE.UIOptions = {
>  		    'allowed-tags': [],
>  		};
> -		for (const option of ['allowed-tags', 'console', 'tag-style']) {
> +		for (const option of ['allowed-tags', 'console', 'tag-style', 'tree-sorting']) {
>  		    PVE.UIOptions[option] = response?.result?.data?.[option];

a thought: why doesn't "PVE.UIOptions" real, i.e., a full fledged module that
hosts the respective functions and state that now is (mostly?) in Utils?

>  		}
>  
> diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
> index d0f7ff760..1963dc3f2 100644
> --- a/www/manager6/Workspace.js
> +++ b/www/manager6/Workspace.js
> @@ -223,7 +223,9 @@ Ext.define('PVE.StdWorkspace', {
>  	let appState = Ext.create('PVE.StateProvider');
>  	Ext.state.Manager.setProvider(appState);
>  
> -	let selview = Ext.create('PVE.form.ViewSelector');
> +	let selview = Ext.create('PVE.form.ViewSelector', {
> +	    flex: 1,
> +	});
>  
>  	let rtree = Ext.createWidget('pveResourceTree', {
>  	    viewFilter: selview.getViewFilter(),
> @@ -449,7 +451,28 @@ Ext.define('PVE.StdWorkspace', {
>  		    margin: '0 0 0 5',
>  		    split: true,
>  		    width: 300,
> -		    items: [selview, rtree],
> +		    items: [
> +			{
> +			    xtype: 'container',
> +			    layout: 'hbox',
> +			    padding: '0 0 5 0',
> +			    items: [
> +				selview,
> +				{
> +				    xtype: 'button',
> +				    iconCls: 'fa fa-fw fa-gear',
> +				    handler: () => {
> +					Ext.create('PVE.window.TreeSortingEdit', {
> +					    autoShow: true,
> +					    browserSettings: true,



> +					    apiCallDone: () => PVE.Utils.fireUIUpdate(),
> +					});
> +				    },
> +				},
> +			    ],
> +			},
> +			rtree,
> +		    ],
>  		    listeners: {
>  			resize: function(panel, width, height) {
>  			    var viewWidth = me.getSize().width;
> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
> index 2e4f76263..0f5eaa680 100644
> --- a/www/manager6/dc/OptionView.js
> +++ b/www/manager6/dc/OptionView.js
> @@ -529,6 +529,15 @@ Ext.define('PVE.dc.OptionView', {
>  	    },
>  	};
>  
> +	me.rows['tree-sorting'] = {
> +	    required: true,
> +	    renderer: (value) => value ? PVE.Parser.printPropertyString(value) : gettext('No sorting overrides'),
> +	    header: gettext('Tree Sorting'),
> +	    editor: {
> +		xtype: 'pveTreeSortingEdit',
> +	    },
> +	};
> +
>  	me.selModel = Ext.create('Ext.selection.RowModel', {});
>  
>  	Ext.apply(me, {
> @@ -565,6 +574,7 @@ Ext.define('PVE.dc.OptionView', {
>  	    }
>  
>  	    PVE.UIOptions['tag-style'] = store.getById('tag-style')?.data?.value;
> +	    PVE.UIOptions['tree-sorting'] = store.getById('tree-sorting')?.data?.value;
>  	    PVE.Utils.updateTagSettings(PVE.UIOptions['tag-style']);
>  	    PVE.Utils.fireUIUpdate();
>  	});
> diff --git a/www/manager6/window/TreeSortingEdit.js b/www/manager6/window/TreeSortingEdit.js
> new file mode 100644
> index 000000000..eff7f3f21
> --- /dev/null
> +++ b/www/manager6/window/TreeSortingEdit.js
> @@ -0,0 +1,113 @@
> +Ext.define('PVE.window.TreeSortingEdit', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveTreeSortingEdit',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    title: gettext('Tree Sorting'),
> +
> +    isCreate: false,
> +
> +    url: '/cluster/options',
> +
> +    browserSettings: false,

is above a preparation of exposing this additonally in the Datacenter Options
panel for configuring the backend config values, not the browser local ones?

I'd probably favor doing that via two widgets, one deriving from the other - but
if we only allow brwoser local storage as starter we'd need the differentation
at all for now.

> +
> +    items: [
> +	{
> +	    xtype: 'inputpanel',
> +	    cbind: {},
> +	    onSetValues: (values) => values?.['tree-sorting'] ?? {},
> +	    onGetValues: function(values) {
> +		if (this.up('window').browserSettings) {
> +		    let sp = Ext.state.Manager.getProvider();

nit: would like to avoid very short abbreviations which could get confusing if
this ever expands or gets used as template and copied.

s/sp/stateProvider/ or maybe even s/sp/localStorage/

> +		    let state = values ? values : null;
> +		    sp.set('pve-tree-sorting', state);

could drop intermediate variable

sp.set('pve-tree-sorting', values || null);

> +		    return {};
> +		}
> +		PVE.Utils.delete_if_default(values, 'group-templates', "1");
> +		PVE.Utils.delete_if_default(values, 'group-guest-types', "1");
> +		// no delete in property strings
> +		delete values.delete;
> +
> +		let value = PVE.Parser.printPropertyString(values);
> +
> +		if (value === '') {
> +		    return { 'delete': 'tree-sorting' };
> +		}
> +		return { 'tree-sorting': value };
> +	    },
> +	    items: [
> +		{
> +		    name: 'sort-field',
> +		    xtype: 'proxmoxKVComboBox',

please put xtype first

> +		    fieldLabel: gettext('Sort Field'),
> +		    labelWidth: 120,

might want to but that in a defaults: {} config at inputpanel level

> +		    comboItems: [
> +			['__default__', Proxmox.Utils.defaultText],
> +			['vmid', gettext('VMID')],

do we really translate VMID, as in, is there a (updated in the last 12 months)
translation that sets this to something different and sensible?

> +			['name', gettext('Name')],
> +		    ],
> +		    defaultValue: '__default__',
> +		    value: '__default__',
> +		    deleteEmpty: false,
> +		},
> +		{
> +		    name: 'group-templates',
> +		    xtype: 'booleanfield',

please put xtype first

besides that and unrelated to your series, this widget is a bit odd and as it
has no pve/proxmox prefix (I initially thought it's a ExtJS one that I just
never stumbled uppon) and should get renamed to something more telling like
pmxBooleanComboBox, probably should move to widget toolkit too

> +		    fieldLabel: gettext('Group Templates'),
> +		    defaultValue: '__default__',
> +		    value: '__default__',
> +		    deleteEmpty: false,
> +		    labelWidth: 120,
> +		},
> +		{
> +		    name: 'group-guest-types',
> +		    xtype: 'booleanfield',

please put xtype first

> +		    fieldLabel: gettext('Group Types'),
> +		    defaultValue: '__default__',
> +		    value: '__default__',
> +		    deleteEmpty: false,
> +		    labelWidth: 120,
> +		},
> +		{
> +		    xtype: 'displayfield',
> +		    userCls: 'pmx-hint',
> +		    value: gettext('Settings are saved in the browser local storage'),
> +		    hidden: true,
> +		    cbind: {
> +			hidden: '{!browserSettings}',
> +		    },
> +		}
> +	    ],
> +	},
> +    ],
> +
> +    submit: function() {
> +	let me = this;
> +
> +	if (me.browserSettings) {
> +	    me.down('inputpanel').getValues();
> +	    me.apiCallDone();
> +	    me.close();
> +	} else {
> +	    me.callParent();
> +	}
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	me.callParent();
> +
> +	me.load({
> +	    success: function(response) {
> +		let values = response?.result?.data?.['tree-sorting'] ?? {};
> +		if (me.browserSettings) {
> +		    let sp = Ext.state.Manager.getProvider();
> +		    values = sp.get('pve-tree-sorting');
> +		}
> +		me.down('inputpanel').setValues({ 'tree-sorting': values });
> +	    },
> +	});
> +    },
> +
> +});

Re: [pve-devel] [PATCH manager 2/3] ui: add TreeSortingEdit window

[Dominik Csapak]

On 2/2/23 11:56, Thomas Lamprecht wrote:
> looks like going in the right direction and feasible, some comments inline
> 
> high level: subject is using module name 1:1, as mentioned a few times please
> avoid that, for ~ 99.9% cases it's not useful, for sure not for d/changelog but
> not really for other devs in `git log` (sorting trees can mean widely different
> things), maybe:
> 
> ui: add edit window for changing resource tree sort behavior

yes, sorry, for adding new widgets i still default in my head to just
adding the class name...

> 
> And for the module/file name I maybe wouldn't imply that this is for sorting
> only, I could immagine that we expose other knobs for the tree behavior or
> visuals in the future (e.g., add more font-weight to names, or

yeah sure, does 'TreeSettingsEdit' (or 'TreeOptionsEdit') make sense?

> 
> Am 01/02/2023 um 16:49 schrieb Dominik Csapak:
>> in cluster options (for datacenter.cfg) and besides the view selector
>> (for the browser local storage)
> 
> I'd rather do it browser-local storage only, that way we can play a bit
> around possbly adding or changing knob (names) and once stable for a time
> and users are happy we can commit to adding this to datacenter config w
> 

yeah sure

>>
>> the edit window is the same for bot but either makes an api call or
>> saves the resulting values into the browser local storage.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> not sure about just showing 'Default' for the options in both cases.
>> I could make the text different for the datacenter options (there it's
>> fixed) and make the text dynamic for the browser local storage, but it
>> does not seem worth it really...
>>
>> could also always show the inherent defaults, but that would be wrong
>> for the local storage when something is set in the datacenter config...

any commend on this?


>>
>>   www/manager6/Makefile                  |   1 +
>>   www/manager6/Utils.js                  |   2 +-
>>   www/manager6/Workspace.js              |  27 +++++-
>>   www/manager6/dc/OptionView.js          |  10 +++
>>   www/manager6/window/TreeSortingEdit.js | 113 +++++++++++++++++++++++++
>>   5 files changed, 150 insertions(+), 3 deletions(-)
>>   create mode 100644 www/manager6/window/TreeSortingEdit.js
>>
>> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
>> index 05afeda40..157d4da52 100644
>> --- a/www/manager6/Makefile
>> +++ b/www/manager6/Makefile
>> @@ -119,6 +119,7 @@ JSSRC= 							\
>>   	window/ScheduleSimulator.js			\
>>   	window/Wizard.js				\
>>   	window/GuestDiskReassign.js				\
>> +	window/TreeSortingEdit.js			\
>>   	ha/Fencing.js					\
>>   	ha/GroupEdit.js					\
>>   	ha/GroupSelector.js				\
>> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
>> index ee9e4bd5d..0c57e0844 100644
>> --- a/www/manager6/Utils.js
>> +++ b/www/manager6/Utils.js
>> @@ -1865,7 +1865,7 @@ Ext.define('PVE.Utils', {
>>   		PVE.UIOptions = {
>>   		    'allowed-tags': [],
>>   		};
>> -		for (const option of ['allowed-tags', 'console', 'tag-style']) {
>> +		for (const option of ['allowed-tags', 'console', 'tag-style', 'tree-sorting']) {
>>   		    PVE.UIOptions[option] = response?.result?.data?.[option];
> 
> a thought: why doesn't "PVE.UIOptions" real, i.e., a full fledged module that
> hosts the respective functions and state that now is (mostly?) in Utils?

there wasn't that much in there, but i agree it's own module does make more sense,
i'll do a refactor in a v2

> 
>>   		}
>>   
>> diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
>> index d0f7ff760..1963dc3f2 100644
>> --- a/www/manager6/Workspace.js
>> +++ b/www/manager6/Workspace.js
>> @@ -223,7 +223,9 @@ Ext.define('PVE.StdWorkspace', {
>>   	let appState = Ext.create('PVE.StateProvider');
>>   	Ext.state.Manager.setProvider(appState);
>>   
>> -	let selview = Ext.create('PVE.form.ViewSelector');
>> +	let selview = Ext.create('PVE.form.ViewSelector', {
>> +	    flex: 1,
>> +	});
>>   
>>   	let rtree = Ext.createWidget('pveResourceTree', {
>>   	    viewFilter: selview.getViewFilter(),
>> @@ -449,7 +451,28 @@ Ext.define('PVE.StdWorkspace', {
>>   		    margin: '0 0 0 5',
>>   		    split: true,
>>   		    width: 300,
>> -		    items: [selview, rtree],
>> +		    items: [
>> +			{
>> +			    xtype: 'container',
>> +			    layout: 'hbox',
>> +			    padding: '0 0 5 0',
>> +			    items: [
>> +				selview,
>> +				{
>> +				    xtype: 'button',
>> +				    iconCls: 'fa fa-fw fa-gear',
>> +				    handler: () => {
>> +					Ext.create('PVE.window.TreeSortingEdit', {
>> +					    autoShow: true,
>> +					    browserSettings: true,
> 
> 
> 
>> +					    apiCallDone: () => PVE.Utils.fireUIUpdate(),
>> +					});
>> +				    },
>> +				},
>> +			    ],
>> +			},
>> +			rtree,
>> +		    ],
>>   		    listeners: {
>>   			resize: function(panel, width, height) {
>>   			    var viewWidth = me.getSize().width;
>> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
>> index 2e4f76263..0f5eaa680 100644
>> --- a/www/manager6/dc/OptionView.js
>> +++ b/www/manager6/dc/OptionView.js
>> @@ -529,6 +529,15 @@ Ext.define('PVE.dc.OptionView', {
>>   	    },
>>   	};
>>   
>> +	me.rows['tree-sorting'] = {
>> +	    required: true,
>> +	    renderer: (value) => value ? PVE.Parser.printPropertyString(value) : gettext('No sorting overrides'),
>> +	    header: gettext('Tree Sorting'),
>> +	    editor: {
>> +		xtype: 'pveTreeSortingEdit',
>> +	    },
>> +	};
>> +
>>   	me.selModel = Ext.create('Ext.selection.RowModel', {});
>>   
>>   	Ext.apply(me, {
>> @@ -565,6 +574,7 @@ Ext.define('PVE.dc.OptionView', {
>>   	    }
>>   
>>   	    PVE.UIOptions['tag-style'] = store.getById('tag-style')?.data?.value;
>> +	    PVE.UIOptions['tree-sorting'] = store.getById('tree-sorting')?.data?.value;
>>   	    PVE.Utils.updateTagSettings(PVE.UIOptions['tag-style']);
>>   	    PVE.Utils.fireUIUpdate();
>>   	});
>> diff --git a/www/manager6/window/TreeSortingEdit.js b/www/manager6/window/TreeSortingEdit.js
>> new file mode 100644
>> index 000000000..eff7f3f21
>> --- /dev/null
>> +++ b/www/manager6/window/TreeSortingEdit.js
>> @@ -0,0 +1,113 @@
>> +Ext.define('PVE.window.TreeSortingEdit', {
>> +    extend: 'Proxmox.window.Edit',
>> +    alias: 'widget.pveTreeSortingEdit',
>> +    mixins: ['Proxmox.Mixin.CBind'],
>> +
>> +    title: gettext('Tree Sorting'),
>> +
>> +    isCreate: false,
>> +
>> +    url: '/cluster/options',
>> +
>> +    browserSettings: false,
> 
> is above a preparation of exposing this additonally in the Datacenter Options
> panel for configuring the backend config values, not the browser local ones?
> 
> I'd probably favor doing that via two widgets, one deriving from the other - but
> if we only allow brwoser local storage as starter we'd need the differentation
> at all for now.

yeah i'm using that in the next patch to also expose it in the datacenter options
but i'll drop it for browser local storage only

> 
>> +
>> +    items: [
>> +	{
>> +	    xtype: 'inputpanel',
>> +	    cbind: {},
>> +	    onSetValues: (values) => values?.['tree-sorting'] ?? {},
>> +	    onGetValues: function(values) {
>> +		if (this.up('window').browserSettings) {
>> +		    let sp = Ext.state.Manager.getProvider();
> 
> nit: would like to avoid very short abbreviations which could get confusing if
> this ever expands or gets used as template and copied.
> 
> s/sp/stateProvider/ or maybe even s/sp/localStorage/
> 
>> +		    let state = values ? values : null;
>> +		    sp.set('pve-tree-sorting', state);
> 
> could drop intermediate variable
> 
> sp.set('pve-tree-sorting', values || null);
> 
>> +		    return {};
>> +		}
>> +		PVE.Utils.delete_if_default(values, 'group-templates', "1");
>> +		PVE.Utils.delete_if_default(values, 'group-guest-types', "1");
>> +		// no delete in property strings
>> +		delete values.delete;
>> +
>> +		let value = PVE.Parser.printPropertyString(values);
>> +
>> +		if (value === '') {
>> +		    return { 'delete': 'tree-sorting' };
>> +		}
>> +		return { 'tree-sorting': value };
>> +	    },
>> +	    items: [
>> +		{
>> +		    name: 'sort-field',
>> +		    xtype: 'proxmoxKVComboBox',
> 
> please put xtype first
> 
>> +		    fieldLabel: gettext('Sort Field'),
>> +		    labelWidth: 120,
> 
> might want to but that in a defaults: {} config at inputpanel level
> 
>> +		    comboItems: [
>> +			['__default__', Proxmox.Utils.defaultText],
>> +			['vmid', gettext('VMID')],
> 
> do we really translate VMID, as in, is there a (updated in the last 12 months)
> translation that sets this to something different and sensible?

only arabic seems to translate VMID, and not even consistent
(it's VMID in some other arabic strings) so i think we would be fine to
drop that. we're not consitently using gettext for it anyway

> 
>> +			['name', gettext('Name')],
>> +		    ],
>> +		    defaultValue: '__default__',
>> +		    value: '__default__',
>> +		    deleteEmpty: false,
>> +		},
>> +		{
>> +		    name: 'group-templates',
>> +		    xtype: 'booleanfield',
> 
> please put xtype first
> 
> besides that and unrelated to your series, this widget is a bit odd and as it
> has no pve/proxmox prefix (I initially thought it's a ExtJS one that I just
> never stumbled uppon) and should get renamed to something more telling like
> pmxBooleanComboBox, probably should move to widget toolkit too

sure

> 
>> +		    fieldLabel: gettext('Group Templates'),
>> +		    defaultValue: '__default__',
>> +		    value: '__default__',
>> +		    deleteEmpty: false,
>> +		    labelWidth: 120,
>> +		},
>> +		{
>> +		    name: 'group-guest-types',
>> +		    xtype: 'booleanfield',
> 
> please put xtype first
> 
>> +		    fieldLabel: gettext('Group Types'),
>> +		    defaultValue: '__default__',
>> +		    value: '__default__',
>> +		    deleteEmpty: false,
>> +		    labelWidth: 120,
>> +		},
>> +		{
>> +		    xtype: 'displayfield',
>> +		    userCls: 'pmx-hint',
>> +		    value: gettext('Settings are saved in the browser local storage'),
>> +		    hidden: true,
>> +		    cbind: {
>> +			hidden: '{!browserSettings}',
>> +		    },
>> +		}
>> +	    ],
>> +	},
>> +    ],
>> +
>> +    submit: function() {
>> +	let me = this;
>> +
>> +	if (me.browserSettings) {
>> +	    me.down('inputpanel').getValues();
>> +	    me.apiCallDone();
>> +	    me.close();
>> +	} else {
>> +	    me.callParent();
>> +	}
>> +    },
>> +
>> +    initComponent: function() {
>> +	let me = this;
>> +
>> +	me.callParent();
>> +
>> +	me.load({
>> +	    success: function(response) {
>> +		let values = response?.result?.data?.['tree-sorting'] ?? {};
>> +		if (me.browserSettings) {
>> +		    let sp = Ext.state.Manager.getProvider();
>> +		    values = sp.get('pve-tree-sorting');
>> +		}
>> +		me.down('inputpanel').setValues({ 'tree-sorting': values });
>> +	    },
>> +	});
>> +    },
>> +
>> +});
> 

[pve-devel] [PATCH manager 3/3] fix #1408: ui: ResourceTree: sort the tree according to tree-sorting options

[Dominik Csapak]

Considers the newly added options from both browser local storage(preferred)
or from the datacenter config we have to save the last sorting mechanism
there, so we can detect if it changes and trigger the movement/text changes
(otherwise the tree nodes won't be updated/moved)

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

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 0c57e0844..14f03e6d1 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1930,6 +1930,18 @@ Ext.define('PVE.Utils', {
 	Ext.GlobalEvents.fireEvent('loadedUiOptions');
     },
 
+    getTreeSortingValue: function(key) {
+	let sp = Ext.state.Manager.getProvider();
+	let browserValues = sp.get('pve-tree-sorting');
+	let defaults = {
+	    'sort-field': 'vmid',
+	    'group-templates': true,
+	    'group-guest-types': true,
+	};
+
+	return browserValues?.[key] ?? PVE.UIOptions?.['tree-sorting']?.[key] ?? defaults[key];
+    },
+
     tagTreeStyles: {
 	'__default__': `${Proxmox.Utils.defaultText} (${gettext('Circle')})`,
 	'full': gettext('Full'),
diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index 5c92d4128..5456a7eda 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -44,24 +44,34 @@ Ext.define('PVE.tree.ResourceTree', {
 
     // private
     nodeSortFn: function(node1, node2) {
+	let me = this;
 	let n1 = node1.data, n2 = node2.data;
 
 	if (!n1.groupbyid === !n2.groupbyid) {
-	    // first sort (group) by type
-	    if (n1.type > n2.type) {
-		return 1;
-	    } else if (n1.type < n2.type) {
-		return -1;
+	    let n1IsGuest = n1.type === 'qemu' || n1.type === 'lxc';
+	    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;
+		}
 	    }
+
 	    // then sort (group) by ID
-	    if (n1.type === 'qemu' || n2.type === 'lxc') {
-		if (!n1.template !== !n2.template) {
+	    if (n1IsGuest) {
+		if (me['group-templates'] && (!n1.template !== !n2.template)) {
 		    return n1.template ? 1 : -1; // sort templates after regular VMs
 		}
-		if (n1.vmid > n2.vmid) { // prefer VMID as metric for guests
-		    return 1;
-		} else if (n1.vmid < n2.vmid) {
-		    return -1;
+		if (me['sort-field'] === 'vmid') {
+		    if (n1.vmid > n2.vmid) { // prefer VMID as metric for guests
+			return 1;
+		    } else if (n1.vmid < n2.vmid) {
+			return -1;
+		    }
+		} else {
+		    return n1.name.localeCompare(n2.name);
 		}
 	    }
 	    // same types but not a guest
@@ -115,6 +125,11 @@ Ext.define('PVE.tree.ResourceTree', {
 		status += '</div> ';
 	    }
 	}
+	if (Ext.isNumeric(info.vmid) && info.vmid > 0) {
+	    if (PVE.Utils.getTreeSortingValue('sort-field') !== 'vmid') {
+		info.text = `${info.name} (${String(info.vmid)})`;
+	    }
+	}
 
 	info.text += PVE.Utils.renderTags(info.tags, PVE.Utils.tagOverrides);
 
@@ -203,8 +218,22 @@ Ext.define('PVE.tree.ResourceTree', {
 	return me.addChildSorted(node, info);
     },
 
+    saveSortingOptions: function() {
+	let me = this;
+	let changed = false;
+	for (const key of ['sort-field', 'group-templates', 'group-guest-types']) {
+	    let newValue = PVE.Utils.getTreeSortingValue(key);
+	    if (me[key] !== newValue) {
+		me[key] = newValue;
+		changed = true;
+	    }
+	}
+	return changed;
+    },
+
     initComponent: function() {
 	let me = this;
+	me.saveSortingOptions();
 
 	let rstore = PVE.data.ResourceStore;
 	let sp = Ext.state.Manager.getProvider();
@@ -242,6 +271,7 @@ Ext.define('PVE.tree.ResourceTree', {
 	    let sm = me.getSelectionModel();
 	    let lastsel = sm.getSelection()[0];
 	    let parents = [];
+	    let sorting_changed = me.saveSortingOptions();
 	    for (let node = lastsel; node; node = node.parentNode) {
 		parents.push(node);
 	    }
@@ -258,7 +288,7 @@ Ext.define('PVE.tree.ResourceTree', {
 		// getById() use find(), which is slow (ExtJS4 DP5)
 		let item = rstore.data.get(olditem.data.id);
 
-		let changed = false, moved = false;
+		let changed = sorting_changed, moved = sorting_changed;
 		if (item) {
 		    // test if any grouping attributes changed, catches migrated tree-nodes in server view too
 		    for (const attr of moveCheckAttrs) {
-- 
2.30.2