Craig,
I'm a little confused. What is the difference between returning null
and delegating to the wrapped navigation handler, which in the case of
the default implementation, will also return null? I guess in most
cases I can think of the dialog as the only thing that should be
handling navigation while the dialog is in progress. It seems to
violate the principals of delegation though. You are not giving
anyone else a chance to deal with the outcome. I will try to think of
another usecase that might convince you.
For now I guess I'm cool with you reverting it (we can always add it
back later.)
Sean
On 9/10/06, Craig McClanahan <[EMAIL PROTECTED]> wrote:
On 9/10/06, Rahul Akolkar <[EMAIL PROTECTED]> wrote:
>
> On 9/9/06, Craig McClanahan <[EMAIL PROTECTED]> wrote:
> > Sean's changes fo the dialog2 sandbox implementation in r440205 includes
> the
> > following change that I object to:
> >
> > ---
>
shale/sandbox/shale-dialog2/src/main/java/org/apache/shale/dialog2/faces/Dialog2NavigationHandler.java
> > (original)
> > +++
> >
>
shale/sandbox/shale-dialog2/src/main/java/org/apache/shale/dialog2/faces/Dialog2NavigationHandler.java
> > Mon Sep 4 16:52:55 2006
> > @@ -135,6 +135,9 @@
> > + context + "' with navigation to viewId '"
> > + viewId + "'");
> > }
> > + if (viewId == null) {
> > + original.handleNavigation(context, fromAction,
> outcome);
> > + }
> > }
> > navigate(context, viewId);
> >
> >
> > My objection is on two grounds:
> >
> > * Philosophically, navigation within a dialog should be self contained,
> > and attempts to navigate via an outcome not defined by the dialog
> > definition should be treated as an error condition. Otherwise, we're
> > going to find ourselves in situations when the actual navigation being
> > performed is non-deterministic ... it's based on what (if any) dialogs
> > called me as a subdialog, so I have to do a lot more searching to
> > find problems. It also makes static analysis of an application to
> > determine
> > whether all its outcome values are legal virtually impossible.
> >
> <snip/>
>
> I'm in agreement with the above, and we cannot undermine the benefits
> of static analysis -- we need that end-to-end approach from modeling
> to generation to testing.
>
> As a sidebar, there are practical considerations (such as global
> navbars) that we should perhaps explore via sample dialogs in the
> sandbox, and attempt to establish a set of best practice guidelines
> for the same (such as mapping those to some end states?).
I've got a big chunk of time reserved this afternoon to focuson dialog2, and
examples are definitely on the list of things I want to work on. Besides
menus, another really common usecase I'd like to see us cover gracefully is
a popup window (which starts its own dialog instance, per our current
approach) that can gracefuly "pull" data from the dialog of the main page at
startup, and "push" data back again when the popup dialog finishes.
> Although this particular change only delegates to the original
> > navigation handler if the last state executed returned null, the
> > stated justification for this action ("delegate when we don't know
> > what the response means") does not match the reality here. In
> > particular, a null return from a state is a *deliberate* decision by
> > the dialog to stay on the current view id. That decision should be
> > respected.
> >
> <snap/>
>
> I've always been seeing that change in light of Sean's earlier commit
> r440216 [1]. This means that the legacy implementation returns null in
> two cases, deliberately or on IllegalStateException in transition()
> (which is masked into a null return).
The latter sounds like the wrong approach -- to be consistent with a "self
contained" paradigm for dialog definitions, it should really throw the
exception. Among the details to be addressed is documentation, and a
tightening up on the Javadoc contracts about exceptions and return values.
> * Technically, this particular change doesn't look like it will accomplish
> > anything anyway, because it is not followed by a "return" statement.
> > Instead, control will fall through to the call to navigate() anyway.
> >
> <snip/>
>
> Indeed.
>
> -Rahul
>
> [1] http://svn.apache.org/viewvc?view=rev&revision=440216
Craig
