vakata / jstree

jquery tree plugin
http://jstree.com
MIT License
5.14k stars 1.38k forks source link

State plugin delay #1082

Closed alexandernst closed 9 years ago

alexandernst commented 9 years ago

I think there is a problem (or maybe a feature request) concerning the state plugin. I have explained it here (there is also an example showing the problem).

Can you have a look at it and tell us what you think about it?

vakata commented 9 years ago

Sorry, I do not use angular, so I really can't help you, I can only guarantee that if you load data properly (in one of the many ways described in the docs) the state plugin will work just fine.

Nodes added after ready has fired will not be picked up by the state plugin, read the docs on creating nodes and loading nodes - those are two different things.

Let me know if I can help in any way. If you have spotted a problem - please provide steps to reproduce or a demo, but without an angular wrapper.

Best regards, Ivan

alexandernst commented 9 years ago

@vakata The problem is that the data is added way after the jstree is initiated. By the time the data gets added, state plugin has already kicked in and restored the state (unsuccessfully because there are no nodes).

Maybe a good solution would be to allow passing a state argument to restore_state. Then we could catch the state with state.filter() and restore it after the data is loaded manually with restore_state(state). What do you think?

vakata commented 9 years ago

The question is why do you add data after the ready event? And how do you add that data?

alexandernst commented 9 years ago

@vakata Nodes are added with create_node, and I'm adding them way after the jstree is initialized because the data that should be loaded depends on user select (dropdown) (note that even nothing is selected, the tree should be visible).

vakata commented 9 years ago

I am sure there are way more elegant solutions to this (including refreshing the tree, with the needed data from the dropdown, and even using different state keys).

Anyway, the state plugin has nothing to do with create_node - that is by design and it will not change.

Aside from that - create_node should not be used to populate the tree - it is a run-time method so that the end user can modify the structure, and as such is has limitations. Using it to populate the tree will be very slow, it will also prevent any plugin that reacts on ready from doing its job (just like in your case with state).

Your best bet is to start loading your data properly and not using create_node.

Best regards, Ivan

alexandernst commented 9 years ago

@vakata So... let me see if I understand it correctly.

  1. Create the (empty) tree.
  2. Fill core.data with nodes, based on user selection
  3. Call refresh().

Is that how I should do it? Also, if I do it that way, will the state plugin work? I mean, will it restore my open nodes after calling refresh()?

vakata commented 9 years ago

It all depends on your requirements - you can either: 1) set core.data to a function, in that function read the current user selection and return nodes accordingly, and when the selection is changed - call .refresh 2) destroy the tree each time selection is changed and create it anew, and do not forget to set a proper state.key for each instance - so that the correct state will be restored and it will not be overwritten 3) Same as 2) but with a single state key, and the initial (empty) tree should not have the state plugin loaded so that state is not lost.

Personally 2 or 3 are easier and the choice between them depends on your needs - I can not tell you which approach is better for your use case without diving into the specifics of your application, which I simply should not and can not do.

Best regards, Ivan

alexandernst commented 9 years ago

IMHO all your suggestions don't make any sense from performance or complexity point of view.

  1. A function inside jstree's config that will listen for events on the DOM and do ajax calls and parsing and refreshing? Doesn't sound like a thing one would put in it's config. Really, think about it.
  2. Why would I destroy and recreate my entire tree if the only thing I want to do is to restore a state?! Something as simple as "look, I had nodes 1, 5 and 9 opened; open them". Instead, this is going to destroy the entire tree, DOM operations, data parsing, nodes creation, more DOM operations, triggering all sort of events. Completely wasting all type of resources and time.
  3. Same as point 2.

You NACKed my plugin because it had a couple of calls to jQuery and now you're suggesting me to destroy and recreate the entire tree or write complex functions to do all sort of thing instead of just making a simple change in the state plugin to allow restoring a state from an (optional!) parameter? As I said, doesn't make much sense.

It would be much easier if jstree let me save the state on load (possible already, making a copy of the state with the filter callback from the state plugin) and let me apply it later, on demand. @vakata

vakata commented 9 years ago

Hi,

You totally misunderstood 1) - nothing to do with a function listening for DOM events. I only suggested 2) and 3) because I was under the impression you were loading a totally different tree, and not storing different states of the same tree - be more clear next time.

