You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@myfaces.apache.org by Leonardo Uribe <lu...@gmail.com> on 2010/05/19 06:51:41 UTC

Re: [jsr-314-open] PostAddToViewEvent publishing conditions

Hi

2010/5/18 Andy Schwartz <an...@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
>>
>>
>

Re: [jsr-314-open] PostAddToViewEvent publishing conditions

Posted by Andy Schwartz <an...@oracle.com>.

Hey Leonardo -

Thanks for giving this a go and for sharing your findings!  I am still 
hopeful that we can make something along these lines work.  I will try 
to take a look at your patch to see if I have any ideas.  (Though have 
other things I need to take care of first - so may be a few days before 
I get to this.)

Andy

On 5/19/10 11:15 PM, Leonardo Uribe wrote:
> Hi
>
> I tried the proposal of use some variation of TemplateClient API (I 
> attach it as reference on MYFACES-2638-6.patch ). The solution works 
> for simple cases, but it does not when nested composite components are 
> used.
>
> Look this example (I removed the non relevant code):
>
> testCompositeInsertChildren.xhtml (the page when it is used)
>
> <testComposite:compositeInsertChildren>
>     <h:outputText value="GAMMA " />
> </testComposite:compositeInsertChildren>
>
> testComposite:compositeInsertChildren
>
> <composite:implementation>
>     <testComposite:compositeInsertChildrenInner>
>         <h:outputText value="BETA " />
>         <composite:insertChildren />
>     </testComposite:compositeInsertChildrenInner>
> </composite:implementation>
>
> testComposite:compositeInsertChildrenInner
>
> <composite:implementation>
>     <h:outputText value="ALFA " />
>     <composite:insertChildren/>
>     <h:outputText value="OMEGA " />
> </composite:implementation>
>
> The example should render this:
>
> ALFA BETA GAMMA OMEGA
>
> But it is rendered this:
>
> ALFA GAMMA OMEGA
>
> The current algorithm do it correctly because the tree is completely 
> built before relocate, so the listener relocates BETA too. Facelets 
> executed the inner FaceletHandler instance first and then the outer 
> ones. On composite components, it executes first the composite 
> component facelet, then the FaceletHandler children.
>
> Suggestions are welcome.
>
> regards,
>
> Leonardo Uribe

Re: [jsr-314-open] Fwd: PostAddToViewEvent publishing conditions

Posted by Andy Schwartz <an...@gmail.com>.

Gang -

I attempted to send this earlier today from my work account but it
didn't seem to make it through.  Re-sending from gmail (with one minor
correction).  Hopefully I'll have better luck this time. :-)

(Apologies if we end up with multiple copies of this...)

On 5/26/10 1:27 AM, Martin Marinschek wrote:
> Hi Andy,
>
> my 2cents (isn´t worth much in this discussion if I don´t look any
> deeper into this ;)
>
>
>> Given that ui:decorate is a non-trimming ui:composition, why would these
>> behave differently with respect to interaction with the TemplateClient?  I
>> would think that both of these handlers should be calling pushClient().
>>
>
> I don´t see why ui:decorate should be different from ui:composition, either.
>

Unless someone understands why the implementation is inconsistent
across these two cases, I think that we should align the
implementations.  Note that given the bug that Max logged (against
Mojarra):

https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1684

I believe that DecorateHandler has got this one right (ie. both
DecorateHandler and CompositionHandler should be calling
pushClient()).

>> Also, the multi-level search strategy seems somewhat questionable to me.  A
>> template exposes a certain number of of names into which content could be
>> inserted.  The consumer of the template leverages that contract and
>> specifies content to be inserted into some subset of those names.  When
>> attempting to locate content to insert, why should the template look any
>> further up the stack than the nearest consumer (ie. the most recently pushed
>> TemplateClient)?
>>
>
>
> Well, templates are certainly hierarchical - does the nearest consumer
> carry all information, or just the one which is defined in itself?
>

My take on this is that templates are reusable objects with a specific
contract (much like composite components, or, well, any component for
that matter).  The contract is that the template exposes certain slots
that users of the template can fill in with their own content.  The
names of the slots are defined via ui:insert tags in the template
implementation.

Yeah, I know, you already know this. :-)

If the template itself happens to depend on yet another template, I
view that as an implementation detail of the template.  So, if we've
got this situation:

- view.xhtml, which includes:
 - outerTemplate.xhtml, which includes:
   - innerTemplate.xhtml

