Introduces RunAsyncActivity, a simple wrapper to turn any activity (issue1386806)

[rj...@google.com]

Reviewers: jat,

Description:
Introduces RunAsyncActivity, a simple wrapper to turn any activity
into a code split point.

Please review this at http://gwt-code-reviews.appspot.com/1386806/

Affected files:
M eclipse/user/.classpath
M user/build.xml
M user/src/com/google/gwt/activity/Activity.gwt.xml
A user/src/com/google/gwt/activity/client/RunAsyncActivity.java
M user/src/com/google/gwt/core/client/AsyncProvider.java
A user/src/javax/inject/Inject.gwt.xml
A user/test/com/google/gwt/activity/ActivitySuite.gwt.xml
M user/test/com/google/gwt/activity/ActivitySuite.java
A user/test/com/google/gwt/activity/client/RunAsyncActivityTest.java

[t.br...@gmail.com]

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java
File user/src/com/google/gwt/activity/client/RunAsyncActivity.java
(right):

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java#newcode66
user/src/com/google/gwt/activity/client/RunAsyncActivity.java:66:
GWT.runAsync(new RunAsyncCallback() {
I'm not used to GWT.runAsync but won't this create a single split-point
for all provided activities, rather than a split-point for each of them
(as it would be the case with AsyncProvider)?
If that's the case, I'd rather remove that overload and mandate an
AsyncProvider.

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java#newcode87
user/src/com/google/gwt/activity/client/RunAsyncActivity.java:87:
wrapped.start(panel, eventBus);
What if the activity is started, cancelled before it loads, and started
again?
As coded here, it'll start twice. I guess GWT.runAsync takes care of the
ongoing script load and doesn't try to load it twice; but that's not
enough here, because the "cancel" call will be lost, and actually, the
wrapped activity from the first onSuccess will be lost in "started"
state.
Moreover, AsyncProvider might return a new Activity instance each time
it's called, which makes it impossible to control the actually Activity
life-time from the ActivityMapper (you might return the same
RunAsyncActivity instance, but it doesn't mean you'll have a single
instance of the wrapped activity).

I once started coding such an AsyncActivityWrapper but never actually
tested it. I stored the panel and eventBus in fields and only
async-loaded the activity once to make sure the callback is only called
once; and I didn't attempt to async-load the activity if it was already
loaded, or loading. Something like:
if (loaded()) {
wrapped.start(panel,eventBus);
} else {
this.panel = panel;
this.eventBus = eventBus;
if (!loading()) { // only ever trigger a single async load
asyncProvider.get(...
public void onSuccess(Activity result) {
wrapped = result;
wrapped.start(RunAsyncActivity.this.panel,
RunAsyncActivity.this.eventBus);
}
...
});
loading = true;
}

As I said, I never actually tested my code, so maybe it's not worth
it...

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java#newcode100
user/src/com/google/gwt/activity/client/RunAsyncActivity.java:100:
wrapped.onCancel();
How about an 'else { cancelled = true; }' and in the onSuccess then only
call the activity's start() if !cancelled?

The same would have to be done in onStop() too (even though it shouldn't
ever happen, because the activity shouldn't stopped if it hasn't yet
"fully started" by calling the AcceptsOneWidget back)

(either a new boolean field or a specific value assigned to wrapped)

As coded here, if you cancel the activity before it's loaded, the
onSuccess will start it anyway and the onCancel will be lost.

http://gwt-code-reviews.appspot.com/1386806/

[rj...@google.com]

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java
File user/src/com/google/gwt/activity/client/RunAsyncActivity.java
(right):

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java#newcode66
user/src/com/google/gwt/activity/client/RunAsyncActivity.java:66:
GWT.runAsync(new RunAsyncCallback() {

Well hell, I think you're right.

I started on this patch because Antoine's
http://gwt-code-reviews.appspot.com/1383802/ seemed over-complex to me,
but I begin to see the issues he was facing.

Antoine, wouldn't your AbstractAsyncActivity have the same issue? I'm
not sure there is any way to automatically make split points without
resorting to code generation, probably via AsyncProxy

On 2011/03/25 01:53:05, tbroyer wrote:
> I'm not used to GWT.runAsync but won't this create a single
split-point for all
> provided activities, rather than a split-point for each of them (as it
would be
> the case with AsyncProvider)?
> If that's the case, I'd rather remove that overload and mandate an
> AsyncProvider.

http://gwt-code-reviews.appspot.com/1386806/diff/1/user/src/com/google/gwt/activity/client/RunAsyncActivity.java#newcode87
user/src/com/google/gwt/activity/client/RunAsyncActivity.java:87:
wrapped.start(panel, eventBus);

On 2011/03/25 01:53:05, tbroyer wrote:
> What if the activity is started, cancelled before it loads, and
started again?

Oops. I just plain forgot not to create the thing twice. Fixed it, and
the unit test is now watching it for it. Thanks again.

> As coded here, it'll start twice. I guess GWT.runAsync takes care of
the ongoing
> script load and doesn't try to load it twice; but that's not enough
here,
> because the "cancel" call will be lost, and actually, the wrapped
activity from
> the first onSuccess will be lost in "started" state.
> Moreover, AsyncProvider might return a new Activity instance each time
it's
> called, which makes it impossible to control the actually Activity
life-time
> from the ActivityMapper (you might return the same RunAsyncActivity
instance,
> but it doesn't mean you'll have a single instance of the wrapped
activity).

Shouldn't be an issue now that the provider only fires once.

On 2011/03/25 01:53:05, tbroyer wrote:
> How about an 'else { cancelled = true; }' and in the onSuccess then
only call
> the activity's start() if !cancelled?

Will do.

> The same would have to be done in onStop() too (even though it
shouldn't ever
> happen, because the activity shouldn't stopped if it hasn't yet "fully
started"
> by calling the AcceptsOneWidget back)

No, that's pointless. onStop() is only called if start() returns.

[rj...@google.com]

http://gwt-code-reviews.appspot.com/1386806/

[rj...@google.com]

http://gwt-code-reviews.appspot.com/1386806/

[Antoine DESSAIGNE]

Hi,

In fact, in my patch (here: http://gwt-code-reviews.appspot.com/1383802/), i didn't want to add the provider.

I still think that it's best to only put the AbstractAsyncActivity. The provider was a request from the issue itself but I don't think it's a good idea.

My major concern is flow control. Right now, there is nothing that would prevent from calling the methods of an Activity in a different order. On an Activity object there's nothing that prevents from calling the start() method after calling the onCancel() method. I think this is a good thing. That way you can use the same Activity object several time.

Therefore, i think that this simple class (http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java?column_width=80) should be the only thing added. (and remove the HasFailure interface).

In fact, i don't see a single case where I would want to run the same Activity sometime in runAsync and sometime directly.

But again, it was my first patch, so I wanted to do what was requested in the description of the issue even if I don't think it's a good thing.

Antoine.

[t.br...@gmail.com]

Disclaimer: I didn't review the test yet.

http://gwt-code-reviews.appspot.com/1386806/diff/1011/user/src/com/google/gwt/activity/Activity.gwt.xml
File user/src/com/google/gwt/activity/Activity.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1386806/diff/1011/user/src/com/google/gwt/activity/Activity.gwt.xml#newcode4
user/src/com/google/gwt/activity/Activity.gwt.xml:4: <inherits
name='javax.inject.Inject'/>
Will fail now that you removed the module.

http://gwt-code-reviews.appspot.com/1386806/diff/1011/user/src/com/google/gwt/activity/client/RunAsyncActivity.java
File user/src/com/google/gwt/activity/client/RunAsyncActivity.java
(right):

http://gwt-code-reviews.appspot.com/1386806/diff/1011/user/src/com/google/gwt/activity/client/RunAsyncActivity.java#newcode57
user/src/com/google/gwt/activity/client/RunAsyncActivity.java:57:
wrapped.start(panel, eventBus);
It still wouldn't handle the case of start/cancel/start.
This is what would happen from an ActivityManager:
activity.start(panel, new ResettableEventBus(bus));
activity.onCancel();
// activity2.start(), or activity2==null
// activity2.onCancel(), or mayStop+onStop
activity.start(panel, new ResettableEventBus(bus));

If the runAsync completes between the onCancel and the second start, all
is well, as loaded() will be true. If the runAsync completes after the
second start though, you'll:
1) get a second instance from the AsyncProvider
2) start the activity from the first get() and never cancel it
This could be mitigated by an additional 'loading' flag/state, as I
proposed yesterday.
This could happen if, for instance, the RunAsyncActivity is cached
somehow (or a singleton) and either the user changes his mind quickly,
or the network or server lags (and the user then navigates back and
forth until the app becomes "responsive" again; this is something I
happen to do quite regularly on GMail and the new Google Groups!)

http://gwt-code-reviews.appspot.com/1386806/diff/1011/user/src/com/google/gwt/core/client/AsyncProvider.java
File user/src/com/google/gwt/core/client/AsyncProvider.java (right):

http://gwt-code-reviews.appspot.com/1386806/diff/1011/user/src/com/google/gwt/core/client/AsyncProvider.java#newcode26
user/src/com/google/gwt/core/client/AsyncProvider.java:26: * @see
RunAsyncProvider
I can't find any RunAsyncProvider class; did you really mean to edit the
javadoc?
If you wanted to remove the reference to javax.inject.Provider, how
about doing an onSuccess(new Foo()) instead? that would have little to
no added value compared to an AsyncProxy, but it'd still show how to use
AsyncProvider with GWT.runAsync.

http://gwt-code-reviews.appspot.com/1386806/

[Thomas Broyer]

On Fri, Mar 25, 2011 at 9:09 AM, Antoine DESSAIGNE wrote:
> In fact, i don't see a single case where I would want to run the same
> Activity sometime in runAsync and sometime directly.

It's more that if you change your mind about what should run sync and
async, you shouldn't have to change your activity, because the
activity should still work the same.

I've attached my own, never-tested, attempt. Note that it assumes
you'll the methods in"the right order", but that can be changed
easily. It also switches to a permanent "failed" state if the runAsync
failed, but maybe it should re-attempt the runAsync in this case.
Finally, it was written before the AsyncProvider made it into GWT (and
GIN); the idea was that every subclass would implement doAsync as a
simple GWT.runAsync call (to create a split-point specific to the
subclass) and the createInstance() as a javax.inject.Provider::get().
The advantage compared to AsyncProvider is that you don't even create
the instance if the activity was cancelled before the fragment
finished loading.

[Antoine DESSAIGNE]

Thomas,

For clarity purpose, in this mail:

- a "synchronous Activity" is an activity where the code of the start() method runs directly

- an "asynchronous Activity" is an activity where the code of the start() method runs through GWT.runAsync.

I think there are 2 points.

The first one is whether or not it is useful be able to convert (through proxies, providers, ...) a synchronous Activity into an asynchronous one. I don't think of a use case where it can be useful. You either want your activity to run as a sync one or a async one, not both. If it's not used then why providing a proxy, provider, ... It only requires a single class with a starts() that performs a GWT.runAsync and 2 abstract methods: doStart() and onFailure()

The second one is whether or not it is useful to prevent the doStart() method from starting if the onCancel() method was called. IMO, this is not useful. As of today a synchronous Activity should check if the onCancel() was called in order to stop further initialization, there is no built-in control. As for the asynchronous activity:

- the onCancel() method won't prevent from downloading the JS file

- the developer is still required to check if onCancel() is called in order to prevent further initialization

I don't see the benefits of adding an extra layer of control that doesn't exist in synchronous activities.

Antoine.

2011/3/25 Thomas Broyer <t.br...@gmail.com>