Hello Pavitra,
It was a long time since I last heard from you :). See my comments below.
Regards,
~ Simon
On 8/13/07, Pavitra Subramaniam <[EMAIL PROTECTED]> wrote:
>
> 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).
True, hence should not be in base ProcessModel as Adam said. We can provide
a decorator to wrap process model to add previous/next step support. That
wrapper could be named SequentialProcessModel or OrderedProcessModel
2. Suggest renaming isPreviousStepAvailable() and isNextStepAvailable()
> to hasPreviousStep() and hasNextStep().
Sounds better, but alas not EL compatible, so I would prefer my original
suggestion.
May also be useful to have a
> 'isCurrentStep()' method.
>
That method would return something like
getRowKey().equals(getFocusRowKey())?
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())
> ...
> }
Yep, could be useful for a page template with back/forward buttons.
4. MaxPathKeyProcessModel could be renamed to MaxVisitedProcessModel
> although like Adam recommends this could be part of the ProcessModel (or
> the OrderedProcessModel) class itself.
Adam wanted to keep it in ProcessMenuModel (where it's right now). The only
reason why I would like to place that code in another class is to make it
more obvious for users. I do like the MaxVisitedProcessModel name.
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.
That part is more controversial. Currently I'm not aware of any model class
that force the type of its content. I agree that semantically this should be
the responsibility of the the step though. I don't know about immediate, I
would say step level as well. I wouldn't mind creating a ProcessStep class
though, but I don't know if it should be ProcessStepModel as Model classes
normally represent a collection in JSF and this could confuse new users.
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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> >
> >
>
