Re: [gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
I've shared a publicly visible copy here: https://docs.google.com/document/d/1Oo_imxskoGX5O9l9LhHDtJ0yJkHvNTNQqU3dkkekZYI/edit?hl=en_US Does that work for you? On Fri, Jul 8, 2011 at 3:22 PM, stephen.haber...@gmail.com wrote: Is it okay to make that public? I think it is OK. We usually publish them in the Wiki, but Docs is getting so good that I wanted to try it this way. Using docs makes sense. Will it eventually be made public? Perhaps copy/pasted over into the wiki for posterity? http://gwt-code-reviews.**appspot.com/1466809/http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/**group/Google-Web-Toolkit-**Contributorshttp://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
https://docs.google.com/document/d/1Oo_imxskoGX5O9l9LhHDtJ0yJkHvNTNQqU3dkkekZYI/edit?hl=en_US Does that work for you? Awesome! Yep, it works. Thanks. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/diff/3020/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java
File user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/3020/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java#newcode38
user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java:38:
public boolean isParentOrRenderer(Element parent) {
Can this use UiRendererUtilsImpl#findRootElement()? Seems like the code
is very similar.
http://gwt-code-reviews.appspot.com/1466809/diff/3020/user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/3020/user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java#newcode155
user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java:155:
int endOfFirstTag = html.indexOf();
This still needs to handle self closing (ends in /) Strings:
div id=placeholder /
You can copy the code from RenderableStamper.stamp():
if (html.charAt(endOfFirstTag - 1) == '/') {
endOfFirstTag--;
}
http://gwt-code-reviews.appspot.com/1466809/diff/3020/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/3020/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1441
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1441:
jMethod.getParameterTypes().length = 1
You should also handle the case where render() is declared without any
parameters. This conditional would skip over the no-arg case without
throwing an error.
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
LGTM http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Is it okay to make that public? I think it is OK. We usually publish them in the Wiki, but Docs is getting so good that I wanted to try it this way. Using docs makes sense. Will it eventually be made public? Perhaps copy/pasted over into the wiki for posterity? http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf
File tools/api-checker/config/gwt23_24userApi.conf (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode58
tools/api-checker/config/gwt23_24userApi.conf:58:
:user/src/com/google/gwt/uibinder/client/UiRendererUtils.java\
On 2011/07/01 14:10:48, jlabanca wrote:
This doesn't match the other lines. It shouldn't be necessary to
exclude new
classes from the excludedFiles_old list.
Done.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
On 2011/07/01 20:59:37, rdcastro wrote:
You should add a blank line before @param
Done.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode25
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:25: public
class UiRendererUtils {
On 2011/07/01 14:10:48, jlabanca wrote:
If this is an impl class, you should make it package protected or at
least
append Impl to the class name so nobody uses it.
Done. It needs to be public to be accessible from generated code.
Appended Impl to the name instead.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode38
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:38: *
Retrieves a specific element within a previously rendered elements.
On 2011/07/01 20:59:37, rdcastro wrote:
/s/elements./element./
Done.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode41
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:41: *
@param fieldName name of the field to retrieve
On 2011/07/01 20:59:37, rdcastro wrote:
you forgo the javadoc for attribute (I was actually curious about that
one :-) )
Done. UiRendere'd code passes AbstractUiRenderer#RENDERED_ATTRIBUTE
(i.e. gwtuirendered) as a parameter. Should I use some other attrbute
name? (i.e. with underscores)
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode45
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:45: Element
root = findRootElement(parent, attribute);
Yes, the root element is needed. A Cell the user will probably have the
parent at hand. But we need the root. It Contains a special attribute
which holds the first half of the id of every element in the rendered
structure.
Then to get a field, it is matter of composing it with the field name.
On 2011/07/01 20:59:37, rdcastro wrote:
Do you really need the root element? Couldn't you just check if parent
is
attached?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode47
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:47: return
null;
On 2011/07/01 14:10:48, jlabanca wrote:
Should this throw an IllegalArgumentException? If root is null, it
indicates
that an invalid parent was passed into the method.
Done.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode55
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:55: Element
elementById = Document.get().getElementById(renderedId);
On 2011/07/01 14:10:48, jlabanca wrote:
You can move the assertion below this line, and only run it if
elementById is
null. That way, you can throw a Runtime exception in prod mode, but
you only
walk up the DOM in the error case.
if (elementById == null) {
if (!isAttachedToDom(root)) {
throw new RuntimeException(UiRendered element is not attached to
DOM while
getting \ + fieldName + \);
} else {
throw new IllegalArgumentException(fieldName not found within
rendered
element);
}
}
Done.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode87
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:87: assert
isRenderedElementSingleChild(ret)//
I was trying to tick the Eclipse auto-formatter into
doing-what-I-want(TM). I'll remove it.
On 2011/07/01 20:59:37, rdcastro wrote:
Are the trailing // on purpose?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode88
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
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 ListJMethod 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\,
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Was curious to see what it'd look like; finally have a few comments.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
Why isn't T declared as T extends Cell? then?
http://gwt-code-reviews.appspot.com/1466809/diff/1/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/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1229
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1229: return
firstLetter.toLowerCase() + name.substring(4);
Beware of the turkish locale!
You should use Introspector.decapitalize here
http://download.oracle.com/javase/6/docs/api/java/beans/Introspector.html#decapitalize(java.lang.String)
Or use Character.toLowerCase rather than String.toLowerCase.
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Was curious to see what it'd look like; finally have a few comments. http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Thanks Thomas!.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
On 2011/07/01 10:18:30, tbroyer wrote:
Why isn't T declared as T extends Cell? then?
In the end we figured it would be good to let people render for purposes
other than Cell widgets. The documentation (mistakenly) still gives the
impression that this is exclusively Cell related, though.
http://gwt-code-reviews.appspot.com/1466809/diff/1/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/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1229
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1229: return
firstLetter.toLowerCase() + name.substring(4);
On 2011/07/01 10:18:30, tbroyer wrote:
Beware of the turkish locale!
You should use Introspector.decapitalize here
http://download.oracle.com/javase/6/docs/api/java/beans/Introspector.html#decapitalize%28java.lang.String)
Or use Character.toLowerCase rather than String.toLowerCase.
Thanks! I was not aware of this.
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf
File tools/api-checker/config/gwt23_24userApi.conf (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode58
tools/api-checker/config/gwt23_24userApi.conf:58:
:user/src/com/google/gwt/uibinder/client/UiRendererUtils.java\
This doesn't match the other lines. It shouldn't be necessary to exclude
new classes from the excludedFiles_old list.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode25
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:25: public
class UiRendererUtils {
If this is an impl class, you should make it package protected or at
least append Impl to the class name so nobody uses it.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode47
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:47: return
null;
Should this throw an IllegalArgumentException? If root is null, it
indicates that an invalid parent was passed into the method.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode55
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:55: Element
elementById = Document.get().getElementById(renderedId);
You can move the assertion below this line, and only run it if
elementById is null. That way, you can throw a Runtime exception in
prod mode, but you only walk up the DOM in the error case.
if (elementById == null) {
if (!isAttachedToDom(root)) {
throw new RuntimeException(UiRendered element is not attached to
DOM while getting \ + fieldName + \);
} else {
throw new IllegalArgumentException(fieldName not found within
rendered element);
}
}
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode88
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:88: :
Parent Element of previously rendered element contains more than one
child;
This assertion should only be executed if ret is the first child of the
parent. If the parent itself is the rendered Element, you don't need
the check.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode117
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:117: public
static SafeHtml stampUiRendererAttribute(SafeHtml safeHtml, String
attributeName,
Can you JavaDoc explaining what this is doing and what it returns.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode120
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:120: int
endOfFirstTag = html.indexOf('');
What if the SafeHtml does not contain any HTML, or if it is self closing
(ends in /):
div id=placeholder /
If UiBinder controls the inputs and can guarantee this doesn't happen,
you should note that. An assertion in dev mode would be good too.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode121
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:121: html =
html.substring(0, endOfFirstTag) + + attributeName + =' +
attributeValue + '
We generally use double quotes instead of quotes. I'm not sure if it
matters a lot.
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode123
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:123: return
new SafeHtmlBuilder().appendHtmlConstant(html).toSafeHtml();
This is simpler:
return SafeHtmlUtils.fromTrustedString(html);
http://gwt-code-reviews.appspot.com/1466809/diff/1/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/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1732
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1732:
ListJMethod getters = findGetterNames(owner);
What happens if there are other methods in the interface that aren't
supported by UiRenderer? Do we rely on javac to trigger a compiler
exception?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1739
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1739: String
elementParameter = getter.getParameters()[0].getName();
We should check that there is exactly 1 parameter, and its type is
assignable from Element.
http://gwt-code-reviews.appspot.com/1466809/
--
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
I had a few nits below but I felt a bit out of context. Is there an
overview somewhere of what you guys are trying to accomplish? What
should rendering for Cells look like?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java
File user/src/com/google/gwt/uibinder/client/UiRenderer.java (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRenderer.java#newcode32
user/src/com/google/gwt/uibinder/client/UiRenderer.java:32: * @param
cell {@link com.google.gwt.cell.client.Cell Cell} that will receive the
event
You should add a blank line before @param
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtils.java
(right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode38
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:38: *
Retrieves a specific element within a previously rendered elements.
/s/elements./element./
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode41
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:41: *
@param fieldName name of the field to retrieve
you forgo the javadoc for attribute (I was actually curious about that
one :-) )
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode45
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:45: Element
root = findRootElement(parent, attribute);
Do you really need the root element? Couldn't you just check if parent
is attached?
http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode87
user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:87: assert
isRenderedElementSingleChild(ret)//
Are the trailing // on purpose?
http://gwt-code-reviews.appspot.com/1466809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
On 2011/07/01 20:59:37, rdcastro wrote: I had a few nits below but I felt a bit out of context. Is there an overview somewhere of what you guys are trying to accomplish? What should rendering for Cells look like? Yes, we have a design doc... which I forgot to share. Here is the link, comments welcome: https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA Is it okay to make that public? http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
Thanks for sharing, I'll read it tomorrow and try and come up with more interesting suggestions :-) On Fri, Jul 1, 2011 at 6:14 PM, rchan...@google.com wrote: On 2011/07/01 20:59:37, rdcastro wrote: I had a few nits below but I felt a bit out of context. Is there an overview somewhere of what you guys are trying to accomplish? What should rendering for Cells look like? Yes, we have a design doc... which I forgot to share. Here is the link, comments welcome: https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Implements UiBinder rendering for Cells. (issue1466809)
On 2011/07/01 21:19:25, stephenh wrote: https://docs.google.com/a/google.com/document/pub?id=1a-_8IdBrBmWCnhV6rnQVmzXB_bXQ9JQ4ya-k1_s1_sA Is it okay to make that public? I think it is OK. We usually publish them in the Wiki, but Docs is getting so good that I wanted to try it this way. http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
