mbraak / jqTree

Tree widget for jQuery
https://mbraak.github.io/jqTree/
Apache License 2.0
1.02k stars 177 forks source link

appendNode shouldn't be so expensive #44

Closed rosenfeld closed 9 years ago

rosenfeld commented 12 years ago

my onCreateLi is an expensive operation. In some cases it will require some additional calls to the server and create dialogs, etc.

So, I have some template function for populating it li, so that it looks like this:

onCreateLi: (node, $li)-> $li.find('.title').html renderTemplate(node)

the renderTemplate will make some AJAX calls to fill a hidden dialog returned by renderTemplate with items from this call.

Something like:

some content here

The problem is that appendNode will remove all nodes and create all of them again on each call:

https://github.com/mbraak/jqTree/blob/dev/tree.jquery.coffee#L688 https://github.com/mbraak/jqTree/blob/dev/tree.jquery.coffee#L683

There is no need for appendNode to behave this way. This should be much less expensive operation and only append a node and eventually toggle the parent node to become a folder if it is the first element.

Even if you want to recreate the parent node, you could detach the children instead, create the parent node, re-attach them and append the new node. This should be much faster, but can also lead to unexpected results (just like the current situation). For example, one might want to attach some data to the ul element ($(ulElement).data 'some', 'thing'). When you remove and recreate the ul that data would be lost.

This is usually not expected. If you're just appending one node, the other nodes should remain intact. In a similar way, if you're removing a node, only that node and their children should be affected. Recreating the entire tree is way too expensive.

Could you please consider reducing the need for calling onCreateLi more than once for each new node?

mbraak commented 12 years ago

It's interesting that people use jqtree in ways that I had not anticipated. I will do some tests with a slow onCreateLi.

rosenfeld commented 12 years ago

It is not just a matter of being slow. You don't want to perform some actions more than once for each appended node.

mbraak commented 12 years ago

I changed apendNode so it recreates less elements. This is now in version 0.11.

This is a small improvement. I'm still looking for ways to improve this.

SirCumz commented 12 years ago

you could make dialogs on the tree.click event?

rosenfeld commented 12 years ago

@SirCumz are you commenting on the right issue? How are dialogs related to appendNode performance?

blind-coder commented 10 years ago

This still seems very expensive, so I'm destroying the tree and recreate it with the completed data. I have to store the old data in a global variable for that, though :-(

Note that I'm adding roughly 100-200 items at a time.

mbraak commented 10 years ago

Perhaps there should be a method to add multiple items.

Something like:

var parent_node = $('#tree1').tree('getNodeById', 123);

$('#tree1').appendNodes(
  [
    { label: 'abc', id: 456 },
    { label: 'def', id: 457 }
  ],
  parent_node
)

Would this be helpful?

blind-coder commented 10 years ago

Sure thing! That would again help me remove some code from my project :-)

rosenfeld commented 10 years ago

Yes, very helpful :)

mbraak commented 10 years ago

I think the appendNodes function is not necessary. The loadData can do the same:

$('#tree1').tree(
  'loadData',
  [
    { label: 'abc', id: 456 },
    { label: 'def', id: 457 }
  ],
  parent_node
);
rosenfeld commented 10 years ago

Yes. I guess I'm using this already. Maybe it's time to close this issue...