C'mon, give me a little credit; I wouldn't suggest a change without having tested to prove that it works:) I changed HorizontalPanel to implement HasHorizontalAlignment and HasVerticalAlignment instead of implementing HasAlignment. The parsers generate in the correct order and the resulting code passes all tests with this change (yes, it fails with the existing code).
Are you sure this feature is being exercised by the current widget set without issue? --Stephanie On Thu, Nov 18, 2010 at 5:24 PM, Ray Ryan <[email protected]> wrote: > > > On Thu, Nov 18, 2010 at 11:59 AM, <[email protected]> wrote: > >> >> http://gwt-code-reviews.appspot.com/1121801/diff/1/2 >> File >> user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java >> (right): >> >> http://gwt-code-reviews.appspot.com/1121801/diff/1/2#newcode34 >> >> user/src/com/google/gwt/uibinder/elementparsers/HasAlignmentParser.java:34: >> public class HasAlignmentParser implements ElementParser { >> On 2010/11/18 19:45:35, rjrjr wrote: >> >>> On 2010/11/18 19:34:50, sbrubaker wrote: >>> > On 2010/11/18 18:22:17, rjrjr wrote: >>> > > No reason for these to be inner classes. >>> > >>> > These classes were inlined per your comment at >>> > http://gwt-code-reviews.appspot.com/1109801/diff/1/5#newcode1000, >>> >> >> Yes, but they're not as inlined as they sensibly could be. >>> >> >> > but on further >>> > thought I think it makes more sense to keep them as separate classes >>> >> and >> >>> > register these parsers as well. This solves the alignment issue for >>> >> any class >> >>> > that implements HasHorizontalAlignment or HasVerticalAlignment, and >>> >> there's no >> >>> > harm done to classes that implement HasAlignment (since the relevant >>> attributes >>> > are consumed by the first parser pass). >>> >> >> My comment there suggested doing that iff it would actually solve any >>> >> problems. >> >>> The fact that you had to create HasAlignment to solve the actual bug >>> >> at hand >> >>> made me wonder. >>> >> >> What will be fixed by the existence of HasHorizontalAlignment and >>> HasVerticalAlignment? >>> >> >> For any class that implements HasHorizontalAlignment or >> HasVerticalAlignment rather than HasAlignment, the horizontalAlignment >> and verticalAlignment tags are effectively useless, for the same reason >> as HasAlignment. Registering these parsers will fix the parse order for >> classes that implement the Has*Alignment classes directly. >> >> >> http://gwt-code-reviews.appspot.com/1121801/show >> > > We have lots of widgets that implement HasHorizontalAlignment directly. All > I'm asking is if you have confirmed that they are broken now, and fixed by > this parser. It doesn't look like it to me. (Mmmmm, empiricism.) > > I take your point that any future panel that imitates the nasty > order-dependency of the three HasAlignment panels would be fixed by this. > But there aren't any that I can see, and I really dislike speculative > coding. > > rjrjr > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
