[pve-devel] [PATCH v2 manager 0/2] ui: ceph: improve discoverability of warning details

[pve-devel] [PATCH v2 manager 0/2] ui: ceph: improve discoverability of warning details

[Aaron Lauterer]

The goal for this small series is to make it easier/more obvious to see
that there are potentially more details for a warning ceph is showing
by:
* having a tooltip with the details (limited in length)
* making the detail/info button more visible

changes since v1:

* change creation of info window to use autoShow
* rework tooltip to:
  * only destroy it when leaving the whole grid, use show/hide for all
    other situations
  * factor out text handling of newlines and max lines
  * factor out updating and destroying into their own functions
  * add more listeners
  * hide it when hovering the info button
  * auto update it whenever the store is updated

Aaron Lauterer (2):
  ui: ceph: make the warning detail button stand out more
  ui: ceph status: add tooltip with details to warnings

 www/manager6/ceph/Status.js | 123 ++++++++++++++++++++++++++++--------
 1 file changed, 97 insertions(+), 26 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 manager 1/2] ui: ceph: make the warning detail button stand out more

[Aaron Lauterer]

The button for more details is barely noticable as something one can
click on. By making it more obvious that it is a button, users will
hopefully notice it easier.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v1:

- use autoShow in the Ext.Create call

 www/manager6/ceph/Status.js | 56 +++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
index bdcf3f1b..b223ab35 100644
--- a/www/manager6/ceph/Status.js
+++ b/www/manager6/ceph/Status.js
@@ -101,37 +101,39 @@ Ext.define('PVE.node.CephStatus', {
 			    flex: 1,
 			},
 			{
-			    xtype: 'actioncolumn',
+			    xtype: 'widgetcolumn',
 			    width: 40,
 			    align: 'center',
 			    tooltip: gettext('Detail'),
-			    items: [
-				{
-				    iconCls: 'x-fa fa-info-circle',
-				    handler: function(grid, rowindex, colindex, item, e, record) {
-					var win = Ext.create('Ext.window.Window', {
-					    title: gettext('Detail'),
-					    resizable: true,
-					    modal: true,
-					    width: 650,
-					    height: 400,
-					    layout: {
-						type: 'fit',
-					    },
-					    items: [{
-						scrollable: true,
-						padding: 10,
-						xtype: 'box',
-						html: [
-						    '<span>' + Ext.htmlEncode(record.data.summary) + '</span>',
-						    '<pre>' + Ext.htmlEncode(record.data.detail) + '</pre>',
-						],
-					    }],
-					});
-					win.show();
-				    },
+			    widget: {
+				xtype: 'button',
+				baseCls: 'x-btn',
+				userCls: 'x-btn-default-toolbar-small',
+				iconCls: 'fa fa-fw fa-info-circle x-btn-icon-el-default-toolbar-small',
+				handler: function() {
+				    let record = this.getWidgetRecord();
+				    Ext.create('Ext.window.Window', {
+					title: gettext('Detail'),
+					autoShow: true,
+					resizable: true,
+					modal: true,
+					width: 650,
+					height: 400,
+					layout: {
+					    type: 'fit',
+					},
+					items: [{
+					    scrollable: true,
+					    padding: 10,
+					    xtype: 'box',
+					    html: [
+						`<span>${Ext.htmlEncode(record.data.summary)}</span>`,
+						`<pre>${Ext.htmlEncode(record.data.detail)}</pre>`,
+					    ],
+					}],
+				    });
 				},
-			    ],
+			    },
 			},
 		    ],
 		},
-- 
2.30.2

[pve-devel] [PATCH v2 manager 2/2] ui: ceph status: add tooltip with details to warnings

[Aaron Lauterer]

This is another step to make it easier for admins to discover more
information for a warning or problem that is shown in the Ceph health
panel.

The length is limited to give a first glimpse. For the full details one
can click on the info/detail button.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v1:
* only destroy it when leaving the whole grid, use show/hide for all
other situations
* factor out text handling of newlines and max lines
* factor out updating and destroying into their own functions
* add more listeners
* hide it when hovering the info button
* auto update it whenever the store is updated to either hide it
  (nothing under the mouse) or set the content

 www/manager6/ceph/Status.js | 69 +++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
index b223ab35..9de89df5 100644
--- a/www/manager6/ceph/Status.js
+++ b/www/manager6/ceph/Status.js
@@ -76,6 +76,66 @@ Ext.define('PVE.node.CephStatus', {
 			trackRemoved: false,
 			data: [],
 		    },
+		    generateTooltipText: (text) => {
+			text = text?.trimStart();
+			if (text.length > 500) {
+			    text = `${text.substring(0, 500)}…`;
+			}
+			return text.replaceAll('\n', '<br>');
+		    },
+		    updateTooltip: (view, isLeave) => {
+			if (!view.tooltip) {
+			    return;
+			}
+			if (view.store.data.length - 1 < view.tooltip.gridIndex || isLeave) {
+			    view.tooltip.hide();
+			} else {
+			    let data = view.store.getData().items[view.tooltip.gridIndex].data;
+			    if (!data.detail) {
+				return;
+			    }
+			    let text = view.up('#warnings').generateTooltipText(data.detail);
+			    view.tooltip.setData({ text });
+			}
+		    },
+		    destroyTooltip(view) {
+			view.tooltip?.destroy();
+			delete view.tooltip;
+		    },
+
+		    listeners: {
+			destroy: function() {
+			    let view = this.getView();
+			    this.destroyTooltip(view);
+			},
+			itemmouseenter: function(view, record, item, index) {
+			    if (!view) {
+				return;
+			    }
+			    if (!record.data.detail) {
+				return;
+			    }
+			    let text = this.generateTooltipText(record.data.detail);
+			    if (!view.tooltip) {
+				view.tooltip = Ext.create('Ext.tip.ToolTip', {
+				    target: view,
+				    trackMouse: true,
+				    dismissDelay: 0,
+				    tpl: '{text}',
+				    renderTo: Ext.getBody(),
+				});
+			    }
+			    view.tooltip.gridIndex = index;
+			    view.tooltip.setData({ text });
+			    view.tooltip.show();
+			},
+			itemmouseleave: function(view, record, item) {
+			    this.updateTooltip(view, true);
+			},
+			containermouseout: function(view) {
+			    this.destroyTooltip(view);
+			},
+		    },
 		    emptyText: gettext('No Warnings/Errors'),
 		    columns: [
 			{
@@ -133,6 +193,14 @@ Ext.define('PVE.node.CephStatus', {
 					}],
 				    });
 				},
+				listeners: {
+				    mouseover: (view) => {
+					view.up('#warnings').getView().tooltip.hide();
+				    },
+				    mouseout: (view) => {
+					view.up('#warnings').getView().tooltip.show();
+				    },
+				},
 			    },
 			},
 		    ],
