[jira] [Commented] (WICKET-4617) ResourceStreamLocator vs ResourceFinder

["Martin Grigorov (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/WICKET-4617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13404749#comment-13404749 ] 

Martin Grigorov commented on WICKET-4617:
-----------------------------------------

Few comments from me:

1) Do we really need : Application#getResourceFinderForPath(String path) ?
I find this as a helper method which place is not in Application API. 
Additionally I'm not sure that we really need this helper method in any helper class. I personally never needed to create IResourceFinder in my apps.
Additionally it is overridden in WebApplication, so I think no one will ever use the one in Application. AFAIK there are no non-Web impls

I think IResourceSettings#addResourceFolder(String) should become IResourceSettings#addResourceFinder(IResourceFinder). Or just remove this method and let the user use #getResourceFinders().add(myFinder) as all listener related methods in WebApplication currently work.
 
2) make member fields 'final' if they are not going to change after class construction. E.g. WebApplicationPath#path

3) In ClassPathResourceFinder, line 46:
 this.prefix = prefix + "/";

What if the passed argument already have the trailing slash ?
Also there is concatenation at https://github.com/apache/wicket/compare/master...sandbox%2Fresourcefinder#L2R53 which may make this even more broken if 'path' has a leading slash.

4) Use SLF4J '{}' placeholders and avoid logger.isXyzEnabled() is possible.
https://github.com/apache/wicket/compare/master...sandbox%2Fresourcefinder#L4R132

5)  https://github.com/apache/wicket/compare/master...sandbox%2Fresourcefinder#L7R387
Is there a need for Args.nonNull() here ?
 

                
> ResourceStreamLocator vs ResourceFinder
> ---------------------------------------
>
>                 Key: WICKET-4617
>                 URL: https://issues.apache.org/jira/browse/WICKET-4617
>             Project: Wicket
>          Issue Type: Bug
>    Affects Versions: 6.0.0-beta2, 1.5.7
>            Reporter: Carl-Eric Menzel
>            Assignee: Carl-Eric Menzel
>
> I'm a bit confused by the responsibilities of ResourceFinder vs ResourceStreamLocator. Looking around in the code, I found the following:
> - IResourceFinder is apparently only implemented via its extension interface IResourcePath. Its two implementations Path and WebApplicationPath look through a list of filesystem folders for files.
> - ResourceStreamLocator does two things:
>   - it loads resources, either via an IResourceFinder (which only finds filesystem resources) or via the classloader (it does this itself).
>   - it uses a ResourceNameIterator to generate all possible filename variations based on locale and style and then tries to load one of them via the above mechanisms.
> Is this correct?
> If so, I think we have some mixed-up responsibilities here. I propose the following:
> - add a third IResourcePath implementation (e.g. ClassloaderPath) that handles loading of resources in classpaths. It should be able to try multiple paths (e.g. "/", "META-INF/resources" etc).
> - Instead of a single ResourceFinder, Application should have a list of them, by default containing WebApplicationPath (today's default) and the new ClassloaderPath.
> - ResourceStreamLocator should not do any loading on its own at all and just use the ResourceFinders defined in this new list in Application.
> This would also get rid of the hard-coded "META-INF/resources" lookup that currently is done in ResourceStreamLocator (I'll write a second ticket about that, it's causing us some problems).
> I think this could still be done within 6.0. Objections?

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira