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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 82F3F918A7 for ; Thu, 2 Feb 2023 13:16:38 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 47AD5C0D6 for ; Thu, 2 Feb 2023 13:16:08 +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 13:16:03 +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 CE3F740B19; Thu, 2 Feb 2023 13:16:02 +0100 (CET) Message-ID: Date: Thu, 2 Feb 2023 13:16:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:110.0) Gecko/20100101 Thunderbird/110.0 To: Thomas Lamprecht , Proxmox VE development discussion References: <20230201154917.1632212-1-d.csapak@proxmox.com> <20230201154917.1632212-4-d.csapak@proxmox.com> <9cc78b17-4b9d-7710-2948-446b5d95d784@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <9cc78b17-4b9d-7710-2948-446b5d95d784@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.107 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 12:16:38 -0000 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 >> --- >> 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 }); >> + }, >> + }); >> + }, >> + >> +}); >