You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shale.apache.org by sc...@apache.org on 2006/09/05 03:45:12 UTC
svn commit: r440216 -
/shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
Author: schof
Date: Mon Sep 4 18:45:12 2006
New Revision: 440216
URL: http://svn.apache.org/viewvc?view=rev&rev=440216
Log:
LegacyDialogContext should not throw an exception when the transition cannot be found during the advance method. Instead return a null view id and let the wrapped NavigationHandler try to recover. (SHALE-276)
Modified:
shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
Modified: shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
URL: http://svn.apache.org/viewvc/shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java?view=diff&rev=440216&r1=440215&r2=440216
==============================================================================
--- shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java (original)
+++ shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java Mon Sep 4 18:45:12 2006
@@ -219,7 +219,15 @@
public String advance(FacesContext context, String outcome) {
Position position = peek();
- transition(position, outcome);
+
+ try {
+ transition(position, outcome);
+ } catch (IllegalStateException ie) {
+ // The transition method didn't like our outcome but its possible we can
+ // recover if the outcome was intended for a different NavigationHandler
+ return null;
+ }
+
State state = position.getState();
while (true) {
if (state instanceof ActionState) {
Re: svn commit: r440216 - /shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
Posted by Craig McClanahan <cr...@apache.org>.
On 9/5/06, Sean Schofield <se...@gmail.com> wrote:
>
> > * Once you are inside a dialog, the dialog should be the *only* thing
> that
> > manages navigation.
> > There should be *no such thing* as "outcomes not managed by this
> dialog"
> > -- any other approach
> > leads to totally non-deterministic behavior that quickly becomes
> > impossible to debug. Based
> > on this, delegating to the wrapped navigation handler (while inside a
> > dialog) seems to me like an
> > exceedingly BAD idea.
>
> What happens if the dialog is a popup and the user closes it? We now
> have an ongoing dialog associated with the view id. I don't think
> you'd want the navigation on the underlying page to stop working just
> because the user aborted the dialog.
With the new implementation, the popup will run its own dialog instance,
right? That means that whatever happens there will have no effect on the
flow of the main page.
> * If the advance() method returns null, that should mean the same thing
> that
> > an action method
> > returning null means -- redisplay the current view. Otherwise, you
> are
> > going to destroy the
> > ability of a view developer to not worry about whether the view being
> > maintained is used within
> > or outside a dialog (or both). Note that this is going to happen to
> us by
> > default, if any validation
> > errors occur, so the framework MUST respect this approach.
>
> Null outcomes *still* behave this way. When there is a null view id
> from the advance method the DialogNavigationHandler will pass the
> original outcome (null) to the wrapped NavigationHandler which will
> perform as expected.
Good.
> Barring further analysis tomorrow, I'm at the moment -1 on this change.
>
> I'm interested in hearing how you would solve my usecase then. I have
> more usecases if the one above does not convince you.
I think the new implementation deals with this use case ... if so, let's
review the others as well to make sure it works as needed.
> Craig
>
> Sean
>
Craig
Re: svn commit: r440216 - /shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
Posted by Sean Schofield <se...@gmail.com>.
> * Once you are inside a dialog, the dialog should be the *only* thing that
> manages navigation.
> There should be *no such thing* as "outcomes not managed by this dialog"
> -- any other approach
> leads to totally non-deterministic behavior that quickly becomes
> impossible to debug. Based
> on this, delegating to the wrapped navigation handler (while inside a
> dialog) seems to me like an
> exceedingly BAD idea.
What happens if the dialog is a popup and the user closes it? We now
have an ongoing dialog associated with the view id. I don't think
you'd want the navigation on the underlying page to stop working just
because the user aborted the dialog.
> * If the advance() method returns null, that should mean the same thing that
> an action method
> returning null means -- redisplay the current view. Otherwise, you are
> going to destroy the
> ability of a view developer to not worry about whether the view being
> maintained is used within
> or outside a dialog (or both). Note that this is going to happen to us by
> default, if any validation
> errors occur, so the framework MUST respect this approach.
Null outcomes *still* behave this way. When there is a null view id
from the advance method the DialogNavigationHandler will pass the
original outcome (null) to the wrapped NavigationHandler which will
perform as expected.
> Barring further analysis tomorrow, I'm at the moment -1 on this change.
I'm interested in hearing how you would solve my usecase then. I have
more usecases if the one above does not convince you.
> Craig
Sean
Re: svn commit: r440216 - /shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
Posted by Rahul Akolkar <ra...@gmail.com>.
On 9/5/06, Craig McClanahan <cr...@apache.org> wrote:
> On 9/4/06, schof@apache.org <sc...@apache.org> wrote:
> >
> > Author: schof
> > Date: Mon Sep 4 18:45:12 2006
> > New Revision: 440216
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=440216
> > Log:
> > LegacyDialogContext should not throw an exception when the transition
> > cannot be found during the advance method. Instead return a null view id
> > and let the wrapped NavigationHandler try to recover. (SHALE-276)
>
>
> I'm going to need to look at this in more detail (probably not until
> tomorrow night), but I do not believe in the stated premises behind this
> change (and am therefore pretty unlikely to agree with the implemented
> solution). Instead, I believe:
>
> * Once you are inside a dialog, the dialog should be the *only* thing that
> manages navigation.
> There should be *no such thing* as "outcomes not managed by this dialog"
> -- any other approach
> leads to totally non-deterministic behavior that quickly becomes
> impossible to debug. Based
> on this, delegating to the wrapped navigation handler (while inside a
> dialog) seems to me like an
> exceedingly BAD idea.
>
<snip/>
(I've read the rest of the thread).
I think delegation is useful, and pretty much necessary for null
outcomes such as those in SHALE-10 [1]. The mantra if we don't
understand it, we must delegate it -- what I call inconsequential
triggers (inconsequential to the progress of the dialog, that is) --
may also have certain advantages, for example, header & footer style
markup that is disconnected from the dialog. IIRC, one of the usecases
on the user list was running a dialog where each participating view
has a "global navigation" menubar on the side, which seems like a
routine thing to want.
However, as stated above, such delegation adds another dimension to
the authoring, especially for cases such as wrapped handlers having
navigation rules involving blanket patterns etc. where things just got
a bit trickier. Since dialogs can't (yet) afford to have the view
changed from underneath them, it probably means there is some gray
area regarding the ownership of suspension (if there is such a scheme)
or cancellation of the dialog if and when such a departure occurs.
-Rahul
[1] http://issues.apache.org/struts/browse/SHALE-10
Re: svn commit: r440216 - /shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
Posted by Craig McClanahan <cr...@apache.org>.
On 9/4/06, schof@apache.org <sc...@apache.org> wrote:
>
> Author: schof
> Date: Mon Sep 4 18:45:12 2006
> New Revision: 440216
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=440216
> Log:
> LegacyDialogContext should not throw an exception when the transition
> cannot be found during the advance method. Instead return a null view id
> and let the wrapped NavigationHandler try to recover. (SHALE-276)
I'm going to need to look at this in more detail (probably not until
tomorrow night), but I do not believe in the stated premises behind this
change (and am therefore pretty unlikely to agree with the implemented
solution). Instead, I believe:
* Once you are inside a dialog, the dialog should be the *only* thing that
manages navigation.
There should be *no such thing* as "outcomes not managed by this dialog"
-- any other approach
leads to totally non-deterministic behavior that quickly becomes
impossible to debug. Based
on this, delegating to the wrapped navigation handler (while inside a
dialog) seems to me like an
exceedingly BAD idea.
* If the advance() method returns null, that should mean the same thing that
an action method
returning null means -- redisplay the current view. Otherwise, you are
going to destroy the
ability of a view developer to not worry about whether the view being
maintained is used within
or outside a dialog (or both). Note that this is going to happen to us by
default, if any validation
errors occur, so the framework MUST respect this approach.
Barring further analysis tomorrow, I'm at the moment -1 on this change.
Craig
Modified:
>
> shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
>
> Modified:
> shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
> URL:
> http://svn.apache.org/viewvc/shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java?view=diff&rev=440216&r1=440215&r2=440216
>
> ==============================================================================
> ---
> shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
> (original)
> +++
> shale/sandbox/shale-dialog2-legacy/src/main/java/org/apache/shale/dialog2/legacy/LegacyDialogContext.java
> Mon Sep 4 18:45:12 2006
> @@ -219,7 +219,15 @@
> public String advance(FacesContext context, String outcome) {
>
> Position position = peek();
> - transition(position, outcome);
> +
> + try {
> + transition(position, outcome);
> + } catch (IllegalStateException ie) {
> + // The transition method didn't like our outcome but its
> possible we can
> + // recover if the outcome was intended for a different
> NavigationHandler
> + return null;
> + }
> +
> State state = position.getState();
> while (true) {
> if (state instanceof ActionState) {
>
>
>