Thanks for your patience, Marko, and sorry to be so inconsiderate of
your time.


http://gwt-code-reviews.appspot.com/154810/diff/1001/1002
File user/src/com/google/gwt/uibinder/elementparsers/GridParser.java
(right):

http://gwt-code-reviews.appspot.com/154810/diff/1001/1002#newcode37
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:37:
private String maxColumns;
There is no reason for these to be instance variables, should be local
to parse

http://gwt-code-reviews.appspot.com/154810/diff/1001/1002#newcode42
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:42:
maxColumns = elem.consumeRequiredIntAttribute(COLUMNS_ATTRIBUTE);
Neither of these should be required. We can just count up the rows and
columns for them.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1002#newcode48
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:48: if
(row >= Integer.parseInt(maxRows)) {
Should not parse this over and over again.

Also, maxRows could well be a field reference, so you won't know its
actual value at compile time. Use
FieldReferenceConverter.hasFieldReferences(String) to judge if you can
even make the check. (If it's not a field ref, it is definitely a
parseable int literal or an exception will have been thrown by
XMLElement.consumeIntAttribute, presuming that is correctly written to
use IntAttributeParser.)

http://gwt-code-reviews.appspot.com/154810/diff/1001/1002#newcode59
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:59:
private void parseRow(XMLElement elem, String fieldName, UiBinderWriter
writer, int row) throws UnableToCompleteException {
Many lines too long here.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1002#newcode63
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:63:
writer.die("Trying to add more rows then specified by rows attribute.");
These are columns, not rows. All the issues above about maxRows apply
here as well.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1004
File user/src/com/google/gwt/uibinder/rebind/XMLElement.java (right):

http://gwt-code-reviews.appspot.com/154810/diff/1001/1004#newcode575
user/src/com/google/gwt/uibinder/rebind/XMLElement.java:575: return
consumeRequiredAttribute(name, getIntType());
As noted in GridParser, you actually don't want it to be required.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1005
File user/src/com/google/gwt/user/client/ui/Grid.java (right):

http://gwt-code-reviews.appspot.com/154810/diff/1001/1005#newcode60
user/src/com/google/gwt/user/client/ui/Grid.java:60: *
<g:textCell>
In other parsers we have consciously not bothered with text support when
there is parallel html support. There's no real point to it. I think you
should just drop g:textCell

http://gwt-code-reviews.appspot.com/154810/diff/1001/1005#newcode67
user/src/com/google/gwt/user/client/ui/Grid.java:67: *  <g:row>
This example is wrong. I can't put HTML straight into a custom cell, it
has to hold a widget, right?

http://gwt-code-reviews.appspot.com/154810/diff/1001/1006
File
user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java
(right):

http://gwt-code-reviews.appspot.com/154810/diff/1001/1006#newcode2
user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java:2:
* Copyright 2009 Google Inc.
2010

http://gwt-code-reviews.appspot.com/154810/diff/1001/1006#newcode28
user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java:28:
* A unit test. Guess what of.
"Eponymous unit test." I'm trying to get less snarky.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1006#newcode42
user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java:42:
* Test with <g:customCell> tag.
wrong, might as well delete the javadoc.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1006#newcode176
user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java:176:
public void testValidChild() throws UnableToCompleteException,
SAXException,
Should add a test that field refs in rows and columns work.

http://gwt-code-reviews.appspot.com/154810/diff/1001/1009
File user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.ui.xml
(right):

http://gwt-code-reviews.appspot.com/154810/diff/1001/1009#newcode608
user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.ui.xml:608:
<gwt:cell>
Shouldn't these be custom cells? What's actually showing up in the
output?

http://gwt-code-reviews.appspot.com/154810/show

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

To unsubscribe from this group, send email to 
google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with 
the words "REMOVE ME" as the subject.

Reply via email to