The general direction of the patch looks great to me.
@jat, I'll be on vacation from tomorrow through Thanksgiving. Will you be able to land this for Thomas, and get it on to the 2.1.1 branch while I'm gone? http://gwt-code-reviews.appspot.com/1099801/diff/26001/27004 File user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java (right): http://gwt-code-reviews.appspot.com/1099801/diff/26001/27004#newcode36 user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java:36: static final String NO_TIMEZONE_WITHOUT_SPECIFIED_FORMAT = "May not specify a time zone if no format is given."; Sounds good, and no need to mess with the other tests. On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote: > Why are these not private?
Testing.
(I believe I copied this from another class but can find it back; but
clearly
other classes could benefit from it: LayoutPanelParserTest for
instance could
use the constants from LayoutPanelParser instead of duplicating the
string
values)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27009 File user/src/com/google/gwt/user/client/ui/DateLabel.java (right): http://gwt-code-reviews.appspot.com/1099801/diff/26001/27009#newcode33 user/src/com/google/gwt/user/client/ui/DateLabel.java:33: public DateLabel(DateTimeFormat format) { I see. Yes, if you could punch up the javadoc, something like: Extends ValueLabel for convenience when dealing with dates and DateTimeFormat, especially in UiBinder templates. (Note that this class does not accept renderers. To do so use ValueLabel directly.) The use of DateTimeFormatRenderer really is an implementation detail, shouldn't be metioned in the doc. On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote: > This is bad, how can I give this thing a renderer?
My reasoning was that if you do have a renderer, you don't need a
DateLabel,
just use a ValueLabel<Date> instead. Maybe it should just be
documented? http://gwt-code-reviews.appspot.com/1099801/diff/26001/27013 File user/src/com/google/gwt/user/client/ui/NumberLabel.java (right): http://gwt-code-reviews.appspot.com/1099801/diff/26001/27013#newcode32 user/src/com/google/gwt/user/client/ui/NumberLabel.java:32: public NumberLabel(NumberFormat format) { On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote: > Again, should be Renderer<Number>
Same rationale as for DateLabel: if you have a Renderer<Number> at
hand, just
use a ValueLabel<Number>, the whole point of DateLabel and NumberLabel
is to tie
them to specific renderers.
Quibble: the whole point is to make it convenient to deal w/them and format objects, especially from mark up. The renderer is just an implementation detail. http://gwt-code-reviews.appspot.com/1099801/diff/26001/27014 File user/src/com/google/gwt/user/client/ui/SimpleCheckBox.java (right): http://gwt-code-reviews.appspot.com/1099801/diff/26001/27014#newcode235 user/src/com/google/gwt/user/client/ui/SimpleCheckBox.java:235: // overrides in RadioButton HasValue really is the right thing. I'd hold off on setName, and make editor compatibility the goal for this patch. Surely we can get CheckBox to extend SimpleCheckBox down the road. On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/18 05:46:19, rjrjr wrote: > RadioButton doesn't override this, copy paste error. > > I don't see SimpleRadioButton in this patch, shouldn't I?
Ah oops! Maybe we should just stick with TakesValue instead of
HasValue then, it
would make things much simpler! ;-) (I also note that SimpleCheckBox/SimpleRadioButton don't have a
setName like
CheckBox/RadioButton)
http://gwt-code-reviews.appspot.com/1099801/diff/26001/27025 File user/test/com/google/gwt/uibinder/elementparsers/DateLabelParserTest.java (right): http://gwt-code-reviews.appspot.com/1099801/diff/26001/27025#newcode230 user/test/com/google/gwt/uibinder/elementparsers/DateLabelParserTest.java:230: public void testChokeOnUnknownPredefinedFormat() throws SAXException { On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/15 22:14:37, jat wrote: > Should there be a test that shows predefinedFormat working and
having the
right > output?
Added a testHappyWithPredefinedFormat and testHappyWithTimeZoneOffset.
(I wonder whether there should be explicit tests for other things,
given that
they're implicitly tested in other tests; for instance, timezone is
tested in
testHappyWithSubclassWithDateTimeFormatAndTimeZoneConstructor, and
format is
tested in all testHappy* tests except testHappyWithNoFormat and testHappyWithPredefinedFormat)
I think not. http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026 File user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java (right): http://gwt-code-reviews.appspot.com/1099801/diff/26001/27026#newcode145 user/test/com/google/gwt/uibinder/elementparsers/NumberLabelParserTest.java:145: b.append("<g:NumberLabel predefinedFormat='CURRENCY'>"); On 2010/11/18 10:17:55, tbroyer wrote:
On 2010/11/15 22:14:37, jat wrote: > I like the parallelism with DateTimeFormat, and we probably should
make a
> similar change to it that includes a PredefinedFormat enum.
I agree. Do you want me to create another review with this change?
(and update
this one rely on it; or rather include the NumberFormat change right
into this
review?) There's a difference with DateLabelParser though, in that PredefinedFormat.CURRENCY would still have to be special-cased in the NumberLabelParser, at least when used with a currency code.
These changes all sound good to me if jat is on board. http://gwt-code-reviews.appspot.com/1099801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
