dev@javaserverfaces.java.net

Zhijun: review for 3129-dynamicValidator

From: Edward Burns <edward.burns_at_oracle.com>
Date: Thu, 11 Sep 2014 12:36:17 -0700

Hello Zhijun,

Thanks for your work on 3241-dynamicValidator.

I attached another changebundle to the issue. Here is the header of the
changebundle:

Please read through this changebundle, make the requested changes, and
try the tests. I didn't try the tests, so my change may require further
refinement.

SECTION: Modified Files
----------------------------
M test/servlet30/stateSaving/pom.xml

- As mentioned by Manfred, when possible, we try to avoid adding a whole
  new test app. Generally speaking, a new test app must be added if
  either of the following conditions are true.

  * None of the existing are a good fit for this new test

  * The app requires per web-app configuration to test. For example, a
    factory in the faces-config.xml, or some specific parameters in the
    web.xml.

  I looked for a good fit in the existing tests here. I didn't find
  one, but when I asked Manfred, he suggested
  test/servlet30/facelets/core.

  Now, let's look at the new files:

test/servlet30/stateSaving/dynamicAddedValidator/src/main/webapp/WEB-INF/beans.xml

  The tests in test/servlet30 must not have any CDI. As the name
  implies, they are "Servlet 3.0" tests. So, do not include the
  beans.xml in your commit.

test/servlet30/stateSaving/dynamicAddedValidator/nbactions.xml
test/servlet30/stateSaving/dynamicAddedValidator/pom.xml
test/servlet30/stateSaving/dynamicAddedValidator/src/main/java/com/sun/faces/test/servlet30/statesaving/dynamicaddedvalidator/DynamicValidatorBean.java
test/servlet30/stateSaving/dynamicAddedValidator/src/main/webapp/WEB-INF/glassfish-web.xml
test/servlet30/stateSaving/dynamicAddedValidator/src/main/webapp/WEB-INF/web.xml
test/servlet30/stateSaving/dynamicAddedValidator/src/main/webapp/index.xhtml
test/servlet30/stateSaving/dynamicAddedValidator/src/test/java/com/sun/faces/test/servlet30/statesaving/dynamicaddedvalidator/Issue3241IT.java

  Otherwise, looks great, just meld them into
  test/servlet30/facelets/core and don't modify the pom.xml

M jsf-api/src/main/java/javax/faces/component/AttachedObjectListHolder.java

- Now, I saw your changebundle posted 2014-09-11 07:45 JIRA time. I
  don't agree with that fix. I have a different proposal.

  In AttachedObjectListHolder.saveState(), if we are in the initial
  state, recognize that we still need to use a StateHolderSaver() to
  wrap the attached object, even if it does implement StateHolder.

  This is accomplished with these changes:

        if (initialState) {
            Object[] attachedObjects = new Object[this.attachedObjects.size()];
            boolean stateWritten = false;
            for (int i = 0, len = attachedObjects.length; i < len; i++) {
                T attachedObject = this.attachedObjects.get(i);
                if (attachedObject instanceof StateHolder) {
                    StateHolder sh = (StateHolder) attachedObject;
                    if (!sh.isTransient()) {
                        attachedObjects[i] = new StateHolderSaver(context, sh);
                        stateWritten = true;
                    }
                } else {
                    StateHolderSaver toSave = new StateHolderSaver(context, attachedObject);
                    attachedObjects[i] = toSave;
                    stateWritten = true;
                }
            }

Ed
-- 
| edward.burns_at_oracle.com | office: +1 407 458 0017
| 11 work days til JavaOne 2014
|  7 work days til start of JSF 2.3 and Servlet 4.0