@@ -286,6 +354,7 @@ Ext.define('PVE.node.CephStatus', {
 	me.down('#overallhealth').updateHealth(PVE.Utils.render_ceph_health(rec.data.health || {}));
 	// add errors to gridstore
 	me.down('#warnings').getStore().loadRawData(me.generateCheckData(rec.data.health || {}), false);
+	me.down('#warnings').updateTooltip(me.down('#warnings').getView());
 
 	// update services
 	me.getComponent('services').updateAll(me.metadata || {}, rec.data);
-- 
2.30.2

Re: [pve-devel] [PATCH v2 manager 2/2] ui: ceph status: add tooltip with details to warnings

[Dominik Csapak]

nice, works really good now with the different interactions
(button/container/changing store/etc)

some comments inline (mostly minor stuff though)

On 3/3/23 16:59, Aaron Lauterer wrote:
> This is another step to make it easier for admins to discover more
> information for a warning or problem that is shown in the Ceph health
> panel.
> 
> The length is limited to give a first glimpse. For the full details one
> can click on the info/detail button.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since v1:
> * only destroy it when leaving the whole grid, use show/hide for all
> other situations
> * factor out text handling of newlines and max lines
> * factor out updating and destroying into their own functions
> * add more listeners
> * hide it when hovering the info button
> * auto update it whenever the store is updated to either hide it
>    (nothing under the mouse) or set the content
> 
>   www/manager6/ceph/Status.js | 69 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
> 
> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
> index b223ab35..9de89df5 100644
> --- a/www/manager6/ceph/Status.js
> +++ b/www/manager6/ceph/Status.js
> @@ -76,6 +76,66 @@ Ext.define('PVE.node.CephStatus', {
>   			trackRemoved: false,
>   			data: [],
>   		    },
> +		    generateTooltipText: (text) => {
> +			text = text?.trimStart();

above you indicate that text might be undefined (with the 'text?.'),
but below you don't to any checks again

iff text can really be undefined, i'd do
text = text?.trimStart() ?? '';

otherwise just drop the '?'

> +			if (text.length > 500) {
> +			    text = `${text.substring(0, 500)}…`;
> +			}
> +			return text.replaceAll('\n', '<br>');
> +		    },
> +		    updateTooltip: (view, isLeave) => {
> +			if (!view.tooltip) {

i was rather confused at first what 'view' actually is, but
i found out it's the table view of the grid

couldn't you use always the grid ('this') instead ?
or is there some instance where 'this' isn't the grid?

i think that would make the code a bit more readable, since
you don't have to pass the view around

> +			    return;
> +			}
> +			if (view.store.data.length - 1 < view.tooltip.gridIndex || isLeave) {
> +			    view.tooltip.hide();
> +			} else {
> +			    let data = view.store.getData().items[view.tooltip.gridIndex].data;

this probably only works by chance, since the view *also* has the store
bound, normally we'd do 'me.getStore()' (which should work here with
'let me = this;' )

> +			    if (!data.detail) {
> +				return;
> +			    }
> +			    let text = view.up('#warnings').generateTooltipText(data.detail);

also this could just be 'me.generateTooltipText(...)', no?

> +			    view.tooltip.setData({ text });
> +			}
> +		    },
> +		    destroyTooltip(view) {
> +			view.tooltip?.destroy();
> +			delete view.tooltip;
> +		    },
> +
> +		    listeners: {
> +			destroy: function() {
> +			    let view = this.getView();
> +			    this.destroyTooltip(view);
> +			},
> +			itemmouseenter: function(view, record, item, index) {
> +			    if (!view) {
> +				return;
> +			    }
> +			    if (!record.data.detail) {
> +				return;
> +			    }
> +			    let text = this.generateTooltipText(record.data.detail);
> +			    if (!view.tooltip) {
> +				view.tooltip = Ext.create('Ext.tip.ToolTip', {
> +				    target: view,
> +				    trackMouse: true,
> +				    dismissDelay: 0,
> +				    tpl: '{text}',
> +				    renderTo: Ext.getBody(),
> +				});
> +			    }
> +			    view.tooltip.gridIndex = index;
> +			    view.tooltip.setData({ text });
> +			    view.tooltip.show();
> +			},
> +			itemmouseleave: function(view, record, item) {
> +			    this.updateTooltip(view, true);
> +			},
> +			containermouseout: function(view) {
> +			    this.destroyTooltip(view);
> +			},
> +		    },
>   		    emptyText: gettext('No Warnings/Errors'),
>   		    columns: [
>   			{
> @@ -133,6 +193,14 @@ Ext.define('PVE.node.CephStatus', {
>   					}],
>   				    });
>   				},
> +				listeners: {
> +				    mouseover: (view) => {
> +					view.up('#warnings').getView().tooltip.hide();

isn't here the 'view' actually the button?

> +				    },
> +				    mouseout: (view) => {
> +					view.up('#warnings').getView().tooltip.show();

same here?

> +				    },
> +				},
>   			    },
>   			},
>   		    ],
> @@ -286,6 +354,7 @@ Ext.define('PVE.node.CephStatus', {
>   	me.down('#overallhealth').updateHealth(PVE.Utils.render_ceph_health(rec.data.health || {}));
>   	// add errors to gridstore
>   	me.down('#warnings').getStore().loadRawData(me.generateCheckData(rec.data.health || {}), false);
> +	me.down('#warnings').updateTooltip(me.down('#warnings').getView());
>   
>   	// update services
>   	me.getComponent('services').updateAll(me.metadata || {}, rec.data);

Re: [pve-devel] [PATCH v2 manager 2/2] ui: ceph status: add tooltip with details to warnings

[Aaron Lauterer]

On 3/8/23 10:27, Dominik Csapak wrote:
> nice, works really good now with the different interactions
> (button/container/changing store/etc)
> 
> some comments inline (mostly minor stuff though)
> 
> On 3/3/23 16:59, Aaron Lauterer wrote:
[...]
>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>> index b223ab35..9de89df5 100644
>> --- a/www/manager6/ceph/Status.js
>> +++ b/www/manager6/ceph/Status.js
>> @@ -76,6 +76,66 @@ Ext.define('PVE.node.CephStatus', {
>>               trackRemoved: false,
>>               data: [],
>>               },
>> +            generateTooltipText: (text) => {
>> +            text = text?.trimStart();
> 
> above you indicate that text might be undefined (with the 'text?.'),
> but below you don't to any checks again
> 
> iff text can really be undefined, i'd do
> text = text?.trimStart() ?? '';
> 
> otherwise just drop the '?'


hmm yeah, AFAICT there are already checks in place before we even call 
generateTooltipText -> dropping the '?'

> 
>> +            if (text.length > 500) {
>> +                text = `${text.substring(0, 500)}…`;
>> +            }
>> +            return text.replaceAll('\n', '<br>');
>> +            },
>> +            updateTooltip: (view, isLeave) => {
>> +            if (!view.tooltip) {
> 
> i was rather confused at first what 'view' actually is, but
> i found out it's the table view of the grid
> 
> couldn't you use always the grid ('this') instead ?
> or is there some instance where 'this' isn't the grid?
> 
> i think that would make the code a bit more readable, since
> you don't have to pass the view around

as discussed off-list: arrow functions do not set 'this' in the same context -> 
switch to regular 'updateTooltip: function(..) {' instead of arrow functions and 
in other places too.
> 

[...]