Tree: persist=true should also remember previously selected nodes

[Simon Speich]

When using tree.persist = true the state of all expanded nodes is saved in a cookie and the nodes are expanded on the next tree load. But this does not include the last selected node, e.g. all nodes are expanded but nothing is selected and focused, which is only half of what I would expect from persisting its state.

They easiest way (but maybe not the most efficient?) would be to use an additional cookie which stores the path and then set it in _initState() with this.set('path', cookie(cookieTreePath)).

[bill]

I guess that makes sense. Note that there can be multiple selected nodes ("paths" is preferred to "path"), so I guess they should all be saved.

[Simon Speich]

Would dijit.tree._dndSelector.setSelection() be the right place to set the cookie and tee._initState() to read it? If so I could try to provide a patch (I know you are currently busy getting 1.7 out, so I don't mind to wait for an answer).

[bill]

I guess that's right, you can't save the state in _state() because that only gets called when a user opens/closes a node.

[Simon Speich]

adds saving of selection state to tree

[Simon Speich]

I created (my first) patch from trunk which takes multiple selection into account. It uses the same string format for the cookie as does tree._state() to save opened nodes, e.g.: continentRoot/NA/MX/Mexico City,continentRoot/NA/MX/Guadalajara

The paths are saved in tree._dndSelector to a new cookie in _updateSelectionProperties(). It is read back in tree._initState() and converted to a two dim array to set the paths as: tree.set('paths', [['continentRoot','NA','MX','Mexico City'],['continentRoot','NA','MX','Guadalajara']])

Notes:

• I put the storing of the cookie in tree._dndSelector() to _updateSelectionProperties() instead of setSelection() since that saves looping through the selected nodes again.
• I had to move up the initialization of the dndController in tree.postCreated() otherwise the property tree._dndSelector.cookieName would not be available yet in tree._initState()
• There is one catch though. Click on a child node to select it, then close its parents without deselecting it. Then hit reload. Since tree.set('path') opens all nodes down to the selected one, you will get a initial tree state where the child node is correctly selected, but it's parent incorrectly expanded. I thought of using _dndSelector.setSelected() instead of tree.set('path'), but that's not possible, since not all nodes are loaded yet. Another option if I understand correctly would be to call tree.set('path') before setting its open/closed state and use the returned deferred to set the state after that, but that seems to require (to many?) code changes.

[bill]

Thanks, I checked over the patch. It looks generally good, although it's a bit messy how it saves the cookie in one file but restores it in another file. Maybe restoring the selection should occur from _dndSelector.js too?

I'm not too worried about the catch you mentioned above (where selected nodes are hidden). But it does make me wonder about race conditions. You are calling set("paths", ...) at the same time the the tree itself is loading (and the data in this._openedNodes is getting applied). Maybe the set("paths") should be delayed until after the tree finishes loading? Or rather, wait until the tree finishes loading and then call _dndSelector.setSelected()?

Also, a minor thing, when you find a push() inside of a forEach() that probably means you should be using map() instead, for example I think that

var paths = [];
array.forEach(oreo.split(','), function(path){
    paths.push(path.split('/'));
}, this);

could be:

var paths = array.map(oreo.split(","), function(path){
   return path.split("/");
})

The same could be said of _updateSelectionProperties() but I guess that's an exception since it's setting three new arrays rather than just one.

[Simon Speich]

Adds saving selection state to tree

[Simon Speich]

Created a revised patch as suggested:

• I moved the restoring of the state (reading to cookie) to the _dndSelector.js
• used map instead of push
• delayed set(paths) by using on(this.tree, onload, _setState)

[bill]

Thanks. I'm wondering if (as you originally pondered) it should call _dndSelector.setSelected() rather than set("paths", ...). As you pointed out the set("paths", ...) call could trigger more database accesses and be asynchronous, which means that the tree could still effectively be loading after the onload event occurs.

[Simon Speich]

I'm confused now. Doesn't calling setSelected() require all nodes to be loaded first, which might not be the case and that's what calling set('paths') takes care of?

Are you saying I shouldn't connect to the onLoad(), because then onLoad() doesn't necessarily signal a complete loading anymore (since it could fire before set('paths) has finished)?

If so, would it be ok to just defer firing onLoad() in tree._load() by doing something like:

// not working yet
this._expandNode(rn).addCallback(lang.hitch(this, function(){
	Deferred.when(this.dndController._initState(), lang.hitch(this, function(){
		this._loadDeferred.callback(true);
		this.onLoad();
	}));
}));

[bill]

I meant that _initState() would skip selecting any nodes that weren't already loaded.

And yes, I was worried about onLoad() not necessarily signaling a complete loading anymore.

What do you think?

[Simon Speich]

I agree that onLoad() should signal a complete loading.

So the remaining question when setting the selection state is whether nodes that aren't already loaded should: A) be skipped or B) be expanded and firing onLoad() should be deferred

right?

I personally prefer B) since the whole idea was to recreate the complete (selection) state of the tree. The drawback is that it could take longer until onLoad() fires. Up to you to decide.

[bill]

I guess (B) is better, and I guess it would be code like you wrote above.

[Simon Speich]

Adds loading and saving of selection state to tree

[Simon Speich]

Created new version which defers firing onLoad() until selection is applied to tree.

[bill]

Cool, looks good. I think that just setting paths will affect all the other elements, rather than having to do:

this.tree._set("paths", paths);
this.tree._set("path", paths[0] || []);
this.tree._set("selectedNodes", nodes);
this.tree._set("selectedNode", nodes[0] || null);
this.tree._set("selectedItems", items);
this.tree._set("selectedItem", items[0] || null);

<edit> Oh actually nevermind, I see that code was there before your change. I'll leave it (for now anyway).

[bill]

In [27020/dojo]:

Make Tree's persist=true save and restore selected nodes (in addition to which nodes are open/closed), patch from Simon Speich (CLA on file), thanks! Fixes #14058 !strict.

[bill]

In [27033/dojo]:

Tree._loadDeferred was firing before the appropriate TreeNodes were selected (either the nodes saved in the "persist" cookie, or those specified via paths[] argument to constructor).

There's a question of what should happen when persist=true but also there's a paths[] argument specified to the constructor. I opted to follow the paths[] argument. I think prior to this check in initialization was calling set("paths", ...) twice, once w/the paths[] argument to the constructor and again with the saved selection state.

Refs #14058 !strict.

[bill]

See #14443, a problem with this patch.