public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status
@ 2024-11-29 10:41 Christian Ebner
  2024-11-29 10:41 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown Christian Ebner
  2025-02-25 16:17 ` [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status Thomas Lamprecht
  0 siblings, 2 replies; 6+ messages in thread
From: Christian Ebner @ 2024-11-29 10:41 UTC (permalink / raw)
  To: pbs-devel, pve-devel

Until now, the reported smart value is returned unconditionally, even
if the drive might report an `UNKNOWN` status.
To allow for better handling of the unknown smart state, also return
the utils helper text in that case. This allows for better handling
of e.g. conditionally showing the smart values window.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/panel/DiskList.js | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
index dc10ac5..dfd8c8e 100644
--- a/src/panel/DiskList.js
+++ b/src/panel/DiskList.js
@@ -7,7 +7,12 @@ Ext.define('pmx-disk-list', {
 	{
 	    name: 'status',
 	    convert: function(value, rec) {
-		if (value) return value;
+		if (value) {
+		    if (value.toLowerCase() === 'unknown') {
+			return Proxmox.Utils.unknownText;
+		    }
+		    return value;
+		}
 		if (rec.data.health) {
 		    return rec.data.health;
 		}
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown
  2024-11-29 10:41 [pbs-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status Christian Ebner
@ 2024-11-29 10:41 ` Christian Ebner
  2025-02-25 16:19   ` [pbs-devel] [pve-devel] " Thomas Lamprecht
  2025-02-25 16:17 ` [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status Thomas Lamprecht
  1 sibling, 1 reply; 6+ messages in thread
From: Christian Ebner @ 2024-11-29 10:41 UTC (permalink / raw)
  To: pbs-devel, pve-devel

Do not allow to open the smart values window by either double clicking
the record or clicking the show button, if the selected drives status
is unknown.

Fetching the smart values for such devices might fail. Devices which
do not support this can be, e.g. USB pen drives used as removable
datastores in Proxmox Backup Server.

Reported in the community forum:
https://forum.proxmox.com/threads/158217/

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/panel/DiskList.js | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
index dfd8c8e..3a1632c 100644
--- a/src/panel/DiskList.js
+++ b/src/panel/DiskList.js
@@ -86,6 +86,9 @@ Ext.define('Proxmox.DiskList', {
 	    if (!selection || selection.length < 1) return;
 
 	    let rec = selection[0];
+	    if (!rec.data.status || rec.data.status === Proxmox.Utils.unknownText) {
+		return;
+	    }
 	    Ext.create('Proxmox.window.DiskSmart', {
 		baseurl: view.baseurl,
 		dev: rec.data.name,
@@ -369,7 +372,8 @@ Ext.define('Proxmox.DiskList', {
 		parentXType: 'treepanel',
 		disabled: true,
 		enableFn: function(rec) {
-		    if (!rec || rec.data.parent) {
+		    if (!rec || rec.data.parent || !rec.data.status ||
+			rec.data.status === Proxmox.Utils.unknownText) {
 			return false;
 		    } else {
 			return true;
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status
  2024-11-29 10:41 [pbs-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status Christian Ebner
  2024-11-29 10:41 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown Christian Ebner
@ 2025-02-25 16:17 ` Thomas Lamprecht
  1 sibling, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2025-02-25 16:17 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christian Ebner, pbs-devel

Am 29.11.24 um 11:41 schrieb Christian Ebner:
> Until now, the reported smart value is returned unconditionally, even
> if the drive might report an `UNKNOWN` status.
> To allow for better handling of the unknown smart state, also return
> the utils helper text in that case. This allows for better handling
> of e.g. conditionally showing the smart values window.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/panel/DiskList.js | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
> index dc10ac5..dfd8c8e 100644
> --- a/src/panel/DiskList.js
> +++ b/src/panel/DiskList.js
> @@ -7,7 +7,12 @@ Ext.define('pmx-disk-list', {
>  	{
>  	    name: 'status',
>  	    convert: function(value, rec) {
> -		if (value) return value;
> +		if (value) {
> +		    if (value.toLowerCase() === 'unknown') {
> +			return Proxmox.Utils.unknownText;
> +		    }

hmm, using translated strings for internal state is not fully ideal IMO.
Maybe just normalize it here to lowercase and place a renderer on where
it matters?

> +		    return value;
> +		}
>  		if (rec.data.health) {
>  		    return rec.data.health;
>  		}



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown
  2024-11-29 10:41 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown Christian Ebner
@ 2025-02-25 16:19   ` Thomas Lamprecht
  2025-02-25 16:22     ` Christian Ebner
  0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2025-02-25 16:19 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christian Ebner, pbs-devel

Am 29.11.24 um 11:41 schrieb Christian Ebner:
> Do not allow to open the smart values window by either double clicking
> the record or clicking the show button, if the selected drives status
> is unknown.
> 
> Fetching the smart values for such devices might fail. Devices which
> do not support this can be, e.g. USB pen drives used as removable
> datastores in Proxmox Backup Server.
> 
> Reported in the community forum:
> https://forum.proxmox.com/threads/158217/
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/panel/DiskList.js | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
> index dfd8c8e..3a1632c 100644
> --- a/src/panel/DiskList.js
> +++ b/src/panel/DiskList.js
> @@ -86,6 +86,9 @@ Ext.define('Proxmox.DiskList', {
>  	    if (!selection || selection.length < 1) return;
>  
>  	    let rec = selection[0];
> +	    if (!rec.data.status || rec.data.status === Proxmox.Utils.unknownText) {
> +		return;
> +	    }
>  	    Ext.create('Proxmox.window.DiskSmart', {
>  		baseurl: view.baseurl,
>  		dev: rec.data.name,
> @@ -369,7 +372,8 @@ Ext.define('Proxmox.DiskList', {
>  		parentXType: 'treepanel',
>  		disabled: true,
>  		enableFn: function(rec) {
> -		    if (!rec || rec.data.parent) {
> +		    if (!rec || rec.data.parent || !rec.data.status ||
> +			rec.data.status === Proxmox.Utils.unknownText) {
>  			return false;
>  		    } else {
>  			return true;

pre-existing but an if-else that returns boolean seldomly makes sense,
i.e. this could be:

enableFn: rec => rec && !rec.data.parent && rec.data.status && rec.data.status !== Proxmox.Utils.unknownText,

or with my comment for patch 1 addressed it might be:

enableFn: rec => rec && !rec.data.parent && rec.data.status && rec.data.status !== 'unknown',




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown
  2025-02-25 16:19   ` [pbs-devel] [pve-devel] " Thomas Lamprecht
@ 2025-02-25 16:22     ` Christian Ebner
  2025-02-25 16:41       ` Thomas Lamprecht
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Ebner @ 2025-02-25 16:22 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, pbs-devel

On 2/25/25 17:19, Thomas Lamprecht wrote:
> Am 29.11.24 um 11:41 schrieb Christian Ebner:
>> Do not allow to open the smart values window by either double clicking
>> the record or clicking the show button, if the selected drives status
>> is unknown.
>>
>> Fetching the smart values for such devices might fail. Devices which
>> do not support this can be, e.g. USB pen drives used as removable
>> datastores in Proxmox Backup Server.
>>
>> Reported in the community forum:
>> https://forum.proxmox.com/threads/158217/
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   src/panel/DiskList.js | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
>> index dfd8c8e..3a1632c 100644
>> --- a/src/panel/DiskList.js
>> +++ b/src/panel/DiskList.js
>> @@ -86,6 +86,9 @@ Ext.define('Proxmox.DiskList', {
>>   	    if (!selection || selection.length < 1) return;
>>   
>>   	    let rec = selection[0];
>> +	    if (!rec.data.status || rec.data.status === Proxmox.Utils.unknownText) {
>> +		return;
>> +	    }
>>   	    Ext.create('Proxmox.window.DiskSmart', {
>>   		baseurl: view.baseurl,
>>   		dev: rec.data.name,
>> @@ -369,7 +372,8 @@ Ext.define('Proxmox.DiskList', {
>>   		parentXType: 'treepanel',
>>   		disabled: true,
>>   		enableFn: function(rec) {
>> -		    if (!rec || rec.data.parent) {
>> +		    if (!rec || rec.data.parent || !rec.data.status ||
>> +			rec.data.status === Proxmox.Utils.unknownText) {
>>   			return false;
>>   		    } else {
>>   			return true;
> 
> pre-existing but an if-else that returns boolean seldomly makes sense,
> i.e. this could be:
> 
> enableFn: rec => rec && !rec.data.parent && rec.data.status && rec.data.status !== Proxmox.Utils.unknownText,
> 
> or with my comment for patch 1 addressed it might be:
> 
> enableFn: rec => rec && !rec.data.parent && rec.data.status && rec.data.status !== 'unknown',
> 
> 
Acked, will incorporate the suggestions for both patches in a v2, thanks 
for feedback!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown
  2025-02-25 16:22     ` Christian Ebner
@ 2025-02-25 16:41       ` Thomas Lamprecht
  0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2025-02-25 16:41 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christian Ebner, pbs-devel

Am 25.02.25 um 17:22 schrieb Christian Ebner:
> Acked, will incorporate the suggestions for both patches in a v2, thanks 
> for feedback!

btw before I forget it: This is some nice UX fine-polishing, but IMO the smart
status panel is not really ideal as main disk panel that opens on double click
and so on.

I'd rather transform this to open a generic disk, or partition, information
window showing things like model, serial, (part)uuid, in a main tab and make
that easily copyable. The smart data could be moved to a second tab in such
a window and only loaded when that opened. We could then mask its grid there
if the smart status is unknown or hide it completely if it cannot be queried,
like for partitions.
Some bigger changes but IMO quite a bit better UX than the status quo, which
feels rather confusing.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-02-25 16:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-29 10:41 [pbs-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status Christian Ebner
2024-11-29 10:41 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] disk list: disable show smart values button if status unknown Christian Ebner
2025-02-25 16:19   ` [pbs-devel] [pve-devel] " Thomas Lamprecht
2025-02-25 16:22     ` Christian Ebner
2025-02-25 16:41       ` Thomas Lamprecht
2025-02-25 16:17 ` [pbs-devel] [pve-devel] [PATCH proxmox-widget-toolkit 1/2] panel: disk list: return consistent value for unknown smart status Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal