jsr344-experts@javaserverfaces-spec-public.java.net

[jsr344-experts] Re: ComponentSystemEvent API review

From: Leonardo Uribe <lu4242_at_gmail.com>
Date: Tue, 12 Mar 2013 22:01:34 -0500

Hi

2013/3/12 Andy Schwartz <andy.schwartz_at_oracle.com>:
> Gang -
>
> ComponentSystemEvent has this new override:
>
> Index: javax/faces/event/ComponentSystemEvent.java
> ===================================================================
> --- javax/faces/event/ComponentSystemEvent.java (revision 8845)
> +++ javax/faces/event/ComponentSystemEvent.java (revision 11719)
>
> + /**
> + * <p class="changed_added_2_2">Before calling the corresponding method
> + * on the superclass, verify that there is a current component so
> + * that EL expressions that start with #{component} or #{cc} operate
> + * as expected.</p>
> + *
> + * @param listener {_at_link FacesListener} to evaluate
> + * @since 2.2
> + */
> + @Override
> + public void processListener(FacesListener listener) {
> + UIComponent c = getComponent();
> + UIComponent cFromStack;
> + boolean didPush = false;
> + FacesContext context = FacesContext.getCurrentInstance();
> + cFromStack = UIComponent.getCurrentComponent(context);
> + if (null == cFromStack) {
> + didPush = true;
> + c.pushComponentToEL(context, null);
> + }
> + try {
> + if (listener instanceof SystemEventListener) {
> + super.processListener(listener);
> + } else if (listener instanceof ComponentSystemEventListener) {
> +
> ((ComponentSystemEventListener)listener).processEvent(this);
> + }
> + } finally {
> + if (didPush) {
> + c.popComponentFromEL(context);
> + }
> + }
> + }
>
>
> Which was added to address this spec issue:
>
> JAVASERVERFACES_SPEC_PUBLIC-1043 When publishing a ComponentSystemEvent,
> ensure the EL current component is pushed correctly
> http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1043
>
> In my mind, it should be the responsibility of whatever code calls
> publishEvent() to ensure that proper context is set up before publishing the
> event. This should already be happening implicitly for most events, eg. the
> current component should already be set up for any events that are published
> during JSF lifecycle processing. Failure to do this would indicate a bug in
> the calling code that we don't want to mask with some bonus
> ComponentSystemEvent magic.
>

+1. I supposed the documentation of that method was just to specify that the
calling code should ensure that condition.

I have to say that those lines are really bad for performance
perspective, because
call FacesContext.getCurrentInstance() is very expensive in that context.

> If there are some cases where the JSF implementations are failing to take
> care of this (eg. when delivering PreRenderViewEvents?), I would prefer that
> we clean up the implementations to handle this correctly rather than
> complicate ComponentSystemEvent.processListener().
>

I remember this problem. I fixed it some time ago, but I only considered
#{cc} case. The solution implemented was done on f:event tag handler. See

https://issues.apache.org/jira/browse/MYFACES-3289

But the only problem is if the component is inside a datatable, the code
on the listener will only execute once, and not per row, which is
something expected and the affected properties on the component has
the same limitations.

> If we're worried about missing some cases, I suppose we could add a Debug
> project stage test to check/warn about this.
>

I would strongly prefer do not do nothing, because any code in that
part is really
bad for performance.

regards,

Leonardo Uribe

> Andy
>