MyFaces Core
  1. MyFaces Core
  2. MYFACES-228

Sortheader functionality does not work if datatable contains editable components

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.0.9m9
    • Fix Version/s: 1.1.0
    • Component/s: None
    • Labels:
      None
    • Environment:
      WindowsXP, JDK 1.4.1_06, Tomcat 5.5

      Description

      I'm using a dataTable with sortheader. Everything is fine as long as I
      use outputText instead of a inputText. If I use inputText, the
      List/array is sorted correct on the srever side when my value binding is invkoked by the datatable, but the old model (unsorted) is displayed.

      I debuged that a bit and recognized, that as soon as I click any command button, the sorted list gets displayed correctly. This is because refresh() is called in the datatable when clicking a commandButton. Refresh isn't called when u click on the sort header command. On the other hand using outputText works fine. While looking at the HtmlDataTableHack class i found

      private static int restoreDescendantComponentStates(UIComponent component,
      EditableValueHolderState[] states,
      EditableValueHolderState[] initialStates,
      int counter, int level)
      {
      for (Iterator it=getChildrenAndOptionalFacetsIterator(level, component); it.hasNext()
      {
      UIComponent child = (UIComponent)it.next();
      //clear this descendant's clientId:
      child.setId(child.getId()); //HACK: This assumes that setId always clears the cached clientId. Can we be sure?
      if (child instanceof EditableValueHolder)
      {
      if (states != null)

      { states[counter].restore((EditableValueHolder)child); }

      else if (initialStates != null)

      { initialStates[counter].restore((EditableValueHolder)child); }

      else

      { // No state saved yet and no initial state !? // Should never be possible, but let's reset the component // state to null values ((EditableValueHolder)child).setValue(null); ((EditableValueHolder)child).setLocalValueSet(false); ((EditableValueHolder)child).setValid(true); ((EditableValueHolder)child).setSubmittedValue(null); }

      counter++;
      }
      counter = restoreDescendantComponentStates(child, states, initialStates, counter,level+1);
      }
      return counter;
      }

      Not sure, but it seems that this codes cuases the old model state to be rendered and not the newly sorted one. If I have time, I will track it down a bit more.

      You can simply reproduce this by changing the sortTable example. You
      only need to change outputText into inputText.

        Activity

        Hide
        Rolf Kulemann added a comment -

        Sorry, I use JDK 1.4.2_06 and not 1.4.1

        Show
        Rolf Kulemann added a comment - Sorry, I use JDK 1.4.2_06 and not 1.4.1
        Hide
        Alex Mayer added a comment -

        I can confirm the non-sorting behaviour of the sortheader in case of using a selectbooleancheckbox in the table (in each row).

        I use JDK1.5.0_3 and Tomcat 5.5

        Show
        Alex Mayer added a comment - I can confirm the non-sorting behaviour of the sortheader in case of using a selectbooleancheckbox in the table (in each row). I use JDK1.5.0_3 and Tomcat 5.5
        Hide
        sean schofield added a comment -

        I also verified the problem by modifying the simple example as Rolf suggested. I noticed that the sort functionality does in fact work but the values in h:inputText disappear. In other words, the rows are sorted correctly based on what values are in the h:inputText even though the values are not being displayed.

        Show
        sean schofield added a comment - I also verified the problem by modifying the simple example as Rolf suggested. I noticed that the sort functionality does in fact work but the values in h:inputText disappear. In other words, the rows are sorted correctly based on what values are in the h:inputText even though the values are not being displayed.
        Hide
        Rolf Kulemann added a comment -

        The values ONLY disappear, if the datable with sortheader is used OUTSIDE a form.
        If you change the sortTable example this way, so put the datatble inside a form component, the old values appear (unsorted)

        Show
        Rolf Kulemann added a comment - The values ONLY disappear, if the datable with sortheader is used OUTSIDE a form. If you change the sortTable example this way, so put the datatble inside a form component, the old values appear (unsorted)
        Hide
        Rolf Kulemann added a comment -

        NOTE: I told you that refresh() is not called when clicking on a sort link. Thats not true. when I debug the myfaces code (HtmlDataTable and HtmlDataTable) it seems that the call stack is the same for normal "actions" and the sortheader action. Ic an not loose the feeling that the method restoreDescendantComponentStates is the evil place. But to be honest, I'm not yet so familiar with the myfaces code and tracking things down is hard for me

        Show
        Rolf Kulemann added a comment - NOTE: I told you that refresh() is not called when clicking on a sort link. Thats not true. when I debug the myfaces code (HtmlDataTable and HtmlDataTable) it seems that the call stack is the same for normal "actions" and the sortheader action. Ic an not loose the feeling that the method restoreDescendantComponentStates is the evil place. But to be honest, I'm not yet so familiar with the myfaces code and tracking things down is hard for me
        Hide
        Rolf Kulemann added a comment -

        I guess I found the offending code. But before I attach a patch, I would like to discuss the problem based on code a bit more, because there are some things I do not understand.

        The offendning class is http://svn.apache.org/viewcvs.cgi/myfaces/trunk/src/share/org/apache/myfaces/renderkit/RendererUtils.java?rev=167744&view=markup

        If you look at the methods getStringValue, getDateValue, and getBooleanValue, the problem/bug is obvious. Lets look at getStringValue for example:
        ------------------------------------------------
        public static String getStringValue(FacesContext facesContext,
        UIComponent component)
        {
        try
        {
        if (!(component instanceof ValueHolder))

        { throw new IllegalArgumentException("Component : "+getPathToComponent(component)+"is not a ValueHolder"); }

        if (component instanceof EditableValueHolder)
        {
        Object submittedValue = ((EditableValueHolder)component).getSubmittedValue();
        if (submittedValue != null)
        {
        if (submittedValue instanceof String)

        { return (String)submittedValue; }
        else
        { throw new IllegalArgumentException("Expected submitted value of type String for component : " +getPathToComponent(component)); }
        }
        }

        Object value = ((ValueHolder)component).getValue();

        ....
        return value.toString();
        ------------------------------------------------

        As soon as an EditableValueHolder should be rendered, the submmited value is rendered and not the value backed by the underlying model. In case of the sortheader action, this is wrong, since the old unsorted model will be rendered.

        I guess there is a good reason why the submitted value is rendered, BUT I can not understand why such a distiction is made in the render process, since IMHO only values from the model should be redndered. I have commented out the hole section
        -------------------------------------------------
        /*if (component instanceof EditableValueHolder)
        {
        Object submittedValue = ((EditableValueHolder)component).getSubmittedValue();
        if (submittedValue != null)
        {
        if (submittedValue instanceof String)
        { return (String)submittedValue; }

        else

        { throw new IllegalArgumentException("Expected submitted value of type String for component : " +getPathToComponent(component)); }

        }
        }*/
        -------------------------------------------------

        and sorting works just fine. Dunno what side effects this would cause. So what is the reason for this check? Again, only values from the model should be rendered, but I guess there is a reason, why you render the submitted values of EditableValueHolders and not the values backed by the model.

        Answers?

        Show
        Rolf Kulemann added a comment - I guess I found the offending code. But before I attach a patch, I would like to discuss the problem based on code a bit more, because there are some things I do not understand. The offendning class is http://svn.apache.org/viewcvs.cgi/myfaces/trunk/src/share/org/apache/myfaces/renderkit/RendererUtils.java?rev=167744&view=markup If you look at the methods getStringValue, getDateValue, and getBooleanValue, the problem/bug is obvious. Lets look at getStringValue for example: ------------------------------------------------ public static String getStringValue(FacesContext facesContext, UIComponent component) { try { if (!(component instanceof ValueHolder)) { throw new IllegalArgumentException("Component : "+getPathToComponent(component)+"is not a ValueHolder"); } if (component instanceof EditableValueHolder) { Object submittedValue = ((EditableValueHolder)component).getSubmittedValue(); if (submittedValue != null) { if (submittedValue instanceof String) { return (String)submittedValue; } else { throw new IllegalArgumentException("Expected submitted value of type String for component : " +getPathToComponent(component)); } } } Object value = ((ValueHolder)component).getValue(); .... return value.toString(); ------------------------------------------------ As soon as an EditableValueHolder should be rendered, the submmited value is rendered and not the value backed by the underlying model. In case of the sortheader action, this is wrong, since the old unsorted model will be rendered. I guess there is a good reason why the submitted value is rendered, BUT I can not understand why such a distiction is made in the render process, since IMHO only values from the model should be redndered. I have commented out the hole section ------------------------------------------------- /*if (component instanceof EditableValueHolder) { Object submittedValue = ((EditableValueHolder)component).getSubmittedValue(); if (submittedValue != null) { if (submittedValue instanceof String) { return (String)submittedValue; } else { throw new IllegalArgumentException("Expected submitted value of type String for component : " +getPathToComponent(component)); } } }*/ ------------------------------------------------- and sorting works just fine. Dunno what side effects this would cause. So what is the reason for this check? Again, only values from the model should be rendered, but I guess there is a reason, why you render the submitted values of EditableValueHolders and not the values backed by the model. Answers?
        Hide
        sean schofield added a comment -

        What do the committers think about this? There are tons of refs to the getStringValue method. My guess is that the reason why the submitted value is used is so if there is a validation error, the incorrect value can be rendered without changing the model. So this behavior is probably correct although clearly not desirable in the case of a sortable data table.

        Show
        sean schofield added a comment - What do the committers think about this? There are tons of refs to the getStringValue method. My guess is that the reason why the submitted value is used is so if there is a validation error, the incorrect value can be rendered without changing the model. So this behavior is probably correct although clearly not desirable in the case of a sortable data table.
        Hide
        Oliver Rossmueller added a comment -

        I suppose the bug is in HtmlCommandSortHeaderTag where attribute 'immediate' is set to 'true' for the sort header command link. This way the corresponding action event is handled after ApplyRequestValues phase which is too early in case there are input components in the data table. The submitted form values will be submitted to the model in UpdateModelValues phase, so the fix might be to set 'immediate=false' for the sort header command links so the action event will be handled after the InvokeApplication phase when all the form values are in the model.

        Show
        Oliver Rossmueller added a comment - I suppose the bug is in HtmlCommandSortHeaderTag where attribute 'immediate' is set to 'true' for the sort header command link. This way the corresponding action event is handled after ApplyRequestValues phase which is too early in case there are input components in the data table. The submitted form values will be submitted to the model in UpdateModelValues phase, so the fix might be to set 'immediate=false' for the sort header command links so the action event will be handled after the InvokeApplication phase when all the form values are in the model.
        Hide
        sean schofield added a comment -

        This doesn't seem to solve the problem. I tried commenting out the offending code and got the same result. Note that the problem occurs without changes to the values of any of the input components. Just sorting the columns with no changes to the values results in the values not being rendered correctly (or at all.) I haven't tried changing the input components yet - there may be a problem with that too in which case I think Oliver's suggestion might do the trick.

        Show
        sean schofield added a comment - This doesn't seem to solve the problem. I tried commenting out the offending code and got the same result. Note that the problem occurs without changes to the values of any of the input components. Just sorting the columns with no changes to the values results in the values not being rendered correctly (or at all.) I haven't tried changing the input components yet - there may be a problem with that too in which case I think Oliver's suggestion might do the trick.
        Hide
        sean schofield added a comment -

        I think we need immediate=true in this case. We don't want to update the model or validate anything when the users requests a sort. So these steps should be skipped (as they are now with immediate = true.)

        Show
        sean schofield added a comment - I think we need immediate=true in this case. We don't want to update the model or validate anything when the users requests a sort. So these steps should be skipped (as they are now with immediate = true.)
        Hide
        Oliver Rossmueller added a comment -

        I found some time to dig a little bit deeper and in fact immediate=true/false is not the problem. Here are my findings:

        o) surround the datatable by <h:form> and sorting works with inputText
        o) change line 86 of org.apache.myfaces.renderkit.html.HtmlRendererUtils to

        ((EditableValueHolder) component).setSubmittedValue(null);

        and sorting works also without surrounding <h:form>

        I'm not sure why submittedValue is set to empty string as in the current implementation and started a corresponding thread on the dev list. The line above is the potential fix to this issue but might raise some other issue. Let's see if there is an answer on the list.

        Show
        Oliver Rossmueller added a comment - I found some time to dig a little bit deeper and in fact immediate=true/false is not the problem. Here are my findings: o) surround the datatable by <h:form> and sorting works with inputText o) change line 86 of org.apache.myfaces.renderkit.html.HtmlRendererUtils to ((EditableValueHolder) component).setSubmittedValue(null); and sorting works also without surrounding <h:form> I'm not sure why submittedValue is set to empty string as in the current implementation and started a corresponding thread on the dev list. The line above is the potential fix to this issue but might raise some other issue. Let's see if there is an answer on the list.
        Hide
        sean schofield added a comment -

        Martin's change improves things. Its along the lines of what Oliver suggested. Here is the code snippet for those not following the discussion on the dev list:

        if(isDisabledOrReadOnly(component))
        return;

        ((EditableValueHolder) component).setSubmittedValue(paramMap
        .get(clientId));

        There are still some issues concerning editable values in a sortable table that need to be addressed. First, what is the expected/desired behavior when you have say a checkbox in a datatable and then your sort it? Should your changes to the checkbox be preserved when you sort? Is that what the average user would expect? IMO the answer is "yes" but what I would expect and how I would do it are two different issues

        Can we agree this is the desired behavior?

        Show
        sean schofield added a comment - Martin's change improves things. Its along the lines of what Oliver suggested. Here is the code snippet for those not following the discussion on the dev list: if(isDisabledOrReadOnly(component)) return; ((EditableValueHolder) component).setSubmittedValue(paramMap .get(clientId)); There are still some issues concerning editable values in a sortable table that need to be addressed. First, what is the expected/desired behavior when you have say a checkbox in a datatable and then your sort it? Should your changes to the checkbox be preserved when you sort? Is that what the average user would expect? IMO the answer is "yes" but what I would expect and how I would do it are two different issues Can we agree this is the desired behavior?
        Hide
        Oliver Rossmueller added a comment -

        IMO the answer is "yes", too. I guess no user would understand the alternative behaviour where some values are entered in input fields and then after sorting the column all the old values are back again. I for sure would take this kind of behaviour as a bug.

        Show
        Oliver Rossmueller added a comment - IMO the answer is "yes", too. I guess no user would understand the alternative behaviour where some values are entered in input fields and then after sorting the column all the old values are back again. I for sure would take this kind of behaviour as a bug.
        Hide
        Rolf Kulemann added a comment -

        Oliver wrote:

        <em>
        I found some time to dig a little bit deeper and in fact immediate=true/false is not the problem. Here are my findings:
        o) surround the datatable by <h:form> and sorting works with inputText
        ....
        </em>

        In our project all our data tables are surrounded by h:form tags, but the sort functionality does not work. We use the last official MyFaces release. Maybe the behavior has changed in SVN trunk. Oliver, can you send me/post your test JSP/JSF file where u tried to surround the data table with a h:form tag?

        Show
        Rolf Kulemann added a comment - Oliver wrote: <em> I found some time to dig a little bit deeper and in fact immediate=true/false is not the problem. Here are my findings: o) surround the datatable by <h:form> and sorting works with inputText .... </em> In our project all our data tables are surrounded by h:form tags, but the sort functionality does not work. We use the last official MyFaces release. Maybe the behavior has changed in SVN trunk. Oliver, can you send me/post your test JSP/JSF file where u tried to surround the data table with a h:form tag?
        Hide
        Oliver Rossmueller added a comment -

        Rolf,

        I just used the sorted datatable example from the examples webapp and embedded the data table within a form. Sorting worked for me with that setup and the (at that time) latest trunk.

        Show
        Oliver Rossmueller added a comment - Rolf, I just used the sorted datatable example from the examples webapp and embedded the data table within a form. Sorting worked for me with that setup and the (at that time) latest trunk.
        Hide
        sean schofield added a comment -

        Mathias Broekelmann has contributed some patches to UIData and other classes in an attempt to fix this. I've committed these but the problem remains. If you do not use <h:form> then the data sorts but the values are lost (maybe this is unavoidable?) If you include an <h:form> then the data is not sorted at all.

        Show
        sean schofield added a comment - Mathias Broekelmann has contributed some patches to UIData and other classes in an attempt to fix this. I've committed these but the problem remains. If you do not use <h:form> then the data sorts but the values are lost (maybe this is unavoidable?) If you include an <h:form> then the data is not sorted at all.
        Hide
        sean schofield added a comment -

        Latest patch has not improved things much as far as this specific problem goes. If you use immediate="false" in the sort header you still have a problem.

        Show
        sean schofield added a comment - Latest patch has not improved things much as far as this specific problem goes. If you use immediate="false" in the sort header you still have a problem.
        Hide
        Mathias Broekelmann added a comment -

        The patch should solve the bug.

        Show
        Mathias Broekelmann added a comment - The patch should solve the bug.
        Hide
        sean schofield added a comment -

        I had a problem applying the patch but I applied manually and still experienced problems. Stacktraces are gone but now clicking on sortheader or data scroller does nothing. I checked the latest patch in so please make sure to do an update.

        Show
        sean schofield added a comment - I had a problem applying the patch but I applied manually and still experienced problems. Stacktraces are gone but now clicking on sortheader or data scroller does nothing. I checked the latest patch in so please make sure to do an update.
        Hide
        Mathias Broekelmann added a comment -

        I´ve verified the repository state with my working copy and found a difference. I forgot to add a new listener for the simple examples which may cause a stacktrace. But that´s may be not the problem.

        I´ve testet it with the simple examples (like the Paged and Sortable Data Table example) and it works pretty well. It works as expected with input fields edititing and immediatly sorting or scrolling works nice.

        Show
        Mathias Broekelmann added a comment - I´ve verified the repository state with my working copy and found a difference. I forgot to add a new listener for the simple examples which may cause a stacktrace. But that´s may be not the problem. I´ve testet it with the simple examples (like the Paged and Sortable Data Table example) and it works pretty well. It works as expected with input fields edititing and immediatly sorting or scrolling works nice.
        Hide
        Rolf Kulemann added a comment -

        I have tested a nightly JSF build (20051207 dowloaded from http://svn.apache.org/builds/myfaces/nightly/). The problem is the same with the sortHeader example as soon as you surround the dataTable with a h:form tag,
        Any ideas?

        BTW: I have problems to check out jsf from svn. I followed the instructions on http://myfaces.apache.org/community/versioning.html. That is why I tried the nightly.

        Show
        Rolf Kulemann added a comment - I have tested a nightly JSF build (20051207 dowloaded from http://svn.apache.org/builds/myfaces/nightly/ ). The problem is the same with the sortHeader example as soon as you surround the dataTable with a h:form tag, Any ideas? BTW: I have problems to check out jsf from svn. I followed the instructions on http://myfaces.apache.org/community/versioning.html . That is why I tried the nightly.
        Hide
        Rolf Kulemann added a comment -

        I found a solution for the sample. I set immediate="false" on the sort header and the sample sorts even within a form tag.

        <h:column>
        <f:facet name="header">
        <x:commandSortHeader columnName="type" arrow="true" immediate="false">
        <h:outputText value="#

        {example_messages['sort_cartype']}

        " />
        </x:commandSortHeader>
        </f:facet>
        <h:inputText value="#

        {car.type}

        " />
        <f:facet name="footer">
        <h:outputText id="ftr1" value="(footer col1)" />
        </f:facet>
        </h:column>

        Show
        Rolf Kulemann added a comment - I found a solution for the sample. I set immediate="false" on the sort header and the sample sorts even within a form tag. <h:column> <f:facet name="header"> <x:commandSortHeader columnName="type" arrow="true" immediate="false"> <h:outputText value="# {example_messages['sort_cartype']} " /> </x:commandSortHeader> </f:facet> <h:inputText value="# {car.type} " /> <f:facet name="footer"> <h:outputText id="ftr1" value="(footer col1)" /> </f:facet> </h:column>
        Hide
        Mathias Broekelmann added a comment -

        I think we should change the default setting for the immediate flag to false for the sortheader component.

        Show
        Mathias Broekelmann added a comment - I think we should change the default setting for the immediate flag to false for the sortheader component.
        Hide
        sean schofield added a comment -

        Mathias, I agree that we should change the default. Go ahead and submit a patch if you want. I also tested the examples that you worked with and confirmed everything works fine so lat step is for me to try with more sophisticated example ... It looks like we can wrap this one up shortly.

        Show
        sean schofield added a comment - Mathias, I agree that we should change the default. Go ahead and submit a patch if you want. I also tested the examples that you worked with and confirmed everything works fine so lat step is for me to try with more sophisticated example ... It looks like we can wrap this one up shortly.
        Hide
        Rolf Kulemann added a comment -

        NOTE: I changed one outputField to an inputField in the sortHeader sample! Maybe you can change the sample that way in SVN.

        Show
        Rolf Kulemann added a comment - NOTE: I changed one outputField to an inputField in the sortHeader sample! Maybe you can change the sample that way in SVN.
        Hide
        sean schofield added a comment -

        Mathias, I tried the latest version of openDataTable.jsp. I see that you changed the fields to an input field but the input value is lost when sorting. Do you think its possible to make it work in this case (with the "open-ended" number of columns)? If not we can change this example back to output:Text and mark this bug as closed.

        Show
        sean schofield added a comment - Mathias, I tried the latest version of openDataTable.jsp. I see that you changed the fields to an input field but the input value is lost when sorting. Do you think its possible to make it work in this case (with the "open-ended" number of columns)? If not we can change this example back to output:Text and mark this bug as closed.
        Hide
        Mathias Broekelmann added a comment -

        Strange. It definitly works on my box with input fields. I´m able to sort the dynamic columns right after filling some inputfields and those values are not lost - just sorted as expected.
        Have you update all other things I´ve commited ?

        Show
        Mathias Broekelmann added a comment - Strange. It definitly works on my box with input fields. I´m able to sort the dynamic columns right after filling some inputfields and those values are not lost - just sorted as expected. Have you update all other things I´ve commited ?
        Hide
        Bruno Aranda added a comment -

        It works for me also. Have in mind that all the items in the dataTable are sorted and if you put an item that starts with 'z', for instance, it will appear in the last page or in the first page, depending on the sort...

        Show
        Bruno Aranda added a comment - It works for me also. Have in mind that all the items in the dataTable are sorted and if you put an item that starts with 'z', for instance, it will appear in the last page or in the first page, depending on the sort...
        Hide
        Mathias Broekelmann added a comment -

        I close this issue since the examples are working now with input fields and everything works as expected as long as the immediate flag for the sortheader is set to false.

        Show
        Mathias Broekelmann added a comment - I close this issue since the examples are working now with input fields and everything works as expected as long as the immediate flag for the sortheader is set to false.

          People

          • Assignee:
            sean schofield
            Reporter:
            Rolf Kulemann
          • Votes:
            4 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development