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