[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
r8387 On Wed, Jul 14, 2010 at 12:32 PM, rj...@google.com wrote: Looks good, will submit On 2010/07/12 20:14:38, markovuksanovic wrote: Some error checking changes... http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Reverted, breaks Grid subclasses. Oops, working on the fix. On Fri, Jul 16, 2010 at 2:42 PM, Ray Ryan rj...@google.com wrote: r8387 On Wed, Jul 14, 2010 at 12:32 PM, rj...@google.com wrote: Looks good, will submit On 2010/07/12 20:14:38, markovuksanovic wrote: Some error checking changes... http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Nice. Just some error checking nits left. http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001
File user/src/com/google/gwt/uibinder/elementparsers/GridParser.java
(right):
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode86
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:86:
writer.getOracle().findType(Grid.class.getName()), 0, 0);
This line serves no purpose that I can see.
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode130
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:130:
writer.die(Grid's lt;g:row tag in %s may only contain %s or %s
element.,
No need for lt; here, and g: shouldn't be hard coded
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode149
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:149:
throws UnableToCompleteException {
You're not enforcing that the child elements have the same prefix as the
parent. (Note that you should not hardcode g:, just make sure that
they all have the same value for XmlElement#getPrefix.)
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode152
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:152:
String tagName = child.getLocalName();
Compare prefix too
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode154
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:154:
writer.die(Invalid Grid child element: + tagName);
%1$s:Grid elements must contain only %1$s:%2$s children, found
%3$s:%4$s, elem.getPrefix(), ROW_TAG, child.getPrefix(), tagName
http://gwt-code-reviews.appspot.com/154810/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Did the changes... Will post the patch a little later... I need to
convert it from git to svn format first...
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001
File user/src/com/google/gwt/uibinder/elementparsers/GridParser.java
(right):
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode86
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:86:
writer.getOracle().findType(Grid.class.getName()), 0, 0);
You should be right about this
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode86
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:86:
writer.getOracle().findType(Grid.class.getName()), 0, 0);
On 2010/07/12 17:07:40, Ray Ryan wrote:
This line serves no purpose that I can see.
Done.
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode130
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:130:
writer.die(Grid's lt;g:row tag in %s may only contain %s or %s
element.,
On 2010/07/12 17:07:40, Ray Ryan wrote:
No need for lt; here, and g: shouldn't be hard coded
Done.
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode149
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:149:
throws UnableToCompleteException {
Done. Also made sure that cells have the same prefix as parent.
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode152
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:152:
String tagName = child.getLocalName();
On 2010/07/12 17:07:40, Ray Ryan wrote:
Compare prefix too
Done.
http://gwt-code-reviews.appspot.com/154810/diff/63002/70001#newcode154
user/src/com/google/gwt/uibinder/elementparsers/GridParser.java:154:
writer.die(Invalid Grid child element: + tagName);
On 2010/07/12 17:07:40, Ray Ryan wrote:
%1$s:Grid elements must contain only %1$s:%2$s children, found
%3$s:%4$s,
elem.getPrefix(), ROW_TAG, child.getPrefix(), tagName
Done.
http://gwt-code-reviews.appspot.com/154810/diff/63002/70003
File user/src/com/google/gwt/user/client/ui/Grid.java (right):
http://gwt-code-reviews.appspot.com/154810/diff/63002/70003#newcode35
user/src/com/google/gwt/user/client/ui/Grid.java:35: * Grid widget
consists of lt;g:row elements. Each lt;g:row element
On 2010/07/03 20:46:51, markovuksanovic wrote:
One small typo - can contain on or more - should be - can contain
one or
more
Done.
http://gwt-code-reviews.appspot.com/154810/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
ping... http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Changed the way in which uibinder xml file is parsed. http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
I just found one small typo - but I won't commit a new patch now just in case something else needs to be fixed as well. @rjrjr: If everything else is ok, I'll commit an updated patch. http://gwt-code-reviews.appspot.com/154810/diff/63002/70003 File user/src/com/google/gwt/user/client/ui/Grid.java (right): http://gwt-code-reviews.appspot.com/154810/diff/63002/70003#newcode35 user/src/com/google/gwt/user/client/ui/Grid.java:35: * Grid widget consists of lt;g:row elements. Each lt;g:row element One small typo - can contain on or more - should be - can contain one or more http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
I just found one small typo - but I won't commit a new patch now just in case something else needs to be fixed as well. @rjrjr: If everything else is ok, I'll commit an updated patch. http://gwt-code-reviews.appspot.com/154810/diff/63002/70003 File user/src/com/google/gwt/user/client/ui/Grid.java (right): http://gwt-code-reviews.appspot.com/154810/diff/63002/70003#newcode35 user/src/com/google/gwt/user/client/ui/Grid.java:35: * Grid widget consists of lt;g:row elements. Each lt;g:row element One small typo - can contain on or more - should be - can contain one or more http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Remember this? It's really close, I think just one more change is needed (below). http://gwt-code-reviews.appspot.com/154810/diff/22003/54004 File user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java (right): http://gwt-code-reviews.appspot.com/154810/diff/22003/54004#newcode114 user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java:114: fieldName.resizeColumns(2);, All these redundant calls to resizeColumns and resizeRows are troubling. It would be better to emit a single call. http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Yeah, I do remember. I will make the change later today. On 2010/07/02 14:40:57, Ray Ryan wrote: Remember this? It's really close, I think just one more change is needed (below). http://gwt-code-reviews.appspot.com/154810/diff/22003/54004 File user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java (right): http://gwt-code-reviews.appspot.com/154810/diff/22003/54004#newcode114 user/test/com/google/gwt/uibinder/elementparsers/GridParserTest.java:114: fieldName.resizeColumns(2);, All these redundant calls to resizeColumns and resizeRows are troubling. It would be better to emit a single call. http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
Ok, I had a look and just remembered why I used more calls to resize methods. The GridParser walks through the Grid and on the fly calculates the size. So if it comes to an element that doesn't fit into the current grid element it expands it (either by number of columns, or rows - depends which one is necessary). I thought it would be better to calculate the grid size as I walk through the grid compared to walking it first time to get the size and then once more to populate the elements. So if we had a grid like this 1, 2, 4 5, 6 7, 8, 9, 10 The method would make three calls to resize columns when walking the first row. Then expand to fit the next row and then add elements as along as their number is less then 3. (as the current size of the grid is 2, 3). The there would be one more resize rows call to expand for the next row and one more resize columns call to resize columns to fit in number 10. So in the end you would end up with grid size 3, 4. Does that make sense? http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
You're trading off compile time simplicity at the expense of runtime efficiency, and it's a bad trade. Rather than emitting java lines as you walk the XML, you can build a model of what you want to write. Once you're done with your traversal, you can write out a single call to resize(int, int), and then write the rest. On Fri, Jul 2, 2010 at 12:20 PM, markovuksano...@gmail.com wrote: Ok, I had a look and just remembered why I used more calls to resize methods. The GridParser walks through the Grid and on the fly calculates the size. So if it comes to an element that doesn't fit into the current grid element it expands it (either by number of columns, or rows - depends which one is necessary). I thought it would be better to calculate the grid size as I walk through the grid compared to walking it first time to get the size and then once more to populate the elements. So if we had a grid like this 1, 2, 4 5, 6 7, 8, 9, 10 The method would make three calls to resize columns when walking the first row. Then expand to fit the next row and then add elements as along as their number is less then 3. (as the current size of the grid is 2, 3). The there would be one more resize rows call to expand for the next row and one more resize columns call to resize columns to fit in number 10. So in the end you would end up with grid size 3, 4. Does that make sense? http://gwt-code-reviews.appspot.com/154810/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
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: *
lt;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: * lt;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.
[gwt-contrib] Re: Add support for creating Grid elements using UiBinder. (issue154810)
One other note: your patch is very likely to land after http://gwt-code-reviews.appspot.com/241801, and may need to be tweaked. Take a look at the element parser changes there to get a feel for it. 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.
