[pbs-devel] [PATCH proxmox-backup] fix #5422: ui: make columns in gcview sortable

[pbs-devel] [PATCH proxmox-backup] fix #5422: ui: make columns in gcview sortable

[Gabriel Goller]

These columns were not sortable for some reason. Tested it as well and
the sorting seems to work fine.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/config/GCView.js | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/www/config/GCView.js b/www/config/GCView.js
index bcea72a5..1c9288e4 100644
--- a/www/config/GCView.js
+++ b/www/config/GCView.js
@@ -164,7 +164,7 @@ Ext.define('PBS.config.GCJobView', {
 	{
 	    header: gettext('Schedule'),
 	    dataIndex: 'schedule',
-	    sortable: false,
+	    sortable: true,
 	    hideable: false,
 	    renderer: (value) => value ? value : Proxmox.Utils.NoneText,
 	    minWidth: 85,
@@ -182,7 +182,7 @@ Ext.define('PBS.config.GCJobView', {
 	    text: gettext('Duration'),
 	    dataIndex: 'duration',
 	    renderer: Proxmox.Utils.render_duration,
-	    sortable: false,
+	    sortable: true,
 	    minWidth: 80,
 	    flex: 1,
 	},
@@ -207,7 +207,7 @@ Ext.define('PBS.config.GCJobView', {
 	    dataIndex: 'removed-bytes',
 	    renderer: (value, meta, record) => record.data.upid !== null
 		? Proxmox.Utils.format_size(value, true) : "-",
-	    sortable: false,
+	    sortable: true,
 	    minWidth: 85,
 	    flex: 1,
 	},
@@ -216,7 +216,7 @@ Ext.define('PBS.config.GCJobView', {
 	    dataIndex: 'pending-bytes',
 	    renderer: (value, meta, record) => record.data.upid !== null
 		? Proxmox.Utils.format_size(value, true) : "-",
-	    sortable: false,
+	    sortable: true,
 	    minWidth: 80,
 	    flex: 3,
 	},
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] fix #5422: ui: make columns in gcview sortable

[Max Carrara]

On Tue May 7, 2024 at 10:13 AM CEST, Gabriel Goller wrote:
> These columns were not sortable for some reason. Tested it as well and
> the sorting seems to work fine.
>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>

LGTM

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  www/config/GCView.js | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/www/config/GCView.js b/www/config/GCView.js
> index bcea72a5..1c9288e4 100644
> --- a/www/config/GCView.js
> +++ b/www/config/GCView.js
> @@ -164,7 +164,7 @@ Ext.define('PBS.config.GCJobView', {
>  	{
>  	    header: gettext('Schedule'),
>  	    dataIndex: 'schedule',
> -	    sortable: false,
> +	    sortable: true,
>  	    hideable: false,
>  	    renderer: (value) => value ? value : Proxmox.Utils.NoneText,
>  	    minWidth: 85,
> @@ -182,7 +182,7 @@ Ext.define('PBS.config.GCJobView', {
>  	    text: gettext('Duration'),
>  	    dataIndex: 'duration',
>  	    renderer: Proxmox.Utils.render_duration,
> -	    sortable: false,
> +	    sortable: true,
>  	    minWidth: 80,
>  	    flex: 1,
>  	},
> @@ -207,7 +207,7 @@ Ext.define('PBS.config.GCJobView', {
>  	    dataIndex: 'removed-bytes',
>  	    renderer: (value, meta, record) => record.data.upid !== null
>  		? Proxmox.Utils.format_size(value, true) : "-",
> -	    sortable: false,
> +	    sortable: true,
>  	    minWidth: 85,
>  	    flex: 1,
>  	},
> @@ -216,7 +216,7 @@ Ext.define('PBS.config.GCJobView', {
>  	    dataIndex: 'pending-bytes',
>  	    renderer: (value, meta, record) => record.data.upid !== null
>  		? Proxmox.Utils.format_size(value, true) : "-",
> -	    sortable: false,
> +	    sortable: true,
>  	    minWidth: 80,
>  	    flex: 3,
>  	},



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] fix #5422: ui: make columns in gcview sortable

[Thomas Lamprecht]

Would change subject to:

fix #5422: ui: garbage collection: make columns sortable

Using the class name "gcview" is rather implementation detail and not
really telling to devs when skimming git log entries or users, if this
gets copied over into the debian/changelog.

Am 07/05/2024 um 10:13 schrieb Gabriel Goller:
> These columns were not sortable for some reason. Tested it as well and

The original reason was that PBS did not have a PBS-wide GC/prune view
until the recently released 3.2, and so each GC view only had a single
row, as GC jobs are always affecting the whole datastore anyway.
Thus sorting did not make sense to have for single line entries, showing
the sort-arrows would only add a tiny bit of confusion for users, as that
would imply that more entries could be present.

Now for the PBS-wide view sorting makes sense, but for the per-datastore
GC view it still doesn't.

You could drop the `sortable` config from the columns entries and
set that dynamically in initComponent, we loop over all columns there
already and have the information if the view is instantiated for a single
datastore or for all.

E.g., I'd do something like (untested):

diff --git a/www/config/GCView.js b/www/config/GCView.js
index 1c9288e4..c3b87fe5 100644
--- a/www/config/GCView.js
+++ b/www/config/GCView.js
@@ -224,12 +224,12 @@ Ext.define('PBS.config.GCJobView', {
 
     initComponent: function() {
        let me = this;
-       let hideLocalDatastore = !!me.datastore;
+       let isSingleDatastore = !!me.datastore;
 
        for (let column of me.columns) {
+           column.sortable = !isSingleDatastore;
            if (column.dataIndex === 'store') {
-               column.hidden = hideLocalDatastore;
-               break;
+               column.hidden = isSingleDatastore;
            }
        }
 

> the sorting seems to work fine.
> 

Could do with a fixes trailer like:

Fixes: db3fd213 ("fix #3217: ui: global prune and gc job view")

> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  www/config/GCView.js | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/www/config/GCView.js b/www/config/GCView.js
> index bcea72a5..1c9288e4 100644
> --- a/www/config/GCView.js
> +++ b/www/config/GCView.js
> @@ -164,7 +164,7 @@ Ext.define('PBS.config.GCJobView', {
>  	{
>  	    header: gettext('Schedule'),
>  	    dataIndex: 'schedule',
> -	    sortable: false,
> +	    sortable: true,
>  	    hideable: false,
>  	    renderer: (value) => value ? value : Proxmox.Utils.NoneText,
>  	    minWidth: 85,
> @@ -182,7 +182,7 @@ Ext.define('PBS.config.GCJobView', {
>  	    text: gettext('Duration'),
>  	    dataIndex: 'duration',
>  	    renderer: Proxmox.Utils.render_duration,
> -	    sortable: false,
> +	    sortable: true,
>  	    minWidth: 80,
>  	    flex: 1,
>  	},
> @@ -207,7 +207,7 @@ Ext.define('PBS.config.GCJobView', {
>  	    dataIndex: 'removed-bytes',
>  	    renderer: (value, meta, record) => record.data.upid !== null
>  		? Proxmox.Utils.format_size(value, true) : "-",
> -	    sortable: false,
> +	    sortable: true,
>  	    minWidth: 85,
>  	    flex: 1,
>  	},
> @@ -216,7 +216,7 @@ Ext.define('PBS.config.GCJobView', {
>  	    dataIndex: 'pending-bytes',
>  	    renderer: (value, meta, record) => record.data.upid !== null
>  		? Proxmox.Utils.format_size(value, true) : "-",
> -	    sortable: false,
> +	    sortable: true,
>  	    minWidth: 80,
>  	    flex: 3,
>  	},



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] fix #5422: ui: make columns in gcview sortable

[Gabriel Goller]

On 15.05.2024 14:34, Thomas Lamprecht wrote:
>Would change subject to:
>
>fix #5422: ui: garbage collection: make columns sortable
>
>Using the class name "gcview" is rather implementation detail and not
>really telling to devs when skimming git log entries or users, if this
>gets copied over into the debian/changelog.

I agree.

>Am 07/05/2024 um 10:13 schrieb Gabriel Goller:
>> These columns were not sortable for some reason. Tested it as well and
>
>The original reason was that PBS did not have a PBS-wide GC/prune view
>until the recently released 3.2, and so each GC view only had a single
>row, as GC jobs are always affecting the whole datastore anyway.
>Thus sorting did not make sense to have for single line entries, showing
>the sort-arrows would only add a tiny bit of confusion for users, as that
>would imply that more entries could be present.

pre-3.2 pbs only used a 1x1 table (so not really a table :) ) for gc jobs, 
so no sorting possible there. I wondered why the patch from Stefan 
Lendl only included the sortable config on some and not all columns :)

>Now for the PBS-wide view sorting makes sense, but for the per-datastore
>GC view it still doesn't.
>
>You could drop the `sortable` config from the columns entries and
>set that dynamically in initComponent, we loop over all columns there
>already and have the information if the view is instantiated for a single
>datastore or for all.
>
>E.g., I'd do something like (untested):
>
>diff --git a/www/config/GCView.js b/www/config/GCView.js
>index 1c9288e4..c3b87fe5 100644
>--- a/www/config/GCView.js
>+++ b/www/config/GCView.js
>@@ -224,12 +224,12 @@ Ext.define('PBS.config.GCJobView', {
>
>     initComponent: function() {
>        let me = this;
>-       let hideLocalDatastore = !!me.datastore;
>+       let isSingleDatastore = !!me.datastore;
>
>        for (let column of me.columns) {
>+           column.sortable = !isSingleDatastore;
>            if (column.dataIndex === 'store') {
>-               column.hidden = hideLocalDatastore;
>-               break;
>+               column.hidden = isSingleDatastore;
>            }
>        }
>
>

Wow, this works like a charm!

>> the sorting seems to work fine.
>>
>
>Could do with a fixes trailer like:
>
>Fixes: db3fd213 ("fix #3217: ui: global prune and gc job view")
>

Will do!

And thanks for the review!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel