Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Duplicate
    • Affects Version/s: 1.4-M1
    • Fix Version/s: None
    • Component/s: wicket
    • Labels:
      None

      Description

      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.

      1. tree.diff
        168 kB
        Sven Meier

        Issue Links

          Activity

          Hide
          Martin Grigorov added a comment -

          Dulicate of WICKET-4240.

          Show
          Martin Grigorov added a comment - Dulicate of WICKET-4240 .
          Hide
          Vladimir Kovalyuk added a comment -

          Eelco, I like http://code.google.com/p/wicket-tree/
          I suggest including it into wicket-extensions to support "the big tree" case.

          Show
          Vladimir Kovalyuk added a comment - Eelco, I like http://code.google.com/p/wicket-tree/ I suggest including it into wicket-extensions to support "the big tree" case.
          Hide
          Eelco Hillenius added a comment -

          Frankly, I currently don't care anymore about compatibility with Swing. Just saying where I was coming from when I wrote the first (pre-Ajax) implementation.

          And indeed, the problem with optimizing the tree by getting rid of the markup (deltas) being sent for every request is that you wouldn't be able to put actual Wicket components in there, which defeats the purpose of the framework I guess Maybe we can just conclude that Wicket tree is great for small to medium sized trees in a web interface, but if you need to support larger trees, you better write something (maybe non-Wicket or just not flexible) from scratch.

          Show
          Eelco Hillenius added a comment - Frankly, I currently don't care anymore about compatibility with Swing. Just saying where I was coming from when I wrote the first (pre-Ajax) implementation. And indeed, the problem with optimizing the tree by getting rid of the markup (deltas) being sent for every request is that you wouldn't be able to put actual Wicket components in there, which defeats the purpose of the framework I guess Maybe we can just conclude that Wicket tree is great for small to medium sized trees in a web interface, but if you need to support larger trees, you better write something (maybe non-Wicket or just not flexible) from scratch.
          Hide
          Matej Knopp added a comment -

          But we only update portions that have actually changed (even though we need to send entire rows). The problem is that there is lot of generated markup for each row. Only way around here would be to send tree data and build the markup on client. But that would mean you can't put wicket components in nodes.

          Also the swing TreeModel is not very nice interface but it does it's job. It's much better now in wicket then it was since i was able to get rid of TreeNode - so now any object can be a node.

          And for simple implementation all you need is to implement 5 simple methods if you want to build it from scratch. Tracking changes is bit more difficult but if you look like TreeModelListener it does look pretty sane. Even though probably more complex as it need to be for our needs.

          However we need to be able to track changes to model to be able to do partial updates. Partial updates are the key point of wicket's AbstractTree. For 1.5 it would be nice to get rid of the swing classes simplifying the model, but only if we can build an adapter so that swing tree model can still be used.

          Show
          Matej Knopp added a comment - But we only update portions that have actually changed (even though we need to send entire rows). The problem is that there is lot of generated markup for each row. Only way around here would be to send tree data and build the markup on client. But that would mean you can't put wicket components in nodes. Also the swing TreeModel is not very nice interface but it does it's job. It's much better now in wicket then it was since i was able to get rid of TreeNode - so now any object can be a node. And for simple implementation all you need is to implement 5 simple methods if you want to build it from scratch. Tracking changes is bit more difficult but if you look like TreeModelListener it does look pretty sane. Even though probably more complex as it need to be for our needs. However we need to be able to track changes to model to be able to do partial updates. Partial updates are the key point of wicket's AbstractTree. For 1.5 it would be nice to get rid of the swing classes simplifying the model, but only if we can build an adapter so that swing tree model can still be used.
          Hide
          Eelco Hillenius added a comment -

          I'm still not too bothered by the fact that Wicket's tree uses some Swing classes. When writing the initial implementation, I actually had the requirement of being able to reuse the model in a Swing application, and that worked well back then.

          That said, I'm not against a more straightforward approach either. There are several things to not like in this tree's implementation and some of the Swings classes. However, the real bottleneck with our tree and I presume the rewrite as well, has to do with the fact that we send Ajax updates using markup. With a larger tree this results in really large updates (100k+ for a single Ajax request). A smarter tree build from the ground up would only send node deltas and let the tree figure out after an initial render how to go about displaying that. Some loss of flexibility maybe, but for a huge efficiency gain.

          Show
          Eelco Hillenius added a comment - I'm still not too bothered by the fact that Wicket's tree uses some Swing classes. When writing the initial implementation, I actually had the requirement of being able to reuse the model in a Swing application, and that worked well back then. That said, I'm not against a more straightforward approach either. There are several things to not like in this tree's implementation and some of the Swings classes. However, the real bottleneck with our tree and I presume the rewrite as well, has to do with the fact that we send Ajax updates using markup. With a larger tree this results in really large updates (100k+ for a single Ajax request). A smarter tree build from the ground up would only send node deltas and let the tree figure out after an initial render how to go about displaying that. Some loss of flexibility maybe, but for a huge efficiency gain.
          Hide
          Igor Vaynberg added a comment -

          we are still planning on doing this in 1.5

          Show
          Igor Vaynberg added a comment - we are still planning on doing this in 1.5
          Hide
          Sven Meier added a comment -

          No more effort should be put in the current tree implementation.
          It's concepts are and will be foreign to Wicket - see http://code.google.com/p/wicket-tree for an alternative implementation.

          Show
          Sven Meier added a comment - No more effort should be put in the current tree implementation. It's concepts are and will be foreign to Wicket - see http://code.google.com/p/wicket-tree for an alternative implementation.
          Hide
          Thomas Mäder added a comment -

          Sven, have you ever had a look at the JFace TreeViewer? The INodeProvider looks very much like JFace's ITreeContentProvider. However, the JFace TreeViewer doesn't require you to know the exact delta to the underlying data structure, but does a smart diff (you can just say refresh(Node) and the tree will figure out the delta). I have in the past written an adapter from an ITreeContentProvider to a TreeModel, so if there's interest, I could help out with some code.

          Show
          Thomas Mäder added a comment - Sven, have you ever had a look at the JFace TreeViewer? The INodeProvider looks very much like JFace's ITreeContentProvider. However, the JFace TreeViewer doesn't require you to know the exact delta to the underlying data structure, but does a smart diff (you can just say refresh(Node) and the tree will figure out the delta). I have in the past written an adapter from an ITreeContentProvider to a TreeModel, so if there's interest, I could help out with some code.
          Hide
          Sven Meier added a comment -

          I see, a single TreeState for multiple pages could be useful.
          But I'd just prefer the tree being more aligned with other Wicket concepts: models and providers of models (I*Provider) .

          I'll try to keep the patch in sync.

          Show
          Sven Meier added a comment - I see, a single TreeState for multiple pages could be useful. But I'd just prefer the tree being more aligned with other Wicket concepts: models and providers of models (I*Provider) . I'll try to keep the patch in sync.
          Hide
          Matej Knopp added a comment -

          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?

          Show
          Matej Knopp added a comment - 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?
          Hide
          Igor Vaynberg added a comment -

          matej and i have been talking about this for a good while. we were discussing something pretty similar to the patch. i think matej is waiting for the point in time when he can break the api and do the refactor. this help will help much, thanks!

          Show
          Igor Vaynberg added a comment - matej and i have been talking about this for a good while. we were discussing something pretty similar to the patch. i think matej is waiting for the point in time when he can break the api and do the refactor. this help will help much, thanks!
          Hide
          Sven Meier added a comment -

          Patch implementing the proposed solution.

          Show
          Sven Meier added a comment - Patch implementing the proposed solution.

            People

            • Assignee:
              Sven Meier
              Reporter:
              Sven Meier
            • Votes:
              3 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development