ICEfaces
  1. ICEfaces
  2. ICE-1445

RowSelector Events in wrong Phase

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.3, 1.6DR#1, 1.6DR#2
    • Fix Version/s: 1.7.1
    • Component/s: ICE-Components
    • Labels:
      None
    • Environment:
      Any

      Description

      The select event of the RowSelector component is explicitly fired in the RowSelectorRenderer during the APPLY_REQUEST_VALUES phase (see code below). This means that during the UPDATE_MODEL_VALUES phase all backing bean changes made by the select listener will be overwritten.

      Code:
       if (rowSelector.getSelectionListener() != null) {
                           RowSelectorEvent evt = new RowSelectorEvent(rowSelector, rowIndex, b);
                           evt.setPhaseId(PhaseId.APPLY_REQUEST_VALUES);
       
                           rowSelector.queueEvent(evt);
       }
       

      The event should better by fired in the INVOKE_APPLICATION phase e.g.

      Code:
       if (rowSelector.getSelectionListener() != null) {
                           RowSelectorEvent evt = new RowSelectorEvent(rowSelector, rowIndex, b);
                           evt.setPhaseId(PhaseId.APPLY_REQUEST_VALUES);
       
                           rowSelector.queueEvent(evt);
       }

      A similar issue applies to the ValueChangeEvent fired by the SelectInputText component. For some reasons the event is fired in the PROCESS_VALIDATIONS phase witch leads to the same problem as described above. Changes made at the backing bean will be overwritten in the UPDATE_MODEL_VALUES phase.

        Issue Links

          Activity

          Hide
          Mark Collette added a comment -

          The selectionListener is fired then, because we now skip all of the other phases, and go straight to render, so that no validations will be done.

          Show
          Mark Collette added a comment - The selectionListener is fired then, because we now skip all of the other phases, and go straight to render, so that no validations will be done.
          Hide
          Thomas Greve added a comment -

          This bug is still present in 1.7.0, cf. this thread http://www.icefaces.org/JForum/posts/list/0/8645.page. Any plans to fix it?

          Show
          Thomas Greve added a comment - This bug is still present in 1.7.0, cf. this thread http://www.icefaces.org/JForum/posts/list/0/8645.page . Any plans to fix it?
          Hide
          Mark Collette added a comment -

          I will elaborate on how this is an intentional feature, and not a bug. A lot of the time, when selecting a row, one doesn't want the validators, for other input components in the form, to fire. By having this event happen at APPLY_REQUEST_VALUES, you have the ability to call FacesContext.renderResponse() in your selectionListener, to skip validations. Also, if you need to do processing with inputted values, after UPDATE_MODEL_VALUES, then you can just re-queue the event to INVOKE_APPLICATION. By broadcasting the event at an early point in time, then it allows maximum flexibility.

          Show
          Mark Collette added a comment - I will elaborate on how this is an intentional feature, and not a bug. A lot of the time, when selecting a row, one doesn't want the validators, for other input components in the form, to fire. By having this event happen at APPLY_REQUEST_VALUES, you have the ability to call FacesContext.renderResponse() in your selectionListener, to skip validations. Also, if you need to do processing with inputted values, after UPDATE_MODEL_VALUES, then you can just re-queue the event to INVOKE_APPLICATION. By broadcasting the event at an early point in time, then it allows maximum flexibility.
          Hide
          Mark Collette added a comment -

          To simplify the process of describing which phase to have the selctionListener and selectionAction be executed, I've added the immediate property, where immediate="true" means ApplyRequestValues phase and immediate="false" means InvokeApplication phase. For backwards compatibility, the default value is immediate="true". So now you shouldn't have to deal with re-queuing events if you need it in InvokeApplication.

          TRUNK
          Subversion 16882
          icefaces\component-metadata\src\main\resources\conf\ice_cust_properties\cust-rowselector-props.xml
          icefaces\component\src\com\icesoft\faces\component\ext\RowSelector.java

          ICEfaces 1.7 branch
          Subversion 16883
          icefaces\component-metadata\src\main\resources\conf\ice_cust_properties\cust-rowselector-props.xml
          icefaces\component\src\com\icesoft\faces\component\ext\RowSelector.java

          Show
          Mark Collette added a comment - To simplify the process of describing which phase to have the selctionListener and selectionAction be executed, I've added the immediate property, where immediate="true" means ApplyRequestValues phase and immediate="false" means InvokeApplication phase. For backwards compatibility, the default value is immediate="true". So now you shouldn't have to deal with re-queuing events if you need it in InvokeApplication. TRUNK Subversion 16882 icefaces\component-metadata\src\main\resources\conf\ice_cust_properties\cust-rowselector-props.xml icefaces\component\src\com\icesoft\faces\component\ext\RowSelector.java ICEfaces 1.7 branch Subversion 16883 icefaces\component-metadata\src\main\resources\conf\ice_cust_properties\cust-rowselector-props.xml icefaces\component\src\com\icesoft\faces\component\ext\RowSelector.java
          Hide
          Thomas Greve added a comment -

          The introduction of the "immediate" property is exactly what was required here.
          However, I still consider the original behaviour being a bug.
          If a JSP contains a commandButton and that button just changes some internal state without the validation being called, this button explicitly has to be set to immediate. This is the very purpose of that parameter, failing to do so is a bug in the JSP..
          If a JSP contains a RowSelector and that RowSelector just changes some internal state without the validation being called, it should be required to set this RowSelector explicitly to immediate. Every other component has its immediate property set to false by default, so setting it to true by default here violates the principle of least surprise. Setting the default to false would make ICEfaces more orthogonal.

          Show
          Thomas Greve added a comment - The introduction of the "immediate" property is exactly what was required here. However, I still consider the original behaviour being a bug. If a JSP contains a commandButton and that button just changes some internal state without the validation being called, this button explicitly has to be set to immediate. This is the very purpose of that parameter, failing to do so is a bug in the JSP.. If a JSP contains a RowSelector and that RowSelector just changes some internal state without the validation being called, it should be required to set this RowSelector explicitly to immediate. Every other component has its immediate property set to false by default, so setting it to true by default here violates the principle of least surprise. Setting the default to false would make ICEfaces more orthogonal.
          Hide
          Mark Collette added a comment -

          I agree that it would be more consistent with the default value being false. But management here takes a hard line against breaking backwards compatibility. My hands are tied.

          Show
          Mark Collette added a comment - I agree that it would be more consistent with the default value being false. But management here takes a hard line against breaking backwards compatibility. My hands are tied.
          Hide
          Thomas Greve added a comment -

          Even when the backwards behaviour is obviously buggy?
          Well, we have our local patch set here, anyway. Just another item to the list.

          Show
          Thomas Greve added a comment - Even when the backwards behaviour is obviously buggy? Well, we have our local patch set here, anyway. Just another item to the list.

            People

            • Assignee:
              Unassigned
              Reporter:
              Grün Ling
            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: