[jsr-314-open-mirror] [jsr-314-open] Repetitive calls to FacesContext.getCurrentInstance() from listeners of system events
by Leonardo Uribe
Hi
Reviewing some stuff, it was notice that the FacesContext instance is not
passed when event processing occur, so every time a system event should be
processed, a call to FacesContext.getCurrentInstance() should be done in
almost all cases.
Below there are one example based on myfaces code (removed non relevant
code):
public class HtmlStylesheetRenderer extends Renderer implements
ComponentSystemEventListener
{
public void processEvent(ComponentSystemEvent event)
{
UIComponent component = event.getComponent();
FacesContext facesContext = FacesContext.getCurrentInstance();
facesContext.getViewRoot().addComponentResource(facesContext,
component, "head");
}
......
}
It could be good to pass the current facesContext (note the code in
Application.publishEvent receive it as param), to prevent those unnecessary
calls and enhance code performance. In theory it is possible to cache
facesContext object on listeners suscribed using
UIViewRoot.subscribeToViewEvent() because those listeners are not saved (but
maybe not because if the same view is used on portlet....), but that's not
possible on ComponentSystemEventListener instances.
I'll open an issue for this one.
regards,
Leonardo Uribe
13 years, 12 months
Re: [jsr-314-open-mirror] [jsr-314-open] PostAddToViewEvent publishing conditions
by Andy Schwartz
Leonardo -
Thanks for this detailed analysis - lots of good information here.
I've been meaning to follow up on this for some time now. Better late
than never I guess. :-)
My thoughts inline below...
On 3/17/10 9:10 PM, Leonardo Uribe wrote:
> Actually the event publishing conditions of
> PostAddToViewEvent/PreRemoveFromViewEvent are not very clear. These
> conditions depends if partial state saving is used or not and if
> facelets updates the view or not. The problem is this fact is not
> very intuitive, so for write code correctly the user needs to be aware
> of that, in other words this should be documented somewhere. Also,
> there are some technical questions that could be of interest on this
> mailing list and myfaces dev list too.
>
> In theory, PostAddToViewEvent is used in this cases:
>
> - h:outputScript and h:outputStylesheet renderers uses it to relocate
> component resources to UIViewRoot facet "head" or "body" target
> (h:outputStylesheet by default to "head").
I have some concerns about the resource relocation implementation. As
currently implemented (in Mojarra at least - haven't checked MyFaces
yet), the <h:outputScript>/<h:outputStylesheet> components are literally
removed from their original location in the component tree and are
re-parented to the UIViewRoot. This has some non-obvious consequences,
including:
1. EL no longer evaluates in its original context.
2. When re-executing the tags during render response on postbacks, the
components are no longer found in their original location and are
re-created (on every postback).
#1 means that if an <h:outputScript>/<h:outputStylesheet> instance uses
an EL expression that references context that is set up by an ancestor
component, these EL expressions will fail to resolve correct after the
component is relocated to a new parent. Mojarra contains code that
attempts to work around this problem for one particular case: composite
component context (ie. "#{cc}"). However, composite components are not
the only components that may set up EL-reachable state.
#2 means that we are doing unnecessary work - and potentially losing
state since we re-create the component instance from scratch on each
request.
I *think* that the correct way to handle this is to leave the
<h:outputScript>/<h:outputStylesheet> components in their original
locations and to visit these before the render occurs to give these
components a chance to contribute resources while in their proper
context. This would allow ancestors to establish context that may be
necessary for proper EL evaluation. You have hinted at something like
this in the related "add component resources depending on the owner
component state" thread (which I am also hoping to review) - ie. perhaps
we need a new event/hook after tag execution but before render that we
can use to collect the rendered resources.
Note that if we are thinking about adding such an event, we need to
consider doing this in a way that allows components to opt into event
delivery. That is, instead of performing yet another full tree
traversal, ideally we should perform a partial tree visit where we only
visit components that have expressed an interest in performing
pre-render processing. (Or, if no such components are present, we can
skip the visit altogether.) This is going to be an important
optimization for the use cases that I care the most about: views with
very large/deep component trees.
Oh, and, one nice side effect to moving resource collection out of
PostAddToViewEvent and into a special pre-render processing pass is that
we could take advantage of VisitHint.SKIP_UNRENDERED to ensure that we
only visit/collect resources for components that are, in fact,
rendered. Our current approach doesn't allow for this - ie.
PostAddToViewEvents are delivered to both rendered and non-rendered
components, and as a result all resources are added regardless of
whether they are in rendered subtrees.
> Note "head" and "body" facets are transient.
> - cc:insertChildren and cc:insertFacet, to relocate components to some
> place inside cc:implementation. The reason why we do that through a
> listener attached to PostAddToViewEvent is we need to take into
> account the case of nested composite components using
> cc:insertFacet/cc:insertChildren. Note when the component tree is
> built the first time, PostAddToViewEvent is propagated from root to nodes.
I don't fully understand why the PostAddToViewEvents approach is
necessary for implementing composite component facet/children
insertion. The current solution - relocating composite component
facets/children to a new parent inside of the composite component
implementation suffers from problem #2 above. And in this case, the
loss of state that occurs when re-creating these components is
significant. Stateful components - such as Trinidad's <tr:showDetail>:
http://myfaces.apache.org/trinidad/trinidad-api/tagdoc/tr_showDetail.html
Cannot be used as a facet/child of a JSF 2 composite component, since
any state that they maintain is lost after tags are re-executed on
postback (during render response).
Fortunately, we have a precedent for how to solve this problem - ie.
Facelets already handles this nicely for the
<ui:composition>/<ui:define>/<ui:insert> case, which is very similar to
composite component facet/children insertion. Facelets uses the
TemplateClient API to ensure that components are inserted into the
proper target location *during* tag execution. When tags are
re-executed against an existing component tree on postbacks, the
components are still in the same location (in the target insertion
location) and thus are not re-created. There is no loss of state.
We are using a similar approach here to implement ADF Faces declarative
components (our own pre-JSF2 composite component solution) and don't
suffer from any state loss problems. (Note that we would use the
TemplateClient API directly, but this was hidden in JSF2 so we ended up
hacking around this to achieve the same behavior.)
Is there a reason why the Facelets TemplateClient approach was
not/cannot be used for composite components as well?
> - When Partial State Saving is enabled, it is necessary to attach a
> listener to PostAddToViewEvent/PreRemoveFromViewEvent, so if some user
> add/remove a component, we can keep track of those changes and
> save/restore the component tree properly, in other words, support
> component addition or removal programatically.
This seems like a good use of PostAddToViewEvents. (Not sure that we
really should be leveraging PostAddToViewEvents for the other two use
cases.)
>
> Now, the instants of time where PostAddToViewEvent is published are:
>
> - With Partial State Saving enabled
>
> * When the view is build at first time.
> * In a postback when the view is build but before the state of the
> component is restored.
>
Right - this is during restore view when we execute tags to build up the
view.
Note that once Mojarra issue 1313 is fixed:
https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1313
We will also re-execute tags during render response (as we already do
for full state saving), which means that we'll see additional
PostAddToViewEvents.
> - With Partial State Saving disabled
>
> * When the view is build at first time.
> * In a postback when the view is "refreshed",
(This is the 1313 case - ie. we'll see this with partial state saving
enabled too once 1313 is fixed.)
> because all component nodes are detached and attached to the tree. In
> other words, on render response phase, vdl.buildView is called and in
> this case facelets algorithm add all transient components (usually all
> html markup not saved), and to do that, it detach and attach all
> components to be in right order.
Hrm. Something is very off here. I sort of get the idea that Facelets
needs to detach/re-attach components to restore the original order.
(Sort of. Does JSP do this too?). However, the fact that Facelets
happens to temporarily remove and then immediately re-add components for
the purpose of preserving the original order does not, in my mind, mean
that we should be delivering PostAddToViewEvents for this scenario.
The component was added to the component tree during restore view... it
is still in the component tree, in the same parent, possibly in the same
exact location under that parent after we re-execute the tags during
render response. This does not strike me as a "component added"
situation. The fact that Facelets happens to do this should be
transparent to component authors - ie. we should not be delivering
PostAddToViewEvents for this case.
> This also has some other implications, like c:if tag depends on this
> to work correctly.
>
> A developer that is not aware of this fact could think that
> PostAddToViewEvent is called when the view is build.
I can see how the current implementation can be very confusing. I agree
that we need to simplify this. I believe that at least part of the
problem is that we are delivering PostAddToViewEvents for cases that
shouldn't be treated as an "add". This seems like an implementation bug
to me, though perhaps some spec clarification is also necessary.
>
>
> This is true with PSS is enabled, but is not true when PSS is
> disabled. On this topic:
>
> [jsr-314-open] add component resources depending on the owner
> component state
>
> It was described why PostAddToViewEvent cannot be used in that case.
I suspect that a post tag execution/pre-render (partial) visit might
work better for this case as well.
> But let's change a litte bit the problem. We have a custom component
> that creates some other on PostAddToViewEvent and add it as a
> children. It should work but in fact it only works when PSS is
> enabled, with PSS disabled that code will cause a duplicate id exception.
Is that because we are re-delivering PostAddToViewEvents during refresh
for components which were already present in the component tree? If so,
we should stop doing that. :-)
> Of course, the developer can solve it with some effort but the point
> is we are not consider the element of least surprise.
Agreed. The fact that the developer needs to get creative here seems
like a sign that something is off.
>
> But this fact this is only the tip of the iceberg. In mojarra (this
> was already fixed on myfaces), components relocated by
> cc:insertChildren or cc:insertFacet does not have state when PSS is
> disabled, because when the view is "refreshed" the components are
> created again, but facelets algorithm remove all components with no
> bound to a facelet tag handler, and then the listeners attached to
> PostAddToViewEvent relocates the new ones, so this effect is not
> notice at first view.
Exactly! That's my #2 above. This definitely needs to be fixed. As I
mentioned above, I think the right way to solve this problem is to use
the Facelets TemplateClient approach, though I am interested to hear
whether there are reasons why we did not/could not use this solution.
>
> Do you remember PostAddToViewEvent propagation ordering is important
> by cc:insertChildren/cc:insertFacet? well, with PSS disabled, the
> ordering now is from nodes to root, because all nodes are detached and
> attached from nodes to root,
I don't think that we should be delivering PostAddToViewEvents for these
temporary detach/re-attach cases.
> so in myfaces we did something like this (I removed some non relevant
> code to be more clear):
>
> try
> {
> if (refreshTransientBuild)
> {
> context.setProcessingEvents(false);
> }
> // populate UIViewRoot
> _getFacelet(renderedViewId).apply(context, view);
> }
> finally
> {
> if (refreshTransientBuild)
> {
> context.setProcessingEvents(true);
>
> // When the facelet is applied, all components are
> removed and added from view,
> // but the difference resides in the ordering.
> Since the view is
> // being refreshed, if we don't do this manually,
> some tags like
> // cc:insertChildren or cc:insertFacet will not
> work correctly, because
> // we expect PostAddToViewEvent will be propagated
> from parent to child, and
> // facelets refreshing algorithm do the opposite.
>
> FaceletViewDeclarationLanguage._publishPreRemoveFromViewEvent(context,
> view);
>
> FaceletViewDeclarationLanguage._publishPostAddToViewEvent(context, view);
> }
> }
>
> In few words, disable event processing, and fire PostAddToViewEvent
> and PreRemoveFromViewEvent in the expected order to build the tree
> correctly. If we don't do this, h:outputScript and h:outputStylesheet
> will not work correctly (remember that UIViewRoot "head" and "body"
> facets are transient, so if these components are relocated, are in
> fact transient too).
I think that we should use a post tag execution/pre-render partial visit
to collect these resources.
> Also, care must be taken to prevent the listener of programatic
> component addition/removal register components on this time.
>
> The conclusion based on this reasoning, it is clear the need of new
> event that be specific for relocate components (keep it simple for
> christ sake!). This event should be propagated for all components in
> the tree after the view is build from root to nodes.
I am very nervous about adding new events that are delivered to all
components since this adds undesirable overhead for views with large #s
of components. If we do need to add new events, I would prefer that we
go with partial visits rather than full visits if at all possible.
> It could be good to have some clearification about the real intention
> of PostAddToViewEvent/PreRemoveFromViewEvent.
Yep, we definitely need some clarification here. Given the
inconsistencies/unexpected behaviors in the current implementation, I
can see how our users will find these events confusing.
> Really, better names for those events are
> PostAddToComponentTreeEvent/PreRemoveFromComponentTreeEvent.
>
I don't see the distinction that you are making here... ie.
"PostAddToView" and "PostAddToComponentTree" seem very similar to me.
Andy
> Suggestions are welcome.
>
> regards,
>
> Leonardo Uribe
>
14 years
[jsr-314-open-mirror] [jsr-314-open] Visit Tree api cannot traverse "real" component tree
by Leonardo Uribe
Hi
In myfaces implementation, I have found multiple locations where it is
necessary to traverse the "real" component tree. That means, it is necessary
to visit all components ignoring UIData "rows", or components that its
children renderer multiple components (for example t:dataList or ui:repeat).
It could be good to have a "standard" way to do that. I have the following
use cases where I can't use Visit Tree:
1. Publish PostAddToViewEvent
2. Add/remove components to the tree when partial state saving is used.
3. Add attached objects (like a custom client behavior that it is necessary
to attach on components with some conditions (t:autoScroll) ).
That problem has a side effect: PostRestoreStateEvent should be published
only for "real" component instances (the algorithm is restoring only one),
but in fact if you have a datatable it is published for all rows, that means
the same even is published for the same component over and over and over and
over and over.
It could be good to have a hint that could be called
VisitHint.VISIT_REAL_COMPONENTS or something like that that visit only real
components.
In othe side, looking the documentation there is a hint called
VisitHint.EXECUTE_LIFECYCLE that says the following:
"Hint that indicates that the visit is being performed as part of lifecycle
phase execution and as such phase-specific actions (initialization) may be
taken."
I understand why VisitHint.SKIP_TRANSIENT and VisitHint.SKIP_UNRENDERED, but
looking the javadoc I couldn't find when, how or why that hint should be
set. At this time, myfaces just ignore it, but if it is used, there should
be some documentation on UIComponent.visitTree or UIData.visitTree. Maybe
I'm wrong about that, and if myfaces don't use it really there is not
problem, but the point is: Is mojarra really implementing the algorithm that
says the javadoc?. It could be good if someone could check this algorithm
against javadoc, because really it is very suspicious.
Should I open an issue for these ones (one for each problem)?
regards,
Leonardo Uribe
14 years
[jsr-314-open-mirror] [jsr-314-open] [ADMIN] JSF Spec Short Term Plans
by Edward Burns
SECTION: Introduction
=====================
I realize my last message to this group was on 29 March 2010 ("Use a
Renderer on a composite component"). The reason for this prolonged
silence is that I was focused on Mojarra implementation work. As most
of you may know, our star Mojarra implementation lead, Ryan Lubke, has
moved further down the stack and now works full time on Grizzly. At the
same time, my colleague Roger Kitain has been helping to integrate the
JSR-299 RI from JBoss into GlassFish. This has left me no time to
participate in this list. I expect my participation to increase to a
normal level now that Roger is completing his JSR-299 obligations and we
now have a new implementation team member: Sheetal Vartak. I'll
formally introduce Sheetal in another message so we can welcome her
properly. Both of these developments, and the other status information
in this email will lead to more activity from Roger and I on this list.
SECTION: Short Term Development Plans
=====================================
When we last took up the topic of development plans, at the 20100224 EG
Meeting in Vienna, Austria, the only officially announced effort was JSF
2.0 Rev a. In order to accomodate Sun's traditional constraints on the
scope of work done in a "Rev a" type release, we will now be undertaking
two simultaneous development efforts.
"2.0 Rev a" will include work that does not require any change in
behavior of the implementation (including any signature changes) or
TCK. This work will be limited to spec edits. This work will be
conducted under the JCP Minor Revision process [3]. Because there are
no implementation changes, no new release of the Reference
Implementation (RI) or Test and Compatibility Kit (TCK) will be
produced to accompany 2.0 Rev a. The existing JSF 2.0 RI and TCK will
be sufficient for this purpose.
"2.1" will include work that does not fit into 2.0 Rev a, yet is small
enough in scope (at the community informed discretion of Roger and
myself) to not require a full JSR. This work will be conducted under
the JCP Minor Revision process [3]. Because this work does include
implementation changes, a new RI and TCK will be produced to accompany
the spec.
These two efforts will run in parallel, but I expect this list to focus
more on 2.1 since the issues most of you care about do require changes
in the behavior of the implementation.
SECTION: 2.0 Rev a: Maintenance Lead: Roger Kitain
==================================================
The JCP ChangeLog is at [4] and the issue tracker target milestone is
"2.0 Rev a". We aim to start the 30 review clock on 2.0 Rev a on Friday
14 May 2010. We aim to have a new draft of the spec incorporating the
edits around 21 May 2010.
SECTION: 2.1: Maintenance Lead: Ed Burns
========================================
The JCP ChangeLog is at [5] and the issue tracker target milestone is
"2.1". We aim to start the 30 review clock on 2.1 in mid September
2010. Somewhere in the next two weeks I want to have an EG meeting
where we can review the issues currently in discussion on this list, the
issues in the 2.1 ChangeLog, and the issues in the issue tracker so we
can control the scope of work for 2.1. In the meantime, I'm going to
catch up on JSR-314-OPEN.
SECTION: Regarding the JCP status of the JSR-314 EG and JSR-314-OPEN(a)jcp.org
============================================================================
Now, regarding this list, I must point out the following text from
section 3.5 FINAL RELEASE of the JCP Process Document [1]:
Upon Final Release, the Expert Group will have completed its work and
disbands. The Spec Lead will typically be the Maintenance Lead and may
call upon Expert Group members and others for aid in that role.
The JSR-314 Expert Group is indeed disbanded and Roger and I will share
the Maintenance Lead responsibilities.
For those of you who formally joined the JSR-314 Expert Group under the
Individual Expert Participation Agreement (IEPA), note this text from
section FUNDAMENTAL DEFINITIONS [2] of the JCP Process Document.
There is no fee associated with the IEPA and it is valid until the
Expert Group disbands.
For those of you who formally joined the JSR-314 Expert Group under the
Java Specification Participation Agreement (JSPA), note this text from
the same section.
[The JSPA is] A one-year renewable agreement between Sun Microsystems
and a company, organization or individual that allows the latter
entities to participate in the Java Community Process.
This list, JSR-314-OPEN(a)JCP.ORG, is the official email list of record
for JSR-314 development. Per the above stated section 3.5 FINAL
RELEASE, the JSR-314 Expert Group is officially disbanded, however, I
plan to continue to use this email list as the email list of record for
Maintenance Review work on JSF. Note that any intellectual property and
other rights you had under your IEPA have expired. If you are here
under a JSPA, note that those rights are contingent on the JSPA being
current and not expired.
Thanks for sticking it out through this silent time. I assure you
Roger, Barbara, and I have made every effort to get to this point as
quickly as possible.
Sincerely,
Ed Burns JSF 2.1 Maintenance Lead
[1] http://jcp.org/en/procedures/jcp2#3.4
[2] http://jcp.org/en/procedures/jcp2#F
[3] http://jcp.org/en/procedures/jcp2#4.2.1
[4] http://wiki.jcp.org/wiki/index.php?page=JSF+2.0+Rev+A+Change+Log
[5] http://wiki.jcp.org/wiki/index.php?page=JSF+2.1+Change+Log
14 years
Re: [jsr-314-open-mirror] [jsr-314-open] UIViewRoot.getComponentResources() documentation does not match with the implementation
by Leonardo Uribe
Hi
Doing some tests I notice the implementation in mojarra of
UIViewRoot.getComponentResources() does not match with the spec javadoc.
This javadoc says this:
*"...Return an unmodifiable List of UIComponents for the provided target
agrument. Each component in the List is assumed to represent a resource
instance.
The default implementation must use an algorithm equivalent to the the
following.
* Locate the facet for the component by calling getFacet() using target
as the argument.
* If the facet is not found, create the facet by calling
context.getApplication().createComponent() using javax.faces.Panel as the
argument
o Set the id of the facet to be target
o Add the facet to the facets Map using target as the key
* return the children of the facet...."*
The javadoc says cleary that the returning facet is created using
javax.faces.Panel as argument. But mojarra uses
"javax.faces.ComponentResourceContainer" as argument. The effect can be seen
when you try to use myfaces on glassfish v3 and you set the classloader
delegate to true (I know that configuration is wrong, but the exception
caught my attention).
Looking on google I saw this issue:
https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1427
I also saw that the line:
"*Set the id of the facet to be target"
*is to respected too. Tomahawk has an example
(myfaces-example-simple20/calendar.jsf) that has this code
<h:panelGroup id="body">
It works with mojarra (2.0.3-SNAPSHOT) but do not with myfaces. The reason
is myfaces is doing what spec says, but mojarra set the id to
"javax_faces_location_body". That's not fair. On JSF 2.0 Rev A Change Log
there is no issue related to this one.
I have to say it, my personal opinion is that spec changes should not be
done without document them on javadoc or add them to JSF 2.0 Rev A Change
Log. Do things like that, only makes things more difficult to track, even
worst, important details that needs to be on the spec just will be missing.
Could anyone include this detail on:
http://wiki.jcp.org/wiki/index.php?page=JSF+2.0+Rev+A+Change+Log
and correct the spec javadoc?
regards,
Leonardo Uribe
14 years
Re: [jsr-314-open-mirror] [jsr-314-open] UIComponent.broadcast only propagate events on ClientBehavior instances
by Leonardo Uribe
Hi
Checking the new Behavior api (for implement cc:clientBehavior), I note that
the javadoc of UIComponent.broadcast says this:
"....Broadcast the specified FacesEvent to all registered event listeners
who have expressed an interest in events of this type. Listeners are called
in the order in which they were added.
If the event is an instance of BehaviorEvent and the current component is
the source of the event call BehaviorEvent.getBehavior() to get the Behavior
for the event. If the behavior implements ClientBehavior, call
Behavior.broadcast(javax.faces.event.BehaviorEvent)}....."
The wrong line is:
".....If the behavior implements ClientBehavior, call
Behavior.broadcast....."
So, if a user try to create a custom Behavior, the method broadcast() will
be useless, and custom behaviors will not catch events. I think it is a bug
on the javadoc, so I'll correct it on myfaces.
regards,
Leonardo Uribe
14 years
Re: [jsr-314-open-mirror] [jsr-314-open] Use a Renderer on a composite component
by Ed Burns
>>>>> On Sun, 28 Mar 2010 21:21:10 -0500, Leonardo Uribe <lu4242(a)gmail.com> said:
LU> The problem is why it is mandatory to set "javax.faces.Composite" as
LU> renderer type. The javadoc should say:
LU> "...If the renderer type is not set (return null), Call
LU> UIComponent.setRendererType(java.lang.String)<file:///D:/jdk-1_5_0-doc/jsf20/mojarra-2.0.3-SNAPSHOT/docs/javadocs/javax/faces/component/UIComponent.html#setRendererType%28java.lang.String%29>on
LU> the
LU> UIComponent instance, passing "javax.faces.Composite" as the argument...".
LU> In that case, a user can override the rendererType on the constructor and
LU> avoid this hack that works with the current spec:
I understand the problem you have uncovered. Take a look at the
renderkit docs for javax.faces.NamingContainer/javax.faces.Composite.
There are specific requirements in there that depend on the composite
component metadata specification.
I thought it would be toooooooo subtle to allow the Renderer to be
customized beause this contract must also be followed in the custom
renderer case.
LU> Why override the default Renderer and use a custom one? Let's
LU> suppose the component proposed needs some custom code for converter,
LU> or for decode. The right place to put that kind of code is the
LU> Renderer class, not the component, but note it is possible to put
LU> that on the component class.
Yes I understand that the Renderer is the right place for such things
but the chosen programming model for customization of composite
components is to override the top level component. Simple. If we're
going to change that I need a more compelling reason than correctness.
Ed
--
| edward.burns(a)oracle.com | office: 408 884 9519 OR x31640
| homepage: | http://ridingthecrest.com/
14 years