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: sbrubaker@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@9660 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java b/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
index 1ecbfba..ee3789c 100644
--- a/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
+++ b/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
@@ -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);
+ 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);
- }
+ // 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);
+ // 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);
- }
+ 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 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);
- }
+ // 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);
+ }
- 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);
- }
+ // 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);
+ }
- // Select the new keyboard row.
- int newSelectedRow = pending.getKeyboardSelectedRow();
- if (newSelectedRow >= 0 && newSelectedRow < rowDataCount) {
- view.setKeyboardSelected(newSelectedRow, true,
- 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;
}
-
- // We are done resolving state.
- isResolvingState = false;
}
/**
diff --git a/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java b/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
index e387698..1ff0fa6 100644
--- a/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
+++ b/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
@@ -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;
@@ -272,6 +274,88 @@
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>();
MockView<String> view = new MockView<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.
+ */
+ }
+ }
}
