Hi
Sorry for the late response. I'm still on vacations, so I'm not
reviewing my mail frequently these days.
2011/12/22 Edward Burns <edward.burns_at_oracle.com>:
>>>>>> On Tue, 2 Aug 2011 22:54:03 -0500, Leonardo Uribe <lu4242_at_gmail.com> said:
>
> LU> Hi
> LU> I have attached a patch for myfaces (2.0.x) here:
>
> LU> https://issues.apache.org/jira/browse/MYFACES-3257
>
> LU> Note the algorithm in some point call processSaveState(), to save a
> LU> "branch" and then return VisitResult.REJECT. Additionally, UIViewRoot
> LU> is saved at last, because some information is saved there that will be
> LU> used later to restore the tree. In practice, this could cause a
> LU> slightly different behavior between MyFaces and Mojarra, because both
> LU> algorithms do the same in a different way.
>
> LU> It could be good to know:
>
> LU> 1. If the patch proposed comply with the proposed clarification.
>
> Rather than risk making a mistake in interpreting your patch let me
> re-state here the resolution I just committed:
>
> M jsf-api/src/main/java/javax/faces/application/Application.java
>
> mark {get,set}StateManager() as deprecated, pointing to the new
> equivalents.
>
> M jsf-api/src/main/java/javax/faces/application/StateManager.java
> M jsf-api/src/main/java/javax/faces/application/StateManagerWrapper.java
>
> Deprecate these classes, pointing to the new equivalents.
>
> M jsf-api/src/main/java/javax/faces/view/StateManagementStrategy.java
>
> require the use of the visit API to perform the saving.
>
> This last change is just appending this text to the class javadocs for
> StateManagementStrategy.
>
> * <p class="changed_added_2_2">Implementations must call
> * {_at_link javax.faces.component.UIComponent#visitTree} on the
> * {_at_link javax.faces.component.UIViewRoot} to perform the saving and restoring
> * of the view in the {_at_link #saveView} and {_at_link #restoreView} methods,
> * respectively.
> * </p>
>
> Does that fit with your changes, Leonardo?
>
I only have one observation about this change. In MyFaces I use
invokeOnComponent
for restoreView and visitTree for saveView. The reason behind that is
UIData.visitTree
description does not deal with clientIds that does not contain row
information like
UIData.invokeOnComponent does.
Suppose you have this fragment:
<h:dataTable id="table">
<h:column>
<h:outputText id="text"/>
</h:column>
</h:dataTable>
When the state is restored, in some situations it is possible to have
state associated
to "table:text", but this clientId is only used to locate the "real"
component instance,
because in a strict sense, since the container is a UIData instance
there are many
"virtual" components with state per row (no matter if you are using
rowStatePreserved="true" or not). The problem is if you do a call to
UIData.visitTree
trying to find "table:text", the specified algorithm does not check if
the clientId defines
any row or not and if no row definition is found then the child are
not scanned. But
UIData.invokeOnComponent do that check and call the right code.
In conclusion, to add the previous suggested documentation we need to specify
how UIData.visitTree should behave in cases when the state is
restored, and fix that
part properly on all components that descend from UIData.
Checking the javadoc, I also notice h:dataTable component does not have
rowStatePreserved property is not on the spec javadoc.
As a final comment, doing some enhancements in MyFaces into its PSS
algorithm I notice a use case that breaks rowStatePreserved feature of UIData
components. Suppose a fragment like this:
<c:if ....>
<h:dataTable rowStatePreserved="true">
.....
</h:dataTable>
</c:if>
Currently, this case fails because when h:dataTable is added to the
component tree, UIData.markInitialState() could be already called, so the
"snapshot" of the initial state is never stored. I think we need to
add an additional
method on UIData and other similar components to take the "snapshot" of
the initial state per row (this step is done internally on
UIData.markInitialState()).
Note in this case we can't call UIData.markInitialState(), because this part
of the component tree has been added dynamically and we need to store that
information inside the state
> LU> 2. If the patch should be applied on 2.0.x or since 2.1.x.
>
> I am only suggesting this change for 2.2. We would have to do a JCP
> revision of the spec to backport it to 2.1. Do you want that?
>
I think it is ok to let it for 2.2, unless Andy has a problem with
this for Trinidad.
Note the change agreed in MyFaces over saveView (use visitTree) was already
done in 2.0.x and 2.1.x branches.
regards,
Leonardo Uribe
> Ed
>
>
> --
> | edward.burns_at_oracle.com | office: +1 407 458 0017
> | homepage: | http://ridingthecrest.com/