[pbs-devel] [PATCH proxmox-backup v2 0/5] ui: unify and improve tape restore window

[pbs-devel] [PATCH proxmox-backup v2 0/5] ui: unify and improve tape restore window

[Dominik Csapak]

changes from v1:
* split patches in proper self-containted commits
* changed layout in restore window
   datastore mapping grid is now on the right, other options on the left
* integrated the selection checkbox into the checkbox selection model of
  the grid (removing a seperate field)
* integrated the filter textbox into the gridfilters plugin, to save space
  (like we do in pves bulk action window)
  the benefit of this is that we do not have to combine/split the
  store:snapshots anymore besides when assembling the submitdata,
  this makes the code a little nicer imho

Dominik Csapak (5):
  ui: tape/BackupOverview: fix wrong media-set text for singlerestore
  ui: tape/BackupOverview: move restore buttons inline
  ui: tape/window/TapeRestore: add SnapshotGrid Component
  ui: tape/window/TapeRestore: enabling selecting multiple snapshots
  ui: tape/BackupOverview: also allow to filter by group for restore

 www/tape/BackupOverview.js     |  99 ++++++----------
 www/tape/window/TapeRestore.js | 209 +++++++++++++++++++++++++--------
 2 files changed, 196 insertions(+), 112 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 1/5] ui: tape/BackupOverview: fix wrong media-set text for singlerestore

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index c028d58d..91ff1aac 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -29,7 +29,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 		return;
 	    }
 	    let restoreid = node.data.restoreid;
-	    let mediaset = node.data.text;
+	    let mediaset = node.data['media-set'];
 	    let uuid = node.data['media-set-uuid'];
 	    let datastores = [node.data.store];
 
@@ -155,14 +155,15 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    let view = me.getView();
 
 	    Proxmox.Utils.setErrorMask(view, true);
-	    const media_set = node.data['media-set-uuid'];
+	    const media_set_uuid = node.data['media-set-uuid'];
+	    const media_set = node.data.text;
 
 	    try {
 		let list = await PBS.Async.api2({
 		    method: 'GET',
 		    url: `/api2/extjs/tape/media/content`,
 		    params: {
-			'media-set': media_set,
+			'media-set': media_set_uuid,
 		    },
 		});
 
@@ -181,6 +182,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 		    entry.text = entry.snapshot;
 		    entry.leaf = true;
 		    entry.children = [];
+		    entry['media-set'] = media_set;
 		    entry.restoreid = `${entry.store}:${entry.snapshot}`;
 		    let iconCls = PBS.Utils.get_type_icon_cls(entry.snapshot);
 		    if (iconCls !== '') {
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup v2 1/5] ui: tape/BackupOverview: fix wrong media-set text for singlerestore

[Thomas Lamprecht]

On 14.05.21 14:59, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/tape/BackupOverview.js | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pbs-devel] [PATCH proxmox-backup v2 2/5] ui: tape/BackupOverview: move restore buttons inline

[Dominik Csapak]

instead of having them in the toolbar. This makes the UI more consistent
with the datastore content view.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js | 86 +++++++++++++-------------------------
 1 file changed, 28 insertions(+), 58 deletions(-)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index 91ff1aac..0e105274 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -16,58 +16,30 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    }).show();
 	},
 
-	restoreSingle: function(button, record) {
+	restore: function(view, rI, cI, item, e, rec) {
 	    let me = this;
-	    let view = me.getView();
-	    let selection = view.getSelection();
-	    if (!selection || selection.length < 1) {
-		return;
-	    }
 
-	    let node = selection[0];
-	    if (node.data.restoreid === undefined) {
-		return;
-	    }
-	    let restoreid = node.data.restoreid;
-	    let mediaset = node.data['media-set'];
+	    let node = rec;
+	    let mediaset = node.data.is_media_set ? node.data.text : node.data['media-set'];
 	    let uuid = node.data['media-set-uuid'];
-	    let datastores = [node.data.store];
-
-	    Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
-		mediaset,
-		uuid,
-		list: [
-		    restoreid,
-		],
-		datastores,
-		listeners: {
-		    destroy: function() {
-			me.reload();
-		    },
-		},
-	    }).show();
-	},
 
