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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with UTF8SMTPS id E4185706D4 for ; Fri, 14 May 2021 08:30:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with UTF8SMTP id D4BA812813 for ; Fri, 14 May 2021 08:30:08 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with UTF8SMTPS id C1DBB127D9 for ; Fri, 14 May 2021 08:30:06 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with UTF8SMTP id 8E42142966; Fri, 14 May 2021 08:30:00 +0200 (CEST) Message-ID: Date: Fri, 14 May 2021 08:29:58 +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: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20210512114952.19599-1-d.csapak@proxmox.com> <130787f9-a4cd-def7-b04b-239c620db67b@proxmox.com> From: Dominik Csapak In-Reply-To: <130787f9-a4cd-def7-b04b-239c620db67b@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.018 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [content.store, data.store, values.store, entry.store, rec.data, node.data] 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: Fri, 14 May 2021 06:30:08 -0000 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 >> --- >> 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('
'); >> - 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, >> + }, >> + ], >> +}); >> >