Re: [PATCH pve-manager] fix: #4490 ui: backup job window: add filter/search for virtual guest grid

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 19.03.26 um 13:50 schrieb David Riley:
> Add a collapsible form for filtering by Name, Status, Pool, Type, 
> Tags, and HA State.
> 
> Add corresponding columns (pool, type, tags, hastate) to the guest 
> grid so the visual output matches the available filters.
> 
> Expand/collapse the filter UI depending on the selected Backup Mode 
> (All, Pool, Include/Exclude).

I can spot some smaller code but more importantly UX issues, the biggest
one would be that this could be confused for an implementation of
very much looks like an implementation of #4186 "more flexible virtual
guest selection mechanism for backup jobs, possibly including tags and
multiple pools"

I can e.g. still edit the filters if I change the base selection from
selected-guests to all-guests, then e.g. select a some tag as filter
and save the job thinking guests matching those tag will be backed up
by this job, but rather all guests will be backed up, which e.g., can
lead to full backup storage and/or long running jobs causing IO pressure.

And even with selected-guests chosen as basic selection mode, users might
still expect that the filter is applied on job run time, not just once on
job edit or create.

The filter also makes the window rather tall, still fine for nowadays
rather common display sizes, but the low-res bare minimum we try to
support such that it's at least possible to be able to use all UI
controls, doesn't have to look good, might be 

>From some years ago a user mentioned using 768p, but with OS task bar
and the like only roughly 1300x680 is left for the PVE ui. Now, that
has been some years and here it's also a bit borderline as the filters
can be collapsed, making the window short enough to use it at that size.

The grid is now pretty crammed and I cannot see much of the actual
guest (host) names nor much else of the available columns. We can get us
some space here by e.g. changing the renderer for type to just use the
short VM and CT abbreviations (it's used in other places in the UI,
and here the surrounding context makes it telling enough). The status
column could be icon-only with the descriptive status text being shown
as tooltip. The name column definitively needs to be wider. HA state
might become a "HA configured or not" check mark, potentially with a
third state for "ignored" special state (e.g., rendering to fa-check,
fa-minus, fa-info icons).

But while that can be improved, the main point is IMO the confusion
potential for this being real filter, and IMO it'd be much better if
we implement that when adding such complex (nice!) filters to the backup
window, as that solves both cases.

As stop-gap you could implement a very simple search text-field, e.g.,
something similar to what the Backup "Job Detail" window got. That's
will already providee most of the convenience needed here, should be
much simpler and avoids confusion potential for this being "live" filter
saved to the job and is less intrusive UI wise.
For the filters we could then see if making guest selection a dedicated
tab might be worth it.

> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  www/manager6/dc/Backup.js | 354 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 350 insertions(+), 4 deletions(-)

> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 956a7cdf..28d63bbb 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js

[...]

> +        prepareFilters: function () {
> +            let me = this;
> +
> +            me.filterState = {};
> +            me.activeFilter = new Ext.util.Filter({
> +                id: 'filter',
> +                scope: me,
> +                filterFn: me.filterFn,
> +            });
> +
> +            let statusMap = {};
> +            let poolMap = {};
> +            let haMap = {};
> +            let tagMap = {};
> +
> +            PVE.data.ResourceStore.each((rec) => {
> +                if (['qemu', 'lxc'].indexOf(rec.data.type) !== -1) {

the .includes [0] for array is available in ES 2024 baseline (and much earlier
already), so this can become:

if (['qemu', 'lxc'].includes(rec.data.type))

[0]: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

> +                    statusMap[rec.data.status] = true;
> +                }
> +                if (rec.data.type === 'pool') {
> +                    poolMap[rec.data.pool] = true;
> +                }
> +                if (rec.data.hastate !== '') {

This also evaluates to true on null/undef though? Maybe go for

rec.data.hastate?.length

> +                    haMap[rec.data.hastate] = true;
> +                }
> +                if (rec.data.tags !== '') {
> +                    rec.data.tags.split(/[,; ]/).forEach((tag) => {
> +                        if (tag !== '') {
> +                            tagMap[tag] = true;
> +                        }
> +                    });
> +                }
> +            });
> +
> +            let statusList = Object.keys(statusMap).map((key) => [key, key]);
> +            statusList.unshift(['', gettext('All')]);
> +            let poolList = Object.keys(poolMap).map((key) => [key, key]);
> +            let haList = Object.keys(haMap).map((key) => [key, key]);
> +            let tagList = Object.keys(tagMap).map((key) => ({ value: key }));
> +
> +            me.lookup('includetagfilter').getStore().setData(tagList);
> +            me.lookup('excludetagfilter').getStore().setData(tagList);
> +            me.lookup('statusfilter').getStore().setData(statusList);
> +            me.lookup('hastatefilter').getStore().setData(haList);
> +            me.lookup('poolfilter').getStore().setData(poolList);
> +        },
> +
>          init: function (view) {
>              let me = this;
> +            me.prepareFilters();
>  
>              if (view.isCreate) {
>                  me.lookup('modeSelector').setValue('include');
> @@ -330,6 +502,171 @@ Ext.define('PVE.dc.BackupEdit', {
>                                  'data-qtip': gettext('Description of the job'),
>                              },
>                          },
> +                        {
> +                            xtype: 'fieldset',
> +                            reference: 'filters',
> +                            collapsible: true,
> +                            title: gettext('Filters'),
> +                            layout: 'hbox',
> +                            margin: '0 2 10 0',
> +                            items: [
> +                                {
> +                                    xtype: 'container',
> +                                    flex: 1,
> +                                    padding: 5,
> +                                    layout: {
> +                                        type: 'vbox',
> +                                        align: 'stretch',
> +                                    },
> +                                    defaults: {
> +                                        listeners: {
> +                                            change: 'filterChange',
> +                                        },
> +                                        isFormField: false,
> +                                    },
> +                                    items: [
> +                                        {
> +                                            fieldLabel: gettext('Name'),
> +                                            reference: 'namefilter',
> +                                            xtype: 'textfield',

style nit: please order the xtype to the top of such configs.

btw. as filters like these would be used in a few parts (e.g., bulk actions, which
probably was your template I'd guess) we probably should factor out some common
stuff. But might not be relevant if we go for the simple stop-gap for now and
actual flexible selection that #smart tracks.