[pbs-devel] [PATCH proxmox-backup v2] ui: tape/BackupOverview: unify and improve tape restore window
Dominik Csapak
d.csapak at proxmox.com
Fri May 14 08:29:58 CEST 2021
On 5/12/21 21:34, Thomas Lamprecht wrote:
> On 12.05.21 13:49, Dominik Csapak wrote:
>> adds a snapshot list to the restore window where the user can
>> select distinct snapshots to restore
>>
>> this also replaces the restore / restore single button by
>> inline actions for the media-set, backup groups and snapshots
>>
>> when clicking the action for a group/snapshot, the snapshotselector
>> gets activated and preselected/filtered so that it includes those
>> snapshots (can be overridden from the user)
>>
>> this change means that we now load the media set everytime we open
>> the window, since we want to have the list of snapshots.
>> (we could build that from the tree again, or hold it there somewhere as
>> list, but i do not think we gain much with that, and it simplifies the
>> datastore selection code a bit)
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>> changes from v1:
>> * fixed snapshot selection checkbox (was in wrong context, looup needs
>> to happen on the referenceHolder)
>> * removed unnecessary 'node' variable in 'restore' function
>> * fixed 'media-set' text in window (was the text of the node we clicked on)
>>
>
> Recently it seem to me like we alternate between "place every new function and
> it's use in 42 separate patches" and "dump^W squash three-zillion changes into
> one" like a metronome, none of those is ideal IMO.
>
> IOW, this really needs to be two patches, one for the content overview changes
> and the restore widow.
ok will do
>
> Also, from my last reply to the v1 thread:
>
>> IMO slightly better than radio-group/checkbox as it removes a UI control
>> element completely, so less choice/confusion, but without scarifying flexibility,
>> so IMO a good idea.
>
> Now you added the extra checkbox... Why not just use the grid-header checkbox like
> PVE backup-job edit-window and tick it by default when restoring a whole set?
>
> As said, reduction by one knob without losing any flexibility - seems like a win
> win to me, or do I overlook something?
>
> With the list below the window's aspect ration feels wrong and the list a bit
> squished, I'd add a few 100s px to the window height to improve that.
>
ok this was a misunderstanding then...
yeah using the 'check all' checkbox makes sense, but then i am unsure
how to handle filtering.
now only visible entries get restored, so if a user would (with the
check all) filter, it would seem like still all are selected and
we generate wrong assumptions?
we *should* be able to uncheck the check all box though to reduce
confusion, and this seems to me the best way forward.
the alternative is that we restore *all* selected entries, filtered or
not, but this can be similarly confusing since things get restored that
weren't visible
in pve's 'mass *' windows we have a behaviour like the first suggestion,
but i just noticed, depending on which column gets filtered, the
'check all' box sometimes stays checked and sometimes not...
as for the height, if there are multiple datastores on the media-set
(idk if you tested with this), there is another grid for the
datastore mapping, with that the window is already quite tall
maybe a multi-step 'wizard' would be better?
let the user first select the basic choices (drive,user,etc.)
then a window with a full-size grid of the snapshots, and then
depending on what he selected, either a target datastore
or datastore mapping input?
(or the snapshots first, and the rest of the info on the second panel?)
>> www/tape/BackupOverview.js | 91 ++++----------
>> www/tape/window/TapeRestore.js | 211 ++++++++++++++++++++++++++-------
>> 2 files changed, 197 insertions(+), 105 deletions(-)
>>
>> diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
>> index c028d58d..97725600 100644
>> --- a/www/tape/BackupOverview.js
>> +++ b/www/tape/BackupOverview.js
>> @@ -16,58 +16,16 @@ 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.text;
>> - 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;
>> - 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 restoreid = rec.data.restoreid;
>> Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
>> mediaset,
>> uuid,
>> - datastores,
>> + prefilter: restoreid,
>> listeners: {
>> destroy: function() {
>> me.reload();
>> @@ -99,6 +57,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;
>> @@ -155,14 +114,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,
>> },
>> });
>>
>> @@ -179,9 +139,11 @@ 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.restoreid = `${entry.store}:${entry.snapshot}`;
>> + entry['media-set'] = media_set;
>> let iconCls = PBS.Utils.get_type_icon_cls(entry.snapshot);
>> if (iconCls !== '') {
>> entry.iconCls = `fa ${iconCls}`;
>> @@ -217,6 +179,9 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
>> text,
>> 'media-set-uuid': entry['media-set-uuid'],
>> leaf: false,
>> + restore: true,
>> + restoreid: `${store}:${text}`,
>> + 'media-set': media_set,
>> iconCls: `fa ${iconCls}`,
>> children: [],
>> });
>> @@ -287,22 +252,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: [
>> @@ -313,6 +262,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',
>> diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
>> index 7e4f5cae..1844e5eb 100644
>> --- a/www/tape/window/TapeRestore.js
>> +++ b/www/tape/window/TapeRestore.js
>> @@ -10,14 +10,13 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>> showTaskViewer: true,
>> isCreate: true,
>>
>> + allowSnapshots: false,
>> +
>> 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.allowSnapshots = true;
>> + me.title = gettext('Restore Snapshot(s)');
>> }
>> return {};
>> },
>> @@ -45,8 +44,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(',');
>> @@ -71,15 +70,6 @@ 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'),
>> @@ -138,6 +128,30 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
>> defaultBindProperty: 'value',
>> hidden: true,
>> },
>> + {
>> + boxLabel: gettext('Snapshot Selection'),
>> + labelWidth: 200,
>> + xtype: 'proxmoxcheckbox',
>> + isFormField: false,
>> + cbind: {
>> + value: '{allowSnapshots}',
>> + },
>> + listeners: {
>> + change: function(cb, value) {
>> + let me = this;
>> + me.up('window').lookup('snapshotGrid').setDisabled(!value);
>> + },
>> + },
>> + },
>> + {
>> + xtype: 'pbsTapeSnapshotGrid',
>> + reference: 'snapshotGrid',
>> + name: 'snapshots',
>> + cbind: {
>> + disabled: '{!allowSnapshots}',
>> + prefilter: '{prefilter}',
>> + },
>> + },
>> ],
>> },
>> ],
>> @@ -171,29 +185,39 @@ 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
>> + 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));
>> + let grid = me.lookup('snapshotGrid');
>> + let store = grid.getStore();
>> + store.setData(response.result.data);
>> + if (me.prefilter !== undefined) {
>> + store.each((rec) => {
>> + let restoreid = `${rec.data.store}:${rec.data.snapshot}`;
>> + if (restoreid.indexOf(me.prefilter) !== -1) {
>> + rec.set('include', true);
>> + }
>> + });
>> +
>> + grid.setFilter(me.prefilter);
>> + }
>> + store.sort('snapshot');
>> + },
>> + failure: function() {
>> + // ignore failing api call, maybe catalog is missing
>> + me.setDataStores();
>> + },
>> + });
>> + }, 10);
>> },
>> });
>>
>> @@ -202,6 +226,10 @@ Ext.define('PBS.TapeManagement.DataStoreMappingGrid', {
>> alias: 'widget.pbsDataStoreMappingField',
>> mixins: ['Ext.form.field.Field'],
>>
>> + maxHeight: 150,
>> + scrollable: true,
>> + margin: '0 0 10 0',
>> +
>> getValue: function() {
>> let me = this;
>> let datastores = [];
>> @@ -308,3 +336,106 @@ 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.getStore().each((rec) => {
>> + if (rec.data.include) {
>> + let store = rec.data.store;
>> + let snap = rec.data.snapshot;
>> + snapshots.push(`${store}:${snap}`);
>> + }
>> + });
>> +
>> + return snapshots;
>> + },
>> +
>> + setValue: function(value) {
>> + let me = this;
>> + // not implemented
>> + return me;
>> + },
>> +
>> + getErrors: function(value) {
>> + let me = this;
>> + let firstSelected = me.getStore().findBy((rec) => !!rec.data.include);
>> +
>> + if (firstSelected === -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: 200,
>> +
>> + viewConfig: {
>> + emptyText: gettext('No Snapshots'),
>> + markDirty: false,
>> + },
>> +
>> + setFilter: function(value) {
>> + let me = this;
>> + me.down('textfield').setValue(value);
>> + },
>> +
>> + tbar: [
>> + {
>> + fieldLabel: gettext('Filter'),
>> + xtype: 'textfield',
>> + isFormField: false,
>> + listeners: {
>> + change: function(field, value) {
>> + let me = this;
>> + let grid = me.up('grid');
>> + let store = grid.getStore();
>> + store.clearFilter();
>> + store.filterBy((rec) => {
>> + let restoreid = `${rec.data.store}:${rec.data.snapshot}`;
>> + return restoreid.indexOf(value) !== -1;
>> + });
>> + grid.checkChange();
>> + },
>> + },
>> + },
>> + ],
>> +
>> + store: { data: [] },
>> +
>> + columns: [
>> + {
>> + xtype: 'checkcolumn',
>> + text: gettext('Include'),
>> + dataIndex: 'include',
>> + listeners: {
>> + checkchange: function(cb, value) {
>> + let grid = this.up('grid');
>> + grid.checkChange();
>> + },
>> + },
>> + },
>> + {
>> + text: gettext('Source Datastore'),
>> + dataIndex: 'store',
>> + flex: 1,
>> + },
>> + {
>> + text: gettext('Snapshot'),
>> + dataIndex: 'snapshot',
>> + flex: 4,
>> + },
>> + ],
>> +});
>>
>
More information about the pbs-devel
mailing list