Hi Simon,
I like your recommendations below and I have a few suggestions.
1. I have seen use cases where a model can be non-sequential, iow, stops
in the train can appear in a random order and all are enabled the first
time the processTrain comes up. In such cases getNextStep() and
getPreviousStep() may not be relevant. So perhaps the current
ProcessModel should be renamed to SequentialProcessModel (or
OrderedProcessModel something similar).
2. Suggest renaming isPreviousStepAvailable() and isNextStepAvailable()
to hasPreviousStep() and hasNextStep(). May also be useful to have a
'isCurrentStep()' method.
3. The fact that you have methods getNextStep() and getPreviousStep(),
implies we can figure out if a previous step is visited, disabled etc.
by doing the following
if (hasPreviousStep())
{
if (getPreviousStep().isVisited())
...
}
4. MaxPathKeyProcessModel could be renamed to MaxVisitedProcessModel
although like Adam recommends this could be part of the ProcessModel (or
the OrderedProcessModel) class itself.
5. Ideally we want some of these methods to be part of a
'ProcessStepModel' because whether a step is visited or disabled or
active etc. pertains to a step and not to the train. Although the
immediate property of a step can be at the ProcessModel level or at the
step level.
Thanks
Pavitra
On 8/13/2007 12:56 PM PST, Simon Lessard wrote:
Yeah the maxPath part could be just left in ProcessMenuModel.
Previous/Next is not for train component, but rather for page
templates using both a train and previous/next step buttons. The only
problem I have with placing those in a sub class is reuse with
multiple implementations of ProcessModel. However, while thinking
about it, it could be possible using decorator pattern in a generic
way to add previous/next support to an existing model. Therefore, I
don't think I'll add those.
Regards,
~ Simon
On 8/13/07, *Adam Winer* <[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]>>
wrote:
D'oh. :) Quite true. Could we also eliminate the "MaxPath" model
part,
and just merge that into ProcessMenuModel?
W/regard to the previous/next: in theory, the train could run
entirely off of those, by iterating both forward and backward.
But that's not necessary. Anyway, I'd be OK (I think) with
seeing those in the base model, since they are quite generically
useful, but I haven't thought it through in detail or tried applying
it to the existing component set.
-- Adam
On 8/13/07, Simon Lessard <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
> The real issue imho is more with the methods linked with
previous/next step
> management. I guess those could be easily placed in the
subclass though as
> the train itself won't use them.
>
> public abstract boolean isNextStepAvailable(); /* Could be
optional or
> maybe in a subclass, this would check if there's a step before
the current
> one */
> public abstract boolean isPreviousStepAvailable(); /* As above */
> public abstract Object getNextStep(); /* As above */
> public abstract Object getPreviousStep(); /* As above */
> Regards,
>
> ~ Simon
>
>
> On 8/13/07, Simon Lessard <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
> > Hmmm, isn't that what I suggested? Current class is
ProcessMenuModel, the
> new one does not include the "Menu" part.
> >
> >
> >
> > On 8/13/07, Adam Winer < [EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
> > > So... what if we left the current class alone, in terms of
its API and
> > > name, and just exposed a new base class?
> > >
> > > -- Adam
> > >
> > >
> > > On 8/13/07, Adam Winer <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]> > wrote:
> > > > I've got some concern for backwards compatibility -
this'll break
> > > > some code on our end. I'm hoping Jeanne will have some
comments
> > > > too.
> > > >
> > > > -- Adam
> > > >
> > > >
> > > > On 8/13/07, Martin Marinschek <
[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]> >
wrote:
> > > > > No voice means you can go ahead ;)
> > > > >
> > > > > regards,
> > > > >
> > > > > Martin
> > > > >
> > > > > On 8/13/07, Danny Robinson < [EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
> > > > > > Sorry Simon, I have little/no experience with this part of
> Trinidad so can't
> > > > > > comment. I trust your judgement, so you have my vote
if you need
> it ;-)
> > > > > >
> > > > > >
> > > > > > On 8/13/07, Simon Lessard <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]> > wrote:
> > > > > > > So I assume it would be +0 for everyone?
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 8/10/07, Simon Lessard <
[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]>> wrote:
> > > > > > > > Hello everybody,
> > > > > > > >
> > > > > > > > Currently Trinidad includes a ProcessMenuModel
class that
> contains
> > > > > > undesirable methods. The complete method list (not
including
> inherited ones)
> > > > > > is:
> > > > > > > >
> > > > > > > >
> > > > > > > > public boolean isImmediate()
> > > > > > > > public boolean isReadOnly()
> > > > > > > > public boolean isVisited()
> > > > > > > > public void clearMaxPath()
> > > > > > > > public void setMaxPathKey(Object maxPathKey)
> > > > > > > > public Object getMaxPathKey()The methods involving
maxPathKey
> are the
> > > > > > ones annoying me the most. However, as it's part of
the public API
> we have
> > > > > > to keep backward compatibility as much as possible.
Note also that
> > > > > > isReadOnly should not named that way as readOnly
support was
> removed from
> > > > > > process classes in favor of disabled since a readOnly
link did not
> make much
> > > > > > sense.
> > > > > > > >
> > > > > > > > Anyway, I would rather have the following class
structure and
> method
> > > > > > signatures:
> > > > > > > >
> > > > > > > > public abstract class ProcessModel
> > > > > > > >
> > > > > > > >
> > > > > > > > public abstract boolean isDisabled();
> > > > > > > > public abstract boolean isImmediate();
> > > > > > > > public abstract boolean isVisited();
> > > > > > > > public abstract boolean isNextStepAvailable(); /*
Could be
> optional or
> > > > > > maybe in a subclass, this would check if there's a
step before the
> current
> > > > > > one */
> > > > > > > > public abstract boolean isPreviousStepAvailable();
/* As above
> */
> > > > > > > > public abstract Object getNextStep(); /* As above */
> > > > > > > > public abstract Object getPreviousStep(); /* As
above */public
> class
> > > > > > MaxPathKeyProcessModel extends ProcessModel /* Or a
better name */
> > > > > > > >
> > > > > > > >
> > > > > > > > Implements all methods using the old ProcessMenuModel
> [EMAIL PROTECTED]
> > > > > > > > public class ProcessMenuModel extends
MaxPathKeyProcessModel
> > > > > > > >
> > > > > > > >
> > > > > > > > Empty class except for isReadOnly() that should return
> > > > > > super.isDisabled ()
> > > > > > > > The structure above would clean up the Model class
that really
> shouldn't
> > > > > > contain very implementation specific code like the max
path key
> algorithm
> > > > > > and would allow us to add new ProcessModel classes
with more
> > > > > > functionalities. For example I had one using a mode
for step
> access right
> > > > > > like: MAX_PLUS_NEXT, MAX, ANY, NEXT_ONLY, etc.
> > > > > > > >
> > > > > > > > The previous/next step methods could be useful for
page
> templates since
> > > > > > it would be possible to include the train in the
header as well as
> a
> > > > > > previous and next step buttons in the page footer in a
generic way
> using the
> > > > > > very same process model. Note that we might have to
also include
> methods
> > > > > > like isPreviousVisited(), isPreviousDisabled() and
such to fully
> support
> > > > > > that.
> > > > > > > >
> > > > > > > > Opinions, suggestions?
> > > > > > > >
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > >
> > > > > > > > ~ Simon
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Chordiant Software Inc.
> > > > > > www.chordiant.com <http://www.chordiant.com>
> > > > >
> > > > >
> > > > > --
> > > > >
> > > > > http://www.irian.at
> > > > >
> > > > > Your JSF powerhouse -
> > > > > JSF Consulting, Development and
> > > > > Courses in English and German
> > > > >
> > > > > Professional Support for Apache MyFaces
> > > > >
> > > >
> > >
> >
> >
>
>
