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

[pve-devel] [PATCH 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

This will hopefully reduce the time spent on figuring out what exactly
is wrong and how one can fix it.

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 | 97 ++++++++++++++++++++++++++-----------
 1 file changed, 70 insertions(+), 27 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH 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>
---

While moving code around I also changed:
- var -> let
- string concat to template literals

 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..45583f51 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();
+				    let 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();
 				},
-			    ],
+			    },
 			},
 		    ],
 		},
-- 
2.30.2

[pve-devel] [PATCH 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>
---
 www/manager6/ceph/Status.js | 41 +++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
index 45583f51..19a1243a 100644
--- a/www/manager6/ceph/Status.js
+++ b/www/manager6/ceph/Status.js
@@ -76,6 +76,47 @@ Ext.define('PVE.node.CephStatus', {
 			trackRemoved: false,
 			data: [],
 		    },
+
+		    listeners: {
+			destroy: function() {
+			    let view = this.getView();
+			    if (view.tooltip) {
+				view.tooltip.destroy();
+				delete view.tooltip;
+			    }
+			},
+			itemmouseenter: function(record, item) {
+			    let view = this.getView();
+			    if (!view) {
+				return;
+			    }
+			    if (!item.data.detail) {
+				return;
+			    }
+			    let text = item.data.detail.trimStart().replaceAll('\n', '<br>');
+			    if (text.length > 500) {
+			        text = `${text.substring(0, 400)}…`;
+			    }
+			    if (!view.tooltip) {
+				view.tooltip = Ext.create('Ext.tip.ToolTip', {
+				    target: view,
+				    trackMouse: true,
+				    dismissDelay: 0,
+				    tpl: '{text}',
+				    renderTo: Ext.getBody(),
+				});
+			    }
+			    view.tooltip.setData({ text });
+			    view.tooltip.show();
+			},
+			itemmouseleave: function(record, item) {
+			    let view = this.getView();
+			    if (view.tooltip) {
+				view.tooltip.destroy();
+				delete view.tooltip;
+			    }
+			},
+		    },
 		    emptyText: gettext('No Warnings/Errors'),
 		    columns: [
 			{
-- 
2.30.2

[pve-devel] [PATCH manager follow-up 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>
---
Please use this follow up!

I found a small bug just after sending it regarding the max length.
The length check and substring did have different values.

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

diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
index 45583f51..19a1243a 100644
--- a/www/manager6/ceph/Status.js
+++ b/www/manager6/ceph/Status.js
@@ -76,6 +76,47 @@ Ext.define('PVE.node.CephStatus', {
 			trackRemoved: false,
 			data: [],
 		    },
+
+		    listeners: {
+			destroy: function() {
+			    let view = this.getView();
+			    if (view.tooltip) {
+				view.tooltip.destroy();
+				delete view.tooltip;
+			    }
+			},
+			itemmouseenter: function(record, item) {
+			    let view = this.getView();
+			    if (!view) {
+				return;
+			    }
+			    if (!item.data.detail) {
+				return;
+			    }
+			    let text = item.data.detail.trimStart().replaceAll('\n', '<br>');
+			    if (text.length > 500) {
+			        text = `${text.substring(0, 500)}…`;
+			    }
+			    if (!view.tooltip) {
+				view.tooltip = Ext.create('Ext.tip.ToolTip', {
+				    target: view,
+				    trackMouse: true,
+				    dismissDelay: 0,
+				    tpl: '{text}',
+				    renderTo: Ext.getBody(),
+				});
+			    }
+			    view.tooltip.setData({ text });
+			    view.tooltip.show();
+			},
+			itemmouseleave: function(record, item) {
+			    let view = this.getView();
+			    if (view.tooltip) {
+				view.tooltip.destroy();
+				delete view.tooltip;
+			    }
+			},
+		    },
 		    emptyText: gettext('No Warnings/Errors'),
 		    columns: [
 			{
-- 
2.30.2

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

[Dominik Csapak]

one nit inline:

On 2/22/23 10:36, Aaron Lauterer wrote:
> 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>
> ---
> 
> While moving code around I also changed:
> - var -> let
> - string concat to template literals
> 
>   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..45583f51 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();
> +				    let 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();

when touching this, you could add 'autoShow: true' to the properties,
then we don't have to save the window in a variable at all (and don't call .show())

not a reason to send a v2 though

>   				},
> -			    ],
> +			    },
>   			},
>   		    ],
>   		},

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

[Dominik Csapak]

one bug did occur when trying it out:

if i had my mouse over a warning, and then the warnings vanished all,
the tooltip was still there and i could trigger it with hovering over
the grid

so we want to clean the tooltips up on every load anyway,
or at least when the number of items is smaller than the position
my mouse is over ;)

some comments inline

