Comments inline ...
Dave Johnson wrote:
Thanks for the feedback. Lots of good suggestions there.
Comments inliine...
On 6/14/06, Allen Gilliland <[EMAIL PROTECTED]> wrote:
I think this looks good and will be a give improvement over what we
currently have. Here's a few notes/comments ...
1. I don't understand what "Page models return collections of POJOs"
means. I don't consider that a requirement of a PageModel. I think a
PageModel can return any type of object it wants. Can you explain what
you meant by that? Aside from that I agree with your definition and
what models we will have.
You're right. Page models don't only return POJOs. I'll strike that.
2. I am a little uncertain about the whole website.pageModels and
configuring different blogs with different models. I think this idea
sounds good, but I am worried it's going to invite lots of problems,
particularly for sites that are upgrading. Older sites are going to
have a mix of old stuff on their site which needs to be supported, so
I'm not sure we can really offer a weblog by weblog config option to
alter the model. For example, what if someone upgrading from 2.x wants
to keep some older themes available, then we have a problem.
Yes. This is the tricky part. We need to make sure the options are
easy to understand, clearly defined and we pick good defaults.
My biggest concern with this is doing it on a weblog by weblog basis, if
we make this decision on a site-wide level then i think we will be in
better shape.
The idea in the current proposal is to support these options for upgraders:
1 - Turn on Atlas, completely turn off old model
site.macromodel="roller_3.0_only"
Velocity context loaded with only NEW model stuff
Create weblog page shows only NEW themes
This is the default for new installations
I think this is a worthy option and should be the default for new
installations starting with 3.0.
2 - Turn on Atlas, but continue to support old model
site.macromodel=roller_3.0
Velocity context loaded with either OLD or NEW model stuff, depending on
blog
Create weblog page shows NEW themes only
This is the default for upgrades
Again, doing this on a blog by blog basis has me *really* scared. I am
okay with this option as a site-wide option, but I see a lot of
potential problems doing it blog by blog.
3 - Ignore Atlas page models and macros entirely
You do this by leaving site.macromodel undefined (or setting to
roller_2.0_only?)
Velocity context loaded with only OLD model stuff
Create weblog page shows only OLD themes
ISSUE: Should we even support this option?
ISSUE: Should frontpage weblog will work in this scenario?
I don't think we need to support this option. IMO if we keep the 2 sets
of macros and models completely independent then we can include both of
them at the same time without problems. So I think upgrading systems
should continue to get the legacy context stuff, but new installations
don't.
One Roller site can support both classic and Atlas themes, but I
really want to avoid having themes that use a mix of classic and Atlas
macros. A weblog MUST pick which macro/model it will use. I was going
to use website.pageModels as a flag to indicate which macro/model, but
perhaps that is too confusing. Instead, we could force weblogs to
declare which version of macros should be used.
website.macrosVersion=2.0
To be honest, I don't think this is realistic. I agree that this is the
ideal way it would work, but I don't see it happening. We don't have
any real way of ensuring that upgraded installations don't do something
to make old template stuff available to weblogs that are set to use the
3.0 macros only.
For weblogs that use the 3.0 model, we'd use website.pageModels to
list all of the page models to be loaded (including the "main"
$pageModel):
website.pageModels - list of additional page models to add
I agree with this, but for upgraded sites I think these are just added
to the context in addition to the legacy stuff.
Does that make sense?
3. I think we need to have a little more discussion about the feed urls.
I didn't realize that the comment feeds needed to be available in both
rss and atom, because that causes a problem with the current url
proposal. If we are planning to offer each possible feed in multiple
flavors (rss, atom, etc) then we need to plan for that a bit better.
Best practice is to offer one feed format, so that's what I'd like
Roller to do by default. Each weblog has two feeds: one for entries
and one for comments. The feed format used by these feeds is
determined by the weblog's website.feedType field, which may be either
"atom_1.0" or "rss_2.0".
Hmm, this one has me a bit worried as well. Why is it a best practice
to only offer one type of feed format? Seems to me that only alienates
some users who can only support either rss or atom, but not both. I
prefer that we be able to support both feed types at the same time.
Anyone else know much about this? Should we offer both atom and rss
feeds for everything, or try and pick one or the other?
4. I'd like to suggest that the new WeblogPageModel have a different
name. $pageModel doesn't mean anything to ordinary users, so picking
something shorter and more descriptive would be good. I think the names
for $config, $site, and $planet are all good. All the other context
objects sound fine to me as well.
How about $page or $roller?
$page sounds pretty good.
-- Allen
5. I'm questioning if the WeblogPageModel should really provide methods
for accessing entry, bookmark, category, and page collections. My take
is that those things should be part of the Weblog pojo, so rather than
construct a model and methods for that we should just giving the user
access to their weblog and letting them use the pojo methods.
Yes. I like that better. I'll update the proposal.
6. You have a section called "Changes to existing POJOs" where you list
new methods to be added. This is the part I disagree with the most so
far. I don't think those methods should be added to our existing POJOs
because they are not related to the domain model in any way, they are
only related to the way we render our UI. I would much prefer to see
these methods be added to a class which extends the current pojos. This
also has the benefit that we don't need new methods for some things like
getTransformedText(), instead I think we should just override the
default pojos getText() method to return the transformed text.
Yes. I like the idea of extending the POJOs. I'll update the proposal.
7. I think the macros look pretty good except that I would prefer to see
the macros not have empty param lists unless they really don't need any
variables to do their work. I think that any variable that is needed to
do the work should be passed in (except for the utility classes). For
example, #showBookmarkList($weblog).
Agreed. I'll update the proposal.
8. I would also prefer that the #showWeblogEntryPager() macros *not*
display the next/prev links as part of the macro. I think that
functionality should be in a separate macro.
I had trouble separating them in the past, but I'll take another shot.
Overall I think this is a very solid approach. As you work on the
macros and page models I'd like to see them in the 3.0 branch so that I
can actually see the code itself. I think the more people we have
actually looking at that stuff the better so that we can try and spot
potential pitfalls and ways we can keep things as tidy as possible.
It's also more productive if we can watch and comment as progress is
being made.
Yes. I'd like to commit this work to the 3.0 branch.
- Dave
