[pbs-devel] [PATCH proxmox-backup v1] fixes #4001: file explorer/download UX improvements

[pbs-devel] [PATCH proxmox-backup v1] fixes #4001: file explorer/download UX improvements

[Stefan Sterz]

this series provides several ux improvements for the file browser. the
tar and zip download buttons will be merged into a menu button when
both are available, the currently selected file is outlined in the
bottom bar, the amount of items contained in a folder will be
displayed as their size and the backup group will be prepended to
downloaded files.

Stefan Sterz (2):
  fix #4001: datastore/catalog: add number of files to directory entry
  fix #4001: ui: add prefix to files downloaded through the pxar browser

 pbs-datastore/src/catalog.rs | 10 ++++++----
 www/datastore/Content.js     |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

Stefan Sterz (3):
  fix #4001: FileBrowser: add menu to button and selected entry label
  fix #4001: FileBrowser: show number of items in a directory as size
  fix #4001: FileBrowser: add a configurable prefix to downloaded files

 src/window/FileBrowser.js | 91 ++++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 21 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH widget-toolkit v1 1/3] fix #4001: FileBrowser: add menu to button and selected entry label

[Stefan Sterz]

this commit adds a label showing the currently selected entry in the
file browser and merges the "Download .tar.zst" and "Download .zip"
button into one menu button.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
note: i am not really a fan of decoding the filepath via `atob()` 
here, but that is how the data is sent from PBS. if you have
suggestions on how to improve this, feedback is appreciated.

 src/window/FileBrowser.js | 59 +++++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index f4a22b6..bb262bc 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -76,7 +76,9 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    'd': true, // directories
 	},
 
-	// set to true to show the tar download button
+	// enable tar download, this will add a menu to the
+	// "Download" button when the selection can be downloaded as
+	// .tar files
 	enableTar: false,
     },
 
@@ -135,13 +137,19 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    if (!selection || selection.length < 1) return;
 
 	    let data = selection[0].data;
+	    let st = Ext.String.format(gettext('Selected "{0}"'), atob(data.filepath));
+	    view.lookup('selectText').setText(st);
+
 	    let canDownload = view.downloadURL && view.downloadableFileTypes[data.type];
