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

[Doug Rathbone <dougrathbone@gmail.com>]

Hi,

Thanks for the review.

The dual From headers were unintentional. Git author is
dougrathbone@gmail.com, but SMTP went out via Fastmail. I'll fix that for
v2.

Agree on the commit message and the state handling nits. I'll drop the CLA
note from the commit, remove the custom applyState, and switch to
!!defaultExpanded / let me = this.

On the buttons: a single toggle sounds better. I'll make it update on
expand/collapse and keep the last action for mixed states. Happy to add
icons too (e.g. fa-compress / fa-expand). I'll leave it next to Reload for
now unless you'd rather it on the right.

See v2 attached.

Thanks,
Doug

On Fri, Sep 4, 2026 at 5:32 PM Dominik Csapak <d.csapak@proxmox.com> wrote:

> 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'),
>
>
>