Wicket
  1. Wicket
  2. WICKET-3702

wicket:border: inconsistency between add() and remove()

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC4
    • Fix Version/s: 1.5-RC5
    • Component/s: wicket
    • Labels:
      None

      Description

      Assuming c1 is a Border and c2 is some component, the following sequence crashes with duplicate addition:

      c1.add(c2);
      c1.remove(c2);
      c1.add(c2);

      The reason for this is that remove() doesn't remove the object from the bodycontainer. The sequence can be made to work by changing the middle line to:

      c1.getBodyContainer().remove(c2);

      That remove() doesn't look the component from the same container as add() adds it to, seems to violate the principle of least astonishment. Unfortunately the Component structure manipulation API has more methods, such as swap(), size(), get(), etc. which are final, and can't be overridden by Border as they are. It could be best to force all users to use c1.getBodyContainer().add() instead of c1.add(), because consistent operation is probably easier to deal with in the long run than behavior that conforms to initial assumptions but has flaws elsewhere.

      This ticket suggests removing the overload of add() and documenting the difference in migration guide.

      1. wicket-3702.patch
        33 kB
        Martin Grigorov

        Activity

        Hide
        Martin Grigorov added a comment -

        add() is made this way because it is more convenient for the user to add child components to the border body. Most of the time the Border itself adds the components in the "border area". The other methods are not made this way because they are not so often used.
        The javadoc will be updated.

        Show
        Martin Grigorov added a comment - add() is made this way because it is more convenient for the user to add child components to the border body. Most of the time the Border itself adds the components in the "border area". The other methods are not made this way because they are not so often used. The javadoc will be updated.
        Hide
        Igor Vaynberg added a comment -

        actually there was a request to make add() behave normally for borders
        clients can use border.addtobody() instead of add()

        this is to make things more consistent. if i add() something i should be able to remove() it and see it in iterator() and size()

        Show
        Igor Vaynberg added a comment - actually there was a request to make add() behave normally for borders clients can use border.addtobody() instead of add() this is to make things more consistent. if i add() something i should be able to remove() it and see it in iterator() and size()
        Hide
        Martin Grigorov added a comment -

        Define "behave normally for borders".
        Also take into account the last few comments at https://issues.apache.org/jira/browse/WICKET-2494

        Show
        Martin Grigorov added a comment - Define "behave normally for borders". Also take into account the last few comments at https://issues.apache.org/jira/browse/WICKET-2494
        Hide
        Igor Vaynberg added a comment -

        the problem is consistency. we either forward all related methods or none.

        Show
        Igor Vaynberg added a comment - the problem is consistency. we either forward all related methods or none.
        Hide
        Igor Vaynberg added a comment -

        thinking more about this...

        it would be trivial to forward all the necessary methods. a tweak to markupcontainer with an extra layer of indirection is all it would take. however, if we do this it will make using borders to the user seem consistent, but it will make our own framework code inconsistent.

        it is easy enough to forward add(), remove(), size(), iterator() - most others are written in terms of these. however, do we forward visitChildren()? we would have to to make the experience consistent for user, but by doing those we will make our internal framework code inconsistent. for example - our internal framework code now will not see components added to the actual border - just like the client wont.

        esentially we are creating a portal for the client: OUTSIDE->?>?>BODY and letting user manipulate the body from the outside, skipping the layer in-between. but by making it consistent we also make the layer in-between inaccessible to our own code.

        so the question is: what has the bigger impact. how many usecases exist where the user needs to treat the border without knowing it is a border vs how many usecases exist where we traverse components without needing to know what kind of component it is....i think the latter are much more prevalent.

        long story short, i was wrong in WICKET-2494

        Show
        Igor Vaynberg added a comment - thinking more about this... it would be trivial to forward all the necessary methods. a tweak to markupcontainer with an extra layer of indirection is all it would take. however, if we do this it will make using borders to the user seem consistent, but it will make our own framework code inconsistent. it is easy enough to forward add(), remove(), size(), iterator() - most others are written in terms of these. however, do we forward visitChildren()? we would have to to make the experience consistent for user, but by doing those we will make our internal framework code inconsistent. for example - our internal framework code now will not see components added to the actual border - just like the client wont. esentially we are creating a portal for the client: OUTSIDE->? >? >BODY and letting user manipulate the body from the outside, skipping the layer in-between. but by making it consistent we also make the layer in-between inaccessible to our own code. so the question is: what has the bigger impact. how many usecases exist where the user needs to treat the border without knowing it is a border vs how many usecases exist where we traverse components without needing to know what kind of component it is....i think the latter are much more prevalent. long story short, i was wrong in WICKET-2494
        Hide
        Martin Grigorov added a comment -

        A test is failing: org.apache.wicket.extensions.markup.html.repeater.data.table.DataTableTest.test_1()

        The problem is that
        org.apache.wicket.extensions.markup.html.repeater.data.table.HeadersToolbar.newSortableHeader(String, String, ISortStateLocator)
        can return a border or non-border and the client code cannot know that it has to use .addToBody() instead of .add().

        Show
        Martin Grigorov added a comment - A test is failing: org.apache.wicket.extensions.markup.html.repeater.data.table.DataTableTest.test_1() The problem is that org.apache.wicket.extensions.markup.html.repeater.data.table.HeadersToolbar.newSortableHeader(String, String, ISortStateLocator) can return a border or non-border and the client code cannot know that it has to use .addToBody() instead of .add().
        Hide
        Martin Grigorov added a comment - - edited

        We have to decide what to do here.
        Removing 'final' from add(), get(), remove(), ... will be the best from user point of view, but it is not the best for the framework.

        For those who don't know what is the problem now: if the method signature says "returns Component" but the implementation actually returns a Border then following calls to component.add() will do the wrong thing, it will add to the border itself, not to its body.

        Maybe AWT is not the best example but there Container class doesn't forbid overriding add(), remove(), ...

        Show
        Martin Grigorov added a comment - - edited We have to decide what to do here. Removing 'final' from add(), get(), remove(), ... will be the best from user point of view, but it is not the best for the framework. For those who don't know what is the problem now: if the method signature says "returns Component" but the implementation actually returns a Border then following calls to component.add() will do the wrong thing, it will add to the border itself, not to its body. Maybe AWT is not the best example but there Container class doesn't forbid overriding add(), remove(), ...
        Hide
        Martin Grigorov added a comment -

        A patch that removes final from add(), remove(), replace, addOrReplace() and size(). This way Border can override them and delegate them to the respective getBodyContainer().xyz().

        All unit tests pass. My application also uses Borders and all seems OK.

        Show
        Martin Grigorov added a comment - A patch that removes final from add(), remove(), replace, addOrReplace() and size(). This way Border can override them and delegate them to the respective getBodyContainer().xyz(). All unit tests pass. My application also uses Borders and all seems OK.
        Hide
        Pedro Santos added a comment -

        +1 for Martin's patch. Some times it is good to look at AWT API because they probably went through component API design questions like us. JFrame for instance required to get the content panel to add a new component in begining [1]. But its JFrame#add/remove/etc/Impl got changed to delegate to content panel.
        frame.getContentPanel().add(newComp) --> frame.add(newCompo)
        I see some similarities here.

        1 - http://weblogs.java.net/blog/hansmuller/archive/2005/11/jframeadd_conte.html

        Show
        Pedro Santos added a comment - +1 for Martin's patch. Some times it is good to look at AWT API because they probably went through component API design questions like us. JFrame for instance required to get the content panel to add a new component in begining [1] . But its JFrame#add/remove/etc/Impl got changed to delegate to content panel. frame.getContentPanel().add(newComp) --> frame.add(newCompo) I see some similarities here. 1 - http://weblogs.java.net/blog/hansmuller/archive/2005/11/jframeadd_conte.html
        Hide
        Igor Vaynberg added a comment -

        giving up on fixing this properly for now. add and remove are now in sync but iterator/size/get are not.

        Show
        Igor Vaynberg added a comment - giving up on fixing this properly for now. add and remove are now in sync but iterator/size/get are not.

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            Antti S. Lankila
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development