From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Wed,  6 Apr 2022 13:26:17 +0200 (CEST)
Message-ID: <e24841cb-8076-3aa7-f989-dc16f968f2cc@proxmox.com>
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 <m.heiserer@proxmox.com>
References: <20220404130211.4138797-1-m.heiserer@proxmox.com>
 <20220404130211.4138797-3-m.heiserer@proxmox.com>
From: Fabian Ebner <f.ebner@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=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