Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#4904 closed enhancement (fixed)

tree to save open/closed state of each node

Reported by: tk Owned by: tk
Priority: high Milestone: 1.0
Component: Dijit Version: 0.9
Keywords: Cc: tk
Blocked By: Blocking:

Description (last modified by tk)

I bring this up because it seems like it would be a common use case (more common than having SplitPane? remember its sizing). Should Tree support remembering what items are expanded via cookies?

If this is something that we could consider adding, I'll write up a patch for review... but I figured I'd propose it here and see what comes of it.

-Karl

Attachments (2)

Tree.diff (4.7 KB) - added by tk 12 years ago.
Revised patch using _setChildren to prevent lazy load problems
Tree2.diff (6.0 KB) - added by tk 12 years ago.
Converting from arrays to hashes for SaveState?

Download all attachments as: .zip

Change History (19)

comment:1 Changed 12 years ago by tk

Description: modified (diff)

comment:2 Changed 12 years ago by bill

Summary: dijit.tree proposal (Cookie support)tree to save open/closed state of each node

Yeah, certainly there is an argument for that to happen. I don't think that information should be inside the data store (as it's just a view thing. As for putting it in a cookie, it seems like that might be too much information for a cookie. Not sure.

comment:3 Changed 12 years ago by tk

My idea was to just build an array by looping through tree.getChildren() and storing child.item.id (or whatever its identifier attribute is) and serialize it (or store in string format) ONLY of the expanded nodes... so you get rootnode1,child2,subchild4 in the Cookie...

comment:4 Changed 12 years ago by tk

Patch doesnt include where to tie in to Tree yet...

Should we tie into tree.expandNode() or make the developer decide what should cause the saveState() call and restoreState() calls?

comment:5 Changed 12 years ago by tk

Owner: set to tk
Status: newassigned

I played around with the Tree some more and found the best places to tie in this feature...

it was added to left/right arrow handlers, and expandoNodeClick handler, this allows for the tree to programatically change states with only the users physical interactions causing saveState() to fire..

I also made save/restoreState public since programtic changes aren't monitor, this allows the user to create stateSaves whenever.

This also includes an update so that when restoreState() is called, all nodes collapse to ensure correct state recovery... didnt even cause a flicker when I called restoreState repeatedly from my local machine.

And you will see 2 modifications to the template files, just ecapsulating 1 attribute in each file in quotes like they should be.

comment:6 Changed 12 years ago by tk

I forgot to mention I also added "useCookies" attribute, to disable state caching.

comment:7 Changed 12 years ago by bill

The problem is that the tree nodes load asynchronously, so we can't have synchronous code to expand the nodes because some of them won't even exist yet (especially deeply nested ones). I'd suggest code that monitors as nodes come in (setChildren and addChildren, I guess) and then auto-expand nodes as appropriate. IE, convert the cookie into a hash and then do lookups into it.

comment:8 Changed 12 years ago by tk

I think I found the solution to this problem....

All children are loaded/created via _TreeNode widget's _setChildren() function... all children node's dojo.date.item attributes are exposed here, so it would be easy to compare each nodes identifier to the stored cookie and expand them as they are added... Basically added a line or 2 in the loop that calls new dijit._TreeNode()

I'll test this when I get home.

Changed 12 years ago by tk

Attachment: Tree.diff added

Revised patch using _setChildren to prevent lazy load problems

comment:9 Changed 12 years ago by Karl Tiedt

Resolution: fixed
Status: assignedclosed

(In [11200]) fixes #4904 moved restoreState onLoad checks into _setChildren where we can scan all new child nodes as they are created and then expand them if needed. This means that saveState/restoreState() work *with* lazyloading, and will not force a hidden node to be loaded at any time.

restoreState() and saveState() are public due to the fact that programatic tree manipulations will not force an automatic saveState.

We also have useCookies parameter to disable cookies.

And fixed 2 attributes in templates that weren't encapsulated in quotes.

comment:10 Changed 12 years ago by Karl Tiedt

(In [11201]) refs #4904 Updating test case to show use of Cookies and non use of Cookies.

comment:11 Changed 12 years ago by Karl Tiedt

(In [11202]) refs #4904 Creates an additional cacheArray to not destroy lazy loading of Child nodes (previous implementation worked with single nodes, this allows multiple nodes to be expanded under parent nodes that are collapsed and they will all continue to function despite Tree interactions getting to that point.

comment:12 Changed 12 years ago by Karl Tiedt

(In [11203]) refs #4904 removes fixme by using array.slice() to clone the array.

comment:13 Changed 12 years ago by Karl Tiedt

(In [11211]) refs #4904 fixes restoreState() so it takes advantage of cacheArray...

TODO:

convert to hash instead of array.

comment:14 Changed 12 years ago by tk

Resolution: fixed
Status: closedreopened

Bill,

The has idea didnt pan out just yet, going to revisit tommorow (its 1:30am). Current problem when converting to hashes: if you have a->b->c->d and C is expanded and B and D are not... previously cacheArray would hold C safely... now with a hash.. I have a problem...

C cant expand until its loaded meaning B is expanded, with cacheArray this would work by monitoring setChildren for when B loads its children and when C is loaded, it expands

With hashes, I expand B and as C loads it expands just fine... now I close C and saveState fires, but now if I query for expanded nodes to update them C remains true in the hash since I didnt overwrite it. First thought was "query for all nodes with aaa:expanded attribute and save them all as true/false, but then this busts the attempt at caching for lazyloaded items that arent loaded still...

Possible solution, but expands lines of code more so... place saveState updating into expand/collapseNode functions to toggle specific nodes in hash to true/false as they are updated... not as clean but would work...

Thoughts? (or ideas I missed?)

-Karl

ps. Current checkin has all the fixes we discussed, persist,_openedItemsId (and until hashes are hashed out (pun intended) _cachedItemsId)

comment:15 Changed 12 years ago by bill

I'd sugggest:

  1. modify expand/collapseNode to update the hash by doing this.hash[id]=true or delete this.hash[id] (delete is better than setting to false). Then the hash is always accurate.
  2. remove the dojo.query() call from saveState(). It can simply serialize the hash.
  3. come to think of it you don't need a dojo.query() call in restoreState either. you can just loop through the items in this._itemNodeMap and either collapse or expand them, according to the hash.

comment:16 Changed 12 years ago by bill

Milestone: 1.0

Changed 12 years ago by tk

Attachment: Tree2.diff added

Converting from arrays to hashes for SaveState?...

comment:17 Changed 12 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [11252]) Cleanup of Tree persistence code to:

  • desupport saveState() and restoreState() as public functions. A tree remembers which nodes are open, and when it's re-instantiated it automatically opens those nodes. No new methods or flags are supported except for persist=true/false flag.
  • store open nodes in a hash rather than an array
  • remove one-off functions

Fixes #4904. Some of code changes from Tree2.diff in that bug incorporated.

Note: See TracTickets for help on using tickets.