[jsr-314-open-mirror] [jsr-314-open] [153-UIDataStateSaving] spec clarifications requested by Andy

[Leonardo Uribe]

Hi

2010/11/3 Ed Burns <edward.burns at oracle.com>

> >>>>> On Tue, 19 Oct 2010 12:35:05 -0400, Andy Schwartz <
> andy.schwartz at oracle.com> said:
>
> EB> When the markInitialState() method is called, save the state of the
> EB> direct children
>
> AS> Is it just the direct children?  Don't we need to save the state for
> AS> non-direct descendents as well?
>
> I see what you mean.  Yes, we should, and I've updated the spec language.
>
> AS> Should we mention that non-iterated facets of UIData are excluded
> AS> from this?
>
> I have mentioned that.
>
> EB> without a dependency on any particular row value. For discussion, this
> EB> rolled up state is called the "initial rolled up state".
>
> AS> Minor issue, but for some reason the term "rolled up state" doesn't
> seem
> AS> to be clicking for me.  I think of this as the "full initial state" -
> AS> ie. we are doing a full state save to capture the initial state of
> these
> AS> components so that we can get back to this later.
>
> I'm going to stick with rolled up state.  The word "initial" is used so
> frequently in this sort of discussion that I don't want to use it any more.
>
> EB> When setRowIndex(int) is called, the following additional actions must
> EB> be taken before the usual call to setRowIndex(int).
>
> AS> Instead of "before the usual call to setRowIndex()", maybe "before any
> AS> other work is performed"?  It isn't clear what "the usual call" means.
>
> EB> Traverse the children as done with markInitialState(), saving the
> EB> state of each child. Because the
> EB> saveState(javax.faces.context.FacesContext) calls during this pass
> EB> happen after the call to markInitialState(), the state saved is the so
> EB> called "delta" state of each component.
>
> AS> What happens is partial state saving is disabled?  Do we still
> implement
> AS> this behavior and just save/restore the full state, or do we bail on
> the
> AS> preserveRowComponentState behavior?
>
> I have clarified that the preserveRowComponentState is not impacted by
> the application partial state saving behavior.
>
> AS> BTW, we don't need to perform this traversal if the current row index
> AS> (the row index at the point in time when setRowIndex() is called) is
> -1.
>
> EB> Because this traversal happens during a per-row operation (in this
> EB> case, setRowIndex(int)) the rolled up state
>
> AS> Okay, so I guess "rolled up" state != "full initial state" - more like
> AS> the per-row saved state.
>
>
> EB> must be saved in a row-aware data structure. One implementation choice
> EB> would be to save the state from this pass in a Map keyed by the return
> EB> from
> UIComponent.getContainerClientId(javax.faces.context.FacesContext).
>
> AS> Though this might not work well if we need to save state for subtrees
> as
> AS> opposed to only direct children - ie. might be better to use the full
> AS> client id.  Perhaps we should leave this implementation suggestion out?
>
> I have done so.
>
> EB> It is permissible for the rolled up state to be null or empty.
> EB>
> EB> If the current row index is not -1, traverse the children as in the
> EB> previous step,
>
> AS> Do we need to perform two separate traversals?  Woudn't it be more
> AS> efficient to consolidate these into a single traversal?
>
> Perhaps, but that's how Leonardo first implemented it and I am not
> inclined to try to optimize it here.
>
>
Yes, it is possible. On the first implementation, I wanted to keep things
simple
and understandable, so I keep them on different methods, but now the
algorithm
is well understood.


> EB> but, instead of calling saveState(javax.faces.context.FacesContext),
> EB> call UIComponent.saveTransientState(javax.faces.context.FacesContext).
> EB> Save the rolled up state in a separate row-aware data structure from
> EB> the one used in the preciding
>
> AS> preceding (sp)
>
> Thanks.
>
> EB> step.
> EB>
> EB> Call DataModel.setRowIndex(int) on the model, as normal. Store the row
> EB> data as request scope attributes, as normal.
> EB>
> EB> If the rolled up state saved during the call to markInitialState() is
> EB> not null or empty,
>
> AS> Since this is the "initial rolled up state" - ie. the full state,
> AS> wouldn't this be non-null?
>
> I'm not sure of the answer to this one.  Leonardo?
>
>
In theory, if UIData.markInitialState is called correctly, the "initial
rolled up
state" is not null, so the check could be skipped. In the first versions of
the
algorithm, the check was used to fallback to the previous algorithm, but now
this is done using a check for the property ( isPreserveRowComponentState()
).

