Re: [pbs-devel] [PATCH proxmox-backup v2 2/5] fix #3067: pbs ui: add support for a notes field in the dashboard

[Dominik Csapak <d.csapak@proxmox.com>]

high level comment:

i get what you tried to achieve here with the
container for info and notes, but i'd probably
simply add it as a regular panel.

that way, the user is not confused when the column selection
does not work the way he expects (e.g. setting it to 3 columns
still always has only the info/note as the first line)

so either a regular panel (without the collapsing feature, maybe
a browser storage option to hide it? like the days) or let
it behave like on the datastore summary (info+notes = 1 panel)

some comments inline

On 2/24/22 15:18, Stefan Sterz wrote:
> adds a panel to the dashboard displaying a comment similar to the
> comments panel in a datastore summary
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>   www/Dashboard.js                  | 110 ++++++++++++++++++------------
>   www/Makefile                      |   2 +-
>   www/datastore/Summary.js          |   6 +-
>   www/{datastore => panel}/Notes.js |  19 +++++-
>   4 files changed, 85 insertions(+), 52 deletions(-)
>   rename www/{datastore => panel}/Notes.js (81%)
> 
> diff --git a/www/Dashboard.js b/www/Dashboard.js
> index 70c2305b..a78ad375 100644
> --- a/www/Dashboard.js
> +++ b/www/Dashboard.js
> @@ -122,7 +122,7 @@ Ext.define('PBS.Dashboard', {
>   		if (key !== 'summarycolumns') {
>   		    return;
>   		}
> -		Proxmox.Utils.updateColumns(view);
> +		Proxmox.Utils.updateColumnWidth(view);

this does not work like that? you'd have to
use the 'view.query...' expression like below else the
child panels do not get updated

>   	    });
>   	},
>       },
> @@ -192,26 +192,14 @@ Ext.define('PBS.Dashboard', {
>   
>       listeners: {
>   	resize: function(panel) {
> -	    Proxmox.Utils.updateColumns(panel);
> +	    panel.query('>').forEach(c => Proxmox.Utils.updateColumnWidth(c));
>   	},
>       },
>   
>       title: gettext('Dashboard'),
>   
> -    layout: {
> -	type: 'column',
> -    },
> -
>       bodyPadding: '20 0 0 20',
>   
> -    minWidth: 700,
> -
> -    defaults: {
> -	columnWidth: 0.49,
> -	xtype: 'panel',
> -	margin: '0 20 20 0',
> -    },
> -
>       tools: [
>   	{
>   	    type: 'gear',
> @@ -224,42 +212,74 @@ Ext.define('PBS.Dashboard', {
>   
>       items: [
>   	{
> -	    xtype: 'pbsNodeInfoPanel',
> -	    reference: 'nodeInfo',
> -	    height: 280,
> -	},
> -	{
> -	    xtype: 'pbsDatastoresStatistics',
> +	    xtype: 'container',
> +	    layout: {
> +		type: 'hbox',
> +		align: 'stretch',
> +	    },
> +	    margin: '0 0 20 0',
> +	    padding: '0 20 0 0',
>   	    height: 280,
> -	},
> -	{
> -	    xtype: 'pbsLongestTasks',
> -	    bind: {
> -		title: gettext('Longest Tasks') + ' (' +
> -		Ext.String.format(gettext('{0} days'), '{days}') + ')',
> +	    defaults: {
> +		flex: 1,
>   	    },
> -	    reference: 'longesttasks',
> -	    height: 250,
> -	},
> -	{
> -	    xtype: 'pbsRunningTasks',
> -	    height: 250,
> -	},
> -	{
> -	    bind: {
> -		title: gettext('Task Summary') + ' (' +
> -		Ext.String.format(gettext('{0} days'), '{days}') + ')',
> +	    minWidth: 700,
> +	    items: [{
> +		xtype: 'pbsNodeInfoPanel',
> +		reference: 'nodeInfo',
> +		margin: '0 20 0 0',
>   	    },
> -	    xtype: 'pbsTaskSummary',
> -	    height: 200,
> -	    reference: 'tasksummary',
> +	    {
> +		xtype: 'pbsNotes',
> +		reference: 'nodeNotes',
> +		node: 'localhost',
> +		loadOnInit: true,
> +	    }],
>   	},
>   	{
> -	    iconCls: 'fa fa-ticket',
> -	    title: 'Subscription',
> -	    height: 200,
> -	    reference: 'subscription',
> -	    xtype: 'pbsSubscriptionInfo',
> +	    xtype: 'container',
> +	    layout: {
> +		type: 'column',
> +	    },
> +	    minWidth: 700,
> +	    defaults: {
> +		columnWidth: 0.5,
> +		xtype: 'panel',
> +		margin: '0 20 20 0',
> +	    },
> +	    items: [{
> +		xtype: 'pbsDatastoresStatistics',
> +		height: 250,
> +	    },
> +	    {
> +		xtype: 'pbsLongestTasks',
> +		bind: {
> +		    title: gettext('Longest Tasks') + ' (' +
> +		    Ext.String.format(gettext('{0} days'), '{days}') + ')',
> +		},
> +		reference: 'longesttasks',
> +		height: 250,
> +	    },
> +	    {
> +		xtype: 'pbsRunningTasks',
> +		height: 250,
> +	    },
> +	    {
> +		bind: {
> +		    title: gettext('Task Summary') + ' (' +
> +		    Ext.String.format(gettext('{0} days'), '{days}') + ')',
> +		},
> +		xtype: 'pbsTaskSummary',
> +		height: 250,
> +		reference: 'tasksummary',
> +	    },
> +	    {
> +		iconCls: 'fa fa-ticket',
> +		title: 'Subscription',
> +		height: 200,
> +		reference: 'subscription',
> +		xtype: 'pbsSubscriptionInfo',
> +	    }],
>   	},
>       ],
>   });
> diff --git a/www/Makefile b/www/Makefile
> index 455fbeec..636d4a57 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -81,6 +81,7 @@ JSSRC=							\
>   	panel/StorageAndDisks.js			\
>   	panel/UsageChart.js				\
>   	panel/NodeInfo.js				\
> +	panel/Notes.js				    \
>   	ZFSList.js					\
>   	DirectoryList.js				\
>   	LoginView.js					\
> @@ -88,7 +89,6 @@ JSSRC=							\
>   	SystemConfiguration.js				\
>   	Subscription.js					\
>   	datastore/Summary.js				\
> -	datastore/Notes.js				\
>   	datastore/PruneAndGC.js				\
>   	datastore/Prune.js				\
>   	datastore/Content.js				\
> diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
> index c3769257..d11646f0 100644
> --- a/www/datastore/Summary.js
> +++ b/www/datastore/Summary.js
> @@ -206,7 +206,7 @@ Ext.define('PBS.DataStoreSummary', {
>   		    },
>   		},
>   		{
> -		    xtype: 'pbsDataStoreNotes',
> +		    xtype: 'pbsNotes',
>   		    flex: 1,
>   		    cbind: {
>   			datastore: '{datastore}',
> @@ -278,14 +278,14 @@ Ext.define('PBS.DataStoreSummary', {
>   	    success: function(response) {
>   		let path = Ext.htmlEncode(response.result.data.path);
>   		me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
> -		me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
> +		me.down('pbsNotes').setNotes(response.result.data.comment);
>   	    },
>   	    failure: function(response) {
>   		// fallback if e.g. we have no permissions to the config
>   		let rec = Ext.getStore('pbs-datastore-list')
>   		    .findRecord('store', me.datastore, 0, false, true, true);
>   		if (rec) {
> -		    me.down('pbsDataStoreNotes').setNotes(rec.data.comment || "");
> +		    me.down('pbsNotes').setNotes(rec.data.comment || "");
>   		}
>   	    },
>   	});
> diff --git a/www/datastore/Notes.js b/www/panel/Notes.js
> similarity index 81%
> rename from www/datastore/Notes.js
> rename to www/panel/Notes.js
> index 2928b7ec..4128dd8b 100644
> --- a/www/datastore/Notes.js
> +++ b/www/panel/Notes.js
> @@ -1,6 +1,6 @@
> -Ext.define('PBS.DataStoreNotes', {
> +Ext.define('PBS.panel.Notes', {

imo, the changes below would warrant its own patch
(makes it easier to review)

>       extend: 'Ext.panel.Panel',
> -    xtype: 'pbsDataStoreNotes',
> +    xtype: 'pbsNotes',
>       mixins: ['Proxmox.Mixin.CBind'],
>   
>       title: gettext("Comment"),
> @@ -11,7 +11,16 @@ Ext.define('PBS.DataStoreNotes', {
>   
>       cbindData: function(initalConfig) {
>   	let me = this;
> -	me.url = `/api2/extjs/config/datastore/${me.datastore}`;
> +
> +	if (('node' in me && 'datastore' in me) ||
> +	    (!('node' in me) && !('datastore' in me))) {
> +	    throw 'either both a node and a datastore were given or neither. only provide one.';

i'd do that check differently for multiple reasons

"'foo' in bar" is not syntax we really use
having a single error for multiple error cases seems complicated

something like this is more readable:
---
if (!me.node && !me.datastore) {
     throw "no ...";
}
if (me.node && me.datastore) {
     throw "both ....";
}
--

alternatively, you could prefer the 'node' (or datastore, doesn't matter)
instead and only throw an error if none is given

> +	} else if ('node' in me) {
> +	    me.url = `/api2/extjs/nodes/${me.node}/config`;
> +	} else {
> +	    me.url = `/api2/extjs/config/datastore/${me.datastore}`;
> +	}
> +
>   	return { };
>       },
>   
> @@ -97,6 +106,10 @@ Ext.define('PBS.DataStoreNotes', {
>   	let sp = Ext.state.Manager.getProvider();
>   	me.collapseMode = sp.get('notes-collapse', 'never');
>   
> +	if (me.loadOnInit === true) {
> +	    me.load();
> +	}
> +
>   	if (me.collapseMode === 'auto') {
>   	    me.setCollapsed(true);
>   	}