On Wed, Mar 2, 2011 at 18:44, Vincent Massol <vincent@massol.net> wrote:

I forgot to menton the alternative which is to create a WebAppVelocityFactory component impl (hint = "webapp"), in the same manner that I have already created WebAppVelocityConfiguration and WebAppVelocityEngine.

I think my preference foes to the API modification now.

WebAppVelocityFactory sounds nicer but I don't know very well this subject so I think I will trust you on that. Here is a +0.5 ;)

Thanks -Vincent

On Mar 2, 2011, at 4:49 PM, Vincent Massol wrote:

...

ok I have no added the following 2 new component impls: * WebAppVelocityConfiguration * WebAppVelocityEngine

However it's not enough since DefaultVelocityEngine.createVelocityEngine() does a component lookup on VelocityEngine and we need a way for it to look up with a passed hint.

I'm proposing to modify the current VelocityFactory API from:

   VelocityEngine createVelocityEngine(String key, Properties properties) throws XWikiVelocityException;

to:

   VelocityEngine createVelocityEngine(String key, Properties properties, String engineType) throws XWikiVelocityException;

where engineType is the hint corresponding to the engine to look up.

Then DefaultVelocityManager can call VelocityFactory, passing the "webapp" hint to createVelocityEngine.

WDYT?

Thanks -Vincent

On Mar 2, 2011, at 1:56 PM, Vincent Massol wrote:

...

Just a heads up of what I'm working on right now: cleaning up xwiki-velocity dependencies.

I'm going to split it into several submodules: * xwiki-velocity-management (or xwiki-velocity-jmx) * xwiki-velocity-default * xwiki-velocity-webapp (implementation of VelocityEngine and VelocityConfiguration with hint = "webapp",  this is to use use velocity with the WebappResourceLoader loading templates from the webapp's root dir)

-Vincent

_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Thomas Mortagne

ok I think I'm going to do something different... :) Here's a new proposal: * Create a XWikiWebappResourceLoader that extends Velocity Tool's WebappResourceLoader and that sets the ServletContext in the velocity engine application properties (required for WebappResourceLoader to work) * Users of the xwiki-velocity module will need to specify a resource loader (should they need one - it's not mandatory if you don't use templates). More specifically they'll need to pass: ** resource.loader = "xwiki" ** xwiki.resource.loader.class = XWikiWebappResourceLoader.class.getName() Technically we'll be able to acces the ComponentManager in XWikiWebappResourceLoader because in DefaultVelocityEngine's init() method, just after we do a new org.apache.velocity.app.VelocityEngine() we'll do an engine.setApplicationAttribute(ComponentManager.class.getName(), componentManager) This is then accessible from XWikiWebappResourceLoader since the rsvc (aka velocity engine) is accessible from there (we extend WebappResourceLoader). Note that this also allows us to extend/implement other Velocity subsystems and to make our CM available to them. WDYT? I think it's much better since: * we don't change any API * we use the existing velocity configuration system to configure the behavior to use * it's in the spirit of the Velocity framework * it's flexible for the users of the xwiki-velocity module Thanks -Vincent On Mar 3, 2011, at 1:51 AM, Sergiu Dumitriu wrote:

On 03/02/2011 06:44 PM, Vincent Massol wrote:

...

I forgot to menton the alternative which is to create a WebAppVelocityFactory component impl (hint = "webapp"), in the same manner that I have already created WebAppVelocityConfiguration and WebAppVelocityEngine.

I think my preference foes to the API modification now.

I don't like either one.

Adding the hint parameter to the API is nice if there ever will be a need to have different types of engines at runtime. Do we have this need at the moment? I don't think so, so IMO it's better to keep the API as simple as possible while still serving our needs.

Implementing WebAppVelocityFactory somehow beats the definition of a factory, at least in my view. A factory usually is a single class that knows how to create objects of different types. Normally the preferred approach is indeed to add a "hint" parameter, but as I said we don't really need it at the moment.

What if we just change the implementation of the factory so that the default hint is taken from a configuration? What we'll need to do is to provide a different configuration for use inside XWiki.

...

Thanks -Vincent

On Mar 2, 2011, at 4:49 PM, Vincent Massol wrote:

...

ok I have no added the following 2 new component impls: * WebAppVelocityConfiguration * WebAppVelocityEngine

However it's not enough since DefaultVelocityEngine.createVelocityEngine() does a component lookup on VelocityEngine and we need a way for it to look up with a passed hint.

I'm proposing to modify the current VelocityFactory API from:

VelocityEngine createVelocityEngine(String key, Properties properties) throws XWikiVelocityException;

