Re: [pve-devel] [PATCH manager 1/5 v2] Storage GUI: Rewrite backup content view as TreePanel.

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

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: `<i class="fa fa-shield"></i>`,
> +	    tooltip: gettext('Protected'),
> +	    width: 30,
> +	    renderer: v => v ? `<i data-qtip="${gettext('Protected')}" class="fa fa-shield"></i>` : '',

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);
> +	    }
> +	},
>      },
>  });