MyFaces Core
  1. MyFaces Core
  2. MYFACES-2418

Implement h:selectManyXXX collectionType and hideNoSelectionOption

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0-beta
    • Component/s: JSR-314
    • Labels:
      None
    1. myfaces-2418-tck-problems-shared.patch
      28 kB
      Jakob Korherr
    2. myfaces-2418-tck-problems-proposal.patch
      37 kB
      Jakob Korherr
    3. myfaces-2418-tck-problems-api.patch
      24 kB
      Jakob Korherr
    4. myfaces-2418-tck-problem-part2.patch
      15 kB
      Jakob Korherr
    5. myfaces-2418.patch
      21 kB
      Jakob Korherr

      Activity

      Hide
      Jakob Korherr added a comment -

      Here is my patch for this issue. It solves both, the collectionType attribute (I already implemented a part of this functionality in MYFACES-2136, I just added some more Collection support in this patch) and the hideNoSelectionOption attribute.

      While implementing the hideNoSelectionOption attribute, I discovered two little bugs: when h:selectManyCheckbox or h:selectOneRadio render SelectItemGroups, the IDs of the checkbox/radio-elements are not unique and they close TD and TR elements, which they maybe never opened before (I sometimes got a warning from ResponseWriterImpl). The patch fixes this two bugs too.

      Show
      Jakob Korherr added a comment - Here is my patch for this issue. It solves both, the collectionType attribute (I already implemented a part of this functionality in MYFACES-2136 , I just added some more Collection support in this patch) and the hideNoSelectionOption attribute. While implementing the hideNoSelectionOption attribute, I discovered two little bugs: when h:selectManyCheckbox or h:selectOneRadio render SelectItemGroups, the IDs of the checkbox/radio-elements are not unique and they close TD and TR elements, which they maybe never opened before (I sometimes got a warning from ResponseWriterImpl). The patch fixes this two bugs too.
      Hide
      Leonardo Uribe added a comment -

      I applied the patch but still there are two problems. It seems the new algorithm described in UISelectMany spec javadoc for (... the Renderer for this component must perform the following logic for getConvertedValue().... ) do something not documented but very helpful.

      If you have a bean with a method like this:

      public Object getSomeFieldValue()

      { ..... }

      and you have something like

      <h:selectManyXXX value="#{bean.someFieldValue}"/>

      and when the view is rendered first this method returns null, no conversion is done. Myfaces throws an exception since the return value is not an array or collection.

      The second problem is when you have something like this

      public List<Integer> getSomeFieldValue(){ ..... }

      In this case, ri works but myfaces fails when try to convert the value. In other words, the algorithm should detect the type and use the proper converter retrieved from target class (in this case Integer).

      Since the documentation is poor in this case (and maybe wrong!), but the functionality is obviously wanted, on myfaces side we should do the same, testing different cases on ri and fixing the algorithm. Note this two cases are tested by TCK test.

      The only mention about the intention of this stuff is here:

      https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=230

      Show
      Leonardo Uribe added a comment - I applied the patch but still there are two problems. It seems the new algorithm described in UISelectMany spec javadoc for (... the Renderer for this component must perform the following logic for getConvertedValue().... ) do something not documented but very helpful. If you have a bean with a method like this: public Object getSomeFieldValue() { ..... } and you have something like <h:selectManyXXX value="#{bean.someFieldValue}"/> and when the view is rendered first this method returns null, no conversion is done. Myfaces throws an exception since the return value is not an array or collection. The second problem is when you have something like this public List<Integer> getSomeFieldValue(){ ..... } In this case, ri works but myfaces fails when try to convert the value. In other words, the algorithm should detect the type and use the proper converter retrieved from target class (in this case Integer). Since the documentation is poor in this case (and maybe wrong!), but the functionality is obviously wanted, on myfaces side we should do the same, testing different cases on ri and fixing the algorithm. Note this two cases are tested by TCK test. The only mention about the intention of this stuff is here: https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=230
      Hide
      Jakob Korherr added a comment -

      I tested these two scenarios an mojarra and I already have a solution for the first problem.

      The second problem is a bit strange. If you use something like this

      public List<Integer> getSomeFieldValue()

      { ..... }

      on mojarra and you also have SelectItems containing Integers, mojarra renders the SelectItems, while MyFaces already fails here, because it cannot get a Converter to convert the Integers to Strings for rendering. This is, of course, easy to solve (we just have to take a look at the value type of the SelectItems).

      However, if you submit your selections on mojarra it adds the submitted values, which are of type String, without any conversion to the ArrayList, which will be set in setSomeFieldValue(List<Integer> value). This works, because java does not check the type at runtime.
      But now we have the problem that when we want to access the values, we will get Strings instead of the expected Integers, which may cause «strange looking» ClassCastExceptions in the user code.

      Should we behave exactly like mojarra at this point, which on my opinion is stupid and may confuse a lot of developers, or should we try to obtain a Converter some other way?
      I think looking at the type of the SelectItems will provide us a valid converter and would be a better way.

      The spec, of course, does not mention anything about this.

      Show
      Jakob Korherr added a comment - I tested these two scenarios an mojarra and I already have a solution for the first problem. The second problem is a bit strange. If you use something like this public List<Integer> getSomeFieldValue() { ..... } on mojarra and you also have SelectItems containing Integers, mojarra renders the SelectItems, while MyFaces already fails here, because it cannot get a Converter to convert the Integers to Strings for rendering. This is, of course, easy to solve (we just have to take a look at the value type of the SelectItems). However, if you submit your selections on mojarra it adds the submitted values, which are of type String, without any conversion to the ArrayList, which will be set in setSomeFieldValue(List<Integer> value). This works, because java does not check the type at runtime. But now we have the problem that when we want to access the values, we will get Strings instead of the expected Integers, which may cause «strange looking» ClassCastExceptions in the user code. Should we behave exactly like mojarra at this point, which on my opinion is stupid and may confuse a lot of developers, or should we try to obtain a Converter some other way? I think looking at the type of the SelectItems will provide us a valid converter and would be a better way. The spec, of course, does not mention anything about this.
      Hide
      Jakob Korherr added a comment -

      Here is my patch, which solves these problems. The patch uses the values of the SelectItems of the UISelectMany to obtain a suitable Converter (as mentioned in my previous comment).

      Show
      Jakob Korherr added a comment - Here is my patch, which solves these problems. The patch uses the values of the SelectItems of the UISelectMany to obtain a suitable Converter (as mentioned in my previous comment).
      Hide
      Jakob Korherr added a comment -

      Testing some cases in mojarra and again in myfaces, mojarra does not fail if the itemValue attribute is not provided on f:selectItems when value does not contain pure SelectItems. Mojarra behaves like var and itemValue attributes were set in the following way: var="item" itemValue="#

      {item}

      ".

      This patch solves this particular problem.

      Show
      Jakob Korherr added a comment - Testing some cases in mojarra and again in myfaces, mojarra does not fail if the itemValue attribute is not provided on f:selectItems when value does not contain pure SelectItems. Mojarra behaves like var and itemValue attributes were set in the following way: var="item" itemValue="# {item} ". This patch solves this particular problem.
      Hide
      Jakob Korherr added a comment -

      I merged the patches and splitted them to the api part and the shared part.

      Show
      Jakob Korherr added a comment - I merged the patches and splitted them to the api part and the shared part.
      Hide
      Leonardo Uribe added a comment -

      Thanks to Jakob Korherr for this patch

      Show
      Leonardo Uribe added a comment - Thanks to Jakob Korherr for this patch

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development