On 2/22/23 10:39, 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>
> ---
> Please use this follow up!
> 
> I found a small bug just after sending it regarding the max length.
> The length check and substring did have different values.
> 
>   www/manager6/ceph/Status.js | 41 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
> 
> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
> index 45583f51..19a1243a 100644
> --- a/www/manager6/ceph/Status.js
> +++ b/www/manager6/ceph/Status.js
> @@ -76,6 +76,47 @@ Ext.define('PVE.node.CephStatus', {
>   			trackRemoved: false,
>   			data: [],
>   		    },
> +
> +		    listeners: {
> +			destroy: function() {
> +			    let view = this.getView();
> +			    if (view.tooltip) {
> +				view.tooltip.destroy();
> +				delete view.tooltip;
> +			    }

you could also do :

view.tooltip?.destroy();
delete view.tooltip;

without the if

also the function is the same on itemmouseleave and destroy, so please
refactor that into a function
(since we also have to use it elsewhere too, see my comment at the top)

> +			},
> +			itemmouseenter: function(record, item) {
> +			    let view = this.getView();
> +			    if (!view) {
> +				return;
> +			    }
> +			    if (!item.data.detail) {
> +				return;
> +			    }
> +			    let text = item.data.detail.trimStart().replaceAll('\n', '<br>');

any reason why we don't also use the <pre> tag here like in the popup?
would at least be less computational intensive for long errors

also i'd check the length before replacing the newlines, otherwise
it can happen that you cut the text of in the middle of a <br> tag ;)


> +			    if (text.length > 500) {
> +			        text = `${text.substring(0, 500)}…`;

nit: any special reason for the ellipsis glyph instead of 3 dots?
not that it's a problem, just want to know

> +			    }
> +			    if (!view.tooltip) {
> +				view.tooltip = Ext.create('Ext.tip.ToolTip', {
> +				    target: view,
> +				    trackMouse: true,
> +				    dismissDelay: 0,
> +				    tpl: '{text}',
> +				    renderTo: Ext.getBody(),
> +				});
> +			    }
> +			    view.tooltip.setData({ text });
> +			    view.tooltip.show();
> +			},
> +			itemmouseleave: function(record, item) {
> +			    let view = this.getView();
> +			    if (view.tooltip) {
> +				view.tooltip.destroy();
> +				delete view.tooltip;
> +			    }
> +			},
> +		    },
>   		    emptyText: gettext('No Warnings/Errors'),
>   		    columns: [
>   			{

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

[Aaron Lauterer]

On 3/2/23 14:15, Dominik Csapak wrote:
> one bug did occur when trying it out:
> 
> if i had my mouse over a warning, and then the warnings vanished all,
> the tooltip was still there and i could trigger it with hovering over
> the grid
> 
> so we want to clean the tooltips up on every load anyway,

Wouldn't it cause the tooltip to disappear, or at least flicker?

> or at least when the number of items is smaller than the position
> my mouse is over ;)
Yep

> 
> some comments inline
> 
> On 2/22/23 10:39, 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>
>> ---
>> Please use this follow up!
>>
>> I found a small bug just after sending it regarding the max length.
>> The length check and substring did have different values.
>>
>>   www/manager6/ceph/Status.js | 41 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>> index 45583f51..19a1243a 100644
>> --- a/www/manager6/ceph/Status.js
>> +++ b/www/manager6/ceph/Status.js
>> @@ -76,6 +76,47 @@ Ext.define('PVE.node.CephStatus', {
>>               trackRemoved: false,
>>               data: [],
>>               },
>> +
>> +            listeners: {
>> +            destroy: function() {
>> +                let view = this.getView();
>> +                if (view.tooltip) {
>> +                view.tooltip.destroy();
>> +                delete view.tooltip;
>> +                }
> 
> you could also do :
> 
> view.tooltip?.destroy();
> delete view.tooltip;
> 
> without the if
> 
> also the function is the same on itemmouseleave and destroy, so please
> refactor that into a function
> (since we also have to use it elsewhere too, see my comment at the top)

Okay, will do.

> 
>> +            },
>> +            itemmouseenter: function(record, item) {
>> +                let view = this.getView();
>> +                if (!view) {
>> +                return;
>> +                }
>> +                if (!item.data.detail) {
>> +                return;
>> +                }
>> +                let text = item.data.detail.trimStart().replaceAll('\n', 
>> '<br>');
> 
> any reason why we don't also use the <pre> tag here like in the popup?
> would at least be less computational intensive for long errors

I did try it with the pre tag, the problem then is, that the tooltip has a max 
length and the rest of the line is hidden. but that part contains important 
information as well. So I opted for a line break. Visibly not as pleasing, but 
it shows the full line.

> 
> also i'd check the length before replacing the newlines, otherwise
> it can happen that you cut the text of in the middle of a <br> tag ;)

Okay.

> 
> 
>> +                if (text.length > 500) {
>> +                    text = `${text.substring(0, 500)}…`;
> 
> nit: any special reason for the ellipsis glyph instead of 3 dots?
> not that it's a problem, just want to know

Because the ellipsis is the thing to use in such a situation? It is possible 
that my understanding is wrong. I have no problem with 3 dots as well.

> 
>> +                }
>> +                if (!view.tooltip) {
>> +                view.tooltip = Ext.create('Ext.tip.ToolTip', {
>> +                    target: view,
>> +                    trackMouse: true,
>> +                    dismissDelay: 0,
>> +                    tpl: '{text}',
>> +                    renderTo: Ext.getBody(),
>> +                });
>> +                }
>> +                view.tooltip.setData({ text });
>> +                view.tooltip.show();
>> +            },
>> +            itemmouseleave: function(record, item) {
>> +                let view = this.getView();
>> +                if (view.tooltip) {
>> +                view.tooltip.destroy();
>> +                delete view.tooltip;
>> +                }
>> +            },
>> +            },
>>               emptyText: gettext('No Warnings/Errors'),
>>               columns: [
>>               {
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel