Hi Zhijun,
As I have stated before please reproduce this first as adding the
required validator in a non-dynamic way. Once you have it reproducing
that way then lets have a look at it.
Thanks!
Manfred
On 9/12/14, 1:35 AM, zhijun Ren wrote:
> Hi Ed,
>
> Just confirmed that your solution can't work, the reason is that the
> restore part(the method restoreState() ) can't handle the state saved
> in saveState() method of AttachedObjectListHolder. and in this case,
> restoreState() is not so easy to modify.
>
> The class AttachedObjectListHolder is not designed well and fragile to
> work, for example, in comments, it says there is a 1:1 relationship
> between restored state and the attachedObjects of
> AttachedObjectListHolder, actually this is not correct cause the
> attachedObjects is not always those restored by the state saved in
> saveState().
>
> I need to discuss with you about the solution if you wish.
>
> I want to defend my previous solution here If the correct behavior is
> that dynamically added validators should be kept and it only need one
> line change:
> In restoreState() method of class AttachedObjectListHolder:
>
> this.attachedObjects.add(restored);
> == change to the following ===>
> add(restored)
>
> Cause the add() method will call clearInitialState() to set the
> initialState to false, this is the legacy logic, when initialState is
> false, the attached RequireValidator is saved and kept for future
> restore.
>
> Can you tell me why the initialState is set to false whenever a
> validator added?
>
>
> BR,
> Zhijun
>
>
>
> On 9/12/14, 8:45, zhijun Ren wrote:
>> Hi Ed,
>>
>> I have tried this solution days before and it did not work.
>>
>> Cause the restore part also need to change accordingly.
>>
>> I will try to work out a feasible one according to your thought.
>>
>> BR,
>> Zhijun
>>
>> On 9/12/14, 3:36, Edward Burns wrote:
>>> 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