Revision: 9660
Author: jlaba...@google.com
Date: Tue Feb 1 07:22:51 2011
Log: Handling errors more from user code more gracefully in
HasDataPresenter. If Cells, Cell Widgets, or the SelectionModel throw
exceptions during the rendering loop, we no longer lock the presenter's
rendering loop indefinitely.
Review at http://gwt-code-reviews.appspot.com/1310804
Review by: sbruba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9660
Modified:
/trunk/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
/trunk/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
=======================================
---
/trunk/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
Fri Dec 10 10:16:22 2010
+++
/trunk/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
Tue Feb 1 07:22:51 2011
@@ -1193,37 +1193,43 @@
* after the user has interacted with the widget at least once. This
* prevents values from being selected by default.
*/
- if (KeyboardSelectionPolicy.BOUND_TO_SELECTION ==
keyboardSelectionPolicy
- && selectionModel != null && pending.viewTouched) {
- T oldValue = oldState.getSelectedValue();
- Object oldKey = getRowValueKey(oldValue);
- T newValue = rowDataCount > 0
- ? pending.getRowDataValue(pending.getKeyboardSelectedRow()) :
null;
- Object newKey = getRowValueKey(newValue);
- /*
- * Do not deselect the old value unless we have a new value to
select, or
- * we will have a null selection event while we wait for
asynchronous data
- * to load.
- */
- if (newKey != null && !newKey.equals(oldKey)) {
- // Check both values for selection before setting selection, or the
- // selection model may resolve state early.
- boolean oldValueWasSelected = (oldValue == null) ? false
- : selectionModel.isSelected(oldValue);
- boolean newValueWasSelected = (newValue == null) ? false
- : selectionModel.isSelected(newValue);
-
- // Deselect the old value.
- if (oldValueWasSelected) {
- selectionModel.setSelected(oldValue, false);
- }
-
- // Select the new value.
- pending.selectedValue = newValue;
- if (newValue != null && !newValueWasSelected) {
- selectionModel.setSelected(newValue, true);
+ try {
+ if (KeyboardSelectionPolicy.BOUND_TO_SELECTION ==
keyboardSelectionPolicy
+ && selectionModel != null && pending.viewTouched) {
+ T oldValue = oldState.getSelectedValue();
+ Object oldKey = getRowValueKey(oldValue);
+ T newValue = rowDataCount > 0
+ ? pending.getRowDataValue(pending.getKeyboardSelectedRow()) :
null;
+ Object newKey = getRowValueKey(newValue);
+ /*
+ * Do not deselect the old value unless we have a new value to
select,
+ * or we will have a null selection event while we wait for
asynchronous
+ * data to load.
+ */
+ if (newKey != null && !newKey.equals(oldKey)) {
+ // Check both values for selection before setting selection, or
the
+ // selection model may resolve state early.
+ boolean oldValueWasSelected = (oldValue == null) ? false
+ : selectionModel.isSelected(oldValue);
+ boolean newValueWasSelected = (newValue == null) ? false
+ : selectionModel.isSelected(newValue);
+
+ // Deselect the old value.
+ if (oldValueWasSelected) {
+ selectionModel.setSelected(oldValue, false);
+ }
+
+ // Select the new value.
+ pending.selectedValue = newValue;
+ if (newValue != null && !newValueWasSelected) {
+ selectionModel.setSelected(newValue, true);
+ }
}
}
+ } catch (RuntimeException e) {
+ // Unlock the rendering loop if the user SelectionModel throw an
error.
+ isResolvingState = false;
+ throw e;
}
// If the keyboard row changes, add it to the modified set.
@@ -1338,64 +1344,70 @@
/*
* Push changes to the view.
*/
- if (redrawRequired) {
- // Redraw the entire content.
- SafeHtmlBuilder sb = new SafeHtmlBuilder();
- view.render(sb, pending.rowData, pending.pageStart, selectionModel);
- SafeHtml newContents = sb.toSafeHtml();
- if (!newContents.equals(lastContents)) {
- lastContents = newContents;
- view.replaceAllChildren(pending.rowData, newContents,
- pending.keyboardStealFocus);
- }
- view.resetFocus();
- } else if (range0 != null) {
- // Replace specific rows.
- lastContents = null;
-
- // Replace range0.
- {
- int absStart = range0.getStart();
- int relStart = absStart - pageStart;
+ try {
+ if (redrawRequired) {
+ // Redraw the entire content.
SafeHtmlBuilder sb = new SafeHtmlBuilder();
- List<T> replaceValues = pending.rowData.subList(relStart, relStart
- + range0.getLength());
- view.render(sb, replaceValues, absStart, selectionModel);
- view.replaceChildren(replaceValues, relStart, sb.toSafeHtml(),
- pending.keyboardStealFocus);
- }
-
- // Replace range1 if it exists.
- if (range1 != null) {
- int absStart = range1.getStart();
- int relStart = absStart - pageStart;
- SafeHtmlBuilder sb = new SafeHtmlBuilder();
- List<T> replaceValues = pending.rowData.subList(relStart, relStart
- + range1.getLength());
- view.render(sb, replaceValues, absStart, selectionModel);
- view.replaceChildren(replaceValues, relStart, sb.toSafeHtml(),
- pending.keyboardStealFocus);
- }
-
- view.resetFocus();
- } else if (keyboardRowChanged) {
- // Update the keyboard selected rows without redrawing.
- // Deselect the old keyboard row.
- int oldSelectedRow = oldState.getKeyboardSelectedRow();
- if (oldSelectedRow >= 0 && oldSelectedRow < rowDataCount) {
- view.setKeyboardSelected(oldSelectedRow, false, false);
- }
-
- // Select the new keyboard row.
- int newSelectedRow = pending.getKeyboardSelectedRow();
- if (newSelectedRow >= 0 && newSelectedRow < rowDataCount) {
- view.setKeyboardSelected(newSelectedRow, true,
- pending.keyboardStealFocus);
- }
- }
-
- // We are done resolving state.
- isResolvingState = false;
+ view.render(sb, pending.rowData, pending.pageStart,
selectionModel);
+ SafeHtml newContents = sb.toSafeHtml();
+ if (!newContents.equals(lastContents)) {
+ lastContents = newContents;
+ view.replaceAllChildren(pending.rowData, newContents,
+ pending.keyboardStealFocus);
+ }
+ view.resetFocus();
+ } else if (range0 != null) {
+ // Replace specific rows.
+ lastContents = null;
+
+ // Replace range0.
+ {
+ int absStart = range0.getStart();
+ int relStart = absStart - pageStart;
+ SafeHtmlBuilder sb = new SafeHtmlBuilder();
+ List<T> replaceValues = pending.rowData.subList(relStart,
relStart
+ + range0.getLength());
+ view.render(sb, replaceValues, absStart, selectionModel);
+ view.replaceChildren(replaceValues, relStart, sb.toSafeHtml(),
+ pending.keyboardStealFocus);
+ }
+
+ // Replace range1 if it exists.
+ if (range1 != null) {
+ int absStart = range1.getStart();
+ int relStart = absStart - pageStart;
+ SafeHtmlBuilder sb = new SafeHtmlBuilder();
+ List<T> replaceValues = pending.rowData.subList(relStart,
relStart
+ + range1.getLength());
+ view.render(sb, replaceValues, absStart, selectionModel);
+ view.replaceChildren(replaceValues, relStart, sb.toSafeHtml(),
+ pending.keyboardStealFocus);
+ }
+
+ view.resetFocus();
+ } else if (keyboardRowChanged) {
+ // Update the keyboard selected rows without redrawing.
+ // Deselect the old keyboard row.
+ int oldSelectedRow = oldState.getKeyboardSelectedRow();
+ if (oldSelectedRow >= 0 && oldSelectedRow < rowDataCount) {
+ view.setKeyboardSelected(oldSelectedRow, false, false);
+ }
+
+ // Select the new keyboard row.
+ int newSelectedRow = pending.getKeyboardSelectedRow();
+ if (newSelectedRow >= 0 && newSelectedRow < rowDataCount) {
+ view.setKeyboardSelected(newSelectedRow, true,
+ pending.keyboardStealFocus);
+ }
+ }
+ } finally {
+ /*
+ * We are done resolving state, so unlock the rendering loop. We
unlock
+ * the loop even if user rendering code throws an error to avoid
throwing
+ * an additional, misleading IllegalStateException.
+ */
+ isResolvingState = false;
+ }
}
/**
=======================================
---
/trunk/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
Fri Dec 10 10:16:22 2010
+++
/trunk/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
Tue Feb 1 07:22:51 2011
@@ -17,6 +17,7 @@
import com.google.gwt.core.client.Scheduler;
import com.google.gwt.event.shared.EventHandler;
+import com.google.gwt.event.shared.GwtEvent;
import com.google.gwt.event.shared.GwtEvent.Type;
import com.google.gwt.event.shared.HandlerRegistration;
import com.google.gwt.junit.client.GWTTestCase;
@@ -35,6 +36,7 @@
import com.google.gwt.view.client.Range;
import com.google.gwt.view.client.RangeChangeEvent;
import com.google.gwt.view.client.SelectionChangeEvent;
+import com.google.gwt.view.client.SelectionChangeEvent.Handler;
import com.google.gwt.view.client.SelectionModel;
import com.google.gwt.view.client.SingleSelectionModel;
@@ -271,6 +273,88 @@
assertNull(handler.getLastRange());
handler.reset();
}
+
+ /**
+ * Test that the presenter can gracefully handle a view that throws
exceptions
+ * when rendering the content.
+ */
+ public void testBadViewSelectionModel() {
+ SelectionModel<String> badModel = new SelectionModel<String>() {
+ public void fireEvent(GwtEvent<?> event) {
+ }
+
+ public Object getKey(String item) {
+ return null;
+ }
+
+ public HandlerRegistration addSelectionChangeHandler(Handler
handler) {
+ return null;
+ }
+
+ public boolean isSelected(String object) {
+ throw new NullPointerException();
+ }
+
+ public void setSelected(String object, boolean selected) {
+ throw new NullPointerException();
+ }
+ };
+
+ // Use the bad view in a presenter.
+ MockView<String> view = new MockView<String>();
+ HasData<String> listView = new MockHasData<String>();
+ HasDataPresenter<String> presenter = new
HasDataPresenter<String>(listView,
+ view, 10, null);
+ presenter.setSelectionModel(badModel);
+
presenter.setKeyboardSelectionPolicy(KeyboardSelectionPolicy.BOUND_TO_SELECTION);
+ testPresenterWithBadUserCode(presenter);
+ }
+
+ /**
+ * Test that the presenter can gracefully handle a view that throws
exceptions
+ * when rendering the content.
+ */
+ public void testBadViewRender() {
+ MockView<String> badView = new MockView<String>() {
+ @Override
+ public void render(SafeHtmlBuilder sb, List<String> values, int
start,
+ SelectionModel<? super String> selectionModel) {
+ throw new NullPointerException();
+ }
+ };
+
+ // Use the bad view in a presenter.
+ HasData<String> listView = new MockHasData<String>();
+ HasDataPresenter<String> presenter = new
HasDataPresenter<String>(listView,
+ badView, 10, null);
+ testPresenterWithBadUserCode(presenter);
+ }
+
+ /**
+ * Test that the presenter can gracefully handle a view that throws
exceptions
+ * when rendering the children.
+ */
+ public void testBadViewReplaceChildren() {
+ MockView<String> badView = new MockView<String>() {
+ @Override
+ public void replaceAllChildren(List<String> values, SafeHtml html,
+ boolean stealFocus) {
+ throw new NullPointerException();
+ }
+
+ @Override
+ public void replaceChildren(List<String> values, int start,
+ SafeHtml html, boolean stealFocus) {
+ throw new NullPointerException();
+ }
+ };
+
+ // Use the bad view in a presenter.
+ HasData<String> listView = new MockHasData<String>();
+ HasDataPresenter<String> presenter = new
HasDataPresenter<String>(listView,
+ badView, 10, null);
+ testPresenterWithBadUserCode(presenter);
+ }
public void testCalculateModifiedRanges() {
HasData<String> listView = new MockHasData<String>();
@@ -1881,4 +1965,37 @@
int length = range.getLength();
presenter.setRowData(start, createData(start, length));
}
-}
+
+ /**
+ * Test that the presenter can gracefully handle a view or
+ * {@link SelectionModel} that throws an exception.
+ *
+ * @param presenter the presenter to test
+ */
+ private void testPresenterWithBadUserCode(HasDataPresenter<String>
presenter) {
+ // Render some data with an exception.
+ try {
+ populatePresenter(presenter);
+ presenter.setKeyboardSelectedRow(0, false, false);
+ presenter.flush();
+ fail("Expected NullPointerException");
+ } catch (NullPointerException e) {
+ // Expected.
+ }
+
+ // Render additional data with an exception.
+ try {
+ presenter.setVisibleRange(new Range(10, 10));
+ populatePresenter(presenter);
+ presenter.setKeyboardSelectedRow(1, false, false);
+ presenter.flush();
+ fail("Expected NullPointerException");
+ } catch (NullPointerException e) {
+ /*
+ * Expected. If we do not get a NullPointerException, then we are
stuck in
+ * the rendering loop. We should not get an IllegalStateException
from the
+ * rendering loop if the presenter fails gracefully.
+ */
+ }
+ }
+}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