-	restore: function(button, record) {
-	    let me = this;
-	    let view = me.getView();
-	    let selection = view.getSelection();
-	    if (!selection || selection.length < 1) {
-		return;
-	    }
-
-	    let node = selection[0];
-	    let mediaset = node.data.text;
-	    let uuid = node.data['media-set-uuid'];
-	    let datastores = node.data.datastores;
-	    while (!datastores && node.get('depth') > 2) {
-		node = node.parentNode;
+	    let list;
+	    let datastores;
+	    if (node.data.restoreid !== undefined) {
+		list = [node.data.restoreid];
+		datastores = [node.data.store];
+	    } else {
 		datastores = node.data.datastores;
+		while (!datastores && node.get('depth') > 2) {
+		    node = node.parentNode;
+		    datastores = node.data.datastores;
+		}
 	    }
 	    Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
 		mediaset,
 		uuid,
 		datastores,
+		list,
 		listeners: {
 		    destroy: function() {
 			me.reload();
@@ -99,6 +71,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 		if (data[pool][media_set] === undefined) {
 		    data[pool][media_set] = entry;
 		    data[pool][media_set].text = media_set;
+		    data[pool][media_set].restore =true;
 		    data[pool][media_set].tapes = 1;
 		    data[pool][media_set]['seq-nr'] = undefined;
 		    data[pool][media_set].is_media_set = true;
@@ -180,6 +153,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 
 		for (let entry of list.result.data) {
 		    entry.text = entry.snapshot;
+		    entry.restore = true;
 		    entry.leaf = true;
 		    entry.children = [];
 		    entry['media-set'] = media_set;
@@ -289,22 +263,6 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    text: gettext('New Backup'),
 	    handler: 'backup',
 	},
-	{
-	    xtype: 'proxmoxButton',
-	    disabled: true,
-	    text: gettext('Restore Media Set'),
-	    handler: 'restore',
-	    parentXType: 'treepanel',
-	    enableFn: (rec) => !!rec.data['media-set-uuid'],
-	},
-	{
-	    xtype: 'proxmoxButton',
-	    disabled: true,
-	    text: gettext('Restore Snapshot'),
-	    handler: 'restoreSingle',
-	    parentXType: 'treepanel',
-	    enableFn: (rec) => !!rec.data.restoreid,
-	},
     ],
 
     columns: [
@@ -315,6 +273,18 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    sortable: false,
 	    flex: 3,
 	},
+	{
+	    header: gettext('Actions'),
+	    xtype: 'actioncolumn',
+	    items: [
+		{
+		    handler: 'restore',
+		    tooltip: gettext('Restore'),
+		    getClass: (v, m, rec) => rec.data.restore ? 'fa fa-fw fa-undo' : 'pmx-hidden',
+		    isDisabled: (v, r, c, i, rec) => !rec.data.restore,
+                },
+	    ],
+	},
 	{
 	    text: gettext('Tapes'),
 	    dataIndex: 'tapes',
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup v2 2/5] ui: tape/BackupOverview: move restore buttons inline

[Thomas Lamprecht]

On 14.05.21 14:59, Dominik Csapak wrote:
> instead of having them in the toolbar. This makes the UI more consistent
> with the datastore content view.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/tape/BackupOverview.js | 86 +++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 58 deletions(-)
> 
>

applied, thanks!

[pbs-devel] [PATCH proxmox-backup v2 3/5] ui: tape/window/TapeRestore: add SnapshotGrid Component

[Dominik Csapak]

this will be used for letting the user select multiple, individual
snapshots on restore (instead of having a single or the whole media-set)

if a 'prefilter' object is given, we filter the grid by those
values using the gridfilter plugins (like in pve's bulk action windows)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/window/TapeRestore.js | 110 +++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index c09ae7f1..9967d9d8 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -309,3 +309,113 @@ Ext.define('PBS.TapeManagement.DataStoreMappingGrid', {
 	},
     ],
 });
+
+Ext.define('PBS.TapeManagement.SnapshotGrid', {
+    extend: 'Ext.grid.Panel',
+    alias: 'widget.pbsTapeSnapshotGrid',
+    mixins: ['Ext.form.field.Field'],
+
+    getValue: function() {
+	let me = this;
+	let snapshots = [];
+
+	me.getSelection().forEach((rec) => {
+	    let id = rec.get('id');
+	    let store = rec.data.store;
+	    let snap = rec.data.snapshot;
+	    // only add if not filtered
+	    if (me.store.findExact('id', id) !== -1) {
+		snapshots.push(`${store}:${snap}`);
+	    }
+	});
+
+	return snapshots;
+    },
+
+    setValue: function(value) {
+	let me = this;
+	// not implemented
+	return me;
+    },
+
+    getErrors: function(value) {
+	let me = this;
+	if (me.getSelection() < 1) {
+	    me.addCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
+	    let errorMsg = gettext("Need at least one snapshot");
+	    me.getActionEl().dom.setAttribute('data-errorqtip', errorMsg);
+
+	    return [errorMsg];
+	}
+	me.removeCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
+	me.getActionEl().dom.setAttribute('data-errorqtip', "");
+	return [];
+    },
+
+    scrollable: true,
+    height: 350,
+    plugins: 'gridfilters',
+
+    viewConfig: {
+	emptyText: gettext('No Snapshots'),
+	markDirty: false,
+    },
+
+    selModel: 'checkboxmodel',
+    store: {
+	sorters: ['store', 'snapshot'],
+	data: [],
+	filters: [],
+    },
+
+    listeners: {
+	selectionchange: function() {
+	    // to trigger validity and error checks
+	    this.checkChange();
+	},
+    },
+
+    checkChangeEvents: [
+	'selectionchange',
+	'change',
+    ],
+
+    columns: [
+	{
+	    text: gettext('Source Datastore'),
+	    dataIndex: 'store',
+	    filter: {
+		type: 'list',
+	    },
+	    flex: 1,
+	},
+	{
+	    text: gettext('Snapshot'),
+	    dataIndex: 'snapshot',
+	    filter: {
+		type: 'string',
+	    },
+	    flex: 2,
+	},
+    ],
+
+    initComponent: function() {
+	let me = this;
+	me.callParent();
+	if (me.prefilter !== undefined) {
+	    me.store.filters.add(
+		{
+		    id: 'x-gridfilter-store',
+		    property: 'store',
+		    operator: 'in',
+		    value: [me.prefilter.store],
+		},
+		{
+		    id: 'x-gridfilter-snapshot',
+		    property: 'snapshot',
+		    value: me.prefilter.snapshot,
+		},
+	    );
+	}
+    },
+});
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup v2 3/5] ui: tape/window/TapeRestore: add SnapshotGrid Component

