You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jspwiki.apache.org by Harry Metske <ha...@gmail.com> on 2009/05/11 19:43:55 UTC
Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
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 <ja...@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
> );
>
>
>
Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Posted by Harry Metske <ha...@gmail.com>.
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 <an...@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 <ha...@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 <an...@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 <ha...@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 <ja...@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
> >> >> );
> >> >>
> >> >>
> >> >>
> >> >
> >>
> >
>
Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Posted by Andrew Jaquith <an...@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 <ha...@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 <an...@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 <ha...@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 <ja...@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
>> >> );
>> >>
>> >>
>> >>
>> >
>>
>
Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Posted by Janne Jalkanen <ja...@ecyrd.com>.
A static method in BreadcrumbsTag gets my +1...
/Janne
On 12 May 2009, at 20:39, Harry Metske 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 <an...@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
>> <ha...@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 <ja...@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
>>>> );
>>>>
>>>>
>>>>
>>>
>>
Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Posted by Harry Metske <ha...@gmail.com>.
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 <an...@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 <ha...@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 <ja...@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
> >> );
> >>
> >>
> >>
> >
>
Re: svn commit: r773375 - /incubator/jspwiki/trunk/src/java/org/apache/wiki/action/AbstractPageActionBean.java
Posted by Andrew Jaquith <an...@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 <ha...@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 <ja...@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
>> );
>>
>>
>>
>