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