[Thomas Lamprecht]

On 14.05.21 14:59, Dominik Csapak wrote:
> this will be used for letting the user select multiple, individual
> snapshots on restore (instead of having a single or the whole media-set)
> 
> if a 'prefilter' object is given, we filter the grid by those
> values using the gridfilter plugins (like in pve's bulk action windows)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/tape/window/TapeRestore.js | 110 +++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
>

applied, thanks!

[pbs-devel] [PATCH proxmox-backup v2 4/5] ui: tape/window/TapeRestore: enabling selecting multiple snapshots

[Dominik Csapak]

by including the new snapshotselector. If a whole media-set is to be
restored, select all snapshots

to achieve this, we drop the 'restoreid' and 'datastores' properties
for the restore window, and replace them by a 'prefilter' object
(with 'store' and 'snapshot' properties)

to be able to show the snapshots, we now have to always load the
content of that media-set, so drop the short-circuit if we have
the datastores already.

also to improve space-usage, shift the datastores mapping grid in the
right column, and all non datastore related options in the left one,
showing the snapshot grid below
(the datastore mapping is now limited to 150px; ~3 datastores, and scrollable)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js     | 27 +++-------
 www/tape/window/TapeRestore.js | 99 ++++++++++++++++++----------------
 2 files changed, 61 insertions(+), 65 deletions(-)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index 0e105274..eb8ef907 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -19,27 +19,13 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	restore: function(view, rI, cI, item, e, rec) {
 	    let me = this;
 
-	    let node = rec;
-	    let mediaset = node.data.is_media_set ? node.data.text : node.data['media-set'];
-	    let uuid = node.data['media-set-uuid'];
-
-	    let list;
-	    let datastores;
-	    if (node.data.restoreid !== undefined) {
-		list = [node.data.restoreid];
-		datastores = [node.data.store];
-	    } else {
-		datastores = node.data.datastores;
-		while (!datastores && node.get('depth') > 2) {
-		    node = node.parentNode;
-		    datastores = node.data.datastores;
-		}
-	    }
+	    let mediaset = rec.data.is_media_set ? rec.data.text : rec.data['media-set'];
+	    let uuid = rec.data['media-set-uuid'];
+	    let prefilter = rec.data.prefilter;
 	    Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
 		mediaset,
 		uuid,
-		datastores,
-		list,
+		prefilter,
 		listeners: {
 		    destroy: function() {
 			me.reload();
@@ -157,7 +143,10 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 		    entry.leaf = true;
 		    entry.children = [];
 		    entry['media-set'] = media_set;
-		    entry.restoreid = `${entry.store}:${entry.snapshot}`;
+		    entry.prefilter = {
+			store: entry.store,
+			snapshot: entry.snapshot,
+		    };
 		    let iconCls = PBS.Utils.get_type_icon_cls(entry.snapshot);
 		    if (iconCls !== '') {
 			entry.iconCls = `fa ${iconCls}`;
diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index 9967d9d8..9359a030 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -13,12 +13,8 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 
     cbindData: function(config) {
 	let me = this;
-	me.isSingle = false;
-	me.listText = "";
-	if (me.list !== undefined) {
-	    me.isSingle = true;
-	    me.listText = me.list.join('<br>');
-	    me.title = gettext('Restore Snapshot');
+	if (me.prefilter !== undefined) {
+	    me.title = gettext('Restore Snapshot(s)');
 	}
 	return {};
     },
@@ -46,8 +42,8 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		}
 		delete values.mapping;
 
-		if (me.up('window').list !== undefined) {
-		    values.snapshots = me.up('window').list;
+		if (Ext.isString(values.snapshots)) {
+		    values.snapshots = values.snapshots.split(',');
 		}
 
 		values.store = datastores.join(',');
@@ -72,23 +68,11 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 			value: '{uuid}',
 		    },
 		},
-		{
-		    xtype: 'displayfield',
-		    fieldLabel: gettext('Snapshot(s)'),
-		    submitValue: false,
-		    cbind: {
-			hidden: '{!isSingle}',
-			value: '{listText}',
-		    },
-		},
 		{
 		    xtype: 'pbsDriveSelector',
 		    fieldLabel: gettext('Drive'),
 		    name: 'drive',
 		},
-	    ],
-
-	    column2: [
 		{
 		    xtype: 'pbsUserSelector',
 		    name: 'notify-user',
@@ -109,9 +93,13 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		    skipEmptyText: true,
 		    renderer: Ext.String.htmlEncode,
 		},
+	    ],
+
+	    column2: [
 		{
 		    xtype: 'pbsDataStoreSelector',
 		    fieldLabel: gettext('Target Datastore'),
+		    labelWidth: 120,
 		    reference: 'defaultDatastore',
 		    name: 'store',
 		    listeners: {
@@ -122,9 +110,6 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 			},
 		    },
 		},
-	    ],
-
-	    columnB: [
 		{
 		    fieldLabel: gettext('Datastore Mapping'),
 		    labelWidth: 200,
@@ -140,6 +125,20 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		    hidden: true,
 		},
 	    ],
+
+	    columnB: [
+		{
+		    xtype: 'pbsTapeSnapshotGrid',
+		    reference: 'snapshotGrid',
+		    name: 'snapshots',
+		    // will be shown/enabled on successful load
+		    disabled: true,
+		    hidden: true,
+		    cbind: {
+			prefilter: '{prefilter}',
+		    },
+		},
+	    ],
 	},
     ],
 
