[gwt-contrib] Re: More TCK tests (issue1567804)
Missed this, looking now. On Mon, Oct 17, 2011 at 10:26 AM, ncha...@google.com wrote: On 2011/10/13 18:53:54, Nick Chalko wrote: ping http://gwt-code-reviews.**appspot.com/1567804/http://gwt-code-reviews.appspot.com/1567804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: More TCK tests (issue1567804)
LGTM On Oct 17, 2011 4:26 PM, ncha...@google.com wrote: http://gwt-code-reviews.**appspot.com/1567804/diff/1/** user/test/org/hibernate/**jsr303/tck/tests/bootstrap/**customprovider/** TckTestValidatorFactory.javahttp://gwt-code-reviews.appspot.com/1567804/diff/1/user/test/org/hibernate/jsr303/tck/tests/bootstrap/customprovider/TckTestValidatorFactory.java File user/test/org/hibernate/**jsr303/tck/tests/bootstrap/**customprovider/** TckTestValidatorFactory.java (right): http://gwt-code-reviews.**appspot.com/1567804/diff/1/** user/test/org/hibernate/**jsr303/tck/tests/bootstrap/**customprovider/** TckTestValidatorFactory.java#**newcode16http://gwt-code-reviews.appspot.com/1567804/diff/1/user/test/org/hibernate/jsr303/tck/tests/bootstrap/customprovider/TckTestValidatorFactory.java#newcode16 user/test/org/hibernate/**jsr303/tck/tests/bootstrap/**customprovider/** TckTestValidatorFactory.java:**16: package org.hibernate.jsr303.tck.**tests.bootstrap.**customprovider; On 2011/10/17 20:55:03, rjrjr wrote: Is it really appropriate for this to be in the org.hibernate space rather than com.google.gwt? This is the same directory as the test. For some of the tests it was important to be in the same package. http://gwt-code-reviews.**appspot.com/1567804/http://gwt-code-reviews.appspot.com/1567804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add HasEnabled to SuggestBox (issue1567803)
When I run your sample I never see the suggest box. What should I be typing? On Wed, Oct 12, 2011 at 9:10 PM, stephen.haber...@gmail.com wrote: I published a demo here: http://sh-hello.appspot.com/ http://gwt-code-reviews.**appspot.com/1567803/http://gwt-code-reviews.appspot.com/1567803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Generate unique class names in AbstractClientBundleGenerator at design time. (issue1565805)
LGTM On Tue, Oct 11, 2011 at 11:53 AM, scheg...@google.com wrote: Reviewers: rjrjr, Description: Generate unique class names in AbstractClientBundleGenerator at design time. Please review this at http://gwt-code-reviews.**appspot.com/1565805/http://gwt-code-reviews.appspot.com/1565805/ Affected files: M user/src/com/google/gwt/**resources/rebind/context/** AbstractClientBundleGenerator.**java Index: user/src/com/google/gwt/**resources/rebind/context/** AbstractClientBundleGenerator.**java ==**==**=== --- user/src/com/google/gwt/**resources/rebind/context/** AbstractClientBundleGenerator.**java (revision 10696) +++ user/src/com/google/gwt/**resources/rebind/context/** AbstractClientBundleGenerator.**java (working copy) @@ -50,6 +50,7 @@ import com.google.gwt.user.rebind.**ClassSourceFileComposerFactory**; import com.google.gwt.user.rebind.**SourceWriter; +import java.beans.Beans; import java.io.PrintWriter; import java.io.Serializable; import java.net.URL; @@ -968,6 +969,11 @@ toReturn.append(_ + getClass().getSimpleName()); +// If design time, generate new class each time to allow reloading. +if (Beans.isDesignTime()) { + toReturn.append(_designTime + System.currentTimeMillis()); +} + return toReturn.toString(); } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change the superclass of the translatable version of (issue1565803)
LGTM On Oct 6, 2011 5:07 PM, skybr...@google.com wrote: Reviewers: rjrjr, Description: Change the superclass of the translatable version of junit.framework.AssertionFailedError to match the JVM version, for consistency when catching java.lang.AssertionError in testing tools. Fixes issue 6863. Please review this at http://gwt-code-reviews.appspot.com/1565803/ Affected files: M user/super/com/google/gwt/junit/translatable/junit/framework/AssertionFailedError.java Index: user/super/com/google/gwt/junit/translatable/junit/framework/AssertionFailedError.java === --- user/super/com/google/gwt/junit/translatable/junit/framework/AssertionFailedError.java (revision 10689) +++ user/super/com/google/gwt/junit/translatable/junit/framework/AssertionFailedError.java (working copy) @@ -18,7 +18,7 @@ /** * Translatable version of JUnit's codeAssertionFailedError/code. */ -public class AssertionFailedError extends Error { +public class AssertionFailedError extends AssertionError { public AssertionFailedError() { } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Not really practical. On Tue, Oct 4, 2011 at 10:35 AM, stephen.haber...@gmail.com wrote: I meant to comment; this would all be a lot simpler if GWT-RPC could serialize final fields as the default/only behavior (basically remove any configuration variables to turn it on/off). I understand this isn't viable for the next release, as it's a breaking change, and users might be putting values in final fields that they don't want to go over the wire (although hopefully that is rare). But, just throwing it out there, would it be possible to deprecate all of the configuration variables, and say that in the next major release, final fields will always be serialized? Then a lot of the conditional flags and logic could be removed, and the implementation would be simplified. Which isn't critical, but I think would be nice. http://gwt-code-reviews.**appspot.com/1380807/http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Do RF validation via APT instead of command line. (issue1547804)
Jeff, how are you launching your app under eclipse? Are you making m2e fire gwt:run, or are you using Google Plugin for Eclipse Run as Web App? On Thu, Sep 15, 2011 at 2:58 AM, Jeff Larsen larse...@gmail.com wrote: I know I would like it if the RequestFactory stuff was pushed into maven central. Don't make gwt-servlet into a real dependency, add it to the list of dependencies in your plugin. plugin groupIdorg.bsc.maven/**groupId artifactIdmaven-processor-**plugin/artifactId version2.0.5/version executions execution idprocess/id goals goalprocess/goal /goals phasegenerate-sources/**phase configuration !-- TODO(rjrjr) Can this explicit processor line go away if we depend on requestfactory-apt directly? Is that jar in maven central? -- processors processorcom.google.web.**bindery.requestfactory.apt.** RfValidator/processor /processors /configuration /execution /executions dependencies dependency groupIdcom.google.gwt/groupId artifactIdgwt-servlet/artifactId version${gwt-version}/version /dependency /dependencies /plugin /plugins When you switch that to requestfactory-apt, you should be able to get rid of the explicit binding as well, but I have it in mine for documentation purposes. I was just working on this last week for some annotation processors I've written for my application to get around writing the proxies for RequestFactory, building the columns that map to those proxies then also building the constantswithlookup to map the values in there. I needed to do some additional steps when doing this with m2e aswell. Since there are no m2e connectors for any of the annotation processor maven plugins, I worked around that with the build-helper plugin. The build helper plugin allows you to specify additional source folders, so I just mapped a new source folder to where my maven annotation processor was going. I think I found a bug in build-helper-connector where you can't specify source folders under target, but maybe you'll have better luck. Then because I added a new source folder, I needed to add the maven clean plugin to make sure the generated artifacts were getting removed every time a maven clean happened. tl;dr here are the relevant bits of the pom that I was using to get all this working together. plugin groupIdorg.codehaus.mojo/groupId artifactIdbuild-helper-maven-plugin/artifactId version1.7/version executions execution idadd-source/id phasegenerate-sources/phase goals goaladd-source/goal /goals configuration sources source${basedir}/generated-sources/annotations/source /sources /configuration /execution /executions /plugin plugin artifactIdmaven-clean-plugin/artifactId version2.4.1/version configuration filesets fileset directorygenerated-sources/annotations/directory includes include**/*/include /includes /fileset /filesets /configuration /plugin plugin groupIdorg.bsc.maven/groupId artifactIdmaven-processor-plugin/artifactId executions execution idprocess/id goals goalprocess/goal /goals phasegenerate-sources/phase /execution /executions dependencies dependency groupIdcom.ecologic/groupId artifactIdrfprocessor/artifactId version0.0.1-SNAPSHOT/version /dependency /dependencies configuration outputDirectory${basedir}/generated-sources/annotations/outputDirectory processors processorcom.ecologic.rfprocessor.CreateDefaults/processor /processors /configuration -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update the checkstule path for validation sample (issue1547803)
LGTM Oops. Thanks Nick. I'll need to remember to put this on the 2.4 branch. On Sep 13, 2011 7:27 PM, ncha...@google.com wrote: Reviewers: rjrjr, Description: Update the checkstule path for validation sample Fix the @NotSupported count Please review this at http://gwt-code-reviews.appspot.com/1547803/ Affected files: M samples/common.ant.xml M user/build.xml Index: samples/common.ant.xml === --- samples/common.ant.xml (revision 8405) +++ samples/common.ant.xml (working copy) @@ -189,8 +189,8 @@ target name=checkstyle description=Static analysis of source gwt.checkstyle outputdirectory=${sample.build} fileset dir=src - exclude name=org/**/super/org/**/*.java/ - exclude name=com/google/gwt/sample/validation*/**/ValidationMessages.java / + exclude name=main/java/org/**/super/org/**/*.java/ + exclude name=main/java/com/google/gwt/sample/validation*/**/ValidationMessages.java / /fileset /gwt.checkstyle /target Index: user/build.xml === --- user/build.xml (revision 8405) +++ user/build.xml (working copy) @@ -803,13 +803,14 @@ countfilter match=@Failing property=jsr303.marked.Failing init=0/ countfilter match=@NonTckTest property=jsr303.marked.NonTckTest init=0/ countfilter match=@NotSupported property=jsr303.marked.NotSupported init=0/ - countfilter match=@TestNotCompatible property=jsr303.marked.NotSupported init=0/ + countfilter match=@TestNotCompatible property=jsr303.marked.TestNotCompatible init=0/ /filterchain /scan !-- force to zero if not set above -- property name=jsr303.marked.Failing value=0/ property name=jsr303.marked.NonTckTest value=0/ + property name=jsr303.marked.NotSupported value=0/ property name=jsr303.marked.TestNotCompatible value=0/ echo message=Marked Failing = ${jsr303.marked.Failing} / -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Converts the validation sample to build with maven. (issue1537805)
It's resolved if Nick gives his LGTM On Mon, Sep 12, 2011 at 9:34 AM, rchan...@google.com wrote: LGTM. Once the ScriptAssert issue gets resolved. On 2011/09/12 16:27:34, rjrjr wrote: http://gwt-code-reviews.**appspot.com/1537805/http://gwt-code-reviews.appspot.com/1537805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: When -XdisableClassMetadata is used, Class.getName() can return Class$SseedNumber as a class n... (issue1540804)
LGTM Nice catch. Is there really no test to extend for this? :-( On Mon, Sep 12, 2011 at 5:39 PM, cromwell...@google.com wrote: Reviewers: rjrjr, Description: When -XdisableClassMetadata is used, Class.getName() can return Class$SseedNumber as a class name. However, there are other modes where it can return Class$obfuscated function name. In some rare cases, these two could collide of if an obfuscated name of a class ended up as something like 'S123'. This patch changes the WebModeClientOracle to treat Class$S123 differently than 'S123' when deobfuscating class names. Please review this at http://gwt-code-reviews.**appspot.com/1540804/http://gwt-code-reviews.appspot.com/1540804/ Affected files: M user/src/com/google/gwt/rpc/**server/WebModeClientOracle.**java Index: user/src/com/google/gwt/rpc/**server/WebModeClientOracle.**java ==**==**=== --- user/src/com/google/gwt/rpc/**server/WebModeClientOracle.**java (revision 10636) +++ user/src/com/google/gwt/rpc/**server/WebModeClientOracle.**java (working copy) @@ -383,12 +383,14 @@ @Override public String getTypeName(String seedName) { // TODO: Decide how to handle the no-metadata case +ClassData data = null; if (seedName.startsWith(Class$)**) { seedName = seedName.substring(6); -} -ClassData data = seedNamesToClassData.get(**seedName); + data = seedIdsToClassData.get(**seedName); +} + if (data == null) { - data = seedIdsToClassData.get(**seedName); + data = seedNamesToClassData.get(**seedName); } return data == null ? null : data.typeName; } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Converts the validation sample to build with maven. (issue1537805)
My main problem was that I had lost the web.xml file (duh). Still trying to figure out the ScriptAssertValidator issue. I wonder if an extra copy is being pulled in by maven. On Thu Sep 08 18:15:07 GMT-700 2011, Nick Chalko wrote: org.hibernate.validator.**constraints.impl.**ScriptAssertValidator So ScriptAssertValidator should not be getting compiled. Let me try to find where it is excluded. user/src/org/hibernate/validator/HibernateValidator.gwt.xml excludes that file with. source path=constraints exclude name=impl/scriptassert/ / exclude name=super/ / /source Not sure why this is not used. -- Nick Chalko | Software Engineer | ncha...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Anchalko%40google.com | :-) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Converts the validation sample to build with maven. (issue1537805)
Thanks. Do I need the validation-api dependency (in any of samples/*/pom.xml), or does gwt 2.4.0 bring that along? On Fri, Sep 9, 2011 at 12:09 PM, drfibona...@google.com wrote: http://gwt-code-reviews.**appspot.com/1537805/diff/2002/** samples/validation/pom.xmlhttp://gwt-code-reviews.appspot.com/1537805/diff/2002/samples/validation/pom.xml File samples/validation/pom.xml (right): http://gwt-code-reviews.**appspot.com/1537805/diff/2002/** samples/validation/pom.xml#**newcode200http://gwt-code-reviews.appspot.com/1537805/diff/2002/samples/validation/pom.xml#newcode200 samples/validation/pom.xml:**200: artifactIdexec-maven-plugin**/artifactId Looks like the whole pluginManagement section can be omitted as I don't see exec-maven-plugin anywhere else in the POM. http://gwt-code-reviews.**appspot.com/1537805/http://gwt-code-reviews.appspot.com/1537805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Converts the validation sample to build with maven. (issue1537805)
On Thu, Sep 8, 2011 at 6:15 PM, Nick Chalko ncha...@google.com wrote: org.hibernate.validator.**constraints.impl.**ScriptAssertValidator So ScriptAssertValidator should not be getting compiled. Let me try to find where it is excluded. user/src/org/hibernate/validator/HibernateValidator.gwt.xml excludes that file with. source path=constraints exclude name=impl/scriptassert/ / exclude name=super/ / /source Not sure why this is not used. It looks like the ScriptAssertValidator problem is actually on the @ScriptAssert annotation itself: @Target({ TYPE }) @Retention(RUNTIME) *@Constraint(validatedBy = ScriptAssertValidator.class)* @Documented public @interface ScriptAssert { The type oracle tries to process ScriptAssertValidator.class, but it can't because of nick's excludes line. No harm is done, but the console noise is unpleasant. And I wonder what would happen if someone actually tried to use @ScriptAssert? Nick, do you emulate the rest of those constraints, or do they just work? Should I exclude the whole package? Or perhaps I should put in a no-op super source implementation of ScriptAssertValidator. We've done that kind of thing before. rjrjr -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Scripts and configuration to upload GWT bits to Maven repos (issue1520810)
LGTM On Fri, Sep 9, 2011 at 1:26 PM, rchan...@google.com wrote: http://gwt-code-reviews.**appspot.com/1520810/diff/7016/**maven/lib-gwt.shhttp://gwt-code-reviews.appspot.com/1520810/diff/7016/maven/lib-gwt.sh File maven/lib-gwt.sh (right): http://gwt-code-reviews.**appspot.com/1520810/diff/7016/** maven/lib-gwt.sh#newcode99http://gwt-code-reviews.appspot.com/1520810/diff/7016/maven/lib-gwt.sh#newcode99 maven/lib-gwt.sh:99: maven-deploy-file $mavenRepoUrl $mavenRepoId $pomDir/gwt/pom.xml $pomDir/gwt/pom.xml On 2011/09/09 19:59:04, rjrjr wrote: This should happen between the for loops. Done. http://gwt-code-reviews.**appspot.com/1520810/http://gwt-code-reviews.appspot.com/1520810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Converts the validation sample to build with maven. (issue1537805)
On Fri, Sep 9, 2011 at 1:15 PM, Ray Ryan rj...@google.com wrote: On Thu, Sep 8, 2011 at 6:15 PM, Nick Chalko ncha...@google.com wrote: org.hibernate.validator.**constraints.impl.**ScriptAssertValidator So ScriptAssertValidator should not be getting compiled. Let me try to find where it is excluded. user/src/org/hibernate/validator/HibernateValidator.gwt.xml excludes that file with. source path=constraints exclude name=impl/scriptassert/ / exclude name=super/ / /source Not sure why this is not used. It looks like the ScriptAssertValidator problem is actually on the @ScriptAssert annotation itself: @Target({ TYPE }) @Retention(RUNTIME) *@Constraint(validatedBy = ScriptAssertValidator.class)* @Documented public @interface ScriptAssert { The type oracle tries to process ScriptAssertValidator.class, but it can't because of nick's excludes line. No harm is done, but the console noise is unpleasant. And I wonder what would happen if someone actually tried to use @ScriptAssert? Nick, do you emulate the rest of those constraints, or do they just work? Should I exclude the whole package? Or perhaps I should put in a no-op super source implementation of ScriptAssertValidator. We've done that kind of thing before. rjrjr Hmm. You already have a super source ScriptAssertValidator, just like all the other ones. I still don't get what's special about this one. Deleting that exclude line doesn't change anything. Making it exclude the entire impl packages gets rid of the error and the sample still works. Seeing what that does to the tests. (I assume there's a reason you aren't already doing that.) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Converts the validation sample to build with maven. (issue1537805)
On Fri, Sep 9, 2011 at 1:40 PM, Ray Ryan rj...@google.com wrote: On Fri, Sep 9, 2011 at 1:15 PM, Ray Ryan rj...@google.com wrote: On Thu, Sep 8, 2011 at 6:15 PM, Nick Chalko ncha...@google.com wrote: org.hibernate.validator.**constraints.impl.**ScriptAssertValidator So ScriptAssertValidator should not be getting compiled. Let me try to find where it is excluded. user/src/org/hibernate/validator/HibernateValidator.gwt.xml excludes that file with. source path=constraints exclude name=impl/scriptassert/ / exclude name=super/ / /source Not sure why this is not used. It looks like the ScriptAssertValidator problem is actually on the @ScriptAssert annotation itself: @Target({ TYPE }) @Retention(RUNTIME) *@Constraint(validatedBy = ScriptAssertValidator.class)* @Documented public @interface ScriptAssert { The type oracle tries to process ScriptAssertValidator.class, but it can't because of nick's excludes line. No harm is done, but the console noise is unpleasant. And I wonder what would happen if someone actually tried to use @ScriptAssert? Nick, do you emulate the rest of those constraints, or do they just work? Should I exclude the whole package? Or perhaps I should put in a no-op super source implementation of ScriptAssertValidator. We've done that kind of thing before. rjrjr Hmm. You already have a super source ScriptAssertValidator, just like all the other ones. I still don't get what's special about this one. Deleting that exclude line doesn't change anything. Making it exclude the entire impl packages gets rid of the error and the sample still works. Seeing what that does to the tests. (I assume there's a reason you aren't already doing that.) Ah ha! It is you, *you Nick Chalko* that are making this happen! In your super source you have a copy of ScriptAssert.java, where you have no other annotations emulated. It includes an annotation with the reference to the ScriptAssertValidator.class. (I suspect this is a bug in super source if you squint just right, but I don't care.) If I delete that file the noise goes away, the sample runs, and the tests all pass. Any idea why it's there? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Deprecate DeferredCommand and IncrementalCommand. (issue982802)
This isn't a release notes issue. The deprecation happened quite a while ago. DevGuideCodingBasicsDelayed.html is very, very out of date. On Thu, Sep 8, 2011 at 3:31 PM, Doug Anderson do...@google.com wrote: Eric: I can note this and any other deprecation you want in the release notes for next release, say 2.5 (or, if anything was deprecated for 2.4 and we missed it, I can also update the 2.4 notes!). The actual Javadoc is maintained by you folks, so any changes you want in that needs to be made at your end, I'm pretty sure. Doug On Thu, Sep 8, 2011 at 3:01 PM, Eric Ayers zun...@google.com wrote: http://google-web-toolkit.googlecode.com/svn/javadoc/latest/com/google/gwt/user/client/DeferredCommand.html has a pointer to the blessed alternative (Scheduler). @Doug, can you flag this page as needing an update? On Thu, Sep 8, 2011 at 4:43 PM, mark.w...@potentbyte.com wrote: Since this is deprecated what is the alternative to these classes? The documentation continues to provide these as examples. http://code.google.com/**webtoolkit/doc/latest/** DevGuideCodingBasicsDelayed.**htmlhttp://code.google.com/webtoolkit/doc/latest/DevGuideCodingBasicsDelayed.html On 2010/10/13 17:13:43, rjrjr wrote: On Wed, Oct 13, 2010 at 5:28 AM, mailto:zun...@google.com wrote: http://gwt-code-reviews.**appspot.com/982802/diff/1/5http://gwt-code-reviews.appspot.com/982802/diff/1/5 File user/src/com/google/gwt/user/**client/DeferredCommand.java (right): http://gwt-code-reviews.**appspot.com/982802/diff/1/5#**newcode27http://gwt-code-reviews.appspot.com/982802/diff/1/5#newcode27 user/src/com/google/gwt/user/**client/DeferredCommand.java:**27: * API prevents effective mocking. On 2010/10/13 03:50:36, rjrjr wrote: I thought it was deprecated because it was redundant...TMI? IMHO, I don't think you can give too much information on the impetus behind this change. Its going to be painful to see all those deprecation warnings come out the first time for most users. We might want to provide more justification then. We're not just deprecating the thing for mockability, it's redundant code, right? http://gwt-code-reviews.**appspot.com/982802/showhttp://gwt-code-reviews.appspot.com/982802/show http://gwt-code-reviews.**appspot.com/982802/http://gwt-code-reviews.appspot.com/982802/ -- Eric Ayers | Software Engineer | zun...@google.com | +1 404 487 9229 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Event handling support for UiRenderer (issue1534806)
LGTM On Wed Sep 07 11:50:58 GMT-700 2011, rchan...@google.comgt wrote: http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.javahttp://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java File user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java (right): http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java#newcode60 user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java:60http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java%23newcode60%0Auser/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java:60 : if (!isParentOrRenderer(parentOrRoot, RENDERED_ATTRIBUTE)) { On 2011/09/07 17:30:19, rjrjr wrote: Should call the single arg version. No, because UiRendererDispatcher is a static class, so it can only call static methods. This method is null tolerant. Should it NPE instead? Done. Added validation for null on the first three parameters, throwing NPEs on null receiver, root and events. If so, does that fix belong in isParentOrRenderer(Element, String)? No, isParentOrRenderer(Element, String) is designed to be a safe test and it is used elsewhere in that way. I'd find a NPE from it kind of surprising. http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java#newcode322 user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java:322http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java%23newcode322%0Auser/src/com/google/gwt/uibinder/client/impl/AbstractUiRenderer.java:322 : * Walks up the parents of the {@code rendered} element to ascertain that it is attached to the On 2011/09/07 17:30:19, rjrjr wrote: In DevMode, ... ... Always returns true in ProdMode Done. http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.javahttp://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.java File user/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.java (right): http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.java#newcode28 user/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.java:28http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1534806/diff/10001/user/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.java%23newcode28%0Auser/src/com/google/gwt/uibinder/client/impl/UiBinderUtil.java:28 : public class UiBinderUtil { On 2011/09/07 17:30:19, rjrjr wrote: As we discussed offline, it might not be the right time to make this move. But if you keep it, could you add a note to the issue tracker mentioning it as a breaking change (tag ReleaseNote=breakingChange) I'd rather back off. The fix in GPE is not obvious to me. http://gwt-code-reviews.appspot.com/1534806/http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1534806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] dependencies/qualified refs
Scott says: Stephen misunderstands what Dependencies.apiRefs means (which isn't suprising, it's not well commented). The simple and qualified references answer the question What references do I depend on to correctly parse and resolve the source code? Api refs answer the question What program elements do I need to construct a TypeOracle representation of this class? In other words, api refs only include things like superclass, implemented interfaces, method parameter and return types, field types, etc. Things you have to have to construct a valid TypeOracle representation. Method bodies do not contribute to this list at all. Simple and qualified references is generally a superset of that, includes anything references in method bodies. On Wed, Sep 7, 2011 at 8:52 AM, Stephen Haberman stephen.haber...@gmail.com wrote: Hi, There was one place in the name mangling that I had to leave an ugly replacement: https://github.com/stephenh/scalagwt-gwt/blob/embed/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#L109 allValidClasses is by internal name now, but the qualified references coming from ecj are by source name. (Hm, if these are top-level only units, then a simple replacement would be okay. Does anyone know if that is the case, off the top of their heads? I'll fire up the debugger later and see if I can tell.) But the other reason I ask on the list instead of in the review is that, for scalagwt itself, I don't have these ecj data structures, so I'm inferring the simple and qualified refs from the api refs: https://github.com/stephenh/scalagwt-gwt/blob/embed/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#L62 I'm pretty sure this approach is okay, but my concern is that, poking around in the debugger, ecj, ends up with a whole lot more strings as simple/qualified refs than my current, albeit somewhat naive approach in buildFromApiRefs. Given I'd already written buildFromApiRefs, I was tempted to use it for the .java files as well, since api refs are already internal names, and then I could remove the source - internal mangling. But I didn't have enough confidence to do that just yet. If anyone could comment on buildFromApiRefs, both in theory and the current implementation, just for scalagwt but also whether it's potentially usable for the java side, I'd appreciate it. But I'll nonetheless look into whether the qualifiedReferences are only top-level units, in which case the name mangling should be okay. I didn't think of that until just now. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Comment on RequestFactory_2_4 in google-web-toolkit
Fixed, thanks. On Monday, September 5, 2011, wrote: Comment by ido@gmail.com: In Improved request batching section the sample has typo: ctxB.requestB().to(new ReceiverBoolean(){}); should be ctxB.requestB().to(new ReceiverInteger(){}); For more information: http://code.google.com/p/**google-web-toolkit/wiki/**RequestFactory_2_4http://code.google.com/p/google-web-toolkit/wiki/RequestFactory_2_4 -- 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
[gwt-contrib] Re: Event handling support for UiRenderer (issue1534806)
Oh, crazy! I didn't notice that you were invoking it at compile time. And I suppose that's going to let it fail at compile time instead of runtime, isn't it? Okey doke. On Tuesday, September 6, 2011, wrote: On 2011/09/02 00:01:48, rjrjr wrote: http://gwt-code-reviews.**appspot.com/1534806/diff/1/** user/src/com/google/gwt/**uibinder/rebind/**UiBinderWriter.java#** newcode1202http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1202 user/src/com/google/gwt/**uibinder/rebind/**UiBinderWriter.java:1202: try { Should use the TypeOracle, not reflection Here I need to actually call XEvent.getType().getName() to figure out the string the class is associated with. How do I go about using TypeOracle for that? http://gwt-code-reviews.**appspot.com/1534806/http://gwt-code-reviews.appspot.com/1534806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updates pom.xml to use new Request Factory libraries (issue1541803)
LGTM Nice! On Tue, Sep 6, 2011 at 1:22 PM, rchan...@google.com wrote: Reviewers: rjrjr, drfibonacci, Description: Updates pom.xml to use new Request Factory libraries Please review this at http://gwt-code-reviews.**appspot.com/1541803/http://gwt-code-reviews.appspot.com/1541803/ Affected files: M samples/mobilewebapp/pom.xml Index: samples/mobilewebapp/pom.xml ==**==**=== --- samples/mobilewebapp/pom.xml(revision 10615) +++ samples/mobilewebapp/pom.xml(working copy) @@ -48,10 +48,15 @@ !-- Google Web Toolkit (GWT) itself -- dependency - groupIdcom.google.gwt/**groupId - artifactIdgwt-servlet/**artifactId + groupIdcom.google.web.**bindery/groupId + artifactIdrequestfactory-**server/artifactId version${gwtVersion}/**version scoperuntime/scope +/dependency +dependency + groupIdcom.google.web.**bindery/groupId + artifactIdrequestfactory-**apt/artifactId + version${gwtVersion}/**version /dependency dependency groupIdcom.google.gwt/**groupId @@ -75,12 +80,6 @@ /dependency !-- GWT RequestFactory will use JSR 303 javax.validation if you let it -- -dependency - groupIdjavax.validation/**groupId - artifactIdvalidation-api/**artifactId - version1.0.0.GA/version - classifiersources/**classifier -/dependency dependency groupIdorg.hibernate/**groupId artifactIdhibernate-**validator/artifactId @@ -97,17 +96,7 @@ /exclusions /dependency -!-- GWT RequestFactory requires org.json -- -!-- TODO: can we declare the json and validation dependencies somewhere for the world to pick up, -rather than requiring everyone to know about them? -- -dependency - groupIdorg.json/groupId - artifactIdjson/artifactId - version20090211/version -/dependency - !-- Google App Engine (GAE) itself -- - dependency groupIdcom.google.appengine**/groupId artifactIdappengine-api-1.0-**sdk/artifactId @@ -172,11 +161,11 @@ version1.6.1/version /dependency /dependencies - + build -!-- Generate compiled stuff in the folder used for developing mode -- +!-- Generate compiled stuff in the folder used for developing mode -- outputDirectory${project.**build.directory}/${project.** build.finalName}/WEB-INF/**classes/outputDirectory - + plugins !-- GWT Maven Plugin-- plugin @@ -200,7 +189,7 @@ version${gwtVersion}/**version /dependency /dependencies -!-- JS is only needed in the package phase, this speeds up testing -- +!-- JS is only needed in the package phase, this speeds up testing -- executions execution phaseprepare-package/phase @@ -209,8 +198,8 @@ /goals /execution /executions -!-- Plugin configuration. There are many available options, - see gwt-maven-plugin documentation at codehaus.org -- +!-- Plugin configuration. There are many available options, + see gwt-maven-plugin documentation at codehaus.org -- configuration !-- URL that should be automatically opened in the GWT shell (gwt:run). -- runTargetMobileWebApp.html/**runTarget @@ -255,7 +244,7 @@ useManifestOnlyJarfalse/**useManifestOnlyJar forkModealways/forkMode - !-- Folder for generated testing stuff -- + !-- Folder for generated testing stuff -- systemProperties property namegwt.args/name -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] less name mangling
Thanks Steve! We chatted, and Ray C is up for reviewing this, but I'm afraid it'll be the usual maybe-this-week drill. On Fri Sep 02 13:06:44 GMT-700 2011, Stephen Haberman wrote: I'd prefer this problem to be addressed in separate CL independent from of our effort. Here is a commit with just the name mangling. If there are any volunteers from the GWT team to review it, I'll promote it to an issue. https://github.com/stephenh/scalagwt-gwt/commit/5fb1f9717424b8c604f9f2c84b0bc713dcb2http://www.google.com/url?sa=Dq=https://github.com/stephenh/scalagwt-gwt/commit/5fb1f9717424b8c604f9f2c84b0bc713dcb2 I do have a dangling todo in there, but it's somewhat tangential, so I'll avoid it until prompted. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Convert DynaTableRF to use maven. Clean up the docs in the other two (issue1537803)
ping, ready for review On Thu Sep 01 15:08:29 GMT-700 2011, rj...@google.comgt wrote: http://gwt-code-reviews.appspot.com/1537803/http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1537803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] [ANN] Scala+GWT 0.1-M1 released
Woo hoo! On Thu Sep 01 09:26:47 GMT-700 2011, Grzegorz Kossakowski wrote: Hello, I'm excited to announce first milestone of Scala+GWT project. Download (and then follow README instructions) from here: http://goo.gl/Ym3xUhttp://www.google.com/url?sa=Dq=http%3A%2F%2Fgoo.gl%2FYm3xU Release notes (included below) can be found here: http://goo.gl/H8sanhttp://www.google.com/url?sa=Dq=http%3A%2F%2Fgoo.gl%2FH8san Scala+GWT 0.1-M1 The first official milestone release from the Scala+GWT project. This release consists of just samples packaged along with snapshot of jars needed to build them. Those jars include our own version of both GWT and Scala. This release doesn't come with any official artifacts apart from the tarball you can download from http://github.com/scalagwt/scalagwt-samplehttp://www.google.com/url?sa=Dq=http%3A%2F%2Fgithub.com%2Fscalagwt%2Fscalagwt-sample . ### What works Samples show it the best. Here are some highlights: * Mixed Scala/Java projects work very well (for GWT-supported Java subset) * Most of Scala language constructs are supported * Most of Scala library code that makes sense in a browser context is supported, including Scala collections It's fair to say that we are not sure how far one can go with this release. It might be that you can already build something useful with what we already have. The only way to be sure is start hacking! ### Known issues * Compilation is very, very slow. * `scala.immutable.{TreeMap, TreeSet}` are not supported due to various bugs (thus sorted collections don't work) * many patterns in pattern matching logic are not supported (tough issue) examples include * pattern alternatives (`|`) * guard patterns (`if` guard) * GWT's development mode is broken * We are compiling with all optimizations turned off. This results in a slow and very large JavaScript code. ### Reporting issues We appreciate feedback. If you find something that doesn't work (e.g. crashes either Scala or GWT compiler) or JavaScript gives you weird results we'd love to hear about it. The most effective way of reporting issues is to modify `Hello World` sample to show your problem. Exact steps are: 1. Fork `scalagwt-sample` repo from here: http://github.com/scalagwt/scalagwt-samplehttp://www.google.com/url?sa=Dq=http%3A%2F%2Fgithub.com%2Fscalagwt%2Fscalagwt-sample 2. Clone it: `git clone git:// github.com/YOUR_USER_NAME/scalagwt-sample.git`http://www.google.com/url?sa=Dq=http%3A%2F%2Fgithub.com%2FYOUR_USER_NAME%2Fscalagwt-sample.git 3. Modify hello world sample located in `src/com/google/gwt/sample/jribble/client` 4. Commit and publish your example. 5. File a ticket here: http://github.com/scalagwt/scalagwt-samplehttp://www.google.com/url?sa=Dq=http%3A%2F%2Fgithub.com%2Fscalagwt%2Fscalagwt-sample and mention your fork while explaining your issue. If you want to discuss your problem before reporting it, join [scala...@googlegroups.comhttp://www.google.com/url?sa=Dq=mailto%3Ascalagwt%40googlegroups.com ](http://groups.google.com/group/scalagwt). ### What if I don't know GWT? That shouldn't be a big problem. You've got Scala source code for samples that show basic functionality and provides basic setup. You may want to start with channging hello world sample, recompiling it and testing in a browser. ### What if I don't know Scala? You might still want to check out samples to see how they might look like in other language than Java. We'll be cutting a lot more of boilerplate code once GWT libraries and APIs receive enough of Scala's [pimp-love](http://www.artima.com/weblogs/viewpost.jsp?thread=179766http://www.google.com/url?sa=Dq=http%3A%2F%2Fwww.artima.com%2Fweblogs%2Fviewpost.jsp%3Fthread%3D179766 ). ### Need help? Want to discuss something? Join us here: [scala...@googlegroups.comhttp://www.google.com/url?sa=Dq=mailto%3Ascalagwt%40googlegroups.com ](http://groups.google.com/group/scalagwt). Happy playing! -- Grzegorz Kossakowski -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes for MobileWebApp pom.xml (issue1520809)
LGTM On Wed, Aug 31, 2011 at 1:18 PM, drfibona...@google.com wrote: LGTM http://gwt-code-reviews.**appspot.com/1520809/http://gwt-code-reviews.appspot.com/1520809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make UmbrellaException a bit more convenient to read, and test it. (issue1532803)
r10584 On Thu Aug 25 23:08:59 GMT-700 2011, cromwell...@google.comgt wrote: lgtm http://gwt-code-reviews.appspot.com/1532803/http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1532803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] maven source jars
Stephen, if you're game to do the research and the work, we're certainly happy to have it done. Thanks either way. On Wed Aug 24 12:39:51 GMT-700 2011, Stephen Haberman wrote: Hm, perhaps a few. I saw instructions on patching emma; nothing else is leaping out at me. Okay, there are patch files for json and streamhtmlparser. The latter is rebased. Leaving anything that is rebased or patched in the jar seems fine to me, since it's only a few. (I'm replying to myself, but I sent my previous message from the wrong email address, so it is probably moderated/dropped. Here's the text in case my previous message never shows up:) Taking gwt-dev-nodeps further, it looks like most of the deps come from bundling tomcat and htmlunit. Neither of which I use. I think it would be fairly easily, at least from a packaging perspective, to make gwt-dev-nodeps, gwt-tomcat, and gwt-htmlunit jars, none of which have their dependencies in them, but instead pulled in however projects otherwise manage transitive dependencies (poms/what have you). If I got a gwt-dev-nodeps/gwt-tomcat/gwt-htmlunit build working, would anyone be interested in that? - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make UmbrellaException a bit more convenient to read, and test it. (issue1532803)
How's that? On Thu Aug 25 17:04:18 GMT-700 2011, cromwell...@google.comgt wrote: http://gwt-code-reviews.appspot.com/1532803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.javahttp://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1532803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java File user/src/com/google/web/bindery/event/shared/UmbrellaException.java (right): http://gwt-code-reviews.appspot.com/1532803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java#newcode53 user/src/com/google/web/bindery/event/shared/UmbrellaException.java:53http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1532803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java%23newcode53%0Auser/src/com/google/web/bindery/event/shared/UmbrellaException.java:53 : return causes.size() == 1 ? ONE + message : MULTIPLE + message; Couldn't we tack on the rest of the exception messages here when there's multiple instead of just the first? http://gwt-code-reviews.appspot.com/1532803/http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1532803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changes setters/clearers com.google.gwt.dom.client.Style to return Style instead of void (issue1530803)
No good. This defeats compiler optimizations big time. We don't like void returns either, but we like big fat slow apps even less. On Aug 22, 2011 7:50 PM, larse...@gmail.com wrote: Reviewers: jlabanca, rjrjr, Description: Changes Style to return itself instead of returning void. issue 6717 http://code.google.com/p/google-web-toolkit/issues/detail?id=6717 Please review this at http://gwt-code-reviews.appspot.com/1530803/ Affected files: user/src/com/google/gwt/dom/client/Style.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updates MobileWebApp to use GWT Maven Plugin 2.3.0-1 (issue1525806)
LGTM D'oh! I thought that was meant to be *our* version string. Sorry. On Thu Aug 18 08:55:54 GMT-700 2011, Rodrigo Chandia wrote: According to the GWT maven plugin web page 2.3.0-1 is the latest version. http://mojo.codehaus.org/gwt-maven-plugin/http://www.google.com/url?sa=Dq=http%3A%2F%2Fmojo.codehaus.org%2Fgwt-maven-plugin%2F El 18 de agosto de 2011 11:53, rchan...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Archandia%40google.com escribió: http://gwt-code-reviews.**appspot.com/1525806/diff/1/** samples/mobilewebapp/pom.xmlhttp://www.google.com/url?sa=Dq=http%3A%2F%2Fgwt-code-reviews.appspot.com%2F1525806%2Fdiff%2F1%2Fsamples%2Fmobilewebapp%2Fpom.xml File samples/mobilewebapp/pom.xml (right): http://gwt-code-reviews.**appspot.com/1525806/diff/1/** samples/mobilewebapp/pom.xml#**newcode191http://www.google.com/url?sa=Dq=http%3A%2F%2Fgwt-code-reviews.appspot.com%2F1525806%2Fdiff%2F1%2Fsamples%2Fmobilewebapp%2Fpom.xml%23newcode191 samples/mobilewebapp/pom.xml:**191: version2.3.0-1/version On 2011/08/18 15:34:16, rjrjr wrote: What's the -1 for? Also, tab character in this line. Done, untabified the file. The -1 is part of the version string 2.3.0-1 http://gwt-code-reviews.**appspot.com/1525806/http://www.google.com/url?sa=Dq=http%3A%2F%2Fgwt-code-reviews.appspot.com%2F1525806%2F -- Rodrigo Chandia | Software Engineer | rchan...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Archandia%40google.com | 678 7431725 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix http://code.google.com/p/google-web-toolkit/issues/detail?id=6161 (issue1520807)
No, but I'm not sure it can be. The error is actually legitimate: you have @UiField Label and you're trying to stick an instance of Element in it, in this case a funny looking one: g:Label... My alternative is to make this error fatal, which means that you could not use binder to render prefixed dom elements. Not a very compelling use case, I agree, but who knows how many existing templates are already making this error. I'm not really in the mood to be hunted down by the people I'd break... On Thu Aug 18 09:59:22 GMT-700 2011, mros...@google.comgt wrote: http://gwt-code-reviews.appspot.com/1520807/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.javahttp://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1520807/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java (right): http://gwt-code-reviews.appspot.com/1520807/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java#newcode104 user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java:104http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1520807/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java%23newcode104%0Auser/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java:104 : writer.warn(elem, Prefix \%s:\ has unrecognized xmlns \%s\, Will this warning be shown in proximity to where an ERROR is shown when it does fail? http://gwt-code-reviews.appspot.com/1520807/http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1520807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a new CellTableHeaderBuilder API, which allows custom headers and footers in CellTable. C... (issue1499808)
LGTM On Wed, Aug 17, 2011 at 7:32 AM, jlaba...@google.com wrote: A patch is coming that modifies the TableBuilder API to make it more flexible. TableBuilder.Utility is going away completely, and the event handling implementation is moved from AbstractCellTable into AbstractTableBuilder. This will allow users to implement a simple grid based TableBuilder with faster rendering and event handling logic. The new API is also much more builder-like, and simpler to boot.Once those changes are in, I'll revisit HeaderCreator and apply similar changes, hopefully eliminating the Helper class. Either way, there will be follow-up changes that unify the terminology used in TableBuilder/HeaderCreator. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.javahttp://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java File samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java (right): http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.java#**newcode79http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode79 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:79: * Example file. On 2011/08/16 02:16:25, skybrian wrote: Defines a custom table that displays a contact in each row. This is an example that shows how to completely customize the appearance of the headers, data rows, and footers in a CellTable. Done. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.java#**newcode143http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode143 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:143: * A custom header builder. On 2011/08/16 02:16:25, skybrian wrote: Renders custom headers that ... Done. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.java#**newcode164http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode164 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:164: public void buildHeader(Helper**ContactInfo utility) { On 2011/08/16 02:16:25, skybrian wrote: s/utility/helper/ Done. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.java#**newcode209http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode209 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:209: * Build a single header. On 2011/08/16 02:16:25, skybrian wrote: Renders the header of one column, with the given options. Done. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.java#**newcode211http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode211 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:211: * @param utility the utility used to builder the header On 2011/08/16 02:16:25, skybrian wrote: used to build Done. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/** showcase/client/content/cell/**CwCustomDataGrid.java#**newcode213http://gwt-code-reviews.appspot.com/1499808/diff/14004/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode213 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:213: * @param header the header to buil On 2011/08/16 02:16:25, skybrian wrote: the Header to render Done. http://gwt-code-reviews.**appspot.com/1499808/diff/** 14004/samples/showcase/src/**com/google/gwt/sample/**
[gwt-contrib] Re: Provides an integration test for IsRenderable (issue1527804)
I like #1 too. I think we should try to narrow the visibility of PotentialElement as much as we can. So #1 means two things , right? • Widgets are seated in their @UiFields immediately • In an IsRenderable owner, Element and subclasses are only available via LazyDomElement, and @UiField Element is a compile time error I've tweaked the test a bit (will update soon), and I'm happy to report that composites around non-IsRenderables work as expected, with element fields filled immediately. Given that I don't think we need to delay the switch to using lazy widget builder by default. On Wed Aug 17 06:14:52 GMT-700 2011, Hermes Freitas wrote: WidgetInterpreter and WidgetPlaceholderInterpreter shouldn't output LazyDomElement. Rafa, do you remember why? I don't think this aggregates any performance gain for us, am I missing something? And I vote for #1 On Tue, Aug 16, 2011 at 10:10 PM, Rafael Castro rdcas...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Ardcastro%40google.com wrote: +hermes Good point, this is really tricky. The problem here is that we don't actually have the DOM element until the widget is attached. I see 2 options: 1-) We force the UiField to be a LazyDomElement, so this is explicit. 2-) We use PotentialElement with a resolver that throws an Exception (i.e., it's only really resolved when it's attached). what do you think? ps.: really nice tests, thanks for putting them together! On Tue, Aug 16, 2011 at 5:13 PM, rj...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Arjrjr%40google.com wrote: On 2011/08/17 00:12:24, rjrjr wrote: Ready for review. Rafa, this turned up one issue that concerns me: most @UiField fields are not filled in until the widget is attached to the dom, but we're not consistent about it. See the big comment in testDeep. http://gwt-code-reviews.**appspot.com/1527804/http://www.google.com/url?sa=Dq=http%3A%2F%2Fgwt-code-reviews.appspot.com%2F1527804%2F -- --Hermes Freitas -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Provides an integration test for IsRenderable (issue1527804)
Cool, I've filed http://code.google.com/p/google-web-toolkit/issues/detail?id=6701. Can I get an LGTM and submit this thing? On Wed Aug 17 09:58:03 GMT-700 2011, Rafael Castro wrote: Awesome, I like #1 too. I was driving to work this morning and thinking about it: #2 actually encourages bad behavior, because it'll seem it's OK to fiddle with the elements between calling bind and attaching, and it's really not. We _could_ make an effort to make it work, but it's much better to make the flow clearer this way: if you're using lazy widgets, your elements have to be lazy too. On Wed, Aug 17, 2011 at 9:41 AM, Ray Ryan rj...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Arjrjr%40google.com wrote: I like #1 too. I think we should try to narrow the visibility of PotentialElement as much as we can. So #1 means two things , right? • Widgets are seated in their @UiFields immediately • In an IsRenderable owner, Element and subclasses are only available via LazyDomElement, and @UiField Element is a compile time error I've tweaked the test a bit (will update soon), and I'm happy to report that composites around non-IsRenderables work as expected, with element fields filled immediately. Given that I don't think we need to delay the switch to using lazy widget builder by default. On Wed Aug 17 06:14:52 GMT-700 2011, Hermes Freitas wrote: WidgetInterpreter and WidgetPlaceholderInterpreter shouldn't output LazyDomElement. Rafa, do you remember why? I don't think this aggregates any performance gain for us, am I missing something? And I vote for #1 On Tue, Aug 16, 2011 at 10:10 PM, Rafael Castro rdcas...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Ardcastro%40google.com wrote: +hermes Good point, this is really tricky. The problem here is that we don't actually have the DOM element until the widget is attached. I see 2 options: 1-) We force the UiField to be a LazyDomElement, so this is explicit. 2-) We use PotentialElement with a resolver that throws an Exception (i.e., it's only really resolved when it's attached). what do you think? ps.: really nice tests, thanks for putting them together! On Tue, Aug 16, 2011 at 5:13 PM, rj...@google.comhttp://www.google.com/url?sa=Dq=mailto%3Arjrjr%40google.com wrote: On 2011/08/17 00:12:24, rjrjr wrote: Ready for review. Rafa, this turned up one issue that concerns me: most @UiField fields are not filled in until the widget is attached to the dom, but we're not consistent about it. See the big comment in testDeep. http://gwt-code-reviews.**appspot.com/1527804/http://www.google.com/url?sa=Dq=http%3A%2F%2Fgwt-code-reviews.appspot.com%2F1527804%2F -- --Hermes Freitas -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: Re: [gwt-contrib] Aw: Regression: instanceof compiler issue
That's a generous offer, but I don't know that anyone is going to be able to take you up on it. We'll keep an eye out for it here. And if you do manage to trim your failing code down to something you can share, we'll jump. Are you building against trunk? If not, you might try and see if the problem is still there. On Wed, Aug 17, 2011 at 4:22 AM, dflorey daniel.flo...@gmail.com wrote: Hi, unfortunately I did not manage to reproduce the issue in a clean project. But I'd like to show the issue to someone with my existing projects. I've been able to find a workaround by adding the module classes to a fake rpc service. I could setup a TeamViewer session to show the issue on my machine. Daniel -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Aw: Regression: instanceof compiler issue
Can you share some actual code? On Aug 9, 2011 7:37 AM, dflorey daniel.flo...@gmail.com wrote: BTW: It works find in dev mode, just fails when compiled -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Aw: Regression: instanceof compiler issue
Anything that we can compile that actually reproduces the problem. On Tue, Aug 9, 2011 at 8:46 AM, Daniel Florey daniel.flo...@gmail.comwrote: Just found out that the problem only occurs when the interface is empty (marker interface). As soon as any method is added, the instanceof works. What code snippets do you need? My project is too big to share ;-) Daniel 2011/8/9 Ray Ryan rj...@google.com Can you share some actual code? On Aug 9, 2011 7:37 AM, dflorey daniel.flo...@gmail.com wrote: BTW: It works find in dev mode, just fails when compiled -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a new CellTableHeaderBuilder API, which allows custom headers and footers in CellTable. C... (issue1499808)
SGTM On Mon, Aug 8, 2011 at 7:15 PM, jlaba...@google.com wrote: Ray and I hashed out the names a bit. I don't mind shortening CellTableHeaderCreator to HeaderCreator. I don't think it'll cause any conflicts down the road. @rjrjr - any objections to the shorter name? http://gwt-code-reviews.**appspot.com/1499808/diff/1/** samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.javahttp://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java File samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java (right): http://gwt-code-reviews.**appspot.com/1499808/diff/1/** samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java#**newcode82http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode82 samples/showcase/src/com/**google/gwt/sample/showcase/** client/content/cell/**CwCustomDataGrid.java:82: public class CwCustomDataGrid extends ContentWidget { On 2011/08/02 03:47:57, skybrian wrote: This example doesn't really hang together. On the one hand it's showing friends and birthdays (like a social app or a contacts database) and on the other it's showing who's eligible for retirement benefits (like an HR app). It seems like we should pick one or the other? Perhaps take out retirement benefits and replace that with something that makes more sense for a contacts database? I removed the retirement benefits row completely. We only need one example of a spanning row, so the birthday this month is fine. I also wonder if we should simplify it or split into two or more examples where each demonstrates one thing. For someone reading documentation, 800 lines of sample code seems really big and I'd much prefer to read a smaller example. This is a sample that is meant to illustrate what you can do with GWT. We show source code for people who want to delve into it, but the primary purpose of Showcase is to show off GWT, not to provide example code. http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** user/src/com/google/gwt/user/**cellview/client/**AbstractCellTable.javahttp://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java File user/src/com/google/gwt/user/**cellview/client/** AbstractCellTable.java (right): http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** user/src/com/google/gwt/user/**cellview/client/**AbstractCellTable.java#** newcode493http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode493 user/src/com/google/gwt/user/**cellview/client/** AbstractCellTable.java:493: rawHtml = rawHtml.substring(7, rawHtml.length() - 8); On 2011/08/02 03:47:57, skybrian wrote: Perhaps add: assert tag.size() == 5 Done. http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.javahttp://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java File user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java (right): http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java#**newcode32http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode32 user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java:**32: public interface CellTableHeaderBuilderT { On 2011/08/02 03:47:57, skybrian wrote: How about just HeaderBuilder? Its specific to CellTable/DataGrid. Other CellWidgets might have a different header builder. Ray and I came up with CellTableHeaderCreator. http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java#**newcode35http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode35 user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java:**35: * A utility for building the header. On 2011/08/02 03:47:57, skybrian wrote: (or footer) Done. http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java#**newcode44http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode44 user/src/com/google/gwt/user/**cellview/client/** CellTableHeaderBuilder.java:**44: public abstract class UtilityT { On 2011/08/02
[gwt-contrib] Re: Fixing a bug in HasDataPresenter where paging to a negative row index causes an IndexOutOfRange ... (issue1513804)
LGTM On Mon, Aug 8, 2011 at 4:42 PM, jlaba...@google.com wrote: Reviewers: rjrjr, Description: Fixing a bug in HasDataPresenter where paging to a negative row index causes an IndexOutOfRange exception. We now properly trim the keyboard selected row to a non-negative value. Issue: 6383 Please review this at http://gwt-code-reviews.**appspot.com/1513804/http://gwt-code-reviews.appspot.com/1513804/ Affected files: M user/src/com/google/gwt/user/**cellview/client/**HasDataPresenter.java M user/test/com/google/gwt/user/**cellview/client/** HasDataPresenterTest.java Index: user/src/com/google/gwt/user/**cellview/client/** HasDataPresenter.java ==**==**=== --- user/src/com/google/gwt/user/**cellview/client/**HasDataPresenter.java (revision 10507) +++ user/src/com/google/gwt/user/**cellview/client/**HasDataPresenter.java (working copy) @@ -719,8 +719,9 @@ } else if (KeyboardPagingPolicy.CHANGE_**PAGE == keyboardPagingPolicy) { // Go to previous page. while (index 0) { -newPageStart -= pageSize; -index += pageSize; +int shift = Math.min(pageSize, newPageStart); +newPageStart -= shift; +index += shift; } // Go to next page. @@ -731,9 +732,10 @@ } else if (KeyboardPagingPolicy.**INCREASE_RANGE == keyboardPagingPolicy) { // Increase range at the beginning. while (index 0) { -newPageSize += PAGE_INCREMENT; -newPageStart -= PAGE_INCREMENT; -index += PAGE_INCREMENT; +int shift = Math.min(PAGE_INCREMENT, newPageStart); +newPageSize += shift; +newPageStart -= shift; +index += shift; } if (newPageStart 0) { index += newPageStart; Index: user/test/com/google/gwt/user/**cellview/client/** HasDataPresenterTest.java ==**==**=== --- user/test/com/google/gwt/user/**cellview/client/**HasDataPresenterTest.java (revision 10507) +++ user/test/com/google/gwt/user/**cellview/client/**HasDataPresenterTest.java (working copy) @@ -750,6 +750,18 @@ view.**assertKeyboardSelectedRowEmpty**(); assertEquals(10, presenter.getVisibleRange().**getStart()); assertEquals(10, presenter.getVisibleRange().**getLength()); + +// Negative index out of range. +presenter.setVisibleRange(new Range(3, 10)); +presenter.**setKeyboardSelectedRow(3, false, false); +populatePresenter(presenter); +presenter.flush(); +presenter.**setKeyboardSelectedRow(-4, false, false); +populatePresenter(presenter); +presenter.flush(); +assertEquals(0, presenter.**getKeyboardSelectedRow()); +assertEquals(0, presenter.getVisibleRange().**getStart()); +assertEquals(10, presenter.getVisibleRange().**getLength()); } public void testSetKeyboardSelectedRowCurr**entPage() { @@ -880,6 +892,18 @@ assertEquals(0, presenter.getVisibleRange().**getStart()); pageSize += 10; assertEquals(pageSize, presenter.getVisibleRange().**getLength()); + +// Negative index out of range. +presenter.setVisibleRange(new Range(3, 10)); +presenter.**setKeyboardSelectedRow(3, false, false); +populatePresenter(presenter); +presenter.flush(); +presenter.**setKeyboardSelectedRow(-4, false, false); +populatePresenter(presenter); +presenter.flush(); +assertEquals(0, presenter.**getKeyboardSelectedRow()); +assertEquals(0, presenter.getVisibleRange().**getStart()); +assertEquals(13, presenter.getVisibleRange().**getLength()); } public void testSetRowCount() { -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a spot for database column name in Column. Allows us to create an updated SQL statement w... (issue1503806)
You don't have to add a param type if the app data slot is simply of type object. No one will die if they have to cast the thing. But if the people who originally requested this feature are willing to back off and maintain a map instead, is even that much api clutter really warranted? @portersi, what stopped you from using a custom column class instead of a map? On Tue, Aug 2, 2011 at 1:53 PM, jlaba...@google.com wrote: This came up before, and here is the rationale. If you add an app specific data object to Column, you have to add another parameterized type to Column. That's more verbose, somewhat confusing, and it will be Void for most users anyway. I'm a big fan of parameterized types because they make the API feels more bulletproof, but I also think parameterized types can be cumbersome if they aren't intended to support the most common use cases. Subclassing Column is good option if you need a lot of app specific data. Yes you have to cast it, but who cares if you're in control of which columns go into your table. In the original design, we envisioned people subclassing Column for app specific data. So, now we're special casing the data store field as a string. But MANY databases use String names to describe columns, and its just so convenient to have that field accessible from the Column itself. I've seen more than a few requests for this feature. If you are using an SQL database, this will probably come in handy. If you don't need it, don't use it, and the compiler should compile it out. On 2011/08/02 20:13:12, rjrjr wrote: It's John L's call, but that's certainly my preference. On Tue, Aug 2, 2011 at 1:10 PM, John Porter Simons porte...@google.comwrote: The other way we (me and dramos@) discussed doing this was, in our CellTable subclass, have a map from Column to String to store these database column names. I can update to that and revert this if you like. On Tue, Aug 2, 2011 at 1:01 PM, Ray Ryan mailto:rj...@google.com wrote: I was biting my tongue on this one, but I guess I'll jump in and agree, this smells bad. @jlabanca, is there no hook in Column or maybe Cell.Context where this kind of app-specific data can be added? On Tue, Aug 2, 2011 at 12:32 PM, Jeff Larsen mailto:larse...@gmail.com wrote: I'm inclined to agree with Stephen here. No where else in GWT widgetry is there a reference to database related things. I don't think this is a big deal either, but it seems like a more application specific thing rather than something that belongs inside GWT proper. -- http://groups.google.com/**group/Google-Web-Toolkit-**Contributorshttp://groups.google.com/group/Google-Web-Toolkit-Contributors http://gwt-code-reviews.**appspot.com/1503806/http://gwt-code-reviews.appspot.com/1503806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: GWT Code Review planned outage Thursday 4 Aug 2011 5pm ET
http://gwt-code-reviews.appspot.com/ will be read only in five minutes or so. On Wed, Aug 3, 2011 at 12:09 PM, Ray Ryan rj...@google.com wrote: We will be putting the Rietveld server at http://gwt-code-reviews.appspot.com/ into read-only mode for a few hours at about 5pm Eastern Time tomorrow, Thursday August 4th, to deal with some maintenance. rjrjr -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] GWT Code Review [Rietveld]: Better, faster stronger
In the last couple of hours we migrated http://gwt-code-reviews.appspot.com/to a High Replication Datastorehttp://googleappengine.blogspot.com/2011/01/announcing-high-replication-datastore.html. This should be transparent to you, except for much reduced latency.[1] Huge thanks to Fred Sauer, who did all the work. Enjoy! rjrjr [1] Actually, users of the chat notification feature will need to reset that preference to make it go again. So far this is the only hiccup we've noticed. - visit http://gwt-code-reviews.appspot.com/settings - uncheck Notify by chat, save settings - recheck Notify by chat, save settings - accept the new chat invite from the new app id -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] GWT Code Review planned outage Thursday 4 Aug 2011 5pm ET
We will be putting the Rietveld server at http://gwt-code-reviews.appspot.com/ into read-only mode for a few hours at about 5pm Eastern Time tomorrow, Thursday August 4th, to deal with some maintenance. rjrjr -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make generator result caching framework api available publically. (issue1468804)
I'm still not crazy about having addClientData() and getClientData() on separate objects. It seems to me that you've violated your own principal that the GeneratorContext should be the only object that has to get passed to the generator's helpers. Can addClientData() move to the context? That would require getClientData() to be able to retrieve both cached and freshly added data. Is that practical? Sounds like it would be hugely convenient. On Wed, Aug 3, 2011 at 12:43 PM, jbrosenb...@google.com wrote: Some responses. I did consider most of your suggestions, and in fact for a while I did have things as you suggest, so great minds think alike :). I'll outline my reasoning: First, IncrementalGenerators have to live in the same ecosystem as all the currently extent non-incremental Generator implementations. Having a uniform rebind process whereby the StandardRebindOracle is agnostic to this greatly simplified things. If we were starting from scratch, I think it would probably look quite a bit different. Standard Generators only return a type-name (which can possibly be null). The RebindResult allows legacy generators to be cleanly wrapped and presented to the rebind oracle in a uniform way. A RebindResult is not the same as a CachedRebindResult. The StandardRebindOracle uses the info in the RebindResult to decide how to proceed. It may or may not choose to construct a cache entry, and that cached entry might contain a partial subset of newly generated types and types from a previous cache entry. Since a generator itself shouldn't need to know how to integrate cached types and artifacts, this logic is better left up to the StandardRebindOracle. The RebindResult also contains a target type name, and client data, so it's just a container to wrap a generators results. In the case that an IncrementalGenerator decides it can return quickly and request that all previously cached output can be reused, it doesn't need to do anything, including not re-committing cached compilation units and artifacts to the context, etc. That's all magic handled by the rebind oracle. A CachedRebindResult is specifically input to an IncrementalGenerator, but it's cumbersome to pass it in as a separate arg to generateIncrementally. Having it live in the GeneratorContext makes one less thing a generator implementation needs to pass around to it's internal methods (since GeneratorContext at the least is commonly needed all over the place for a generator anyway). Remember too, that much of the time, an IncrementalGenerator will run when generatorResultCaching is not enabled, such as currently for all web-mode compiles, so it's more of a contextual thing anyway, which makes it more intuitive for it to live in the GeneratorContext. I did have a separate GeneratContextExt interface, which extended the base GeneratorContext interface, but ultimately this became unnecessary, and it greatly simplified the framework by unifying things. I think you had a couple other comments offline (but not summarized above), I'll wait for your further comments to respond to those. http://gwt-code-reviews.**appspot.com/1468804/http://gwt-code-reviews.appspot.com/1468804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make generator result caching framework api available publically. (issue1468804)
Oh, and putClientData seems like a better name. On Wed, Aug 3, 2011 at 1:02 PM, Ray Ryan rj...@google.com wrote: I'm still not crazy about having addClientData() and getClientData() on separate objects. It seems to me that you've violated your own principal that the GeneratorContext should be the only object that has to get passed to the generator's helpers. Can addClientData() move to the context? That would require getClientData() to be able to retrieve both cached and freshly added data. Is that practical? Sounds like it would be hugely convenient. On Wed, Aug 3, 2011 at 12:43 PM, jbrosenb...@google.com wrote: Some responses. I did consider most of your suggestions, and in fact for a while I did have things as you suggest, so great minds think alike :). I'll outline my reasoning: First, IncrementalGenerators have to live in the same ecosystem as all the currently extent non-incremental Generator implementations. Having a uniform rebind process whereby the StandardRebindOracle is agnostic to this greatly simplified things. If we were starting from scratch, I think it would probably look quite a bit different. Standard Generators only return a type-name (which can possibly be null). The RebindResult allows legacy generators to be cleanly wrapped and presented to the rebind oracle in a uniform way. A RebindResult is not the same as a CachedRebindResult. The StandardRebindOracle uses the info in the RebindResult to decide how to proceed. It may or may not choose to construct a cache entry, and that cached entry might contain a partial subset of newly generated types and types from a previous cache entry. Since a generator itself shouldn't need to know how to integrate cached types and artifacts, this logic is better left up to the StandardRebindOracle. The RebindResult also contains a target type name, and client data, so it's just a container to wrap a generators results. In the case that an IncrementalGenerator decides it can return quickly and request that all previously cached output can be reused, it doesn't need to do anything, including not re-committing cached compilation units and artifacts to the context, etc. That's all magic handled by the rebind oracle. A CachedRebindResult is specifically input to an IncrementalGenerator, but it's cumbersome to pass it in as a separate arg to generateIncrementally. Having it live in the GeneratorContext makes one less thing a generator implementation needs to pass around to it's internal methods (since GeneratorContext at the least is commonly needed all over the place for a generator anyway). Remember too, that much of the time, an IncrementalGenerator will run when generatorResultCaching is not enabled, such as currently for all web-mode compiles, so it's more of a contextual thing anyway, which makes it more intuitive for it to live in the GeneratorContext. I did have a separate GeneratContextExt interface, which extended the base GeneratorContext interface, but ultimately this became unnecessary, and it greatly simplified the framework by unifying things. I think you had a couple other comments offline (but not summarized above), I'll wait for your further comments to respond to those. http://gwt-code-reviews.**appspot.com/1468804/http://gwt-code-reviews.appspot.com/1468804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make generator result caching framework api available publically. (issue1468804)
On Wed, Aug 3, 2011 at 2:36 PM, jbrosenb...@google.com wrote: On 2011/08/03 20:03:28, rjrjr wrote: Oh, and putClientData seems like a better name. Done On Wed, Aug 3, 2011 at 1:02 PM, Ray Ryan mailto:rj...@google.com wrote: I'm still not crazy about having addClientData() and getClientData() on separate objects. It seems to me that you've violated your own principal that the GeneratorContext should be the only object that has to get passed to the generator's helpers. Can addClientData() move to the context? That would require getClientData() to be able to retrieve both cached and freshly added data. Is that practical? Sounds like it would be hugely convenient. I'm not sure I see why it makes sense for it not to be part of the cached result. It is indeed closely associated specifically with that previously generated result. By keeping it as part of the cachedResult, it makes one less thing that needs to be passed around internally to the generator (only the context needs to be passed around). No, I have two things to pass around. I have to pass around generatorContext to allow helpers to fetch cached data, and I also have to pass around something to allow helpers to post new cached data. That is, in the current scheme I have to write something like: generateIncrementally(TreeLogger logger, GeneratorContext context, String typeName) { MapString, Serializable helperCachedData; new HelperOne(context, helperCachedData).run(); new HelperTwo(context, helperCachedData).run(); RebindResult result = new RebindResult(USE_WHATEVER, typeName + Impl); for (Map.EntryString, Serializable entry : helperCachedData) { result.putClientData(entry.key, entry.value); } return result; } instead of generateIncrementally(TreeLogger logger, GeneratorContext context, String typeName) { new HelperOne(context).run(); new HelperTwo(context).run(); return new RebindResult(USE_WHATEVER, typeName + Impl); } Further, my helpers have to look in two places for cacheable data generated by an upstream helper. The context is specific to the currently running generator environment, and generally gets reset for each generator invocation. Adding more data to the context requires more api and more things for the context to keep track of (currently it just keeps track of a CachedRebindResult object, which it would still need to do). Maybe there's a better name than clientData here, suggestions? It's specific to a generator run invoked that was invoked with the same rebind rule and requested type name as are currently in effect. But it's historical data, and in fact is not essential at all to the generator (if it's not there, the generator will run to completion). http://gwt-code-reviews.**appspot.com/1468804/http://gwt-code-reviews.appspot.com/1468804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make generator result caching framework api available publically. (issue1468804)
LGTM On Wed, Aug 3, 2011 at 4:17 PM, jbrosenb...@google.com wrote: Well, a generator has to keep it's own state anyway, in practice. In the case of AbstractClientBundleGenerator, it actually creates a ResourceContext and passes that around, along with the GeneratorContext (this was before any IncrementalGenerator enhancements). In the case of RPC, there are 2 SerializableTypeOracleBuilders and the GeneratorContext that get passed around. I'm not sure, it sounds like you are asking to have a generic functionality for the GeneratorContext, so generators can park data there as needed (not really specific to IncrementalGenerators). What would the semantics be, would it always get added to the cache in entirety, or is there a separate api for saving data for current state and another for specifically placing data that needs to be cached? Is it cleared when each new generator invocation occurs? Since this change is not attempting to modify how the framework behaves with respect to legacy generators, I'd rather not invent api in this patch that's not related to the task at hand. Using your example, before any IncrementalGenerator work, both ClientBundle and RPC look similar to your first example (e.g. there's both a GeneratorContext and helperCachedData passed around everywhere). The only change I've done is to add a way (without modifying current generators), to prepare extra data for subsequent cache reuse checking. In fact, the client data stored for cache reuse checking is generally newly prepared data, not really needed unless caching is enabled. So, the way things work now, are very similar to the current state, but with cache use support added at the beginning (to check reusability) and at the end (to prepare and remember data for future checking). Like so: generateIncrementally(**TreeLogger logger, GeneratorContext context, String typeName) { if (checkCacheReusability(**context)) return new RebindResult(RebindMode.USE_**CACHE,...) // this part is no different for non-incremental generators MyGeneratorStateClass generatorState; new HelperOne(context, generatorState).run(); new HelperTwo(context, generatorState).run(); if (canBeCacheable(context)) { // possibly expensive task PreparedClientData pcd = prepareClientData(context, helperData); RebindResult result = new RebindResult(USE_WHATEVER, typeName + Impl); result.putClientData(pcd, pcd); return result; } else { // no need to prepare any client data return new RebindResult(RebindMode.USE_**ALL_NEW_WITH_NO_CACHING,...); } } http://gwt-code-reviews.**appspot.com/1468804/http://gwt-code-reviews.appspot.com/1468804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making lazy widgets generation the default option. (issue1499809)
Lets warn gwt-announce that the default will change next Tuesday, and encourage them to try it out themselves in the meantime. Are you okay driving that, and with waiting another week? On Aug 2, 2011 6:33 AM, her...@google.com wrote: Reviewers: rjrjr, Description: Making lazy widgets generation the default option. Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1499809/ Affected files: M user/src/com/google/gwt/uibinder/UiBinder.gwt.xml M user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java Index: user/src/com/google/gwt/uibinder/UiBinder.gwt.xml === --- user/src/com/google/gwt/uibinder/UiBinder.gwt.xml (revision 10484) +++ user/src/com/google/gwt/uibinder/UiBinder.gwt.xml (working copy) @@ -31,7 +31,7 @@ rendering mode and make some widgets lazily created. This is still experimental but should be the default option in a soon future. -- define-configuration-property name=UiBinder.useLazyWidgetBuilders is-multi-valued=false/ - set-configuration-property name=UiBinder.useLazyWidgetBuilders value=false/ + set-configuration-property name=UiBinder.useLazyWidgetBuilders value=true/ generate-with class=com.google.gwt.uibinder.rebind.UiBinderGenerator when-type-assignable class=com.google.gwt.uibinder.client.UiRenderer/ Index: user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java === --- user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java (revision 10484) +++ user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java (working copy) @@ -50,7 +50,7 @@ private static final String XSS_SAFE_CONFIG_PROPERTY = UiBinder.useSafeHtmlTemplates; private static final String LAZY_WIDGET_BUILDERS_PROPERTY = UiBinder.useLazyWidgetBuilders; - + private static boolean gaveSafeHtmlWarning; /** @@ -212,7 +212,7 @@ } private Boolean useLazyWidgetBuilders(MortalLogger logger, PropertyOracle propertyOracle) { - return extractConfigProperty(logger, propertyOracle, LAZY_WIDGET_BUILDERS_PROPERTY, false); + return extractConfigProperty(logger, propertyOracle, LAZY_WIDGET_BUILDERS_PROPERTY, true); } private Boolean useSafeHtmlTemplates(MortalLogger logger, PropertyOracle propertyOracle) { -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Adding a spot for database column name in Column. Allows us to create an updated SQL statement w... (issue1503806)
I was biting my tongue on this one, but I guess I'll jump in and agree, this smells bad. @jlabanca, is there no hook in Column or maybe Cell.Context where this kind of app-specific data can be added? On Tue, Aug 2, 2011 at 12:32 PM, Jeff Larsen larse...@gmail.com wrote: I'm inclined to agree with Stephen here. No where else in GWT widgetry is there a reference to database related things. I don't think this is a big deal either, but it seems like a more application specific thing rather than something that belongs inside GWT proper. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Adding a spot for database column name in Column. Allows us to create an updated SQL statement w... (issue1503806)
It's John L's call, but that's certainly my preference. On Tue, Aug 2, 2011 at 1:10 PM, John Porter Simons porte...@google.comwrote: The other way we (me and dramos@) discussed doing this was, in our CellTable subclass, have a map from Column to String to store these database column names. I can update to that and revert this if you like. On Tue, Aug 2, 2011 at 1:01 PM, Ray Ryan rj...@google.com wrote: I was biting my tongue on this one, but I guess I'll jump in and agree, this smells bad. @jlabanca, is there no hook in Column or maybe Cell.Context where this kind of app-specific data can be added? On Tue, Aug 2, 2011 at 12:32 PM, Jeff Larsen larse...@gmail.com wrote: I'm inclined to agree with Stephen here. No where else in GWT widgetry is there a reference to database related things. I don't think this is a big deal either, but it seems like a more application specific thing rather than something that belongs inside GWT proper. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add font-face support to CssResource. (issue1502806)
Unnur, would you be comfortable taking this? On Tue, Jul 26, 2011 at 12:12 PM, b...@google.com wrote: Reviewers: rjrjr, Message: This is a re-spin of a patch that was written back in October but that got dropped. The original patch is at http://gwt-code-reviews.**appspot.com/943802http://gwt-code-reviews.appspot.com/943802 . Description: Add font-face support to CssResource. Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.**appspot.com/1502806/http://gwt-code-reviews.appspot.com/1502806/ Affected files: M user/src/com/google/gwt/**resources/css/**CssGenerationVisitor.java M user/src/com/google/gwt/**resources/css/GenerateCssAst.**java A user/src/com/google/gwt/**resources/css/ast/CssFontFace.**java M user/src/com/google/gwt/**resources/css/ast/**CssNodeCloner.java M user/src/com/google/gwt/**resources/css/ast/CssVisitor.**java M user/test/com/google/gwt/**resources/client/**CSSResourceTest.java M user/test/com/google/gwt/**resources/client/test.css -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
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
[gwt-contrib] Re: Remove RenderablaComposite. \o/ (issue1455804)
LGTM On Tue, Jun 14, 2011 at 10:09 AM, rchan...@google.com wrote: On 2011/06/14 16:54:43, rdcastro wrote: LGTM http://gwt-code-reviews.**appspot.com/1455804/http://gwt-code-reviews.appspot.com/1455804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] RE:Reduce the use of old event bus classes. The only public api that (issue1446819)
Ready for review. On Tue, Jun 14, 2011 at 5:03 PM, rj...@google.com {subItem.from.uri} wrote: http://gwt-code-reviews.appspot.com/1446819/http://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1446819/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: UiBinder - register custom ElementParser (issue1454804)
As soon as we have done that, we can't make changes to UiBinderWriter and all the other classes the parsers actually talk to, nor can we make sweeping changes to the code they generate. If the problem is retrofitting widgets you don't own, would a non-annotation alternative to UiChild get the job done? Perhaps a config file, perhaps a builder of some kind? On Thu, Jun 9, 2011 at 8:16 PM, jus...@jhickman.com wrote: Unfortunately @UiChild doesn't handle the flexibility I need, especially in cases where you are wanting to write parsers for a 3rd party widget library and do not have the ability to modify the source to include annotations. Is there any way of convincing you to make it more extendable? Even without providing an official mechanism for registering ElementParsers, doing small things such as the following would do wonders: * In UiBinderGenerator, extract the instantiation of the UiBinderWriter into a protected method so that developers can subclass the UiBinderGenerator and construct their own subclass of UiBinderWriter * In UiBinderWriter, make addElementParser() protected rather than private. http://gwt-code-reviews.**appspot.com/1454804/http://gwt-code-reviews.appspot.com/1454804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: UiBinder - register custom ElementParser (issue1454804)
Exactly. And I was thinking we could introduce something like BuildsWidgetW extends IsWidget extends IsWidget. UiBinder could learn to honor the setters and such of the underlying widget as well as the BuildsWidget. On Fri, Jun 10, 2011 at 10:41 AM, Thomas Broyer t.bro...@gmail.com wrote: On Fri, Jun 10, 2011 at 7:22 PM, Ray Ryan rj...@google.com wrote: As soon as we have done that, we can't make changes to UiBinderWriter and all the other classes the parsers actually talk to, nor can we make sweeping changes to the code they generate. If the problem is retrofitting widgets you don't own, would a non-annotation alternative to UiChild get the job done? Perhaps a config file, perhaps a builder of some kind? Particularly as you should be able to make an IsWidget class with @UiChild methods as a lightweight wrapper around the widget, kind of a builder (similar to PotentialElement but at a Widget level). -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resolve PotentialElement children before inserting them into a container. (issue1454810)
LGTM On Fri, Jun 10, 2011 at 10:29 AM, jul...@google.com wrote: Uploaded patch with assertion in DOM.insertListItem as patch set 3. http://gwt-code-reviews.**appspot.com/1454810/diff/3001/** user/src/com/google/gwt/user/**client/DOM.javahttp://gwt-code-reviews.appspot.com/1454810/diff/3001/user/src/com/google/gwt/user/client/DOM.java File user/src/com/google/gwt/user/**client/DOM.java (right): http://gwt-code-reviews.**appspot.com/1454810/diff/3001/** user/src/com/google/gwt/user/**client/DOM.java#newcode974http://gwt-code-reviews.appspot.com/1454810/diff/3001/user/src/com/google/gwt/user/client/DOM.java#newcode974 user/src/com/google/gwt/user/**client/DOM.java:974: String value, int index) { On 2011/06/10 17:12:23, rjrjr wrote: Seems like the assert line should be added here too. assert !PotentialElement.isPotential(**selectElem) : Cannot insert into a PotentialElement; Done. http://gwt-code-reviews.**appspot.com/1454810/http://gwt-code-reviews.appspot.com/1454810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make UriUtils#unsafeCastFromUntrustedString gracefully handle null for (issue1443814)
In general we try to be null-intolerant, although I don't know how consistent we are about it. Basically, nulls should never be quietly cleaned up for you but rather should fail fast if practical. If null is a legal value, it should serve a specific purpose. On Tue, Jun 7, 2011 at 12:44 AM, Thomas Broyer t.bro...@gmail.com wrote: On Tue, Jun 7, 2011 at 1:32 AM, Christoph Kern x...@google.com wrote: It turns out it was easier to fix the specific case this broke in client code (a test that ended up passing null for a URL). Which raises the question, should Image gracefully handle null for URLs, or should the API docs clarify that non-null values are expected? Is there a convention for handling nulls in the GWT API? SafeHtmlUtils at least doesn't handle 'null' (and will throw NPEs). I'd rather have SafeUri follow the same pattern, whether it is to throw NPEs or is changed as proposed here for unsafeCastFromUntrustedString. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Does Composite still make sense?
That is an excellent question. I don't think anyone has yet tried to go that route in earnest, and I suspect the first to do so will find that IsWidget is not yet as first class as it should be, just due to undiscovered corner cases and such. But it sure would be interesting to try to make it work. On Tue, Jun 7, 2011 at 2:31 AM, Andrés Testi andres.a.te...@gmail.comwrote: Since IsWidget is a first class interface, what is the use case to use Composite instead of simply implement IsWidget? Thanks in advance. - Andrés -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduce PotentialElement to help with the implementation of IsRenderable widgets. (issue1450810)
On Thu, Jun 2, 2011 at 8:16 AM, jlaba...@google.com wrote: LGTM http://gwt-code-reviews.appspot.com/1450810/diff/6002/user/src/com/google/gwt/user/client/ui/PotentialElement.java File user/src/com/google/gwt/user/client/ui/PotentialElement.java (right): http://gwt-code-reviews.appspot.com/1450810/diff/6002/user/src/com/google/gwt/user/client/ui/PotentialElement.java#newcode47 user/src/com/google/gwt/user/client/ui/PotentialElement.java:47: return o...@com.google.gwt.user.client.ui.UIObject::finishBuild()(); Does referencing o here create a circular reference between PotentialElement and UiObject that leads to a memory leak in IE? I think the answer is no, because PotentialElement isn't really a native Element, and the IE memory leak is specific to circular references between native Elements and logical objects. Can anyone verify? I had the same reaction. Everything I found on the issue was specific to dom objects. http://gwt-code-reviews.appspot.com/1450810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds ability to query the generator context whether a rebind rule exists for a given type (issue1450806)
LGTM On Tue, May 31, 2011 at 1:39 PM, jbrosenb...@google.com wrote: http://gwt-code-reviews.appspot.com/1450806/diff/3002/dev/core/src/com/google/gwt/core/ext/GeneratorContext.java File dev/core/src/com/google/gwt/core/ext/GeneratorContext.java (right): http://gwt-code-reviews.appspot.com/1450806/diff/3002/dev/core/src/com/google/gwt/core/ext/GeneratorContext.java#newcode34 dev/core/src/com/google/gwt/core/ext/GeneratorContext.java:34: * likely to succeed. On 2011/05/31 16:27:01, rjrjr wrote: Why is the argument a string rather than, say, JClassType? Since this method will be called from within a generator, there may not be a valid JClassType from type oracle (but we can check for the availability of a rebind rule, by sourceTypeName, which is how they are expressed in rebind rules). The actual type might come into existence after the current (and other) generators complete. Wouldn't the second sentence would be more accurate as: is likely to succeed for a type with no default constructor. (Any concrete type with a zero args constructor can be instantiated via GWT.create().) Can't really check whether a concrete type with a zero args constructor might be available, since it might only come into being after other generators run. Also, it isn't GWT.create(sourceTypeName), the argument is a class literal. Good point. I've removed references to GWT.create() altogether in the javadoc. http://gwt-code-reviews.appspot.com/1450806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add RequestContext.find() to support chained requests. (issue1448806)
LGTM On Fri, May 27, 2011 at 7:38 AM, b...@google.com wrote: The base interface for RequestFactory service endpoints. Add disclaimer explaining that this interface (and the others) are normally implemented by generated code, and are subject to incompatible updates? Done. Also added a FakeRequestContext base type in a testing subpackage. And should log an item on the issue tracker with the appropriate breaking change label. http://code.google.com/p/google-web-toolkit/issues/detail?id=6413 Ha! Do you have other sneaky literals like this lying around? Calls to the RequestData constructor are usually only in generated code. http://gwt-code-reviews.appspot.com/1448806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Adding new DataGrid widget. DataGrid is a variation of CellTable that supports a fixed header a... (issue1450805)
On Wed, May 25, 2011 at 9:13 AM, Thomas Broyer t.bro...@gmail.com wrote: On Wednesday, May 25, 2011 5:46:34 PM UTC+2, Jeff Larsen wrote: On Wednesday, May 25, 2011 10:21:18 AM UTC-5, Thomas Broyer wrote: On Wednesday, May 25, 2011 3:29:35 PM UTC+2, Jeff Larsen wrote: Wow, this is awesome. +1 I haven't started digging into the code yet, but I would like to point out a minor nit. In Firefox giving the scrollbars opacity looks OK, but in chrome, it doesn't look right (see attached file). Personally, I think people are used to not being able to see through scrollbars so I would recommend just removing the opacity. This was part of another patch a few weeks ago, it's the CustomScrollPanel. What you're seeing only applies on Windows (IIRC, I had that on Windows XP too on the CustomScrollPanel demo that John put online at the time it proposed the widget), as it shows well on Ubuntu. It's a Chrome bug that I think is not worth working around in GWT. See http://code.google.com/p/chromium/issues/detail?id=24524 Yea, that makes sense that its a chrome bug. I'll just change the css in my application to not use any opacity to get around the issue. I think my main concern was that most people aren't used to opacity in their scrollbars. When I looked at it initially, it just didn't feel like any experiences I've had on the web previously. Actually, when I saw the transparency, I immediately checked whether it wasn't Chrome's default behavior on Linux ;-) (given that I switched from Windows less than a week ago, it woudln't have surprised me much that I wouldn't have noticed it, as much things I read don't have an horizontal scrollbar, and thus wouldn't need transparency on the vertical scrollbar either). Is there any bug tracker you don't know by heart? :) I didn't actually know that bug until 2 mins before sending the previous message: I just search on crbug.com for scroll bar opacity. You don't need to know things by heart when you have a good search engine ;-) (well, I have a good memory, so things regularly ring a bell, and the search engine is then just the mean to find it back) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors Somehow this seems relevant: http://xkcd.com/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix escaping issue with SafeHtml in Safari3 (issue1443806)
LGTM On Wed, May 25, 2011 at 11:53 AM, sbruba...@google.com wrote: Reviewers: rjrjr, Description: Fix escaping issue with SafeHtml in Safari3 Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1443806/ Affected files: M user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java Index: user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java === --- user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java (revision 10224) +++ user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java (working copy) @@ -631,8 +631,8 @@ public void testUiTextWithSafeHtml() { assertEquals(widgetUi.htmlWithComputedSafeHtml.getHTML().toLowerCase(), bthis text should be bold!/b); -assertEquals(widgetUi.htmlWithComputedText.getHTML().toLowerCase(), -lt;bgt;this text won't be bold!lt;/bgt;); +assertEquals(widgetUi.htmlWithComputedText.getHTML().toLowerCase() +.replaceAll(, gt;), lt;bgt;this text won't be bold!lt;/bgt;); assertEquals(widgetUi.labelWithComputedText.getText().toLowerCase(), bthis text won't be bold!/b); } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add the method name to the message of InvocationService to make it easier to (issue1450803)
LGTM On Tue, May 24, 2011 at 5:04 AM, gaill...@google.com wrote: Reviewers: rjrjr, Description: Add the method name to the message of InvocationService to make it easier to find out the rootcause of the exception. Please review this at http://gwt-code-reviews.appspot.com/1450803/ Affected files: M user/src/com/google/gwt/rpc/client/impl/RpcCallbackAdapter.java M user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java M user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java Index: user/src/com/google/gwt/rpc/client/impl/RpcCallbackAdapter.java === --- user/src/com/google/gwt/rpc/client/impl/RpcCallbackAdapter.java (revision 10205) +++ user/src/com/google/gwt/rpc/client/impl/RpcCallbackAdapter.java (working copy) @@ -83,7 +83,7 @@ caught = new StatusCodeException(statusCode, encodedResponse); } else if (encodedResponse == null) { // This can happen if the XHR is interrupted by the server dying -caught = new InvocationException(No response payload); +caught = new InvocationException(No response payload from + methodName); } else { result = (T) streamFactory.createStreamReader(encodedResponse).readObject(); } Index: user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java === --- user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java (revision 10205) +++ user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java (working copy) @@ -372,7 +372,8 @@ return rb.send(); } catch (RequestException ex) { InvocationException iex = new InvocationException( - Unable to initiate the asynchronous service invocation -- check the network connection, + Unable to initiate the asynchronous service invocation ( + + methodName + ) -- check the network connection, ex); callback.onFailure(iex); } finally { Index: user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java === --- user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java (revision 10205) +++ user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java (working copy) @@ -209,13 +209,13 @@ caught = new StatusCodeException(statusCode, encodedResponse); } else if (encodedResponse == null) { // This can happen if the XHR is interrupted by the server dying -caught = new InvocationException(No response payload); +caught = new InvocationException(No response payload from + methodName); } else if (RemoteServiceProxy.isReturnValue(encodedResponse)) { result = (T) responseReader.read(streamFactory.createStreamReader(encodedResponse)); } else if (RemoteServiceProxy.isThrownException(encodedResponse)) { caught = (Throwable) streamFactory.createStreamReader(encodedResponse).readObject(); } else { -caught = new InvocationException(encodedResponse); +caught = new InvocationException(encodedResponse + from + methodName); } } catch (com.google.gwt.user.client.rpc.SerializationException e) { caught = new IncompatibleRemoteServiceException( -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: RequestFactoryServlet
The blog post would be great. We'll plunder from it freely for our own samples and docs. One note: lately we've been using Objectify rather than JDO for appengine apps. It's pretty pleasant. On Tue, May 24, 2011 at 7:31 AM, Etienne P. etienne...@gmail.com wrote: I've gone through the same effort to get this working using Google Guice based on some pointers provided by Thomas Broyer in another discussion. It works quite well and could be reused easily across applications. I've even done some integration with the JDO API to get this working in GAE (not without a good amount of trial and error). I'll be posting detailed instructions on my blog and committing some code to github that people can reuse easily. Unless there is a better place for it in which case I'd be glad to contribute the code there as well. Suggestions? On May 23, 10:14 pm, Jeff Larsen larse...@gmail.com wrote: Oh and I did test this to make sure it would with Spring. If you're interested here is a link to my test project. https://github.com/larsenje/SpringRequestFactoryExample -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Public (steph...@gmail.com): (issue1450802)
r10214 On Tue, May 24, 2011 at 6:04 AM, jlaba...@google.com wrote: LGTM http://gwt-code-reviews.appspot.com/1450802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Detaching the table section in CellTable before setting the innerHTML to replace the rows. This ... (issue1443805)
Rather than detaching, could you render the new contents in a hidden div and then replace the tbody? IE is a little tricky about doing this kind of thing with table elements, but HTMLPanel.HTMLPanel(String, String) shows the workaround for that (just render inside a div and all is forgiven). On Tue, May 24, 2011 at 12:50 PM, jlaba...@google.com wrote: In Firefox 3.6 and older, detaching a tbody from a table causes Firefox to convert all of the TD elements to divs. Firefox 4.0 doesn't have this problem and works as expected. I updated the patch to skip the detach in Firefox 3.6-. http://gwt-code-reviews.appspot.com/1443805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Detaching the table section in CellTable before setting the innerHTML to replace the rows. This ... (issue1443805)
You're working too hard in IE. You don't need to create the entire table, you can create fragments so long as you're doing it inside a div On Tue, May 24, 2011 at 1:21 PM, jlaba...@google.com wrote: committed as r10218 before I read Ray's comment. On 2011/05/24 20:06:24, rjrjr wrote: Rather than detaching, could you render the new contents in a hidden div and then replace the tbody? I suppose that would work too, but the bug is only in Firefox 3.6 and earlier. I'm not sure it would make much of a difference because we still end up with a detach, and attach, and an innerHTML call. I don't follow your logic here. Couldn't you say the same about the entire optimization? And FF 3.6 is (unfortunately) still a pretty big browser base. IE is a little tricky about doing this kind of thing with table elements, but HTMLPanel.HTMLPanel(String, String) shows the workaround for that (just render inside a div and all is forgiven). That's an understatement. You cannot remove a tbody in IE, nor can you set its innerHTML. Instead, we create a new table in a detached div, as you suggest, then move every row from the new tbody into the existing tbody. Wow, that's horrible. On Tue, May 24, 2011 at 12:50 PM, mailto:jlaba...@google.com wrote: In Firefox 3.6 and older, detaching a tbody from a table causes Firefox to convert all of the TD elements to divs. Firefox 4.0 doesn't have this problem and works as expected. I updated the patch to skip the detach in Firefox 3.6-. http://gwt-code-reviews.appspot.com/1443805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors http://gwt-code-reviews.appspot.com/1443805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] EventBus package(s) in future GWT releases
We're trying to make bindery the correct location, but we're a bit in mid-step. But you shouldn't have to have two instances. gwt...EventBus extends binder...EventBus. On Sun, May 22, 2011 at 10:29 AM, dd cafeb...@googlemail.com wrote: Hey g-men, with GWT 2.3 the autobean, event and requestfactory packages moved from com.google.gwt to com.google.web.bindery. The 'new' RequestFactory.initialize(EventBus) depends on com.google.web.bindery.event.shared.EventBus. In contrast the PlaceController.initialize(EventBus) depends on com.google.gwt.event.shared.EventBus. Using both EventBus packages leads to two @Singleton EventBus instances in my application. Is EventBus intended to exist in two Packages in future GWT releases or will there be one EventBus location? Thanks in advance (and for your *great* work) Daniel -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Better home for ImageResourceRenderer (issue1446807)
Meh, that's a lot of work for not a lot of gain. Why don't we just move it to user.client.ui, right next to AbstractImagePrototype? I don't see why we'd make it a nested class. On Thu, May 19, 2011 at 10:32 PM, Ray Ryan rj...@google.com wrote: I was wondering if we should move the code from abstract image prototype here, make it depend on the renderer. Deferred binding and all. What do you think? On May 19, 2011 9:41 PM, jlaba...@google.com wrote: http://gwt-code-reviews.appspot.com/1446807/diff/1/user/src/com/google/gwt/resources/client/ImageResourceRenderer.java File user/src/com/google/gwt/resources/client/ImageResourceRenderer.java (right): http://gwt-code-reviews.appspot.com/1446807/diff/1/user/src/com/google/gwt/resources/client/ImageResourceRenderer.java#newcode21 user/src/com/google/gwt/resources/client/ImageResourceRenderer.java:21: import com.google.gwt.user.client.ui.AbstractImagePrototype; This creates a dependency from gwt.resources to gwt.user. Maybe we can make this an inner class of AbsractImagePrototype, since that is what is uses for the implementation? http://gwt-code-reviews.appspot.com/1446807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: 1. Assert, at runtime, that GWT is running in Standards Mode (i.e. with an appropriate DOCTYPE d... (issue1422816)
LGTM On Fri, May 20, 2011 at 12:09 PM, jlaba...@google.com wrote: LGTM http://gwt-code-reviews.appspot.com/1422816/diff/8001/user/src/com/google/gwt/user/client/DocumentModeAsserter.java File user/src/com/google/gwt/user/client/DocumentModeAsserter.java (right): http://gwt-code-reviews.appspot.com/1422816/diff/8001/user/src/com/google/gwt/user/client/DocumentModeAsserter.java#newcode69 user/src/com/google/gwt/user/client/DocumentModeAsserter.java:69: + currentMode + ' is not one of: + Arrays.toString(allowedModes); We should give the user better instructions here. Change the doctype at the top of you applications host html page to one of + Arrays.toString(allowedModes) + . Or, add the following line to your gwt.xml file to continue using quirks mode, but understand that GWT no longer supports Quirks mode: extend-property name=\document.compatMode\ values=\BackCompat\/ http://gwt-code-reviews.appspot.com/1422816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Facilitate implementations of EventBus in other packages (issue1443804)
Looks good to me. Do you still need the patch that brings back GwtEvent.setSource, or is this enough by itself? On Thu, May 19, 2011 at 3:58 PM, stephen.haber...@gmail.com wrote: Reviewers: rjrjr, Description: Adds two protected static methods to EventBus that expose otherwise inaccessible methods on Event (setSource and dispatch) to subclasses of EventBus. This is not a huge deal to me, but, in general, it would be nice to move my EventBus implementation out of a c.g package. These two methods are the key to doing so, and the protected static method approach seemed like a reasonable way to expose them to only a small subset of user code (namely subclasses of EventBus). Please review this at http://gwt-code-reviews.appspot.com/1443804/ Affected files: user/src/com/google/web/bindery/event/shared/EventBus.java Index: user/src/com/google/web/bindery/event/shared/EventBus.java === --- user/src/com/google/web/bindery/event/shared/EventBus.java (revision 10188) +++ user/src/com/google/web/bindery/event/shared/EventBus.java (working copy) @@ -30,6 +30,24 @@ */ public abstract class EventBus { + /** Invokes {@code event.dispatch} with {@code handler}. + * + * This allows EventBus implementations in different packages to dispatch + * events even though the {@code event.dispatch} method is protected. + */ + protected static H void dispatchEvent(EventH event, H handler) { +event.dispatch(handler); + } + + /** Sets {@code source} as the source of {@code event}. + * + * This allows EventBus implementations in different packages to set an + * event source even though the {@code event.setSource} method is protected. + */ + protected static void setSourceOnEvent(Event? event, Object source) { +event.setSource(source); + } + /** * Adds an unfiltered handler to receive events of this type from all sources. * p -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Better home for ImageResourceRenderer (issue1446807)
I was wondering if we should move the code from abstract image prototype here, make it depend on the renderer. Deferred binding and all. What do you think? On May 19, 2011 9:41 PM, jlaba...@google.com wrote: http://gwt-code-reviews.appspot.com/1446807/diff/1/user/src/com/google/gwt/resources/client/ImageResourceRenderer.java File user/src/com/google/gwt/resources/client/ImageResourceRenderer.java (right): http://gwt-code-reviews.appspot.com/1446807/diff/1/user/src/com/google/gwt/resources/client/ImageResourceRenderer.java#newcode21 user/src/com/google/gwt/resources/client/ImageResourceRenderer.java:21: import com.google.gwt.user.client.ui.AbstractImagePrototype; This creates a dependency from gwt.resources to gwt.user. Maybe we can make this an inner class of AbsractImagePrototype, since that is what is uses for the implementation? http://gwt-code-reviews.appspot.com/1446807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] GwtEvent.setSource visibility
Yes please, we shouldn't have broken existing code like that. On Tue, May 17, 2011 at 3:25 PM, Stephen Haberman stephen.haber...@gmail.com wrote: Hi, With the bindery package change, I have a custom EventBus implementation that no longer compiles. I was calling GwtEvent.setSource (and squatting in the c.g.g.e.shared package to do so), but now setSource is defined in the bindery package and so inaccessible. I can move my EventBus to the new bindery package and squat there, but it makes it complicated supporting both pre-2.4 and post-2.4 codebases. If possible, adding a GwtEvent.setSource that restored the old visibility would be preferable. I have a simple patch to do so if that's okay. At some point it would be nice to be able to implement an EventBus without squatting in a c.g package, though I understand the appeal of keeping the not for user code methods protected. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: restore GwtEvent.setSource visibility (issue1450801)
Sneaky. You asked for package protected. I don't want to *expand* the public foot print. On Tue, May 17, 2011 at 3:34 PM, stephen.haber...@gmail.com wrote: Reviewers: rjrjr, Description: Redefines Event.setSource in the GwtEvent subclass to restore c.g.g.e.shared package visibility. Please review this at http://gwt-code-reviews.appspot.com/1450801/ Affected files: user/src/com/google/gwt/event/shared/GwtEvent.java Index: user/src/com/google/gwt/event/shared/GwtEvent.java === --- user/src/com/google/gwt/event/shared/GwtEvent.java (revision 10188) +++ user/src/com/google/gwt/event/shared/GwtEvent.java (working copy) @@ -101,6 +101,10 @@ setSource(null); } + protected void setSource(Object source) { +super.setSource(source); + } + void overrideSource(Object source) { super.setSource(source); } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] rfc on 6068 fix
Bob V is the authority here, but he's on vacation this week. On Sun, May 8, 2011 at 7:08 AM, Patrick Julien pjul...@gmail.com wrote: Here is what I am planning to do to fix issue 6068 http://code.google.com/p/google-web-toolkit/issues/detail?id=6068 1. Add to BeanMethod.java (package com.google.web.bindery.autobean.vm.impl;) an TO_ARRAY value 2. Add to JBeanMethod.java (com.google.web.bindery.autobean.gwt.rebind.model;) an TO_ARRAY value 3. in AutoBeanFactoryGenerator#writeShim (package com.google.web.bindery.autobean.gwt.rebind;) handle the TO_ARRAY case by checking the: a. if the type is assignable to Collection? b. Determine if toArray has one of two supported method signatures in Collection? c. Generate code that assigns each element of the newly created array by wrapping it first 4. Add a unit test to check for unfrozen beans. a. Get a collection of AutoBeans b. assert they are frozen c. Initialize an ArrayList using this other collection d. Assert the objects stored the second ArrayList are unfrozen Open Questions: 1. Should I try to handle immutable collections, i.e., Collections.unmodifiable* or guava's Immutable*? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Better error reporting (issue1444801)
Verbal LGTM from John. r10167 On Mon, May 9, 2011 at 1:40 PM, rj...@google.com wrote: http://gwt-code-reviews.appspot.com/1444801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] AssertionError break Scheduler
Filing a bug would be helpful, thanks. If you want to offer a fix the details are at http://code.google.com/webtoolkit/makinggwtbetter.html On May 6, 2011 12:50 AM, Laurent Le Goff legoff.laur...@gmail.com wrote: -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Eclipse configuration for MobileWebApp. (issue1437801)
On Fri, May 6, 2011 at 7:52 AM, rchan...@google.com wrote: On 2011/05/05 22:15:56, rjrjr wrote: Rietveld is choking on this. Yes, something is broken in my git5 client that plays badly with reitveld. The non-eclipse instructions in the README.txt are wrong, aren't they? ant war can't work because of the missing appengine jars. Shouldn't we be saying that the sample is configured to assume you're using GPE? Has been working reliably for me. I actually use it to detect when things fail because Eclipse setup has broken. Can you try setting up a local.properties file pointing to App Engine SDK? # Starting from trunk cd samples/mobilewebapp vim local.properties #Define gwt.sdk and appengine.sdk ant -f user-build.xml war ant -f user-build.xml devmode This will work but for server-side validation because war/WEB-INF/lib does not contain hibernate-validation, slf4j-api, slf4j-log4j and log4j. But the distributed app does get the necessary jars in war/WEB-INF/lib and produce a proper war. Cool. How? http://gwt-code-reviews.appspot.com/1437801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Eclipse configuration for MobileWebApp. (issue1437801)
LGTM, by the way. On Fri, May 6, 2011 at 9:30 AM, Rodrigo Chandia rchan...@google.com wrote: On Fri, May 6, 2011 at 12:09 PM, Ray Ryan rj...@google.com wrote: But the distributed app does get the necessary jars in war/WEB-INF/lib and produce a proper war. Cool. How? 'samples/build.xml' calls 'samples/mobilewebapp/ant.xml' with target 'source+libs'. That target copies all necessary files and libs from tools. Several files get renamed: user-build.xml - build.xml user-classpath - .classpath user-project - .project user-settings/ - .settings/ For example: # Starting from trunk ant dist-dev # to get an updated GWT in build/staging cd samples/mobilewebapp ant # copies and renames files cd build/out/samples/MobileWebApp ls war/WEB-INF/lib # contains jars from tools # Either use ant vim local.properties # define gwt.sdk and appengine.sdk ant war ant devmode # no 'ant deploy' because I was lazy # OR use eclipse 1. Import Existing projects... 2. Fix the appengine and GWT JAR warnings 3. Run/Debug as Web Application (clean you launch configs, just in case) 4. GWT compile 5. Google - AppEngine deploy In the distributed gwt-x.y.z.zip gwt.sdk already points to '../../' but local.properties still needs to define 'appengine.sdk' -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improving SimpleAppCacheLinker noise. (issue1438802)
LGTM On Fri, May 6, 2011 at 12:13 PM, rchan...@google.com wrote: OK, I promise I'll try to fix my rietveld reviews. Please use the Unified diff view for the time being. On 2011/05/06 19:01:27, rchandia wrote: http://gwt-code-reviews.appspot.com/1438802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Eclipse configuration for MobileWebApp. (issue1437801)
Rietveld is choking on this. The non-eclipse instructions in the README.txt are wrong, aren't they? ant war can't work because of the missing appengine jars. Shouldn't we be saying that the sample is configured to assume you're using GPE? On Thu, May 5, 2011 at 2:59 PM, rchan...@google.com wrote: Reviewers: cramsdale, rjrjr, Description: Eclipse configuration for MobileWebApp. Relies on AppEngine and GPE README.txt instructions updated to help setting up Eclipse in this case Please review this at http://gwt-code-reviews.appspot.com/1437801/ Affected files: A eclipse/samples/MobileWebApp/.checkstyle A eclipse/samples/MobileWebApp/.classpath A eclipse/samples/MobileWebApp/.project A eclipse/samples/MobileWebApp/.settings/com.google.appengine.eclipse.core.prefs A eclipse/samples/MobileWebApp/.settings/com.google.gdt.eclipse.core.prefs M samples/build.xml M samples/common.ant.xml M samples/expenses/build.xml M samples/mobilewebapp/README.txt M samples/mobilewebapp/build.xml M samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/desktop/DesktopTaskListView.java M samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/mobile/MobileTaskListView.java M samples/mobilewebapp/user-build.xml A samples/mobilewebapp/user-classpath A samples/mobilewebapp/user-project A samples/mobilewebapp/user-settings/com.google.appengine.eclipse.core.prefs M samples/mobilewebapp/war/WEB-INF/appengine-web.xml A samples/mobilewebapp/war/WEB-INF/classes/marker -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)
Rodrigo, it looks like John is on vacation for the rest of the week. Can you finish this review? On Thu, May 5, 2011 at 2:24 PM, rj...@google.com wrote: Ready for another look. OrientationMonitor is replaced with OrientationHelper. John, I think I've stumbled onto a pretty nice widget plugin pattern there. I was able to delete the superclass for the shells without adding much boilerplate or copy/paste. What do you think? Notice that I've snuck a method onto the HasAttachHandlers interface. I think we can get away with that since it's meant to be implemented by Widget — I'm willing to break a few tests to make it more useful. Hmm. I suppose I could instead introduce an interface into the demo code, IsAttachable extends HasAttachHandlers. The change sure feels right, though. http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java File samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java (right): http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java#newcode50 samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java:50: * @return the local {@link Storage} object, or null unsupporting browsers On 2011/05/04 20:29:44, jlabanca wrote: /r/null unsupporting/null in unsupporting Done. http://gwt-code-reviews.appspot.com/1433801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Force locale to en_US for user unit tests (issue1430801)
This is probably html unit honoring the locale and changing its behavior from what the tests expect. On May 4, 2011 2:02 AM, t.bro...@gmail.com wrote: On 2011/05/03 17:51:18, rjrjr wrote: Running ant clean dist-dev test, this appears to break the i18n suite under html unit. Oops! Only tested with a -Dgwt.junit.testcase.includes (which obviously didn't include I18N tests) That seems weird though that simply running the JVM in a different locale suddenly breaks tests of things that are emulated. Testcase: testMessageDateTime took 0.15 sec FAILED Remote test failed at 172.31.131.172 / Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 expected=It is Feb 15, 2010 actual=It is 2010 Feb 15 junit.framework.AssertionFailedError: Remote test failed at 172.31.131.172 / Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 expected=It is Feb 15, 2010 actual=It is 2010 Feb 15 Actually, 3 tests always fail for me, whichever locale I'm running them in (including using es_MX, or removing the -Duser.language/user.region system props altogether!) And they always fail with the exact same error; so it looks more like a regression in the I18N code (particularly as all other I18N tests pass). test.dev.htmlunit: [echo] Writing test results to C:\Documents and Settings\tbr\Mes documents\Mes Projets\gwt\trunk\build\out\user\test/dev-htmlunit/reports for test.dev.htmlunit.tests [junit] WARNING: multiple versions of ant detected in path for junit [junit] jar:file:/C:/eclipse/plugins/org.apache.ant_1.7.1.v20100518-1145/lib/ant.jar!/org/apache/tools/ant/Project.class [junit] and jar:file:/C:/Documents%20and%20Settings/tbr/Mes%20documents/Mes%20Projets/gwt/trunk/build/lib/gwt-dev.jar!/org/apache/tools/ant/Project.class [junit] Running com.google.gwt.i18n.I18NSuite [junit] expected=It is Feb 15, 2010 actual=It is 2010 Feb 15) [junit] expected=Es ist 15. Feb 2010 actual=It is 2010 Feb 15) [junit] expected=Short: 2010-02-01 actual=Short: 2010-02-02) http://gwt-code-reviews.appspot.com/1430801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: 1. Assert, at runtime, that GWT is running in Standards Mode (i.e. with an appropriate DOCTYPE d... (issue1422816)
Please tell me that this is the last step in the following chain. It seems unlikely if you're only providing the property now. First you provide the property to allow quirks, and give compiler warnings that only go away if you set standards mode or set the quirks property. (And publicize that it will be the default in the next release.) Then you change the default and break existing apps. This change should also include updates to the javadoc of widgets that require quirks to point to the new property. On May 4, 2011 7:38 AM, jlaba...@google.com wrote: This is a breaking change for existing apps that run in quirks mode. I'm alright with that because I don't see an alternative, but we'll need to call if out in the release notes and tell people the workaround. http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/UserAgent.gwt.xml File user/src/com/google/gwt/user/UserAgent.gwt.xml (right): http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/UserAgent.gwt.xml#newcode29 user/src/com/google/gwt/user/UserAgent.gwt.xml:29: extra newline http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/UserAgent.gwt.xml#newcode56 user/src/com/google/gwt/user/UserAgent.gwt.xml:56: define-configuration-property name=document.compatMode I think we should break this out into a separate DocumentMode.gwt.xml file and inherit it in User.gwt.xml. It isn't part of the UserAgent. http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/UserAgent.gwt.xml#newcode67 user/src/com/google/gwt/user/UserAgent.gwt.xml:67: extra newline http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/client/DocumentModeAsserter.java File user/src/com/google/gwt/user/client/DocumentModeAsserter.java (right): http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/client/DocumentModeAsserter.java#newcode27 user/src/com/google/gwt/user/client/DocumentModeAsserter.java:27: * rendering mode is of of the values allowed by the /r/of of/one of http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java File user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java (right): http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java#newcode49 user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java:49: logger.log(TreeLogger.ERROR, OOPS, e); Maybe something better than OOPS http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java#newcode57 user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java:57: JClassType remoteService = typeOracle.findType(typeName); Isn't removeService the same as userType? http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java#newcode76 user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java:76: logger.log(TreeLogger.WARN, Unable to find value for ' If we are going to throw an exception, this should be an ERROR instead of a WARN http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java#newcode102 user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java:102: for (IteratorString iterator = documentModes.iterator(); iterator.hasNext();) { You can shorten this to: for (String next : documentModes) { http://gwt-code-reviews.appspot.com/1422816/diff/1/user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java#newcode103 user/src/com/google/gwt/user/rebind/DocumentModeGenerator.java:103: sw.println(\ + iterator.next() + \, ); This array will always end with a comma. Does Java handle that correctly? return new String[]{a,b,}; http://gwt-code-reviews.appspot.com/1422816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Including the TaskProxy (when available) in TaskEditPlace so we do not do an extra round trip to... (issue1428810)
I mean that you should have been able to fix it by replace place1 == place2 with place1.equals(place2). On Wed, May 4, 2011 at 8:39 AM, jlaba...@google.com wrote: @rjrjr - What do you mean? http://gwt-code-reviews.appspot.com/1428810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Including the TaskProxy (when available) in TaskEditPlace so we do not do an extra round trip to... (issue1428810)
Also, this change is a stark reminder that we need to get real about request caching in RequestFactory. You fundamentally shouldn't be doing this, you should be able to make the redundant request counting on a lower layer to send it only if necessary. Bob, I've lost track -- are there hooks in place yet that we could implement app specific client side caching in a sample like this? On Wed, May 4, 2011 at 8:56 AM, Ray Ryan rj...@google.com wrote: I mean that you should have been able to fix it by replace place1 == place2 with place1.equals(place2). On Wed, May 4, 2011 at 8:39 AM, jlaba...@google.com wrote: @rjrjr - What do you mean? http://gwt-code-reviews.appspot.com/1428810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Including the TaskProxy (when available) in TaskEditPlace so we do not do an extra round trip to... (issue1428810)
On Wed, May 4, 2011 at 9:47 AM, Bob Vawter robertvaw...@google.com wrote: Bob, I've lost track -- are there hooks in place yet that we could implement app specific client side caching in a sample like this? You can call RequestFactory.getSerializer() with an implementation of a ProxyStore (e.g. DefaultProxyStore) in order to indefinitely persist a proxy. Beyond simple serialization, there's no mechanism in place to short-circuit requests. That's proxies. Can requests be serialized? For that matter, any reason not to just hold on to them? I'm not talking about LocalStorage or anything here, just optimizing within a single session. I know we can extend or replace DefaultRequestTransport on the client side, e.g. as done in Expenses for GAE integrationhttps://cs.corp.google.com/#google3/third_party/java_src/gwt/svn/trunk/samples/expenses/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java. If requests have reasonable equality semantics and are somewhat introspectable, even just at the instanceof level, we might be able to get some simple caching done in this sample. @jlabanca, I know your time is short and you gotta do what you gotta do. I don't mean to hold up this change for caching. I just want to have the conversation while we have a use case staring us in the face. -- Bob Vawter Google Web Toolkit Team -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
I'd be inclined to start with a) and see what happens. On Wed, May 4, 2011 at 10:02 AM, rdcas...@google.com wrote: http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java (right): http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode205 user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:205: if (isFullyInitialized()) { On 2011/05/04 16:47:17, rjrjr wrote: This seems pretty unexpected. I would have thought it would either be an error (RuntimeException), or else that I would do the opposite of this: replace my existing element with the new one. I'd be totally fine with a RuntimeError, it'll probably be m ore manageable in the long-term. I was trying to support this because even though it's less efficient, we can still make it work. The scenario here is: 1-) Build Attachable tree. This includes calling all the render() stuff and so on. Let's assume that this particular AttachableHTMLPanel is in the middle of the tree. 2-) For some reason do something to this particular panel (that's in the middle of the tree) that calls its getElement() [let's say attach it to the document]. This triggers the process of building the widget tree as if this panel was the root (i.e., hidden div, set innerHTML, getting elements for all children and initializing them). At this point everything works 3-) Now you go and attach the real root of the tree, an ancestor of this panel. We can do one of 3 things: (a) throw an error. You probably didn't mean to do this as it's less efficient (b) replace the element that our parent assigned us (along with its subtree) with the subtree we already built in step 2 (c) re-do everything in step 2, ignoring the fact that we already initialized all children widget I think (a) would be ideal, specially for debugging purposes. Honestly I implemented (b) because I was a little afraid and wanted to be as backwards-compatible as possible. I don't think (c) would work out-of-the-box, as we'd have to support initializing Widgets twice (i.e., re-setting the element, etc.) Makes sense? http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Including the TaskProxy (when available) in TaskEditPlace so we do not do an extra round trip to... (issue1428810)
Something as simple as short circuiting all equivalent RequestFactory.find(EntityProxyIdP) requests, where equivalent takes into account the proxy version as well as its id, could be very effective. On Wed, May 4, 2011 at 10:04 AM, John LaBanca jlaba...@google.com wrote: On Wed, May 4, 2011 at 12:58 PM, Ray Ryan rj...@google.com wrote: On Wed, May 4, 2011 at 9:47 AM, Bob Vawter robertvaw...@google.comwrote: Bob, I've lost track -- are there hooks in place yet that we could implement app specific client side caching in a sample like this? You can call RequestFactory.getSerializer() with an implementation of a ProxyStore (e.g. DefaultProxyStore) in order to indefinitely persist a proxy. Beyond simple serialization, there's no mechanism in place to short-circuit requests. That's proxies. Can requests be serialized? For that matter, any reason not to just hold on to them? I'm not talking about LocalStorage or anything here, just optimizing within a single session. I know we can extend or replace DefaultRequestTransport on the client side, e.g. as done in Expenses for GAE integrationhttps://cs.corp.google.com/#google3/third_party/java_src/gwt/svn/trunk/samples/expenses/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java. If requests have reasonable equality semantics and are somewhat introspectable, even just at the instanceof level, we might be able to get some simple caching done in this sample. @jlabanca, I know your time is short and you gotta do what you gotta do. I don't mean to hold up this change for caching. I just want to have the conversation while we have a use case staring us in the face. This change was already submitted, but a more comprehensive solution would be nice. -- Bob Vawter Google Web Toolkit Team -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
Once we've validated the work, seems like a lot of the Attachable support should be baked into UiObject, Widget and Panel in some kind of opt-in manner. On Wed, May 4, 2011 at 10:20 AM, Rafael Castro rdcas...@google.com wrote: Liked it. With the stuff I added to our subclass of AttachableHTMLPanel, this already works pretty well. I have to review some other tricky cases (like if you add a non-attachable widget to an Attachable panel before finishing the initialization), but we're pretty close. The other cases that could trigger this is calling some UIObject method that we haven't yet @Override (like we did for setStyleName). Those call getElement() and hurt us. On Wed, May 4, 2011 at 2:15 PM, rj...@google.com wrote: How's that? Is the bit I wrote about after adding it to a panel accurate? Seems like we're trying to get to a world where the add would be fine, and the wrap call wouldn't happen until the parent is wrapped — are we there already? http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java (right): http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode211 user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:211: throw new IllegalStateException( wrapElement() cannot be called twice, or after a call to getElement() has forced the widget to render itself (e.g. after adding it to a panel) http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds user authentication to MobileWebApp. Tasks are now specific to each user. Authentication ... (issue1432801)
I guess I'm speaking strictly of the null checks. It's fine for our sample code not to have a real auth system in its storage, of course. Seems like your UserServiceWrapper should have a requireCurrentUserId() method that throws an exception if there is no id, and Task shouldn't be friendly. Is that reasonable? On Wed, May 4, 2011 at 11:19 AM, rj...@google.com wrote: http://gwt-code-reviews.appspot.com/1432801/diff/1/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/server/domain/Task.java File samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/server/domain/Task.java (right): http://gwt-code-reviews.appspot.com/1432801/diff/1/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/server/domain/Task.java#newcode48 samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/server/domain/Task.java:48: } This seems bad. I'd never allow code like this in a production app. An unauthorized user shouldn't even be able to reach this class. http://gwt-code-reviews.appspot.com/1432801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Force locale to en_US for user unit tests (issue1430801)
LGTM Thanks, will submit. On Tue, May 3, 2011 at 2:40 AM, t.bro...@gmail.com wrote: Reviewers: rjrjr, bobv, Description: Force locale to en_US for user unit tests Force locale to en_US for user unit tests, otherwise validation tests fail (use hard-coded checks on locale-dependent messages) Please review this at http://gwt-code-reviews.appspot.com/1430801/ Affected files: M user/build.xml Index: user/build.xml diff --git a/user/build.xml b/user/build.xml index 4f6286fd2e654d9b63ad55bc393807b23954c084..e7696fff725e9e66ee2072d7975173bbc420f178 100755 --- a/user/build.xml +++ b/user/build.xml @@ -2,7 +2,8 @@ property name=gwt.root location=.. / property name=project.tail value=user / property name=test.args value=-ea / - property name=test.jvmargs value=-ea / + !-- Bean validation and RequestFactory tests use hard-coded checks on locale-dependent messages -- + property name=test.jvmargs value=-ea -Duser.language=en -Duser.region=US / !-- support old variables names -- condition property=gwt.hosts.web.remote value=${gwt.remote.browsers} @@ -763,7 +764,7 @@ excludes=${gwt.tck.testcase.dev.excludes} / gwt.junit test.name=test.dev.htmlunit test.args=${test.args} -standardsMode - test.jvmargs=-ea -Dcom.google.gwt.sample.validationtck.util.Failing.include=true -Dcom.google.gwt.sample.validationtck.util.NonTckTest.exclude=true + test.jvmargs=${test.jvmargs} -Dcom.google.gwt.sample.validationtck.util.Failing.include=true -Dcom.google.gwt.sample.validationtck.util.NonTckTest.exclude=true test.out=${junit.out}/tck-dev-htmlunit test.cases=tck.dev.htmlunit.tests haltonfailure=false -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Promoting LazyDomElement to be used externally. LazyDomElement can be (issue1427809)
On Tue, May 3, 2011 at 9:49 AM, her...@google.com wrote: http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java File user/src/com/google/gwt/uibinder/client/LazyDomElement.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode33 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:33: * lt;div ui:field=myDiv class={anyClass}/gt; On 2011/05/02 16:51:54, rjrjr wrote: The class bit is noise, can you snip it? Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode45 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:45: */ On 2011/05/02 16:51:54, rjrjr wrote: /** * Creates an instance to fetch the element with the given id. */ Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode54 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:54: On 2011/05/02 16:51:54, rjrjr wrote: /** * Returns the dom element * @return the dom element * @throws RuntimeException is the element cannot be found */ Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode59 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:59: throw new RuntimeException(Element is not attached.); On 2011/05/02 16:51:54, rjrjr wrote: Cannot find element with id \ + domId + \. Perhaps it is not attached to the document body? Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java File user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java#newcode46 user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java:46: if (!templateFieldType.isAssignableTo(parameterType)) { On 2011/05/02 16:51:54, rjrjr wrote: Needs a unit test. See FieldWriterOfGeneratedCssResourceTest for example. Done. http://gwt-code-reviews.appspot.com/1427809/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/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode380 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:380: // But if the owner field is an instance of LazyDomElement then the code On 2011/05/02 16:51:54, rjrjr wrote: /* for long comments Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode463 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:463: // We can switch types if useLazyWidgetBuilders is enabled and the On 2011/05/02 16:51:54, rjrjr wrote: /* Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode710 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:710: return lazyDomElementClass.isAssignableFrom(ownerField.getType().getRawType()); On 2011/05/02 16:51:54, rjrjr wrote: Can getRawType can return null? Is isAssignableFrom null safe? Nop, OwnerField dies if it can't resolve the type. http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java File user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java#newcode85 user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java:85: public JClassType getRawType() { On 2011/05/03 16:37:02, rjrjr wrote: Please inline this. You meant remove this method? This is very handy for tests. Oh, I see. Could you put a comment in the method body to that effect? http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java File user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java#newcode22 user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java:22: public class FieldWriterOfExistingTypeTest extends TestCase { On 2011/05/03 16:37:02, rjrjr wrote: I forgot how handy EasyMock is True. http://gwt-code-reviews.appspot.com/1427809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Promoting LazyDomElement to be used externally. LazyDomElement can be (issue1427809)
LGTM On Tue, May 3, 2011 at 9:54 AM, Ray Ryan rj...@google.com wrote: On Tue, May 3, 2011 at 9:49 AM, her...@google.com wrote: http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java File user/src/com/google/gwt/uibinder/client/LazyDomElement.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode33 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:33: * lt;div ui:field=myDiv class={anyClass}/gt; On 2011/05/02 16:51:54, rjrjr wrote: The class bit is noise, can you snip it? Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode45 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:45: */ On 2011/05/02 16:51:54, rjrjr wrote: /** * Creates an instance to fetch the element with the given id. */ Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode54 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:54: On 2011/05/02 16:51:54, rjrjr wrote: /** * Returns the dom element * @return the dom element * @throws RuntimeException is the element cannot be found */ Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode59 user/src/com/google/gwt/uibinder/client/LazyDomElement.java:59: throw new RuntimeException(Element is not attached.); On 2011/05/02 16:51:54, rjrjr wrote: Cannot find element with id \ + domId + \. Perhaps it is not attached to the document body? Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java File user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java#newcode46 user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java:46: if (!templateFieldType.isAssignableTo(parameterType)) { On 2011/05/02 16:51:54, rjrjr wrote: Needs a unit test. See FieldWriterOfGeneratedCssResourceTest for example. Done. http://gwt-code-reviews.appspot.com/1427809/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/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode380 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:380: // But if the owner field is an instance of LazyDomElement then the code On 2011/05/02 16:51:54, rjrjr wrote: /* for long comments Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode463 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:463: // We can switch types if useLazyWidgetBuilders is enabled and the On 2011/05/02 16:51:54, rjrjr wrote: /* Done. http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode710 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:710: return lazyDomElementClass.isAssignableFrom(ownerField.getType().getRawType()); On 2011/05/02 16:51:54, rjrjr wrote: Can getRawType can return null? Is isAssignableFrom null safe? Nop, OwnerField dies if it can't resolve the type. http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java File user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java#newcode85 user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java:85: public JClassType getRawType() { On 2011/05/03 16:37:02, rjrjr wrote: Please inline this. You meant remove this method? This is very handy for tests. Oh, I see. Could you put a comment in the method body to that effect? http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java File user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java (right): http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java#newcode22 user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java:22: public class FieldWriterOfExistingTypeTest extends TestCase { On 2011/05/03 16:37:02, rjrjr wrote: I forgot how handy EasyMock is True. http://gwt-code-reviews.appspot.com/1427809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Force locale to en_US for user unit tests (issue1430801)
Running ant clean dist-dev test, this appears to break the i18n suite under html unit. Testcase: testMessageDateTime took 0.15 sec FAILED Remote test failed at 172.31.131.172 / Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 expected=It is Feb 15, 2010 actual=It is 2010 Feb 15 junit.framework.AssertionFailedError: Remote test failed at 172.31.131.172 / Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 expected=It is Feb 15, 2010 actual=It is 2010 Feb 15 On Tue, May 3, 2011 at 9:42 AM, Ray Ryan rj...@google.com wrote: LGTM Thanks, will submit. On Tue, May 3, 2011 at 2:40 AM, t.bro...@gmail.com wrote: Reviewers: rjrjr, bobv, Description: Force locale to en_US for user unit tests Force locale to en_US for user unit tests, otherwise validation tests fail (use hard-coded checks on locale-dependent messages) Please review this at http://gwt-code-reviews.appspot.com/1430801/ Affected files: M user/build.xml Index: user/build.xml diff --git a/user/build.xml b/user/build.xml index 4f6286fd2e654d9b63ad55bc393807b23954c084..e7696fff725e9e66ee2072d7975173bbc420f178 100755 --- a/user/build.xml +++ b/user/build.xml @@ -2,7 +2,8 @@ property name=gwt.root location=.. / property name=project.tail value=user / property name=test.args value=-ea / - property name=test.jvmargs value=-ea / + !-- Bean validation and RequestFactory tests use hard-coded checks on locale-dependent messages -- + property name=test.jvmargs value=-ea -Duser.language=en -Duser.region=US / !-- support old variables names -- condition property=gwt.hosts.web.remote value=${gwt.remote.browsers} @@ -763,7 +764,7 @@ excludes=${gwt.tck.testcase.dev.excludes} / gwt.junit test.name=test.dev.htmlunit test.args=${test.args} -standardsMode - test.jvmargs=-ea -Dcom.google.gwt.sample.validationtck.util.Failing.include=true -Dcom.google.gwt.sample.validationtck.util.NonTckTest.exclude=true + test.jvmargs=${test.jvmargs} -Dcom.google.gwt.sample.validationtck.util.Failing.include=true -Dcom.google.gwt.sample.validationtck.util.NonTckTest.exclude=true test.out=${junit.out}/tck-dev-htmlunit test.cases=tck.dev.htmlunit.tests haltonfailure=false -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix Attachable for those poor fellows who don't have the bliss of SafeHtml enabled (yet). (issue1426808)
LGTM On Fri, Apr 29, 2011 at 2:06 PM, rdcas...@google.com wrote: Reviewers: rjrjr, Description: Fix Attachable for those poor fellows who don't have the bliss of SafeHtml enabled (yet). Please review this at http://gwt-code-reviews.appspot.com/1426808/ Affected files: M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java Index: user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java === --- user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (revision 10113) +++ user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (working copy) @@ -792,7 +792,7 @@ */ public String tokenForSafeHtmlExpression(String expression) { if (!useSafeHtmlTemplates) { - return tokenForStringExpression(expression); + return tokenForStringExpression(expression + .asString()); } String token = tokenator.nextToken(expression); -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. (issue1427810)
This still appears to have all the problems of http://gwt-code-reviews.appspot.com/1426803. On Mon, May 2, 2011 at 11:33 AM, rchan...@google.com wrote: Reviewers: rjrjr, Description: SafeHtmlRenderer code gen for UiBinder. Picking-up patch from rietveld issue 1426803 Please review this at http://gwt-code-reviews.appspot.com/1427810/ Affected files: M user/src/com/google/gwt/uibinder/UiBinder.gwt.xml M user/src/com/google/gwt/uibinder/elementparsers/CustomButtonParser.java M user/src/com/google/gwt/uibinder/elementparsers/DialogBoxParser.java M user/src/com/google/gwt/uibinder/elementparsers/DomElementParser.java M user/src/com/google/gwt/uibinder/elementparsers/GridParser.java M user/src/com/google/gwt/uibinder/elementparsers/HTMLPanelParser.java M user/src/com/google/gwt/uibinder/elementparsers/HasHTMLParser.java M user/src/com/google/gwt/uibinder/elementparsers/StackLayoutPanelParser.java M user/src/com/google/gwt/uibinder/elementparsers/TabLayoutPanelParser.java M user/src/com/google/gwt/uibinder/elementparsers/TabPanelParser.java M user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java M user/src/com/google/gwt/uibinder/rebind/FieldManager.java M user/src/com/google/gwt/uibinder/rebind/FieldWriter.java M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors