MyFaces Core
  1. MyFaces Core
  2. MYFACES-2616

Fix UIData state saving model (spec issue 153)

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-beta-2
    • Fix Version/s: 2.1.0
    • Component/s: JSR-314
    • Labels:
      None

      Description

      In short, this topic is an attempt to add full state to components inside UIData. I'll do a brief resume, so people can understand this one easily.

      UIData uses the same component instances to render multiple rows. Suppose this example:

      <h:dataTable id="cities" var="city" value="#

      {country.cities}">
      <h:column>
      <h:outputText value="#{city}" />
      </h:column>
      </h:dataTable>

      In the component tree it is created this hierarchy:

      HtmlDatatable
      UIColumn
      HtmlOutputText

      If we have 10 cities, the same component is used over and over to render all 10 cities. The reason to do that in this way is keep state as small as possible.

      Now let's suppose something like this:

      <h:dataTable id="cities" var="city" value="#{country.cities}

      ">
      <h:column>
      <h:inputText value="#

      {city}

      " />
      </h:column>
      </h:dataTable>

      It was changed the output component for an input one. If this table is in a form and the values are submitted, the same component is used to apply request values, validate and apply them to the model (update process). To make this possible, UIData class has some code to preserve this values between phases (using EditableValueHolder interface), so when each phase is processed, all rows are traversed and you get the expected behavior.

      Now suppose something more complex: We have a code that use invokeOnComponent to change the style of my inputText. In theory, only one row should change. But in fact, all rows are rendered with the same color. Why? because we use the same component to render all rows, and we don't preserve the children component state between rows.

      There is a lot of issues, questions, and side effects related to this issue, but just to put some of them here:

      TOMAHAWK-1062 InputTextArea doesn't work properly inside facet DetailStamp
      TOMAHAWK-96 Data table Scroller not working the dataTable which was actually contained in other DataTable
      https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=153 Problems with UIData state saving

      Also, it is well know that one reason why people uses c:forEach in facelets, is because this one create "full" components per each row. It is very easy to find articles on internet.

      Now, with jsf 2.0 we have partial state saving, so we have a chance to fix this one once and for all. I tried fix this one per months (maybe years!), but talking with Martin Marinschek on JSFDays, some ideas came out and finally it was found a possibility to fix this one using the existing api and with little changes on the spec.

      The proposal is this:

      1. Do not call UIComponent.markInitialState() on ComponentTagHandlerDelegate, as ComponentHandler javadoc says, instead call it after PostAddToViewEvent are published on vdl.buildView(). We need to call it from root to nodes, so the parent component is marked first. I know the place where this call comes is from trinidad tag handler, but this call needs to be fixed in a more predictable way.
      2. Use an attribute on facesContext to identify when the VDL is marking the initial state (in myfaces there is already an attribute called "org.apache.myfaces.MARK_INITIAL_STATE"). This is necessary to indicate UIData instances that it is time to save the full state of all component children,
      3. Allow UIData to hold a map where the key are client ids and the value are the deltas of all components per row. This map should be saved and restored.
      4. Change UIData.setRowIndex() to restore and save the component state.

      I'll attach a patch on this issue with the algorithm proposed (because it is based on myfaces codebase). It was tested and it works. But note it is necessary to fix the javadoc for UIData.markInitialState(), ComponentHandler and maybe vdl.buildView(), so the intention is propose this change for jsf 2.0 rev A. Note this works only with PSS enabled because without it we don't have a place to notify UIData instances that it is necessary to get the full state. Also, note this patch preserve backward compatibility, because the old way to store/save is applied after the full state is restored.

      Really, I have the strong temptation to apply some similar code on myfaces UIRepeat component (because this class is private), but I prefer first ask to EG.

      1. UIDataPreserveRowComponentStateTest.java
        4 kB
        Leonardo Uribe
      2. myfaces-uidata-component-state-test.zip
        34 kB
        Leonardo Uribe
      3. fixUIDataPSS-9-3-no-markInitialStateFix-3.patch
        33 kB
        Leonardo Uribe
      4. fixUIDataPSS-9-3-2.patch
        43 kB
        Leonardo Uribe
      5. fixUIDataPSS-8.patch
        33 kB
        Leonardo Uribe
      6. fixUIDataPSS-7.patch
        28 kB
        Leonardo Uribe
      7. fixUIDataPSS-6.patch
        20 kB
        Leonardo Uribe

        Activity

        Hide
        Leonardo Uribe added a comment -

        Attached patch with fix for transient properties as suggested by Alexander Smirnov. The idea is create an interface that allows to save/restore transient properties like UIForm.submitted, to be saved per row. In that way, the state could be fully restored and saved for each component.

        Show
        Leonardo Uribe added a comment - Attached patch with fix for transient properties as suggested by Alexander Smirnov. The idea is create an interface that allows to save/restore transient properties like UIForm.submitted, to be saved per row. In that way, the state could be fully restored and saved for each component.
        Hide
        Leonardo Uribe added a comment -

        Just one clearification about why do #1:

        Originally, in jsf 1.2 and earlier, trinidad in its tag handler (see org.apache.myfaces.trinidadinternal.facelets.TrinidadComponentHandler) has the call to markInitialState(). It is known that PSS comes from trinidad idea (I checked how it works long time ago), so in mojarra implementation this method is called from its ComponentTagHandlerDelegate. In javax.faces.view.facelets.ComponentHandler javadoc there is a mention of this call without any details.

        The problem is that put this call there makes markInitialState() method be called from nodes to root. Also, aditional calls to markInitialState() are required to be set in other places. The fix on MYFACES-2616 requires the opposite, call from root to nodes, because on UIData.markInitialState() the fix requires put some code that save the full state of all children in a map to be used later (on UIData.setRowIndex() ).

        It could be good to have all calls to markInitialState() in just one place, in one specific moment, and identify when this call comes from vld.buildView or from UIData.setRowIndex() (we need only to save the full state if the call comes from vdl.buildView).

        Show
        Leonardo Uribe added a comment - Just one clearification about why do #1: Originally, in jsf 1.2 and earlier, trinidad in its tag handler (see org.apache.myfaces.trinidadinternal.facelets.TrinidadComponentHandler) has the call to markInitialState(). It is known that PSS comes from trinidad idea (I checked how it works long time ago), so in mojarra implementation this method is called from its ComponentTagHandlerDelegate. In javax.faces.view.facelets.ComponentHandler javadoc there is a mention of this call without any details. The problem is that put this call there makes markInitialState() method be called from nodes to root. Also, aditional calls to markInitialState() are required to be set in other places. The fix on MYFACES-2616 requires the opposite, call from root to nodes, because on UIData.markInitialState() the fix requires put some code that save the full state of all children in a map to be used later (on UIData.setRowIndex() ). It could be good to have all calls to markInitialState() in just one place, in one specific moment, and identify when this call comes from vld.buildView or from UIData.setRowIndex() (we need only to save the full state if the call comes from vdl.buildView).
        Hide
        Martin Marinschek added a comment -

        Hi Leonardo,

        while I see that your approach would be good, I see one problem with this - and that is performance due to the additional tree visit (and the EG has already said that it is not happy about adding more tree-visits).

        Can we find a solution so that your requirements are fulfilled, and no additional tree-visit is necessary?

        best regards,

        Martin

        Show
        Martin Marinschek added a comment - Hi Leonardo, while I see that your approach would be good, I see one problem with this - and that is performance due to the additional tree visit (and the EG has already said that it is not happy about adding more tree-visits). Can we find a solution so that your requirements are fulfilled, and no additional tree-visit is necessary? best regards, Martin
        Hide
        Leonardo Uribe added a comment -

        Attached to this issue there is a proposal to solve this issue. But first I'll do another review to show how all elements "fit" together.

        The objective of the solution proposed is enable component state per row. Please note this idea suggest there is a "relation" between the row model and the components used to render the row. In few words, the model must be "reliable". If the rows in the model are added, removed or sorted, the component state should be reset or something should be done. The patch proposed does not address this problem, because it falls ouside the original scope of the problem.

        To solve this issue, we should consider the existence of variables in a component that should not be saved or restored on the state. Sometimes, there are variables inside the component that should not be saved on the state but they will be used on the lifecycle (let's call them transient variables). An example is UIForm "submitted" variable. This one should be assigned on apply request values phase, but it will be used on later phases.

        Note that a property that could be assigned using view declaration language (jsp or facelets) markup should be on the state, otherwise its value will be reset between requests using jsf 1.2 state saving.

        In theory, any solution to this issue must do something like this:

        • Save the "initial state" of all component children, excluding facets of nearest children (usually instances of UIColumn), after the component tree is build.
        • When setRowIndex method is called, it should save the "partial" or "delta" state of the current row somewhere, change the row, and finally restore the state using the "initial state" of the row and the delta saved, if any.

        Based on the previous ideas, the following (personal) conclusions arise:

        1. Use a property to enable disable this behavior is preferred, because it is possible to have a non "reliable" model, or the user could have situations where it is not required to preserve the state per component. The patches adds a property called "preserveRowComponentState".

        2. It is necessary to know "when" we can calculate the "initial state" of the rows: In the proposed patches, the idea is use UIData.markInitialState and check if we should save the "initial" state. To do that I'm considering two options:

        a. Move markInitialState calls, so they will be called after the component tree is built by facelets vdl. In this way we can ensure we are calling from parents to children, and we can call saveState directly on all children.
        b. Do not move the calls, but take into consideration that some children could already be marked, so for those ones we need to call clearInitialState, saveState and then markInitialState.

        Why two options?.

        Some time ago, myfaces used a listener attached to PostAddToViewEvent for "relocate" components. In theory we have 4 cases: h:outputScript, h:outputStylesheet, cc:insertChildren and cc:insertFacet. PostAddToViewEvent listener is used on h:outputScript and h:outputStylesheet by the spec, but it was necessary to use it on cc:insertChildren and cc:insertFacet, because there was a problem with nested composite components. But right now, after some discussions and new ideas provided, myfaces core 2.0.1 has a algorithm that uses a variant of facelets template client api, so cc:insertChildren and cc:insertFacet does not use a PostAddToViewEvent listener anymore.

        Allow component relocation using PostAddToViewEvent causes a lot of trouble. It is more, add/remove components "programatically" causes a lot of problems too (that's another discussion). Just one example: components relocated using cc:insertChildren or cc:insertFacet does not preserve state, because on each request, the components are relocated and the ones with the state are removed by facelets refresh algorithm, used by c:if tag. The way to detect it is just change a property on a relocated component using invokeOnComponent, but note this is only valid if the view is refreshed before render it.

        The option (a) consider that if PostAddToViewEvent listeners are used to relocate components, only it is safe to save the "initial state" after the tree is completely built, so we require a fix for markInitialState() and some way to indicate we are calling this method from the specified location.

        The option (b) consider that PostAddToViewEvent should not be used to relocate components. If components are relocated, the destiny should be outside a UIData instance (h:outputScript and h:outputStylesheet usually relocates to "head" or "body"), otherwise the "initial state" will be invalid. Since latest myfaces code allows this, the option (b) will work there. Anyway, I provide a patch for mojarra, so people can see how should looks like, but to make it work requires some big changes.

        3. The "delta" state of each row should be saved / restored somewhere with the page: In the proposed patch the deltas are saved with the state of UIData.

        4. A way to save/restore transient variables like UIForm submitted should be provided. The proposed patch includes two new interfaces called TransientStateHelper and TransientStateHolder to deal with this stuff (save/restore and manipulate them). As a curious fact, some days ago, a non very clean solution was applied on "TRINIDAD-1849 subform in table not working" that could be solved better with interfaces like the ones proposed here.

        Additionally, and example was provided to test this one.

        Suggestions, critics and tomatoes are welcome.

        Show
        Leonardo Uribe added a comment - Attached to this issue there is a proposal to solve this issue. But first I'll do another review to show how all elements "fit" together. The objective of the solution proposed is enable component state per row. Please note this idea suggest there is a "relation" between the row model and the components used to render the row. In few words, the model must be "reliable". If the rows in the model are added, removed or sorted, the component state should be reset or something should be done. The patch proposed does not address this problem, because it falls ouside the original scope of the problem. To solve this issue, we should consider the existence of variables in a component that should not be saved or restored on the state. Sometimes, there are variables inside the component that should not be saved on the state but they will be used on the lifecycle (let's call them transient variables). An example is UIForm "submitted" variable. This one should be assigned on apply request values phase, but it will be used on later phases. Note that a property that could be assigned using view declaration language (jsp or facelets) markup should be on the state, otherwise its value will be reset between requests using jsf 1.2 state saving. In theory, any solution to this issue must do something like this: Save the "initial state" of all component children, excluding facets of nearest children (usually instances of UIColumn), after the component tree is build. When setRowIndex method is called, it should save the "partial" or "delta" state of the current row somewhere, change the row, and finally restore the state using the "initial state" of the row and the delta saved, if any. Based on the previous ideas, the following (personal) conclusions arise: 1. Use a property to enable disable this behavior is preferred, because it is possible to have a non "reliable" model, or the user could have situations where it is not required to preserve the state per component. The patches adds a property called "preserveRowComponentState". 2. It is necessary to know "when" we can calculate the "initial state" of the rows: In the proposed patches, the idea is use UIData.markInitialState and check if we should save the "initial" state. To do that I'm considering two options: a. Move markInitialState calls, so they will be called after the component tree is built by facelets vdl. In this way we can ensure we are calling from parents to children, and we can call saveState directly on all children. b. Do not move the calls, but take into consideration that some children could already be marked, so for those ones we need to call clearInitialState, saveState and then markInitialState. Why two options?. Some time ago, myfaces used a listener attached to PostAddToViewEvent for "relocate" components. In theory we have 4 cases: h:outputScript, h:outputStylesheet, cc:insertChildren and cc:insertFacet. PostAddToViewEvent listener is used on h:outputScript and h:outputStylesheet by the spec, but it was necessary to use it on cc:insertChildren and cc:insertFacet, because there was a problem with nested composite components. But right now, after some discussions and new ideas provided, myfaces core 2.0.1 has a algorithm that uses a variant of facelets template client api, so cc:insertChildren and cc:insertFacet does not use a PostAddToViewEvent listener anymore. Allow component relocation using PostAddToViewEvent causes a lot of trouble. It is more, add/remove components "programatically" causes a lot of problems too (that's another discussion). Just one example: components relocated using cc:insertChildren or cc:insertFacet does not preserve state, because on each request, the components are relocated and the ones with the state are removed by facelets refresh algorithm, used by c:if tag. The way to detect it is just change a property on a relocated component using invokeOnComponent, but note this is only valid if the view is refreshed before render it. The option (a) consider that if PostAddToViewEvent listeners are used to relocate components, only it is safe to save the "initial state" after the tree is completely built, so we require a fix for markInitialState() and some way to indicate we are calling this method from the specified location. The option (b) consider that PostAddToViewEvent should not be used to relocate components. If components are relocated, the destiny should be outside a UIData instance (h:outputScript and h:outputStylesheet usually relocates to "head" or "body"), otherwise the "initial state" will be invalid. Since latest myfaces code allows this, the option (b) will work there. Anyway, I provide a patch for mojarra, so people can see how should looks like, but to make it work requires some big changes. 3. The "delta" state of each row should be saved / restored somewhere with the page: In the proposed patch the deltas are saved with the state of UIData. 4. A way to save/restore transient variables like UIForm submitted should be provided. The proposed patch includes two new interfaces called TransientStateHelper and TransientStateHolder to deal with this stuff (save/restore and manipulate them). As a curious fact, some days ago, a non very clean solution was applied on " TRINIDAD-1849 subform in table not working" that could be solved better with interfaces like the ones proposed here. Additionally, and example was provided to test this one. Suggestions, critics and tomatoes are welcome.
        Hide
        Leonardo Uribe added a comment -

        JUnit test for this patches

        Show
        Leonardo Uribe added a comment - JUnit test for this patches

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Leonardo Uribe
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development