Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.2
    • Fix Version/s: 1.5-M1
    • Component/s: wicket
    • Labels:
      None

      Description

      "It would be nice if any component can listen to form's event that signals form components to update their models"
      -There are some components that implemented they input logic using the request parameter (default FormComponet behavior). When Wicket bring those components outside the from to his processing, we get some problems: they will not receive the user input on page by request parameter map.
      Currently on the user mailing list we can see some case:
      http://markmail.org/search/list:org.apache.wicket.users#query:list%3Aorg.apache.wicket.users%20date%3A200909%20from%3A%22Flavius%22+page:1+mid:ykgrynnsyqaum22o+state:results
      If we execute the project sent ed by Flavius, we can see that the request has no parameter to the DropDownChoice component "project", that is on the page and outside the form. The component get his convertInput and updateModel called by the form process. The convertInput call getInputAsArray, that calls:
      String[] values = getRequest().getParameters(getInputName());
      and get null.
      To resolve that problem on wicket trunk, I'm sending a path that remove from the form processing, calls to components outside him(Flavius, maybe you want to apply on your workspace Wicket). But this is only an option. I think Wicket need review the way to process the form:
      1- add inputs from outside form components to request
      2 - remove outside form components from his processing
      3 - any other option

      1. patch3.txt
        1 kB
        Pedro Santos
      2. patch.txt
        0.9 kB
        Pedro Santos
      3. patch.txt
        17 kB
        Pedro Santos
      4. BorderVisit.zip
        66 kB
        Flavius
      5. BaseBorder.java
        8 kB
        Juergen Donnerstag

        Activity

        Hide
        Igor Vaynberg added a comment -

        juergen, does all this special border handling logic still make sense given users should properly put getborderbodycontainer() into the proper parent?

        so if i have

        class myborder extends border {
        public myborder()

        { form f=new form(); add(f); }

        }

        <wicket:border><form><wicket:body/></form></wicket:border>

        this is essentially a user error - incorrect use of border, and is fixed by:

        class myborder extends border {
        public myborder()

        { form f=new form(); add(f); f.add(getborderbodycontainer()); <== align border body with class hierarchy }

        }

        Show
        Igor Vaynberg added a comment - juergen, does all this special border handling logic still make sense given users should properly put getborderbodycontainer() into the proper parent? so if i have class myborder extends border { public myborder() { form f=new form(); add(f); } } <wicket:border><form><wicket:body/></form></wicket:border> this is essentially a user error - incorrect use of border, and is fixed by: class myborder extends border { public myborder() { form f=new form(); add(f); f.add(getborderbodycontainer()); <== align border body with class hierarchy } }
        Hide
        Juergen Donnerstag added a comment -

        Yes it does make sense. Imagine

        <span wicket:id="myBorder">
        <input wicket:id="myInput" .../>
        </span>

        you'll add it like myBorder.add(myInput). It will not be added to the border body container. Only if you would do myBorder.getBorderBodyContainer().add(myInput).

        Show
        Juergen Donnerstag added a comment - Yes it does make sense. Imagine <span wicket:id="myBorder"> <input wicket:id="myInput" .../> </span> you'll add it like myBorder.add(myInput). It will not be added to the border body container. Only if you would do myBorder.getBorderBodyContainer().add(myInput).
        Hide
        Igor Vaynberg added a comment -

        perhaps this is a good example where we can make component#add() nonfinal, and let the border reroute add calls to the bodycontainer while adding an border#addToBorder(component) method which will add components to the border itself.

        the problem is a lot of our code has workarounds that are there just for the border and the impedance mismatch it creates in component hierarchies.

        this is not doable for 1.4.x and there we have to live with the pain, but maybe this is something we can address in 1.5. the only downside is that authors of borders have to know to use #addToBorder instead of the regular #add, but i think that is not too big a price to pay for having clean code with consistent behavior. we can also use good exception messaging to make the users aware of how to properly build borders.

        Show
        Igor Vaynberg added a comment - perhaps this is a good example where we can make component#add() nonfinal, and let the border reroute add calls to the bodycontainer while adding an border#addToBorder(component) method which will add components to the border itself. the problem is a lot of our code has workarounds that are there just for the border and the impedance mismatch it creates in component hierarchies. this is not doable for 1.4.x and there we have to live with the pain, but maybe this is something we can address in 1.5. the only downside is that authors of borders have to know to use #addToBorder instead of the regular #add, but i think that is not too big a price to pay for having clean code with consistent behavior. we can also use good exception messaging to make the users aware of how to properly build borders.
        Hide
        Pedro Santos added a comment - - edited

        The problem is that user can't deliberately add an form component to border and don't get it participating from any border inner form processing.

        Show
        Pedro Santos added a comment - - edited The problem is that user can't deliberately add an form component to border and don't get it participating from any border inner form processing.
        Hide
        Juergen Donnerstag added a comment -

        Applying the patch in 1.4 is a no go. I understand Pedro's use case but applying his fxi/patch does only create new problems.

        I was thinking along the same line as Igor. There are numerous exception in core for Border only and even Border.java itself is rather complicated. All that could be removed, at the cost of introducing addToBorder. I'd suggest to go this route for 1.5

        For 1.4 may be we can provide some BaseBorder class which has all the magic removed and since it is not derived from Border, the exceptions in Form etc would not apply. In 1.5 BaseBorder would become Border and BaseBorder would be removed again.

        Show
        Juergen Donnerstag added a comment - Applying the patch in 1.4 is a no go. I understand Pedro's use case but applying his fxi/patch does only create new problems. I was thinking along the same line as Igor. There are numerous exception in core for Border only and even Border.java itself is rather complicated. All that could be removed, at the cost of introducing addToBorder. I'd suggest to go this route for 1.5 For 1.4 may be we can provide some BaseBorder class which has all the magic removed and since it is not derived from Border, the exceptions in Form etc would not apply. In 1.5 BaseBorder would become Border and BaseBorder would be removed again.
        Hide
        Flavius added a comment -

        I've attached a QuickStart that demonstrates the issue here. This sample code does "work" with wicket 1.3.x. It no longer "works" with 1.4.x. So, in the strictest sense, the changes in 1.4.x eliminated backward compatibility. Now I realize that with migration to a new major version, some refactoring is usually necessary and I don't mind that at all. Even if it was a good amount of refactoring that's fine.

        But with this scenario, no amount of refactoring will allow the Border here with a dropdown to work with the current codebase. To make this work, I've had to go into the Form class and remove the call to internalUpdateFormComponentModels for the Border component. This is not good, but so far it seems to be working in this project without any adverse effects. I haven't done a full regression test yet, though.

        When I move to 1.4.2, and any subsequent 1.4.x build, I'll be forced to do the same. Is there some way the 1.4.3 build can include an "opt out" for the Border to not be visited on form submission? I realize this is no trivial request, but I can't think of any other alternatives, other than altering the Wicket
        source code with each build or abandoning Borders altogether and losing all the benefits gained from that.

        If myBorder.getBorderBodyContainer().add(...) is the right way to do things with a border, I would elect for that route. A javadoc comment can be added to show users how this needs to be done and, as Igor mentioned, a warning message to the dev otherwise can help them

        Show
        Flavius added a comment - I've attached a QuickStart that demonstrates the issue here. This sample code does "work" with wicket 1.3.x. It no longer "works" with 1.4.x. So, in the strictest sense, the changes in 1.4.x eliminated backward compatibility. Now I realize that with migration to a new major version, some refactoring is usually necessary and I don't mind that at all. Even if it was a good amount of refactoring that's fine. But with this scenario, no amount of refactoring will allow the Border here with a dropdown to work with the current codebase. To make this work, I've had to go into the Form class and remove the call to internalUpdateFormComponentModels for the Border component. This is not good, but so far it seems to be working in this project without any adverse effects. I haven't done a full regression test yet, though. When I move to 1.4.2, and any subsequent 1.4.x build, I'll be forced to do the same. Is there some way the 1.4.3 build can include an "opt out" for the Border to not be visited on form submission? I realize this is no trivial request, but I can't think of any other alternatives, other than altering the Wicket source code with each build or abandoning Borders altogether and losing all the benefits gained from that. If myBorder.getBorderBodyContainer().add(...) is the right way to do things with a border, I would elect for that route. A javadoc comment can be added to show users how this needs to be done and, as Igor mentioned, a warning message to the dev otherwise can help them
        Hide
        Igor Vaynberg added a comment -

        what about WICKET-2027 ?

        this is something that has been working since 1.4-rc2, we cannot break it in 1.4.x.

        Show
        Igor Vaynberg added a comment - what about WICKET-2027 ? this is something that has been working since 1.4-rc2, we cannot break it in 1.4.x.
        Hide
        Flavius added a comment -

        That's a good point, Igor (regarding WICKET-2027).

        Juergen, I like your idea and I think that would work for me. I could instantiate BaseBorder instead of Border. That would keep everything in 1.4.x working as is and it would essentially solve my dilemma. I presume then that BaseBorder would not visit the components in the Border (or it's children) on a form submission.

        Thinking out loud, what about a member var in Border called visitComponentsOnFormSubmit or something like that?

        Add a method like
        public Border setVisitComponentsOnFormSubmit(boolean visitComponents)

        { this.visitComponentsOnFormSubmit = visitComponents; return this; }

        Then the Border can check this flag to see if it should visit the Border's components.

        Either way, I think this works. Thanks guys, for your help.

        Show
        Flavius added a comment - That's a good point, Igor (regarding WICKET-2027 ). Juergen, I like your idea and I think that would work for me. I could instantiate BaseBorder instead of Border. That would keep everything in 1.4.x working as is and it would essentially solve my dilemma. I presume then that BaseBorder would not visit the components in the Border (or it's children) on a form submission. Thinking out loud, what about a member var in Border called visitComponentsOnFormSubmit or something like that? Add a method like public Border setVisitComponentsOnFormSubmit(boolean visitComponents) { this.visitComponentsOnFormSubmit = visitComponents; return this; } Then the Border can check this flag to see if it should visit the Border's components. Either way, I think this works. Thanks guys, for your help.
        Hide
        Pedro Santos added a comment -

        I agree with Juergen. I came up with an implementation idea (on path)

        • Rename Border to AbstractBorder or BaseBorder
        • Create a Border subclass from it
        • create a javadoc explaining what it means
        Show
        Pedro Santos added a comment - I agree with Juergen. I came up with an implementation idea (on path) Rename Border to AbstractBorder or BaseBorder Create a Border subclass from it create a javadoc explaining what it means
        Hide
        Pedro Santos added a comment - - edited

        Actually any implementation idea on that way (create another border ) implies in some other things. For example, we need to define if an markup file is required for BaseBorder.
        I give a test on the Flavius project with new BaseBorder, and came up altering MarkupFragmentFinder to don't require markup files for BaseBorder component (path 3)

        Show
        Pedro Santos added a comment - - edited Actually any implementation idea on that way (create another border ) implies in some other things. For example, we need to define if an markup file is required for BaseBorder. I give a test on the Flavius project with new BaseBorder, and came up altering MarkupFragmentFinder to don't require markup files for BaseBorder component (path 3)
        Hide
        Juergen Donnerstag added a comment -

        The attached BaseBorder passes all Border junit tests, of course after making changes related to getBorderBody(), without any modification to any other core class. So you can simply drop it in your application, replace Border with this new class where necessary and should be fine.

        Show
        Juergen Donnerstag added a comment - The attached BaseBorder passes all Border junit tests, of course after making changes related to getBorderBody(), without any modification to any other core class. So you can simply drop it in your application, replace Border with this new class where necessary and should be fine.
        Hide
        Juergen Donnerstag added a comment -

        I just added to 1.5 the BaseBorder and a modified version of Border which now derives from BaseBorder. I copied the Border test case for BaseBorder and made them work with changes necessary related to getBorderBodyContainer(), addToBody() etc.

        Show
        Juergen Donnerstag added a comment - I just added to 1.5 the BaseBorder and a modified version of Border which now derives from BaseBorder. I copied the Border test case for BaseBorder and made them work with changes necessary related to getBorderBodyContainer(), addToBody() etc.
        Hide
        Juergen Donnerstag added a comment -

        A component has been provided for 1.4, which is not included in 1.4 source tree, but which has been added to 1.5.

        Show
        Juergen Donnerstag added a comment - A component has been provided for 1.4, which is not included in 1.4 source tree, but which has been added to 1.5.
        Hide
        Igor Vaynberg added a comment -

        so does this mean for 1.5 you will be removing all the special handling all over the code related to borders?

        Show
        Igor Vaynberg added a comment - so does this mean for 1.5 you will be removing all the special handling all over the code related to borders?
        Hide
        Juergen Donnerstag added a comment -

        We could do, but than every application with a border needs modification.

        Show
        Juergen Donnerstag added a comment - We could do, but than every application with a border needs modification.
        Hide
        Igor Vaynberg added a comment -

        i think that is ok. 1.5 will have api breaks anyways so this can be one of the migration things. it would be great to finally have a properly working border implementation though, because the current mess takes up a lot of support time.

        Show
        Igor Vaynberg added a comment - i think that is ok. 1.5 will have api breaks anyways so this can be one of the migration things. it would be great to finally have a properly working border implementation though, because the current mess takes up a lot of support time.
        Hide
        Flavius added a comment -

        Thanks for your help, Juergen.

        I just dropped the BaseBorder.java you attached here into the BorderVisit quickstart I attached earlier. It's messing
        up the hierarchy on all the pages. In the project I simply made BorderMenu extend this BaseBorder instead of
        org.apache.wicket.markup.html.border.Border

        Show
        Flavius added a comment - Thanks for your help, Juergen. I just dropped the BaseBorder.java you attached here into the BorderVisit quickstart I attached earlier. It's messing up the hierarchy on all the pages. In the project I simply made BorderMenu extend this BaseBorder instead of org.apache.wicket.markup.html.border.Border
        Hide
        Juergen Donnerstag added a comment -

        MarkupContainer.add(child) is final for now. I think it would be nice if Border.add() would actually add to the body and addToBorder() would add components to <wicket:border>...</wicket:border>. I know we had this discussion on the list before to remove final from add(). What do you think?

        Show
        Juergen Donnerstag added a comment - MarkupContainer.add(child) is final for now. I think it would be nice if Border.add() would actually add to the body and addToBorder() would add components to <wicket:border>...</wicket:border>. I know we had this discussion on the list before to remove final from add(). What do you think?
        Hide
        Igor Vaynberg added a comment -

        i disagree. if border.add adds to border itself and not to the bodycontainer then you cannot treat a border as a regular component downstream, you have to know its a border and use addtobody method.

        i think only the person authoring the border should have to deal with addtoborder method, and everyone downstream should not have to know anything about it. i am guessing the authoring to usage ration is pretty low, so we should make it as easy as possible for the users of the borders.

        Show
        Igor Vaynberg added a comment - i disagree. if border.add adds to border itself and not to the bodycontainer then you cannot treat a border as a regular component downstream, you have to know its a border and use addtobody method. i think only the person authoring the border should have to deal with addtoborder method, and everyone downstream should not have to know anything about it. i am guessing the authoring to usage ration is pretty low, so we should make it as easy as possible for the users of the borders.
        Hide
        Juergen Donnerstag added a comment -

        That is what I said, didn't I?

        Show
        Juergen Donnerstag added a comment - That is what I said, didn't I?

          People

          • Assignee:
            Juergen Donnerstag
            Reporter:
            Pedro Santos
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development