Re: [pve-devel] [PATCH v4 manager 2/4] ui: storage: Rewrite backup content view as TreePanel.

[Fabian Ebner <f.ebner@proxmox.com>]

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