-	    let zipBtn = me.lookup('downloadBtn');
-	    let tarBtn = me.lookup('downloadTar');
-	    zipBtn.setDisabled(!canDownload);
-	    tarBtn.setDisabled(!canDownload);
-	    zipBtn.setText(data.type === 'd' ? gettext('Download .zip') : gettext('Download'));
-	    tarBtn.setVisible(data.type === 'd' && view.enableTar);
+	    let enableMenu = view.enableTar && data.type === 'd';
+
+	    let downloadBtn = view.lookup('downloadBtn');
+	    downloadBtn.setDisabled(!canDownload || enableMenu);
+	    downloadBtn.setHidden(!canDownload || enableMenu);
+
+	    let menuBtn = view.lookup('menuBtn');
+	    menuBtn.setDisabled(!canDownload || !enableMenu);
+	    menuBtn.setHidden(!canDownload || !enableMenu);
 	},
 
 	errorHandler: function(error, msg) {
@@ -150,7 +158,7 @@ Ext.define("Proxmox.window.FileBrowser", {
 		return false;
 	    }
 	    me.lookup('downloadBtn').setDisabled(true);
-	    me.lookup('downloadTar').setDisabled(true);
+	    me.lookup('menuBtn').setDisabled(true);
 	    if (me.initialLoadDone) {
 		Ext.Msg.alert(gettext('Error'), msg);
 		return true;
@@ -300,19 +308,40 @@ Ext.define("Proxmox.window.FileBrowser", {
 	},
     ],
 
-    buttons: [
+    fbar: [
 	{
-	    text: gettext('Download .tar.zst'),
-	    handler: 'downloadTar',
-	    reference: 'downloadTar',
-	    hidden: true,
-	    disabled: true,
+	    text: '',
+	    xtype: 'label',
+	    reference: 'selectText',
 	},
 	{
-	    text: gettext('Download .zip'),
+	    text: gettext('Download'),
+	    xtype: 'button',
 	    handler: 'downloadZip',
 	    reference: 'downloadBtn',
 	    disabled: true,
+	    hidden: true,
+	},
+	{
+	    text: gettext('Download as'),
+	    xtype: 'button',
+	    reference: 'menuBtn',
+	    menu: {
+		items: [
+		    {
+			iconCls: 'fa fa-fw fa-file-zip-o',
+			text: gettext('.zip'),
+			handler: 'downloadZip',
+			reference: 'downloadZip',
+		    },
+		    {
+			iconCls: 'fa fa-fw fa-archive',
+			text: gettext('.tar.zst'),
+			handler: 'downloadTar',
+			reference: 'downloadTar',
+		    },
+		],
+	    },
 	},
     ],
 });
-- 
2.30.2

[pbs-devel] [PATCH widget-toolkit v1 2/3] fix #4001: FileBrowser: show number of items in a directory as size

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/window/FileBrowser.js | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index bb262bc..54967c2 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -2,6 +2,19 @@ Ext.define('proxmox-file-tree', {
     extend: 'Ext.data.Model',
 
     fields: ['filepath', 'text', 'type', 'size',
+	{
+	    name: 'sizedisplay',
+	    calculate: data => {
+		if (data.size === undefined) {
+		    return '';
+		} else if (data.type === 'd') {
+		    let fs = data.size === 1 ? gettext('{0} item') : gettext('{0} items');
+		    return Ext.String.format(fs, data.size);
+		}
+
+		return Proxmox.Utils.format_size(data.size);
+	    },
+	},
 	{
 	    name: 'mtime',
 	    type: 'date',
@@ -270,10 +283,15 @@ Ext.define("Proxmox.window.FileBrowser", {
 		},
 		{
 		    text: gettext('Size'),
-		    dataIndex: 'size',
-		    renderer: value => value === undefined ? '' : Proxmox.Utils.format_size(value),
+		    dataIndex: 'sizedisplay',
 		    sorter: {
 			sorterFn: function(a, b) {
+			    if (a.data.type === 'd' && b.data.type !== 'd') {
+				return -1;
+			    } else if (a.data.type !== 'd' && b.data.type === 'd') {
+				return 1;
+			    }
+
 			    let asize = a.data.size || 0;
 			    let bsize = b.data.size || 0;
 
-- 
2.30.2

[pbs-devel] [PATCH widget-toolkit v1 3/3] fix #4001: FileBrowser: add a configurable prefix to downloaded files

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/window/FileBrowser.js | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index 54967c2..9ad2e81 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -93,6 +93,9 @@ Ext.define("Proxmox.window.FileBrowser", {
 	// "Download" button when the selection can be downloaded as
 	// .tar files
 	enableTar: false,
+
+	// prefix to prepend to downloaded file names
+	downloadPrefix: '',
     },
 
     controller: {
@@ -124,12 +127,11 @@ Ext.define("Proxmox.window.FileBrowser", {
 
 	    let data = selection[0].data;
 
-	    let atag = document.createElement('a');
-
-	    atag.download = data.text;
 	    let params = { ...view.extraParams };
 	    params.filepath = data.filepath;
-	    atag.download = data.text;
+
+	    let atag = document.createElement('a');
+	    atag.download = view.downloadPrefix + data.text;
 	    if (data.type === 'd') {
 		if (tar) {
 		    params.tar = 1;
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v1 1/2] fix #4001: datastore/catalog: add number of files to directory entry

[Stefan Sterz]

when listing the content of a catalog, add the number of files
contained in the directory as its size. also removes redundant code,
the size of a file is already set when creating the archive entry.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
this requires patch two from the widget toolkit part of this patch to
be applied already. otherwise the formatting and sorting of the folder
"size" will be wrong.

 pbs-datastore/src/catalog.rs | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/catalog.rs b/pbs-datastore/src/catalog.rs
index c07b71a6..6cb8aeb4 100644
--- a/pbs-datastore/src/catalog.rs
+++ b/pbs-datastore/src/catalog.rs
@@ -706,10 +706,11 @@ impl<R: Read + Seek> CatalogReader<R> {
             components.push(b'/');
             components.extend(&direntry.name);
             let mut entry = ArchiveEntry::new(&components, Some(&direntry.attr));
-            if let DirEntryAttribute::File { size, mtime } = direntry.attr {
-                entry.size = size.into();
-                entry.mtime = mtime.into();
+
+            if let DirEntryAttribute::Directory { start: _ } = direntry.attr {
+                entry.size = Some(u64::try_from(self.read_dir(&direntry)?.len())?);
             }
+
             res.push(entry);
         }
 
@@ -911,7 +912,8 @@ pub struct ArchiveEntry {
     pub entry_type: String,
     /// Is this entry a leaf node, or does it have children (i.e. a directory)?
     pub leaf: bool,
-    /// The file size, if entry_type is 'f' (file)
+    /// The file size, if entry_type is 'f' (file) or the amount of files in a
+    /// directory if entry_type is 'd' (directory)
     #[serde(skip_serializing_if = "Option::is_none")]
     pub size: Option<u64>,
     /// The file "last modified" time stamp, if entry_type is 'f' (file)
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v1 1/2] fix #4001: datastore/catalog: add number of files to directory entry

[Thomas Lamprecht]

Am 5/5/22 um 15:52 schrieb Stefan Sterz:
> when listing the content of a catalog, add the number of files
> contained in the directory as its size. also removes redundant code,
> the size of a file is already set when creating the archive entry.
> 

did you mean to write "the mtime of a file ..." as, we obviously need to
set the size differently to get the number of children, but you dropped
setting the mtime, which is OK as ArchiveEntry::new does that already,
or did I overlooked something?

> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> this requires patch two from the widget toolkit part of this patch to
> be applied already. otherwise the formatting and sorting of the folder
> "size" will be wrong.

ok so no hard depends.

> 
>  pbs-datastore/src/catalog.rs | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-datastore/src/catalog.rs b/pbs-datastore/src/catalog.rs
> index c07b71a6..6cb8aeb4 100644
> --- a/pbs-datastore/src/catalog.rs
> +++ b/pbs-datastore/src/catalog.rs
> @@ -706,10 +706,11 @@ impl<R: Read + Seek> CatalogReader<R> {
>              components.push(b'/');
>              components.extend(&direntry.name);
>              let mut entry = ArchiveEntry::new(&components, Some(&direntry.attr));
> -            if let DirEntryAttribute::File { size, mtime } = direntry.attr {
> -                entry.size = size.into();
> -                entry.mtime = mtime.into();
> +
> +            if let DirEntryAttribute::Directory { start: _ } = direntry.attr {
> +                entry.size = Some(u64::try_from(self.read_dir(&direntry)?.len())?);
>              }
> +
>              res.push(entry);
>          }
>  
> @@ -911,7 +912,8 @@ pub struct ArchiveEntry {
>      pub entry_type: String,
>      /// Is this entry a leaf node, or does it have children (i.e. a directory)?
>      pub leaf: bool,
> -    /// The file size, if entry_type is 'f' (file)
> +    /// The file size, if entry_type is 'f' (file) or the amount of files in a
> +    /// directory if entry_type is 'd' (directory)
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub size: Option<u64>,
>      /// The file "last modified" time stamp, if entry_type is 'f' (file)

Re: [pbs-devel] [PATCH proxmox-backup v1 1/2] fix #4001: datastore/catalog: add number of files to directory entry

[Stefan Sterz]

On 5/12/22 17:27, Thomas Lamprecht wrote:
> Am 5/5/22 um 15:52 schrieb Stefan Sterz:
>> when listing the content of a catalog, add the number of files
>> contained in the directory as its size. also removes redundant code,
>> the size of a file is already set when creating the archive entry.
>>
> 
> did you mean to write "the mtime of a file ..." as, we obviously need to
> set the size differently to get the number of children, but you dropped
> setting the mtime, which is OK as ArchiveEntry::new does that already,
> or did I overlooked something?
> 

sorry for the confusingly worded comment. the following is a bit more
verbose than it might need to be in order to be a bit clearer ^^' :

the original code would assign the `size` _and_ `mtime` again from the
`DirEntryAttribute` if it was of type `DirEntryAttribute::File`.
however, that is redundant as _both_ are already set by
`ArchiveEntry::new()` and `ArchiveEntry::new_with_size()` from the
same `DirEntryAttribute`. as you pointed out.

therefore, i removed those conditional assignments and added another
one for `DirEntryAttribute::Directory` because here
`ArchiveEntry::new()` wouldn't set any size.

hopefully this makes more sense.

>> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
>> ---
>> this requires patch two from the widget toolkit part of this patch to
>> be applied already. otherwise the formatting and sorting of the folder
>> "size" will be wrong.
> 
> ok so no hard depends.
> 

no, it will build just fine and no errors will be logged in the
browser console either, but the amount of items will be displayed as
the size in bytes and sorting will work accordingly, which would be
confusing in most cases id assume.

>>
>>  pbs-datastore/src/catalog.rs | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/pbs-datastore/src/catalog.rs b/pbs-datastore/src/catalog.rs
>> index c07b71a6..6cb8aeb4 100644
>> --- a/pbs-datastore/src/catalog.rs
>> +++ b/pbs-datastore/src/catalog.rs
>> @@ -706,10 +706,11 @@ impl<R: Read + Seek> CatalogReader<R> {
>>              components.push(b'/');
>>              components.extend(&direntry.name);
>>              let mut entry = ArchiveEntry::new(&components, Some(&direntry.attr));
>> -            if let DirEntryAttribute::File { size, mtime } = direntry.attr {
>> -                entry.size = size.into();
>> -                entry.mtime = mtime.into();
>> +
>> +            if let DirEntryAttribute::Directory { start: _ } = direntry.attr {
>> +                entry.size = Some(u64::try_from(self.read_dir(&direntry)?.len())?);
>>              }
>> +
>>              res.push(entry);
>>          }
>>  
>> @@ -911,7 +912,8 @@ pub struct ArchiveEntry {
>>      pub entry_type: String,
>>      /// Is this entry a leaf node, or does it have children (i.e. a directory)?
>>      pub leaf: bool,
>> -    /// The file size, if entry_type is 'f' (file)
>> +    /// The file size, if entry_type is 'f' (file) or the amount of files in a
>> +    /// directory if entry_type is 'd' (directory)
>>      #[serde(skip_serializing_if = "Option::is_none")]
>>      pub size: Option<u64>,
>>      /// The file "last modified" time stamp, if entry_type is 'f' (file)
> 

[pbs-devel] [PATCH proxmox-backup v1 2/2] fix #4001: ui: add prefix to files downloaded through the pxar browser

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
this requires patch three from the widget toolkit portion of this
series. otherwise this won't have any effect.

 www/datastore/Content.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index 1be63e0c..0113b704 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -619,6 +619,7 @@ Ext.define('PBS.DataStoreContent', {
 		listURL: `/api2/json/admin/datastore/${view.datastore}/catalog`,
 		downloadURL: `/api2/json/admin/datastore/${view.datastore}/pxar-file-download`,
 		enableTar: true,
+		downloadPrefix: `${type}-${id}-`,
 		extraParams: {
 		    'backup-id': id,
 		    'backup-time': (time.getTime()/1000).toFixed(0),
-- 
2.30.2

[pbs-devel] applied-series: [PATCH proxmox-backup v1] fixes #4001: file explorer/download UX improvements

[Thomas Lamprecht]

Am 5/5/22 um 15:52 schrieb Stefan Sterz:
> this series provides several ux improvements for the file browser. the
> tar and zip download buttons will be merged into a menu button when
> both are available, the currently selected file is outlined in the
> bottom bar, the amount of items contained in a folder will be
> displayed as their size and the backup group will be prepended to
> downloaded files.
> 
> Stefan Sterz (2):
>   fix #4001: datastore/catalog: add number of files to directory entry
>   fix #4001: ui: add prefix to files downloaded through the pxar browser
> 
>  pbs-datastore/src/catalog.rs | 10 ++++++----
>  www/datastore/Content.js     |  1 +
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> Stefan Sterz (3):
>   fix #4001: FileBrowser: add menu to button and selected entry label
>   fix #4001: FileBrowser: show number of items in a directory as size
>   fix #4001: FileBrowser: add a configurable prefix to downloaded files
> 
>  src/window/FileBrowser.js | 91 ++++++++++++++++++++++++++++++---------
>  1 file changed, 70 insertions(+), 21 deletions(-)
> 



applied series, thanks!