QeelwaEtech / omnifaces

Automatically exported from code.google.com/p/omnifaces
0 stars 1 forks source link

ResetInputAjaxActionListener : Add attribute for specific id (or id that wraps several other elements) #147

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
It would be very useful (to me at least) feature to be able to control the id 
of the element or elements wrapper to perform the reset to,

Cause right now it looks like using the <f:actionListener 
type="org.omnifaces.eventlistener.ResetInputAjaxActionListener" /> inside a 
<h:commandButton starts a really extensive visit tree, 

In my scenario this "extensive visit tree" causes a not even rendered elements 
to try to invoke their EL expression in the value attribute which cause 
eventually Null Pointer Exceptions, And I don't want to change my code by 
adding null pointer checks , that's why I made their render condition to be 
false, so... 

Regards,

Daniel.

Original issue reported on code.google.com by vedm...@gmail.com on 27 Feb 2013 at 9:56

GoogleCodeExporter commented 9 years ago
Another thing that will be really nice to have or at least to control is the 
ability not to reset a non rendered elements...

Original comment by vedm...@gmail.com on 27 Feb 2013 at 10:43

GoogleCodeExporter commented 9 years ago
Basically, this is controlled by the value of <f:ajax render> attribute (or in 
case of PrimeFaces, the update attribute). Which value do you have in there?

Original comment by balusc on 27 Feb 2013 at 11:01

GoogleCodeExporter commented 9 years ago
I do have some specific single value in the f:ajax , but it seems that it 
visits much more than that id value , I have place a break point on that piece 
of code and noticed that it doing a lots of tree visit and one of them (the one 
that trigger the problematic EL expression has absolutely nothing with the id 
value that is used in the original f:ajax render tag) , and I'f i remove the 
<f:actionListener 
type="org.omnifaces.eventlistener.ResetInputAjaxActionListener" /> from my code 
all works well and that problematic EL expression is never being invoked...

Original comment by vedm...@gmail.com on 27 Feb 2013 at 11:06

GoogleCodeExporter commented 9 years ago
Does that value happen to represent a value which is mandatory in order to 
crawl the component's children? (e.g. <ui:repeat value> or <h:dataTable value>)

Original comment by balusc on 27 Feb 2013 at 11:09

GoogleCodeExporter commented 9 years ago
yes , its a value of p:dataTable which is wrapped inside a <h:panelGroup 
rendered="false"> , which is a distant child of some other <h:panelGroup which 
render condition is false too....

Original comment by vedm...@gmail.com on 27 Feb 2013 at 11:12

GoogleCodeExporter commented 9 years ago
Okay. Then this is indeed expected behavior and the problem makes sense.

I made a quick fix in the attached snapshot (added VisitHint.SKIP_UNRENDERED to 
VISIT_HINTS constant), let me know if it does the job for you.

In the meanwhile I will think out if it wouldn't possibly affect existing use 
cases.

Original comment by balusc on 27 Feb 2013 at 11:19

Attachments:

GoogleCodeExporter commented 9 years ago
This fix would theoretically affect cases wherein the value of the rendered 
attribute of UIInput component or its parent is manipulated during invoke 
action phase and could theoretically break existing use cases.

Coming back to your NPE problem, is this caused because you're doing e.g. 
return foo.getBar(); instead of return foo; and then using 
value="#{bean.foo.bar}" and let EL do the nullchecks itself? It would otherwise 
be a rather simple fix.

Original comment by balusc on 27 Feb 2013 at 11:30

GoogleCodeExporter commented 9 years ago
Thanks BalusC! this snapshot does fix it, by setting this issue to accept do 
you mean that the feature of adding attribute for specific id (or id that wraps 
several other elements) will be implemented or only that 
VisitHint.SKIP_UNRENDERED will be added to VISIT_HINTS constant ?

Regards,

Daniel.

Original comment by vedm...@gmail.com on 27 Feb 2013 at 11:35

