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

[Dominik Csapak <d.csapak@proxmox.com>]

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