From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id BB1811FF0AA for ; Fri, 04 Sep 2026 09:32:46 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 53CCF21538; Fri, 04 Sep 2026 09:32:46 +0200 (CEST) Message-ID: <3d04c155-9cd6-4fe6-9d4b-03d87b8cb588@proxmox.com> Date: Fri, 4 Sep 2026 09:32:41 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH widget-toolkit] ui: disk list: add collapse/expand all controls To: Doug Rathbone , pve-devel@lists.proxmox.com References: <20260830030035.3209873-1-oncheckin@fastmail.com.au> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260830030035.3209873-1-oncheckin@fastmail.com.au> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788507157658 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.510 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: OJCV5GBEQ5Q3R5MMZSB6WWZOAS5YQMKQ X-Message-ID-Hash: OJCV5GBEQ5Q3R5MMZSB6WWZOAS5YQMKQ X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Doug Rathbone X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi, thanks for the patch! On 9/2/26 10:32 AM, Doug Rathbone wrote: > From: Doug Rathbone seems there were multiple from headers in your email? (not sure if this was intentional) > > The disk tree always reloads fully expanded, which gets messy on hosts > with many disks and partitions. > > Add Collapse All and Expand All buttons. Remember the last choice so > reloads and revisits keep the preferred view. > > Forum users have asked for this. A 2021 pve-devel thread discussed > defaulting to collapsed; Thomas preferred collapse/expand controls with > stateful memory instead. > > Tested on PVE 9.2 node with 10+ disks. CLA submitted separately. > the part about the cla does not really belong into the commit message. ( can put such comments directly below the '---' line then it's not part of the commit message) some comments inline > Signed-off-by: Doug Rathbone > --- > src/panel/DiskList.js | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js > index 0762bab..f50c5dd 100644 > --- a/src/panel/DiskList.js > +++ b/src/panel/DiskList.js > @@ -71,6 +71,21 @@ Ext.define('Proxmox.DiskList', { > stateful: true, > stateId: 'tree-node-disks', > > + defaultExpanded: true, > + > + getState: function () { > + let state = this.callParent(arguments) || {}; > + state.defaultExpanded = this.defaultExpanded !== false; > + return state; > + }, nit: convention is that we use 'let me = this;' on methods, especially if we use 'this' multiple times. (same in the function below) not a blocker though and could be fixed up also, you could just write: `state.defaultExpanded = !!this.defaultExpanded;` no real reason to compare it to a literal bool > + > + applyState: function (state) { > + if (state && state.defaultExpanded !== undefined) { > + this.defaultExpanded = state.defaultExpanded; this is now inconsistent with the call above I'd either always use `!!variable` or just a plain assignment > + } > + this.callParent(arguments); > + }, > + also the whole applyState method is not necessary as the default applyState already applies all state variables to 'this' > controller: { > xclass: 'Ext.app.ViewController', > > @@ -134,6 +149,20 @@ Ext.define('Proxmox.DiskList', { > }); > }, > > + collapseAll: function () { > + let view = this.getView(); > + view.collapseAll(); > + view.defaultExpanded = false; > + view.saveState(); > + }, > + > + expandAll: function () { > + let view = this.getView(); > + view.expandAll(); > + view.defaultExpanded = true; > + view.saveState(); > + }, > + > wipeDisk: function () { > let me = this; > let view = me.getView(); > @@ -188,10 +217,11 @@ Ext.define('Proxmox.DiskList', { > } > > let disks = {}; > + let defaultExpanded = view.defaultExpanded !== false; same pattern as above, please use `!!view.defaultExpanded` > > for (const item of records) { > let data = item.data; > - data.expanded = true; > + data.expanded = defaultExpanded; > data.children = data.partitions ?? []; > for (let p of data.children) { > p['disk-type'] = 'partition'; > @@ -388,6 +418,14 @@ Ext.define('Proxmox.DiskList', { > text: gettext('Reload'), > handler: 'reload', > }, > + { > + text: gettext('Collapse All'), > + handler: 'collapseAll', > + }, > + { > + text: gettext('Expand All'), > + handler: 'expandAll', > + }, Two things here: I'm not really a big fan of the button placement here, but I'm not sure if putting them on the right is better or not... Also in general I'd probably prefer a single button that toggles, and in case there are mixed expanded/collapsed it should just do what it last said (e.g. if i collapse all, then expand some manually, a click would expand the rest, and vice versa) this would mean though that wed have to update the buttons on every expand/collapse What do you think? Also it might make sense to introduce icons for these button? (I know the others on these panels don't have them, but that shouldn't hold us back for doing so) > { > xtype: 'proxmoxButton', > text: gettext('Show S.M.A.R.T. values'),