@@ -172,29 +171,34 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 	let me = this;
 
 	me.callParent();
-	if (me.datastores) {
-	    me.setDataStores(me.datastores);
-	} else {
-	    // use timeout so that the window is rendered already
-	    // for correct masking
-	    setTimeout(function() {
-		Proxmox.Utils.API2Request({
-		    waitMsgTarget: me,
-		    url: `/tape/media/content?media-set=${me.uuid}`,
-		    success: function(response, opt) {
-			let datastores = {};
-			for (const content of response.result.data) {
-			    datastores[content.store] = true;
-			}
-			me.setDataStores(Object.keys(datastores));
-		    },
-		    failure: function() {
-			// ignore failing api call, maybe catalog is missing
-			me.setDataStores();
-		    },
-		});
-	    }, 10);
-	}
+	// use timeout so that the window is rendered already
+	// for correct masking
+	let grid = me.lookup('snapshotGrid');
+	setTimeout(function() {
+	    Proxmox.Utils.API2Request({
+		waitMsgTarget: me,
+		url: `/tape/media/content?media-set=${me.uuid}`,
+		success: function(response, opt) {
+		    let datastores = {};
+		    for (const content of response.result.data) {
+			datastores[content.store] = true;
+		    }
+		    me.setDataStores(Object.keys(datastores));
+		    if (response.result.data.length > 0) {
+			grid.setDisabled(false);
+			grid.setVisible(true);
+			grid.getStore().setData(response.result.data);
+			grid.getSelectionModel().selectAll();
+			// we've shown a big list, center the window again
+			me.center();
+		    }
+		},
+		failure: function() {
+		    // ignore failing api call, maybe catalog is missing
+		    me.setDataStores();
+		},
+	    });
+	}, 10);
     },
 });
 
