On 5/22/10 9:36 PM, Leonardo Uribe wrote: Great! Agreed. Should have much cleaner results than the current system event approach to insertion. 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. 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-deco... The only difference between ui:composition and ui:decoration is that ui:composition strips surrounding content whereas ui:composition will not: 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. 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

On 5/26/10 1:27 AM, Martin Marinschek 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()). 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. 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

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: 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()). 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. 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