In my ideal world (which, at the moment, does not match reality),
view.xhtml should only be aware of the slots that are exposed by
outerTemplate.xhtml.  That is, view.xhtml should not depend on the
fact that outerTemplate.xhtml happens to include innerTemplate.xhtml,
which may have its own slots.

The reality is that when nesting templates, the outermost template
exposes *all* of the slots defined by any nested templates (no matter
how many levels deep) to the consumer.  This seems like a violation of
encapsulation to me.   We are exposing implementation details up
through the template hierarchy that, I think, should not necessarily
be exposed.

Of course, I understand that it may be useful to be able to pass
content from view.xhtml all the way through to a slot defined by
innerTemplate.xhtml.   If outerTemplate.xhtml wants to allow certain
content to be passed through from view.xhtml to innerTemplate.xthml,
that's fine.  However, this should be done by introducing a ui:insert
inside of outerTemplate.xhtml - ie. by explicitly exposing a new slot
that consumers of outerTemplate.xhtml can rely on.  That way, if the
author of outerTemplate.xhtml reconsiders the implementation and
decides not to delegate to innerTemplate.xhtml after all, users of
outerTemplate.xhtml are not hosed.

Oh well, this looks like another cases where I am many years too late.
 That seems to be my fate these days.

Even if we cannot change this behavior for ui:insert now, I do think
it is important that we avoid this behavior for composite components.
Since we are tinkering with the
composite:inertFacet/composite:insertChildren implementations now to
be more Facelets/TemplateClient-like, let's just be careful to avoid
introducing the Facelets nested insert behavior into composite
components.

>> Right, so I see that InsertHandler is implemented this way, but... why?
>>  Seems silly to have ui:insert serve two entirely different purposes - ie.
>> to both identify an insertion point as well as to define content for
>> included templates.   Why should ui:insert act both as a ui:insert *and* as
>> a ui:define?   If a template author wants to pass content through from an
>> outer page to an inner/nested template, seems like the right way to
>> accomplish this would be to wrap the ui:insert inside of a ui:define.
>>
>
> Well, if you define a template, you use ui:insert to mark the spots
> where content can be - and then you include default content right
> away. Is it this which makes ui:insert a thing of both worlds?
>

Let's take a step back and look at the role of the TemplateClient
contract.  TemplateClient allows consumers of templates (eg.
ui:composition) to provide access to content that should be inserted
into the template - ie. content defined by ui:define, inserted into
ui:insert.  So, for example, both CompositionHandler and
DecorateHandler are TemplateClients.

ui:insert is clearly a user of the TemplateClient interface.
InsertHandler (via a call to FaceletContext.includeDefinition()) pulls
in content provided by outer TemplateClients (eg. an ancestor
ui:composition).

This is all well and good.

Where things get strange is that InsertHandler also implements
TemplateClient.  This means that, in addition to being a consumer of
TemplateClient content, ui:insert also exposes its own content for
consumption by other consumers of TemplateClients.

So, if you've got:

 <ui:insert name="foo">
   body content
 </ui:insert>

The "body content" serves two roles:

1.  It provides default content in the event that no matching
ui:define for "foo" can be found.
2.  It is exposed as content for consumption while performing the
definition inclusion (during the call to
FaceletContext.includeDefinition()).

Case #1 is expected/documented.

Case #2 is where ui:insert seems to acting as a ui:define.  This is
non-obvious, and, as far as I can tell, not documented.

To me this looks like an old implementation quirk that we have
inherited.  Does someone here happen to understand why this is useful?
 If this is useful, we need to document this behavior.  If it isn't
useful, we should remove this part of the implementation.

Andy

> best regards,
>
> Martin
>
>

Re: [jsr-314-open] Fwd: PostAddToViewEvent publishing conditions

Posted by Andy Schwartz <an...@oracle.com>.

On 5/26/10 12:49 PM, Andy Schwartz wrote: 
>
> Unless someone understands why the implementation is inconsistent 
> across these two cases, I think that we should align the 
> implementations.  Note that given the bug that Max logged (against 
> Mojarra):
>
> https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1684
>
> I believe that CompositionHandler has got this one right (ie. both 
> DecorateHandler and CompositionHandler should be calling pushClient()).

That should have read:  "I believe that DecorateHandler has got this one 
right".

I need to learn to proofread my emails before I send them instead of 
after. :-)

Andy

Re: [jsr-314-open] Fwd: PostAddToViewEvent publishing conditions

Posted by Andy Schwartz <an...@oracle.com>.

