[jsr-314-open-mirror] [jsr-314-open] [2.1 Spec Review] UIData state saving

[Andy Schwartz]

Gang -

I am reviewing the 2.1 Java API changes today.  Decided to start with 
UIData state saving.  Comments on the doc for 
UIData.setPreserveRowComponentState():


> When the markInitialState() method is called, save the state of the 
> direct children

Is it just the direct children?  Don't we need to save the state for 
non-direct descendents as well?  Should we mention that non-iterated 
facets of UIData are excluded from this?



> of this instance by traversing them as if this component was a normal 
> UIComponent instance, not a UIData, and calling 
> saveState(javax.faces.context.FacesContext) on each, collecting the 
> state as the traversal proceeds. State collected in this manner will 
> be referred to as "rolled up" state. Store the resultant rolled up 
> state in a data structure with a scope equivalent to that of an 
> instance variable. This is necessary so that state can be restored 
> without a dependency on any particular row value. For discussion, this 
> rolled up state is called the "initial rolled up state".

Minor issue, but for some reason the term "rolled up state" doesn't seem 
to be clicking for me.  I think of this as the "full initial state" - 
ie. we are doing a full state save to capture the initial state of these 
components so that we can get back to this later.


>
> When setRowIndex(int) is called, the following additional actions must 
> be taken before the usual call to setRowIndex(int).

Instead of "before the usual call to setRowIndex()", maybe "before any 
other work is performed"?  It isn't clear what "the usual call" means.


>
> Traverse the children as done with markInitialState(), saving the 
> state of each child. Because the 
> saveState(javax.faces.context.FacesContext) calls during this pass 
> happen after the call to markInitialState(), the state saved is the so 
> called "delta" state of each component.

What happens is partial state saving is disabled?  Do we still implement 
this behavior and just save/restore the full state, or do we bail on the 
preserveRowComponentState behavior?

BTW, we don't need to perform this traversal if the current row index 
(the row index at the point in time when setRowIndex() is called) is -1.


> Because this traversal happens during a per-row operation (in this 
> case, setRowIndex(int)) the rolled up state

Okay, so I guess "rolled up" state != "full initial state" - more like 
the per-row saved state.


> must be saved in a row-aware data structure. One implementation choice 
> would be to save the state from this pass in a Map keyed by the return 
> from UIComponent.getContainerClientId(javax.faces.context.FacesContext).

Though this might not work well if we need to save state for subtrees as 
opposed to only direct children - ie. might be better to use the full 
client id.  Perhaps we should leave this implementation suggestion out?


> It is permissible for the rolled up state to be null or empty.
>
> If the current row index is not -1, traverse the children as in the 
> previous step,

Do we need to perform two separate traversals?  Woudn't it be more 
efficient to consolidate these into a single traversal?


> but, instead of calling saveState(javax.faces.context.FacesContext), 
> call UIComponent.saveTransientState(javax.faces.context.FacesContext). 
> Save the rolled up state in a separate row-aware data structure from 
> the one used in the preciding

preceding (sp)


> step.
>
> Call DataModel.setRowIndex(int) on the model, as normal. Store the row 
> data as request scope attributes, as normal.
>
> If the rolled up state saved during the call to markInitialState() is 
> not null or empty,

Since this is the "initial rolled up state" - ie. the full state, 
wouldn't this be non-null?

I am mostly asking just because I want to make sure I understand how 
this all fits together.  The reference to null/empty state makes me 
think that this might be partial state, but I believe we are dealing 
with full state here.



> perform the reverse of the two save operations, as described next. 
> This is necessary to indicate that transient state from the previous 
> row must be discarded, in order to not pollute the state of the 
> current row.
>
> If the per-row state saved in step a. above is null, traverse the 
> children and restore the child state using the initial rolled up state.

Does the "per-row state saved in step a. above" refer to the stave saved 
for the row that we are leaving?  Or for the state that we saved the 
last time that visited the current row?

That is, if we are on row 1 and setRowIndex(2) is called, does the 
"per-row state saved in step a" mean the state produced for row 1 before 
we adjusted the index, or the state that we previously saved away the 
last time we visited row 2?

This is important to specify clearly.


>
> If the per-row state saved in step a. above is not null, traverse the 
> children and restore the child state using the state saved during step 
> a., using the initial rolled up state only as a backup in the case 
> that per-row state is not available.

Okay, so I guess that "per-row state saved in step a. above" refers to 
the previously saved state for the current row.  However, since this is 
delta state, before we apply this isn't it necessary to first restore 
the components to their initial state by restoring the full initial 
saved state?  If we don't restore the components to their initial state 
before applying the delta state, won't we run the risk that state from 
the previous row might bleed over into the current row?  (In the case 
where the state from the previous from was null, this won't be an issue.)

Also, at some point is it necessary to tell the StateHelper that it 
needs to clear out any previously saved deltas so that it can start 
tracking deltas for the currently active row?  Or does that happen 
implicitly at some point, eg. when we restore the row state?

>
> If the current row index is -1, traverse the children and pass null to 
> UIComponent.restoreTransientState(javax.faces.context.FacesContext, 
> java.lang.Object).

Hopefully we can consolidate the restore-related traversals as well.

>
> If the current row index is not -1, take the following actions.
>
> If the per-row state saved in step b. above is null, traverse the 
> children and restore the transient state by passing null to each 
> child's 
> UIComponent.restoreTransientState(javax.faces.context.FacesContext, 
> java.lang.Object) method
>
> If the per-row state saved in step b. above is not null, traverse the 
> children and restore the transient state from the state saved in step 
> b. above, calling 
> UIComponent.restoreTransientState(javax.faces.context.FacesContext, 
> java.lang.Object) on each child, and passing the appropriate state.

Sounds like we are doing the same thing whether or not the transient 
state is null, so perhaps we can simplify the wording.

Few other questions:

1.  Leonardo raised an issue regarding the timing of when 
markInitialState() is called - ie. that markInitialState() needs to be 
called on the parent before the children - otherwise UIData won't be 
able to capture the full initial state of its children.  How did we 
solve this problem?  Are there spec changes relating to this?  Did we 
find a way to do this that doesn't require introducing yet another full 
tree traversal?

2.  Regarding the name of the new property...  
is/setPreserveRowComponentState() is a bit of a mouthful.  Could we 
maybe shorten this to is/setRowStatePreserved()?

3.  Are there cases where it might be useful to enable per-row transient 
state saving without also enabling non-transient state saving (which 
seems more expensive)?  I wonder whether it should be possible to enable 
these independently, in which case we may want to consider using an enum 
property instead of a boolean.  Also wondering whether transient row 
state saving should just be on by default or possibly always on.

Andy