[
https://issues.apache.org/jira/browse/WICKET-1600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12595404#action_12595404
]
Matej Knopp commented on WICKET-1600:
-------------------------------------
Hi, I haven't got time to go through the patch in depth, so some of my comments
might be irrelevant :)
Overall, I like the idea a lot. I had in mind something like this, but perhaps
little less radical.
The idea of TreeState separated from tree has it's excuse. I though of usecase
when the tree state could be e.g. kept in session. I.e. you have a navigation
tree and different page instances would share the set of expanded/selected
nodes. There is a slight complication now that deaulttreestate holds reference
to tree, so I'm not sure how much is this relevant at all.
The other thing I'm not sure about is removal of listeners. I do agree that the
concept is a bit foreign to wicket but it did serve it's purpose. On the other
hand, I don't think lot of people register other listener than the tree itself
so even this might not be a problem.
Also, the patch is a bit premature :) I don't have where to apply it, are you
willing to keep it in sync with wicket trunk so it can be applied once 1.5 is
branched?
> Wicket tree improvement
> -----------------------
>
> Key: WICKET-1600
> URL: https://issues.apache.org/jira/browse/WICKET-1600
> Project: Wicket
> Issue Type: Improvement
> Components: wicket
> Affects Versions: 1.4-M1
> Reporter: Sven Meier
> Assignee: Matej Knopp
> Priority: Minor
> Fix For: 1.5-M1
>
> Attachments: tree.diff
>
>
> I see the following issues with Wicket's tree implementation that should be
> solved:
> AbstractTree and its subclasses were written with the Swing JTree API in
> mind. This is not a bad thing per se, but the JTree abstractions are not very
> well suited for a web application. Matej recently removed some of these
> dependencies, but there's still a lot of code that uses TreeNode,
> TreeModelEvent and such.
> AbstractTree holds a TreeModel in its model, attaching a listener to it. This
> is an unusual approach for a Wicket component:
> - It implies that changes in the TreeModel are automatically propagated to
> the user interface. This is not always the case, as in an ajax request the
> AbstractTree has to be explicitely notified to update itself.
> - It requires the AbstractTree to monitor the model for a replaced TreeModel.
> Most importantly the TreeModel API is complicated and ambiguous (just see the
> javadoc of TreeModelEvent and TreeModelListener) which makes life harder
> especially for those who want to use AbstractTree with their own TreeModel
> implementation (which is difficult enough). Although not directly visible in
> the API, an implementor has to privide the parent of a node - see
> AbstractTree#getParentNode(). Tree listeners and events are an annoyance
> which doesn't match Wicket's elegance.
> Currently many components in the AbstractTree hierarchy hold a reference to
> real nodes of the TreeModel (e.g. junctionLink). TreeState seems like a
> foreign concept to Wicket, holding references to nodes too. To support
> detachment AbstractTree tests wether a node implements IDetachable,
> effectively duplicating functionality that is normally provider by Component
> and its model out of the box.
> The nodes are currently used to identify state (e.g. the parent) in the tree.
> To add a node, the nodes have to implement equals() and hashCode() to be
> correctly identified in the tree state. This might be unacceptable, e.g. if
> entities (business objects) are used as nodes, probably loaded from a
> database.
> Therefore I propose to refactor AbstractTree and subclasses as follows:
> A new interface INodeProvider (similar to IDataProvider) defines a concise
> API to define a tree of nodes. The method #model(Object) gives an implementor
> the possibility to wrap nodes in a suitable model, e.g. a detachable one. The
> provided model can handle #equal() and #hashcode() for identification of
> nodes in the tree.
> References to nodes are always indirect through the model, never to the real
> node object.
> Handling of node parents is completely managed inside AbstractTree, no need
> for an ExtendedTreeModel.
> The model of an AbstractTree is used for node selection, similar to
> AbstractChoices.
> Listeners are replaced with notification methods on AbstractTree. No need for
> tree paths on changes.
> Expansion state is held in the AbstracTree#TreeItems (instead of former
> ITreeState) - similar to component visibility.
> The attached patch tries to achieve all this (or at least show how a solution
> could look like), additionally:
> - AbstractTree now utilizes AbstractRepeater and
> #setOutputMarkupPlaceholderTag() instead of custom solutions.
> - The examples are modified and enhanced with 'add' and 'remove'
> functionality, org.apache.wicket.examples.nested.Home shows usage of an Swing
> TreeModel adapter.
> - Lots of generics are added (Swing's TreeModel will probably never be
> generic).
> - The code is (hopefully) simplified.
> I apologies for putting all this stuff into a single patch, but I wasn't able
> to split these issues apart - it all depends on the proposed INodeProvider
> interface.
> For users of AbstractTree this solution has the following impacts:
> - minor API changes for subclasses
> - if a node is collapsed, all state of descendants will be lost, i.e. if the
> node is expanded once again, all previously expanded children will stay
> collapsed (similar to how Gnome handles trees)
> - optimization for adding multiple new children in one AJAX request is not
> (yet) supported, this was part of the former custom markup handling.
> I will continue working on AbstractTree with tests and documentation. But
> before doing that, I'd like to get a feedback from wicket committers
> (especially Matej), if this proposed solution matches their conception.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
