[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
LGTM.
http://gwt-code-reviews.appspot.com/1700803/diff/7001/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/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1138
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1138:
private void ensureInjectedCssFields() {
On 2012/06/01 23:09:27, rchandia wrote:
On 2012/05/27 19:26:12, rdayal wrote:
thought bubble
I wonder if this leads to code bloat (i.e. having to litter these
calls before
accessing a CSSResource field). Why don't we just have a static init
in
CSSResource classes
/thought bubble
ACK. I agree, but I can't say why this strange design decision.
Perhaps one of
the old timers would know. I'll ask.
Ok - did you hear anything?
http://gwt-code-reviews.appspot.com/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1576
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1576:
private void validateRendererGetters(JClassType owner) throws
UnableToCompleteException {
On 2012/06/01 23:09:27, rchandia wrote:
On 2012/05/27 19:26:12, rdayal wrote:
Stupid question - when this method is called, isn't the owner that
is passed
in
the name of the interface (i.e. MyUiBinder, which extends UiBinder)?
In that
case, how can there be any getters? Isn't it an empty interface in
all cases?
The difference here is that the interface is extending UiRenderer
which does
allow getters. See here:
https://docs.google.com/document/d/1Oo_imxskoGX5O9l9LhHDtJ0yJkHvNTNQqU3dkkekZYI/edit
Thx for the in-person explanation.
http://gwt-code-reviews.appspot.com/1700803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
Submitted as r11028 http://gwt-code-reviews.appspot.com/1700803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
http://gwt-code-reviews.appspot.com/1700803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
http://gwt-code-reviews.appspot.com/1700803/diff/7001/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/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1138
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1138:
private void ensureInjectedCssFields() {
On 2012/05/27 19:26:12, rdayal wrote:
thought bubble
I wonder if this leads to code bloat (i.e. having to litter these
calls before
accessing a CSSResource field). Why don't we just have a static init
in
CSSResource classes
/thought bubble
ACK. I agree, but I can't say why this strange design decision. Perhaps
one of the old timers would know. I'll ask.
http://gwt-code-reviews.appspot.com/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1571
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1571: * the
getter must have a single parameter, the parameter type matches
On 2012/05/27 19:26:12, rdayal wrote:
How about -
If the getter return type is assignable to Element, the getter must
have a
single parameter, and the parameter type must be assignable to
Element.
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1576
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1576:
private void validateRendererGetters(JClassType owner) throws
UnableToCompleteException {
On 2012/05/27 19:26:12, rdayal wrote:
Stupid question - when this method is called, isn't the owner that is
passed in
the name of the interface (i.e. MyUiBinder, which extends UiBinder)?
In that
case, how can there be any getters? Isn't it an empty interface in all
cases?
The difference here is that the interface is extending UiRenderer which
does allow getters. See here:
https://docs.google.com/document/d/1Oo_imxskoGX5O9l9LhHDtJ0yJkHvNTNQqU3dkkekZYI/edit
http://gwt-code-reviews.appspot.com/1700803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
http://gwt-code-reviews.appspot.com/1700803/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/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1576
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1576: if
(field == null || (!FieldWriterType.DEFAULT.equals(field.getFieldType())
On 2012/05/24 14:49:24, rchandia wrote:
On 2012/05/23 01:51:11, rdayal wrote:
Maybe wrap this small block of code into a helper method (for
readability)?
In a sense, this method is a helper method intending to make the
caller easier
to read.
I am not sure how extracting this block will make it more readable.
I'd get a
confusing method signature and name :P
ok.
void
dieIfNoFieldNorFieldTypeDefaultNorFieldTypeGeneratedCss(FieldWriter
field,
String getterName, String fieldName)
Also, is there a reason as to why you moved this higher up in the
validation
order?
Yes, this way the following if-checks get simpler (field != null and
types are
either GENERATED_CSS or DEFAULT). It did not matter before because
there was
only one one type of ui:field being considered.
ok.
http://gwt-code-reviews.appspot.com/1700803/diff/7001/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/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1138
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1138:
private void ensureInjectedCssFields() {
thought bubble
I wonder if this leads to code bloat (i.e. having to litter these calls
before accessing a CSSResource field). Why don't we just have a static
init in CSSResource classes
/thought bubble
http://gwt-code-reviews.appspot.com/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1571
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1571: * the
getter must have a single parameter, the parameter type matches
How about -
If the getter return type is assignable to Element, the getter must
have a single parameter, and the parameter type must be assignable to
Element.
http://gwt-code-reviews.appspot.com/1700803/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1576
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1576:
private void validateRendererGetters(JClassType owner) throws
UnableToCompleteException {
Stupid question - when this method is called, isn't the owner that is
passed in the name of the interface (i.e. MyUiBinder, which extends
UiBinder)? In that case, how can there be any getters? Isn't it an empty
interface in all cases?
http://gwt-code-reviews.appspot.com/1700803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
http://gwt-code-reviews.appspot.com/1700803/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/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1367
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1367: for
(ImplicitCssResource css : bundleClass.getCssMethods()) {
On 2012/05/23 01:51:11, rdayal wrote:
Nit: refactor into a helper method (as the code is identical to the
three lines
below).
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1568
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1568: *
{@code Element}.
On 2012/05/23 01:51:11, rdayal wrote:
Update this comment to reflect the new checks that you'd added.
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1576
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1576: if
(field == null || (!FieldWriterType.DEFAULT.equals(field.getFieldType())
On 2012/05/23 01:51:11, rdayal wrote:
Maybe wrap this small block of code into a helper method (for
readability)?
In a sense, this method is a helper method intending to make the caller
easier to read.
I am not sure how extracting this block will make it more readable. I'd
get a confusing method signature and name :P
void dieIfNoFieldNorFieldTypeDefaultNorFieldTypeGeneratedCss(FieldWriter
field, String getterName, String fieldName)
Also, is there a reason as to why you moved this higher up in the
validation
order?
Yes, this way the following if-checks get simpler (field != null and
types are either GENERATED_CSS or DEFAULT). It did not matter before
because there was only one one type of ui:field being considered.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1578
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1578:
die(%s does not match a \ui:field='%s'\ declaration in %s,
getterName, fieldName,
On 2012/05/23 01:51:11, rdayal wrote:
This error message does not seem to reflect the fact that there may be
a
mismatch in the type (as opposed to a failure to find a matching
getter name)
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2221
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2221: //
Esle the non-root elements are found by id
On 2012/05/23 01:51:11, rdayal wrote:
Spelling (Else).
Move this comment within the else block.
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2225
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2225: //
return (ElementSubclass) findUiField(parent);
On 2012/05/23 01:51:11, rdayal wrote:
Seems like this comment is inaccurate.
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2230
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2230: //
return (ElementSubclass) findPreviouslyRendered(parent);
On 2012/05/23 01:51:11, rdayal wrote:
Seems like this comment is inaccurate.
Done.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java
File
user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java
(right):
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java#newcode105
user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java:105:
public static final MockJavaResource UI_STYLE = new
MockJavaResource(foo.UiStyle) {
On 2012/05/23 01:51:11, rdayal wrote:
Good man (adding test code). I like your style.
http://www.anyclip.com/movies/starsky-hutch/biker-bar-fight/
(start watching at 1:20)
Yup. Blow by blow, test by test.
http://gwt-code-reviews.appspot.com/1700803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
http://gwt-code-reviews.appspot.com/1700803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
Great job, and thanks for doing this. Let's go one more round.
Also, would you be willing to update the UIBinder docs? :D :D
http://gwt-code-reviews.appspot.com/1700803/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/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1367
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1367: for
(ImplicitCssResource css : bundleClass.getCssMethods()) {
Nit: refactor into a helper method (as the code is identical to the
three lines below).
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1568
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1568: *
{@code Element}.
Update this comment to reflect the new checks that you'd added.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1576
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1576: if
(field == null || (!FieldWriterType.DEFAULT.equals(field.getFieldType())
Maybe wrap this small block of code into a helper method (for
readability)?
Also, is there a reason as to why you moved this higher up in the
validation order?
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1578
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1578:
die(%s does not match a \ui:field='%s'\ declaration in %s,
getterName, fieldName,
This error message does not seem to reflect the fact that there may be a
mismatch in the type (as opposed to a failure to find a matching getter
name)
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2221
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2221: //
Esle the non-root elements are found by id
Spelling (Else).
Move this comment within the else block.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2225
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2225: //
return (ElementSubclass) findUiField(parent);
Seems like this comment is inaccurate.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2230
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2230: //
return (ElementSubclass) findPreviouslyRendered(parent);
Seems like this comment is inaccurate.
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java
File
user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java
(right):
http://gwt-code-reviews.appspot.com/1700803/diff/1/user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java#newcode105
user/test/com/google/gwt/uibinder/rebind/AbstractUiBinderWriterTest.java:105:
public static final MockJavaResource UI_STYLE = new
MockJavaResource(foo.UiStyle) {
Good man (adding test code). I like your style.
http://www.anyclip.com/movies/starsky-hutch/biker-bar-fight/
(start watching at 1:20)
http://gwt-code-reviews.appspot.com/1700803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added style getters to UiRenderers (issue1700803)
On 2012/05/03 18:50:43, rchandia wrote: Ping http://gwt-code-reviews.appspot.com/1700803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
