Hi<br><br><div class="gmail_quote">2010/3/29 Martin Marinschek <span dir="ltr"><<a href="mailto:mmarinschek@apache.org">mmarinschek@apache.org</a>></span><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hi guys,<br>
<br>
Iīll work with Leonardoīs proposal:<br>
<div class="im"><br>
1. Do not call UIComponent.markInitialState() on<br>
ComponentTagHandlerDelegate, as ComponentHandler javadoc says, instead<br>
call it after PostAddToViewEvent are published on vdl.buildView(). We<br>
need to call it from root to nodes, so the parent component is marked<br>
first. I know the place where this call comes is from trinidad tag<br>
handler, but this call needs to be fixed in a more predictable way.<br>
<br>
</div>I think I believe I understand why you need this, but can you explain<br>
it further?<br>
<div class="im"><br></div></blockquote><div><br>In few words, facelets algorithm create components and attach them from leaves<br>to root. That means, when markInitialState() is called, all children was already<br>marked. The last one that needs to be marked is root, but in this case it is<br>
responsibility of vdl.buildView() to mark it. If new components are attached during<br>the process (for example, the panel storing components inside <br>composite:implementation) we should mark the too. In theory, all components<br>
on the tree should be marked in this step (in the patch I'm excluding transient ones<br>but thinking more about it we should not).<br><br>If we have some code on UIData.markInitialState() we have to call clearInitialState<br>
on all children, save the component initial state and call again markInitialState. The<br>idea behind do not call UIComponent.markInitialState() on ComponentTagHandlerDelegate, <br>is in avoid do that in reverse order and just do it in one step. <br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im">
2. Use an attribute on facesContext to identify when the VDL is<br>
marking the initial state (in myfaces there is already an attribute<br>
called "org.apache.myfaces.MARK_INITIAL_STATE"). This is necessary to<br>
indicate UIData instances that it is time to save the full state of<br>
all component children,<br>
<br>
</div>+1 on this! we should always have an API to know what JSF is doing<br>
where in its lifecycle<br>
<div class="im"><br>
3. Allow UIData to hold a map where the key are client ids and the<br>
value are the deltas of all components per row. This map should be<br>
saved and restored.<br>
<br>
</div>Isn't that an implementation detail? If we talk about implementation<br>
details, I think we should have the full state of all sub-components<br>
once, and the deltas per row (just like the full-state now).<br>
<div class="im"><br></div></blockquote><div><br>Yes, that is an implementation detail, and the code proposed do what you<br>said, we have full state of one row, saved when UIData.markInitialState() is called:<br><br>private Object _initialDescendantFullComponentState = null;<br>
<br> and a map of delta per row:<br><br>private Map<String, Object> _rowDeltaStates = new HashMap<String, Object>();<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im">
4. Change UIData.setRowIndex() to restore and save the component state.<br>
<br>
</div>Isnīt UIData doing this as of today?<br>
<br></blockquote><div><br>Yes. The current implementation just save and restore components that implements<br>EditableValueHolder interface, but only per request, so those values are not saved<br>on the state. The code proposed additionally save and restore the delta, and the<br>
delta per row is saved on UIData state.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
By the way - I donīt like Alexanderīs idea of having a<br>
restoreRowState/saveRowState (this additional API would only be useful<br>
inside repeater-components). Instead, I would propagate a<br>
getTransientAttributesMap() API, and the repeating-component would<br>
just take care of saving/restoring this as well. I believe this API<br>
could be used for a lot of other stuff. It would essentially a<br>
component-request scope, which we donīt have yet, and for which we had<br>
quite a few usages in cs-JSF already.<br>
<br>
We could also embed this state in the stateMap with a transient=true<br>
attribute, thatīs the other option - then it wonīt be a public API<br>
however.<br>
<br></blockquote><div><br>The important here is provide methods to save and restore that map/variable set. <br>As long as we have some way to do that, we could call it from <br>UIData.setRowIndex. I like the idea to extend StateHelper with accesors <br>
and save/restore methods for transient components.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
best regards,<br>
<br>
Martin<br>
<br>
-<br>
<div><div></div><div class="h5"><br>
On Sun, Mar 21, 2010 at 7:21 PM, Leonardo Uribe <<a href="mailto:lu4242@gmail.com">lu4242@gmail.com</a>> wrote:<br>
> Hi<br>
><br>
> It is probably that myfaces implementation of UIData do something different<br>
> that RI. Myfaces uses a map to hold dataModel instances to handle better<br>
> the use case when nested datatables occur.<br>
><br>
> The problems described on the last email are different to the one I'm trying<br>
> to<br>
> solve. My only intention is make note that it is possible to solve the<br>
> problem<br>
> described on the original proposal, only do very small changes on the spec<br>
> (in other words the scope of the proposal is not that huge, the dataModel<br>
> handling is different from the component state handling, but both problems<br>
> are related).<br>
><br>
> Really I would like to see a way to call markInitialState like the one<br>
> described.<br>
> The current way to call it is a chaos. I had to put this call on different<br>
> tag handlers<br>
> and the order how this method is called is the inverse one than someone<br>
> could<br>
> expect. Without this change, if someone wants to do something similar needs<br>
> to<br>
> call clearInitialState, save and then call again to markInitialState on the<br>
> code<br>
> that saves the state.<br>
><br>
> The other change described, a param to indicate when the current view is<br>
> using<br>
> partial state saving and all calls to markInitialState should occur, to<br>
> diferentiate<br>
> between the call when the view is built and the call when a row is set is<br>
> document<br>
> an implementation detail that both myfaces and mojarra contains (I suppose,<br>
> otherwise how ComponentHandlerDelegate implementation could know when<br>
> do not call this method if the current view is not using PSS ). Note the<br>
> documentation<br>
> of ComponentHandler just says this method is called but does not says<br>
> anything<br>
> about the conditions, so the implementation should decide that but there is<br>
> no<br>
> formal way to comunicate this state, but if you look closely,<br>
> ComponentHandler<br>
> "delegate" all that steps described on the javadoc, so in reality this class<br>
> does<br>
> not do anything described there and that can be considered an error in the<br>
> javadoc.<br>
> All that documentation should be on<br>
> TagHandlerDelegateFactory.createComponentHandlerDelegate.<br>
><br>
> regards,<br>
><br>
> Leonardo Uribe<br>
><br>
> 2010/3/19 Alexander Smirnov <<a href="mailto:asmirnov@exadel.com">asmirnov@exadel.com</a>><br>
>><br>
>> The problem is far worse then you think :-( UIData itself has transient<br>
>> properties, the most important of them is dataModel. Standalone UIData<br>
>> caches model between phases, but resets it if you have enclosed<br>
>> iteration components that gives huge impact on performance if data model<br>
>> does not cache results itself. In Mojarra, UIRepeat knows about UIData<br>
>> but latest has no clue about repeater, so if you put data table inside<br>
>> the <ui:repeat> , results will be unpredictable. Is MyFaces has a better<br>
>> implementation ? I haven't seen deep into its UIData code for a while...<br>
>> Also, it is permanent pain in the complex components that require to<br>
>> store some information during request or work like iterator ( that are<br>
>> really all trees, menus, tab panels, Tomahawk columns, and more ).<br>
>> Numeric row ids in the data model is additional limitation for models<br>
>> that connected to the database in concurrent environment there<br>
>> additional code requires to keep references between row number and<br>
>> primary key; otherwise application have a chance to update/delete wrong<br>
>> record.<br>
>> For these reasons, I think that all UIRepeat/UIData should be rewritten<br>
>> from scratch as it already done in the Trinidad/ADF. I'll take a closer<br>
>> look to your patch.<br>
>><br>
>> On 03/19/2010 12:07 PM, Leonardo Uribe wrote:<br>
>> > Hi<br>
>> ><br>
>> > Yes, I know this too. In few words, all "transient" properties, that<br>
>> > means,<br>
>> > all properties that are not stored in the state are not saved. There are<br>
>> > very<br>
>> > few examples. To solve this one, there are multiple alternatives. I was<br>
>> > thinking<br>
>> > on extend StateHelper interface to add some methods like<br>
>> > putTransientProperty and getTransientProperty. Trinidad stores all<br>
>> > properties<br>
>> > on the FacesBean object.<br>
>> ><br>
>> > Anyway, I think an "mixed" solution could solve this problem: create an<br>
>> > interface called RowStateTransientHolder, so only the components that<br>
>> > implements this type of attributes could have the chance to<br>
>> > save and restore its state.<br>
>> ><br>
>> > But note in any case, it is necessary to do something like this to solve<br>
>> > the<br>
>> > use case described.<br>
>> ><br>
>> > I didn't knew that localValueSet was a transient property too.<br>
>> ><br>
>> > regards,<br>
>> ><br>
>> > Leonardo Uribe<br>
>> ><br>
>> > 2010/3/19 Alexander Smirnov <<a href="mailto:asmirnov@exadel.com">asmirnov@exadel.com</a><br>
>> > <mailto:<a href="mailto:asmirnov@exadel.com">asmirnov@exadel.com</a>>><br>
>> ><br>
>> > I've thought about these problems for a long time too. Your proposal<br>
>> > has<br>
>> > a disadvantage because per-row state is quite different from the<br>
>> > component state. Take a look for the UIInput implementation:<br>
>> > localValueSet field is never saved in the state while it should be<br>
>> > vary<br>
>> > from row to row. The same thing happens to the UIForm.submitted<br>
>> > attribute, that has been eventually added to the UIData spec instead<br>
>> > of<br>
>> > common solution, that would save/restore all local variables. My<br>
>> > proposal was a separate RowStateHolder interface with<br>
>> > save/restoreRowState methods there component developer can preserve<br>
>> > internal variables. In the your stenario, there should be some flag<br>
>> > that<br>
>> > tells component to save all values, even these that are not supposed<br>
>> > to<br>
>> > be preserved between requests.<br>
>> ><br>
>> > On 03/19/2010 10:27 AM, Leonardo Uribe wrote:<br>
>> > > In short, this topic is an attempt to add full state to components<br>
>> > > inside UIData. I'll do a brief resume, so people can understand<br>
>> > this one<br>
>> > > easily.<br>
>> > ><br>
>> > > UIData uses the same component instances to render multiple rows.<br>
>> > > Suppose this example:<br>
>> > ><br>
>> > > <h:dataTable id="cities" var="city" value="#{country.cities}"><br>
>> > > <h:column><br>
>> > > <h:outputText value="#{city}" /><br>
>> > > </h:column><br>
>> > > </h:dataTable><br>
>> > ><br>
>> > > In the component tree it is created this hierarchy:<br>
>> > ><br>
>> > > HtmlDatatable<br>
>> > > UIColumn<br>
>> > > HtmlOutputText<br>
>> > ><br>
>> > > If we have 10 cities, the same component is used over and over to<br>
>> > render<br>
>> > > all 10 cities. The reason to do that in this way is keep state as<br>
>> > small<br>
>> > > as possible.<br>
>> > ><br>
>> > > Now let's suppose something like this:<br>
>> > ><br>
>> > > <h:dataTable id="cities" var="city" value="#{country.cities}"><br>
>> > > <h:column><br>
>> > > <h:inputText value="#{city}" /><br>
>> > > </h:column><br>
>> > > </h:dataTable><br>
>> > ><br>
>> > > It was changed the output component for an input one. If this<br>
>> > table is<br>
>> > > in a form and the values are submitted, the same component is used<br>
>> > to<br>
>> > > apply request values, validate and apply them to the model (update<br>
>> > > process). To make this possible, UIData class has some code to<br>
>> > preserve<br>
>> > > this values between phases (using EditableValueHolder interface),<br>
>> > so<br>
>> > > when each phase is processed, all rows are traversed and you get<br>
>> > the<br>
>> > > expected behavior.<br>
>> > ><br>
>> > > Now suppose something more complex: We have a code that use<br>
>> > > invokeOnComponent to change the style of my inputText. In theory,<br>
>> > only<br>
>> > > one row should change. But in fact, all rows are rendered with the<br>
>> > same<br>
>> > > color. Why? because we use the same component to render all rows,<br>
>> > and we<br>
>> > > don't preserve the children component state between rows.<br>
>> > ><br>
>> > > There is a lot of issues, questions, and side effects related to<br>
>> > this<br>
>> > > issue, but just to put some of them here:<br>
>> > ><br>
>> > > TOMAHAWK-1062 InputTextArea doesn't work properly inside facet<br>
>> > DetailStamp<br>
>> > > TOMAHAWK-96 Data table Scroller not working the dataTable which<br>
>> > was<br>
>> > > actually contained in other DataTable<br>
>> > ><br>
>> ><br>
>> > <a href="https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=153" target="_blank">https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=153</a><br>
>> > > Problems with UIData state saving<br>
>> > ><br>
>> > > Also, it is well know that one reason why people uses c:forEach in<br>
>> > > facelets, is because this one create "full" components per each<br>
>> > row. It<br>
>> > > is very easy to find articles on internet.<br>
>> > ><br>
>> > > Now, with jsf 2.0 we have partial state saving, so we have a<br>
>> > chance to<br>
>> > > fix this one once and for all. I tried fix this one per months<br>
>> > (maybe<br>
>> > > years!), but talking with Martin Marinschek on JSFDays, some ideas<br>
>> > came<br>
>> > > out and finally it was found a possibility to fix this one using<br>
>> > the<br>
>> > > existing api and with little changes on the spec.<br>
>> > ><br>
>> > > The proposal is this:<br>
>> > ><br>
>> > > 1. Do not call UIComponent.markInitialState() on<br>
>> > > ComponentTagHandlerDelegate, as ComponentHandler javadoc says,<br>
>> > instead<br>
>> > > call it after PostAddToViewEvent are published on vdl.buildView().<br>
>> > We<br>
>> > > need to call it from root to nodes, so the parent component is<br>
>> > marked<br>
>> > > first. I know the place where this call comes is from trinidad tag<br>
>> > > handler, but this call needs to be fixed in a more predictable<br>
>> > way.<br>
>> > > 2. Use an attribute on facesContext to identify when the VDL is<br>
>> > marking<br>
>> > > the initial state (in myfaces there is already an attribute called<br>
>> > > "org.apache.myfaces.MARK_INITIAL_STATE"). This is necessary to<br>
>> > indicate<br>
>> > > UIData instances that it is time to save the full state of all<br>
>> > component<br>
>> > > children,<br>
>> > > 3. Allow UIData to hold a map where the key are client ids and the<br>
>> > value<br>
>> > > are the deltas of all components per row. This map should be saved<br>
>> > and<br>
>> > > restored.<br>
>> > > 4. Change UIData.setRowIndex() to restore and save the component<br>
>> > state.<br>
>> > ><br>
>> > > I'll attach a patch on myfaces issue tracker with the algorithm<br>
>> > proposed<br>
>> > > (because it is based on myfaces codebase). It was tested and it<br>
>> > works.<br>
>> > > But note it is necessary to fix the javadoc for<br>
>> > > UIData.markInitialState(), ComponentHandler and maybe<br>
>> > vdl.buildView(),<br>
>> > > so the intention is propose this change for jsf 2.0 rev A. Note<br>
>> > this<br>
>> > > works only with PSS enabled because without it we don't have a<br>
>> > place to<br>
>> > > notify UIData instances that it is necessary to get the full<br>
>> > state.<br>
>> > > Also, note this patch preserve backward compatibility, because the<br>
>> > old<br>
>> > > way to store/save is applied after the full state is restored.<br>
>> > ><br>
>> > > Really, I have the strong temptation to apply some similar code on<br>
>> > > myfaces UIRepeat component (because this class is private), but I<br>
>> > prefer<br>
>> > > first ask to EG to know what you guys think about it.<br>
>> > ><br>
>> > > Suggestions are welcome,<br>
>> > ><br>
>> > > regards,<br>
>> > ><br>
>> > > Leonardo Uribe<br>
>> > ><br>
>> ><br>
>> ><br>
><br>
><br>
<br>
<br>
<br>
</div></div>--<br>
<div><div></div><div class="h5"><br>
<a href="http://www.irian.at" target="_blank">http://www.irian.at</a><br>
<br>
Your JSF powerhouse -<br>
JSF Consulting, Development and<br>
Courses in English and German<br>
<br>
Professional Support for Apache MyFaces<br>
</div></div></blockquote></div><br>