Responding to some offline questions: In order to have these parsers executed before CellPanelParser, I had to > create a parser for HasAlignment. I see your point about classes that only > use HasHorizontalAlignment or HasVerticalAlignment. I could register those > 2 parsers as well, but that means they get executed twice for anything that > implements HasAlignment. No real harm done since those attributes will be > consumed by the first pass, but that seems sloppy. I can inline them into > HasAlignment, though that is subpar for people who use HasVerticalAlignment > and HasHorizontalAlignment independently.
I don't see anything wrong with inlining them if they won't actually fix anything. Beyond the ordering issue, they add no value that isn't already in BeanParser, right? Nothing in the stock widget set implements HasVerticalAlignment directly, so HasVerticalAlignmentParser has no value to add. Does anything not work with the widgets that implement HasHorizontalAlignment or its child interface, HasAutoHorizontalAlignment? If not, there's no reason to break it HasHorizontalAlignmentParser either. If there is a problem, would HasHorizontalAlignmentParser actually fix it? I could use more guidance on testing. I've added a positive check to my > unit testing (sorry for the miss), and there is already a case to confirm > that nothing is written when alignment args aren't entered. I could use a > hand with the UiBinderTest suite. The one test that I included fails > without the parser and passes with it. Do you have any suggestions for > other tests that should be included? The UiBinderTest is a big, fat, slow integration test. What you have there looks exactly right to me, just enough to make sure the whole stack works. Getting the bulk of your coverage from the unit tests by adding the couple of happy paths I mentioned should be plenty. On Mon, Nov 15, 2010 at 2:08 PM, <rj...@google.com> wrote: > > http://gwt-code-reviews.appspot.com/1109801/diff/1/2 > File > user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java > (right): > > http://gwt-code-reviews.appspot.com/1109801/diff/1/2#newcode24 > user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:24: > * Parses widgets that inherit from {...@link > com.google.gwt.user.client.ui.HasHorizontalAlignment}. > wrong class linked > > http://gwt-code-reviews.appspot.com/1109801/diff/1/3 > File > > user/src/com/google/gwt/uibinder/elementparsers/HasHorizontalAlignmentParser.java > (right): > > http://gwt-code-reviews.appspot.com/1109801/diff/1/3#newcode32 > > user/src/com/google/gwt/uibinder/elementparsers/HasHorizontalAlignmentParser.java:32: > writer.getDesignTime().handleUIObject(writer, elem, fieldName); > why? > > http://gwt-code-reviews.appspot.com/1109801/diff/1/4 > File > > user/src/com/google/gwt/uibinder/elementparsers/HasVerticalAlignmentParser.java > (right): > > http://gwt-code-reviews.appspot.com/1109801/diff/1/4#newcode32 > > user/src/com/google/gwt/uibinder/elementparsers/HasVerticalAlignmentParser.java:32: > writer.getDesignTime().handleUIObject(writer, elem, fieldName); > ditto > > http://gwt-code-reviews.appspot.com/1109801/diff/1/5 > File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java > (right): > > http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1000: > addWidgetParser("HasAlignment"); > It won't fire for widgets that implement only HasHorizontalAlignment or > HasVerticalAlignment, which is not a small set. Is that okay? > > If so, I'd inline the other two parsers into HasAlignment, or someone > will wonder why the more specific ones don't always fire. Should also > document in HasAlignmentParser why it's necessary (link to the issue), > since it's redundant with BeanParser. > > http://gwt-code-reviews.appspot.com/1109801/diff/1/6 > File > > user/test/com/google/gwt/uibinder/elementparsers/HasAlignmentParserTest.java > (right): > > http://gwt-code-reviews.appspot.com/1109801/diff/1/6#newcode2 > > user/test/com/google/gwt/uibinder/elementparsers/HasAlignmentParserTest.java:2: > * Copyright 2009 Google Inc. > 2010 > > http://gwt-code-reviews.appspot.com/1109801/diff/1/6#newcode38 > > user/test/com/google/gwt/uibinder/elementparsers/HasAlignmentParserTest.java:38: > } > Should test success case as well. And should test that the two > attributes are optional. > > http://gwt-code-reviews.appspot.com/1109801/diff/1/7 > File user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java > (right): > > http://gwt-code-reviews.appspot.com/1109801/diff/1/7#newcode579 > user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java:579: > "class=\"gwt-StackPanelItem"); > This fails w/o the new parser? > > > http://gwt-code-reviews.appspot.com/1109801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
