Build NodeTree on demand #583
Conversation
- do not build tree recursively, only show the top-level - build the tree when users select it
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #583 +/- ##
=======================================
Coverage 96.17% 96.17%
=======================================
Files 11 11
Lines 1177 1177
=======================================
Hits 1132 1132
Misses 45 45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@danielhollas This PR is also relevant to your app; please review it if you have time. |
|
@giovannipizzi This PR tried to fix one of the two issues from the Node Tree widget, which we discussed in yesterday's meeting. Comments are welcome. |
|
@superstar54 this is amazing, thanks so much on working on this. 👏 I badly need it for my app as well so I will definitely take a look and test it with my app, hopefully tomorrow. It's nice to see that there was not more code needed for this; I was a bit afraid that it will be more difficult to implement. |
|
That's fantastic, great! If Daniel checks it it would be great and then please merge it! Pinging @cpignedoli as well. As a note (on existing code, not what you changed): is |
| else getattr(node, "pk", None) | ||
| ) | ||
|
|
||
| self._build_tree(self.find_node(node_pk, getattr(node, "namespaces", None))) |
There was a problem hiding this comment.
From just looking at this code, I wonder how is it ensured that the tree is build only once? i.e. if a user selects / deselects the same node multiple times, is it going to trigger the build_tree multiple times or will the existing nodes be reused somehow?
There was a problem hiding this comment.
Thanks for the review. It's designed to build the tree every time the user selects it, for two reasons:
- if the job is running, it will ensure the user sees the latest node tree.
- since it only builds the first level of the child nodes, it should be quick even for a large number of child nodes.
but, yes, I will check if the already loaded child nodes will be reused or not.
There was a problem hiding this comment.
Hi @danielhollas , the already loaded child nodes will be reused when running the _build_tree.
👍 It's a typo. I fixed it. |
|
@superstar54 I noticed the change in color of the WorkChains with finished[0] from green to gray, |
|
@superstar54 this is great. I tested on my docker for something small just to confirm it works. In addiiton, I checked with @AndresOrtegaGuerrero on a large Raman case, this PR seems to solve our problems. |
Hi @AndresOrtegaGuerrero , thanks for pointing out this. it should be fixed in 795af3f. |
Indeed now is fixed!, thank is great job |
Fix #584
This PR introduces the idea that: only build NodeTree on demand.
Demo
Here I used the result of the IR/Raman plugin (from @AndresOrtegaGuerrero ) as a example. Before, it's almost impossible to load it in the NodeTree.
Using this PR:
qeapp-node-tree.mp4