On 5/26/10 1:27 AM, Martin Marinschek wrote:
> Hi Andy,
>
> my 2cents (isn´t worth much in this discussion if I don´t look any
> deeper into this ;)
>
>   
>> Given that ui:decorate is a non-trimming ui:composition, why would these
>> behave differently with respect to interaction with the TemplateClient?  I
>> would think that both of these handlers should be calling pushClient().
>>     
>
> I don´t see why ui:decorate should be different from ui:composition, either.
>   

Unless someone understands why the implementation is inconsistent across 
these two cases, I think that we should align the implementations.  Note 
that given the bug that Max logged (against Mojarra):

https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1684

I believe that CompositionHandler has got this one right (ie. both 
DecorateHandler and CompositionHandler should be calling pushClient()).

>> Also, the multi-level search strategy seems somewhat questionable to me.  A
>> template exposes a certain number of of names into which content could be
>> inserted.  The consumer of the template leverages that contract and
>> specifies content to be inserted into some subset of those names.  When
>> attempting to locate content to insert, why should the template look any
>> further up the stack than the nearest consumer (ie. the most recently pushed
>> TemplateClient)?
>>     
>
>
> Well, templates are certainly hierarchical - does the nearest consumer
> carry all information, or just the one which is defined in itself?
>   

My take on this is that templates are reusable objects with a specific 
contract (much like composite components, or, well, any component for 
that matter).  The contract is that the template exposes certain slots 
that users of the template can fill in with their own content.  The 
names of the slots are defined via ui:insert tags in the template 
implementation.

Yeah, I know, you already know this. :-)

If the template itself happens to depend on yet another template, I view 
that as an implementation detail of the template.  So, if we've got this 
situation:

- view.xhtml, which includes:
  - outerTemplate.xhtml, which includes:
    - innerTemplate.xhtml

In my ideal world (which, at the moment, does not match reality), 
view.xhtml should only be aware of the slots that are exposed by 
outerTemplate.xhtml.  That is, view.xhtml should not depend on the fact 
that outerTemplate.xhtml happens to include innerTemplate.xhtml, which 
may have its own slots.

The reality is that when nesting templates, the outermost template 
exposes *all* of the slots defined by any nested templates (no matter 
how many levels deep) to the consumer.  This seems like a violation of 
encapsulation to me.   We are exposing implementation details up through 
the template hierarchy that, I think, should not necessarily be exposed.

Of course, I understand that it may be useful to be able to pass content 
from view.xhtml all the way through to a slot defined by 
innerTemplate.xhtml.   If outerTemplate.xhtml wants to allow certain 
content to be passed through from view.xhtml to innerTemplate.xthml, 
that's fine.  However, this should be done by introducing a ui:insert 
inside of outerTemplate.xhtml - ie. by explicitly exposing a new slot 
that consumers of outerTemplate.xhtml can rely on.  That way, if the 
author of outerTemplate.xhtml reconsiders the implementation and decides 
not to delegate to innerTemplate.xhtml after all, users of 
outerTemplate.xhtml are not hosed.

Oh well, this looks like another cases where I am many years too late.  
That seems to be my fate these days.

Even if we cannot change this behavior for ui:insert now, I do think it 
is important that we avoid this behavior for composite components.  
Since we are tinkering with the 
composite:inertFacet/composite:insertChildren implementations now to be 
more Facelets/TemplateClient-like, let's just be careful to avoid 
introducing the Facelets nested insert behavior into composite components.

>> Right, so I see that InsertHandler is implemented this way, but... why?
>>  Seems silly to have ui:insert serve two entirely different purposes - ie.
>> to both identify an insertion point as well as to define content for
>> included templates.   Why should ui:insert act both as a ui:insert *and* as
>> a ui:define?   If a template author wants to pass content through from an
>> outer page to an inner/nested template, seems like the right way to
>> accomplish this would be to wrap the ui:insert inside of a ui:define.
>>     
>
> Well, if you define a template, you use ui:insert to mark the spots
> where content can be - and then you include default content right
> away. Is it this which makes ui:insert a thing of both worlds?
>   

Let's take a step back and look at the role of the TemplateClient 
contract.  TemplateClient allows consumers of templates (eg. 
ui:composition) to provide access to content that should be inserted 
into the template - ie. content defined by ui:define, inserted into 
ui:insert.  So, for example, both CompositionHandler and DecorateHandler 
are TemplateClients.

ui:insert is clearly a user of the TemplateClient interface.  
InsertHandler (via a call to FaceletContext.includeDefinition()) pulls 
in content provided by outer TemplateClients (eg. an ancestor 
ui:composition).

