From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B115891715 for ; Thu, 2 Feb 2023 11:56:38 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 91335AE45 for ; Thu, 2 Feb 2023 11:56:38 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 2 Feb 2023 11:56:37 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 004DC43B6C for ; Thu, 2 Feb 2023 11:56:37 +0100 (CET) Message-ID: <9cc78b17-4b9d-7710-2948-446b5d95d784@proxmox.com> Date: Thu, 2 Feb 2023 11:56:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:110.0) Gecko/20100101 Thunderbird/110.0 Content-Language: en-GB, de-AT To: Proxmox VE development discussion , Dominik Csapak References: <20230201154917.1632212-1-d.csapak@proxmox.com> <20230201154917.1632212-4-d.csapak@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20230201154917.1632212-4-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.005 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.09 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager 2/3] ui: add TreeSortingEdit window X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Feb 2023 10:56:38 -0000 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 > --- > 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 }); > + }, > + }); > + }, > + > +});