incubator-jspwiki-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Harry Metske <harry.met...@gmail.com>
Subject Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Date Tue, 12 May 2009 18:46:10 GMT
hmm, I hadn't considered that scenario yet.
I now understand and agree with your point.
I will file a low-prio JIRA for that.


regards,
Harry

2009/5/12 Andrew Jaquith <andrew.r.jaquith@gmail.com>

> Sorry, I was really addressing two issues here, and I should have
> addressed one of them.
>
> First, there's the issue of where to stick the code that
> gets/stores/modifies breadcrumbs. AbstractPageActionBean is a poor
> place for it. Keeping ALL of the processing in BreadcrumbsTag is fine
> by me. In fact, there's a comment in AbstrctPageActionBean suggesting
> it should all go in BreadcrumbsTag. +1 for that.
>
> The second issue is whether we ought to model the breadcrumbs object
> itself, rather than just have it be a bag of Strings. At the moment,
> it's a little flawed -- the code that handles page-delete events only
> works for cases where there's just one user active. But pages deleted
> by one user aren't reflected in the breadcrumbs of another. This isn't
> a big deal, really, but it might be something we should plan to
> address at some point. That's what prompted the comment about having
> an Breadcrumbs class. Definitely not a 3.0 item. :)
>
> On Tue, May 12, 2009 at 1:39 PM, Harry Metske <harry.metske@gmail.com>
> wrote:
> > well, the real information for the BreadcrumbsTrail is currently in a
> > (breadCrumbTrail) String attribute in the HttpSession.
> > Isn't wrapping this string in a Serializable Breadcrumb class a bit
> overdone
> > ?
> > It will increase the Session size, and will not boost
> > performance/scalability either.
> >
> > So, I like to keep it in BreadcrumbsTag for now if you don't mind.
> >
> > regards,
> > Harry
> >
> > 2009/5/11 Andrew Jaquith <andrew.r.jaquith@gmail.com>
> >
> >> PS. This breadcrumb code is absolutely NOT meant to sit in the
> >> AbstractPageActionBean class. It should really be put somewhere else.
> >> Actually, I think what we should really do is create a Breadcrumbs
> >> class and have it made available as a property of WikiSession.
> >>
> >> On Mon, May 11, 2009 at 1:43 PM, Harry Metske <harry.metske@gmail.com>
> >> wrote:
> >> > yup, I'll fix that tomorrow too.
> >> >
> >> > I will also switch to the commit-then-review approach from now on.
> >> >
> >> > regards,
> >> > Harry
> >> >
> >> >
> >> >
> >> > 2009/5/10 <jalkanen@apache.org>
> >> >
> >> >> Author: jalkanen
> >> >> Date: Sun May 10 18:14:07 2009
> >> >> New Revision: 773375
> >> >>
> >> >> URL: http://svn.apache.org/viewvc?rev=773375&view=rev
> >> >> Log:
> >> >> Added a FIXME
> >> >>
> >> >> Modified:
> >> >>
> >> >>
> >>
>  incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
> >> >>
> >> >> Modified:
> >> >>
> >>
> incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
> >> >> URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java?rev=773375&r1=773374&r2=773375&view=diff
> >> >>
> >> >>
> >>
> ==============================================================================
> >> >> ---
> >> >>
> >>
> incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
> >> >> (original)
> >> >> +++
> >> >>
> >>
> incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
> >> >> Sun May 10 18:14:07 2009
> >> >> @@ -74,6 +74,7 @@
> >> >>      *
> >> >>      * @param pageName the pageName to be removed from the breadcrumb
> >> >>      */
> >> >> +    // FIXME: Is this in the right place? Shouldn't this be a static
> >> >> method in BreadcrumbsTag?
> >> >>     void deleteFromBreadCrumb( String pageName )
> >> >>     {
> >> >>         HttpSession session = getContext().getRequest().getSession(
> >> false
> >> >> );
> >> >>
> >> >>
> >> >>
> >> >
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message