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?).
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).
* 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
It seems like what we really want is the following change instead:
--- 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) {
+ return;
+ }
}
navigate(context, viewId);
Alternatively, we could modify navigate() to just "do nothing" if the passed
viewId is null.
Craig