jsTree already does what you are suggesting, and the jQuery calls in your plugin (I did not even know it was you) have nothing to do with the current issue - if you want me to go into detail - I will - just ask.

As for your issue - start describing the problem more accurately - you did not let me know what your problem is, just what you think a solution should look like, and it was a bad idea. This time - please explain what you need, not what you think the fix should be.

Best regards, Ivan

vakata commented 9 years ago

Judging from your comments - here is what you probably need in a single line of code using set_state, you do not even need the state plugin: http://jsfiddle.net/DGAF4/429/

As you may notice I am manually creating the nodes, then applying a state that will reveal them. I guess that is what you need. You can collect the current state at any given moment using get_state and do whatever you need with it - store it for later use, etc. Once again - set_state and get_state are core functions, all plugins respect those and they work without the need of the state plugin.

The state plugin simply calls get_state every time a node is opened or selected, stores the result in localStorage and then calls set_state the next time the same tree fires a ready.jstree event.

Here is everything state related: http://www.jstree.com/api/#/?q=state

If you need something else - let me know, and provided a detailed description.

Now, as for your plugin: 1) I can not just pull any plugin to the core, because of two reasons

2) the jQuery calls are a problem, because they are very expensive, that is why the _redraw_node function is written in vanillaJS, if you are interested I can show you performance tests I did.

3) I was and am really happy you decided to make that plugin - third party plugins are great and I am really excited when a new one emerges. If I was too harsh - I apologize, I just wanted to make sure your plugin will work properly in all jstree use cases / configs / etc.

That being said - I do not appreciate your tone. As grateful as I am for any bug reports and the plugin, if you do not like the way I am developing jstree there is nothing I can do about it, every day I deal with hundreds of different use cases, help debug various systems, etc, and I do it all for free in my spare time. There is a limit to what I can do, especially when I am not given enough information.

Please let me know if the above solution is what you needed - if not - please clarify so that I can help you further.

Best regards, Ivan

alexandernst commented 9 years ago

A few hours after posting I kind of worked out the save/restore thing, but thank you for making that jsfiddle!

About the plugin, the way I see it, once the plugin reaches certain maturity level, it doesn't need any further maintenance (try to search the last time you had to patch one of the core plugins). Anyways, I won't push that subject any further, the plugin can live in a separate repository too, no harm.

And yes, you do have a point, my tone was inappropriate, I'm sorry about that.

One last thing (I'm not sure if I should open yet another bug report for this one). It would be nice to have a scheme of the life-cycle of the events. Which one is triggered before/after which one. For example, load_node, redraw, refresh and create_node. What is the order of those events if I add a tree using create_node?

Does the refresh event gets triggered when creating a node or only after calling refresh()? Does the redraw event gets called after each call to create_node, or are you using some sort of debounce (underscore) to trigger that event only once N ms after the last call of create_node?

vakata commented 9 years ago

That is truly my bad - the documentation is far from perfect - there is no clear order of events that you can expect, etc. I will try to improve on this in the future. I view the docs as a constant work in progress. I will dedicate some time especially for the docs in the near future.

I do not agree about the maturity level - yes, at some point it is mostly minor tweaks, but still - there is always something happening behind the scenes - I added a few lines to the core just a day ago - it is easy to see from the commit log. I hope that you can understand why I am reluctant to adopt plugins. As I said in my initial comment about the plugin - I will adopt it as soon as it becomes frequently requested. It is pretty robust now, and it also seems quite finished - with the repo and npm package. I do not think it is a bad idea to keep it as a separate plugin for the time being.

As for the events (just a few lines until I get the time to update the docs):

Best regards, Ivan

meraid commented 6 years ago

Hi Ivan, I am pretty new to web and currently using jstree and I load/build my tree nodes using a function like "data": function (obj, cb) {cb.call(this, getdata));. I have a problem and think I am not doing it correctly. When I add or remove a node from the tree the state plugin doesn't save whether a node has been added or removed. But once I check the check box or expand a node then state is saved in the 'checkbox' and 'core : open' array on my local storage. How can I clear the node values from those arrays if I remove a node . I read earlier in this discussion to refresh the tree after removing or adding a node but it doesn't work.

Thanks Ash