AW: [riena-dev] TreeRidget questions and comments

["Volanakis, Elias" <evolanakis@xxxxxxxxxxxxxx>]

Hi Carsten,

thanks for the detailed feedback. Please see my inlined comments marked with ## EV. 

Kind regards,
Elias.


-----Ursprüngliche Nachricht-----

1. Should the selection be set with an ITreeNode or with DefaultObservableTreeNode.getUserObject()?

As it is now the ITreeNode should be used since this must work with all implementations of ITreeNode. As you suggested (4.) the method getUserObject() could be moved to ITreeNode. But I agree with you that even then we should still use the node. If needed we could add a convenience method setSelectionByValue(..).

## EV: Selecting by ITreeNode sounds reasonable (if we keep them, see prev. mail). Otherwise I would suggest selected by value.


2. Does it make sense to have selection methods with indices for a tree?

We have these methods because ITreeRidget and ITableRidget share a common interface ISelectableRidget. For the tree the indices should be interpreted as 'as the tree is currently displayed' i.e. depending on the expansion state. I agree with you that solution (a) not to support this for the tree would be better. But we need these methods for the ITreeTableRidget which is a sub-interface of ITreeRidget. Maybe we should move the methods to another interface ISelectableTableRidget...?

## EV: I slightly disagree: Even for the ITreeTableRidget (a tree with columns) it is not clear to me what the benefit of selecting by index is, because the indices are bound to change with changes in the tree structure. Or did I miss something? 


3. How should we handle expand/collapse method calls on the ridget if it is not bound to a UI-widget?

You are right: just doing nothing is not a good solution. We could replicate the expansion state of the tree in the ridget. This means we would have to add a listener to update the ridget whenever the user opens or closes a node. Then we could copy the selection state into the UI widget when it is bound with setUIControl(..). Or we could keep a history of method calls and invoke them on the widget once it is bound.

I think for programmer it should be transparent if a ridget is currently bound or not so I don't think we should throw an exception. 

## EV: I agree. Assuming that the model is the same we can replicate the expansion state. The JFace TreeViewer has #getExpandedElements / #setExpandedElements method that could be used to "preserve" the expansion state. If a new model is bound, I would "reset" the expansion state.



4. Should there be a method getValue() in ITreeNode?

This sounds like a good idea. I think there will be always some nodes in a tree that have a value. There may be trees where only the leafs have values while the other nodes are used to categorize them. But (like it is implemented in DefaultObservableTreeNode) there can be a default implementation that returns null if no value is set.

## EV: I agree.


5. DefaultObservableTreeNode vs. DefaultTreeNode and its subclasses

It think we should use only DefaultObservableTreeNode and remove DefaultTreeNode. The DefaultTreeNode hierarchie was copied from the preceeding project. DefaultObservableTreeNode is and adaption of DefaultTreeNode that implements the Eclipse API IObservable. Keeping the DefaultTreeNode hierarchie is not necessary since the code is in the old CVS and would be only confusing. Actually DefaultTreeNode appears to be used in many examples but should be replaceable with DefaultObservableTreeNode.

## EV: Yes, that is what I've been thinking too.


---
Elias Volanakis
Technical Lead
Innoopract, Inc.
351 NW 12th Avenue
Portland, Oregon 97209
Tel: +1-503-552-1457
Fax: +1-503-715-4915
Mobile: +1-503-929-5537
evolanakis@xxxxxxxxxxxxxx
http://rapblog.innoopract.com