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

        Igor Vaynberg made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Igor Vaynberg made changes -
        Assignee Martin Grigorov [ mgrigorov ] Igor Vaynberg [ ivaynberg ]
        Martin Grigorov made changes -
        Attachment wicket-3702.patch [ 12482550 ]
        Martin Grigorov made changes -
        Assignee Igor Vaynberg [ ivaynberg ] Martin Grigorov [ mgrigorov ]
        Martin Grigorov made changes -
        Priority Minor [ 4 ] Major [ 3 ]
        Martin Grigorov made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Igor Vaynberg made changes -
        Fix Version/s 1.5-RC5 [ 12316423 ]
        Igor Vaynberg made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Igor Vaynberg made changes -
        Assignee Martin Grigorov [ mgrigorov ] Igor Vaynberg [ ivaynberg ]
        Igor Vaynberg made changes -
        Assignee Martin Grigorov [ mgrigorov ]
        Igor Vaynberg made changes -
        Resolution Won't Fix [ 2 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Martin Grigorov made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Won't Fix [ 2 ]
        Antti S. Lankila made changes -
        Field Original Value New Value
        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 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.
        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.
        Priority Major [ 3 ] Minor [ 4 ]
        Antti S. Lankila created issue -

          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