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

[Ed Burns]

>>>>> 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.

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?

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.

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.

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.

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?

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