Please add test cases (or a TODO) for ValueListBox and anything else that is reasonably stable.
http://gwt-code-reviews.appspot.com/780802/diff/3001/4002 File bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/employee/EmployeeEditActivity.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4002#newcode88 bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/employee/EmployeeEditActivity.java:88: super.start(display, eventBus); Putting the super call here could introduce some inderminability if findEmployeeEntries() comes back synchronously. Can you move it above the async request? http://gwt-code-reviews.appspot.com/780802/diff/3001/4008 File bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditActivity.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4008#newcode88 bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditActivity.java:88: super.start(display, eventBus); Putting the super call here could introduce some inderminability if findEmployeeEntries() comes back synchronously. Can you move it above the async request? http://gwt-code-reviews.appspot.com/780802/diff/3001/4008#newcode96 bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditActivity.java:96: Extra newline. http://gwt-code-reviews.appspot.com/780802/diff/3001/4018 File user/src/com/google/gwt/app/place/AbstractRecordEditActivity.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4018#newcode95 user/src/com/google/gwt/app/place/AbstractRecordEditActivity.java:95: extra spaces http://gwt-code-reviews.appspot.com/780802/diff/3001/4021 File user/src/com/google/gwt/app/place/PropertyColumn.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4021#newcode30 user/src/com/google/gwt/app/place/PropertyColumn.java:30: * A column that displays a record property as a string. NB: Property objects What does NB mean? http://gwt-code-reviews.appspot.com/780802/diff/3001/4025 File user/src/com/google/gwt/requestfactory/client/impl/RecordJsoImpl.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4025#newcode315 user/src/com/google/gwt/requestfactory/client/impl/RecordJsoImpl.java:315: assert(null != requestFactory) : "rf"; Can you put a more descriptive error string: "requestFactory is null" http://gwt-code-reviews.appspot.com/780802/diff/3001/4025#newcode317 user/src/com/google/gwt/requestfactory/client/impl/RecordJsoImpl.java:317: assert(null != schema) : "schema"; ditto http://gwt-code-reviews.appspot.com/780802/diff/3001/4025#newcode319 user/src/com/google/gwt/requestfactory/client/impl/RecordJsoImpl.java:319: assert(null != token) : "token"; ditto http://gwt-code-reviews.appspot.com/780802/diff/3001/4026 File user/src/com/google/gwt/user/client/ui/ValueListBox.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4026#newcode35 user/src/com/google/gwt/user/client/ui/ValueListBox.java:35: private final Map<Integer, T> indexToValue = new HashMap<Integer, T>(); You could just use a List (or an array) since you know the values aren't sparse and start at 0. http://gwt-code-reviews.appspot.com/780802/diff/3001/4026#newcode36 user/src/com/google/gwt/user/client/ui/ValueListBox.java:36: private final Map<String, Integer> renderedToIndex = new HashMap<String, Integer>(); What if two items render to the same thing? This should be valuesToIndex. Alternatively, you can replace both of these maps with a List<T> values. indexToValue => values.get(i) valueToIndex => values.indexOf(value) http://gwt-code-reviews.appspot.com/780802/diff/3001/4026#newcode74 user/src/com/google/gwt/user/client/ui/ValueListBox.java:74: public void setValues(Collection<T> values) { This method name is confusing because a native ListBox actually supports multiple selection. We should rename the method setContrainedValues() to match the interface name HasConstrainedValues(). http://gwt-code-reviews.appspot.com/780802/diff/3001/4026#newcode102 user/src/com/google/gwt/user/client/ui/ValueListBox.java:102: Integer index = renderedToIndex.get(rendered); What if two items render to the same thing? http://gwt-code-reviews.appspot.com/780802/diff/3001/4026#newcode104 user/src/com/google/gwt/user/client/ui/ValueListBox.java:104: addValue(value); This violates the contract of HasConstrainedValues. If the value is not in the set of contrained values, either throw an error, or set the selected index to -1 to deselect everything. I'm guessing that you want to support adding a value that is no longer selectable (a legacy value for example). In that case, you should provide a boolean setter to make the list box values unconstrained. isUnconstrainedValueAllowed setUnconstrainedValueAllowed http://gwt-code-reviews.appspot.com/780802/diff/3001/4027 File user/test/com/google/gwt/valuestore/server/SimpleFoo.java (right): http://gwt-code-reviews.appspot.com/780802/diff/3001/4027#newcode19 user/test/com/google/gwt/valuestore/server/SimpleFoo.java:19: import com.google.gwt.valuestore.shared.SimpleBarRecord; Unused import? http://gwt-code-reviews.appspot.com/780802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
