Thanks, replies inline below. I'll duck all the hard suggestions, do the easy ones and submit.
http://gwt-code-reviews.appspot.com/77805/diff/1/4 File user/src/com/google/gwt/uibinder/rebind/BundleWriter.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/4#newcode41 Line 41: private final JClassType strictAnnotationType; Yup. But I'm hoping to beat you in. http://gwt-code-reviews.appspot.com/77805/diff/1/4#newcode82 Line 82: writer.write("import %s;", strictAnnotationType.getQualifiedSourceName()); Agree, not quite there yet though. http://gwt-code-reviews.appspot.com/77805/diff/1/4#newcode111 Line 111: writeImageOptionsAnnotation(image.getFlipRtl(), image.getRepeatStyle()); Not off the top of my head, unless we adopt some kind of convention of annotations on your annotations--seems like a lot of bother. Unless the pace of change there gets pretty rapid, I'm comfortable maintaining this by hand. http://gwt-code-reviews.appspot.com/77805/diff/1/5 File user/src/com/google/gwt/uibinder/rebind/GetEscapedInnerTextVisitor.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/5#newcode52 Line 52: // TODO(jgw): write this back just as it came in. Not so far. http://gwt-code-reviews.appspot.com/77805/diff/1/5#newcode75 Line 75: parent != null && "pre".equals(parent.getTagName()); Interesting question. If it did I think they'd be no-ops--scripts don't fire as a side effect of setting innerHTML as I recall. Still, you could imagine someone putting a script here and fishing it out for later use. Anyway, this whole escaping mechanism is a disaster, and I'll have to rewrite it soon (post MS2) to fix a few bugs. I'll add a todo here to think about the script case at rewrite time. http://gwt-code-reviews.appspot.com/77805/diff/1/8 File user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/8#newcode121 Line 121: if (elem.hasAttribute(FLIP_RTL_ATTRIBUTE)) { See above. I'd rather not deal with that right now unless you see a quick and simple approach I'm missing. http://gwt-code-reviews.appspot.com/77805/diff/1/9 File user/src/com/google/gwt/uibinder/rebind/XMLElement.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/9#newcode347 Line 347: writer.die("%s must contain only text", this); Not that I've encountered. http://gwt-code-reviews.appspot.com/77805/diff/1/10 File user/src/com/google/gwt/uibinder/rebind/model/ImplicitClientBundle.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/10#newcode73 Line 73: * Called to declare a new CssResource accessor on this bundle. Mmmm, copy paste. Fixed, thanks. http://gwt-code-reviews.appspot.com/77805/diff/1/14 File user/src/com/google/gwt/uibinder/sample/client/WidgetBasedUi.ui.xml (right): http://gwt-code-reviews.appspot.com/77805/diff/1/14#newcode187 Line 187: Well of course you do. Who wouldn't? Looks good to me too! http://gwt-code-reviews.appspot.com/77805/diff/1/16 File user/test/com/google/gwt/uibinder/rebind/XMLElementTest.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/16#newcode73 Line 73: // TODO(rjrjr) Can't test this until die() is factored out of UiBinderWriter Yes, the TODO is a bit misleading. I've refactored die out of the way, but XMLElement will still depend upon UiBinderWriter and I still can't mock that effectively. Will update the TODO http://gwt-code-reviews.appspot.com/77805 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
