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