Mofifying HasDataPresenter to be more resilient to state changes while it is resolving state changes. For example, if user code triggers a SelectionChangeEvent while selection is being resolved, we can handle that.
Review at http://gwt-code-reviews.appspot.com/1583804
Review by: dcavalcanti@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10728 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 a7d87f5..f129db5 100644
--- a/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
+++ b/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
@@ -376,8 +376,9 @@
static final int PAGE_INCREMENT = 30;
/**
- * The maximum number of times we can try to {@link #resolvePendingState()}
- * before we assume there is an infinite loop.
+ * The maximum number of times we can try to
+ * {@link #resolvePendingState(JsArrayInteger)} before we assume there is an
+ * infinite loop.
*/
private static final int LOOP_MAXIMUM = 10;
@@ -429,9 +430,10 @@
private ScheduledCommand pendingStateCommand;
/**
- * A counter used to detect infinite loops in {@link #resolvePendingState()}.
- * An infinite loop can occur if user code, such as reading the
- * {@link SelectionModel}, causes the table to have a pending state.
+ * A counter used to detect infinite loops in
+ * {@link #resolvePendingState(JsArrayInteger)}. An infinite loop can occur if
+ * user code, such as reading the {@link SelectionModel}, causes the table to
+ * have a pending state.
*/
private int pendingStateLoop = 0;
@@ -513,15 +515,7 @@
* Flush pending changes to the view.
*/
public void flush() {
- /*
- * resolvePendingState can exit early user code applied more pending state,
- * so we need to loop until we are sure that the pending state is clear. If
- * the user calls this method while resolving pending state, then do not
- * attempt to resolve pending state again.
- */
- while (pendingStateCommand != null && !isResolvingState) {
- resolvePendingState();
- }
+ resolvePendingState(null);
}
/**
@@ -987,7 +981,7 @@
public void execute() {
// Verify that this command was the last one scheduled.
if (pendingStateCommand == this) {
- resolvePendingState();
+ resolvePendingState(null);
}
}
};
@@ -1060,14 +1054,28 @@
/**
* Resolve the pending state and push updates to the view.
+ *
+ * @param modifiedRows the modified rows that need to be updated, or null if
+ * none. The modified rows may be mutated.
+ * @return true if the state changed, false if not
*/
- private void resolvePendingState() {
+ private boolean resolvePendingState(JsArrayInteger modifiedRows) {
pendingStateCommand = null;
+ /*
+ * We are already resolving state. New changes will be flushed after the
+ * current flush is finished.
+ */
+ if (isResolvingState) {
+ return false;
+ }
+ isResolvingState = true;
+
// Early exit if there is no pending state.
if (pendingState == null) {
+ isResolvingState = false;
pendingStateLoop = 0;
- return;
+ return false;
}
/*
@@ -1076,6 +1084,7 @@
*/
pendingStateLoop++;
if (pendingStateLoop > LOOP_MAXIMUM) {
+ isResolvingState = false;
pendingStateLoop = 0; // Let user code handle exception and try again.
throw new IllegalStateException(
"A possible infinite loop has been detected in a Cell Widget. This "
@@ -1085,16 +1094,13 @@
}
/*
- * Check for conflicting state resolution code. This can happen if the
- * View's render methods modify the view and flush the pending state.
+ * Swap the states in case user code triggers more changes, which will
+ * create a new pendingState.
*/
- if (isResolvingState) {
- throw new IllegalStateException(
- "The Cell Widget is attempting to render itself within the render "
- + "loop. This usually happens when your render code modifies the "
- + "state of the Cell Widget then accesses data or elements " + "within the Widget.");
- }
- isResolvingState = true;
+ State<T> oldState = state;
+ PendingState<T> pending = pendingState;
+ state = pendingState;
+ pendingState = null;
/*
* Keep track of the absolute indexes of modified rows.
@@ -1102,11 +1108,11 @@
* Use a native array to avoid dynamic casts associated with emulated Java
* Collections.
*/
- JsArrayInteger modifiedRows = JavaScriptObject.createArray().cast();
+ if (modifiedRows == null) {
+ modifiedRows = JavaScriptObject.createArray().cast();
+ }
// Get the values used for calculations.
- State<T> oldState = state;
- PendingState<T> pending = pendingState;
int pageStart = pending.getPageStart();
int pageSize = pending.getPageSize();
int pageEnd = pageStart + pageSize;
@@ -1115,7 +1121,7 @@
/*
* Resolve keyboard selection. If the row value still exists, use its index.
* If the row value exists in multiple places, use the closest index. If the
- * row value not longer exists, use the current index.
+ * row value no longer exists, use the current index.
*/
pending.keyboardSelectedRow =
Math.max(0, Math.min(pending.keyboardSelectedRow, rowDataCount - 1));
@@ -1145,10 +1151,9 @@
}
/*
- * Update the SelectionModel based on the keyboard selected value. This must
- * happen before we read the selection state. We only bind to selection
- * after the user has interacted with the widget at least once. This
- * prevents values from being selected by default.
+ * Update the SelectionModel based on the keyboard selected value. We only
+ * bind to selection after the user has interacted with the widget at least
+ * once. This prevents values from being selected by default.
*/
try {
if (KeyboardSelectionPolicy.BOUND_TO_SELECTION == keyboardSelectionPolicy
@@ -1176,7 +1181,7 @@
if (oldValueWasSelected) {
selectionModel.setSelected(oldValue, false);
}
-
+
// Select the new value.
pending.selectedValue = newValue;
if (newValue != null && !newValueWasSelected) {
@@ -1187,10 +1192,17 @@
pending.selectedValue = null;
}
}
+
+ // User code may have created a new pending state, so we need to
+ // propagate this new selected value.
+ if (pendingState != null) {
+ pendingState.selectedValue = pending.selectedValue;
+ }
}
} catch (RuntimeException e) {
// Unlock the rendering loop if the user SelectionModel throw an error.
isResolvingState = false;
+ pendingStateLoop = 0;
throw e;
}
@@ -1206,37 +1218,52 @@
* longer have a SelectionModel but had selected rows, we still need to
* update the rows.
*/
- for (int i = pageStart; i < pageStart + rowDataCount; i++) {
- // Check the new selection state.
- T rowValue = pending.getRowDataValue(i - pageStart);
- boolean isSelected =
- (rowValue != null && selectionModel != null && selectionModel.isSelected(rowValue));
+ try {
+ for (int i = pageStart; i < pageStart + rowDataCount; i++) {
+ // Check the new selection state.
+ T rowValue = pending.getRowDataValue(i - pageStart);
+ boolean isSelected =
+ (rowValue != null && selectionModel != null && selectionModel.isSelected(rowValue));
- // Compare to the old selection state.
- boolean wasSelected = oldState.isRowSelected(i);
- if (isSelected) {
- pending.selectedRows.add(i);
- if (!wasSelected) {
+ // Compare to the old selection state.
+ boolean wasSelected = oldState.isRowSelected(i);
+ if (isSelected) {
+ pending.selectedRows.add(i);
+ if (pendingState != null) {
+ // Propagate changes if user code created a new pending state.
+ pendingState.selectedRows.add(i);
+ }
+ if (!wasSelected) {
+ modifiedRows.push(i);
+ }
+ } else if (wasSelected) {
modifiedRows.push(i);
}
- } else if (wasSelected) {
- modifiedRows.push(i);
}
+ } catch (RuntimeException e) {
+ // Unlock the rendering loop if the user SelectionModel throw an error.
+ isResolvingState = false;
+ pendingStateLoop = 0;
+ throw e;
}
/*
* We called methods in user code that could modify the view, so early exit
* if there is a new pending state waiting to be resolved.
*/
- if (pendingStateCommand != null) {
+ if (pendingState != null) {
isResolvingState = false;
- return;
- }
- pendingStateLoop = 0;
+ // Do not reset pendingStateLoop, or we will not detect the infinite loop.
- // Swap the states.
- state = pendingState;
- pendingState = null;
+ /*
+ * Try to resolve state again. If we are successful, then the modified
+ * rows will be redrawn. If we are not successful, then we still need to
+ * redraw the modified rows.
+ */
+ if (resolvePendingState(modifiedRows)) {
+ return true;
+ }
+ }
// Add the replaced ranges as modified rows.
boolean replacedEmptyRange = false;
@@ -1358,6 +1385,11 @@
*/
isResolvingState = false;
}
+
+ // If any new state exists, resolve it now. If there is no new state, reset
+ // the pendingStateLoop.
+ resolvePendingState(null);
+ return true;
}
/**
