Ok, I think I have some more useful feedback now. Nice document, I think we must add support in RenderablePanel to have UiRenderer objects added to them, that'd be awesome. Components that don't have to be widgets :-)
http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java File user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java (right): http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java#newcode279 user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java:279: && !resourceType.getErasedType().equals(matchingResourceType.getErasedType())) { do you need equals? Could it just be assignable? http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java#newcode284 user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java:284: resourceType = matchingResourceType.getErasedType(); you don't need to do this, right? You could just pass matchingResourceType.getErasedType() to registerField below. I'm just mentioning because it throw me off a bit and I kept trying to find where you used this value afterwards and overlooked the "return" http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java#newcode295 user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java:295: if (writer.getOwnerClass() != null How about breaking each of these if blocks into its own method? This is getting really hard to follow. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java#newcode295 user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java:295: if (writer.getOwnerClass() != null Can we return or die here if writer.getOwnerClass() == null? It would make the following code simpler (and the old code seems to assume it's != null. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java#newcode423 user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java:423: private JClassType findRenderParameterType(String resourceName) { part of this logic is repeated in UiBinderWriter.findRenderParameters(). In particular there you allow multiple render() methods as long as only one of them has SafeHtmlBuilder as the first parameter. Here you make no such check. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (right): http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode458 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:458: * @return that variable's name. @param fieldName ... http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode460 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:460: public String declareDomIdHolder(String fieldName) throws UnableToCompleteException { Don't know if you guys use @Nullable much in GWT code, but this would be a good place to put that. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1094 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1094: private List<JMethod> findGetterNames(JClassType owner) { this method could be static. In fact, how do you guys feel about adding a utility class to this directory. UiBinderWriter is huge already, maybe we can clean it up a bit.. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1379 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1379: private String renderMethodParameters(JParameter[] renderParameters) { this too could be static. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1706 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1706: fieldManager.initializeWidgetsInnerClass(w, getOwnerClass()); Yeah, I think it is. What this does is generate the block of "build_fieldX();" calls that initialize all fields you've declared. I think you need it here. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1721 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1721: writeRenderParameterDefinitions(w, renderParameters); wouldn't it be better to declare these before the render() method? Just because that's probably how we'd right it if it was "normal" code. http://gwt-code-reviews.appspot.com/1466809/diff/6001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1745 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1745: w.write("return (%s) UiRendererUtils.findInnerField(%s, \"%s\", RENDERED_ATTRIBUTE);", Do you know about LazyDomElement (https://csearch.corp.google.com/#google3/third_party/java_src/gwt/svn/trunk/user/src/com/google/gwt/uibinder/client/LazyDomElement.java&q=LazyDomElement) ? I might be missing something, but I think you could make things simpler if you declared the elements you need to return via getters as LazyDomElements. Taht way you don't need a special way to build their IDs and the generated code for the getter would be a simple lazyDomElement.get(); http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
