You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by Adam Winer <aw...@gmail.com> on 2007/05/01 00:48:22 UTC
1.2 TLD compatibility
I'm checking out the following three issues, and have comments on the
patches for each.
https://issues.apache.org/jira/browse/ADFFACES-475:
- Changes the deferred-method method name from "myMethod() to action()".
I don't get it. The method name in the TLD has zero runtime meaning. The
spec
should not require any particular name, and it'd be a spec bug if it does
(or, at least, the spec should clarify). If we're going to change anything,
I'd
rather change the method name from generically being "myMethod()" to always
being the TLD attribute name (e.g., "action()", "beforePhase()", etc.).
But, mostly I'm inclined to push back on the spec.
https://issues.apache.org/jira/browse/ADFFACES-476:
- Changes rtexprvalue on "id" from true to false
Supporting rtexprvalue on "id" is useful for component libs. I think this
should be a configurable option on the plugin (by default, on) that we
can turn off for the spec components but keep on for Trinidad +
Tomahawk.
https://issues.apache.org/jira/browse/ADFFACES-477
- Changes the deferred-value type on "converter" and "binding" from
java.lang.Object to the appropriate type
I'm OK with this one, though I don't see any RT benefits. There's
some small tooling benefit to the change. I think the simplest change
to make is removing the current "CAN_COERCE" check in the code,
instead of hardcoding "binding" and "converter" as special attrs.
-- Adam
Re: 1.2 TLD compatibility
Posted by Tim McConnell <ti...@gmail.com>.
Hi, I do not know the answer Adam's last question below concerning boolean
versus java.language.Boolean, although I'm interested in the answer. Would
someone else please advise ?? Thanks much.
Tim McConnell
Adam Winer wrote:
>
>
> On 4/30/07, *Tim McConnell* <tim.mcconne@gmail.com
> <ma...@gmail.com>> wrote:
>
> Hi Adam, thanks very much for reviewing my patches so promptly. Here
> are my
> comments to your comments:
>
> ADFFACES-475:
> - If the method name is immaterial at runtime then the only change
> for the
> companion MYFACES-1599 JIRA would be to update the return value
> which I've done
> with the patch attached to MYFACES-1599. If no one objects I think I
> should just
> close ADFFACES-475.
>
> ADFFACES-476:
> - I really like that solution. I shall provide you with another
> patch (assuming
> I can discern what the spec components are).
>
>
> I wouldn't hardcode in the plugin what a spec component is, etc. I'd
> make it a property of the plugin, so that in a pom you could have
> <idExpressions>false</idExpressions>
> ... and it would turn it off for that project.
>
>
> ADFFACES-477:
> - To be honest I was a bit hesitate to alter the current
> "CAN_COERCE" check
> since I was not fully certain of the implications. If there are
> none, then it
> seems the _CAN_COERCE map can be removed as well since that is the
> only place it
> is used. If that is the case I shall provide you with another patch.
>
>
>
> I'm not 100% sure of the implications either. :) I'll run some tests
> and make
> sure this doesn't break anything obvious.
>
> BTW, does it matter whether a deferred-value type is java.lang.Boolean
> or boolean? I figure they're identical in runtime behavior, since null
> will be coerced to Boolean.FALSE and false (I think).
>
> -- Adam
>
Re: 1.2 TLD compatibility
Posted by Tim McConnell <ti...@gmail.com>.
Adam Winer wrote:
> On 4/30/07, Adam Winer <aw...@gmail.com> wrote:
>>
>>
>> On 4/30/07, Tim McConnell <ti...@gmail.com> wrote:
>> > Hi Adam, thanks very much for reviewing my patches so promptly. Here
>> are
>> my
>> > comments to your comments:
>> >
>> > ADFFACES-475:
>> > - If the method name is immaterial at runtime then the only change
>> for the
>> > companion MYFACES-1599 JIRA would be to update the return value
>> which I've
>> done
>> > with the patch attached to MYFACES-1599. If no one objects I think I
>> should just
>> > close ADFFACES-475.
>
> I've closed it.
Thanks....
>
>> > ADFFACES-476:
>> > - I really like that solution. I shall provide you with another patch
>> (assuming
>> > I can discern what the spec components are).
>>
>> I wouldn't hardcode in the plugin what a spec component is, etc. I'd
>> make it a property of the plugin, so that in a pom you could have
>> <idExpressions>false</idExpressions>
>> ... and it would turn it off for that project.
>
> I added <disableIdExpressions> which should do the trick for
> the spec components.
>
Yes, it works beautifully....
>> > ADFFACES-477:
>> > - To be honest I was a bit hesitate to alter the current "CAN_COERCE"
>> check
>> > since I was not fully certain of the implications. If there are
>> none, then
>> it
>> > seems the _CAN_COERCE map can be removed as well since that is the only
>> place it
>> > is used. If that is the case I shall provide you with another patch.
>>
>>
>> I'm not 100% sure of the implications either. :) I'll run some tests and
>> make
>> sure this doesn't break anything obvious.
>
> I did some testing - one thing it immediately broke was
> the "converter" attribute when set to a String. The code generation
> was using valueExpression.getValue(null) when isLiteralText() was true,
> which will trigger a coercion (and failure). The fix is to use
> getExpressionString(). I've applied that fix to the MyFaces component
> generator (and the Trinidad as well, of course).
>
> But to be more conservative, I've only added Converter and UIComponent
> to the list of types that can appear in <deferred-value/>.
>
Works as well....
> I've also added a <coerceStrings/> plugin configuration setting
> that can let you turn on java.lang.String coercion per project.
>
> -- Adam
>
>
>> BTW, does it matter whether a deferred-value type is java.lang.Boolean
>> or boolean? I figure they're identical in runtime behavior, since null
>> will be coerced to Boolean.FALSE and false (I think).
>>
>> -- Adam
>>
>>
>
Thanks Adam !! The patches attached to the companion MyFaces Jiras are now ready
to be committed:
MYFACES-1598
MYFACES-1599
MYFACES-1600
--
Thanks,
Tim McConnell
Re: 1.2 TLD compatibility
Posted by Adam Winer <aw...@gmail.com>.
On 4/30/07, Adam Winer <aw...@gmail.com> wrote:
>
>
> On 4/30/07, Tim McConnell <ti...@gmail.com> wrote:
> > Hi Adam, thanks very much for reviewing my patches so promptly. Here are
> my
> > comments to your comments:
> >
> > ADFFACES-475:
> > - If the method name is immaterial at runtime then the only change for the
> > companion MYFACES-1599 JIRA would be to update the return value which I've
> done
> > with the patch attached to MYFACES-1599. If no one objects I think I
> should just
> > close ADFFACES-475.
I've closed it.
> > ADFFACES-476:
> > - I really like that solution. I shall provide you with another patch
> (assuming
> > I can discern what the spec components are).
>
> I wouldn't hardcode in the plugin what a spec component is, etc. I'd
> make it a property of the plugin, so that in a pom you could have
> <idExpressions>false</idExpressions>
> ... and it would turn it off for that project.
I added <disableIdExpressions> which should do the trick for
the spec components.
> > ADFFACES-477:
> > - To be honest I was a bit hesitate to alter the current "CAN_COERCE"
> check
> > since I was not fully certain of the implications. If there are none, then
> it
> > seems the _CAN_COERCE map can be removed as well since that is the only
> place it
> > is used. If that is the case I shall provide you with another patch.
>
>
> I'm not 100% sure of the implications either. :) I'll run some tests and
> make
> sure this doesn't break anything obvious.
I did some testing - one thing it immediately broke was
the "converter" attribute when set to a String. The code generation
was using valueExpression.getValue(null) when isLiteralText() was true,
which will trigger a coercion (and failure). The fix is to use
getExpressionString(). I've applied that fix to the MyFaces component
generator (and the Trinidad as well, of course).
But to be more conservative, I've only added Converter and UIComponent
to the list of types that can appear in <deferred-value/>.
I've also added a <coerceStrings/> plugin configuration setting
that can let you turn on java.lang.String coercion per project.
-- Adam
> BTW, does it matter whether a deferred-value type is java.lang.Boolean
> or boolean? I figure they're identical in runtime behavior, since null
> will be coerced to Boolean.FALSE and false (I think).
>
> -- Adam
>
>
Re: 1.2 TLD compatibility
Posted by Adam Winer <aw...@gmail.com>.
On 4/30/07, Tim McConnell <ti...@gmail.com> wrote:
>
> Hi Adam, thanks very much for reviewing my patches so promptly. Here are
> my
> comments to your comments:
>
> ADFFACES-475:
> - If the method name is immaterial at runtime then the only change for the
> companion MYFACES-1599 JIRA would be to update the return value which I've
> done
> with the patch attached to MYFACES-1599. If no one objects I think I
> should just
> close ADFFACES-475.
>
> ADFFACES-476:
> - I really like that solution. I shall provide you with another patch
> (assuming
> I can discern what the spec components are).
I wouldn't hardcode in the plugin what a spec component is, etc. I'd
make it a property of the plugin, so that in a pom you could have
<idExpressions>false</idExpressions>
... and it would turn it off for that project.
> ADFFACES-477:
> - To be honest I was a bit hesitate to alter the current "CAN_COERCE"
> check
> since I was not fully certain of the implications. If there are none, then
> it
> seems the _CAN_COERCE map can be removed as well since that is the only
> place it
> is used. If that is the case I shall provide you with another patch.
I'm not 100% sure of the implications either. :) I'll run some tests and
make
sure this doesn't break anything obvious.
BTW, does it matter whether a deferred-value type is java.lang.Boolean
or boolean? I figure they're identical in runtime behavior, since null
will be coerced to Boolean.FALSE and false (I think).
-- Adam
Re: 1.2 TLD compatibility
Posted by Tim McConnell <ti...@gmail.com>.
Hi Adam, thanks very much for reviewing my patches so promptly. Here are my
comments to your comments:
ADFFACES-475:
- If the method name is immaterial at runtime then the only change for the
companion MYFACES-1599 JIRA would be to update the return value which I've done
with the patch attached to MYFACES-1599. If no one objects I think I should just
close ADFFACES-475.
ADFFACES-476:
- I really like that solution. I shall provide you with another patch (assuming
I can discern what the spec components are).
ADFFACES-477:
- To be honest I was a bit hesitate to alter the current "CAN_COERCE" check
since I was not fully certain of the implications. If there are none, then it
seems the _CAN_COERCE map can be removed as well since that is the only place it
is used. If that is the case I shall provide you with another patch.
Thanks,
Tim McConnell
Adam Winer wrote:
> I'm checking out the following three issues, and have comments on the
> patches for each.
>
> https://issues.apache.org/jira/browse/ADFFACES-475:
> - Changes the deferred-method method name from "myMethod() to action()".
> I don't get it. The method name in the TLD has zero runtime meaning.
> The spec
> should not require any particular name, and it'd be a spec bug if it does
> (or, at least, the spec should clarify). If we're going to change
> anything, I'd
> rather change the method name from generically being "myMethod()" to always
> being the TLD attribute name (e.g., "action()", "beforePhase()", etc.).
> But, mostly I'm inclined to push back on the spec.
>
> https://issues.apache.org/jira/browse/ADFFACES-476:
> - Changes rtexprvalue on "id" from true to false
> Supporting rtexprvalue on "id" is useful for component libs. I think this
> should be a configurable option on the plugin (by default, on) that we
> can turn off for the spec components but keep on for Trinidad +
> Tomahawk.
>
> https://issues.apache.org/jira/browse/ADFFACES-477
> - Changes the deferred-value type on "converter" and "binding" from
> java.lang.Object to the appropriate type
> I'm OK with this one, though I don't see any RT benefits. There's
> some small tooling benefit to the change. I think the simplest change
> to make is removing the current "CAN_COERCE" check in the code,
> instead of hardcoding "binding" and "converter" as special attrs.
>
> -- Adam
>