Add Model support for TreeItemSelector control source data #2023
Conversation
| { | ||
| parent::setModel($model); | ||
|
|
||
| $nodes_array = (clone $this->model)->export(); |
There was a problem hiding this comment.
Why clone? Why to materialize/export here?
There was a problem hiding this comment.
Materialize is faster, as it does not involved a new DB query for every node. So up to you what is better - fix to use model only below.
| $newNode['name'] = $node->getTitle(); | ||
| $newNode['id'] = $node->getId(); | ||
| $newNode['parent_id'] = $node->get($this->parentIdField); | ||
| $newNode['nodes'] = $this->addNodes(clone $model, $node->getId()); |
There was a problem hiding this comment.
Because we have to set a new parentId condition on an unconditioned model, otherwise will have contradictory conditions
There was a problem hiding this comment.
Correct myself - this clone is indeed not needed, as we clone earlier into nodeModel already. Correct in latest commit
| $nodeModel = (clone $model)->addCondition($this->parentIdField, $parentId); | ||
|
|
||
| foreach ($nodeModel as $node) { | ||
| if ($node->get($this->parentIdField) === $parentId) { |
There was a problem hiding this comment.
Because we have to set a new parentId condition on an unconditioned model, otherwise will have contradictory conditions
|
Can you check this PR please - I'd like to get the model support for TreeItemSelector pushed into PR. Desperately needed. Thank you |
mvorisek
changed the title
|
|
||
| $nodeModel = (clone $model)->addCondition($this->parentIdField, $parentId); | ||
|
|
||
| foreach ($nodeModel as $node) { |
There was a problem hiding this comment.
This implies a DB query (Model::getIterator() which implies SELECT). Recursively.
Single query recursive data fetch can be hard to implement and will require RECURSIVE CTE.
A good enough compromise might be to fetch +1 tree level collectively at once instead of 1 leaf at each time.
There was a problem hiding this comment.
@mvorisek Recursive CTE is an overkill for such a simple task. Or shall I import the whole tree definition table into an array, and do the recursive operation inside the array? That would speed it up as well, and we talk about reasonably sized trees typically (otherwise we would need another treeview component which dynamically loads sub-trees upon click, there are only few Vue components doing this beautifully). Materializing the whole table into an array, I did before to speed up the script, but you did not like it.
There was a problem hiding this comment.
It it obvious both extremes - current dummy impl. and whole table materialization is not a solution. The later case can produce OOM error easily, imagine the perf. on table with millions of records. It would be good to impl. recursive iterator in atk4/data Model and use it here.
There was a problem hiding this comment.
I think your worries are not realistic, as models for this TreeItemSelector are never tables of million entries. And if they are TreeItem is the wrong control as written before: you would need a Tree Control which dynamically loads sub-trees on clicks. As said, Recursive CTE is an overkill for this simple control, unless you see yourself in a position to implement this soon. Even with recursive CTE, the control would not load properly in time with your million-entries example. Otherwise I suggest we go for the faster export variant. Working well in real life with a few hundreds entries.
There was a problem hiding this comment.
Michael, let's focus on the solution and please reread my posts above. I did not written CTE is needed, I have written the current demo is awfully slow, which is a fact and I hope we both understand the PR cannot be merged as is.
I can also tell that table with millions of records is a realistic usecase, not for display, but imagine any tree features per some item like product. No product will have a lot of features, but the whole table can be huge. And as long as you cannot resolve the "minimal spanning tree" in one query, you cannot fetch the data using a single query.
The current implementation of TreeItemSelector requires a static array of nodes to display a tree. However, TreeItemSelector should also be usable with tree nodes in an
Atk4/Model.The included implementation offers an easy way to use any table which has tree items and references to a parent tree item id to build a dynamic tree.
It includes also an extended demo to show how it is being used.
Given the usage of model data, this will also be a prerequisite to understand the typecasting issue stated in #2022