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 >> >> ); >> >> >> >> >> >> >> > >> >
