Comments inline...
On 6/9/06, Allen Gilliland <[EMAIL PROTECTED]> wrote:
comments inline ... Dave Johnson wrote: > - Context loading process should not do unncessary work > - Context loading process and page models should not depend on Velocity Definitely agree with these 2 objectives.
Yes!
> 1) establish a simple TemplateContext interface so page models can deal > with the Velocity Context in a generic way. Something simple like this: > > interface TemplateContext > put(String s, Object o) > Object get(String s) I'm not sure that we want the Models to have the ability to talk the VelocityContext, but if we do then I would prefer we just use a Map. I can't see any reason to make our own interface/implementation when it does the same thing as a Map.
Good point. A map will work fine. There will be a little extra overhead because we'll have to transfer the things from the map and to the VelocityContext (which unfortunately does not implement Map).
> 2) we eliminate the ContextLoader class and make page models responsible > for loading the template context with the various globals that are > associated with the page model. I would actually prefer that Models don't have any knowledge of the VelocityContext or a generic equivalent. I think the Models should be totally self contained, so they shouldn't need to know how/where they are being used.
Agree.
Unfortunately this means we basically would need to leave the existing ContextLoading stuff as is, but consider it deprecated. Then moving forward we redo our velocity themes/templates to use the new model objects.
Disagree. We need to clean this stuff up into a nice little bundles (the page models) so Roller can work without it. I'll explain that below.
> 3) we create three different page models for our three different rendering > situations: weblog display, weblog entry/comments display and feed display. > First, all page models implement this interface because they're pluggable: > > interface PageModel > getModelName() > init(request, response, TemplateContext) > This is a fairly different approach than what I was thinking about. In this approach you aren't really using Models just to provide rendering functionality, instead they are used in large part for providing context loading functionality. I would prefer that we keep those 2 things separate actually.
I totally agree that, moving forward, our page models should not put things into the context . But some page model developers might not share our great love of page models, so why not leave the Context argument in the init() method so developers can decide for themselves what goes in context with their page model? Plus, we need it ourselves.
An example is the way we use Models in the admin/authoring UI struts pages. The struts actions load up the model (jsp scopes like request, page, session) with any objects or Models that are to be used at rendering time. However the Models are not allowed to write their own stuff to the jsp scopes. I would prefer we follow this approach. What I am proposing is that instead of setting up the VelocityContext like we have in the past, where it's littered with dozens and dozens of variables everywhere. Instead we just load it with Models moving forward and all rendering functionality is used via the models.
Yes and that's exactly why I started the move to page models in the editor UI, I hate littering the request with typeless attributes.
> 4) change the PageServlet and FlavorServlet classes to load the appropriate > page models. > I still agree with this sentiment that the Servlets should load only the models which are needed/allowed to render the content, but I am not on board with all the various Models that you listed.
Do you really object to the three models: weblog, entry and feed -- or are you only objecting to moving context loader stuff to page models? Ok, so I guess the best argument I have for doing this work is: If we tuck the context loading stuff away into the page models, make the page models configurable as I have suggested then folks who do not want to use our page models can simply provide their own and they will not have their velocity context's littered with the ContextLoader crap. We're doing to have to live with the old page models for a while, I'm documenting them now and we're using them in 3.0. So why not treat them systematically and make it possible for people to completely opt-out of them if they want to provide thier own page models. If you still object to this approach, maybe you could suggest an alternative plan for accomplishing these goals? - Dave
