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 2256168E81 for ; Tue, 22 Mar 2022 09:43:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 12B6E1B59F for ; Tue, 22 Mar 2022 09:43:07 +0100 (CET) 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 AB2FB1B594 for ; Tue, 22 Mar 2022 09:43:05 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 80BDF4179B for ; Tue, 22 Mar 2022 09:43:05 +0100 (CET) Message-ID: Date: Tue, 22 Mar 2022 09:42:58 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.2 From: Fabian Ebner To: pve-devel@lists.proxmox.com, Matthias Heiserer , Thomas Lamprecht References: <20220318135226.2360890-1-m.heiserer@proxmox.com> <20220318135226.2360890-2-m.heiserer@proxmox.com> Content-Language: en-US In-Reply-To: <20220318135226.2360890-2-m.heiserer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.119 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [me.store, me.sm, me.storage] Subject: Re: [pve-devel] [PATCH manager 1/5 v2] Storage GUI: Rewrite backup content view as TreePanel. X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Mar 2022 08:43:37 -0000 Nit: for the commit title, prefixing like "ui: storage:" is preferred. I really think we should start with the tree fully expanded, or we will get shouted at by users relying on notes to find their backups ;) @Thomas: The backups are now ordered ascending by date again. In PBS it's also like that. Should that be changed or do we switch back in PVE? Am 18.03.22 um 14:52 schrieb Matthias Heiserer: > +Ext.define('PVE.storage.BackupView', { > + extend: 'Ext.tree.Panel', > alias: 'widget.pveStorageBackupView', > + mixins: ['Proxmox.Mixin.CBind'], > + rootVisible: false, > > - showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'], > + title: gettext('Content'), > > - initComponent: function() { > - var me = this; > + cbindData: function(initialCfg) { > + this.isPBS = initialCfg.pluginType === 'pbs'; That's part of what cbind should do for you if you return { isPBS: ... }, and that's how it should be used to avoid breaking the abstraction. But since the storage can change (in the guest backup view) this should rather be a normal bind and be updated there. > + return {}; > + }, > + isStorage: false, Doesn't seem to be used. > > - var nodename = me.nodename = me.pveSelNode.data.node; > - if (!nodename) { > - throw "no node name specified"; > - } > + controller: { > + xclass: 'Ext.app.ViewController', > > - var storage = me.storage = me.pveSelNode.data.storage; > - if (!storage) { > - throw "no storage ID specified"; > - } > + groupnameHelper: function(item) { Nit: IMHO something like getGroupName() would be a bit more readable at the call sites. > + if (item.vmid) { > + return PVE.Utils.get_backup_type(item.volid, item.format) + `/${item.vmid}`; > + } else { > + return 'Other'; > + } > + }, > > - me.content = 'backup'; > + init: function(view) { > + let me = this; > + me.storage = view?.pveSelNode?.data?.storage; Nit: here you use ?., but below you assume it's set (which I suppose we can). > + me.nodename = view.nodename || view.pveSelNode.data.node; Is there ever a case where it's not the selected node? > + me.vmid = view.pveSelNode.data.vmid; > + me.vmtype = view.pveSelNode.data.type; > > - var sm = me.sm = Ext.create('Ext.selection.RowModel', {}); > + me.store = Ext.create('Ext.data.Store', { > + model: 'PVE.storage.BackupModel', > + }); > + me.store.on('load', me.onLoad, me); > + view.getStore().setConfig('filterer', 'bottomup'); > + view.getStore().setSorters(['text']); > > - var reload = function() { > - me.store.load(); > - }; > + if (me.vmid) { > + me.getView().getStore().filter({ > + property: 'vmid', > + value: me.vmid, > + exactMatch: true, > + }); > + } else { > + me.lookup('storagesel').setVisible(false); > + me.lookup('backupNowButton').setVisible(false); > + } > + Proxmox.Utils.monStoreErrors(view, me.store); > + }, > > - let pruneButton = Ext.create('Proxmox.button.Button', { > - text: gettext('Prune group'), > - disabled: true, > - selModel: sm, > - setBackupGroup: function(backup) { > - if (backup) { > - let name = backup.text; > - let vmid = backup.vmid; > - let format = backup.format; > + onLoad: function(store, records, success, operation) { > + let me = this; > + let view = me.getView(); > + let selection = view.getSelection()?.[0]; > + selection = selection?.parentNode?.data?.text +selection?.data?.volid; Style nit: missing space after + and could use `${...}${...}` syntax instead. (...) > + if (selection) { > + let rootnode = view.getRootNode(); > + let selected; > + rootnode.cascade(node => { > + if (selected) {return false;} // skip if already found Style nit: if body on the same line > + let id = node.parentNode?.data?.text + node.data?.volid; > + if (id === selection) { > + selected = node; > + return false; > + } > + return true; > }); > - win.show(); > - win.on('destroy', reload); > - }, > - }); > + view.setSelection(selected); > + view.getView().focusRow(selected); After removing a backup I get a Uncaught TypeError: node is undefined referencing this line. > + } > + Proxmox.Utils.setErrorMask(view, false); > + }, > (...) > + > + removeHandler: function(button, event, rec) { > + let me = this; > + const volid = rec.data.volid; > + Proxmox.Utils.API2Request({ > + url: `/nodes/${me.nodename}/storage/${me.storage}/content//${volid}`, Nit: duplicate / in line above > + method: 'DELETE', > + callback: () => me.reload(), > + failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus), > + }); > + }, > + > + searchKeyupFn: function(field) { > + let me = this; > + me.getView().getStore().getFilters().removeByKey('volid'); > + me.getView().getStore().filter([ > + { > + property: 'volid', > + value: field.getValue(), > + anyMatch: true, > + caseSensitive: false, > + id: 'volid', > }, > - verification: { > - header: gettext('Verify State'), > - dataIndex: 'verification', > - renderer: PVE.Utils.render_backup_verification, > + ]); > + }, > + > + searchClearHandler: function(field) { > + field.triggers.clear.setVisible(false); > + field.setValue(this.originalValue); > + this.getView().getStore().clearFilter(); > + }, > + > + searchChangeFn: function(field, newValue, oldValue) { > + if (newValue !== field.originalValue) { > + field.triggers.clear.setVisible(true); > + } > + }, > + > + storageSelectorBoxReady: function(selector, width, height, eOpts) { > + selector.setNodename(this.nodename); > + }, Would cbind also be an option? > + > + storageSelectorChange: function(self, newValue, oldValue, eOpts) { > + let me = this; > + me.storage = newValue; > + me.getView().getSelectionModel().deselectAll(); > + me.reload(); > + }, > + > + backupNowHandler: function(button, event) { > + let me = this; > + Ext.create('PVE.window.Backup', { > + nodename: me.nodename, > + vmid: me.vmid, > + vmtype: me.vmtype, > + storage: me.storage, > + listeners: { > + close: () => me.reload(), > }, > - }; > - } > + }).show(); > + }, > + }, > + > + columns: [ > + { > + xtype: 'treecolumn', > + header: gettext("Backup Group"), > + dataIndex: 'text', > + renderer: function(value, _metadata, record) { > + if (record.phantom) { return value; } Style nit: if body on the same line. Could be avoided using ternary ? here. > + return PVE.Utils.render_storage_content(...arguments); > + }, > + flex: 2, > + }, > + { > + header: gettext('Notes'), > + flex: 1, > + renderer: Ext.htmlEncode, > + dataIndex: 'notes', > + }, > + { > + header: ``, > + tooltip: gettext('Protected'), > + width: 30, > + renderer: v => v ? `` : '', Style nit: line too long > + sorter: (a, b) => (b.data.protected || 0) - (a.data.protected || 0), > + dataIndex: 'protected', > + }, > + { > + header: gettext('Date'), > + width: 150, > + dataIndex: 'ctime', > + xtype: 'datecolumn', > + format: 'Y-m-d H:i:s', > + }, > + { > + header: gettext('Format'), > + width: 100, > + dataIndex: 'format', > + }, > + { > + header: gettext('Size'), > + width: 100, > + renderer: Proxmox.Utils.format_size, > + dataIndex: 'size', > + }, > + { > + header: gettext('Encrypted'), > + dataIndex: 'encrypted', > + renderer: PVE.Utils.render_backup_encryption, > + cbind: { > + hidden: '{!isPBS}', > + }, > + }, > + { > + header: gettext('Verify State'), > + dataIndex: 'verification', > + renderer: PVE.Utils.render_backup_verification, > + cbind: { > + hidden: '{!isPBS}', > + }, > + }, > + ], > + > + tbar: [ > + { > + xtype: 'button', > + text: gettext('Backup now'), > + handler: 'backupNowHandler', > + reference: 'backupNowButton', > + }, > + { > + xtype: 'proxmoxButton', > + text: gettext('Restore'), > + handler: 'restoreHandler', > + parentXType: "treepanel", > + disabled: true, > + enableFn: record => record.phantom === false, Style nit: !record.phantom is shorter Repeated below a few times > + }, > + { > + xtype: 'proxmoxButton', > + text: gettext('File Restore'), > + handler: 'restoreFilesHandler', > + cbind: { > + hidden: '{!isPBS}', > + }, > + parentXType: "treepanel", > + disabled: true, > + enableFn: record => record.phantom === false, > + }, > + { > + xtype: 'proxmoxButton', > + text: gettext('Show Configuration'), > + handler: 'showConfigurationHandler', > + parentXType: "treepanel", > + disabled: true, > + enableFn: record => record.phantom === false, > + }, > + { > + xtype: 'proxmoxButton', > + text: gettext('Edit Notes'), > + handler: 'editNotesHandler', > + parentXType: "treepanel", > + disabled: true, > + enableFn: record => record.phantom === false, > + }, > + { > + xtype: 'proxmoxButton', > + text: gettext('Change Protection'), > + handler: 'changeProtectionHandler', > + parentXType: "treepanel", > + disabled: true, > + enableFn: record => record.phantom === false, > + }, > + '-', > + { > + xtype: 'proxmoxButton', > + text: gettext('Prune group'), > + setBackupGroup: function(backup) { > + let me = this; > + if (backup) { > + let volid = backup.volid; > + let vmid = backup.vmid; > + let format = backup.format; > > - me.callParent(); > + let vmtype = PVE.Utils.get_backup_type(volid, format); > + if (vmid && vmtype) { > + me.setText(gettext('Prune group') + ` ${vmtype}/${vmid}`); > + me.vmid = vmid; > + me.vmtype = vmtype; > + me.setDisabled(false); > + return; > + } > + } > + me.setText(gettext('Prune group')); > + me.vmid = null; > + me.vmtype = null; > + me.setDisabled(true); > + }, > + handler: 'pruneGroupHandler', > + parentXType: "treepanel", > + disabled: true, > + reference: 'pruneButton', > + enableFn: () => true, This makes the disabling in setBackupGroup not work as intended. > + }, > + { > + xtype: 'proxmoxButton', > + text: gettext('Remove'), > + handler: 'removeHandler', > + parentXType: 'treepanel', > + disabled: true, > + enableFn: record => record.phantom === false && !record?.data?.protected, > + confirmMsg: function(rec) { > + let name = rec.data.text; > + return Ext.String.format(gettext('Are you sure you want to remove entry {0}'), `'${name}'`); Sytle nit: line too long > + }, > + }, > + '->', > + { > + xtype: 'pveStorageSelector', > + fieldLabel: gettext('Storage'), > + storageContent: 'backup', > + reference: 'storagesel', > + listeners: { > + change: 'storageSelectorChange', > + boxready: 'storageSelectorBoxReady', > + }, > > - me.store.getSorters().clear(); > - me.store.setSorters([ > - { > - property: 'vmid', > - direction: 'ASC', > + }, > + gettext('Search') + ':', > + ' ', > + { > + xtype: 'textfield', > + width: 200, > + enableKeyEvents: true, > + emptyText: gettext('Name, Format'), > + listeners: { > + keyup: { > + buffer: 500, > + fn: 'searchKeyupFn', > + }, > + change: 'searchChangeFn', > }, > - { > - property: 'vdate', > - direction: 'DESC', > + triggers: { > + clear: { > + cls: 'pmx-clear-trigger', > + weight: -1, > + hidden: true, > + handler: 'searchClearHandler', > + }, > }, > - ]); > + }, > + ], > + > + listeners: { > + activate: function() { > + let me = this; > + // only load on first activate to not load every tab switch > + if (!me.firstLoad) { > + me.getController().reload(); > + me.firstLoad = true; > + } This does not seem to apply here, because there are no tabs to switch to/from. Switching to and back from a different storage content type leads to me.firstLoad being undefined again. But I'd actually want to see changes when doing so in any case ;) > + }, > + selectionchange: function(model, srecords, eOpts) { > + let pruneButton = this.getController().lookup('pruneButton'); > + if (srecords.length === 1) { > + pruneButton.setBackupGroup(srecords[0].data); > + } else { > + pruneButton.setBackupGroup(null); > + } > + }, > }, > });