Fixing a bug in CellTree where refreshing an empty list of children causes an AssertionError that is not captured by the console. CellTreeNodeView#loadChildState() accesses the first child element of the current node in preparation of a loop, but we didn't handle the case where their is no first child because the node is empty. I also added a catch block in HasDataPresenter to rethrow Errors as RuntimeExceptions so we can view them in dev mode.
Review at http://gwt-code-reviews.appspot.com/1463805
Review by: dconnelly@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10353 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java b/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
index c35ed19..04e8c8a 100644
--- a/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
+++ b/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
@@ -79,8 +79,6 @@
SafeHtml outerDiv(SafeStyles cssString, String classes, SafeHtml content);
}
- private static final Template template = GWT.create(Template.class);
-
/**
* The {@link com.google.gwt.view.client.HasData} used to show children. This
* class is intentionally static because we might move it to a new
@@ -89,7 +87,7 @@
*
* @param <C> the child item type
*/
- private static class NodeCellList<C> implements HasData<C> {
+ static class NodeCellList<C> implements HasData<C> {
/**
* The view used by the NodeCellList.
@@ -102,11 +100,13 @@
this.childContainer = childContainer;
}
+ @Override
public <H extends EventHandler> HandlerRegistration addHandler(H handler,
Type<H> type) {
return handlerManger.addHandler(type, handler);
}
+ @Override
public void render(SafeHtmlBuilder sb, List<C> values, int start,
SelectionModel<? super C> selectionModel) {
// Cache the style names that will be used for each child.
@@ -190,6 +190,7 @@
}
}
+ @Override
public void replaceAllChildren(List<C> values, SafeHtml html,
boolean stealFocus) {
// Hide the child container so we can animate it.
@@ -227,6 +228,7 @@
}
}
+ @Override
public void replaceChildren(List<C> values, int start, SafeHtml html,
boolean stealFocus) {
Map<Object, CellTreeNodeView<?>> savedViews = saveChildState(values,
@@ -242,10 +244,12 @@
loadChildState(values, start, savedViews);
}
+ @Override
public void resetFocus() {
nodeView.tree.resetFocus();
}
+ @Override
public void setKeyboardSelected(int index, boolean selected,
boolean stealFocus) {
// Keyboard selection is handled by CellTree.
@@ -254,6 +258,7 @@
nodeView.tree.getStyle().cellTreeKeyboardSelectedItem(), selected);
}
+ @Override
public void setLoadingState(LoadingState state) {
nodeView.updateImage(state == LoadingState.LOADING);
showOrHide(nodeView.emptyMessageElem, state == LoadingState.LOADED
@@ -262,7 +267,7 @@
/**
* Reload the open children after rendering new items in this node.
- *
+ *
* @param values the values being replaced
* @param start the start index
* @param savedViews the open nodes
@@ -275,12 +280,12 @@
ProvidesKey<C> keyProvider = nodeInfo.getProvidesKey();
Element container = nodeView.ensureChildContainer();
- Element childElem = container.getChild(start).cast();
+ Element childElem = (values.size() == 0) ? null : Element.as(container.getChild(start));
CellTreeNodeView<?> keyboardSelected = nodeView.tree.getKeyboardSelectedNode();
for (int i = start; i < end; i++) {
C childValue = values.get(i - start);
- CellTreeNodeView<C> child = nodeView.createTreeNodeView(nodeInfo,
- childElem, childValue, null);
+ CellTreeNodeView<C> child =
+ nodeView.createTreeNodeView(nodeInfo, childElem, childValue, null);
CellTreeNodeView<?> savedChild = savedViews.remove(keyProvider.getKey(childValue));
// Copy the saved child's state into the new child
if (savedChild != null) {
@@ -405,12 +410,12 @@
}
}
+ final HasDataPresenter<C> presenter;
private final Cell<C> cell;
private final int defaultPageSize;
private HandlerManager handlerManger = new HandlerManager(this);
private final NodeInfo<C> nodeInfo;
private CellTreeNodeView<?> nodeView;
- private final HasDataPresenter<C> presenter;
public NodeCellList(final NodeInfo<C> nodeInfo,
final CellTreeNodeView<?> nodeView, int pageSize) {
@@ -428,6 +433,7 @@
// Use a pager to update buttons.
presenter.addRowCountChangeHandler(new RowCountChangeEvent.Handler() {
+ @Override
public void onRowCountChange(RowCountChangeEvent event) {
int rowCount = event.getNewRowCount();
boolean isExact = event.isNewRowCountExact();
@@ -437,15 +443,18 @@
});
}
+ @Override
public HandlerRegistration addCellPreviewHandler(Handler<C> handler) {
return presenter.addCellPreviewHandler(handler);
}
+ @Override
public HandlerRegistration addRangeChangeHandler(
RangeChangeEvent.Handler handler) {
return presenter.addRangeChangeHandler(handler);
}
+ @Override
public HandlerRegistration addRowCountChangeHandler(
RowCountChangeEvent.Handler handler) {
return presenter.addRowCountChangeHandler(handler);
@@ -458,6 +467,7 @@
presenter.clearSelectionModel();
}
+ @Override
public void fireEvent(GwtEvent<?> event) {
handlerManger.fireEvent(event);
}
@@ -466,58 +476,72 @@
return defaultPageSize;
}
+ @Override
public int getRowCount() {
return presenter.getRowCount();
}
+ @Override
public SelectionModel<? super C> getSelectionModel() {
return presenter.getSelectionModel();
}
+ @Override
public C getVisibleItem(int indexOnPage) {
return presenter.getVisibleItem(indexOnPage);
}
+ @Override
public int getVisibleItemCount() {
return presenter.getVisibleItemCount();
}
+ @Override
public List<C> getVisibleItems() {
return presenter.getVisibleItems();
}
+ @Override
public Range getVisibleRange() {
return presenter.getVisibleRange();
}
+ @Override
public boolean isRowCountExact() {
return presenter.isRowCountExact();
}
+ @Override
public final void setRowCount(int count) {
setRowCount(count, true);
}
+ @Override
public void setRowCount(int size, boolean isExact) {
presenter.setRowCount(size, isExact);
}
+ @Override
public void setRowData(int start, List<? extends C> values) {
presenter.setRowData(start, values);
}
+ @Override
public void setSelectionModel(final SelectionModel<? super C> selectionModel) {
presenter.setSelectionModel(selectionModel);
}
+ @Override
public final void setVisibleRange(int start, int length) {
setVisibleRange(new Range(start, length));
}
+ @Override
public void setVisibleRange(Range range) {
presenter.setVisibleRange(range);
}
+ @Override
public void setVisibleRangeAndClearData(Range range,
boolean forceRangeChangeEvent) {
presenter.setVisibleRangeAndClearData(range, forceRangeChangeEvent);
@@ -538,12 +562,14 @@
this.nodeView = nodeView;
}
+ @Override
public int getChildCount() {
assertNotDestroyed();
flush();
return nodeView.getChildCount();
}
+ @Override
public Object getChildValue(int index) {
assertNotDestroyed();
checkChildBounds(index);
@@ -551,21 +577,25 @@
return nodeView.getChildNode(index).value;
}
+ @Override
public int getIndex() {
assertNotDestroyed();
return (nodeView.parentNode == null) ? 0
: nodeView.parentNode.children.indexOf(nodeView);
}
+ @Override
public TreeNode getParent() {
assertNotDestroyed();
return getParentImpl();
}
+ @Override
public Object getValue() {
return nodeView.value;
}
+ @Override
public boolean isChildLeaf(int index) {
assertNotDestroyed();
checkChildBounds(index);
@@ -573,6 +603,7 @@
return nodeView.getChildNode(index).isLeaf();
}
+ @Override
public boolean isChildOpen(int index) {
assertNotDestroyed();
checkChildBounds(index);
@@ -580,6 +611,7 @@
return nodeView.getChildNode(index).isOpen();
}
+ @Override
public boolean isDestroyed() {
if (!nodeView.isDestroyed) {
/*
@@ -594,10 +626,12 @@
return nodeView.isDestroyed || !nodeView.isOpen();
}
+ @Override
public TreeNode setChildOpen(int index, boolean open) {
return setChildOpen(index, open, true);
}
+ @Override
public TreeNode setChildOpen(int index, boolean open, boolean fireEvents) {
assertNotDestroyed();
checkChildBounds(index);
@@ -650,6 +684,8 @@
*/
private static final SafeHtml LEAF_IMAGE = SafeHtmlUtils.fromSafeConstant("<div style='position:absolute;display:none;'></div>");
+ private static final Template template = GWT.create(Template.class);
+
/**
* The temporary element used to render child items.
*/
@@ -710,6 +746,11 @@
}
/**
+ * The list view used to display the nodes.
+ */
+ NodeCellList<?> listView;
+
+ /**
* True during the time a node should be animated.
*/
private boolean animate;
@@ -753,11 +794,6 @@
private boolean isDestroyed;
/**
- * The list view used to display the nodes.
- */
- private NodeCellList<?> listView;
-
- /**
* The info about children of this node.
*/
private NodeInfo<?> nodeInfo;
@@ -1029,6 +1065,7 @@
tree.cellIsEditing = parentCell.isEditing(context, cellParent, value);
if (cellWasEditing && !tree.cellIsEditing) {
CellBasedWidgetImpl.get().resetFocus(new Scheduler.ScheduledCommand() {
+ @Override
public void execute() {
tree.setFocus(true);
}
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 3eecf69..74b81d8 100644
--- a/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
+++ b/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
@@ -178,42 +178,52 @@
this.pageSize = pageSize;
}
+ @Override
public int getKeyboardSelectedRow() {
return keyboardSelectedRow;
}
+ @Override
public T getKeyboardSelectedRowValue() {
return keyboardSelectedRowValue;
}
+ @Override
public int getPageSize() {
return pageSize;
}
+ @Override
public int getPageStart() {
return pageStart;
}
+ @Override
public int getRowCount() {
return rowCount;
}
+ @Override
public int getRowDataSize() {
return rowData.size();
}
+ @Override
public T getRowDataValue(int index) {
return rowData.get(index);
}
+ @Override
public List<T> getRowDataValues() {
return Collections.unmodifiableList(rowData);
}
+ @Override
public T getSelectedValue() {
return selectedValue;
}
+ @Override
public boolean isRowCountExact() {
return rowCountIsExact;
}
@@ -226,10 +236,12 @@
* method should only be called on the state after it has been resolved.
* </p>
*/
+ @Override
public boolean isRowSelected(int index) {
return selectedRows.contains(index);
}
+ @Override
public boolean isViewTouched() {
return viewTouched;
}
@@ -372,8 +384,8 @@
/**
* The number of rows to jump when PAGE_UP or PAGE_DOWN is pressed and the
- * {@link KeyboardSelectionPolicy} is
- * {@link KeyboardSelectionPolicy.INCREMENT_PAGE}.
+ * {@link HasKeyboardPagingPolicy.KeyboardPagingPolicy} is
+ * {@link HasKeyboardPagingPolicy.KeyboardPagingPolicy#INCREASE_RANGE}.
*/
static final int PAGE_INCREMENT = 30;
@@ -459,6 +471,7 @@
this.state = new DefaultState<T>(pageSize);
}
+ @Override
public HandlerRegistration addCellPreviewHandler(
CellPreviewEvent.Handler<T> handler) {
return view.addHandler(handler, CellPreviewEvent.getType());
@@ -469,11 +482,13 @@
return view.addHandler(handler, LoadingStateChangeEvent.TYPE);
}
+ @Override
public HandlerRegistration addRangeChangeHandler(
RangeChangeEvent.Handler handler) {
return view.addHandler(handler, RangeChangeEvent.getType());
}
+ @Override
public HandlerRegistration addRowCountChangeHandler(
RowCountChangeEvent.Handler handler) {
return view.addHandler(handler, RowCountChangeEvent.getType());
@@ -502,6 +517,7 @@
/**
* @throws UnsupportedOperationException
*/
+ @Override
public void fireEvent(GwtEvent<?> event) {
// HasData should fire their own events.
throw new UnsupportedOperationException();
@@ -532,6 +548,7 @@
return Math.min(getPageSize(), getRowCount() - getPageStart());
}
+ @Override
public KeyboardPagingPolicy getKeyboardPagingPolicy() {
return keyboardPagingPolicy;
}
@@ -568,10 +585,12 @@
: getCurrentState().getKeyboardSelectedRowValue();
}
+ @Override
public KeyboardSelectionPolicy getKeyboardSelectionPolicy() {
return keyboardSelectionPolicy;
}
+ @Override
public ProvidesKey<T> getKeyProvider() {
return keyProvider;
}
@@ -581,22 +600,27 @@
*
* @return the data size
*/
+ @Override
public int getRowCount() {
return getCurrentState().getRowCount();
}
+ @Override
public SelectionModel<? super T> getSelectionModel() {
return selectionModel;
}
+ @Override
public T getVisibleItem(int indexOnPage) {
return getCurrentState().getRowDataValue(indexOnPage);
}
+ @Override
public int getVisibleItemCount() {
return getCurrentState().getRowDataSize();
}
+ @Override
public List<T> getVisibleItems() {
return getCurrentState().getRowDataValues();
}
@@ -604,6 +628,7 @@
/**
* Return the range of data being displayed.
*/
+ @Override
public Range getVisibleRange() {
return new Range(getPageStart(), getPageSize());
}
@@ -626,7 +651,7 @@
}
/**
- * Check if the next call to {@link #keyboardPrevious()} would succeed.
+ * Check if the next call to {@link #keyboardPrev()} would succeed.
*
* @return true if there is a previous row accessible by the keyboard
*/
@@ -662,6 +687,7 @@
return isRowCountExact() && getRowCount() == 0;
}
+ @Override
public boolean isRowCountExact() {
return getCurrentState().isRowCountExact();
}
@@ -736,6 +762,7 @@
ensurePendingState().redrawRequired = true;
}
+ @Override
public void setKeyboardPagingPolicy(KeyboardPagingPolicy policy) {
if (policy == null) {
throw new NullPointerException("KeyboardPagingPolicy cannot be null");
@@ -839,6 +866,7 @@
}
}
+ @Override
public void setKeyboardSelectionPolicy(KeyboardSelectionPolicy policy) {
if (policy == null) {
throw new NullPointerException("KeyboardSelectionPolicy cannot be null");
@@ -849,12 +877,14 @@
/**
* @throws UnsupportedOperationException
*/
+ @Override
public final void setRowCount(int count) {
// Views should defer to their own implementation of
// setRowCount(int, boolean)) per HasRows spec.
throw new UnsupportedOperationException();
}
+ @Override
public void setRowCount(int count, boolean isExact) {
if (count == getRowCount() && isExact == isRowCountExact()) {
return;
@@ -869,6 +899,7 @@
RowCountChangeEvent.fire(display, count, isExact);
}
+ @Override
public void setRowData(int start, List<? extends T> values) {
int valuesLength = values.size();
int valuesEnd = start + valuesLength;
@@ -912,6 +943,7 @@
}
}
+ @Override
public void setSelectionModel(final SelectionModel<? super T> selectionModel) {
clearSelectionModel();
@@ -919,6 +951,7 @@
this.selectionModel = selectionModel;
if (selectionModel != null) {
selectionHandler = selectionModel.addSelectionChangeHandler(new SelectionChangeEvent.Handler() {
+ @Override
public void onSelectionChange(SelectionChangeEvent event) {
// Ensure that we resolve selection.
ensurePendingState();
@@ -933,16 +966,19 @@
/**
* @throws UnsupportedOperationException
*/
+ @Override
public final void setVisibleRange(int start, int length) {
// Views should defer to their own implementation of setVisibleRange(Range)
// per HasRows spec.
throw new UnsupportedOperationException();
}
+ @Override
public void setVisibleRange(Range range) {
setVisibleRange(range, false, false);
}
+ @Override
public void setVisibleRangeAndClearData(Range range,
boolean forceRangeChangeEvent) {
setVisibleRange(range, true, forceRangeChangeEvent);
@@ -1049,6 +1085,7 @@
* existing finally commands (such as SelectionModel commands).
*/
pendingStateCommand = new ScheduledCommand() {
+ @Override
public void execute() {
// Verify that this command was the last one scheduled.
if (pendingStateCommand == this) {
@@ -1418,6 +1455,9 @@
pending.keyboardStealFocus);
}
}
+ } catch (Error e) {
+ // Force the error into the dev mode console.
+ throw new RuntimeException(e);
} finally {
/*
* We are done resolving state, so unlock the rendering loop. We unlock
diff --git a/user/test/com/google/gwt/user/cellview/client/CellTreeTest.java b/user/test/com/google/gwt/user/cellview/client/CellTreeTest.java
index e5b43fb..05b6ae4 100644
--- a/user/test/com/google/gwt/user/cellview/client/CellTreeTest.java
+++ b/user/test/com/google/gwt/user/cellview/client/CellTreeTest.java
@@ -15,6 +15,8 @@
*/
package com.google.gwt.user.cellview.client;
+import com.google.gwt.cell.client.TextCell;
+import com.google.gwt.view.client.ListDataProvider;
import com.google.gwt.view.client.TreeViewModel;
/**
@@ -26,6 +28,32 @@
super(false);
}
+ public void testRefreshEmptyNode() {
+ // An empty data provider.
+ final ListDataProvider<String> provider = new ListDataProvider<String>();
+ TreeViewModel model = new TreeViewModel() {
+ @Override
+ public NodeInfo<?> getNodeInfo(Object value) {
+ TextCell cell = new TextCell();
+ return new DefaultNodeInfo<String>(provider, cell);
+ }
+
+ @Override
+ public boolean isLeaf(Object value) {
+ return false;
+ }
+ };
+
+ // Create the tree.
+ CellTree tree = createAbstractCellTree(model, null);
+ tree.rootNode.listView.presenter.flush();
+
+ // Refresh the empty list.
+ provider.refresh();
+ provider.flush();
+ tree.rootNode.listView.presenter.flush();
+ }
+
/**
* Test that replacing a subset of children updates both the TreeNode value
* and the underlying DOM correctly.
