Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9339 closed enhancement (fixed)

Tree: we need public functions to access a node from an item

Reported by: Kenny Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.0
Keywords: tree item node _itemNodeMap Cc:
Blocked By: Blocking:

Description

Hello,

Well, that's not a bug, just a feature that will help us much.

How to access a treeNode when you have a ref to a store item or some datas?

For now, it exists only the private function _itemNodeMap to achieve that.
It works good, but it's private.

Do you think it will be possible to add some public methods in the tree api that give us that possibility?

Thanks Kenny

Attachments (1)

tree.patch (7.5 KB) - added by alle 10 years ago.
New patch with tests

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by bill

Summary: in the tree api, we need public functions to access a node from an itemTree: we need public functions to access a node from an item

I could make this public, but why do you want to access the TreeNode? I tried to design Tree so that you would never need to access or even know about TreeNode's.

Also, in the future there may be multiple TreeNode's associated w/a single item (see #5535), so I think any such public function would need to return an array.

comment:2 Changed 10 years ago by Kenny

Hello Bill,

Thanks for answering.

Well, here are 2 exemples where I need that:

  • somewhere, I destroy and re-create a tree. After the reload, I need to setSelected the same node as before the reloed. And after the reload, my ref to my treeNode I had is not longer valid. So I have some datas that give me the possibility to do a fetch on the store and get a ref to the correct item.

From there, I need to have a ref to the correspondant TreeNode? with which I can call functions to update others things.

  • another point, is when I create a child node, I want to directly go in edit mode (for the name / an inlineEditBox) for the newly created node.

Again, I have the datas that give me the possibility to do a fetch on the store but then, I only have a ref to the item and not the treeNode.

Maybe I'm missing something, but from there, the only way I found to have my ref to the treeNode is to use the private function _itemNodeMap

Like this:

var _props = {name: "xxx", nom_dossier: "yyy", tri: 1};
                
structureStore.fetch({query: _props, onComplete:function(items, request) {
        if (items.length > 0) {
                myClickedNode = dijit.byId("structureTree")._itemNodeMap[items[0].id];
                myClickedNode.setSelected(true);
        } else {
                myClickedNode = null;
        }
        onPropertiesPageShow();
}});

I think the multiple TreeNodes? associated with a single item souldn't be a problem. I guess there will be a way (attr) to be able to find the one we are looking for.

Also, I know that I'm not the only want to have that problem. Yesterday, on the irc, annother user told me he had the same problem.

Kenny

comment:3 Changed 10 years ago by Kenny

I was thinking that in a general way, everytime you have some datas from outside the tree and then you want to access a treeNode, for visual effects or to activate functions that need a treeNode as param, you need that public function.

To my knowledge, the best way to do that is to use the fetch function on the store. But then, you have a ref to an item, not the node.

It could have been nice to have a fetch equivalent, like a fetchNode function that will return an array of treeNode (well like fetch) if the query points to several nodes.
That would be great.

ie, other parts of my page show infos about the selected page.
The functions to update these parts are called on the onclick event of the treeNodes. It's easy there to have a ref to the treeNode clicked and give it as param.
But, after a reload of the tree, I have to manually launch these functions and select the same treeNode as before.
All I have, as the ref to the treeNode before the reload is no longer valid, are datas to query the store and find back the right treeNode ref.

comment:4 Changed 10 years ago by bill

Milestone: tbd1.4

I guess it makes sense. Want to finish #9361 first since it's related.

At that point maybe it's as simple as renaming _itemNodesMap to be public.

comment:5 Changed 10 years ago by alle

With #9631 there can be more than one node related to the same item so a read method ( getNodesByItem(<item>) ) have to return an array of nodes.
That's not a problem but having different path in tree to different nodes related to same item can make selection persistence on tree reload a bit difficult. Same for some sort of tree selection syncing.
There's a bigger problem when items and nodes are lazy-loaded so _itemsNodeMap change on tree reload and can change without store changes (so without notification too).
Ex: we can try set old selection but there's no node related to that item because it will be a child node of a closed node that hasn't jet loaded its children.

I think a better way to get/set selection based on item that support tree-reload and tree-selection-syncing is getting/setting full tree selection path mapped to related items. Creating an attr named "ItemSelectionPath?" or something prettier which is an array of items starting from root item and ending to item related to selected node.
This way a tree can load not already loaded children as needed (no lazy-loading problem on tree reload) and there's no problem about selecting right node too.

