#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: |
Description
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)
Change History (23)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
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
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
Attachment: | TreeSaveSelectionState.patch added |
---|
adds saving of selection state to tree
comment:4 Changed 11 years ago by
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 ofsetSelection()
since that saves looping through the selected nodes again. - I had to move up the initialization of the dndController in
tree.postCreated()
otherwise the propertytree._dndSelector.cookieName
would not be available yet intree._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 oftree.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
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 doesn't 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.
Changed 11 years ago by
Attachment: | TreeSaveSelectionState2.patch added |
---|
Adds saving selection state to tree
comment:6 Changed 11 years ago by
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
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
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(); })); }));
comment:9 Changed 11 years ago by
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
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.
comment:11 Changed 11 years ago by
I guess (B) is better, and I guess it would be code like you wrote above.
Changed 11 years ago by
Attachment: | TreeSaveSelectionState3.patch added |
---|
Adds loading and saving of selection state to tree
comment:12 Changed 11 years ago by
Created new version which defers firing onLoad() until selection is applied to tree.
comment:13 Changed 11 years ago by
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).
comment:14 Changed 11 years ago by
Milestone: | tbd → 1.8 |
---|---|
Owner: | set to bill |
Status: | new → assigned |
Summary: | Tree: _initState() should also remember last selected/focused node → Tree: persist=true should also remember previously selected nodes |
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.