On Tue, Oct 27, 2015 at 4:25 PM, andrea del bene <[email protected]> wrote:
> > > On 26/10/2015 13:11, Martin Grigorov wrote: > >> Hi Andrea, >> >> On Mon, Oct 26, 2015 at 2:02 PM, <[email protected]> wrote: >> >> Repository: wicket >>> Updated Branches: >>> refs/heads/wicket-7.x 92e9d6dd1 -> f0364dad0 >>> >>> >>> improved WICKET-5988 relaying on component resolvers >>> >>> Please do not make changes to already released tickets. WICKET-5988 is >> part >> of 7.1.0. >> Better create a new ticket and explain what is the new problem and what is >> the improvement >> > The rational behind this change is that the previous fix was more a > workaround for WICKET-5988 while this new one is based on something that is > already in place, that is the component-resolving capability of Border. It doesn't matter. Since WICKET-5988 is closed and part of a release already new commits should use a new ticket. If a user finds a regression in 7.2.0 (and there is a regression! see below) then it is impossible to see a related ticket in the changelog > > > >> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/f0364dad >>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/f0364dad >>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/f0364dad >>> >>> Branch: refs/heads/wicket-7.x >>> Commit: f0364dad0db9d1d3b49891ca9a49662a4e69e378 >>> Parents: 92e9d6d >>> Author: Andrea Del Bene <[email protected]> >>> Authored: Mon Oct 26 13:00:53 2015 +0100 >>> Committer: Andrea Del Bene <[email protected]> >>> Committed: Mon Oct 26 13:02:27 2015 +0100 >>> >>> ---------------------------------------------------------------------- >>> .../wicket/markup/html/border/Border.java | 27 >>> +++++++++----------- >>> .../form/feedback/FeedbackFormPage_result2.html | 2 +- >>> 2 files changed, 13 insertions(+), 16 deletions(-) >>> ---------------------------------------------------------------------- >>> >>> >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/f0364dad/wicket-core/src/main/java/org/apache/wicket/markup/html/border/Border.java >>> ---------------------------------------------------------------------- >>> diff --git >>> >>> a/wicket-core/src/main/java/org/apache/wicket/markup/html/border/Border.java >>> >>> b/wicket-core/src/main/java/org/apache/wicket/markup/html/border/Border.java >>> index 766d296..f0d5412 100644 >>> --- >>> >>> a/wicket-core/src/main/java/org/apache/wicket/markup/html/border/Border.java >>> +++ >>> >>> b/wicket-core/src/main/java/org/apache/wicket/markup/html/border/Border.java >>> @@ -166,7 +166,7 @@ public abstract class Border extends >>> WebMarkupContainer implements IComponentRes >>> super(id, model); >>> >>> body = new BorderBodyContainer(id + "_" + BODY); >>> - addToBorder(body); >>> + //addToBorder(body); >>> >>> This line should be removed, not just commented out. >> > right > >> >> >> } >>> >>> /** >>> @@ -205,7 +205,17 @@ public abstract class Border extends >>> WebMarkupContainer implements IComponentRes >>> @Override >>> public Border add(final Component... children) >>> { >>> - getBodyContainer().add(children); >>> + for (Component component : children) >>> + { >>> + if (component.equals(body)) >>> >>> remove() uses identity check: >> >> https://github.com/apache/wicket/blob/master/wicket-core/src/main/java/org/apache/wicket/markup/html/border/Border.java#L222 >> >> agree. I should be coherent with the existing code. > >> + { >>> + addToBorder(component); >>> + } >>> + else >>> + { >>> + getBodyContainer().add(component); >>> + } >>> + } >>> >>> This change looks wrong. >> The contract is: >> - add() adds to the border's body. >> - addToBorder() adds to the border itself >> >> All other methods like: replace(), remove(), addOrReplace(), etc. should >> behave as add(), i.e. they should manipulate the border's body. >> > I don't see the problem. remove() has exactly the same behaviour. What's > wrong if we handle body component in a different way? The problem I see is that now addOrReplace() doesn't behave as #add(). Nor #replace(), nor #remove(String). All of them should have the same checks inside. Border.java is the main reason why #add(), #replace() and others are no more 'final' methods in MarkupContainer. If Wicket-core cannot make sure that it overrides them consistently then we cannot expect that the applications will do it properly. > > >> return this; >>> } >>> >>> @@ -631,17 +641,4 @@ public abstract class Border extends >>> WebMarkupContainer implements IComponentRes >>> return borderMarkup != null ? borderMarkup : markup; >>> } >>> >>> - @Override >>> - protected void onBeforeRender() >>> - { >>> - super.onBeforeRender(); >>> - /** >>> - * https://issues.apache.org/jira/browse/WICKET-5981 >>> - * dequeue border to adjust children hierarchy. >>> - */ >>> - if (!hasBeenRendered()) >>> - { >>> - dequeue(); >>> - } >>> - } >>> } >>> >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/f0364dad/wicket-core/src/test/java/org/apache/wicket/markup/html/form/feedback/FeedbackFormPage_result2.html >>> ---------------------------------------------------------------------- >>> diff --git >>> >>> a/wicket-core/src/test/java/org/apache/wicket/markup/html/form/feedback/FeedbackFormPage_result2.html >>> >>> b/wicket-core/src/test/java/org/apache/wicket/markup/html/form/feedback/FeedbackFormPage_result2.html >>> index da7d6b2..ced751f 100644 >>> --- >>> >>> a/wicket-core/src/test/java/org/apache/wicket/markup/html/form/feedback/FeedbackFormPage_result2.html >>> +++ >>> >>> b/wicket-core/src/test/java/org/apache/wicket/markup/html/form/feedback/FeedbackFormPage_result2.html >>> @@ -1,6 +1,6 @@ >>> <html> >>> <body> >>> -<form wicket:id="form" id="form1" method="post" >>> >>> action="./org.apache.wicket.markup.html.form.feedback.FeedbackFormPage?1-2.IFormSubmitListener-form"><div >>> >>> style="width:0px;height:0px;position:absolute;left:-100px;top:-100px;overflow:hidden"><input >>> type="hidden" name="form1_hf_0" id="form1_hf_0" /></div> >>> +<form wicket:id="form" id="form1" method="post" >>> >>> action="./org.apache.wicket.markup.html.form.feedback.FeedbackFormPage?0-2.IFormSubmitListener-form"><div >>> >>> style="width:0px;height:0px;position:absolute;left:-100px;top:-100px;overflow:hidden"><input >>> type="hidden" name="form1_hf_0" id="form1_hf_0" /></div> >>> <span wicket:id="feedback"><wicket:border> >>> <wicket:body> >>> <input type="text" wicket:id="input" value="" >>> name="feedback:feedback_body:input"> >>> >>> >>> IMO the previous version of the fix was better. >> > As I said, the previous solution was more a workaround rather than a real > solution. But that's my opinion :-) > > Well, it was more clear to me what happens. If the new version is fixed and some documentation (javadoc) is added then I'll be happier.
