Wicket
  1. Wicket
  2. WICKET-3789

visitChildren and friends require inner classes. Replace with iterators

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      I never really liked that we need inner classes (implementations of IVisitor) for traversing the component hierarchy. When needed, it comes easily to me that I need MarkupContainer.visitChildren(), but I always need to look up how to use it and friends. Debugging is also more complicated than it has to (breakpoint inside the visit function: either another breakpoint behind visitChildren or "press the continue-until-end-of-method" key several times until you are back). For these reasons I gave Iterators another try. I've attached a patch for others to review and provide feedback. I've also added test cases (ComponentIteratorTest) and I changed quite some visitChildren occassions in core. Maven compiles and successfully executes all tests.

      I put the new classes in org.apache.wicket.util.iterator
      ComponentIterator - An enhanced iterator with filters. Also supports chaining iterators. Builder API for class filters, isvisible filters, isenabled filters etc. Supports Java for each.
      ComponentHierarchyIterator - enhances ComponentIterator to provide hierarchy traversal. Adds traversal filters to separate traversal control from what next() returns. Same Builder API. Supports Java for each.
      IteratorFilter - Simple abstract class to implement the filter conditions. The Builder API makes use of it to add filters to the iterator.

      Some examples:
      ComponentHierarchyIterator iter = new ComponentHierarchyIterator(page);
      while (iter.hasNext())

      { Component component = iter.next(); }

      for (Component component : new ComponentHierarchyIterator(page))
      {
      }

      new ComponentHierarchyIterator(page)
      .filterLeavesOnly()
      .filterByVisibility()
      .filterByClass(Form.class)

      iter.skipRemainingSiblings(); // skip all remaining component of the same parent
      iter.dontGoDeeper() // provided the current component is a container and potentially has children, they are ignored

      onEndOfSiblings() is a callback function if you want to be informed about the iterator leaving a level

      1. wicket-3789.patch
        65 kB
        Juergen Donnerstag

        Issue Links

          Activity

          Hide
          Pedro Santos added a comment -

          +1
          I like IVisitor because it is a very natural way of visiting children and enable a good decoupling between callback and visiting logic. i.e. you can place both in different classes and extend/reuse them.
          But the majority of use cases are simple enough to demand a new type, and the proposed idea address them nicely.

          Why IteratorFilter#onFilter has the leaf parameter? Seems more natural to have Component#isLeaf instead a polluted API for filters. Most of filters I saw in the patch ignore this flag.

          Show
          Pedro Santos added a comment - +1 I like IVisitor because it is a very natural way of visiting children and enable a good decoupling between callback and visiting logic. i.e. you can place both in different classes and extend/reuse them. But the majority of use cases are simple enough to demand a new type, and the proposed idea address them nicely. Why IteratorFilter#onFilter has the leaf parameter? Seems more natural to have Component#isLeaf instead a polluted API for filters. Most of filters I saw in the patch ignore this flag.
          Hide
          Martin Grigorov added a comment -

          Do you have any expectations about the performance difference compared to visitors ?
          The new Event bus uses heavily the hierarchy traversal.

          Show
          Martin Grigorov added a comment - Do you have any expectations about the performance difference compared to visitors ? The new Event bus uses heavily the hierarchy traversal.
          Hide
          Igor Vaynberg added a comment -

          i do like this idea as well. it needs to be refined as it is still somewhat rough. eg i would like to be able to say

          for (Foo foo:new ComponentHierarchyIterator<Foo>(page, Foo.class)) without having to cast to Foo myself

          i would also like a new ComponentHierarchyIterator().postOrder()

          the part where it gets messier is that it does not separate the configuration of the visit from the visit so i can say weird stuff like this:

          ComponentHierarchyIterator it=new ComponentHierarchyIterator();
          for (Component c:it)

          { it.filterByClass(Bar.class); <== mutating the visit like that - its strange to be able to do this }

          in any case, we should put this off until 1.6. 1.5 is stable and we need to release it asap.

          Show
          Igor Vaynberg added a comment - i do like this idea as well. it needs to be refined as it is still somewhat rough. eg i would like to be able to say for (Foo foo:new ComponentHierarchyIterator<Foo>(page, Foo.class)) without having to cast to Foo myself i would also like a new ComponentHierarchyIterator().postOrder() the part where it gets messier is that it does not separate the configuration of the visit from the visit so i can say weird stuff like this: ComponentHierarchyIterator it=new ComponentHierarchyIterator(); for (Component c:it) { it.filterByClass(Bar.class); <== mutating the visit like that - its strange to be able to do this } in any case, we should put this off until 1.6. 1.5 is stable and we need to release it asap.
          Hide
          Juergen Donnerstag added a comment -

          I committed an inital version based on a much nicer implementation, but it's not used yet anywhere.

          That is now supported
          for (Foo foo:new ComponentHierarchyIterator<Foo>(page, Foo.class))

          what is meant by ComponentHierarchyIterator().postOrder() ???

          >> the part where it gets messier is that it does not separate the configuration of the visit from the visit so i can say weird stuff like this:
          That's not yet implemented. Though you are right on filterByClass(), I can imagine you want to enable/disable certain filters depending on some previous results.

          Show
          Juergen Donnerstag added a comment - I committed an inital version based on a much nicer implementation, but it's not used yet anywhere. That is now supported for (Foo foo:new ComponentHierarchyIterator<Foo>(page, Foo.class)) what is meant by ComponentHierarchyIterator().postOrder() ??? >> the part where it gets messier is that it does not separate the configuration of the visit from the visit so i can say weird stuff like this: That's not yet implemented. Though you are right on filterByClass(), I can imagine you want to enable/disable certain filters depending on some previous results.
          Hide
          Igor Vaynberg added a comment -

          postOrder() means i want to visit deepest children first. see Visisits#visitPostOrder() in trunk. we use this, for example, when processing form components because we want to process children of formcomponentpanels before the panels themselves.

          Show
          Igor Vaynberg added a comment - postOrder() means i want to visit deepest children first. see Visisits#visitPostOrder() in trunk. we use this, for example, when processing form components because we want to process children of formcomponentpanels before the panels themselves.
          Hide
          Juergen Donnerstag added a comment -

          That is implemented. See AbstractHierarchyIterator.childFirst.

          Show
          Juergen Donnerstag added a comment - That is implemented. See AbstractHierarchyIterator.childFirst.
          Hide
          Christoph Leiter added a comment -

          What happened to ComponentHierarchyIterator<T>? As it is now it doesn't support a generic type and its Iterator always returns Component.

          Show
          Christoph Leiter added a comment - What happened to ComponentHierarchyIterator<T>? As it is now it doesn't support a generic type and its Iterator always returns Component.
          Hide
          Martin Grigorov added a comment -

          WICKET-5284 exposed a rather big issue with the iterators - they use recursion that doesn't work for pages with bigger component tree

          Show
          Martin Grigorov added a comment - WICKET-5284 exposed a rather big issue with the iterators - they use recursion that doesn't work for pages with bigger component tree

            People

            • Assignee:
              Juergen Donnerstag
              Reporter:
              Juergen Donnerstag
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development