This is all well and good.

Where things get strange is that InsertHandler also implements 
TemplateClient.  This means that, in addition to being a consumer of 
TemplateClient content, ui:insert also exposes its own content for 
consumption by other consumers of TemplateClients.

So, if you've got:

  <ui:insert name="foo">
    body content
  </ui:insert>

The "body content" serves two roles:

1.  It provides default content in the event that no matching ui:define 
for "foo" can be found.
2.  It is exposed as content for consumption while performing the 
definition inclusion (during the call to 
FaceletContext.includeDefinition()).

Case #1 is expected/documented.

Case #2 is where ui:insert seems to acting as a ui:define.  This is 
non-obvious, and, as far as I can tell, not documented.

To me this looks like an old implementation quirk that we have 
inherited.  Does someone here happen to understand why this is useful?  
If this is useful, we need to document this behavior.  If it isn't 
useful, we should remove this part of the implementation.

Andy

> best regards,
>
> Martin
>
>

Re: [jsr-314-open] Fwd: PostAddToViewEvent publishing conditions

Posted by Martin Marinschek <mm...@apache.org>.

Hi Andy,

my 2cents (isn´t worth much in this discussion if I don´t look any
deeper into this ;)

> Given that ui:decorate is a non-trimming ui:composition, why would these
> behave differently with respect to interaction with the TemplateClient?  I
> would think that both of these handlers should be calling pushClient().

I don´t see why ui:decorate should be different from ui:composition, either.

> Also, the multi-level search strategy seems somewhat questionable to me.  A
> template exposes a certain number of of names into which content could be
> inserted.  The consumer of the template leverages that contract and
> specifies content to be inserted into some subset of those names.  When
> attempting to locate content to insert, why should the template look any
> further up the stack than the nearest consumer (ie. the most recently pushed
> TemplateClient)?


Well, templates are certainly hierarchical - does the nearest consumer
carry all information, or just the one which is defined in itself?

> Seems like this should have been implemented as a simple stack solution
> where the most recent TemplateClient is always used to resolve insertions.
>
> Of course, there may be perfectly fine reasons for why Facelets supports
> extendClient() and performs multi-level searches to locate content to
> insert.  Just not clear what these reasons are.
>
> Note that for composite components, we should not follow the Facelets
> precedent here.   If a composite component exposes a facet for insertion, we
> should only look to immediate consumer of the composite component for
> matches - ie. we should not be walking up the entire ancestor stack.
>
>> 3) Why does InsertHandler call extendClient()() and why does it implement
>> TemplateClient?
>>
>> Because ui:insert could expose a spot too, and it is resolved to there if
>> no ui:define (exposed by ui:composition) with the name is exposed too.
>
> Right, so I see that InsertHandler is implemented this way, but... why?
>  Seems silly to have ui:insert serve two entirely different purposes - ie.
> to both identify an insertion point as well as to define content for
> included templates.   Why should ui:insert act both as a ui:insert *and* as
> a ui:define?   If a template author wants to pass content through from an
> outer page to an inner/nested template, seems like the right way to
> accomplish this would be to wrap the ui:insert inside of a ui:define.

Well, if you define a template, you use ui:insert to mark the spots
where content can be - and then you include default content right
away. Is it this which makes ui:insert a thing of both worlds?

best regards,

Martin

-- 

http://www.irian.at

Your JSF powerhouse -
JSF Consulting, Development and
Courses in English and German

Professional Support for Apache MyFaces

Re: [jsr-314-open] Fwd: PostAddToViewEvent publishing conditions

Posted by Andy Schwartz <an...@oracle.com>.

On 5/22/10 9:36 PM, Leonardo Uribe wrote:
> Hi
>
> After some attempts I was able to make it work ( see 
> MYFACES-2638-6_5.patch ).

Great!

> I think I'll delay a little bit myfaces core 2.0.1 release, because 
> this solution has enought importance to be included. Definitively, the 
> way to go is use TemplateClient stuff here.
>

Agreed.  Should have much cleaner results than the current system event 
approach to insertion.

> In the long term, it is necessary to think how to create a good api 
> for TemplateClient stuff. Right now, DefaultFaceletContext / 
> DefaultFacelet classes do too many things (generate facelets ids to 
> isolate id generation between templates, template client api stuff, 
> handle facelet inclusion ......). On myfaces, the position about 
> change those classes is do not modify the code there too much, so in 
> the future if a new API is published (jsf 2.1?) we could change the 
> code without much effort.