AFAIKS the only remaining problem of the above solution is resetting perfect node selection when an item is child of an other more than once because we don't know which children related to same item select using ItemSelectionPath? only.

comment:6 Changed 10 years ago by bill

That's a good suggestion. Might still want the item --> nodes mapping so the automatic-edit example above can be done.

comment:7 Changed 10 years ago by alle

As far as #9631 is fixed (see new patches) we can start fixing this.

I think we need good names for:
read method used for item -> nodes mapping
item selection path attribute

comment:8 Changed 10 years ago by alle

I'm working on a patch.
Let me know if someone is already doing it.

comment:9 Changed 10 years ago by Kenny

alle & bill,

Thanks a lot for your implication in this. That's great.

++

comment:10 Changed 10 years ago by bill

Oh wow you are doing a lot. (The patch has code to select a node from a certain path.) It looks generally good.

I imagine some people will want to simply say setSelectedItem(foo) and have the tree figure out the path to that item. But I have no idea how to implement that; I think it would require the model to have some method like findParentsOfItem() (the converse of getChildren()), and anyway seems like a ball of wax we don't want to get into. OTOH though I can see it being very useful, especially since the standard interface to trees is via items.

Oh also there should of course be a corresponding method to get the path of the currently selected node. Probably they should both be accessed though the standard attr interface, like attr('path') and attr('path', ...), which return/take an array of items?

BTW this.connect() calls the specified function with "this" as the context so you don't need to have a self variable.

comment:11 Changed 10 years ago by alle

setSelectedItem(item, ancestorItems) already support setSelectedItem(item) because second parameter is optional. If ancestorItems is not present or invalid I select first selectable node related to 'item' already loaded in tree for code semplicity. Also path of selected node is supported via getAncestorItems() and getSelectedItem().
I can't use attr interface because I tried to keep setSelectedItem(item) in same method but 'path' attr and 'selectedItem' attr both read/write where 'path' is recommended can be best solution.

Thanks for this.connect() hint.

I'll add a new patch shortly.

comment:12 Changed 10 years ago by alle

New patch with 'selectedItem' and 'path' attr.
Code cleanup and code documentation too.

comment:13 Changed 10 years ago by bill

Cool, that's looking good. Could you possibly write an automated test for this, probably just expanding/following the model of dijit/tests/Tree.html?

I think also we need to add a path attribute for documentation purposes and so that markup like below works:

<div dojoType=dijit.Tree path="['Foods', 'Fruits', 'Citrus']">

(I guess _setPathAttr() should accept id's as well as items)

Other notes:

  • getNodesByItem: an easier way of copying an array is [].concat(oldArray).
  • _setPathAttr: you can use path.shift() rather than splice(0,1)[0]
  • For _getPathAttr(): rather than splice(), you can just call ary.unshift(val);
  • oh, our coding standards say to have spaces around > and = and == (and ===), and after

Changed 10 years ago by alle

Attachment: tree.patch added

New patch with tests

comment:14 Changed 10 years ago by alle

I think new patch should address all comments and notes including automated tests too.

comment:15 Changed 10 years ago by bill

Owner: set to bill
Status: newassigned

Great, thanks for writing the tests!

The only problem was that the tests didn't run correctly on IE, but apparently you just needed Deferred objects in all the tests (setting/getting path isn't instant since you need to wait for the datastore and for the animations to complete). I fixed that.

Will check in.

comment:16 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [18379]) Tree enhancements:

  • path attribute to set/get the currently selected item in the tree (ex: ["Foods", "Fruit", "Apple"])
  • selectedItem attribute to get/set the currently selected item in the tree. Can only set currently selected item if that item is currently visible (ie, already downloaded from the model) and there's only one TreeNode? matching that item (ie, it's not multi-parented)
  • getNodesByItem() method to get list of TreeNodes matching a given item or item id.

Thanks to Alessandro Ferrari (alle) (CLA on file) for the great patch!

Fixes #9443, #9339 !strict.

comment:17 Changed 10 years ago by Kenny

Bill & Alle..

That's excellent news.

Big thanks for your work.

Note: See TracTickets for help on using tickets.