You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@roller.apache.org by David M Johnson <Da...@Sun.COM> on 2006/01/13 16:44:50 UTC
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Problem in this commit. Apparently, XDoclet doesn't play nice with
inheritance in Servlets. You've made PreviewServlet extend
PageServlet, which is not necessarily a bad thing, but XDoclet is
confused. It's generating a mapping for /page/* to PreviewServlet,
which breaks Roller -- so I'm gonna back this one out.
- Dave
On Jan 12, 2006, at 4:52 PM, agilliland@apache.org wrote:
> Author: agilliland
> Date: Thu Jan 12 13:52:46 2006
> New Revision: 368483
>
> URL: http://svn.apache.org/viewcvs?rev=368483&view=rev
> Log:
> make PreviewServlet extend PageServlet instead of BasePageServlet.
>
>
> Modified:
> incubator/roller/trunk/src/org/roller/presentation/velocity/
> PreviewServlet.java
>
> Modified: incubator/roller/trunk/src/org/roller/presentation/
> velocity/PreviewServlet.java
> URL: http://svn.apache.org/viewcvs/incubator/roller/trunk/src/org/
> roller/presentation/velocity/PreviewServlet.java?
> rev=368483&r1=368482&r2=368483&view=diff
> ======================================================================
> ========
> --- incubator/roller/trunk/src/org/roller/presentation/velocity/
> PreviewServlet.java (original)
> +++ incubator/roller/trunk/src/org/roller/presentation/velocity/
> PreviewServlet.java Thu Jan 12 13:52:46 2006
> @@ -30,7 +30,7 @@
> * @web.servlet-init-param name="properties" value="/WEB-INF/
> velocity.properties"
> * @web.servlet-mapping url-pattern="/preview/*"
> */
> -public class PreviewServlet extends BasePageServlet {
> +public class PreviewServlet extends PageServlet {
>
> private static Log mLogger = LogFactory.getLog
> (PreviewServlet.class);
>
>
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by David M Johnson <Da...@Sun.COM>.
On Jan 13, 2006, at 12:54 PM, Allen Gilliland wrote:
> I wanted to fix it because that doesn't make any sense. There is
> no real reason to have BasePageServlet as an abstract class and
> have PageServlet extend it. The current BasePageServlet *is* the
> PageServlet in reality, so the current object hierarchy is a bit
> broken and misleading. It should be ..
>
> PageServlet - /page/*
> PreviewServlet - /preview/*
>
> since the preview servlet is a true extension of the functionality
> of the PageServlet. I think little inaccuracies like this pile up
> and make the code less concise and ultimately harder to maintain,
> and that matters quite a bit to me.
>
> We already have xdoclet inserting servlet mappings from
> servlets.xml and servlet-mappings.xml file in the metadata/xdoclet
> directory. I would prefer to correct this and put the servlet
> mappings in there rather than leave things the way they are now.
Why don't you go ahead and do that. I don't have any problem with
that approach, but I'm not ready to ditch XDoclet for web.xml
entirely (yet).
- Dave
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by David M Johnson <Da...@Sun.COM>.
On Jan 13, 2006, at 12:54 PM, Allen Gilliland wrote:
> I wanted to fix it because that doesn't make any sense. There is
> no real reason to have BasePageServlet as an abstract class and
> have PageServlet extend it. The current BasePageServlet *is* the
> PageServlet in reality, so the current object hierarchy is a bit
> broken and misleading. It should be ..
>
> PageServlet - /page/*
> PreviewServlet - /preview/*
>
> since the preview servlet is a true extension of the functionality
> of the PageServlet. I think little inaccuracies like this pile up
> and make the code less concise and ultimately harder to maintain,
> and that matters quite a bit to me.
>
> We already have xdoclet inserting servlet mappings from
> servlets.xml and servlet-mappings.xml file in the metadata/xdoclet
> directory. I would prefer to correct this and put the servlet
> mappings in there rather than leave things the way they are now.
Why don't you go ahead and do that. I don't have any problem with
that approach, but I'm not ready to ditch XDoclet for web.xml
entirely (yet).
- Dave
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by Allen Gilliland <Al...@Sun.COM>.
On Fri, 2006-01-13 at 09:32, David M Johnson wrote:
> On Jan 13, 2006, at 11:44 AM, Allen Gilliland wrote:
>
> > Okay, I am going to gripe about this one a bit. I think it's
> > rather stupid that xdoclet is telling us how to write our code.
> > There is no reason why I shouldn't be able to extend a servlet
> > class and have both parent and child mapped as servlets. The
> > reason I made this change is because I would like to tidy up the
> > velocity servlets and rename the BasePageServlet to just
> > PageServlet, which makes more sense.
>
> That's a valid complaint. I'm pretty sure that the XDoclet folks
> would consider this to be a bug.
true, but that's not likely to happen any time soon.
>
> Why not do it like this (or something similar):
>
> PageServlet - no servlet mapping
> WeblogServlet - /page/*
> PreviewSevlet - /preview/*
> RssServlet - /rss/*
> AtomServlet - /atom/*
that's the same thing we have now.
BasePageServlet - no mapping
PageServlet - /page/* (yet there is no code in this class)
PreviewServlet - /preview/*
rss and atom are not actually servlets, they are just redundant mappings to the FlavorServlet.
I wanted to fix it because that doesn't make any sense. There is no real reason to have BasePageServlet as an abstract class and have PageServlet extend it. The current BasePageServlet *is* the PageServlet in reality, so the current object hierarchy is a bit broken and misleading. It should be ..
PageServlet - /page/*
PreviewServlet - /preview/*
since the preview servlet is a true extension of the functionality of the PageServlet. I think little inaccuracies like this pile up and make the code less concise and ultimately harder to maintain, and that matters quite a bit to me.
We already have xdoclet inserting servlet mappings from servlets.xml and servlet-mappings.xml file in the metadata/xdoclet directory. I would prefer to correct this and put the servlet mappings in there rather than leave things the way they are now.
-- Allen
>
> etc.
>
>
> - Dave
>
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by Allen Gilliland <Al...@Sun.COM>.
On Fri, 2006-01-13 at 09:32, David M Johnson wrote:
> On Jan 13, 2006, at 11:44 AM, Allen Gilliland wrote:
>
> > Okay, I am going to gripe about this one a bit. I think it's
> > rather stupid that xdoclet is telling us how to write our code.
> > There is no reason why I shouldn't be able to extend a servlet
> > class and have both parent and child mapped as servlets. The
> > reason I made this change is because I would like to tidy up the
> > velocity servlets and rename the BasePageServlet to just
> > PageServlet, which makes more sense.
>
> That's a valid complaint. I'm pretty sure that the XDoclet folks
> would consider this to be a bug.
true, but that's not likely to happen any time soon.
>
> Why not do it like this (or something similar):
>
> PageServlet - no servlet mapping
> WeblogServlet - /page/*
> PreviewSevlet - /preview/*
> RssServlet - /rss/*
> AtomServlet - /atom/*
that's the same thing we have now.
BasePageServlet - no mapping
PageServlet - /page/* (yet there is no code in this class)
PreviewServlet - /preview/*
rss and atom are not actually servlets, they are just redundant mappings to the FlavorServlet.
I wanted to fix it because that doesn't make any sense. There is no real reason to have BasePageServlet as an abstract class and have PageServlet extend it. The current BasePageServlet *is* the PageServlet in reality, so the current object hierarchy is a bit broken and misleading. It should be ..
PageServlet - /page/*
PreviewServlet - /preview/*
since the preview servlet is a true extension of the functionality of the PageServlet. I think little inaccuracies like this pile up and make the code less concise and ultimately harder to maintain, and that matters quite a bit to me.
We already have xdoclet inserting servlet mappings from servlets.xml and servlet-mappings.xml file in the metadata/xdoclet directory. I would prefer to correct this and put the servlet mappings in there rather than leave things the way they are now.
-- Allen
>
> etc.
>
>
> - Dave
>
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by David M Johnson <Da...@Sun.COM>.
On Jan 13, 2006, at 11:44 AM, Allen Gilliland wrote:
> Okay, I am going to gripe about this one a bit. I think it's
> rather stupid that xdoclet is telling us how to write our code.
> There is no reason why I shouldn't be able to extend a servlet
> class and have both parent and child mapped as servlets. The
> reason I made this change is because I would like to tidy up the
> velocity servlets and rename the BasePageServlet to just
> PageServlet, which makes more sense.
That's a valid complaint. I'm pretty sure that the XDoclet folks
would consider this to be a bug.
Why not do it like this (or something similar):
PageServlet - no servlet mapping
WeblogServlet - /page/*
PreviewSevlet - /preview/*
RssServlet - /rss/*
AtomServlet - /atom/*
etc.
- Dave
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by David M Johnson <Da...@Sun.COM>.
On Jan 13, 2006, at 11:44 AM, Allen Gilliland wrote:
> Okay, I am going to gripe about this one a bit. I think it's
> rather stupid that xdoclet is telling us how to write our code.
> There is no reason why I shouldn't be able to extend a servlet
> class and have both parent and child mapped as servlets. The
> reason I made this change is because I would like to tidy up the
> velocity servlets and rename the BasePageServlet to just
> PageServlet, which makes more sense.
That's a valid complaint. I'm pretty sure that the XDoclet folks
would consider this to be a bug.
Why not do it like this (or something similar):
PageServlet - no servlet mapping
WeblogServlet - /page/*
PreviewSevlet - /preview/*
RssServlet - /rss/*
AtomServlet - /atom/*
etc.
- Dave
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by Allen Gilliland <Al...@Sun.COM>.
Okay, I am going to gripe about this one a bit. I think it's rather stupid that xdoclet is telling us how to write our code. There is no reason why I shouldn't be able to extend a servlet class and have both parent and child mapped as servlets. The reason I made this change is because I would like to tidy up the velocity servlets and rename the BasePageServlet to just PageServlet, which makes more sense.
My feeling is that if xdoclet cannot do the job correctly then it shouldn't be used. I am fine with managing the servlet definitions manually if that's what it means.
-- Allen
On Fri, 2006-01-13 at 07:44, David M Johnson wrote:
> Problem in this commit. Apparently, XDoclet doesn't play nice with
> inheritance in Servlets. You've made PreviewServlet extend
> PageServlet, which is not necessarily a bad thing, but XDoclet is
> confused. It's generating a mapping for /page/* to PreviewServlet,
> which breaks Roller -- so I'm gonna back this one out.
>
> - Dave
>
>
> On Jan 12, 2006, at 4:52 PM, agilliland@apache.org wrote:
>
> > Author: agilliland
> > Date: Thu Jan 12 13:52:46 2006
> > New Revision: 368483
> >
> > URL: http://svn.apache.org/viewcvs?rev=368483&view=rev
> > Log:
> > make PreviewServlet extend PageServlet instead of BasePageServlet.
> >
> >
> > Modified:
> > incubator/roller/trunk/src/org/roller/presentation/velocity/
> > PreviewServlet.java
> >
> > Modified: incubator/roller/trunk/src/org/roller/presentation/
> > velocity/PreviewServlet.java
> > URL: http://svn.apache.org/viewcvs/incubator/roller/trunk/src/org/
> > roller/presentation/velocity/PreviewServlet.java?
> > rev=368483&r1=368482&r2=368483&view=diff
> > ======================================================================
> > ========
> > --- incubator/roller/trunk/src/org/roller/presentation/velocity/
> > PreviewServlet.java (original)
> > +++ incubator/roller/trunk/src/org/roller/presentation/velocity/
> > PreviewServlet.java Thu Jan 12 13:52:46 2006
> > @@ -30,7 +30,7 @@
> > * @web.servlet-init-param name="properties" value="/WEB-INF/
> > velocity.properties"
> > * @web.servlet-mapping url-pattern="/preview/*"
> > */
> > -public class PreviewServlet extends BasePageServlet {
> > +public class PreviewServlet extends PageServlet {
> >
> > private static Log mLogger = LogFactory.getLog
> > (PreviewServlet.class);
> >
> >
>
Re: svn commit: r368483 -
/incubator/roller/trunk/src/org/roller/presentation/velocity/PreviewServlet.java
Posted by Allen Gilliland <Al...@Sun.COM>.
Okay, I am going to gripe about this one a bit. I think it's rather stupid that xdoclet is telling us how to write our code. There is no reason why I shouldn't be able to extend a servlet class and have both parent and child mapped as servlets. The reason I made this change is because I would like to tidy up the velocity servlets and rename the BasePageServlet to just PageServlet, which makes more sense.
My feeling is that if xdoclet cannot do the job correctly then it shouldn't be used. I am fine with managing the servlet definitions manually if that's what it means.
-- Allen
On Fri, 2006-01-13 at 07:44, David M Johnson wrote:
> Problem in this commit. Apparently, XDoclet doesn't play nice with
> inheritance in Servlets. You've made PreviewServlet extend
> PageServlet, which is not necessarily a bad thing, but XDoclet is
> confused. It's generating a mapping for /page/* to PreviewServlet,
> which breaks Roller -- so I'm gonna back this one out.
>
> - Dave
>
>
> On Jan 12, 2006, at 4:52 PM, agilliland@apache.org wrote:
>
> > Author: agilliland
> > Date: Thu Jan 12 13:52:46 2006
> > New Revision: 368483
> >
> > URL: http://svn.apache.org/viewcvs?rev=368483&view=rev
> > Log:
> > make PreviewServlet extend PageServlet instead of BasePageServlet.
> >
> >
> > Modified:
> > incubator/roller/trunk/src/org/roller/presentation/velocity/
> > PreviewServlet.java
> >
> > Modified: incubator/roller/trunk/src/org/roller/presentation/
> > velocity/PreviewServlet.java
> > URL: http://svn.apache.org/viewcvs/incubator/roller/trunk/src/org/
> > roller/presentation/velocity/PreviewServlet.java?
> > rev=368483&r1=368482&r2=368483&view=diff
> > ======================================================================
> > ========
> > --- incubator/roller/trunk/src/org/roller/presentation/velocity/
> > PreviewServlet.java (original)
> > +++ incubator/roller/trunk/src/org/roller/presentation/velocity/
> > PreviewServlet.java Thu Jan 12 13:52:46 2006
> > @@ -30,7 +30,7 @@
> > * @web.servlet-init-param name="properties" value="/WEB-INF/
> > velocity.properties"
> > * @web.servlet-mapping url-pattern="/preview/*"
> > */
> > -public class PreviewServlet extends BasePageServlet {
> > +public class PreviewServlet extends PageServlet {
> >
> > private static Log mLogger = LogFactory.getLog
> > (PreviewServlet.class);
> >
> >
>