Re: [PATCH widget-toolkit] ui: disk list: add collapse/expand all controls

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

Hi, thanks for the patch!

On 9/2/26 10:32 AM, Doug Rathbone wrote:
> From: Doug Rathbone <dougrathbone@gmail.com>

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 <dougrathbone@gmail.com>
> ---
>   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'),