EB> perform the reverse of the two save operations, as described next.
> EB> This is necessary to indicate that transient state from the previous
> EB> row must be discarded, in order to not pollute the state of the
> EB> current row.
>
> EB> If the per-row state saved in step a. above is null, traverse the
> EB> children and restore the child state using the initial rolled up state.
>
> AS> Does the "per-row state saved in step a. above" refer to the stave
> saved
> AS> for the row that we are leaving?  Or for the state that we saved the
> AS> last time that visited the current row?
>
> AS> That is, if we are on row 1 and setRowIndex(2) is called, does the
> AS> "per-row state saved in step a" mean the state produced for row 1
> before
> AS> we adjusted the index, or the state that we previously saved away the
> AS> last time we visited row 2?
>
> AS> This is important to specify clearly.
>
> It's the state before we adjusted the row index.  I've clarified that.
>
> EB> If the per-row state saved in step a. above is not null, traverse the
> EB> children and restore the child state using the state saved during step
> EB> a., using the initial rolled up state only as a backup in the case
> EB> that per-row state is not available.
>
> AS> Okay, so I guess that "per-row state saved in step a. above" refers to
> AS> the previously saved state for the current row.
>
> No, it's the state before we adjusted the row index.  Leonardo, it is
> vitally important that you and Andy and I agree on this.
>
>
The "per-row state saved in step a. above" in other words means the "delta"
state from the row that will be changed. If we are on row 1, to change to
row 2
we need to restore the state of row 2, and check if it is null or not, If it
is null
we use only the initial rolled up state, but if is not, we use both the
initial
rolled up state and the "per-row state saved".


> AS> However, since this is delta state, before we apply this isn't it
> AS> necessary to first restore the components to their initial state by
> AS> restoring the full initial saved state?  If we don't restore the
> AS> components to their initial state before applying the delta state,
> AS> won't we run the risk that state from the previous row might bleed
> AS> over into the current row?  (In the case where the state from the
> AS> previous from was null, this won't be an issue.)
>
> AS> Also, at some point is it necessary to tell the StateHelper that it
> AS> needs to clear out any previously saved deltas so that it can start
> AS> tracking deltas for the currently active row?  Or does that happen
> AS> implicitly at some point, eg. when we restore the row state?
>
> EB> If the current row index is -1, traverse the children and pass null to
> EB> UIComponent.restoreTransientState(javax.faces.context.FacesContext,
> EB> java.lang.Object).
>
> AS> Hopefully we can consolidate the restore-related traversals as well.
>
> EB> If the current row index is not -1, take the following actions.
>
> EB> If the per-row state saved in step b. above is null, traverse the
> EB> children and restore the transient state by passing null to each
> EB> child's
> EB> UIComponent.restoreTransientState(javax.faces.context.FacesContext,
> EB> java.lang.Object) method
> EB>
> EB> If the per-row state saved in step b. above is not null, traverse the
> EB> children and restore the transient state from the state saved in step
> EB> b. above, calling
> EB> UIComponent.restoreTransientState(javax.faces.context.FacesContext,
> EB> java.lang.Object) on each child, and passing the appropriate state.
>
> AS> Sounds like we are doing the same thing whether or not the transient
> AS> state is null, so perhaps we can simplify the wording.
>
> Perhaps, but I'm concerned with getting it wrong.  The current text was
> approved by Leonardo so I'm inclined to go with it.
>
>
In few words, it just say how to restore the "transient state". It is
possible in this
part I just forget to simplify things.

The time I wrote that part I was thinking if there was some kind of
manipulation
on transient state properties before build view time, those values should be

"propagated" to child rows, but the intention of this state is to have a
very
short life time (vdl build time or render time). If a value is necessary
to be stored more than the current request, use StateHelper. After thinking
about
it carefully, the conclusion was that is not necessary, so we can just
assume a
"null" or "empty" state for the transient map.


> AS> Few other questions:
>
> AS> 1.  Leonardo raised an issue regarding the timing of when
> AS> markInitialState() is called - ie. that markInitialState() needs to be
> AS> called on the parent before the children - otherwise UIData won't be
> AS> able to capture the full initial state of its children.  How did we
> AS> solve this problem?  Are there spec changes relating to this?  Did we
> AS> find a way to do this that doesn't require introducing yet another full
> AS> tree traversal?
>
> I don't know.
>
>
It is possible to prevent the full tree traversal, but to do that it is
necessary to
change all code that uses PostAddToViewEvent to relocate components, and
that
means refactor composite component solution as is. This was done on MyFaces,
so there cc:insertChildren and cc:insertFacet does not use a Listener
attached to
that event there, but Mojarra uses it.

Anyway, users could add components on PostAddToViewEvent to the tree, so
in theory the call to markInitialState() should be done after that event is
handled.

AS> 2.  Regarding the name of the new property...
> AS> is/setPreserveRowComponentState() is a bit of a mouthful.  Could we
> AS> maybe shorten this to is/setRowStatePreserved()?
>
> I like that a lot better.
>
> AS> 3.  Are there cases where it might be useful to enable per-row
> transient
> AS> state saving without also enabling non-transient state saving (which
> AS> seems more expensive)?  I wonder whether it should be possible to
> enable
> AS> these independently, in which case we may want to consider using an
> enum
> AS> property instead of a boolean.  Also wondering whether transient row
> AS> state saving should just be on by default or possibly always on.
>
> Leonardo?
>
>
It is possible, but shouldn't that condition just depends on the component
child itself ?.
For example, UIForm has submitted property that in the future will be stored
in transient
map, so this component requires transient state be always on.

best regards,

Leonardo Uribe


> Ed
> --
> | edward.burns at oracle.com | office: +1 407 458 0017
> | homepage:               | http://ridingthecrest.com/
> |  5 work days until German Oracle User's Group Conference
> | 12 work days until GlassFish 3.1 Hard Code Freeze
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/jsr-314-open-mirror/attachments/20101103/ab86377e/attachment-0002.html