done
On Thu, Mar 20, 2008 at 2:03 AM, Vitaly Tsaplin <[EMAIL PROTECTED]>
wrote:
> Oh, sorry. I meant "write in javadoc" :)
>
> On Thu, Mar 20, 2008 at 2:02 AM, Vitaly Tsaplin
> <[EMAIL PROTECTED]> wrote:
> > For me that's probably fine if you will clearly right in javadoc
> > that anyone who overrides checkRequired should carefully call
> > isRequired in order to conform the common behavior.
> >
> >
> >
> > On Thu, Mar 20, 2008 at 1:33 AM, Johan Compagner <[EMAIL PROTECTED]>
> wrote:
> > > Thats all fine, but it was not build/designed that way period...
> > >
> > > The biggest api break was at the moment we removed final, which was
> > > there for quite sometime, and then sneaky add javadoc that it is
> > > required to call isRequired first before calling this method! but
> > > before that for this public callable method this was not needed, so
> > > everybody who uses that method suddenly breaks because of a sneaky
> > > javadoc update and the removal of the final keyword.
> > >
> > > What the javadoc should have stated is that overrides should ALWAYS
> > > first check the isRequired() itself just like the method that they
> > > override already did for 2 years!
> > >
> > > The removal of the final keyword had in this particular case big
> > > consequentes it really changed the api behavior.
> > >
> > > Johan
> > >
> > >
> > >
> > > On 3/20/08, Vitaly Tsaplin <[EMAIL PROTECTED]> wrote:
> > > > What if a requirement cannot be met if an input contains only
> > > > spaces. I would override the checkRequired to do so. And it could
> be
> > > > done for a text field. Why not?
> > > >
> > > > Anyway if I can override checkRequired I may not be calling
> > > > isRequired and it's perfectly legal. Because the method is under
> my
> > > > control. But...setRequired is still there and anyone can call it
> > > > excpecting it's normal behavior. Setting required property to
> false
> > > > (setRequired) should prevent any requirement check whatever it
> would
> > > > be.
> > > >
> > > > On Thu, Mar 20, 2008 at 12:57 AM, Johan Compagner <
> [EMAIL PROTECTED]>
> > > > wrote:
> > > > > hmm
> > > > >
> > > > > the more i think about it
> > > > > the more i stand with my initial reply.
> > > > >
> > > > > checkRequired was always meant to be standalone
> > > > > it always checked from day 1 if required must be checked (thats
> why it is
> > > > > called *check*Required) and then it actually did the test.
> > > > > it was first protected final and then a long time public final
> > > > >
> > > > > validateRequired does something else
> > > > > it calls checkRequired and with that return value it sets an
> adds an
> > > > error
> > > > > message
> > > > > But that is not always what you want (the reason why
> checkRequired is
> > > > > public)
> > > > > so validateRequired is not a substitute and i guess
> checkRequired has to
> > > > be
> > > > > public because we made it public for a reason. (see the thread)
> > > > >
> > > > > The only thing i come up with now is, but that will break all
> > > > > formcomponentpanels that did have implemented checkRequired(),
> > > > > that we make it final again (keep it public) and create a
> protected
> > > > > overridable method that can be used in FCP
> > > > > something like
> > > > >
> > > > > protected boolean doRequiredInputCheck()
> > > > > {
> > > > > }
> > > > >
> > > > > or what ever better name we can come up with.
> > > > >
> > > > > that way we suddenly dont have a completely different
> definition of a
> > > > > method
> > > > >
> > > > > johan
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Mar 20, 2008 at 12:03 AM, Johan Compagner <
> [EMAIL PROTECTED]>
> > > > >
> > > > >
> > > > > wrote:
> > > > >
> > > > > > no some protected methods can be called just fine from the
> outside
> > > > world
> > > > > > like validateRequired() (that could be public yes)
> > > > > >
> > > > > > But checkRequired() doesn't make much to call from the
> outside world
> > > > > > because we have validateRequired()
> > > > > > except that validateRequired() does set an error then and
> doesn't
> > > > return
> > > > > > the actual boolean
> > > > > > So i guess that that is the reason why checkRequired() is
> public.
> > > > > >
> > > > > > it was first public final that as removed for
> FormComponentPanel
> > > > > > and when it was public final it made perfect sense to also
> check for
> > > > > > required there..
> > > > > > So the definition is really changed when we removed final
> before that
> > > > it
> > > > > > was really what i thought initially because it is a public
> callable
> > > > method
> > > > > > (back then the javadoc also didnt specify that it was
> typically
> > > > required
> > > > > > that isRequired must be callled)
> > > > > >
> > > > > > so suddenly final is gone and the definition changed...
> > > > > > Nobody would call isRequired() before it would call
> checkRequired()
> > > > > >
> > > > > > and it was it first few months (2006) it was protected final,
> > > > > > then you made it public final because of this thread
> "[Wicket-user]
> > > > > > Components Label for FeedbackMessage"
> > > > > >
> > > > > > So if anybody programmed against that method back then
> because of that
> > > > > > thread. Those are not calling isRequired() first before
> calling
> > > > > > checkRequired()
> > > > > > Even if i type this i still find it odd that i have to do
> that first,
> > > > > > looking at the names...., if i read
> formcomponent.checkRequired() then
> > > > i
> > > > > > will always asume that it
> > > > > > will not return the wrong value if isRequired == false..
> maybe it is my
> > > > > > bad englisch but that will not change for me)
> > > > > >
> > > > > > johan
> > > > > >
> > > > > >
> > > > > > On Wed, Mar 19, 2008 at 11:40 PM, Igor Vaynberg
> > > > <[EMAIL PROTECTED]>
> > > > > > wrote:
> > > > > >
> > > > > > > do we need to add that to all our protected methods? if
> someone needs
> > > > > > > it, and they probably do since we made it public, we should
> prob make
> > > > > > > validateRequired() public - it is final iirc
> > > > > > >
> > > > > > > -igor
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Mar 19, 2008 at 3:36 PM, Johan Compagner
> > > > <[EMAIL PROTECTED]>
> > > > > > > wrote:
> > > > > > > > yes and add a big javadoc warning that this method is not
> meant to
> > > > be
> > > > > > > called
> > > > > > > > only meant to be overriden..
> > > > > > > >
> > > > > > > > johan
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Mar 19, 2008 at 11:35 PM, Igor Vaynberg <
> > > > > > > [EMAIL PROTECTED]>
> > > > > > > >
> > > > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > ok, so given that we make checkrequired protected
> (pushes it
> > > > into
> > > > > > > 1.5
> > > > > > > > > timeframe) are you ok with moving isrequired() check
> out into
> > > > > > > > > validaterequired() ?
> > > > > > > > >
> > > > > > > > > -igor
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, Mar 19, 2008 at 3:22 PM, Johan Compagner <
> > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > wrote:
> > > > > > > > > > ì guess checkRequired( ) is only overridable because
> of
> > > > > > > > > FormComponentPanel
> > > > > > > > > > so that again that can be overriden to have there
> own
> > > > > > > requirement
> > > > > > > > > check?
> > > > > > > > > >
> > > > > > > > > > so yes it should really be at least protected
> > > > > > > > > >
> > > > > > > > > > On Wed, Mar 19, 2008 at 11:10 PM, Igor Vaynberg <
> > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > thats kinda cludge imho. checkrequired() defines
> the
> > > > process
> > > > > > > of
> > > > > > > > > > > checking, whether that needs to be invoked or not
> is up to
> > > > the
> > > > > > > > > > > formcomponent and its required attribute.
> > > > > > > > > > >
> > > > > > > > > > > we can make validaterequired() public, although i
> dont see
> > > > > > > where you
> > > > > > > > > > > would call only that instead of the entire
> validate()
> > > > > > > pipeline.
> > > > > > > > > > >
> > > > > > > > > > > also right now checkrequired() is only ever
> called from
> > > > > > > > > > > validaterequired() and i think checkrequired()
> should not
> > > > be
> > > > > > > public
> > > > > > > > > > > anyways
> > > > > > > > > > >
> > > > > > > > > > > -igor
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Mar 19, 2008 at 3:06 PM, Johan Compagner
> <
> > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > wrote:
> > > > > > > > > > > > if you also want to check it in
> validateRequired() thats
> > > > > > > fine by me
> > > > > > > > > by
> > > > > > > > > > > the
> > > > > > > > > > > > way
> > > > > > > > > > > > But i dont want it to be removed in
> checkRequired()
> > > > > > > > > > > >
> > > > > > > > > > > > And the javadoc must be updated anyway
> > > > > > > > > > > >
> > > > > > > > > > > > johan
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Mar 19, 2008 at 11:05 PM, Johan
> Compagner <
> > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > nope
> > > > > > > > > > > > > i am against that
> > > > > > > > > > > > > validateRequired is protected
> > > > > > > > > > > > > checkRequired is public
> > > > > > > > > > > > >
> > > > > > > > > > > > > And if i want to test for requirement from
> outside i
> > > > dont
> > > > > > > want
> > > > > > > > > to
> > > > > > > > > > > call
> > > > > > > > > > > > > first for every thing isRequired first
> > > > > > > > > > > > > i find it very odd that a method can return
> very funny
> > > > > > > stuff
> > > > > > > > > when you
> > > > > > > > > > > dont
> > > > > > > > > > > > > call one method before it first
> > > > > > > > > > > > > it should be self contained
> > > > > > > > > > > > >
> > > > > > > > > > > > > And this way it works for quite some time
> now, so i
> > > > dont
> > > > > > > really
> > > > > > > > > > > change
> > > > > > > > > > > > > anything.
> > > > > > > > > > > > > it is just stupid to have a method which
> behavior is
> > > > not
> > > > > > > really
> > > > > > > > > > > defined
> > > > > > > > > > > > > based on that you can call it or not.
> > > > > > > > > > > > > it doesn't make any sense to me to not call
> > > > isRequired()
> > > > > > > in
> > > > > > > > > > > > > checkRequired()
> > > > > > > > > > > > >
> > > > > > > > > > > > > johan
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Wed, Mar 19, 2008 at 10:57 PM, Igor
> Vaynberg <
> > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > why dont we build that check into
> validateRequired()
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > so validateRequired() { if (isrequired() {
> > > > ...current
> > > > > > > code } }
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > that way you never have to call
> checkrequired()
> > > > > > > directly, just
> > > > > > > > > call
> > > > > > > > > > > > > > validaterequired()
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > -igor
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Wed, Mar 19, 2008 at 2:54 PM, Johan
> Compagner <
> > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > i dont agree
> > > > > > > > > > > > > > > then you have to do everywhere
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > if (isRequired()) checkRequired()
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > thats horrible, checkRequired() can
> test that
> > > > just
> > > > > > > as fine
> > > > > > > > > > > > > > > i will update the javadoc
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > johan
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Wed, Mar 19, 2008 at 10:51 PM,
> Vitaly Tsaplin
> > > > <
> > > > > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > But the javadoc says:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > public boolean checkRequired()
> > > > > > > > > > > > > > > > "Checks if the form component's
> 'required'
> > > > > > > requirement is
> > > > > > > > > met.
> > > > > > > > > > > This
> > > > > > > > > > > > > > > > method should typically only be
> called when
> > > > > > > > > > > > > > FormComponent.isRequired()
> > > > > > > > > > > > > > > > returns true."
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > And I agree with javadoc :)
> > > > > > > > > > > > > > > > checkRequired () should be called
> only to know
> > > > "if
> > > > > > > the
> > > > > > > > > form
> > > > > > > > > > > > > > > > component's 'required' requirement is
> met". In
> > > > > > > case
> > > > > > > > > > > isRequired()
> > > > > > > > > > > > > > > > returns false this call does not make
> any
> > > > sense...
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Basically if isRequired () returns
> true you
> > > > know
> > > > > > > that a
> > > > > > > > > > > component
> > > > > > > > > > > > > > > > is required but what you don't know
> is whether
> > > > the
> > > > > > > > > requirement
> > > > > > > > > > > > > > > > condition is met or not and so to
> check it out
> > > > you
> > > > > > > call
> > > > > > > > > > > > > > checkRequired
> > > > > > > > > > > > > > > > (). checkRequired () shouldn't call
> isRequired
> > > > ()
> > > > > > > itself.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Wed, Mar 19, 2008 at 10:39 PM,
> Johan
> > > > Compagner
> > > > > > > <
> > > > > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > it checks if the required needs to
> be checked
> > > > > > > and if
> > > > > > > > > that is
> > > > > > > > > > > the
> > > > > > > > > > > > > > case it
> > > > > > > > > > > > > > > > > checks if the input is set
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On Wed, Mar 19, 2008 at 10:32 PM,
> Vitaly
> > > > > > > Tsaplin <
> > > > > > > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > checkRequired () itself
> shouldn't be
> > > > called
> > > > > > > at all
> > > > > > > > > > > unless
> > > > > > > > > > > > > > > > > > setRequired is true...
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Wed, Mar 19, 2008 at 9:43 PM,
> Johan
> > > > > > > Compagner <
> > > > > > > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > and did you look at
> checkRequired?
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > public boolean
> checkRequired()
> > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > if (isRequired())
> > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > On Wed, Mar 19, 2008 at 2:24
> PM, Vitaly
> > > > > > > Tsaplin <
> > > > > > > > > > > > > > > > > > [EMAIL PROTECTED]>
> > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > Hi guys,
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > According to the wicket
> javadoc the
> > > > > > > method
> > > > > > > > > > > > > > checkRequired () of
> > > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > > > > > FormComponent class
> "...should
> > > > typically
> > > > > > > only
> > > > > > > > > be
> > > > > > > > > > > called
> > > > > > > > > > > > > > when
> > > > > > > > > > > > > > > > > > > > isRequired() returns true."
> > > > > > > > > > > > > > > > > > > > But it seems to be
> different...
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > public final void
> validate()
> > > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > >
> validateRequired();
> > > > > > > > > > > > > > <<<-------------------- here
> > > > > > > > > > > > > > > > > > > > if
> (isValid())
> > > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > >
> > > > convertInput();
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > if
> (isValid()
> > > > &&
> > > > > > > > > isRequired()
> > > > > > > > > > > &&
> > > > > > > > > > > > > > > > > > > > getConvertedInput() == null
> &&
> > > > > > > > > > > > > > > > > > > > isInputNullable())
> > > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > reportRequiredError();
> > > > > > > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > if
> (isValid())
> > > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > validateValidators();
> > > > > > > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > protected final void
> > > > > > > validateRequired()
> > > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > > if
> (!checkRequired())
> > > > > > > > > > > > > > <<<---------------------
> > > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > > > here
> > > > > > > > > > > > > > > > > > > > {
> > > > > > > > > > > > > > > > > > > >
> > > > > > > reportRequiredError();
> > > > > > > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > As you can see the
> checkRequired ()
> > > > is
> > > > > > > called
> > > > > > > > > > > > > > unconditionally.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > Vitaly
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > > > > > > > > > > > > > > To unsubscribe, e-mail:
> > > > > > > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > > > > > > > For additional commands,
> e-mail:
> > > > > > > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > > > > > > > > > > > > To unsubscribe, e-mail:
> > > > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > > > > > For additional commands, e-mail:
> > > > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > > > > > > > > > > To unsubscribe, e-mail:
> > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > > > For additional commands, e-mail:
> > > > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > > > > > > > > To unsubscribe, e-mail:
> > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > > For additional commands, e-mail:
> > > > > > > [EMAIL PROTECTED]
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > > > > > To unsubscribe, e-mail:
> [EMAIL PROTECTED]
> > > > > > > > > > > For additional commands, e-mail:
> > > > [EMAIL PROTECTED]
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > > > To unsubscribe, e-mail:
> [EMAIL PROTECTED]
> > > > > > > > > For additional commands, e-mail:
> [EMAIL PROTECTED]
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > > > > For additional commands, e-mail:
> [EMAIL PROTECTED]
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > For additional commands, e-mail: [EMAIL PROTECTED]
> > >
> > >
> >
>
