You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Igor Vaynberg <ig...@gmail.com> on 2007/08/13 08:09:46 UTC
formcomponentpanel.checkrequred
i am moving this to the list because it is easier to quote here
interested parties see WICKET-839 for pretext
--- from eelco ---
>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).
i am not bending anything - they are what they should be. we didnt change
them for api compatibility. the simple fact remains that there are TWO
required checks. one before type conversion and one after.
>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.
well. it doesnt matter how obvious it is to people. the matter is that it IS
possible ( i have hit a couple of usecases like that ) and we do need to
support the case. although up to recently we havent. we cannot always assume
that any non-empty string entered is converted to a non-null object.
>#validateRequired: if the required flag is true, check whether the form
component received user input, and if it didn't, report an error
>(#reportRequiredError)
define user input. this is the problem - you are only talking about the raw
input. but what about null after type conversion? that null is STILL user
input - just typeconverted.
>#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.
again - the problem here is that you are only operating on the raw input. we
cannot fully perform the validateRequried()/checkRequired() until after type
conversion. it does need to be done in two places - so that all type
converters do not have to deal with null input. the flipside of the coin is
that we do not support required attribute for fields that can convert a null
raw input to a non-null object through type conversion - but i think its ok.
>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.
great, so datefield { isrequired() { return fcp.isrequired()}}, the other
fields are simply not required. everything works without any magic.
>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.
yes, that is fine.
>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.
no i do not agree, that is what i have been trying to say all along! my hack
made the check always pass because this check for RAW input does not apply
to the fcp - IT HAS NO RAW INPUT. you have pointed this out yourself. i dont
really understand, if i call datetimepanel.getInputAsArray() will i get back
anything useful?
>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.
with my idea you dont even need to provide any implementation of this. you
simply make required child component's isrequired() echo the fcp's. it makes
sense to link them like that.
>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.
no, way too overcomplicated for something so simple.
>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.
but this is so ugly from the api standpoint. instead of polling you are
pushing the required check. also in this case datefield.checkrequired() is
called twice - once when the component is not required and once when it is,
because remember we iterate postorder so first datefield.validaterequired()
is called, then the datetimepanel.validaterequired().
-igor
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
On 8/12/07, Igor Vaynberg <ig...@gmail.com> wrote:
> i am moving this to the list because it is easier to quote here
Sorry to cut the discussion short, but I'm really done with this. I
spent hours on this today, while all I wanted was to do a fix. I know
we're not living in a perfect world, and we keep improving things,
etc, etc, but I was fixing an actual problem, which you hold back
without providing a good alternative. If you fix DateTimeField/
DateField so that they will react properly on setRequired in a way
that is simple and short (as it is now), please go for it. I'm gonna
go back fighting that stack of deadlines again.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Matej Knopp <ma...@gmail.com>.
You can take a look at INullAcceptingValidator, it might do what you are after
-Matej
On 11/1/07, Johan Compagner <jc...@gmail.com> wrote:
> we had that and we changed it to a boolean because required is a thing
> that must be validated before converters and other validators
>
> On 11/1/07, Bruno Borges <br...@gmail.com> wrote:
> > isn't this a reason to start with that talk again Igor, about replacing
> > setRequired with an IValidator?
> >
> > regards,
> > bruno
> >
> > Igor Vaynberg wrote:
> > > On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> > >
> > >>>> You are just nitpicking. 99.9% of people won't override setRequired to
> > >>>> start with.
> > >>>>
> > >>> i dont know about that. seems to me that the majority of people who
> > >>> implement a FormComponentPanel will be forced to overwrite it since that
> > >>>
> > >> is
> > >>
> > >>> currently the model you have put in place.
> > >>>
> > >> Yeah, they should, as they probably didn't support the required flag
> > >> properly before.
> > >>
> > >
> > >
> > > they did just fine after my "hack"
> > >
> > >
> > >> i had to do it for
> > >>
> > >>> formcomponentpanels in my projects, and i really dont like the code it
> > >>> causes.
> > >>>
> > >> You had to? Well that's interesting. How did you fix this before this
> > >> change then?
> > >>
> > >
> > >
> > > with my "hack" i had it in my own wicket branch.
> > >
> > > The situation is *exactly like it was before this whole discussion*
> > >
> > >> with the exception that setRequired isn't final anymore, so that I can
> > >> fix the components I'm interested in in a way I think works good. I'm
> > >> pretty sure the fact that setRequired isn't final anymore won't wreak
> > >> havoc.
> > >>
> > >
> > >
> > > yes but pushing the required state is ugly, not to mention all the
> > > noobishess that will come with opening it!
> > >
> > > -igor
> > >
> > >
> > > Eelco
> > >
> > >
> > >
> >
> >
>
Re: formcomponentpanel.checkrequred
Posted by Johan Compagner <jc...@gmail.com>.
we had that and we changed it to a boolean because required is a thing
that must be validated before converters and other validators
On 11/1/07, Bruno Borges <br...@gmail.com> wrote:
> isn't this a reason to start with that talk again Igor, about replacing
> setRequired with an IValidator?
>
> regards,
> bruno
>
> Igor Vaynberg wrote:
> > On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> >
> >>>> You are just nitpicking. 99.9% of people won't override setRequired to
> >>>> start with.
> >>>>
> >>> i dont know about that. seems to me that the majority of people who
> >>> implement a FormComponentPanel will be forced to overwrite it since that
> >>>
> >> is
> >>
> >>> currently the model you have put in place.
> >>>
> >> Yeah, they should, as they probably didn't support the required flag
> >> properly before.
> >>
> >
> >
> > they did just fine after my "hack"
> >
> >
> >> i had to do it for
> >>
> >>> formcomponentpanels in my projects, and i really dont like the code it
> >>> causes.
> >>>
> >> You had to? Well that's interesting. How did you fix this before this
> >> change then?
> >>
> >
> >
> > with my "hack" i had it in my own wicket branch.
> >
> > The situation is *exactly like it was before this whole discussion*
> >
> >> with the exception that setRequired isn't final anymore, so that I can
> >> fix the components I'm interested in in a way I think works good. I'm
> >> pretty sure the fact that setRequired isn't final anymore won't wreak
> >> havoc.
> >>
> >
> >
> > yes but pushing the required state is ugly, not to mention all the
> > noobishess that will come with opening it!
> >
> > -igor
> >
> >
> > Eelco
> >
> >
> >
>
>
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
how would a validator work better here then a boolean?
-igor
On 11/1/07, Bruno Borges <br...@gmail.com> wrote:
> isn't this a reason to start with that talk again Igor, about replacing
> setRequired with an IValidator?
>
> regards,
> bruno
>
> Igor Vaynberg wrote:
> > On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> >
> >>>> You are just nitpicking. 99.9% of people won't override setRequired to
> >>>> start with.
> >>>>
> >>> i dont know about that. seems to me that the majority of people who
> >>> implement a FormComponentPanel will be forced to overwrite it since that
> >>>
> >> is
> >>
> >>> currently the model you have put in place.
> >>>
> >> Yeah, they should, as they probably didn't support the required flag
> >> properly before.
> >>
> >
> >
> > they did just fine after my "hack"
> >
> >
> >> i had to do it for
> >>
> >>> formcomponentpanels in my projects, and i really dont like the code it
> >>> causes.
> >>>
> >> You had to? Well that's interesting. How did you fix this before this
> >> change then?
> >>
> >
> >
> > with my "hack" i had it in my own wicket branch.
> >
> > The situation is *exactly like it was before this whole discussion*
> >
> >> with the exception that setRequired isn't final anymore, so that I can
> >> fix the components I'm interested in in a way I think works good. I'm
> >> pretty sure the fact that setRequired isn't final anymore won't wreak
> >> havoc.
> >>
> >
> >
> > yes but pushing the required state is ugly, not to mention all the
> > noobishess that will come with opening it!
> >
> > -igor
> >
> >
> > Eelco
> >
> >
> >
>
>
Re: formcomponentpanel.checkrequred
Posted by Martijn Dashorst <ma...@gmail.com>.
To elaborate: it is now part of 1.3, the book and about 1000 web applications.
Martijn
On 11/1/07, Martijn Dashorst <ma...@gmail.com> wrote:
> No
>
> Martijn
>
> On 11/1/07, Bruno Borges <br...@gmail.com> wrote:
> > isn't this a reason to start with that talk again Igor, about replacing
> > setRequired with an IValidator?
> >
> > regards,
> > bruno
> >
> > Igor Vaynberg wrote:
> > > On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> > >
> > >>>> You are just nitpicking. 99.9% of people won't override setRequired to
> > >>>> start with.
> > >>>>
> > >>> i dont know about that. seems to me that the majority of people who
> > >>> implement a FormComponentPanel will be forced to overwrite it since that
> > >>>
> > >> is
> > >>
> > >>> currently the model you have put in place.
> > >>>
> > >> Yeah, they should, as they probably didn't support the required flag
> > >> properly before.
> > >>
> > >
> > >
> > > they did just fine after my "hack"
> > >
> > >
> > >> i had to do it for
> > >>
> > >>> formcomponentpanels in my projects, and i really dont like the code it
> > >>> causes.
> > >>>
> > >> You had to? Well that's interesting. How did you fix this before this
> > >> change then?
> > >>
> > >
> > >
> > > with my "hack" i had it in my own wicket branch.
> > >
> > > The situation is *exactly like it was before this whole discussion*
> > >
> > >> with the exception that setRequired isn't final anymore, so that I can
> > >> fix the components I'm interested in in a way I think works good. I'm
> > >> pretty sure the fact that setRequired isn't final anymore won't wreak
> > >> havoc.
> > >>
> > >
> > >
> > > yes but pushing the required state is ugly, not to mention all the
> > > noobishess that will come with opening it!
> > >
> > > -igor
> > >
> > >
> > > Eelco
> > >
> > >
> > >
> >
> >
>
>
> --
> Buy Wicket in Action: http://manning.com/dashorst
> Apache Wicket 1.3.0-beta4 is released
> Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta4/
>
--
Buy Wicket in Action: http://manning.com/dashorst
Apache Wicket 1.3.0-beta4 is released
Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta4/
Re: formcomponentpanel.checkrequred
Posted by Martijn Dashorst <ma...@gmail.com>.
No
Martijn
On 11/1/07, Bruno Borges <br...@gmail.com> wrote:
> isn't this a reason to start with that talk again Igor, about replacing
> setRequired with an IValidator?
>
> regards,
> bruno
>
> Igor Vaynberg wrote:
> > On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> >
> >>>> You are just nitpicking. 99.9% of people won't override setRequired to
> >>>> start with.
> >>>>
> >>> i dont know about that. seems to me that the majority of people who
> >>> implement a FormComponentPanel will be forced to overwrite it since that
> >>>
> >> is
> >>
> >>> currently the model you have put in place.
> >>>
> >> Yeah, they should, as they probably didn't support the required flag
> >> properly before.
> >>
> >
> >
> > they did just fine after my "hack"
> >
> >
> >> i had to do it for
> >>
> >>> formcomponentpanels in my projects, and i really dont like the code it
> >>> causes.
> >>>
> >> You had to? Well that's interesting. How did you fix this before this
> >> change then?
> >>
> >
> >
> > with my "hack" i had it in my own wicket branch.
> >
> > The situation is *exactly like it was before this whole discussion*
> >
> >> with the exception that setRequired isn't final anymore, so that I can
> >> fix the components I'm interested in in a way I think works good. I'm
> >> pretty sure the fact that setRequired isn't final anymore won't wreak
> >> havoc.
> >>
> >
> >
> > yes but pushing the required state is ugly, not to mention all the
> > noobishess that will come with opening it!
> >
> > -igor
> >
> >
> > Eelco
> >
> >
> >
>
>
--
Buy Wicket in Action: http://manning.com/dashorst
Apache Wicket 1.3.0-beta4 is released
Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta4/
Re: formcomponentpanel.checkrequred
Posted by Bruno Borges <br...@gmail.com>.
isn't this a reason to start with that talk again Igor, about replacing
setRequired with an IValidator?
regards,
bruno
Igor Vaynberg wrote:
> On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
>>>> You are just nitpicking. 99.9% of people won't override setRequired to
>>>> start with.
>>>>
>>> i dont know about that. seems to me that the majority of people who
>>> implement a FormComponentPanel will be forced to overwrite it since that
>>>
>> is
>>
>>> currently the model you have put in place.
>>>
>> Yeah, they should, as they probably didn't support the required flag
>> properly before.
>>
>
>
> they did just fine after my "hack"
>
>
>> i had to do it for
>>
>>> formcomponentpanels in my projects, and i really dont like the code it
>>> causes.
>>>
>> You had to? Well that's interesting. How did you fix this before this
>> change then?
>>
>
>
> with my "hack" i had it in my own wicket branch.
>
> The situation is *exactly like it was before this whole discussion*
>
>> with the exception that setRequired isn't final anymore, so that I can
>> fix the components I'm interested in in a way I think works good. I'm
>> pretty sure the fact that setRequired isn't final anymore won't wreak
>> havoc.
>>
>
>
> yes but pushing the required state is ugly, not to mention all the
> noobishess that will come with opening it!
>
> -igor
>
>
> Eelco
>
>
>
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> > > You are just nitpicking. 99.9% of people won't override setRequired to
> > > start with.
> >
> > i dont know about that. seems to me that the majority of people who
> > implement a FormComponentPanel will be forced to overwrite it since that
> is
> > currently the model you have put in place.
>
> Yeah, they should, as they probably didn't support the required flag
> properly before.
they did just fine after my "hack"
> i had to do it for
> > formcomponentpanels in my projects, and i really dont like the code it
> > causes.
>
> You had to? Well that's interesting. How did you fix this before this
> change then?
with my "hack" i had it in my own wicket branch.
The situation is *exactly like it was before this whole discussion*
> with the exception that setRequired isn't final anymore, so that I can
> fix the components I'm interested in in a way I think works good. I'm
> pretty sure the fact that setRequired isn't final anymore won't wreak
> havoc.
yes but pushing the required state is ugly, not to mention all the
noobishess that will come with opening it!
-igor
Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
> > You are just nitpicking. 99.9% of people won't override setRequired to
> > start with.
>
> i dont know about that. seems to me that the majority of people who
> implement a FormComponentPanel will be forced to overwrite it since that is
> currently the model you have put in place.
Yeah, they should, as they probably didn't support the required flag
properly before.
> i had to do it for
> formcomponentpanels in my projects, and i really dont like the code it
> causes.
You had to? Well that's interesting. How did you fix this before this
change then?
The situation is *exactly like it was before this whole discussion*
with the exception that setRequired isn't final anymore, so that I can
fix the components I'm interested in in a way I think works good. I'm
pretty sure the fact that setRequired isn't final anymore won't wreak
havoc.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> > by making setrequired nonfinal the contract you have established is: you
> > always have to override both setrequired and isrequired to make sure
> they
> > return the same value, or call super in setrequired. and you havent done
> > this just for the formcomponetpanel but you have leaked this across all
> > formcomponents.
> >
> > i would much rather have datetimefield define the contract on its
> factory
> > method for newdatetextfield() which 99.9% of people probably wont use.
>
> You are just nitpicking. 99.9% of people won't override setRequired to
> start with.
i dont know about that. seems to me that the majority of people who
implement a FormComponentPanel will be forced to overwrite it since that is
currently the model you have put in place. i had to do it for
formcomponentpanels in my projects, and i really dont like the code it
causes.
-igor
Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> > uhm...a formcomponentpanel is just a formcomponent! so if i use it why
> is it
> > any different? it wouldve broken my label - infact it did, thats how i
> found
> > out the code was broken.
>
> How on freakin' earth did this break your label when the only change
> is that final is removed and you weren't even depending on that!
my label was linked to the datetimefield, which had a broken isrequried()
since you didnt call super in setrequired()
-igor
Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
> uhm...a formcomponentpanel is just a formcomponent! so if i use it why is it
> any different? it wouldve broken my label - infact it did, thats how i found
> out the code was broken.
How on freakin' earth did this break your label when the only change
is that final is removed and you weren't even depending on that!
Eelco
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> > isrequired is a pretty common method to call. formcomponentlabels that
> add a
> > * for required components comes to mind immediately.
>
> Common for FormComponentPanels? Nah.
uhm...a formcomponentpanel is just a formcomponent! so if i use it why is it
any different? it wouldve broken my label - infact it did, thats how i found
out the code was broken.
-igor
Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
> so if you made this mistake imagine everyone else making it!
Like I said, it was a mistake without real consequences, as the thing
that is important here - making the embedded text field do the check -
got done.
> > I'd also like to point out that this bug only had an effect when
> > clients would call isRequired themselves, as for Wicket's functioning
> > it's not really relevant. And making the isRequired function final is
> > really to force people to use setRequired.
>
>
> isrequired is a pretty common method to call. formcomponentlabels that add a
> * for required components comes to mind immediately.
Common for FormComponentPanels? Nah.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> > > isnt this broken? isrequired() doesnt return the same value as
> > > setrequired().
> >
> > You you're right, this would be it:
so if you made this mistake imagine everyone else making it!
> I'd also like to point out that this bug only had an effect when
> clients would call isRequired themselves, as for Wicket's functioning
> it's not really relevant. And making the isRequired function final is
> really to force people to use setRequired.
isrequired is a pretty common method to call. formcomponentlabels that add a
* for required components comes to mind immediately.
-igor
Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> > hrm, so lets see DateTimeField:
> >
> > public final boolean isRequired()
> > {
> > // prevent clients from overriding
> > return super.isRequired();
> > }
> >
> > public FormComponent setRequired(boolean required)
> > {
> > dateField.setRequired(required);
> > return this;
> > }
> >
> >
> > isnt this broken? isrequired() doesnt return the same value as
> > setrequired().
>
> You you're right, this would be it:
>
> public FormComponent setRequired(boolean required)
> {
> super.setRequired(required);
> dateField.setRequired(required);
> return this;
> }
I'd also like to point out that this bug only had an effect when
clients would call isRequired themselves, as for Wicket's functioning
it's not really relevant. And making the isRequired function final is
really to force people to use setRequired.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
> hrm, so lets see DateTimeField:
>
> public final boolean isRequired()
> {
> // prevent clients from overriding
> return super.isRequired();
> }
>
> public FormComponent setRequired(boolean required)
> {
> dateField.setRequired(required);
> return this;
> }
>
>
> isnt this broken? isrequired() doesnt return the same value as
> setrequired().
You you're right, this would be it:
public FormComponent setRequired(boolean required)
{
super.setRequired(required);
dateField.setRequired(required);
return this;
}
> by making setrequired nonfinal the contract you have established is: you
> always have to override both setrequired and isrequired to make sure they
> return the same value, or call super in setrequired. and you havent done
> this just for the formcomponetpanel but you have leaked this across all
> formcomponents.
>
> i would much rather have datetimefield define the contract on its factory
> method for newdatetextfield() which 99.9% of people probably wont use.
You are just nitpicking. 99.9% of people won't override setRequired to
start with.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
hrm, so lets see DateTimeField:
public final boolean isRequired()
{
// prevent clients from overriding
return super.isRequired();
}
public FormComponent setRequired(boolean required)
{
dateField.setRequired(required);
return this;
}
isnt this broken? isrequired() doesnt return the same value as
setrequired().
by making setrequired nonfinal the contract you have established is: you
always have to override both setrequired and isrequired to make sure they
return the same value, or call super in setrequired. and you havent done
this just for the formcomponetpanel but you have leaked this across all
formcomponents.
i would much rather have datetimefield define the contract on its factory
method for newdatetextfield() which 99.9% of people probably wont use.
-igor
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> > and i suppose requiring a supercall from onattach/onbeforerender/etc is
> also
> > Eeeeewww! :) but that is how it works. when you override something you
> have
> > to make sure you do it right.
>
> Well yeah, if there is no other way around. But with how it currently
> is, clients don't have to.
>
> Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
> and i suppose requiring a supercall from onattach/onbeforerender/etc is also
> Eeeeewww! :) but that is how it works. when you override something you have
> to make sure you do it right.
Well yeah, if there is no other way around. But with how it currently
is, clients don't have to.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> > there are two choices here - either you document it and let them know
> they
> > have to properly implement isrequried() in their components - there is
> > absolutely nothing wrong with that!
>
> Requiring clients to let their text fields override isRequired and
> call the datepanel for instance? Eeeeewww!
and i suppose requiring a supercall from onattach/onbeforerender/etc is also
Eeeeewww! :) but that is how it works. when you override something you have
to make sure you do it right.
-igor
> or if you want to be nice you can do it for them in onattach() instead of
> in
> > setrequired(). with our new lifecycle in 1.3 first onattach() is called
> then
> > form.process() so it should always be in sync.
> >
> >
> > See, we can keep bouncing back and forth. I'm ok with the solution we
> > > have now (removed final from setRequired). I did override isRequired
> > > on the panels where I did override setRequired, so that's tight now.
> >
> >
> > i dont know if i like setrequired() open. what do others think?
>
> I don't see a problem with it. And like I did with DateField/
> DateTimeField: override setRequired and isRequired, making the latter
> final... that's water tight. Works without requiring weird
> workarounds.
>
> Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
> there are two choices here - either you document it and let them know they
> have to properly implement isrequried() in their components - there is
> absolutely nothing wrong with that!
Requiring clients to let their text fields override isRequired and
call the datepanel for instance? Eeeeewww!
> or if you want to be nice you can do it for them in onattach() instead of in
> setrequired(). with our new lifecycle in 1.3 first onattach() is called then
> form.process() so it should always be in sync.
>
>
> See, we can keep bouncing back and forth. I'm ok with the solution we
> > have now (removed final from setRequired). I did override isRequired
> > on the panels where I did override setRequired, so that's tight now.
>
>
> i dont know if i like setrequired() open. what do others think?
I don't see a problem with it. And like I did with DateField/
DateTimeField: override setRequired and isRequired, making the latter
final... that's water tight. Works without requiring weird
workarounds.
Eelco
Re: formcomponentpanel.checkrequred
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/12/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> Ok, one more reply then.
>
> > >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.
> >
> > great, so datefield { isrequired() { return fcp.isrequired()}}, the
> other
> > fields are simply not required. everything works without any magic.
>
> I considered that as well, but unfortunately that only works when
> you're in full control of all child components. But DateField and
> DateTimeField both have newDateTextField which is a factory method
> that clients can override. So much for that.
there are two choices here - either you document it and let them know they
have to properly implement isrequried() in their components - there is
absolutely nothing wrong with that!
or if you want to be nice you can do it for them in onattach() instead of in
setrequired(). with our new lifecycle in 1.3 first onattach() is called then
form.process() so it should always be in sync.
See, we can keep bouncing back and forth. I'm ok with the solution we
> have now (removed final from setRequired). I did override isRequired
> on the panels where I did override setRequired, so that's tight now.
i dont know if i like setrequired() open. what do others think?
-igor
Eelco
>
Re: formcomponentpanel.checkrequred
Posted by Eelco Hillenius <ee...@gmail.com>.
Ok, one more reply then.
> >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.
>
> great, so datefield { isrequired() { return fcp.isrequired()}}, the other
> fields are simply not required. everything works without any magic.
I considered that as well, but unfortunately that only works when
you're in full control of all child components. But DateField and
DateTimeField both have newDateTextField which is a factory method
that clients can override. So much for that.
See, we can keep bouncing back and forth. I'm ok with the solution we
have now (removed final from setRequired). I did override isRequired
on the panels where I did override setRequired, so that's tight now.
Eelco