@@ -203,6 +207,9 @@ Ext.define('PBS.TapeManagement.DataStoreMappingGrid', {
     alias: 'widget.pbsDataStoreMappingField',
     mixins: ['Ext.form.field.Field'],
 
+    maxHeight: 150,
+    scrollable: true,
+
     getValue: function() {
 	let me = this;
 	let datastores = [];
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup v2 4/5] ui: tape/window/TapeRestore: enabling selecting multiple snapshots

[Thomas Lamprecht]

On 14.05.21 14:59, Dominik Csapak wrote:
> by including the new snapshotselector. If a whole media-set is to be
> restored, select all snapshots
> 
> to achieve this, we drop the 'restoreid' and 'datastores' properties
> for the restore window, and replace them by a 'prefilter' object
> (with 'store' and 'snapshot' properties)
> 
> to be able to show the snapshots, we now have to always load the
> content of that media-set, so drop the short-circuit if we have
> the datastores already.
> 
> also to improve space-usage, shift the datastores mapping grid in the
> right column, and all non datastore related options in the left one,
> showing the snapshot grid below
> (the datastore mapping is now limited to 150px; ~3 datastores, and scrollable)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/tape/BackupOverview.js     | 27 +++-------
>  www/tape/window/TapeRestore.js | 99 ++++++++++++++++++----------------
>  2 files changed, 61 insertions(+), 65 deletions(-)
> 


I tried it now a bit, also with multi-datastores, I found one thing quite confusing:

1. restore whole mediaset with multiple datastore
2. set mapping for only one datastore and no default datastore
-> the snapshots on other datastore are still selected, but for the user this is highly
   confusing, as they cannot know about what will happen to them? Not restored, used in
   the same mapping, ..

Deselection of those snapshots without a datastore-map is not an option, as the next
datastore-mapping could be still configured immediately after setting the first one.


IIRC, you mentioned the idea of a multi-step wizard, that could solve this here more
nicely. I'd do two steps:

1. Show "Media Set" and "Media Set UUID" at the top, then only the snapshot grid below
2. Now we know all possible datastores from the selected snapshot list, so the
   datastore-map-grid can be filtered to only show relevant ones.
   
The drive could go in either step, at least wouldn't be completely off.

That would separate the "what do I need to restore" from the "where do I want to put it"
a bit and two simpler sequential panels are probably easier to grasp for a user than
one rather complex one.

What do you think?

Re: [pbs-devel] [PATCH proxmox-backup v2 4/5] ui: tape/window/TapeRestore: enabling selecting multiple snapshots

[Thomas Lamprecht]

On 18.05.21 09:00, Thomas Lamprecht wrote:
> On 14.05.21 14:59, Dominik Csapak wrote:
>> by including the new snapshotselector. If a whole media-set is to be
>> restored, select all snapshots
>>
>> to achieve this, we drop the 'restoreid' and 'datastores' properties
>> for the restore window, and replace them by a 'prefilter' object
>> (with 'store' and 'snapshot' properties)
>>
>> to be able to show the snapshots, we now have to always load the
>> content of that media-set, so drop the short-circuit if we have
>> the datastores already.
>>
>> also to improve space-usage, shift the datastores mapping grid in the
>> right column, and all non datastore related options in the left one,
>> showing the snapshot grid below
>> (the datastore mapping is now limited to 150px; ~3 datastores, and scrollable)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>  www/tape/BackupOverview.js     | 27 +++-------
>>  www/tape/window/TapeRestore.js | 99 ++++++++++++++++++----------------
>>  2 files changed, 61 insertions(+), 65 deletions(-)
>>
> 

Oh, and before I forget it, it irked me a bit that the content tree's expansion
state was not restored after the store reloaded triggered due to the restore
edit-window getting closed, all just got collapsed again.
Maybe you can improve that? I'm not even sure that a reload is useful after the
close, as any tape-restore itself did not alter the content there.

Re: [pbs-devel] [PATCH proxmox-backup v2 4/5] ui: tape/window/TapeRestore: enabling selecting multiple snapshots

[Dominik Csapak]

On 5/18/21 7:00 PM, Thomas Lamprecht wrote:
> On 18.05.21 09:00, Thomas Lamprecht wrote:
>> On 14.05.21 14:59, Dominik Csapak wrote:
>>> by including the new snapshotselector. If a whole media-set is to be
>>> restored, select all snapshots
>>>
>>> to achieve this, we drop the 'restoreid' and 'datastores' properties
>>> for the restore window, and replace them by a 'prefilter' object
>>> (with 'store' and 'snapshot' properties)
>>>
>>> to be able to show the snapshots, we now have to always load the
>>> content of that media-set, so drop the short-circuit if we have
>>> the datastores already.
>>>
>>> also to improve space-usage, shift the datastores mapping grid in the
>>> right column, and all non datastore related options in the left one,
>>> showing the snapshot grid below
>>> (the datastore mapping is now limited to 150px; ~3 datastores, and scrollable)
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   www/tape/BackupOverview.js     | 27 +++-------
>>>   www/tape/window/TapeRestore.js | 99 ++++++++++++++++++----------------
>>>   2 files changed, 61 insertions(+), 65 deletions(-)
>>>
>>
> 
> Oh, and before I forget it, it irked me a bit that the content tree's expansion
> state was not restored after the store reloaded triggered due to the restore
> edit-window getting closed, all just got collapsed again.
> Maybe you can improve that? I'm not even sure that a reload is useful after the
> close, as any tape-restore itself did not alter the content there.
> 

yes you're right, restore should not trigger a reload

as for keeping the expanded state on a reload, i'll have to check how
much work that is. it's a bit different than e.g. the datastore content,
since we here have separate api calls for the first and subsequent 
levels, so to check for differences gets a bit more complicated and
possibly more expensive in terms of api calls

Re: [pbs-devel] [PATCH proxmox-backup v2 4/5] ui: tape/window/TapeRestore: enabling selecting multiple snapshots

[Dominik Csapak]

On 5/18/21 9:00 AM, Thomas Lamprecht wrote:
> On 14.05.21 14:59, Dominik Csapak wrote:
>> by including the new snapshotselector. If a whole media-set is to be
>> restored, select all snapshots
>>
>> to achieve this, we drop the 'restoreid' and 'datastores' properties
>> for the restore window, and replace them by a 'prefilter' object
>> (with 'store' and 'snapshot' properties)
>>
>> to be able to show the snapshots, we now have to always load the
>> content of that media-set, so drop the short-circuit if we have
>> the datastores already.
>>
>> also to improve space-usage, shift the datastores mapping grid in the
>> right column, and all non datastore related options in the left one,
>> showing the snapshot grid below
>> (the datastore mapping is now limited to 150px; ~3 datastores, and scrollable)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   www/tape/BackupOverview.js     | 27 +++-------
>>   www/tape/window/TapeRestore.js | 99 ++++++++++++++++++----------------
>>   2 files changed, 61 insertions(+), 65 deletions(-)
>>
> 
> 
> I tried it now a bit, also with multi-datastores, I found one thing quite confusing:
> 
> 1. restore whole mediaset with multiple datastore
> 2. set mapping for only one datastore and no default datastore
> -> the snapshots on other datastore are still selected, but for the user this is highly
>     confusing, as they cannot know about what will happen to them? Not restored, used in
>     the same mapping, ..
> 
> Deselection of those snapshots without a datastore-map is not an option, as the next
> datastore-mapping could be still configured immediately after setting the first one.
> 
> 
> IIRC, you mentioned the idea of a multi-step wizard, that could solve this here more
> nicely. I'd do two steps:
> 
> 1. Show "Media Set" and "Media Set UUID" at the top, then only the snapshot grid below
> 2. Now we know all possible datastores from the selected snapshot list, so the
>     datastore-map-grid can be filtered to only show relevant ones.
>     
> The drive could go in either step, at least wouldn't be completely off.
> 
> That would separate the "what do I need to restore" from the "where do I want to put it"
> a bit and two simpler sequential panels are probably easier to grasp for a user than
> one rather complex one.
> 
> What do you think?
> 


yes i have imagined something like this, but went for the easier option
first. I'll do it like you suggested, putting the drive on the second
page, since i think it fits better with the rest of that page
('where')

thanks for the feedback :)

[pbs-devel] [PATCH proxmox-backup v2 5/5] ui: tape/BackupOverview: also allow to filter by group for restore

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index eb8ef907..3d49678b 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -182,6 +182,12 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 			    text,
 			    'media-set-uuid': entry['media-set-uuid'],
 			    leaf: false,
+			    restore: true,
+			    prefilter: {
+				store,
+				snapshot: `${type}/${group}/`,
+			    },
+			    'media-set': media_set,
 			    iconCls: `fa ${iconCls}`,
 			    children: [],
 			});
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup v2 0/5] ui: unify and improve tape restore window

[Dominik Csapak]

this is of course v3 not v2..
i should probably learn to count^^

On 5/14/21 14:59, Dominik Csapak wrote:
> changes from v1:
> * split patches in proper self-containted commits
> * changed layout in restore window
>     datastore mapping grid is now on the right, other options on the left
> * integrated the selection checkbox into the checkbox selection model of
>    the grid (removing a seperate field)
> * integrated the filter textbox into the gridfilters plugin, to save space
>    (like we do in pves bulk action window)
>    the benefit of this is that we do not have to combine/split the
>    store:snapshots anymore besides when assembling the submitdata,
>    this makes the code a little nicer imho
> 
> Dominik Csapak (5):
>    ui: tape/BackupOverview: fix wrong media-set text for singlerestore
>    ui: tape/BackupOverview: move restore buttons inline
>    ui: tape/window/TapeRestore: add SnapshotGrid Component
>    ui: tape/window/TapeRestore: enabling selecting multiple snapshots
>    ui: tape/BackupOverview: also allow to filter by group for restore
> 
>   www/tape/BackupOverview.js     |  99 ++++++----------
>   www/tape/window/TapeRestore.js | 209 +++++++++++++++++++++++++--------
>   2 files changed, 196 insertions(+), 112 deletions(-)
>