Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#14058 closed enhancement (fixed)

Tree: persist=true should also remember previously selected nodes

Reported by: Simon Speich Owned by: bill
Priority: high Milestone: 1.8
Component: Dijit Version: 1.6.1
Keywords: tree, path, selection, state Cc:
Blocked By: Blocking:


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)).

Attachments (3)

TreeSaveSelectionState.patch (4.1 KB) - added by Simon Speich 11 years ago.
adds saving of selection state to tree
TreeSaveSelectionState2.patch (2.9 KB) - added by Simon Speich 11 years ago.
Adds saving selection state to tree
TreeSaveSelectionState3.patch (3.7 KB) - added by Simon Speich 11 years ago.
Adds loading and saving of selection state to tree

Download all attachments as: .zip

Change History (23)

comment:1 Changed 11 years ago by 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.

comment:2 Changed 11 years ago by 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).

comment:3 Changed 11 years ago by 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.

Changed 11 years ago by Simon Speich

adds saving of selection state to tree

comment:4 Changed 11 years ago by 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']])


  • 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.

comment:5 Changed 11 years ago by 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){
}, this);

could be:

var paths =","), 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.

Last edited 11 years ago by bill (previous) (diff)

Changed 11 years ago by Simon Speich

Adds saving selection state to tree

comment:6 Changed 11 years ago by 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)

comment:7 Changed 11 years ago by 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.

comment:8 Changed 11 years ago by 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(){

comment:9 Changed 11 years ago by 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?

comment:10 Changed 11 years ago by 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


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.

comment:11 Changed 11 years ago by bill

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

Changed 11 years ago by Simon Speich

Adds loading and saving of selection state to tree

comment:12 Changed 11 years ago by Simon Speich

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

comment:13 Changed 11 years ago by 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);

Did you find differently? (If so, that's a bug.)

Version 0, edited 11 years ago by bill (next)

comment:14 Changed 11 years ago by bill

Milestone: tbd1.8
Owner: set to bill
Status: newassigned
Summary: Tree: _initState() should also remember last selected/focused nodeTree: persist=true should also remember previously selected nodes

comment:15 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed

In [27020]:

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.

comment:16 Changed 11 years ago by bill

In [27033]:

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.

comment:17 Changed 10 years ago by bill

See #14443, a problem with this patch.

comment:18 Changed 10 years ago by bill

In [29266]:

Fix race condition when app calls Tree.set("paths", ...) before Tree has finished loading. Rewrite of [27033], refs #14058 !strict.

comment:19 Changed 9 years ago by bill

In [30746]:

Set Tree::persist default to false, like other widgets, and also don't persist state of selected nodes. If desired, selectedPaths can be persisted by app code same as any other widget attribute. Fixes #14443, #16785, refs #14058 !strict.

comment:20 Changed 9 years ago by Bill Keese <[email protected]…>

In a43c360fb0d3c7421bd22182cc65cf0638843eb0/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.