GoogleCodeExporter commented 9 years ago
the NPE is being thrown on the server, I do have a complete control over it, 
but I just didn't want to add NPE checks just to cover the behavior of the 
<f:actionListener 
type="org.omnifaces.eventlistener.ResetInputAjaxActionListener" /> , cause from 
the beginning I was expecting that all those not rendered elements EL 
expressions wont get invoked

Original comment by vedm...@gmail.com on 27 Feb 2013 at 11:49

GoogleCodeExporter commented 9 years ago
Implementing this fix would break existing use cases wherein the rendered 
condition is manipulated during action method. I.e. if a component you'd like 
to reset is unrendered before action, but becomes rendered after action, then 
it would still not be resetted. So I won't commit this exact fix.

The only solution would be to wrap ResetInputAjaxActionListener inside a tag 
file like <o:resetInput> which allows more fine grained control via tag 
attributes. I will look into it when the time allows it.

On the other hand, this NPE is fully expected behavior and in essece a bug in 
your own code.

Original comment by balusc on 27 Feb 2013 at 11:55

GoogleCodeExporter commented 9 years ago
BalusC - I think what Daniel is saying is that, since there is no rendered 
reference to a certain bean, he does not expect it to be instantiated and 
therefore the NPE problem should not occur in this case.

I would agree with Daniel that if a side effect of ResetInputAjaxActionListener 
is that a bean is instantiated when it was not suppose to, that would be an 
incorrect behavior.

On the other hand, and correct me if i'm wrong, I think you are saying that 
Daniel is wrong to expect that a bean would not instantiate just because there 
is no dependency that should incur instantiation and in fact he should always 
anticipate an instantiation of any bean.

Original comment by ben.k...@gmail.com on 27 Feb 2013 at 12:08

GoogleCodeExporter commented 9 years ago
@ben.katz: the ResetInputAjaxActionListener just crawls the entire component 
tree structure in order to find components which needs to be resetted. This 
also covers components which are unrendered at the moment before the action is 
invoked, because they could possibly be switched to rendered during the invoke 
action.

In Daniel's case, he is apparently performing business logic in the getter of a 
p:dataTable value which is throwing a NPE during the component tree visit.

Original comment by balusc on 27 Feb 2013 at 12:16

GoogleCodeExporter commented 9 years ago
It is a logic inside a getter (not my getter but a layer below me :| ) but I 
don't expect all my xhtml pages and their EL expression to be invoked when I 
use the ResetInputAjaxActionListener , That's why I think an attribute to 
control the id of the element or id of a wrapper element would be very useful...

Regards,

Daniel.

Original comment by vedm...@gmail.com on 27 Feb 2013 at 12:35

GoogleCodeExporter commented 9 years ago
I've committed the VisitHint.SKIP_UNRENDERED fix anyway: 
http://code.google.com/p/omnifaces/source/detail?r=8da69c4a825e0aca520c1671b51b6
4e353ab68ef

PrimeFaces <p:resetInput> also does that under the covers and no one seems to 
have complained about that so far.

If it would still end up in problems because the rendered condition is changed 
during invoke action, an extra check in the rendered attribute of the target 
component should easily solve that.

Original comment by balusc on 28 Feb 2013 at 6:19

GoogleCodeExporter commented 9 years ago
Thanks BalusC

Any ideas on when the next stable version of the Omnifaces is going to be 
released ?

Also , my original feature request was for the ability to control the elements 
that are going to be reset (in order to reduce the "visit tree" for irrelevant 
elements) Is it possible to implement in the future ? Cause right now its not 
traversing over the id/s of the f:ajax render attribute value

Thanks again.

Original comment by vedm...@gmail.com on 28 Feb 2013 at 6:49

GoogleCodeExporter commented 9 years ago
This will be considered once one complains about this fix and don't want to 
change all rendered attributes :)

OmniFaces 1.4 is planned this weekend. Or if we don't have time this weekend, 
at least before next weekend.

Original comment by balusc on 28 Feb 2013 at 6:58