From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 080C770602 for ; Wed, 12 May 2021 21:35:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E9CB48494 for ; Wed, 12 May 2021 21:34:50 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 022BC8481 for ; Wed, 12 May 2021 21:34:49 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C6AAF42A28 for ; Wed, 12 May 2021 21:34:48 +0200 (CEST) Message-ID: <130787f9-a4cd-def7-b04b-239c620db67b@proxmox.com> Date: Wed, 12 May 2021 21:34:46 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Thunderbird/89.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Dominik Csapak References: <20210512114952.19599-1-d.csapak@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210512114952.19599-1-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.006 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v2] ui: tape/BackupOverview: unify and improve tape restore window X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 May 2021 19:35:21 -0000 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 > --- > 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. 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. > 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('
'); > - 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, > + }, > + ], > +}); >