Sure, I think this would be worth exploring in 2.1.   We should also try 
to understand how the AST proposal impacts all of this.

>
> In the next days I'll check this patch and document it better.
>
> About mojarra issue 1684 (thanks for the hint), doing the patch it was 
> clear what the intention of the current design:
>
> 1) What is the purpose of extendClient() and why just having 
> pushClient() was
> not enough?
>
> 2) Why does CompositionHandler call extendClient(), while 
> DecorateHandler calls
> pushClient()?
>
> On DefaultFaceletContext, there is a List<TemplateManager> that holds 
> the current templates (clients) that are used to resolve a "spot" 
> identified by a name.
>
> The basic idea is when FaceletContext.includeDefinition(UIComponent 
> parent, String name) is called, the algorithm try to resolve the 
> "spot" from top to bottom.
>
> <ui:composition> and <ui:insert> are added at last, using 
> extendClient(), but <ui:decorate> or user tag handlers create from 
> xhtml use pushClient(). The idea is resolve in this order:
>
> - ui:decorate spots defined by ui:define and user tag handlers created 
> from xhtml (again defined by ui:define).
> - ui:composition spots defined by ui:define.
> - ui:insert spots.
>

Sure, this is how things are currently implemented, but this the 
reasoning behind this approach is still not clear.

According to the old Facelets doc:

https://facelets.dev.java.net/nonav/docs/dev/docbook.html#templating-decorate

The only difference between ui:composition and ui:decoration is that 
ui:composition strips surrounding content whereas ui:composition will not:

> The decorate tag acts the same as a composition tag, but it will not 
> trim everything outside of it.

Given that ui:decorate is a non-trimming ui:composition, why would these 
behave differently with respect to interaction with the TemplateClient?  
I would think that both of these handlers should be calling pushClient().

Also, the multi-level search strategy seems somewhat questionable to 
me.  A template exposes a certain number of of names into which content 
could be inserted.  The consumer of the template leverages that contract 
and specifies content to be inserted into some subset of those names.  
When attempting to locate content to insert, why should the template 
look any further up the stack than the nearest consumer (ie. the most 
recently pushed TemplateClient)?

Seems like this should have been implemented as a simple stack solution 
where the most recent TemplateClient is always used to resolve insertions.

Of course, there may be perfectly fine reasons for why Facelets supports 
extendClient() and performs multi-level searches to locate content to 
insert.  Just not clear what these reasons are.

Note that for composite components, we should not follow the Facelets 
precedent here.   If a composite component exposes a facet for 
insertion, we should only look to immediate consumer of the composite 
component for matches - ie. we should not be walking up the entire 
ancestor stack.

> 3) Why does InsertHandler call extendClient()() and why does it implement
> TemplateClient?
>
> Because ui:insert could expose a spot too, and it is resolved to there 
> if no ui:define (exposed by ui:composition) with the name is exposed too.

Right, so I see that InsertHandler is implemented this way, but... why?  
Seems silly to have ui:insert serve two entirely different purposes - 
ie. to both identify an insertion point as well as to define content for 
included templates.   Why should ui:insert act both as a ui:insert *and* 
as a ui:define?   If a template author wants to pass content through 
from an outer page to an inner/nested template, seems like the right way 
to accomplish this would be to wrap the ui:insert inside of a ui:define.

Again - don't think composite components should follow the Facelets 
precedent here.

Also wonder whether we should be cleaning this up on the Facelets side 
for 2.1.

Andy

>
> best regards,
>
> Leonardo Uribe

Re: [jsr-314-open] Fwd: PostAddToViewEvent publishing conditions

Posted by Leonardo Uribe <lu...@gmail.com>.

Hi

After some attempts I was able to make it work ( see MYFACES-2638-6_5.patch
). I think I'll delay a little bit myfaces core 2.0.1 release, because this
solution has enought importance to be included. Definitively, the way to go
is use TemplateClient stuff here.

In the long term, it is necessary to think how to create a good api for
TemplateClient stuff. Right now, DefaultFaceletContext / DefaultFacelet
classes do too many things (generate facelets ids to isolate id generation
between templates, template client api stuff, handle facelet inclusion
......). On myfaces, the position about change those classes is do not
modify the code there too much, so in the future if a new API is published
(jsf 2.1?) we could change the code without much effort.

In the next days I'll check this patch and document it better.

About mojarra issue 1684 (thanks for the hint), doing the patch it was clear
what the intention of the current design:

1) What is the purpose of extendClient() and why just having pushClient()
was
not enough?

2) Why does CompositionHandler call extendClient(), while DecorateHandler
calls
pushClient()?

On DefaultFaceletContext, there is a List<TemplateManager> that holds the
current templates (clients) that are used to resolve a "spot" identified by
a name.

The basic idea is when FaceletContext.includeDefinition(UIComponent parent,
String name) is called, the algorithm try to resolve the "spot" from top to
bottom.

<ui:composition> and <ui:insert> are added at last, using extendClient(),
but <ui:decorate> or user tag handlers create from xhtml use pushClient().
The idea is resolve in this order:

- ui:decorate spots defined by ui:define and user tag handlers created from
xhtml (again defined by ui:define).
- ui:composition spots defined by ui:define.
- ui:insert spots.

3) Why does InsertHandler call extendClient()() and why does it implement
TemplateClient?

Because ui:insert could expose a spot too, and it is resolved to there if no
ui:define (exposed by ui:composition) with the name is exposed too.

best regards,

Leonardo Uribe

2010/5/21 Andy Schwartz <an...@oracle.com>

> Gang -
>
> Max did some investigation into the problems that Leonardo found when
> attempting to use the TemplateClient mechanism to implement composite
> component children/facet insertion.   It looks like there are some more
> fundamental problems here.  Max captured his thoughts in the following
> Mojarra issue:
>
>
> https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1684
>
> Please review - interesting stuff.
>
> Note - we still think that using a TemplateClient-like approach should be
> possible for composite component insertion, so hopefully the implementations
> can continue to pursue this option.
>
> Andy
>
>
>  ---------- Forwarded message ----------
>> From: Max Starets <ma...@oracle.com>
>> Date: Fri, May 21, 2010 at 3:58 PM
>> Subject: Re: [jsr-314-open] PostAddToViewEvent publishing conditions
>> To: jsr-314-open@jcp.org
>> Cc: dev@myfaces.apache.org
>>
>>
>> Hey Leonardo,
>>
>> You are right - the same issue reproduces with two nested
>> ui:composition templates.
>> I logged the following Mojarra issue for the problem:
>> https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1684
>>
>> I do not think we have to abandon the idea of using TemplateClient
>> mechanism for for the composite component implementation
>> because of this issue. You are already using a separate TemplateClient
>> stack for composites in your patch, so we do not
>> need to wait for the fix before implementing the correct behavior for
>> composites. All we need is a stack with only one TemplateClient
>> being considered 'current'. includeDefinition() should be checking the
>> current TemplateClient only, and we should pop the current
>> TemplateClient before calling TemplateClient.apply(), then restore
>> (push) it when the apply() is done.
>>
>> Max
>>
>>
>> Leonardo Uribe wrote:
>>
>>
>>> Hi
>>>
>>> I tried the proposal of use some variation of TemplateClient API (I
>>> attach it as reference on MYFACES-2638-6.patch ). The solution works for
>>> simple cases, but it does not when nested composite components are used.
>>>
>>> Look this example (I removed the non relevant code):
>>>
>>> testCompositeInsertChildren.xhtml (the page when it is used)
>>>
>>> <testComposite:compositeInsertChildren>
>>>   <h:outputText value="GAMMA " />
>>> </testComposite:compositeInsertChildren>
>>>
>>> testComposite:compositeInsertChildren
>>>
>>> <composite:implementation>
>>>   <testComposite:compositeInsertChildrenInner>
>>>       <h:outputText value="BETA " />
>>>       <composite:insertChildren />
>>>   </testComposite:compositeInsertChildrenInner>
>>> </composite:implementation>
>>>
>>> testComposite:compositeInsertChildrenInner
>>>
>>> <composite:implementation>
>>>   <h:outputText value="ALFA " />
>>>   <composite:insertChildren/>
>>>   <h:outputText value="OMEGA " />
>>> </composite:implementation>
>>>
>>> The example should render this:
>>>
>>> ALFA BETA GAMMA OMEGA
>>>
>>> But it is rendered this:
>>>
>>> ALFA GAMMA OMEGA
>>>
>>> The current algorithm do it correctly because the tree is completely
>>> built before relocate, so the listener relocates BETA too. Facelets executed
>>> the inner FaceletHandler instance first and then the outer ones. On
>>> composite components, it executes first the composite component facelet,
>>> then the FaceletHandler children.
>>>
>>> Suggestions are welcome.
>>>
>>> regards,
>>>
>>> Leonardo Uribe
>>>
>>>
>>
>