incubator-jspwiki-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Jaquith <andrew.r.jaqu...@gmail.com>
Subject Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Date Tue, 12 May 2009 18:40:24 GMT
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
View raw message