[pve-devel] [PATCH manager 2/3] ui: add TreeSortingEdit window
Dominik Csapak
d.csapak at proxmox.com
Thu Feb 2 13:16:01 CET 2023
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 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...
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 });
>> + },
>> + });
>> + },
>> +
>> +});
>
More information about the pve-devel
mailing list