Opened 6 years ago

Closed 6 years ago

Last modified 4 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:

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)

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

Download all attachments as: .zip

Change History (23)

comment:1 Changed 6 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 6 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 6 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 6 years ago by Simon Speich

adds saving of selection state to tree

comment:4 Changed 6 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']])

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.

comment:5 Changed 6 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){
    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.

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

Changed 6 years ago by Simon Speich

Adds saving selection state to tree

comment:6 Changed 6 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 6 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 6 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(){
		this._loadDeferred.callback(true);
		this.onLoad();
	}));
}));

comment:9 Changed 6 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 6 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

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 6 years ago by bill

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

Changed 6 years ago by Simon Speich

Adds loading and saving of selection state to tree

comment:12 Changed 6 years ago by Simon Speich

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

comment:13 Changed 6 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);

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

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

comment:14 Changed 6 years ago by bill

  • Milestone changed from tbd to 1.8
  • Owner set to bill
  • Status changed from new to assigned
  • Summary changed from Tree: _initState() should also remember last selected/focused node to Tree: persist=true should also remember previously selected nodes

comment:15 Changed 6 years ago by bill

  • Resolution set to fixed
  • Status changed from assigned to closed

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 6 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 6 years ago by bill

See #14443, a problem with this patch.

comment:18 Changed 5 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 5 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 4 years ago by Bill Keese <bill@…>

In a43c360fb0d3c7421bd22182cc65cf0638843eb0/dijit:

Remove vestigial code, fixes #17321, refs #14058.
Thanks Simon.

Note: See TracTickets for help on using tickets.