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 98487B13A for ; Wed, 6 Apr 2022 13:26:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8E71F29E78 for ; Wed, 6 Apr 2022 13:26:18 +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 7DC5929E6C for ; Wed, 6 Apr 2022 13:26:17 +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 502FB460D0 for ; Wed, 6 Apr 2022 13:26:17 +0200 (CEST) Message-ID: Date: Wed, 6 Apr 2022 13:25:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: pve-devel@lists.proxmox.com, Matthias Heiserer References: <20220404130211.4138797-1-m.heiserer@proxmox.com> <20220404130211.4138797-3-m.heiserer@proxmox.com> From: Fabian Ebner In-Reply-To: <20220404130211.4138797-3-m.heiserer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.425 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.631 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 - Subject: Re: [pve-devel] [PATCH v4 manager 2/4] ui: storage: 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: Wed, 06 Apr 2022 11:26:18 -0000 Am 04.04.22 um 15:02 schrieb Matthias Heiserer: > +Ext.define('PVE.storage.BackupView', { > + extend: 'Ext.tree.Panel', > alias: 'widget.pveStorageBackupView', > > - showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'], > + rootVisible: false, > + multiColumnSort: true, > + > + title: gettext('Content'), > + > + viewModel: { > + data: { > + isPBS: false, > + isStorage: true, Nit: IMHO fixedStorage might be a little bit more descriptive > + }, > + }, > + (...) > > - var sm = me.sm = Ext.create('Ext.selection.RowModel', {}); > + let viewModel = me.getViewModel(); > + viewModel.set('nodename', me.nodename); > + viewModel.set('isPBS', view.pluginType === 'pbs'); > + > + if (me.vmid) { > + me.getView().getStore().filter(me.guestFilter()); > + viewModel.set('isStorage', false); > + } else { > + me.lookup('storagesel').setVisible(false); > + me.lookup('backupNowButton').setVisible(false); Nit: Could have backupNowButton hidden by default and add a binding (like storagesel already has). Then there should be no need for the else branch. And having viewModel.set('isStorage', !!me.vmid); outside the if feels a bit cleaner/more future-proof to me. > + } > + Proxmox.Utils.monStoreErrors(view, me.store); > + }, > + > + onLoad: function(store, records, success, operation) { > + let me = this; > + let view = me.getView(); > + let selection = view.getSelection()?.[0]; > + selection = selection > + ? `${selection.parentNode.data.text}${selection.data.volid}` > + : false; > + > + let storage = PVE.data.ResourceStore.findRecord( Nit: could use the simpler getById() > + 'id', > + `storage/${me.nodename}/${me.storage}`, > + 0, // startIndex > + false, // anyMatch > + true, // caseSensitive > + true, // exactMatch > + ); > + let viewModel = me.getViewModel(); > + viewModel.set('isPBS', storage.data.plugintype === 'pbs'); > + > + let expanded = {}; > + view.getRootNode().cascadeBy({ > + before: item => { > + if (item.isExpanded() && !item.data.leaf) { > + let id = item.data.text; > + expanded[me.storage + '/' + id] = true; > + return true; > + } > + return false; > + }, > + after: Ext.emptyFn, > + }); > + let groups = me.getRecordGroups(records, expanded); > + > + for (const item of records.map(i => i.data)) { > + item.text = item.volid; > + item.leaf = true; > + item.iconCls = 'fa-file-o'; > + item.backupType = PVE.Utils.get_backup_type(item.volid, item.format); > + item.groupName = me.getGroupName(item); > + groups[me.getGroupName(item)].children.push(item); > + groups[me.getGroupName(item)].size += item.size; Style nit: could create a variable for the group name to avoid calling the function thrice > + } > + > + for (let [_name, group] of Object.entries(groups)) { > + let children = group.children; > + let latest = children.reduce((l, r) => l.ctime > r.ctime ? l : r); > + group.ctime = latest.ctime; > + if (viewModel.get('isPBS')) { > + group.size = latest.size; > + } > + let num_verified = children.reduce((l, r) => l + r.verification === 'ok', 0); > + group.verified = num_verified / children.length; > + } > + > + let children = []; > + Object.entries(groups).forEach(e => children.push(e[1])); > + view.setRootNode({ > + expanded: true, > + children: children, > + }); > + > + if (selection) { > + let rootnode = view.getRootNode(); > + let selected; > + rootnode.cascade(node => { > + if (selected) {return false;} // skip if already found Style nit: same as in v2 ;) > + let id = node.parentNode?.data?.text + node.data?.volid; > + if (id === selection) { > + selected = node; > + return false; > + } > + return true; > + }); (...) > + > + changeProtectionHandler: function(button, event, rec) { > + let me = this; > + const volid = rec.data.volid; > + Proxmox.Utils.API2Request({ > + url: `/api2/extjs/nodes/${me.nodename}/storage/${me.storage}/content/${volid}`, > + method: 'PUT', > + waitMsgTarget: button, IMHO it looks a bit strange when clicking the button > + params: { 'protected': rec.data.protected ? 0 : 1 }, > + failure: (response) => Ext.Msg.alert('Error', response.htmlStatus), > + success: (_) => me.reload(), > + }); > + }, > + > + pruneGroupHandler: function(button, event, rec) { > + let me = this; > + let vmtype = PVE.Utils.get_backup_type(rec.data.volid, rec.data.format); > + Ext.create('PVE.window.Prune', { > + nodename: me.nodename, > + storage: me.storage, > + backup_id: rec.data.vmid, > + backup_type: vmtype, > + rec: rec, > + listeners: { > + destroy: () => me.reload(), > + }, > + }).show(); > + }, > + > + 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}`, > + 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('searchFilter'); > + let volidFilter = Ext.create('Ext.util.Filter', { > + property: 'volid', > + value: field.getValue(), > + anyMatch: true, > + caseSensitive: false, > + }); > + let formatFilter = Ext.create('Ext.util.Filter', { > + property: 'format', > + value: field.getValue(), > + anyMatch: true, > + caseSensitive: false, > + }); > + me.getView().getStore().filter({ > + filterFn: item => volidFilter.filter(item.data) || > + formatFilter.filter(item.data), > + id: 'searchFilter', > + }); Nit: If it's not too much work, it would be nice if searching by the (displayed) group name would also work on PBS. Or maybe we want to change the displayed group name to ct/ID vm/ID there (but then one has to be careful with passing along the correct thing for prune)? > + }, > + > + searchClearHandler: function(field) { > + field.triggers.clear.setVisible(false); > + field.setValue(this.originalValue); > + this.getView().getStore().getFilters().removeByKey('searchFilter'); > + }, > + > + searchChangeFn: function(field, newValue, oldValue) { > + if (newValue !== field.originalValue) { > + field.triggers.clear.setVisible(true); > + } > + }, > + > + storageSelectorChange: function(self, newValue, oldValue, eOpts) { > + let me = this; > + if (!me.getViewModel().get('isStorage')) { > + me.storage = newValue; > + me.collapseAllStatus = true; > + me.store.on('load', () => me.lookup('collapseToggle').click(), me, { single: true }); Style nit: line too long > + 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(); > + }, > + > + toggleCollapseHandler: function(button, event) { > + let me = this; > + let groups = me.getView().getRootNode().childNodes; > + if (me.collapseAllStatus) { > + groups.forEach(node => node.expand()); > + button.setText(button.defaultText); > + } else { > + button.setText(button.altText); > + groups.forEach(node => node.collapse()); > + } > + me.collapseAllStatus = !me.collapseAllStatus; > + }, > + > + checkboxChangeHandler: function(checkbox, filterVMID) { > + let me = this; > + if (filterVMID) { > + me.getView().getStore().filter(me.guestFilter()); > + } else { > + let filters = me.getView().getStore().getFilters(); > + me.guestFilter().forEach(filter => filters.removeByKey(filter.id)); I feel like we should always filter by backup type in the guest view like is done currently. Otherwise, there is the possibility to try and restore e.g. an LXC backup over an existing VM. That probably isn't a common use case, and it just leads to an error. (...) > + triggers: { > + clear: { > + cls: 'pmx-clear-trigger', > + weight: -1, > + hidden: true, > + handler: 'searchClearHandler', > }, > }, > - '-', > - pruneButton, > - ); > - > - if (isPBS) { > - me.extraColumns = { > - encrypted: { > - header: gettext('Encrypted'), > - dataIndex: 'encrypted', > - renderer: PVE.Utils.render_backup_encryption, > - }, > - verification: { > - header: gettext('Verify State'), > - dataIndex: 'verification', > - renderer: PVE.Utils.render_backup_verification, > - }, > - }; > - } > - > - me.callParent(); > + }, > + ], Style nit: wrong indentation