[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.