2010/8/13 Ed Burns
<edward.burns@oracle.com>
>>>>> On Mon, 26 Jul 2010 08:14:42 +0200, Martin Marinschek <mmarinschek@apache.org> said:
MM> Hi guys,
MM> sorry, it took me a while to review the patch, but this is Leonardo´s
MM> suggestion for partial state saving in tables. He also included a test
MM> webapp, and a test for Mojarra.
It's no problem. I'm not surprised it took so long. For me, at least,
this is a very complicated issue to understand. At this point in the
life of JSF, it's these kinds of issues that remain unsolved.
Here is my response.
https://javaserverfaces-spec-public.dev.java.net/nonav/issues/showattachment.cgi/268/message.txt
Included inline for convenience.
I'm now going to just apply Leonardo's patch and see what happens to our
existing automated tests.
Note by default this feature is not active, so I think to test it fully it is necessary to activate it setting
preserveRowComponentState to true.
LU> In theory, any solution to this issue must do something like this:
LU> - Save the "initial state" of all component children, excluding
LU> facets of nearest children (usually instances of UIColumn), after
LU> the component tree is build.
This is just the normal mark initial state, right?
Yes, but for UIData, it must save the initial state of all children in a structure, so
when setRowIndex() is called, it can restore the component state as it was before markInitialState().
LU> - When setRowIndex method is called, it should save the "partial" or
LU> "delta" state of the current row somewhere, change the row, and
LU> finally restore the state using the "initial state" of the row and
LU> the delta saved, if any.
Yes, this makes sense.
LU> Based on the previous ideas, the following (personal) conclusions
LU> arise:
LU> 1. Use a property to enable disable this behavior is preferred,
LU> because it is possible to have a non "reliable" model, or the user
LU> could have situations where it is not required to preserve the state
LU> per component. The patches adds a property called
LU> "preserveRowComponentState".
That's fine with me.
LU> 2. It is necessary to know "when" we can calculate the "initial
LU> state" of the rows: In the proposed patches, the idea is use
LU> UIData.markInitialState and check if we should save the "initial"
LU> state. To do that I'm considering two options:
LU> a. Move markInitialState calls, so they will be called after the
LU> component tree is built by facelets vdl. In this way we can ensure
LU> we are calling from parents to children, and we can call saveState
LU> directly on all children.
LU> b. Do not move the calls, but take into consideration that some
LU> children could already be marked, so for those ones we need to call
LU> clearInitialState, saveState and then markInitialState.
LU> Why two options?.
LU> Some time ago, myfaces used a listener attached to
LU> PostAddToViewEvent for "relocate" components. In theory we have 4
LU> cases: h:outputScript, h:outputStylesheet, cc:insertChildren and
LU> cc:insertFacet.
Mojarra still uses a listener for this.
LU> PostAddToViewEvent listener is used on
LU> h:outputScript and h:outputStylesheet by the spec, but it was
LU> necessary to use it on cc:insertChildren and cc:insertFacet, because
LU> there was a problem with nested composite components. But right now,
LU> after some discussions and new ideas provided, myfaces core 2.0.1
LU> has a algorithm that uses a variant of facelets template client api,
LU> so cc:insertChildren and cc:insertFacet does not use a
LU> PostAddToViewEvent listener anymore.
Andy Schwartz suggested Mojarra move to using the template client
approach for cc:insertChildren and cc:insertFacet in issue 1680 [1]. We
opted for a simpler option to allow the Facelets tagHandler to correctly
locate the relocated child in the postback case. Therefore, I'm very
reluctant to adopt a fix to UIData state saving that will break our fix
for 1680.
In myfaces we did something similar, but after thinking more about it and take
into consideration the semantic of ui:composition, ui:define and ui:insert it
becomes necessary to change it. The way suggested by Andy is better, because
it handle correctly interactions between nested composite components and
template tags (TemplateContext concept). I have some tests that shows it,
but I think it is worth to propose this change on mojarra.
LU> Allow component relocation using PostAddToViewEvent causes a lot
LU> of trouble. It is more, add/remove components "programatically"
LU> causes a lot of problems too (that's another discussion). Just one
LU> example: components relocated using cc:insertChildren or
LU> cc:insertFacet does not preserve state, because on each request, the
LU> components are relocated and the ones with the state are removed by
LU> facelets refresh algorithm, used by c:if tag.
This problem with c:if has been fixed with in issue 1527 [2]. If you
find that the solution in that issue is faulty, please let the Mojarra
team know.
Ok, I'll take a look to this point.
LU> The option (a) consider that if PostAddToViewEvent listeners are
LU> used to relocate components, only it is safe to save the "initial
LU> state" after the tree is completely built, so we require a fix for
LU> markInitialState() and some way to indicate we are calling this
LU> method from the specified location.
This sounds reasonable.
LU> The option (b) consider that PostAddToViewEvent should not be used
LU> to relocate components. If components are relocated, the destiny
LU> should be outside a UIData instance (h:outputScript and
LU> h:outputStylesheet usually relocates to "head" or "body"), otherwise
LU> the "initial state" will be invalid. Since latest myfaces code
LU> allows this, the option (b) will work there. Anyway, I provide a
LU> patch for mojarra, so people can see how should looks like, but to
LU> make it work requires some big changes.
Yes, it would. And we're not in a position to want to make big changes
now.
LU> 3. The "delta" state of each row should be saved / restored
LU> somewhere with the page: In the proposed patch the deltas are saved
LU> with the state of UIData.
That is a reasonable place to save it.
LU> 4. A way to save/restore transient variables like UIForm submitted
LU> should be provided. The proposed patch includes two new interfaces
LU> called TransientStateHelper and TransientStateHolder to deal with
LU> this stuff (save/restore and manipulate them). As a curious fact,
LU> some days ago, a non very clean solution was applied on
LU> "TRINIDAD-1849 subform in table not working" that could be solved
LU> better with interfaces like the ones proposed here.
Ok, this sounds good too. I'm going to try to understand the patch now,
but I think you've done some great work. I just hope that it can be
made to apply to Mojarra as well as your MyFaces.
Thanks Ed for take a look to this issue.
best regards,
Leonardo Uribe
[1] https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1680
[2] https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1527
Ed
--
| edward.burns@oracle.com | office: +1 407 458 0017
| homepage: | http://ridingthecrest.com/
| 03 work days until JSF 2.1 Milestone 2