+1
The improved proposal and default implementation look good.
Thanks!
> On Jul 21, 2016, at 2:01 PM, Leonardo Uribe <leonardo.uribe_at_irian.at> wrote:
>
> Hi
>
> It has sense to use library / name instead Resource instance. In MyFaces, there is a cache holding Resource instances, so the instantiation is only a bit expensive the first time. In MyFaces there is also a ResourceIdentifier interface class called ResourceMeta, but its use is internal only. It helps to identify a resource in a unique way.
>
> But the map we are discussing here only have library / name into account, without take into account libraryVersion/resourceVersion or locale/contract. Its only purpose is to keep track of the rendered resources and avoid duplicates, at least when the full page is built. You only need library / name at that point of the algorithm, so if the methods uses library / name, it looks good to me.
>
> +1
>
> regards,
>
> Leonardo Uribe
>
>
>
> 2016-07-21 4:10 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
> Here's a kickoff of the default implementation:
>
> /**
> * <p class="changed_added_2_3">
> * Mark the resource as identified by given resource and library name as rendered. The default implementation must
> * ensure that {_at_link #isResourceRendered(FacesContext, String, String)} will return <code>true</code> when the
> * resource has already been rendered during the render response phase of the current view.
> * </p>
> * @param context The {_at_link FacesContext} for this request.
> * @param resourceName The name of the resource.
> * @param libraryName The name of the library in which the resource resides, may be <code>null</code>.
> * @since 2.3
> */
> public void markResourceRendered(FacesContext context, String resourceName, String libraryName) {
> String resourceIdentifier = libraryName + ":" + resourceName;
> Set<String> resourceIdentifiers = (Set<String>) context.getAttributes().computeIfAbsent(ResourceHandler.class.getName(), k -> new HashSet<>());
> resourceIdentifiers.add(resourceIdentifier);
> }
>
> /**
> * <p class="changed_added_2_3">
> * Returns whether the resource as identified by given resource and library name has been rendered. The default
> * implementation must return <code>true</code> when the resource has been marked as rendered via
> * {_at_link #markResourceRendered(FacesContext, String, String)} during the render response phase of the current view.
> * </p>
> * @param context The {_at_link FacesContext} for this request.
> * @param resourceName The name of the resource.
> * @param libraryName The name of the library in which this resource resides, may be <code>null</code>.
> * @return Whether the resource as identified by given resource and library name has been rendered.
> * @since 2.3
> */
> public boolean isResourceRendered(FacesContext context, String resourceName, String libraryName) {
> String resourceIdentifier = libraryName + ":" + resourceName;
> Set<String> resourceIdentifiers = (Set<String>) context.getAttributes().get(ResourceHandler.class.getName());
> return resourceIdentifiers != null && resourceIdentifiers.contains(resourceIdentifier);
> }
>
>
> Cheers, B
>
>
> On Thu, Jul 21, 2016 at 11:02 AM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> While at it, should there be a default implementation too? Both Mojarra and MyFaces sets it as context attribute. They only do it differently. Mojarra puts name+library right away as key while MyFaces holds them all in a Map which in turn is stored as context attribute.
>
> I personally suggest holding a Set<String> or perhaps Set<ResourceIdentifier> as context attribute.
>
> Cheers, B
>
> On Thu, Jul 21, 2016 at 10:51 AM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> Hi,
>
> I do agree that UIComponent argument should be replaced. I only don't strongly agree that it should be a Resource instance as creating it is not cheap per se. Basically only the resource identifier (library+name) is sufficient for the task. There is however no such model in Java API. In OmniFaces there's one: https://github.com/omnifaces/omnifaces/blob/625ed0d52c640d7b8a25ba02295c2c77b8f75fe1/src/main/java/org/omnifaces/resourcehandler/ResourceIdentifier.java This should also reduce the need of carrying around loose but closely related name+library variables through the JSF impl.
>
> What do you guys think about taking over this in JSF API as javax.faces.model.ResourceIdentifier? Only, the current API such as createResource() would need another overload for that.
>
> Alternatively, we could also just ask for resource name and library directly.
>
> abstract void markComponentResourceRendered(FacesContext context, String library, String name);
> abstract boolean isComponentResourceRendered(FacesContext context, String library, String name);
>
> That's also more consistent with existing ResourceHandler API. I only don't particularly like it, but I feel it's kind of too late to introduce a ResourceIdentifier as the loose variables are already used over all place in JSF API.
>
> Cheers, B
>
> On Wed, Jul 20, 2016 at 8:28 AM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> Hi,
>
> I was already doubting between UIComponent or a new ResourceIdentifier model with a.o. name and library properties. This should skip the need for a createResource call.
>
> Cheers, B
>
>
> On Wed, Jul 20, 2016, 02:55 Leonardo Uribe <leonardo.uribe_at_irian.at> wrote:
> Hi
>
> I have to change my +1 to +0.
>
> I just notice there is always a calculation of the underlying Resource instance,
> before call the method, so it looks better if we pass the Resource instance
> as parameter.
>
> public void markComponentResourceRendered(FacesContext context,
> Resource resource, UIComponent componentResource)
>
> public boolean isComponentResourceRendered(FacesContext context,
> Resource resource, UIComponent componentResource)
>
> The UIComponent instance could be optional. So it could be valid to call:
>
> ResourceHandler.isComponentResourceRendered(context, resource, null);
>
> The Resource instance is required, from that one it is easier to
> get the libraryName/resourceName. If you have two different component
> instances pointing to the same Resource, once the resource is rendered
> you don't need to render the other one.
>
> From a theoretical perspective, a componentResource is a component
> that:
>
> - Can be added to a specific target ("head", "body", "form").
> - Could have an associated Resource instance (optional).
>
> It is just that if you only have the UIComponent instance, how you calculate
> the underlying Resource instance is not clear, and that step can be different
> between components.
>
> regards,
>
> Leonardo Uribe
>
>
>
> 2016-07-19 18:52 GMT-05:00 Leonardo Uribe <leonardo.uribe_at_irian.at>:
> +1
>
> 2016-07-19 5:55 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
> Hi,
>
> I have rephrased the javadocs on those methods.
>
>
> /**
> * <p class="changed_added_2_3">
> * Mark given component resource as rendered. The default implementation must ensure that
> * {_at_link #isComponentResourceRendered(FacesContext, UIComponent)} will return <code>true</code> when given
> * component resource has already been rendered during the render response phase of the current view.
> * </p>
> * @param context The {_at_link FacesContext} for this request.
> * @param componentResource The {_at_link UIComponent} representing a {_at_link Resource} instance.
> * @since 2.3
> */
> public abstract void markComponentResourceRendered(FacesContext context, UIComponent componentResource);
>
> /**
> * <p class="changed_added_2_3">
> * Returns whether given component resource has been rendered. The default implementation must return
> * <code>true</code> when given component resource has been marked as rendered via
> * {_at_link #markComponentResourceRendered(FacesContext, UIComponent)} during the render response phase of the current
> * view.
> * </p>
> * @param context The {_at_link FacesContext} for this request.
> * @param componentResource The {_at_link UIComponent} representing a {_at_link Resource} instance.
> * @return Whether given component resource has been rendered.
> * @since 2.3
> */
> public abstract boolean isComponentResourceRendered(FacesContext context, UIComponent componentResource);
>
>
> The literal part "current view" is important as this forces the implementation to clear out the marks when the view changes during render response, usually due to an exception which would show an error page view.
>
> Cheers, B
>
> On Tue, Jul 19, 2016 at 10:18 AM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> Hi,
>
> After all, adding those methods to ResourceHandler is cleaner API and allows better abstraction with a custom ResourceHandler.
>
>
> /**
> * <p class="changed_added_2_3">
> * Mark given component resource as rendered.
> * The default implementation stores the resource identifier as context attribute.
> * </p>
> * @param context The {_at_link FacesContext} for this request.
> * @param componentResource The {_at_link UIComponent} representing a {_at_link Resource} instance.
> * @since 2.3
> */
> public void markComponentResourceRendered(FacesContext context, UIComponent componentResource) {
> // TODO
> }
>
> /**
> * <p class="changed_added_2_3">
> * Returns whether given component resource has been rendered.
> * The default implementation checks if the resource identifier has been stored as context attribute.
> * </p>
> * @param context The {_at_link FacesContext} for this request.
> * @param componentResource The {_at_link UIComponent} representing a {_at_link Resource} instance.
> * @return Whether given component resource has been rendered.
> * @since 2.3
> */
> public boolean isComponentResourceRendered(FacesContext context, UIComponent componentResource) {
> // TODO
> }
>
>
> If there are no objections, I will take care of it.
>
> Cheers, B
>
>
>
> On Thu, Sep 3, 2015 at 11:08 PM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> I created https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1404 on this.
>
> Cheers, B
>
> On Wed, Mar 4, 2015 at 6:11 PM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> By just adding it to the set.
>
> Cheers, B
>
>
> On 16:25, Wed, Mar 4, 2015 arjan tijms <arjan.tijms_at_gmail.com> wrote:
> Hi,
>
> Sounds good, but one question:
>
> If the proposed API just returns a Set<String> where presence of a
> resource identifier means that resource has been rendered, how does
> the library indicate it wants to suppress rendering of a certain
> resource?
>
> Kind regards,
> Arjan
>
>
>
>
> On Wed, Mar 4, 2015 at 4:09 PM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> > In hindsight, that boolean is superfluous. Make it a mutable
> > Set<ResourceIdentifier> or Set<String> and just consider the presence in the
> > set as already rendered.
> >
> > Cheers, B
> >
> > On Wed, Mar 4, 2015 at 4:05 PM, Bauke Scholtz <balusc_at_gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> Both Mojarra and MyFaces have an internal way to mark a script or
> >> stylesheet resource as rendered (to avoid duplicate rendering). Mojarra sets
> >> it as a context attribute with name+library as key and a boolean true as
> >> value (see a.o. StylesheetRenderer#encodeEnd()). MyFaces sets it as a
> >> context attribute via a Map<String, Boolean> (see a.o.
> >> HtmlStylesheetRenderer#encodeEnd() via a ResourceUtils helper class).
> >>
> >> For component library developers it would be very useful if this
> >> information is available by standard API means so that the component library
> >> can if necessary check and/or suppress the rendering of those resources. For
> >> example to combine those resources via a special resource handler, or to
> >> automatically delegate to a CDN host, or to turn a script resource into a
> >> deferred script, etcetera.
> >>
> >> I'd propose adding UIViewRoot#getRenderedComponentResources() which
> >> returns a mutable Map<ResourceIdentifier, Boolean> for the purpose where
> >> ResourceIdentifier could also be a String in the standard format
> >> library+":"+name.
> >>
> >> What do others think about it?
> >>
> >> Cheers, B
> >
> >
>
>
>
>
>
>
>
>
>