[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 at 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 });
> +	    },
> +	});
> +    },
> +
> +});