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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > > > >> 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 <[email protected]> > >> 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 <[email protected]> > >> > > >> >> 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 > >> >> ); > >> >> > >> >> > >> >> > >> > > >> > > >
