Atlas: Refactoring ContextLoader out of existence

[Dave Johnson <sn...@gmail.com>]

As I do the work to add Atlas page models to Roller, I'd also like to refactor
the existing page model and context loading process to address a couple of
problems:

- Context loading process should not do unncessary work
- Context loading process and page models should not depend on Velocity

To accomplish this, I'm proposing that we:


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)


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.


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)

The WeblogPageModel becomes the new home for most of the code that was in
the old ContextLoader class, except for entry/comment and feed stuff.

   class WeblogPageModel implements PageModel
     Same as old PageModel class
     Init method loads TemplateContext (as ContextLoader once did)


The WeblogEntryPageModel extends WeblogPageModel because
entry/comments pages need the old PageModel methods. It's init method
calls super.init() and also loads comment specific values.

   WeblogEntryPageModel extends WeblogPageModel
     Init method loads TemplateContext with values needed for entry/comments

The FeedPageModel extends WeblogPageModel, because our feed templates (atom.vm
and rss.vm) depend on the old PageModel. The ContextLoader code that loaded
RSS values goes here.

   FeedPageModel extends WeblogPageModel
     Init method loads TemplateContext with values needed for feeds


4) change the PageServlet and FlavorServlet classes to load the appropriate
page models.

  class PageServlet
    if no weblog entry is specified
        Create page model specified by "weblogpagemodel.classname"
        put it into context
    else
        Create page model specified by "weblogentrypagemodel.classname"
        put it into context
    for each additional page model specified by current weblog
        Create page model
        put it into context

class FlavorServlet
    Create page model specified by "feedpagemodel.classname"
    put it into context


Thoughts, comments, objections?

- Dave

Re: Atlas: Refactoring ContextLoader out of existence

[Allen Gilliland <al...@sun.com>]

comments inline ...

Dave Johnson wrote:
> Comments inline...
> 

*snip*

> 
> 
>> 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.

I agree and disagree.  I agree that cleanup is important, but I disagree 
that we have to put the old ContextLoading stuff into the new PageModel 
stuff.

More details 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.

I think I am confused.  If moving forward we don't want to let models 
put things in the context, then why give them the opportunity?

Personally, I think that page model developers can get along fine 
without having to place objects in the context directly.  To me the 
point of using the PageModel is to provide some encapsulation so that 
instead of having a context with this in it ...

obj1
obj2
obj3
etc
etc2

we have something more organized like this ...

mymodel.obj1
mymodel.obj2
mymodel.obj3
mymodel.etc
mymodel.etc2

you can still access the same exact data and methods through a model 
that you can by putting the objects directly into the context.


> 
> 
>> 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?

No, i don't object completely.  I object to the approach that we need to 
split up the various page models in order organize the context loading 
process.

If these various page models really are going to offer access to 
different data and different methods then that's fine.


> 
> 
> 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.

This sounds like a double edged sword to me.  On one hand I agree it's 
nice for someone to plug in their own page model and ditch all of our 
old crud, but there are big ramifications to that.  There is a large 
potential for someone to mess up and replace our page model with a 
custom one when they still need all of our legacy context stuff.

If we want to let someone EOL our old context loading stuff then I 
suggest we do it apart from the page models.


> 
> 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.

I agree that the page models should be fully pluggable, but I see more 
danger than benefit in letting people remove all the old stuff loaded 
into the context.


> 
> If you still object to this approach, maybe you could suggest an 
> alternative
> plan for accomplishing these goals?

My plan would simply be to deprecate all of the old stuff in the context 
rather than try and EOL it.  We'd do that in these general steps ...

1. Round up the current context loading stuff and any objects it uses 
and group it all together somewhere that is identified as legacy context 
loading crud.  This includes the current page model, page helper, etc. 
Something like ui.rendering.legacy or ui.rendering.old.

2. Leave the legacy context loading process in place on the page 
servlet, which is the only place that users have templates that use that 
stuff.  Remove it from everywhere else, feed, search, etc.

3. Define completely new page models to be used moving forward.  These 
models will be totally self contained and designed how we want things to 
work moving foward.

4. Rewrite our existing velocity templates and themes to use only the 
new page models and any other supporting objects that we want to 
continue supporting for the long term.  To lighten this burden I suggest 
that we make 3.0 the point at which we EOL all the old themes and pick 
just a couple new ones to refactor and ship with Roller for 3.0 and beyond.

5. Optionally we can define some kind of mechanism to turn off the old 
context loading stuff.  Probably the best way to do this is to begin 
tracking what version of Roller the person began their installation 
with.  So starting with new installations 3.0 we could have the old 
context loading stuff disabled by default.  This is still a possible 
problem though, if someone gets their hands on old templates/themes in a 
3.0+ install they will no longer work, so we need to tread carefully here.

So the basic approach is to isolate all of the old stuff and consider it 
deprecated.  Then change our existing stuff to only use the new code 
which we want to use moving forward.

The other fairly cool thing about this is that we would only document 
the new models and objects that we really want to support as we create 
them.  We don't want to tell people about the old stuff and give them a 
chance to start doing things the wrong way.

-- Allen


> 
> - Dave

Re: Atlas: Refactoring ContextLoader out of existence

[Dave Johnson <sn...@gmail.com>]

Comments inline...


On 6/9/06, Allen Gilliland <Al...@sun.com> 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

Re: Atlas: Refactoring ContextLoader out of existence

[Allen Gilliland <Al...@Sun.COM>]

comments inline ...

Dave Johnson wrote:
> As I do the work to add Atlas page models to Roller, I'd also like to 
> refactor
> the existing page model and context loading process to address a couple of
> problems:
> 
> - 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.


> 
> To accomplish this, I'm proposing that we:
> 
> 
> 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.


> 
> 
> 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.

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.


> 
> 
> 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)
> 
> The WeblogPageModel becomes the new home for most of the code that was in
> the old ContextLoader class, except for entry/comment and feed stuff.
> 
>   class WeblogPageModel implements PageModel
>     Same as old PageModel class
>     Init method loads TemplateContext (as ContextLoader once did)
> 
> 
> The WeblogEntryPageModel extends WeblogPageModel because
> entry/comments pages need the old PageModel methods. It's init method
> calls super.init() and also loads comment specific values.
> 
>   WeblogEntryPageModel extends WeblogPageModel
>     Init method loads TemplateContext with values needed for entry/comments
> 
> The FeedPageModel extends WeblogPageModel, because our feed templates 
> (atom.vm
> and rss.vm) depend on the old PageModel. The ContextLoader code that loaded
> RSS values goes here.
> 
>   FeedPageModel extends WeblogPageModel
>     Init method loads TemplateContext with values needed for feeds

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.

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.


> 
> 
> 4) change the PageServlet and FlavorServlet classes to load the appropriate
> page models.
> 
>  class PageServlet
>    if no weblog entry is specified
>        Create page model specified by "weblogpagemodel.classname"
>        put it into context
>    else
>        Create page model specified by "weblogentrypagemodel.classname"
>        put it into context
>    for each additional page model specified by current weblog
>        Create page model
>        put it into context
> 
> class FlavorServlet
>    Create page model specified by "feedpagemodel.classname"
>    put it into context

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.

-- Allen


> 
> 
> Thoughts, comments, objections?
> 
> - Dave