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.
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.
* 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.
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
