The bug https://issues.apache.org/jira/browse/TOMAHAWK-858 has made me
want to bring this up for discussion of the entire team. I really
don't like the solution they are posing as it will cause backward
compatibility problems and is not a full solution. This problem is not
unique to the Tomahawk tabbed pane, so I would love to see a MyFaces
wide (and maybe a JSF 2.0) fix (or approach) to the problem.
The issue is that several components use a "setXxx(...)" method to
update components. This typically is done in renderer code. Here is a
short list of components that I know are affected:
Tomahawk data scroller / data table and the first attribute
Tomahawk tabbed pane
Trinidad UIXShowDetail (I checked in a one component fix for this one,
but I am not 100% happy with it)
I am sure there are more. The problem is that if I have this code (I
am picking on the data table it is the easiest example):
<t:dataTable first="#{bean.firstRowIndex}"...>
<t:dataScroller ...>
...
The data scroller has the code UIData.setFirst(...) when the event is broadcast.
Typically all MyFaces getter code is written as:
if there is a local value
return it
if not, get the value binding / value expression
if set, evaluate it
if the value is null or there was no EL, then return a default value
if available, or else null
The setter code is:
set the local value
Even with Trinidad using a FacesBean, it still suffers from the local
vs. EL problem. What ends up being the major issue is that the local
value always takes precedence over the EL value. This is very rarely
the desired behavior. The local value is only really useful when
component binding is being used and the page author is not using EL to
set attributes.
The solution is more complex. In issue TOMAHAWK-858, someone has
proposed to use EL if it is set, but they did not examine the problem
domain. For example:
<t:dataTable first="#{bean.condition ? 0 : bean.firstRowIndex}" ...
In this crude example, perhaps the hard-coded 0 is to reset the table
to the first record on another link (please do not over analyze the
example, I know it is far from perfect). The problem with this is that
in using a conditional EL expression, this is no longer set-able! So
if the data scroller code attempts to assign 20 (the next page lets
say) to the first EL, it will throw an exception, because a
conditional statement cannot be set.
One solution to this is:
if EL is set,
try to set the EL
if that throws an exception, set the local value
This would technically work, but I hate the code. It is unpredictable
and bad for performance (exceptions are expensive and should be
avoided).
Solution two:
Check if EL is set and ValueExpression.isReadOnly(ELContext context)
returns false
set EL
else set local
Problem is that I do not think that this is always accurate.
The other problem is that once the local value is set, it cannot
always be cleared:
private Integer _first;
public int getFirst() {
if (_first != null) return _first.intValue();
ValueBinding ...
}
public int setFirst(int first) { _first = new Integer(first); }
Here, there is no way to set _first back to null. We could change the
APIs drastically of Tomahawk and Trinidad so that the generation
plugin always has to use objects and not primitives, but that would
break a lot of code and is not a nice API when null is never returned
for a getter when a default value is used.
Another option is a case-by-case fix where the property can be made
"transient". Meaning that the set method does nothing and the get
method is always used. This is a partial fix, but is ugly and requires
that the component users always update the values behind the EL
manually based on events
If EL allowed for different get & set that may work better:
<t:dataTable first="#{get: bean.condition ? 0 : bean.firstRowIndex,
set: bean.firstRowIndex}" ...
In this case the value expression would evaluate different expressions
for get vs. set. The problem here is that it is harder to write,
understand and would require an EL change that should be part of the
J2EE spec. which would be ugly.
Another option is for the user to be able to specify which values
should be local and which should set via EL, but I don't see a clean
way of doing this without making an ugly API (way to many
attribute-metadata attributes).
Before TOMAHAWK-858 is fixed, I would like to see a common approach
that we can take for all of the MyFaces projects so that it is easiest
for users to be familiar with.
Opinions, solutions?
Thanks,
Andrew
