myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: [jsr-314-open] PostAddToViewEvent publishing conditions
Date Wed, 19 May 2010 04:51:41 GMT
Hi

2010/5/18 Andy Schwartz <andy.schwartz@oracle.com>

> 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.
>
>
I agree with you. I was thinking as a temporal fix for 2.0 to create a
"copy" of the component created by <h:outputScript>/<h:outputStylesheet> and
add them as resource as a result of a PostAddToViewEvent, but your
suggestion looks better. The important here is do not relocate components.


> 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?
>
>
Good idea! I think it is possible, but I don't know the details behind
TemplateClient. I'll try it.


>
>  - 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.
>
>
Totally agree. I'll fix this stuff on myfaces, because after doing some work
with tomahawk (t:autoScroll), I conclude it is more important publish
PostAddToViewEvent correctly (I want to create a view listener from
PostAddToViewEvent that attach a client behavior based on some conditions).


>
>  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.
>
>
Leonardo


> Andy
>
>
>  Suggestions are welcome.
>>
>> regards,
>>
>> Leonardo Uribe
>>
>>
>

Mime
View raw message