to:

VelocityEngine createVelocityEngine(String key, Properties properties, String engineType) throws XWikiVelocityException;

where engineType is the hint corresponding to the engine to look up.

Then DefaultVelocityManager can call VelocityFactory, passing the "webapp" hint to createVelocityEngine.

WDYT?

Thanks -Vincent

On Mar 2, 2011, at 1:56 PM, Vincent Massol wrote:

...

Just a heads up of what I'm working on right now: cleaning up xwiki-velocity dependencies.

I'm going to split it into several submodules: * xwiki-velocity-management (or xwiki-velocity-jmx) * xwiki-velocity-default * xwiki-velocity-webapp (implementation of VelocityEngine and VelocityConfiguration with hint = "webapp", this is to use use velocity with the WebappResourceLoader loading templates from the webapp's root dir)

-Vincent

On 03/03/2011 10:17 AM, Vincent Massol wrote:

ok I think I'm going to do something different... :) Here's a new proposal:

* Create a XWikiWebappResourceLoader that extends Velocity Tool's WebappResourceLoader and that sets the ServletContext in the velocity engine application properties (required for WebappResourceLoader to work) * Users of the xwiki-velocity module will need to specify a resource loader (should they need one - it's not mandatory if you don't use templates). More specifically they'll need to pass: ** resource.loader = "xwiki" ** xwiki.resource.loader.class = XWikiWebappResourceLoader.class.getName()

Technically we'll be able to acces the ComponentManager in XWikiWebappResourceLoader because in DefaultVelocityEngine's init() method, just after we do a new org.apache.velocity.app.VelocityEngine() we'll do an engine.setApplicationAttribute(ComponentManager.class.getName(), componentManager)

This is then accessible from XWikiWebappResourceLoader since the rsvc (aka velocity engine) is accessible from there (we extend WebappResourceLoader).

Note that this also allows us to extend/implement other Velocity subsystems and to make our CM available to them.

WDYT?

I think it's much better since: * we don't change any API * we use the existing velocity configuration system to configure the behavior to use * it's in the spirit of the Velocity framework * it's flexible for the users of the xwiki-velocity module

+1.

Thanks -Vincent

On Mar 3, 2011, at 1:51 AM, Sergiu Dumitriu wrote:

...

On 03/02/2011 06:44 PM, Vincent Massol wrote:

...

I forgot to menton the alternative which is to create a WebAppVelocityFactory component impl (hint = "webapp"), in the same manner that I have already created WebAppVelocityConfiguration and WebAppVelocityEngine.

I think my preference foes to the API modification now.

I don't like either one.

Adding the hint parameter to the API is nice if there ever will be a need to have different types of engines at runtime. Do we have this need at the moment? I don't think so, so IMO it's better to keep the API as simple as possible while still serving our needs.

Implementing WebAppVelocityFactory somehow beats the definition of a factory, at least in my view. A factory usually is a single class that knows how to create objects of different types. Normally the preferred approach is indeed to add a "hint" parameter, but as I said we don't really need it at the moment.

What if we just change the implementation of the factory so that the default hint is taken from a configuration? What we'll need to do is to provide a different configuration for use inside XWiki.

...

Thanks -Vincent

On Mar 2, 2011, at 4:49 PM, Vincent Massol wrote:

...

ok I have no added the following 2 new component impls: * WebAppVelocityConfiguration * WebAppVelocityEngine

However it's not enough since DefaultVelocityEngine.createVelocityEngine() does a component lookup on VelocityEngine and we need a way for it to look up with a passed hint.

I'm proposing to modify the current VelocityFactory API from:

VelocityEngine createVelocityEngine(String key, Properties properties) throws XWikiVelocityException;

to:

VelocityEngine createVelocityEngine(String key, Properties properties, String engineType) throws XWikiVelocityException;

where engineType is the hint corresponding to the engine to look up.

Then DefaultVelocityManager can call VelocityFactory, passing the "webapp" hint to createVelocityEngine.

WDYT?

Thanks -Vincent

On Mar 2, 2011, at 1:56 PM, Vincent Massol wrote:

...

Just a heads up of what I'm working on right now: cleaning up xwiki-velocity dependencies.

I'm going to split it into several submodules: * xwiki-velocity-management (or xwiki-velocity-jmx) * xwiki-velocity-default * xwiki-velocity-webapp (implementation of VelocityEngine and VelocityConfiguration with hint = "webapp", this is to use use velocity with the WebappResourceLoader loading templates from the webapp's root dir)

-Vincent

-- Sergiu Dumitriu http://purl.org/net/sergiu/