[pve-devel] [PATCH manager 1/3] fix #4678: ui: don't sort storage backup content by vmid by default

[pve-devel] [PATCH manager 1/3] fix #4678: ui: don't sort storage backup content by vmid by default

[Dominik Csapak]

instead, add the vmid as extra column, so that the user can still sort
by vmid if they wish to

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/storage/BackupView.js | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index fbdf573d3..9ad1a4915 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -220,16 +220,20 @@ Ext.define('PVE.storage.BackupView', {
 		    },
 		},
 	    };
+	} else {
+	    me.extraColumns = {};
 	}
+	me.extraColumns.vmid = {
+	    header: gettext('VMID'),
+	    dataIndex: 'vmid',
+	    hidden: true,
+	    sorter: (a, b) => a.data.vmid - b.data.vmid,
+	};
 
 	me.callParent();
 
 	me.store.getSorters().clear();
 	me.store.setSorters([
-	    {
-		property: 'vmid',
-		direction: 'ASC',
-	    },
 	    {
 		property: 'vdate',
 		direction: 'DESC',
-- 
2.30.2

[pve-devel] [PATCH manager 2/3] ui: make storage backup content stateful

[Dominik Csapak]

so that e.g. a custom sort/column order is saved

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/storage/BackupView.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 9ad1a4915..bb045f5b6 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -5,6 +5,9 @@ Ext.define('PVE.storage.BackupView', {
 
     showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
 
+    stateful: true,
+    stateId: 'storage-backup-content',
+
     initComponent: function() {
 	let me = this;
 
-- 
2.30.2

[pve-devel] [RFC PATCH manager 3/3] ui: enable multiColumnSort for storage backup content

[Dominik Csapak]

this enables the user to sort the grid by multiple columns
simultaneously, e.g. by vmid and then by date

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
sending as rfc because i'm not so sure about this.

on one hand, this allows to recreate the original sorting if users want
that, but the selection is a bit weird. there is no way to 'unsort'
columns again, it simply uses the last 3 columns that were clicked

especially with the last patch (statefulness) it becomes weird, but
maybe we want this more than we want it to be stateful?

 www/manager6/storage/BackupView.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index bb045f5b6..ebc476747 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -5,6 +5,7 @@ Ext.define('PVE.storage.BackupView', {
 
     showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
 
+    multiColumnSort: true,
     stateful: true,
     stateId: 'storage-backup-content',
 
-- 
2.30.2

Re: [pve-devel] [RFC PATCH manager 3/3] ui: enable multiColumnSort for storage backup content

[Aaron Lauterer]

I tried the series including this patch and yeah... sorting becomes very 
unpredicatable and weird. AFAICT it also depends on which order the last 3 
columns are selected.

The old behavior was that it was first sorted by VMID and then within the VMID 
by date right?
Is there a way to group by row property in ExtJS grids?

I personally would probably not use this patch.

On 4/20/23 10:06, Dominik Csapak wrote:
> this enables the user to sort the grid by multiple columns
> simultaneously, e.g. by vmid and then by date
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> sending as rfc because i'm not so sure about this.
> 
> on one hand, this allows to recreate the original sorting if users want
> that, but the selection is a bit weird. there is no way to 'unsort'
> columns again, it simply uses the last 3 columns that were clicked
> 
> especially with the last patch (statefulness) it becomes weird, but
> maybe we want this more than we want it to be stateful?
> 
>   www/manager6/storage/BackupView.js | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
> index bb045f5b6..ebc476747 100644
> --- a/www/manager6/storage/BackupView.js
> +++ b/www/manager6/storage/BackupView.js
> @@ -5,6 +5,7 @@ Ext.define('PVE.storage.BackupView', {
>   
>       showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
>   
> +    multiColumnSort: true,
>       stateful: true,
>       stateId: 'storage-backup-content',
>   

Re: [pve-devel] [RFC PATCH manager 3/3] ui: enable multiColumnSort for storage backup content

[Thomas Lamprecht]

On 20/04/2023 10:06, Dominik Csapak wrote:
> this enables the user to sort the grid by multiple columns
> simultaneously, e.g. by vmid and then by date
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> sending as rfc because i'm not so sure about this.
> 
> on one hand, this allows to recreate the original sorting if users want
> that, but the selection is a bit weird. there is no way to 'unsort'

the original sorting was strange and confusing, while it would be
at least not as confusing with the columns shown it's IMO just not
a good fit for a simple grid.

If we want to make this better, we should adopt a tree view, Fiona
even made a patch for that IIRC, either when she reworked the content
view or when adding group pruning in Proxmox VE (we definitively
talked about doing that off list, the patch might be imagination —
didn't check). Albeit tree views and sorting are naturally not the
greatest thing, an configurable "group by VMID" checkbox could be the
most flexible variant (meaning most edge cases and probabky code too..)

> columns again, it simply uses the last 3 columns that were clicked
> 
> especially with the last patch (statefulness) it becomes weird, but
> maybe we want this more than we want it to be stateful?

tbh. I want neither ;-) At least the chosen full-grid stateful way,
see reply to respective patch.

Re: [pve-devel] [PATCH manager 2/3] ui: make storage backup content stateful

[Thomas Lamprecht]

On 20/04/2023 10:06, Dominik Csapak wrote:
> so that e.g. a custom sort/column order is saved
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/storage/BackupView.js | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
> index 9ad1a4915..bb045f5b6 100644
> --- a/www/manager6/storage/BackupView.js
> +++ b/www/manager6/storage/BackupView.js
> @@ -5,6 +5,9 @@ Ext.define('PVE.storage.BackupView', {
>  
>      showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
>  
> +    stateful: true,
> +    stateId: 'storage-backup-content',

I'm not the biggest fan of stateful grids, if it would only save sorting
it would be OK (can easily be done, but needs to be done manual IIRC), but
by default much more state is saved and reapplied. E.g. the grid column
flex'd widths are then also easily made fixed. Personally I quite often use
the "Reset Layout" just due to that, after having my browser resized and the
columns not adapting in a responsive way – one also cannot clear the state
in a fine grained way, neither by component (at least not easily by changing
to a per, e.g. for here, stateful ID that includes the storage ID) nor by
single thing, like sort order (e.g., a user has no simple way to know what
the original sort order, deemed as best by the devs, was).

> +
>      initComponent: function() {
>  	let me = this;
>  

Re: [pve-devel] [PATCH manager 1/3] fix #4678: ui: don't sort storage backup content by vmid by default

[Thomas Lamprecht]

On 20/04/2023 10:06, Dominik Csapak wrote:
> instead, add the vmid as extra column, so that the user can still sort
> by vmid if they wish to
> 

missing Reported-by tag, would potentially also split this 

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/storage/BackupView.js | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
> index fbdf573d3..9ad1a4915 100644
> --- a/www/manager6/storage/BackupView.js
> +++ b/www/manager6/storage/BackupView.js
> @@ -220,16 +220,20 @@ Ext.define('PVE.storage.BackupView', {
>  		    },
>  		},
>  	    };
> +	} else {
> +	    me.extraColumns = {};

this is a bit odd, would rather set it upfront and change the the isPBS
branch to set each column explicitly, but has nothing to do with your
patch directly and if, the cleanup should happen in a separate one in
any way.

>  	}
> +	me.extraColumns.vmid = {
> +	    header: gettext('VMID'),
> +	    dataIndex: 'vmid',
> +	    hidden: true,
> +	    sorter: (a, b) => a.data.vmid - b.data.vmid,

doesn't this breaks, or at least behaves possibly confusingly, on
custom file names? At least we don't handle the 'vmid' field explicitly
in the 'pve-storage-content' model.

Note that our regex here is very liberal, it basically returns any file
that ends with tar or vma and an optional gz, lzo or zst compressor.

> +	};
>  
>  	me.callParent();
>  
>  	me.store.getSorters().clear();
>  	me.store.setSorters([
> -	    {
> -		property: 'vmid',
> -		direction: 'ASC',
> -	    },
>  	    {
>  		property: 'vdate',
>  		direction: 'DESC',

Re: [pve-devel] [PATCH manager 1/3] fix #4678: ui: don't sort storage backup content by vmid by default

[Thomas Lamprecht]

On 22/04/2023 09:38, Thomas Lamprecht wrote:
> , would potentially also split this

ignore that part, send to early and patch is actually fine in
that regard.