Revision: 9049
Author: rj...@google.com
Date: Wed Oct 13 09:41:33 2010
Log: Fixes
http://code.google.com/p/google-web-toolkit/issues/detail?id=5375,
calling setDisplay(null) from a PlaceChangeEvent.Handler will cause an
NPE in the ActivityManager's own PlaceChangeEvent.Handler
Also undoes demo-time hack that left stopped widgets in place to
reduce flicker. Apps will have to reduce flicker themselves.
Review at http://gwt-code-reviews.appspot.com/988801
Review by: robertvaw...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9049
Modified:
/trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java
/trunk/user/src/com/google/gwt/activity/shared/ActivityMapper.java
/trunk/user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
=======================================
--- /trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java Tue
Oct 12 10:53:11 2010
+++ /trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java Wed
Oct 13 09:41:33 2010
@@ -1,12 +1,12 @@
/*
* Copyright 2010 Google Inc.
- *
+ *
* Licensed under the Apache License, Version 2.0 (the "License"); you may
not
* use this file except in compliance with the License. You may obtain a
copy of
* the License at
- *
+ *
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ *
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -49,7 +49,7 @@
public void setWidget(IsWidget view) {
if (this.activity == ActivityManager.this.currentActivity) {
startingNext = false;
- display.setWidget(view);
+ showWidget(view);
}
}
}
@@ -74,7 +74,7 @@
/**
* Create an ActivityManager. Next call {...@link #setDisplay}.
- *
+ *
* @param mapper finds the {...@link Activity} for a given
* {...@link com.google.gwt.place.shared.Place}
* @param eventBus source of {...@link PlaceChangeEvent} and
@@ -89,11 +89,17 @@
/**
* Deactivate the current activity, find the next one from our
ActivityMapper,
* and start it.
- *
+ * <p>
+ * The current activity's widget will be hidden immediately, which can
cause
+ * flicker if the next activity provides its widget asynchronously. That
can
+ * be minimized by decent caching. Perenially slow activities might
mitigate
+ * this by providing a widget immediately, with some kind of "loading"
+ * treatment.
+ *
* @see
com.google.gwt.place.shared.PlaceChangeEvent.Handler#onPlaceChange(PlaceChangeEvent)
*/
public void onPlaceChange(PlaceChangeEvent event) {
- Activity nextActivity = mapper.getActivity(event.getNewPlace());
+ Activity nextActivity = getNextActivity(event);
Throwable caughtOnStop = null;
Throwable caughtOnStart = null;
@@ -113,11 +119,7 @@
currentActivity = NULL_ACTIVITY;
startingNext = false;
} else if (!currentActivity.equals(NULL_ACTIVITY)) {
- /*
- * TODO until caching is in place, relying on stopped activities to
be
- * good citizens to reduce flicker. This makes me very nervous.
- */
- // display.showActivityWidget(null);
+ showWidget(null);
/*
* Kill off the activity's handlers, so it doesn't have to worry
about
@@ -141,7 +143,7 @@
currentActivity = nextActivity;
if (currentActivity.equals(NULL_ACTIVITY)) {
- display.setWidget(null);
+ showWidget(null);
return;
}
@@ -150,7 +152,7 @@
/*
* Now start the thing. Wrap the actual display with a per-call
instance
* that protects the display from canceled or stopped activities, and
which
- * maintain our startingNext state.
+ * maintains our startingNext state.
*/
try {
currentActivity.start(new ProtectedDisplay(currentActivity),
@@ -174,7 +176,7 @@
/**
* Reject the place change if the current activity is not willing to
stop.
- *
+ *
* @see
com.google.gwt.place.shared.PlaceChangeRequestEvent.Handler#onPlaceChangeRequest(PlaceChangeRequestEvent)
*/
public void onPlaceChangeRequest(PlaceChangeRequestEvent event) {
@@ -190,7 +192,7 @@
* If you are disposing of an ActivityManager, it is important to call
* setDisplay(null) to get it to deregister from the event bus, so that
it can
* be garbage collected.
- *
+ *
* @param display an instance of AcceptsOneWidget
*/
public void setDisplay(AcceptsOneWidget display) {
@@ -201,6 +203,24 @@
updateHandlers(willBeActive);
}
}
+
+ private Activity getNextActivity(PlaceChangeEvent event) {
+ if (display == null) {
+ /*
+ * Display may have been nulled during PlaceChangeEvent dispatch.
Don't
+ * bother the mapper, just return a null to ensure we shut down the
+ * current activity
+ */
+ return null;
+ }
+ return mapper.getActivity(event.getNewPlace());
+ }
+
+ private void showWidget(IsWidget view) {
+ if (display != null) {
+ display.setWidget(view);
+ }
+ }
private void updateHandlers(boolean activate) {
if (activate) {
=======================================
--- /trunk/user/src/com/google/gwt/activity/shared/ActivityMapper.java Tue
Oct 12 10:53:11 2010
+++ /trunk/user/src/com/google/gwt/activity/shared/ActivityMapper.java Wed
Oct 13 09:41:33 2010
@@ -23,7 +23,7 @@
*/
public interface ActivityMapper {
/**
- * Returns the activity to run for the given {...@link Place}.
+ * Returns the activity to run for the given {...@link Place}, or null.
*
* @param place a Place object
*/
=======================================
---
/trunk/user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
Tue Oct 5 17:59:14 2010
+++
/trunk/user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
Wed Oct 13 09:41:33 2010
@@ -189,11 +189,7 @@
assertNull(asyncActivity2.display);
eventBus.fireEvent(new PlaceChangeEvent(place2));
- /*
- * TODO until caching is in place, relying on stopped activities to be
good
- * citizens to reduce flicker. This makes me very nervous.
- */
- // assertNull(realDisplay.widget);
+ assertNull(realDisplay.widget);
assertFalse(asyncActivity1.canceled);
assertTrue(asyncActivity1.stopped);
assertFalse(asyncActivity2.stopped);
@@ -354,6 +350,60 @@
assertNotNull(activity2.display);
assertEquals(0, eventBus.getCount(Event.TYPE));
}
+
+ /**
+ * http://code.google.com/p/google-web-toolkit/issues/detail?id=5375
+ */
+ public void testNullDisplayOnPlaceChange() {
+ manager.setDisplay(realDisplay);
+
+ // Start an activity
+ manager.onPlaceChange(new PlaceChangeEvent(place1));
+
+ /*
+ * Now we're going to place2. During PlaceChangeEvent dispatch,
+ * someone kills the manager's display.
+ */
+ manager.setDisplay(null);
+
+ // Now the place change event reaches the manager
+ manager.onPlaceChange(new PlaceChangeEvent(place2));
+
+ assertNull(activity2.display);
+ assertTrue(activity1.stopped);
+ }
+
+ public void testNullDisplayBeforeAsyncStart() {
+ final AsyncActivity asyncActivity1 = new AsyncActivity(new MyView());
+ final AsyncActivity asyncActivity2 = new AsyncActivity(new MyView());
+
+ ActivityMapper map = new ActivityMapper() {
+ public Activity getActivity(Place place) {
+ if (place.equals(place1)) {
+ return asyncActivity1;
+ }
+ if (place.equals(place2)) {
+ return asyncActivity2;
+ }
+
+ return null;
+ }
+ };
+
+ manager = new ActivityManager(map, eventBus);
+ manager.setDisplay(realDisplay);
+
+ // Start an activity
+ manager.onPlaceChange(new PlaceChangeEvent(place1));
+
+ // Kill the manager
+ manager.setDisplay(null);
+
+ // The activity is ready to play
+ asyncActivity1.finish();
+
+ // Ta da, no NPE
+ }
public void testRejected() {
manager.setDisplay(realDisplay);
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
