You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "Eelco Hillenius (JIRA)" <ji...@apache.org> on 2007/08/12 10:05:43 UTC
[jira] Created: (WICKET-839) FormComponent#checkRequired needs
various improvement
FormComponent#checkRequired needs various improvement
-----------------------------------------------------
Key: WICKET-839
URL: https://issues.apache.org/jira/browse/WICKET-839
Project: Wicket
Issue Type: Bug
Affects Versions: 1.3.0-beta2
Reporter: Eelco Hillenius
Assignee: Eelco Hillenius
Fix For: 1.3.0-beta3
Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. I believe the check should actually be done before the method is called, so that people who override this method don't have to think about checking isRequired first.
Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
public boolean checkRequired()
{
return isRequired() ? left.setRequired(true).checkRequired() &&
right.setRequired(true).checkRequired() : true;
}
which is a pretty ugly hack, while after the change, it can be coded like this:
public boolean checkRequired()
{
return left.checkRequired() && right.checkRequired();
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519371 ]
Eelco Hillenius commented on WICKET-839:
----------------------------------------
Well yeah, what you're proposing is bending the names so that it fits your ideas :). If we would convert to these names, yes I think I could agree, though you'd still have to give me a *good* solution for formcomponentpanels (WICKET-840).
Let's turn this around and let me explain what *my* interpretation is:
#setRequired: the component requires user-input. I actually doubt how obvious is it is to people that a converter may return a null from not-null input and as the required check is done before that, the null may pass.
#validateRequired: if the required flag is true, check whether the form component received user input, and if it didn't, report an error (#reportRequiredError)
#checkRequired: (after my change, but like I argued before for the better regardless of what formcomponentpanel does). Check that the component received the user input it thinks it minimally needs. For most form components this means that it checks the input directly as request parameters/ raw input. But for DateTimePanel for instance, the check should pass if there is any input for the date text field; the time- and AM/PM fields can be ignored.
I think this makes perfect sense, and the fact that the check on whether the required flag was set is moved to validateRequired is only for the better, as checkRequired can now be implemented without having to think about doing that check first. I am in favor this change regardless of WICKET-840, especially since checkRequired in it's old state was definitively not something clients would call themselves.
Now, to get back to WICKET-840, let's take the example of DateTimeField in mind again. Before this change, the required check (if the flag was set) would first always fail, and after your check always pass. I hope you agree neither one is right. With this change, it is easy to implement the check. Not only that, as I added #checkRequired as an abstract method in FormComponentPanel, I *force* implementations to provide a meaningful implementation. And as long as we support setting the required flag on FormComponentPanel - which I think we should, as the child components are hidden for clients - we should make sure they will provide an implementation that makes sense.
An alternative would have been to make the required check conditional, which would be implemented in some way in the visitor that does the validation. Though probably possible, that seemed over-engineered to me, and it wouldn't make implementing custom checks any easier.
So what we have now is a change that didn't break anything, unless in were calling checkRequired directly, depending on the isRequired being done in that method. In the very rare case anyone was doing that rather then letting Wicket do the work or calling validateRequired, the fix is still easy. So I see nothing to get upset about. What we gained with this change and that of WICKET-840 is that FomComponentPanels can/ must now support the required flag. Implementation is straightforward, as it can simply delegate the check to the child components it thinks should get input if the required flag is set to true. Like DateTimeField:
public boolean checkRequired() { return dateField.checkRequired(); }
Child fields can be added without the required flag set (unless of course the panel decides it should always get input, no matter what it's flag says), so checkRequired will only be called by the overriding implementation.
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Igor Vaynberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519322 ]
Igor Vaynberg commented on WICKET-839:
--------------------------------------
>> left.setrequired() and right.setrequired() shouldve been called in the constructor.
>No. They are nested in another component (the formcomponentpanel), which may or may not be required. If that component is not required, >those checks should not be executed. If it is, how are you gonna do those checks? You can only do that in the way I showed. You can't >override setRequired as it is final, and that would have been an ugly hack to start with.
then they should pull
add(new Whatever("left") { boolean isRequired() { return fcp.isrequired(); }}
>> left.checkrequried(), right.checkrequired(), formcomponentpanel.checkrequired()
>No, the whole point is that FormComponentPanel in this case should decide whether to execute the required check on is kids directly. So >left.checkRequired and right.checkRequired should not be called by default (unless the panel already decided that they required in any >case) but instead the panel should decide.
well, the panel doesnt decide - that workflow is not in place. what i am saying is that by the time fcp's checkrequired is called - all of the children's checkrequired() has already been called. fcp is not a "border" for its children in the way that it decides their required attribute, instead they should poll it inside their isrequired() methods. so doing that left.setrequired().checkrequired() inside fcp's checkrequired() is imho hacky.
>> so in this case formcomponentpanel.checkrequired() should simply return true - in fact i think that should just be the default >>implementation of checkrequried for fcp, no need to make it abstract.
>No. I got here because of a bug report on the list about DateField not doing a proper required check. This was because such a component >does not receive direct input, but instead should delegate that check to nested components that do (the date text field in this case). But it >should only do that when it's required flag is set.
well and i fixed that bug by simple making checkrequired() always return true by providing fake input.
>Now this implementation is this:
>public boolean checkRequired()
>{
well, by this time datefield.checkrequired() wouldve been already called anyways.
>return dateField.checkRequired();
>}
>Finally, I want to force people to implement checkRequired as it is something we just cannot provide a meaningful default implementation >for. It is important they think about the implications of the required flag on their custom form and provide an appropriate check.
we dont need to provide it. it is like you said - the formcomponentpanel doesnt have its own input - that is what the checkrequired check is for! so since it doesnt have its own input then checkrequired should always pass.
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (WICKET-839) Make FormComponent#setRequired
non-final
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Eelco Hillenius resolved WICKET-839.
------------------------------------
Resolution: Won't Fix
> Make FormComponent#setRequired non-final
> ----------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Eelco Hillenius updated WICKET-839:
-----------------------------------
Description:
Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
public boolean checkRequired()
{
return isRequired() ? left.setRequired(true).checkRequired() &&
right.setRequired(true).checkRequired() : true;
}
which is a pretty ugly hack, while after the change, it can be coded like this:
public boolean checkRequired()
{
return left.checkRequired() && right.checkRequired();
}
was:
Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. I believe the check should actually be done before the method is called, so that people who override this method don't have to think about checking isRequired first.
Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
public boolean checkRequired()
{
return isRequired() ? left.setRequired(true).checkRequired() &&
right.setRequired(true).checkRequired() : true;
}
which is a pretty ugly hack, while after the change, it can be coded like this:
public boolean checkRequired()
{
return left.checkRequired() && right.checkRequired();
}
Summary: FormComponent#checkRequired should implement only the input check (was: FormComponent#checkRequired needs various improvement)
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519321 ]
Eelco Hillenius commented on WICKET-839:
----------------------------------------
> left.setrequired() and right.setrequired() shouldve been called in the constructor.
No. They are nested in another component (the formcomponentpanel), which may or may not be required. If that component is not required, those checks should not be executed. If it is, how are you gonna do those checks? You can only do that in the way I showed. You can't override setRequired as it is final, and that would have been an ugly hack to start with.
> left.checkrequried(), right.checkrequired(), formcomponentpanel.checkrequired()
No, the whole point is that FormComponentPanel in this case should decide whether to execute the required check on is kids directly. So left.checkRequired and right.checkRequired should not be called by default (unless the panel already decided that they required in any case) but instead the panel should decide.
> so in this case formcomponentpanel.checkrequired() should simply return true - in fact i think that should just be the default implementation of checkrequried for fcp, no need to make it abstract.
No. I got here because of a bug report on the list about DateField not doing a proper required check. This was because such a component does not receive direct input, but instead should delegate that check to nested components that do (the date text field in this case). But it should only do that when it's required flag is set.
Now this implementation is this:
public boolean checkRequired()
{
return dateField.checkRequired();
}
without this change it would be:
public boolean checkRequired()
{
if (isRequired())
{
dateField.setRequired(true);
return dateField.checkRequired();
}
return true;
}
or we would have to open up setRequired so that it would set the required bit on the text field. Both not very good solutions (the latter is not good as it is more limited, and people will probably often forget about it).
Finally, I want to force people to implement checkRequired as it is something we just cannot provide a meaningful default implementation for. It is important they think about the implications of the required flag on their custom form and provide an appropriate check.
Finally finally, I was just surprised to see the isRequired check in this method rather then in validateRequired where it is much better placed. It makes sense to have a method that only does the check whether it has input.
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Reopened: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Igor Vaynberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Igor Vaynberg reopened WICKET-839:
----------------------------------
i dont see whats going on here....
left.setrequired() and right.setrequired() shouldve been called in the constructor.
the iteration order of things has changed so it is actuall
left.checkrequried(), right.checkrequired(), formcomponentpanel.checkrequired()
so in this case formcomponentpanel.checkrequired() should simply return true - in fact i think that should just be the default implementation of checkrequried for fcp, no need to make it abstract.
or did i miss something?
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519372 ]
Eelco Hillenius commented on WICKET-839:
----------------------------------------
Ok, let's look for a third solution then.
One thing I tried to get around was opening up setRequired. I guess that wasn't the best decission as one thing that's not good about my fix is that the messages are done for the top component, while you probably typically want to have messages done via the components that should receive input.
So, I can roll back those changes and make setRequired non-final instead. I still think moving that check was for the better, but I don't care enough about it to go on with this since it doesn't fix anything concrete.
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (WICKET-839) Make FormComponent#setRequired
non-final
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Eelco Hillenius updated WICKET-839:
-----------------------------------
Summary: Make FormComponent#setRequired non-final (was: FormComponent#checkRequired should implement only the input check)
Changed the header so that it will properly display in the change list.
> Make FormComponent#setRequired non-final
> ----------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Eelco Hillenius resolved WICKET-839.
------------------------------------
Resolution: Fixed
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519331 ]
Eelco Hillenius commented on WICKET-839:
----------------------------------------
I thought about it some more, and I really believe we should keep the change as I did it.
Introducing hasInput might pull us from deadlock, but it doesn't communicate it's intended use very good, as just looking at the method I'd expect it to return whether there was any user input. If you look at DateTimeField for instance, when that component has the required flag set to true, it should check whether it has a value for the date field and should ignore whether the time fields had any input in it's evaluation.
Also, having the old version of checkRequired public doesn't make much sense, whereas having that same method that only does the check *does* make sense as it can be used like I showed above.
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Reopened: (WICKET-839) Make FormComponent#setRequired
non-final
Posted by "Igor Vaynberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Igor Vaynberg reopened WICKET-839:
----------------------------------
i dont like opening up setrequired either. my solution doesnt require any of this, so lets see if we can discuss it some more. i really dont mean to be such a pita - but i do think what i am proposing is better.
i moved this discussion to the list cause its hard to quote things here.
> Make FormComponent#setRequired non-final
> ----------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (WICKET-839) Make FormComponent#setRequired
non-final
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Eelco Hillenius resolved WICKET-839.
------------------------------------
Resolution: Fixed
> Make FormComponent#setRequired non-final
> ----------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Igor Vaynberg (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519362 ]
Igor Vaynberg commented on WICKET-839:
--------------------------------------
i dont like your fix because it really doesnt make sense - in fact my hack made more sense, let me try to explain in more detail:
formcomponentpanel does not aggregate RAW input from its children - it simply makes no sense - what it aggregates is the CONVERTED/processed input of its children - and it combines that into one object.
the problem is that the naming is bad. in fact - we have two required checks right now. one is for the raw input and happens before type conversion, the other one is for the converted input. for the converted input checkrequired() is never called. this is needed because it is possible that some entered input might get converted into null by the supplied converter.
i think what you are doing is pigeonholing something into that check that shouldnt be there - and i think part of that is because names are bad as they are right now - and that is partly because we try to preserve the api as much as possible. what my hack did, and why it makes more sense to me, is effectively disable that check for formcomponentpanel.
so here is my take on what the names should be and how it should work:
getinputasarray() -> getrawinput() whatever is getrawinput() right now should be merged into getinputasarray()
this change makes sense because that is what inputasarray() gets - the raw input submitted for that component.
validaterequired() -> validaterequiredrawinput()
checkrequired() -> either merged into validaterequiredinput() or ->checkrequiredrawinput()
see, given these names it would totally make sense that formcomponentpanel.validaterequiredrawinput() always returns true - because like you said yourself: it has no raw input of its own.
its final required check will be performed in FormCOmponent#validate() on the following lines
convertInput();
if (isValid() && isRequired() && getConvertedInput() == null && isInputNullable())
{
reportRequiredError();
}
which should be the only valid required check for the fcp. makes more sense now?
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519327 ]
Eelco Hillenius commented on WICKET-839:
----------------------------------------
I can live with this:
/**
* Checks if the form component's 'required' requirement is met.
*
* @return true if the 'required' requirement is met, false otherwise
*/
public boolean checkRequired()
{
if (isRequired())
{
return hasInput();
}
return true;
}
/**
* Checks whether this component recieved user input.
*
* @return
*/
public boolean hasInput()
{
final String input = getInput();
// when null, check whether this is natural for that component, or
// whether - as is the case with text fields - this can only happen
// when the component was disabled
if (input == null && !isInputNullable())
{
// this value must have come from a disabled field
// do not perform validation
return true;
}
// peform validation by looking whether the value is null or empty
return !Strings.isEmpty(input);
}
which effectively rolls back the change you seem to have a problem with, and give me what I want as well (a method that says whether the component has input).
FormComponentPanel (which like I said should be abstract to start with as it is a panel without a markup file) can then either have hasInput implemented default returning true or defined as abstract (I'm much in favor for the latter).
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WICKET-839) FormComponent#checkRequired should
implement only the input check
Posted by "Eelco Hillenius (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519325 ]
Eelco Hillenius commented on WICKET-839:
----------------------------------------
I don't get your problem with all this. For starters, the implementation of checkRequired is just more sane as it does just the check, not whether it should do the check in the first place. That alone warrants the change. I thought about renaming the method to hasInput... that's probably a good idea still. We currently have validateRequired and checkRequired: I hope you'll agree that that's not too pretty to start with.
> well and i fixed that bug by simple making checkrequired() always return true by providing fake input.
No, you didn't fix it, you provided a workaround. I fixed it, as my implementation now returns whether there was (required) input. Again, we should rename the method to hasInput.
> we dont need to provide it. it is like you said - the formcomponentpanel doesnt have its own input - that is what the checkrequired check is for!
Conceptually it has. It just gathers input from children. It has that required flag, and the whole point of these fixes is to properly support that.
> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
> Key: WICKET-839
> URL: https://issues.apache.org/jira/browse/WICKET-839
> Project: Wicket
> Issue Type: Bug
> Affects Versions: 1.3.0-beta2
> Reporter: Eelco Hillenius
> Assignee: Eelco Hillenius
> Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> public boolean checkRequired()
> {
> return isRequired() ? left.setRequired(true).checkRequired() &&
> right.setRequired(true).checkRequired() : true;
> }
> which is a pretty ugly hack, while after the change, it can be coded like this:
> public boolean checkRequired()
> {
> return left.checkRequired() && right.checkRequired();
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.