[pve-devel] [PATCH manager v2 1/2] ui: resource tree: use 'diskuse' instead of calculating everytime

[pve-devel] [PATCH manager v2 1/2] ui: resource tree: use 'diskuse' instead of calculating everytime

[Dominik Csapak]

the resource store has a field 'diskuse' which it calculates on update.
Use that instead of calculating the value ourselves everytime.

For change detection, we only need a resolution of 0.01 (since we want
to use the percentage as integer) so check that the difference of old and new
is bigger than 0.9% .

This works because we only overwrite the values in the treestore if
anything changed, so multiple small changes to the diskuse will not be
lost.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* improve commmit message to note why many small changes are not lost
* use `>= 0.01` instead of `> 0.009`

 www/manager6/tree/ResourceTree.js | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index bb016f8c..b0e094f1 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -57,7 +57,7 @@ Ext.define('PVE.tree.ResourceTree', {
                 let text = info.text;
                 let status = '';
                 if (info.type === 'storage') {
-                    let usage = info.disk / info.maxdisk;
+                    let usage = info.diskuse;
                     if (usage >= 0.0 && usage <= 1.0) {
                         let barHeight = (usage * 100).toFixed(0);
                         let remainingHeight = (100 - barHeight).toFixed(0);
@@ -186,7 +186,7 @@ Ext.define('PVE.tree.ResourceTree', {
             qtips.push(Ext.String.format(gettext('HA State: {0}'), info.hastate));
         }
         if (info.type === 'storage') {
-            let usage = info.disk / info.maxdisk;
+            let usage = info.diskuse;
             if (usage >= 0.0 && usage <= 1.0) {
                 qtips.push(Ext.String.format(gettext('Usage: {0}%'), (usage * 100).toFixed(2)));
             }
@@ -310,8 +310,6 @@ Ext.define('PVE.tree.ResourceTree', {
         let stateid = 'rid';
 
         const changedFields = [
-            'disk',
-            'maxdisk',
             'vmid',
             'name',
             'type',
@@ -409,14 +407,25 @@ Ext.define('PVE.tree.ResourceTree', {
                         }
                     }
 
-                    // tree item has been updated
-                    for (const field of changedFields) {
-                        if (item.data[field] !== olditem.data[field]) {
+                    let diskuse = item.data.diskuse;
+                    let oldDiskuse = olditem.data.diskuse;
+
+                    if (diskuse !== undefined || oldDiskuse !== undefined) {
+                        if (Math.abs(diskuse - oldDiskuse) >= 0.01) {
                             changed = true;
-                            break;
                         }
                     }
-                    // FIXME: also test filterfn()?
+
+                    if (!changed) {
+                        // tree item has been updated
+                        for (const field of changedFields) {
+                            if (item.data[field] !== olditem.data[field]) {
+                                changed = true;
+                                break;
+                            }
+                        }
+                        // FIXME: also test filterfn()?
+                    }
                 }
 
                 if (changed) {
-- 
2.47.3



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

[pve-devel] [PATCH manager v2 2/2] ui: resource tree: only fire 'refresh' event when something changed

[Dominik Csapak]

to optimize the rendering to the dom, only fire the 'refresh' event of
the store at the end of the updateTree method when either:

* an element changed it's relevant data
* something moved
* a new element was inserted
* an element was removed

We also need to refresh the store when the UI options are reloaded, so
the tags get the correct color.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v2

 www/manager6/tree/ResourceTree.js | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
index b0e094f1..8612269b 100644
--- a/www/manager6/tree/ResourceTree.js
+++ b/www/manager6/tree/ResourceTree.js
@@ -354,6 +354,7 @@ Ext.define('PVE.tree.ResourceTree', {
         let updateTree = function () {
             store.suspendEvents();
 
+            let any_changed = false;
             let rootnode;
             if (firstUpdate) {
                 rootnode = Ext.create('PVETree', {
@@ -437,6 +438,7 @@ Ext.define('PVE.tree.ResourceTree', {
                     }
                     me.setIconCls(info);
                     olditem.commit();
+                    any_changed = true;
                 }
                 if ((!item || moved) && olditem.isLeaf()) {
                     delete index[key];
@@ -454,6 +456,8 @@ Ext.define('PVE.tree.ResourceTree', {
                         let grandParent = parentNode.parentNode;
                         grandParent.removeChild(parentNode, true);
                     }
+
+                    any_changed = true;
                 }
             }
 
@@ -473,10 +477,13 @@ Ext.define('PVE.tree.ResourceTree', {
                 if (child) {
                     index[item.data.id] = child;
                 }
+                any_changed = true;
             });
 
             store.resumeEvents();
-            store.fireEvent('refresh', store);
+            if (any_changed) {
+                store.fireEvent('refresh', store);
+            }
 
             let foundChild = findNode(rootnode, lastsel?.data.id);
 
@@ -632,6 +639,9 @@ Ext.define('PVE.tree.ResourceTree', {
                     return true;
                 },
             });
+
+            // rerender
+            me.store.fireEvent('refresh', store);
         });
     },
 });
-- 
2.47.3



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

Re: [pve-devel] [PATCH manager v2 1/2] ui: resource tree: use 'diskuse' instead of calculating everytime

[Dominik Csapak]

gentle ping for both patches.
They should still apply and greatly reduce the dom changes here on my
machine, and makes inspecting element on the tree much easier ;)


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

Re: [pve-devel] [PATCH manager v2 1/2] ui: resource tree: use 'diskuse' instead of calculating everytime

[Fiona Ebner]

Am 12.12.25 um 8:44 AM schrieb Dominik Csapak:
> @@ -409,14 +407,25 @@ Ext.define('PVE.tree.ResourceTree', {
>                          }
>                      }
>  
> -                    // tree item has been updated
> -                    for (const field of changedFields) {
> -                        if (item.data[field] !== olditem.data[field]) {
> +                    let diskuse = item.data.diskuse;
> +                    let oldDiskuse = olditem.data.diskuse;
> +
> +                    if (diskuse !== undefined || oldDiskuse !== undefined) {

Should this be a '&&' here? Otherwise, the patches look good to me. Can
fix up on applying.

> +                        if (Math.abs(diskuse - oldDiskuse) >= 0.01) {
>                              changed = true;
> -                            break;
>                          }
>                      }
> -                    // FIXME: also test filterfn()?
> +
> +                    if (!changed) {
> +                        // tree item has been updated
> +                        for (const field of changedFields) {
> +                            if (item.data[field] !== olditem.data[field]) {
> +                                changed = true;
> +                                break;
> +                            }
> +                        }
> +                        // FIXME: also test filterfn()?
> +                    }
>                  }
>  
>                  if (changed) {



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

Re: [pve-devel] [PATCH manager v2 1/2] ui: resource tree: use 'diskuse' instead of calculating everytime

[Thomas Lamprecht]

Am 12.12.25 um 08:44 schrieb Dominik Csapak:
> the resource store has a field 'diskuse' which it calculates on update.
> Use that instead of calculating the value ourselves everytime.
> 
> For change detection, we only need a resolution of 0.01 (since we want
> to use the percentage as integer) so check that the difference of old and new
> is bigger than 0.9% .
> 
> This works because we only overwrite the values in the treestore if
> anything changed, so multiple small changes to the diskuse will not be
> lost.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * improve commmit message to note why many small changes are not lost
> * use `>= 0.01` instead of `> 0.009`
> 
>  www/manager6/tree/ResourceTree.js | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
> index bb016f8c..b0e094f1 100644
> --- a/www/manager6/tree/ResourceTree.js
> +++ b/www/manager6/tree/ResourceTree.js

> @@ -409,14 +407,25 @@ Ext.define('PVE.tree.ResourceTree', {
>                          }
>                      }
>  
> -                    // tree item has been updated
> -                    for (const field of changedFields) {
> -                        if (item.data[field] !== olditem.data[field]) {
> +                    let diskuse = item.data.diskuse;

While the API is hard to change, we could still use a bit more telling
variable names, like diskUsage and oldDiskUsage. Just a not, and if I would not
had noticed below tiny potential bug, I might have just fixed this up locally.

> +                    let oldDiskuse = olditem.data.diskuse;
> +
> +                    if (diskuse !== undefined || oldDiskuse !== undefined) {
> +                        if (Math.abs(diskuse - oldDiskuse) >= 0.01) {

this is never true vor the case where one is defined and the other
value is undefined though, as the result then is NaN. But, when old is
undefined and new isn't we probably always want to trigger a change event?

Might be enough to add a extra branch like:

} else if (typeof diskUsage !== typeof oldDiskUsage) {
    changed = true;
}

>                              changed = true;
> -                            break;
>                          }
>                      }
> -                    // FIXME: also test filterfn()?
> +
> +                    if (!changed) {
> +                        // tree item has been updated
> +                        for (const field of changedFields) {
> +                            if (item.data[field] !== olditem.data[field]) {
> +                                changed = true;
> +                                break;
> +                            }
> +                        }
> +                        // FIXME: also test filterfn()?
> +                    }
>                  }
>  
>                  if (changed) {



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

Re: [pve-devel] [PATCH manager v2 1/2] ui: resource tree: use 'diskuse' instead of calculating everytime

[Dominik Csapak]

thanks to both for looking at this.
i'll check this more in depth tomorrow and will send a v3 then


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

Re: [pve-devel] [PATCH manager v2 2/2] ui: resource tree: only fire 'refresh' event when something changed

[Thomas Lamprecht]

Am 12.12.25 um 08:44 schrieb Dominik Csapak:
> to optimize the rendering to the dom, only fire the 'refresh' event of
> the store at the end of the updateTree method when either:
> 
> * an element changed it's relevant data
> * something moved
> * a new element was inserted
> * an element was removed
> 
> We also need to refresh the store when the UI options are reloaded, so
> the tags get the correct color.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> no changes in v2
> 
>  www/manager6/tree/ResourceTree.js | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/tree/ResourceTree.js b/www/manager6/tree/ResourceTree.js
> index b0e094f1..8612269b 100644
> --- a/www/manager6/tree/ResourceTree.js
> +++ b/www/manager6/tree/ResourceTree.js
> @@ -354,6 +354,7 @@ Ext.define('PVE.tree.ResourceTree', {
>          let updateTree = function () {
>              store.suspendEvents();
>  
> +            let any_changed = false;

existing style in this module is already rather mixed, so really not a big style issue,
but would be still nice to prefer camelCase as per our [JS style guide]. Also here,
would probably fixed that just up, but due to replying to patch 1/2 anyway I figured
it doesn't hurt to notice this nit.

JS style guide: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

>              let rootnode;
>              if (firstUpdate) {
>                  rootnode = Ext.create('PVETree', {



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