Not sure about the test details (documenting the differences with
ValueListBox which are another issue –that might need to be addressed
too, but independently as this one) but overall LGTM.
Be sure to see my comment on getValue() though.


http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/src/com/google/gwt/user/client/ui/ValuePicker.java
File user/src/com/google/gwt/user/client/ui/ValuePicker.java (right):

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode116
user/src/com/google/gwt/user/client/ui/ValuePicker.java:116: if
((current == value) || (current != null && current.equals(value))) {
Hmm, there might be a need to use the key provider here instead of
equals() (which kind-of assumes a SimpleKeyProvider).

We're actually mimicking the SingleSelectionModel, with the exception of
the deferred SelectionChangeEvent.

Note that we could also probably get rid of the 'value' field and use
smodel.getSelectedObject(). But I don't think this change is worth the
possible bugs it could also introduce ;-)

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/test/com/google/gwt/user/client/ui/ValuePickerTest.java
File user/test/com/google/gwt/user/client/ui/ValuePickerTest.java
(right):

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/test/com/google/gwt/user/client/ui/ValuePickerTest.java#newcode57
user/test/com/google/gwt/user/client/ui/ValuePickerTest.java:57: return
item.value;
Handle null value?

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/test/com/google/gwt/user/client/ui/ValuePickerTest.java#newcode173
user/test/com/google/gwt/user/client/ui/ValuePickerTest.java:173:
assertEquals(keyProvider.getKey(value),
keyProvider.getKey(subject.getValue()));
There shouldn't be a need for the KeyProvider here: why not simply
assertEquals(value, subject.getValue())?
(or if you use the key provider, use it for the
smodel.getSelectedObject() too)

http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to