Re: [gwt-contrib] GWT SDK 2.3.0.RC1
Thanks guys! On Wed, Apr 27, 2011 at 9:36 PM, Daniel Bell daniel.r.b...@gmail.comwrote: We just upgraded 3 apps too, with one gotcha: it turns out that you need to do a find/replace on com.google.gwt.requestfactory.client. - com.google.web.bindery.requestfactory.gwt.client. before you do com.google.gwt.requestfactory. - com.google.web.bindery.requestfactory.. After that, it seems to work well. On 28 April 2011 11:24, Patrick Julien pjul...@gmail.com wrote: Since you asked so nicely, I can confirm that changing imports and the gwt.xml file was all I needed to do to fix 2 large gwt applications. On Tue, Apr 26, 2011 at 8:48 PM, Chris Ramsdale cramsd...@google.com wrote: Hey GWTC folks, We have a GWT SDK 2.3.0.RC1 build that we would love feedback on. A big change since M1 is the move of AutoBean and RequestFactory to a new package, com.google.web.bindery. The old locations of AutoBean and RequestFactory should still work, but are deprecated. Fixing the deprecation warnings for the most part should be as simple as changing some import lines. If early adopters could verify that assumption, we would be grateful. The RC1 download can be found here: http://code.google.com/p/google-web-toolkit/downloads/detail?name=gwt-2.3.0.rc1.zip -- Chris/Ray, on behalf of the GWT team -- 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: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
LGTM++ On Thu, Apr 28, 2011 at 10:27 AM, unn...@google.com wrote: http://gwt-code-reviews.appspot.com/1425811/diff/5001/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java File user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/5001/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java#newcode52 user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java:52: assertEquals(Hello bBob/b, domUi.div.getInnerHTML()); On 2011/04/27 23:21:30, rjrjr wrote: You might want call .toLowerCase() on both strings to normalize. IE does funny things to tag names, you'll get dinged in the cross browser tests. Done. http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re : Check for IsWidget rather than Widget when determining if a class is a Widget in UiBinder (issue1421809)
It is already the case that you can use IsWidget interfaces as elements in a ui.xml file. On Thu, Apr 28, 2011 at 12:33 PM, Stephen Haberman stephen.haber...@gmail.com wrote: It looks like that would fix Huh. Yeah, that is interesting. Is the widgets must extend Widget restriction being loosened? Per WhyWidgetIsAClass, I thought that was pretty set in stone. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the lazy (issue1420814)
Okay, done, now with passing JRE tests. On Thu, Apr 28, 2011 at 2:35 PM, rj...@google.com wrote: http://gwt-code-reviews.appspot.com/1420814/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder (issue1426803)
Turns out the useLazyWidget stuff isn't passing all of the UiBinder tests yet. Ignoring that path for now seems reasonable. Sorry for the flip flop. On Mon, Apr 25, 2011 at 3:19 PM, rj...@google.com wrote: Oh, the base class exists already: com.google.gwt.text.shared.AbstractSafeHtmlRendererT http://gwt-code-reviews.appspot.com/1426803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: LazyPanel parser should be enabled only if useLazyWidgetBuilders is (issue1423806)
You sure? I kind of liked how you changed this to always run, and explain to
the user what flag to set to make it go.
On Fri, Apr 22, 2011 at 5:43 PM, her...@google.com wrote:
Reviewers: rjrjr, jat,
Description:
LazyPanel parser should be enabled only if useLazyWidgetBuilders is
enabled.
Please review this at http://gwt-code-reviews.appspot.com/1423806/
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
10054)
+++ user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (working
copy)
@@ -1144,7 +1144,9 @@
addWidgetParser(HasAlignment);
addWidgetParser(DateLabel);
addWidgetParser(NumberLabel);
-addWidgetParser(LazyPanel);
+if (useLazyWidgetBuilders) {
+ addWidgetParser(LazyPanel);
+}
}
/**
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: LazyPanel parser should be enabled only if useLazyWidgetBuilders is (issue1423806)
LGTM
On Mon, Apr 25, 2011 at 11:51 AM, Hermes Freitas her...@google.com wrote:
The problem is that the combination LazyPanel + provided = true can be
used to put LazyPanel widgets in templates. There were 2 possible fixes:
1. check whether the LazyPanel has inner widgets or not and then show the
proper message.
2. let things as is and register the new LazyPanel parser only if the flag
is enabled
Due to the urgency of things I went with #2.
On Mon, Apr 25, 2011 at 3:42 PM, Ray Ryan rj...@google.com wrote:
You sure? I kind of liked how you changed this to always run, and explain
to the user what flag to set to make it go.
On Fri, Apr 22, 2011 at 5:43 PM, her...@google.com wrote:
Reviewers: rjrjr, jat,
Description:
LazyPanel parser should be enabled only if useLazyWidgetBuilders is
enabled.
Please review this at http://gwt-code-reviews.appspot.com/1423806/
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
10054)
+++ user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (working
copy)
@@ -1144,7 +1144,9 @@
addWidgetParser(HasAlignment);
addWidgetParser(DateLabel);
addWidgetParser(NumberLabel);
-addWidgetParser(LazyPanel);
+if (useLazyWidgetBuilders) {
+ addWidgetParser(LazyPanel);
+}
}
/**
--
--Hermes Freitas
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: MobileWebApp sample. Showcases GWT providing a single app providing specialized views for Deskto... (issue1427803)
I'm getting up to my elbows in there too, trying to narrow the exposure of ClientFactory. John, are there particular spots I should avoid? On Fri, Apr 22, 2011 at 5:27 AM, Rodrigo Chandia rchan...@google.comwrote: No problem. On Fri, Apr 22, 2011 at 8:20 AM, jlaba...@google.com wrote: Or CloudTasks. @rchandia - please hold off on any big refactor until I get my changes in. I'll try to send them for review today. http://gwt-code-reviews.appspot.com/1427803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. (issue1422809)
Nick, could you take a look at this too? In particular see the bottom of http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java On Thu, Apr 21, 2011 at 6:57 AM, b...@google.com wrote: Reviewers: rjrjr, Message: Request requested. http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java File user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java (right): http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java#newcode526 user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java:526: // Construct an ID that represents domainObject Interesting change starts here. Description: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. Deprecate Violation and Receiver.onViolation(). Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.appspot.com/1422809/ Affected files: M samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java M user/src/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/gwt/client/testing/MockRequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/server/RequestFactoryJarExtractor.java M user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java M user/src/com/google/web/bindery/requestfactory/shared/Receiver.java M user/src/com/google/web/bindery/requestfactory/shared/Violation.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequest.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java M user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java M user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/ui/EditorTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
On Thu, Apr 21, 2011 at 6:54 PM, j...@google.com wrote:
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode60
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:60:
JType... argTypes) {
On 2011/04/22 01:13:17, rjrjr wrote:
I don't know that I would ever use this unless it was based on
JClassType.getOverridableMethods().
But rather than hardcoding that choice, perhaps you should make the
first
argument a JMethod[] to let me choose the set myself? Can put @see
pointing to
getOverridableMethods.
Ok, how about a method taking the method list and one taking the type,
which defaults to either getMethods or getOverridableMethods (whichever
you think is more useful). I would think in this application it is most
interesting in this context, since you want to know if you can call the
method with a set of parameters, and whether it is implemented on that
class or a superclass is just an implementation detail.
Sounds good. Definitely getOverridableMethods.
I'm biting my tongue about the fact that this method is unused. I'm
generally
opposed to speculative coding, but I can imagine cleaning up some
existing code
with this.
It just seemed an obvious extension of the constructor code, but it is
certainly easy enough to leave it out if you prefer.
Nah, I wants it.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode65
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:65: if
(method.isStatic() == isStatic methodName.equals(method.getName())
On 2011/04/22 01:13:17, rjrjr wrote:
could you put () around the == statement?
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode78
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:78: *
@param argType
On 2011/04/22 01:13:17, rjrjr wrote:
What's the difference between a paramType and an argType? Seems like
the
distinction matters here, but param and arg look like synonyms to me.
paramType is the type of the formal parameter declaration, while argType
is the type of the argument to be passed in.
Should the args be leftHandType and rightHandType? This applies
throughout the
class of course, not just here.
That would work, although the terminology seems a bit odd in the context
of a method call.
I guess param and arg make sense. I've never decoupled them before.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode140
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:140:
JType[] ctorArgTypes = paramTypes;
On 2011/04/22 01:13:17, rjrjr wrote:
Looks like ctorArgTypes is refactoring chaff, should be inlined.
Yes, I created it just so I could inline it, then never did :). Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode157
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:157: } else
{
On 2011/04/22 01:13:17, rjrjr wrote:
else if
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
File user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode29
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:29:
On 2011/04/22 01:13:17, rjrjr wrote:
import static com.google.gwt.uibinder.rebind.TypeOracleUtils.*; might
make this
easier to read.
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode145
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:145:
public void testHasCompatibleMethod() {
On 2011/04/22 01:13:17, rjrjr wrote:
Many lines too long. Did you auto format?
Yes, although I did make some edits since then. The @link tags can't be
split, and Reitveld is wrapping at 80 instead of 100. I will make sure
it is formatted properly before submitting.
I have Rietveld set to 100. There's a setting for that.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode146
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:146:
assertTrue(didn't find static m1(Child),
TypeOracleUtils.hasCompatibleMethod(foo, true, m1,
On 2011/04/22 01:13:17, rjrjr wrote:
s/didn't find/Should have found/
Done.
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
LGTM On Thu, Apr 21, 2011 at 9:24 PM, j...@google.com wrote: http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Structural changes to UiBinder to make fields accessible via getters (issue1420804)
Thanks for the quick update, looking now. One thought (doesn't gate this patch): I wonder if your code bloat problem would go away if your Widgets classes were JSOs. On Wed, Apr 20, 2011 at 9:03 AM, her...@google.com wrote: http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/client/UiBinderUtil.java File user/src/com/google/gwt/uibinder/client/UiBinderUtil.java (right): http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/client/UiBinderUtil.java#newcode44 user/src/com/google/gwt/uibinder/client/UiBinderUtil.java:44: element = com.google.gwt.dom.client.Document.get() On 2011/04/19 21:07:20, rjrjr wrote: no need for the long name, this is imported Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java File user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java (right): http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java#newcode38 user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java:38: On 2011/04/19 21:07:20, rjrjr wrote: assert writer.useAttachableStrategyMabobThing(); Well, the parser is not registered if the flag is disabled so this point is never reached. But did the change anyway since this way I can spit out a better warning message. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java#newcode40 user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java:40: String widgetClassPath = writer.getClassPath(Widget.class); On 2011/04/19 21:07:20, rjrjr wrote: just use Widget.class.getName(), getClassPath adds no value I can see. Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java#newcode42 user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java:42: String code = null; On 2011/04/19 21:07:20, rjrjr wrote: Use XMLElement#consumeSingleChildElement and you don't need your own one-and-only-one checks. Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java (right): http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java#newcode95 user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java:95: com.google.gwt.uibinder.client.UiBinderUtil.LazyDomElement, elementPointer); On 2011/04/19 21:07:20, rjrjr wrote: Please use LazyDomElement.class.getName() Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java#newcode98 user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java:98: new com.google.gwt.uibinder.client.UiBinderUtil.LazyDomElement(%s), On 2011/04/19 21:07:20, rjrjr wrote: ditto Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java File user/src/com/google/gwt/uibinder/rebind/FieldManager.java (right): http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode98 user/src/com/google/gwt/uibinder/rebind/FieldManager.java:98: public void addAttachStatement(String fieldName, String format, Object... args) On 2011/04/19 21:07:20, rjrjr wrote: Don't duplicate the FieldWriter api. To get them add a new require(fieldName) method that wraps lookup() and does the die thing. Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode102 user/src/com/google/gwt/uibinder/rebind/FieldManager.java:102: logger.die(Failing adding attach statement. FieldName %s has no FieldWriter associated., On 2011/04/19 21:07:20, rjrjr wrote: die is for user error. If this is really developer error, make it a RuntimeException. Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode112 user/src/com/google/gwt/uibinder/rebind/FieldManager.java:112: public void addDetachStatement(String fieldName, String format, Object... args) On 2011/04/19 21:07:20, rjrjr wrote: ditto Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode126 user/src/com/google/gwt/uibinder/rebind/FieldManager.java:126: public FieldWriter addFieldStatement(String fieldName, String format, Object... args) On 2011/04/19 21:07:20, rjrjr wrote: ditto Done. http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode143
[gwt-contrib] Re: MobileWebApp sample. Showcases GWT providing a single app providing specialized views for Deskto... (issue1427803)
Re: gin, the dependencies will be too complicated for a sample. And this is simple enough that we can just do what gin would have done by hand, makes it a better illustration really. On Wed, Apr 20, 2011 at 3:03 PM, jlaba...@google.com wrote: If Rodrigo has time to add Editors, its likely to get done sooner than I can get to it. As far as ClientFactory, should we consider using gin? On 2011/04/20 21:51:42, rjrjr wrote: LGTM Sorry for the belated review. This is a great sample, we'll get a lot of mileage out of it. My only complaint is that the clientFactory has too many getters and is passed around too much. I'll take a crack at fixing that myself. The app also doesn't use the Editor framework like it should. John L is supposed to be on the hook for that, but I wonder if Rodrigo should go ahead and do it instead, just as an excuse to get familiar with editors. http://gwt-code-reviews.appspot.com/1427803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Finishes the job of making EventBus backward compatible, (issue1425804)
Thanks for the review.
At this point I'm not even sure what advice I would give, so I'm going to
hold off on the do not use bit.
On Tue, Apr 19, 2011 at 3:03 PM, b...@google.com wrote:
LGTM.
http://gwt-code-reviews.appspot.com/1425804/diff/1/user/src/com/google/gwt/event/shared/EventBus.java
File user/src/com/google/gwt/event/shared/EventBus.java (right):
http://gwt-code-reviews.appspot.com/1425804/diff/1/user/src/com/google/gwt/event/shared/EventBus.java#newcode28
user/src/com/google/gwt/event/shared/EventBus.java:28: public H
com.google.web.bindery.event.shared.HandlerRegistration
addHandler(Event.TypeH type, H handler) {
Should the HandlerRegistration in this package have some kind of do not
use for new code warning placed on its javadoc?
Is there a pattern for soft deprecation where we don't want to cause
deprecation warning spam, but don't want to encourage new uses of the
type? Would some kind of empty NewUsesAreBadIdea tag interface help
with this?
http://gwt-code-reviews.appspot.com/1425804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Adds method to customise ServiceLocator instantiation in ServiceLayerDecorator (issue1427801)
This looks like a job for…bobv!
On Mon, Apr 18, 2011 at 10:23 AM, Daniel Bell daniel.r.b...@gmail.comwrote:
Hi All,
I just submitted a patch to Rietveld, but wasn't sure who to add to review
it. Would somebody mind reviewing it for me?
Cheers,
Daniel
On 19 April 2011 03:11, daniel.r.b...@gmail.com wrote:
Reviewers: ,
Description:
Issue 6264: We require a way to override default instantiation of
ServiceLocators in a similar fashion to Locators in
ServiceLayerDecorator, to provide ServiceLocators with Guice.
Please review this at http://gwt-code-reviews.appspot.com/1427801/
Affected files:
user/src/com/google/web/bindery/requestfactory/server/LocatorServiceLayer.java
user/src/com/google/web/bindery/requestfactory/server/ServiceLayer.java
user/src/com/google/web/bindery/requestfactory/server/ServiceLayerDecorator.java
Index:
user/src/com/google/web/bindery/requestfactory/server/ServiceLayerDecorator.java
===
---
user/src/com/google/web/bindery/requestfactory/server/ServiceLayerDecorator.java
(revision 10013)
+++
user/src/com/google/web/bindery/requestfactory/server/ServiceLayerDecorator.java
(working copy)
@@ -61,6 +61,11 @@
}
@Override
+ public T extends ServiceLocator T createServiceLocator(ClassT
clazz) {
+return getNext().createServiceLocator(clazz);
+ }
+
+ @Override
public ClassLoader getDomainClassLoader() {
return getNext().getDomainClassLoader();
}
Index:
user/src/com/google/web/bindery/requestfactory/server/LocatorServiceLayer.java
===
---
user/src/com/google/web/bindery/requestfactory/server/LocatorServiceLayer.java
(revision 10013)
+++
user/src/com/google/web/bindery/requestfactory/server/LocatorServiceLayer.java
(working copy)
@@ -52,7 +52,7 @@
public Object createServiceInstance(Method contextMethod, Method
domainMethod) {
Class? extends ServiceLocator locatorType =
getTop().resolveServiceLocator(contextMethod, domainMethod);
-ServiceLocator locator = newInstance(locatorType,
ServiceLocator.class);
+ServiceLocator locator = createServiceLocator(locatorType);
// Enclosing class may be a parent class, so invoke on service class
Class? declaringClass = contextMethod.getDeclaringClass();
Class? serviceClass =
@@ -61,6 +61,11 @@
}
@Override
+ public T extends ServiceLocator T createServiceLocator(ClassT
serviceLocatorType) {
+return newInstance(serviceLocatorType, ServiceLocator.class);
+ }
+
+ @Override
public Object getId(Object domainObject) {
return doGetId(domainObject);
}
Index:
user/src/com/google/web/bindery/requestfactory/server/ServiceLayer.java
===
---
user/src/com/google/web/bindery/requestfactory/server/ServiceLayer.java
(revision 10013)
+++
user/src/com/google/web/bindery/requestfactory/server/ServiceLayer.java
(working copy)
@@ -128,6 +128,15 @@
public abstract Object createServiceInstance(Method contextMethod,
Method domainMethod);
/**
+ * Create an instance of the requested {@link ServiceLocator} type.
+ *
+ * @param T the requested ServiceLocator type
+ * @param clazz the requested ServiceLocator type
+ * @return an instance of the requested ServiceLocator type
+ */
+ public abstract T extends ServiceLocator T
createServiceLocator(ClassT clazz);
+
+ /**
* Returns the ClassLoader that should be used when attempting to access
* domain classes or resources.
* p
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On Mon, Apr 18, 2011 at 10:39 AM, zh...@google.com wrote:
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java
File user/src/com/google/gwt/user/rebind/rpc/Shared.java (right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode56
user/src/com/google/gwt/user/rebind/rpc/Shared.java:56: private static
SerializeFinalFieldsOptions serializeFinalFieldsValue;
On 2011/04/18 14:18:10, bobv wrote:
See comment in previous revision. Static fields in Generator types
can cause
flaky behavior.
Agree. However, if I make the value non-static, then a lot of changes
should be made in the legacy. We may keep it static for now.
I don't have the full context, but adding flakiness is not a good idea.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode100
user/src/com/google/gwt/user/rebind/rpc/Shared.java:100: propertyOracle,
RPC_PROP_SERIALIZE_FINAL_FIELDS, FALSE).toUpperCase();
On 2011/04/18 14:18:10, bobv wrote:
You can use Enum.valueOf() instead of the if block below.
Done.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
File user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode56
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:56:
private FinalFieldsTestServiceAsync finalFieldsTestService;
On 2011/04/18 14:18:10, bobv wrote:
Member sort order. Fields should be defined before methods.
Done. Also did in FinalFieldsTestFalseNoWarn.java
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:27:
public class FinalFieldsNode implements IsSerializable {
On 2011/04/18 14:18:10, bobv wrote:
Use Serializable instead of IsSerializable.
Done.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
File
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java#newcode28
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java:28:
public FinalFieldsNode transferObject(FinalFieldsNode node) {
On 2011/04/18 14:18:10, bobv wrote:
Check the incoming values in node to make sure that final fields are
being set
properly when sent by the client.
Done.
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Makes EventBus available outside of the gwt package, in (issue1394803)
Done, should submit soon. On Mon, Apr 18, 2011 at 11:13 AM, rj...@google.com wrote: http://gwt-code-reviews.appspot.com/1394803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Remove stray reference to ElementFactory (issue1423801)
LGTM On Fri, Apr 15, 2011 at 7:16 AM, sbruba...@google.com wrote: Reviewers: rjrjr, Description: Remove stray reference to ElementFactory Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1423801/ Affected files: M user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java Index: user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java === --- user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java (revision 9995) +++ user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java (working copy) @@ -47,8 +47,6 @@ private static final String TEMPLATE_SUFFIX = .ui.xml; - private static final String ELEMENT_FACTORY_PROPERTY = uibinder.html.elementfactory; - private static final String XSS_SAFE_CONFIG_PROPERTY = UiBinder.useSafeHtmlTemplates; /** -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move AutoBean package to com.google.web.bindery.autobean package. (issue1414803)
LGTM On Fri, Apr 15, 2011 at 7:00 AM, b...@google.com wrote: Updated the patch, remembering to move the client.impl code into autobean.gwt.client.impl. http://gwt-code-reviews.appspot.com/1414803/diff/1/user/src/com/google/web/bindery/autobean/vm/AutoBeanFactorySource.java File user/src/com/google/web/bindery/autobean/vm/AutoBeanFactorySource.java (right): http://gwt-code-reviews.appspot.com/1414803/diff/1/user/src/com/google/web/bindery/autobean/vm/AutoBeanFactorySource.java#newcode32 user/src/com/google/web/bindery/autobean/vm/AutoBeanFactorySource.java:32: * AutoBeanFactoyModel. On 2011/04/15 00:51:43, rjrjr wrote: Please put this is experimental disclaimer here and in the javadoc of the other vm packages, along with the package info Done. http://gwt-code-reviews.appspot.com/1414803/diff/1/user/src/com/google/web/bindery/autobean/vm/package-info.java File user/src/com/google/web/bindery/autobean/vm/package-info.java (right): http://gwt-code-reviews.appspot.com/1414803/diff/1/user/src/com/google/web/bindery/autobean/vm/package-info.java#newcode24 user/src/com/google/web/bindery/autobean/vm/package-info.java:24: * @see com.google.web.bindery.autobean.vm.AutoBeanFactoryMagic On 2011/04/15 00:51:43, rjrjr wrote: Magic is gone, right? Might want to grep for it. Done. http://gwt-code-reviews.appspot.com/1414803/diff/1/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java File user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java (right): http://gwt-code-reviews.appspot.com/1414803/diff/1/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java#newcode47 user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java:47: public abstract class AbstractRequestFactoryEditorDriverR, E extends EditorR It's in a gwt subpackage? It seems weird to put a RequestFactory dependency back in the main com.google.gwt namespace. http://gwt-code-reviews.appspot.com/1414803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Fixes a typo in GWT emulation of java.util.IdentityHashSet that (issue1395804)
Sorry for chiming in so late. Does the UmbrellaException constructor really need to be public? IIRC, won't GWT RPC be happy enough with a protected or package private constructor? On Tue, Apr 5, 2011 at 10:44 AM, schen...@google.com wrote: http://gwt-code-reviews.appspot.com/1395804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a typo in GWT emulation of java.util.IdentityHashSet that (issue1395804)
Thanks for he confirmation. I'll get it, I'm in there right now anyway. On Wed, Apr 13, 2011 at 3:31 PM, schen...@google.com wrote: Passes tests if the constructor is private. Would you like me to change it? Cheers, Stephen. On 2011/04/13 22:20:12, rjrjr wrote: Sorry for chiming in so late. Does the UmbrellaException constructor really need to be public? IIRC, won't GWT RPC be happy enough with a protected or package private constructor? On Tue, Apr 5, 2011 at 10:44 AM, mailto:schen...@google.com wrote: http://gwt-code-reviews.appspot.com/1395804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors http://gwt-code-reviews.appspot.com/1395804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: GWT Private/Protected Members
We've tried to get better about that kind of thing over the years. The
widgets you listed are among our oldest.
The new widget family described at
http://code.google.com/p/google-web-toolkit/wiki/CellBackedWIdgets should be
better on this score. Re: Tree in particular, you might consider trying
CellTree instead.
When you bump into a particular spot that is thwarting you and see a way to
open it up without breaking existing apps, we're always eager to receive
patches: http://code.google.com/webtoolkit/makinggwtbetter.html
On Tue, Apr 12, 2011 at 3:36 PM, Clécio Varjão cleciovar...@gmail.comwrote:
I understand the cautiousness regarding what is public, but I also
believe that protected methods already have the protection necessary,
assuming that developers who are extending a class usually knows what
they are doing, and the source code is available so developers can
check, and maintain compatibility.
Even methods that are strict for infrastructure could have a suffix
( or prefix ) that indicates developer to use it with caution, and
that this method is not guaranteed to exist in future releases:
E.g:
//The current implementation for the private setNewSelection() method,
could be as follows
public class SuggestBox ... {
protected void setNewSelectionImpl(Suggestion curSuggestion) {
//code
}
}
This way gives developers a great flexibility to extend GWT classes,
and even introduce temporary bug fixes.
In a future release, GWt decides to expose this method as a concrete
implementation and could change to:
public class SuggestBox ... {
protected void setNewSelection(Suggestion curSuggestion) {
setNewSelectionImpl(curSuggestion); // this will maintain
compatibility, but is not required at all.
}
}
However, the method setNewSelectionImpl can completely disappear as it
is an draft and not part of the final API, therefore developers will
be using it with caution.
Anyways, just bouncing ideas. I also understand that if the standard
is overused, and never brought back to the GWT team so it can make
part of the API, it could create chaos among GWT developers.
Cheers!
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Autoformat the api-checker tool source (issue1405801)
I don't think it's reasonable to ask Eric to tweak the auto formatter. We had that conversation already. He's just doing the same thing we have eclipse configured to do, right? I can't look for real right now. Did you really find something aggregious? On Apr 5, 2011 9:34 AM, p...@google.com wrote: -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] RR: Promoting RequestFactory to a higher package
We were only concerned about public api. Do you see anything we're missing there? On Apr 1, 2011 3:09 PM, Thomas Broyer t.bro...@gmail.com wrote: Note that AutoBeanUtils uses WeakMapping which lives in com.google.gwt.core.client (yes, this is a client class used in shared, and thus server code; WeakMapping is also directly referenced through server code, namely in ProxyAutoBean) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Makes EventBus available outside of the gwt package, in (issue1394803)
We want to be able to experiment with non-GWT clients of web services, particularly via RequestFactory. But I have to put emphasis on the word experiment. Non-GWT won't be a supported path soon, if ever. On Thu, Mar 31, 2011 at 8:06 AM, Andrés Testi andres.a.te...@gmail.comwrote: Why bindery package is nested in a web package? Are these APIs not available for non web applications? Regards. - Andrés On 31 mar, 01:19, rj...@google.com wrote: Ready for review. John, can you keep me honest on the treatment of com.google.gwt.event.shared, and the choices made in the new event package? Bob, does this fit what you have in mind for the bindery organization? Note that I've updated Activity and Place to use the new classes, but not RequestFactory. I won't submit this until Dan has his big patch in place, and I'll make the RF changes before I do. http://gwt-code-reviews.appspot.com/1394803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Makes EventBus available outside of the gwt package, in (issue1394803)
On Fri, Apr 1, 2011 at 11:16 AM, John LaBanca jlaba...@google.com wrote: On Fri, Apr 1, 2011 at 2:13 PM, Ray Ryan rj...@google.com wrote: On Fri, Apr 1, 2011 at 10:38 AM, John LaBanca jlaba...@google.comwrote: I don't think Andrés was asking why they weren't in the gwt package. He's sking why they are in the com.google.web package if they are usable outside of the web domain. It seems like we are moving from a very limited package scope to a slightly less limited package scope. I'm sure you've debated this plenty, but since I'm doing the code review, I have to question the package name com.google.web.bindery. com - okay, off to a good start .google - I like it .web - I agree with Andrés here. Wouldn't a use case be to run this code on the server, or even in an Android app? You're reading web to mean HTML. I'm reading it as app that talks to a web service, regardless of what it's written in. It seems like web could be dropped, saving 4 bytes in a lot of files. Not an option, I tried. Creating a new sub-package of com.google is not something we can do unilaterally. .bindery - What's bindery? It sounds like its related to UiBinder, but UiBinder is truly cliient one. Is it the name of the new project? From the README file that I clearly need to add: bindery is a minimal open source web app framework for GWT with experimental support for JRE clients. It is based around an app-wide event bus and and an RPC system especially useful for CRUD style apps. The consistent theme of the code in this package is that it allows binding decoupled systems in a type safe way with a minimum of boilerplate. Thus bindery. And yes, it took a very long time to come up with that name. I don't like it. No good reason, it just doesn't have a nice ring to it. I will give you an alternative in fifteen minutes. That's a very generous offer. If you drop web, we end up with: com.google.gwt - Libraries used to create GWT applications. com.google.bindary - Useful Google Java libraries, but google provides other libraries, so what is bindary's mission? Thanks, John LaBanca jlaba...@google.com On Fri, Apr 1, 2011 at 12:46 PM, Ray Ryan rj...@google.com wrote: We want to be able to experiment with non-GWT clients of web services, particularly via RequestFactory. But I have to put emphasis on the word experiment. Non-GWT won't be a supported path soon, if ever. On Thu, Mar 31, 2011 at 8:06 AM, Andrés Testi andres.a.te...@gmail.com wrote: Why bindery package is nested in a web package? Are these APIs not available for non web applications? Regards. - Andrés On 31 mar, 01:19, rj...@google.com wrote: Ready for review. John, can you keep me honest on the treatment of com.google.gwt.event.shared, and the choices made in the new event package? Bob, does this fit what you have in mind for the bindery organization? Note that I've updated Activity and Place to use the new classes, but not RequestFactory. I won't submit this until Dan has his big patch in place, and I'll make the RF changes before I do. http://gwt-code-reviews.appspot.com/1394803/ -- 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
Re: [gwt-contrib] Re: Makes EventBus available outside of the gwt package, in (issue1394803)
On Fri, Apr 1, 2011 at 10:38 AM, John LaBanca jlaba...@google.com wrote: I don't think Andrés was asking why they weren't in the gwt package. He's sking why they are in the com.google.web package if they are usable outside of the web domain. It seems like we are moving from a very limited package scope to a slightly less limited package scope. I'm sure you've debated this plenty, but since I'm doing the code review, I have to question the package name com.google.web.bindery. com - okay, off to a good start .google - I like it .web - I agree with Andrés here. Wouldn't a use case be to run this code on the server, or even in an Android app? You're reading web to mean HTML. I'm reading it as app that talks to a web service, regardless of what it's written in. It seems like web could be dropped, saving 4 bytes in a lot of files. Not an option, I tried. Creating a new sub-package of com.google is not something we can do unilaterally. .bindery - What's bindery? It sounds like its related to UiBinder, but UiBinder is truly cliient one. Is it the name of the new project? From the README file that I clearly need to add: bindery is a minimal open source web app framework for GWT with experimental support for JRE clients. It is based around an app-wide event bus and and an RPC system especially useful for CRUD style apps. The consistent theme of the code in this package is that it allows binding decoupled systems in a type safe way with a minimum of boilerplate. Thus bindery. And yes, it took a very long time to come up with that name. If you drop web, we end up with: com.google.gwt - Libraries used to create GWT applications. com.google.bindary - Useful Google Java libraries, but google provides other libraries, so what is bindary's mission? Thanks, John LaBanca jlaba...@google.com On Fri, Apr 1, 2011 at 12:46 PM, Ray Ryan rj...@google.com wrote: We want to be able to experiment with non-GWT clients of web services, particularly via RequestFactory. But I have to put emphasis on the word experiment. Non-GWT won't be a supported path soon, if ever. On Thu, Mar 31, 2011 at 8:06 AM, Andrés Testi andres.a.te...@gmail.comwrote: Why bindery package is nested in a web package? Are these APIs not available for non web applications? Regards. - Andrés On 31 mar, 01:19, rj...@google.com wrote: Ready for review. John, can you keep me honest on the treatment of com.google.gwt.event.shared, and the choices made in the new event package? Bob, does this fit what you have in mind for the bindery organization? Note that I've updated Activity and Place to use the new classes, but not RequestFactory. I won't submit this until Dan has his big patch in place, and I'll make the RF changes before I do. http://gwt-code-reviews.appspot.com/1394803/ -- 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: Adds a no-op emulation of TestSuite, to prevent error spam (or outright (issue1399803)
ping On Thu, Mar 31, 2011 at 1:41 PM, rj...@google.com wrote: Reviewers: fabbott, Description: Adds a no-op emulation of TestSuite, to prevent error spam (or outright failure under -strict mode) in web mode tests that accidentally pick the things up in their class path. Please review this at http://gwt-code-reviews.appspot.com/1399803/ Affected files: A user/super/com/google/gwt/junit/translatable/junit/framework/TestSuite.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding table rendering tests to micro benchmarks. Table rendering tests are multiple orders of m... (issue1394802)
pong. The queue is deep… On Thu, Mar 31, 2011 at 11:42 AM, jlaba...@google.com wrote: ping http://gwt-code-reviews.appspot.com/1394802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding table rendering tests to micro benchmarks. Table rendering tests are multiple orders of m... (issue1394802)
Or do I mean that the stack is long? So many metaphors to muddle, so little time. On Thu, Mar 31, 2011 at 12:57 PM, Ray Ryan rj...@google.com wrote: pong. The queue is deep… On Thu, Mar 31, 2011 at 11:42 AM, jlaba...@google.com wrote: ping http://gwt-code-reviews.appspot.com/1394802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] What do you think of adding bidi support to LayoutPanel
The LayoutPanels' already swap properly in RTL locales, don't they?
http://gwt.google.com/samples/Showcase/Showcase.html?locale=ar_YE
http://gwt.google.com/samples/Showcase/Showcase.html?locale=en
On Thu, Mar 31, 2011 at 2:33 PM, Jeff Larsen larse...@gmail.com wrote:
In some instances, it would be nice to have LayoutPanel swap left and
right. I was thinking of something like adding a
private boolean bidi = false;
public void setWidgetLeftWidth(Widget child, double left, Unit leftUnit,
double width,
Unit widthUnit) {
assertIsChild(child);
if (bidi isRtl()) {
getLayer(child).setRightWidth(left, leftUnit, width, widthUnit);
} else {
getLayer(child).setLeftWidth(left, leftUnit, width, widthUnit);
}
animate(0);
}
public void setBidiEnabled(boolean enable){
bidi = enable;
}
etc etc etc.
What do you guys think? This would allow for future enhancements like
making TabLayoutPanel bidi enabled, where the tabs appear on the right side
and work their way left instead of on the starting on the left and being
added to the left.
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding table rendering tests to micro benchmarks. Table rendering tests are multiple orders of m... (issue1394802)
LGTM On Thu, Mar 31, 2011 at 3:40 PM, jlaba...@google.com wrote: http://gwt-code-reviews.appspot.com/1394802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 5129: Accomodate RunAsync in ActivityManager (issue1383802)
It's still unclear to me that your AbstractAsyncActivity actually works. It seems like it will just produce a single split point, as Thomas suggested of my first patch here. Have you seen it make multiple fragments? On Tue, Mar 29, 2011 at 2:36 AM, Antoine DESSAIGNE antoine.dessai...@gmail.com wrote: Hi, Should I modify my patch and repost it or do you go with the http://gwt-code-reviews.appspot.com/1386806/ option ? Again I truely think that it's not necessary to add anything other than the simple AbstractAsyncActivity class (we may call it otherwise though). Antoine. 2011/3/25 rj...@google.com This struck me as being more complicated than warranted. I took a crack at it myself in http://gwt-code-reviews.appspot.com/1386806/, but I may be making some bad assumptions. http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java File user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java (right): http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java#newcode26 user/src/com/google/gwt/activity/shared/AbstractAsyncActivity.java:26: * onCancel. These docs are a bit confusing. All Activities are async, but what you're really talking about here is code splitting via GWT.runAsync() http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java File user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java (right): http://gwt-code-reviews.appspot.com/1383802/diff/1/user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java#newcode85 user/src/com/google/gwt/activity/shared/AsyncActivityProvider.java:85: * Transforms the given synchronous activity into an asynchronous one. Again, synchronous activity isn't a good term. http://gwt-code-reviews.appspot.com/1383802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 5700 (issue1388804)
On Fri, Mar 25, 2011 at 7:26 AM, akito.noz...@gmail.com wrote: I have a simple question on one of the test. As I was making this correction I noticed that one of my test case comment is wrong. The last remove test is actually incorrect (my comment). My question is what is the expected output of the last test. If you have 2 nested event bus, and if you call removeHandlers on the inner eventbus. Do you expect this remove to propagate to the outer event bus and remove it from its registration? I really hadn't thought about nesting the things before your patch came in. You'll note that there is no support for nesting among activities in general. With the current setup, the semantics seem okay. Am I reading it right? - I nest innerBus in outerBus - I register innerHandler and outerHandler on them, respectively - If I call innerBus.removeHandlers(), innerHandler will no longer receive events - If I instead call outerBus.removeHandlers(), neither handler will receive events any longer The only problem is that the HandlerRegistrations vended by InnerResettable cannot be gc'd until removeHandlers is called on both InnerResettable and OuterResettable. If that's the case, I think this implementation is fine, if we javadoc carefully. I think your test shouldn't be about the specific intermediate count on outerBus, but rather focus that both busses get to zero when their removes are called. Am I answering the right question? rjrjr If the answer is no it shouldn't propagate to the upper eventbus. The current setup will work nicely. The only weird thing is that the upper Resettable event bus thinks the event is still attached. If the answer is yes and the eventbus should be removed from the top. I came up with few ways to deal with the situation with pros and cons. 1. Recognize that you are wrapping a resettable event bus and don't record the registrationHandler at this level (simple pass through). On removeHandlers call removeHandhlers on the wrapped bus. * Pros: No new memory leak introduced. * Cons: If other wrapper is used this method will not work correctly. For example ResettableEventBus -CountingEventBus - ResettableEventBus - SimpleEventBus. 2. Create a event that fires when things are removed from simple event bus (RemoveEvent of some sort). * Pros: Will work regardless of how complicated the nesting gets. * Cons: How do we know when to unregister our ResettableEventBus from this event? Simple solution is to unregister when nothing is left in the registration and add it back when something gets attached. Also more complex. Should user be allowed to attach to this event? Number 2 is definitely the best solution but rises many questions... Comment/Suggestion? http://gwt-code-reviews.appspot.com/1388804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add tests of nesting semantics for ResettableEventBus (issue1395802)
I wasn't trying to catch the bug, I was trying to illustrate the bits that work already. On Tue, Mar 29, 2011 at 3:18 PM, pjul...@gmail.com wrote: http://gwt-code-reviews.appspot.com/1395802/diff/1/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java File user/test/com/google/gwt/event/shared/ResettableEventBusTest.java (right): http://gwt-code-reviews.appspot.com/1395802/diff/1/user/test/com/google/gwt/event/shared/ResettableEventBusTest.java#newcode83 user/test/com/google/gwt/event/shared/ResettableEventBusTest.java:83: narrowScope.removeHandlers(); With respect, I think these tests miss the bug completely. ResettableEventBus has registrations which is a set. If you're calling removeHandlers, you're not triggering the bug because this blindly clears the registrations set. To trigger the bug, you need to save the HandlerRegistration object that was returned to you. If you call removeHandler() on the HandlerRegistration object, you'll find that nesting doesn't work properly. http://gwt-code-reviews.appspot.com/1395802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] RR: Promoting RequestFactory to a higher package
Yes, it's true, we spaced that EventBus is part of GWT's public API. We're now thinking that the new packages will be: com.google.bindery.event com.google.bindery.autobean com.google.bindery.requestfactory Patches should start appearing this week. Note that this is strictly a refactoring of code that is already JRE compatible. The i18n subthread is interesting, but out of scope for this effort. On Fri, Mar 25, 2011 at 2:53 PM, Stephen Haberman stephen.haber...@gmail.com wrote: Reactions? Having to change import statements sounds perfectly fine to me. Other misc comments from the peanut gallery, though likely nothing you guys haven't likely already figured out. Just curious. Should c.g.requestfactory have zero GWT dependencies? I.e. this non-GWT/pure JRE jar you spoke of would be everything in that package (without any build tricks to filter out GWT stuff)? Perhaps then GWT-specific RF code like any of the client/rebind code (and even GWT-coupled server code) should stay in c.g.g.requestfactory? Also, the new c.g.rf.shared.RequestFactory imports c.g.gwt's EventBus--no idea if that's a big deal or not, but seems odd if c.g.requestfactory is supposed to be reusable/non-GWT standalone (which, may very likely be a constraint I'm making up--it just seems elegant if non-GWT reuse is what you're after). - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] RR: Promoting RequestFactory to a higher package
RequestFactory is proving itself useful in non-GWT contexts, so we would like to give it more independence. Our plan with the GWT 2.3 release is to copy com.google.gwt.requestfactory to com.google.requestfactory, and deprecate everything in the old location. We will also provide a jar that includes the pure JRE version of request factory with no other GWT dependencies. (This change will probably not make the first 2.3 milestone release.) With 2.4, we'll delete the code in the old location. This is faster than usual promise of keeping deprecated code in place for two releases, but we really don't want to leave so much redundant code around any longer than we have to. The API will not change; client code should be able to migrate by simply by changing import statements. Reactions? rjrjr -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Memory leak in ResettableEventBus
Thanks, I'll look at this today. On Mar 24, 2011 9:08 AM, Akito Nozaki akito.noz...@gmail.com wrote: Not sure how this contributing thing works. I was assuming that things get pushed here automatically if I created an issue at the review site. I uploaded some code to deal with memory leak in ResettableEventBus. Mainly with nested ResettableEventBus and passing ResettableEventBus into ActivityManager. http://code.google.com/p/google-web-toolkit/issues/detail?id=5700 http://gwt-code-reviews.appspot.com/1388804/ Really simple fix and would really love to get this included in the next release as I keep having to patch/include my own ResettableEventBus in my code. As I was working on this code I noticed that CountingEventBus has the same bug but I'm guessing that only gets used in tests? Also I wasn't sure how to write a test that check the internal state. Mainly to test for the memory leak the current ResettableEventBus has. Akito -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Move com.google.gwt.requestfactory to com.google.requestfactory (issue1383808)
Yeah, Dan made the valiant effort here but I don't think it's practical. I don't feel bad asking users to change import statements to pick up bug fixes. It would be great if we can get this into the 2.3 rc. On Mar 24, 2011 7:05 AM, Daniel Rice (דניאל רייס) r...@google.com wrote: I spent a few days attempting this -- it's not so simple. For example: 1) Enums can't extend Enums. Several public types contain nested Enums. 2) If a type oldA extends newA, and a method in it returns an instance of oldA, you cant simply delegate to the implementation in newA since that will return a newA, which is not an instanceof oldA. So you need to write wrappers for everything. 3) It's unclear to me how inheritance works on annotations 4) Generators will have to be modified to work off the different annotation types I think there were other problems as well that I can't recall at the moment. My belief is that continuing with this approach would have polluted the new code base with special cases to handle parallel enums and annotations. I spoke to Ray about it and he agreed to the plan of leaving a snapshot in the old location and trying to remove it in the next release. On Thu, Mar 24, 2011 at 8:57 AM, b...@google.com wrote: Hollow out the to-be-deleted classes to avoid the massive duplication of code going on in this patch. The old types should extend the new types and where that isn't possible, the old type should be rewritten as a delegate. Anything in the rebind or **/impl packages can be a straight move. They're not public types. http://gwt-code-reviews.appspot.com/1383808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: [RFC] GWT Widgets that ROCK!
Antoine, I think the Appearance plan is closer to what you want than you realize. In particular, you should know that we're working on a change to UiBinder to allow it to generate SafeHtmlRenderer instances, and then to allow those instances to manage cell event handling. We'll share a design proposal soon. That said, there is still too much boilerplate and hackery required to make a new one of these things. It does seem like we should be able to require a lot less mindless configuration than we do. Making FooWidget.java, Foo.css and Foo.ui.xml just work by existing, perhaps allowing me to GWT.create(FooWidget.class) with no extra configuration, seems like something we should make an explicit goal. On Wed, Mar 23, 2011 at 6:31 AM, Jens jens.nehlme...@gmail.com wrote: With the appearance pattern your proposed Theme class would be the same as a GWT module containing all the deferred bindings for changing all the appearance classes of all widgets. So you would inherit this theme module and you are done. I think you could also overwrite a specific deferred binding in your app module if the compiler always picks the last matching rule. Or am I missing something? But indeed it would be nice to have a UiBinder template (or a general xml template) which defines the DOM structure of a widget instead of a render method. That way we could pass the widget.ui.xml and its widget.css file to a designer and the developer only has to deal with the appearance class and event handling. But with a separate xml template I guess it may be difficult for a developer to implement the onBrowserEvent method because he can not assume any given DOM structure for a widget (each theme may have changed it drastically). So as you pointed out, event handling will be the major issue. But maybe there is a solution for it? I really like the idea of decoupling the DOM structure and having a separate xml file for it. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
Christoph, can you take this review? On Sat, Mar 19, 2011 at 10:04 AM, t.bro...@gmail.com wrote: I tried to limit the changes to non-formatting ones. I also didn't go as far as http://gwt-code-reviews.appspot.com/1384801 wrt error handling to limit the amount of changes and avoid merge conflicts (or make them easier to resolve). http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Questions on the process of issue fixing.
Thanks for the patches! No on noticed them because you didn't set a reviewer. Since you are addressing specific items on the issue tracker, the owners of those tickets would be the right targets. It's also a good idea to append the urls of the patches to the tickets. rjrjr On Fri, Mar 18, 2011 at 10:31 AM, Antoine DESSAIGNE antoine.dessai...@gmail.com wrote: Hi, I wanted to know more about GWT and therefore I decided to try fixing some of the issues. I've followed the Making it better page as well as the help on upload.py. I've proposed 3 patches ([1], [2] and [3]) but none of them appeared on this mailing-list nor were commented/reviewed. I'm wondering whether I forgot something or if you simply didn't have time to have a look at them. So I have several questions: - Am I proposing patches the right way ? - How do you fix new issues: do you revert your code base after each patch submission, or do you have an easier way ? - Since I'm not sure if I will be able to fix an issue, should I send an email to this list before looking at it ? How should I tell you that i'm working on something ? Also, I have a more technical question: what's the right way to handle CSS and messages in the client.ui package: CssResource (with new CSS files) and Constants ? Thanks a lot for your insights on this matter. Antoine. [1] http://gwt-code-reviews.appspot.com/1369811/ [2] http://gwt-code-reviews.appspot.com/1383802/ [3] http://gwt-code-reviews.appspot.com/1388802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Questions on the process of issue fixing.
To your specific questions: On Fri, Mar 18, 2011 at 1:02 PM, Ray Ryan rj...@google.com wrote: Thanks for the patches! No on noticed them because you didn't set a reviewer. Since you are addressing specific items on the issue tracker, the owners of those tickets would be the right targets. It's also a good idea to append the urls of the patches to the tickets. rjrjr On Fri, Mar 18, 2011 at 10:31 AM, Antoine DESSAIGNE antoine.dessai...@gmail.com wrote: Hi, I wanted to know more about GWT and therefore I decided to try fixing some of the issues. I've followed the Making it better page as well as the help on upload.py. I've proposed 3 patches ([1], [2] and [3]) but none of them appeared on this mailing-list nor were commented/reviewed. I'm wondering whether I forgot something or if you simply didn't have time to have a look at them. So I have several questions: - Am I proposing patches the right way ? You did fine, you just missed step 8 in http://code.google.com/webtoolkit/makinggwtbetter.html#submittingpatches - How do you fix new issues: do you revert your code base after each patch submission, or do you have an easier way ? Most of us use git, and make a separate branch for each fix - Since I'm not sure if I will be able to fix an issue, should I send an email to this list before looking at it ? How should I tell you that i'm working on something ? An issue tracker ticket is the best forum Also, I have a more technical question: what's the right way to handle CSS and messages in the client.ui package: CssResource (with new CSS files) and Constants ? Yes for CssResource. If this is in the context of a new widget, refer to this recent discussion: https://groups.google.com/d/topic/google-web-toolkit-contributors/SclEt5RzbvA/discussion Re: messages, that's an excellent question. John and John? Thanks a lot for your insights on this matter. Antoine. [1] http://gwt-code-reviews.appspot.com/1369811/ [2] http://gwt-code-reviews.appspot.com/1383802/ [3] http://gwt-code-reviews.appspot.com/1388802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Questions on the process of issue fixing.
Ah, I'm a liar, you did update the patches in question. It's perfeclty reasonable to ping the issue itself, or calling us to task on this list like you did. I apologize that we all left you hanging like this. On Fri, Mar 18, 2011 at 1:08 PM, Ray Ryan rj...@google.com wrote: To your specific questions: On Fri, Mar 18, 2011 at 1:02 PM, Ray Ryan rj...@google.com wrote: Thanks for the patches! No on noticed them because you didn't set a reviewer. Since you are addressing specific items on the issue tracker, the owners of those tickets would be the right targets. It's also a good idea to append the urls of the patches to the tickets. rjrjr On Fri, Mar 18, 2011 at 10:31 AM, Antoine DESSAIGNE antoine.dessai...@gmail.com wrote: Hi, I wanted to know more about GWT and therefore I decided to try fixing some of the issues. I've followed the Making it better page as well as the help on upload.py. I've proposed 3 patches ([1], [2] and [3]) but none of them appeared on this mailing-list nor were commented/reviewed. I'm wondering whether I forgot something or if you simply didn't have time to have a look at them. So I have several questions: - Am I proposing patches the right way ? You did fine, you just missed step 8 in http://code.google.com/webtoolkit/makinggwtbetter.html#submittingpatches - How do you fix new issues: do you revert your code base after each patch submission, or do you have an easier way ? Most of us use git, and make a separate branch for each fix - Since I'm not sure if I will be able to fix an issue, should I send an email to this list before looking at it ? How should I tell you that i'm working on something ? An issue tracker ticket is the best forum Also, I have a more technical question: what's the right way to handle CSS and messages in the client.ui package: CssResource (with new CSS files) and Constants ? Yes for CssResource. If this is in the context of a new widget, refer to this recent discussion: https://groups.google.com/d/topic/google-web-toolkit-contributors/SclEt5RzbvA/discussion Re: messages, that's an excellent question. John and John? Thanks a lot for your insights on this matter. Antoine. [1] http://gwt-code-reviews.appspot.com/1369811/ [2] http://gwt-code-reviews.appspot.com/1383802/ [3] http://gwt-code-reviews.appspot.com/1388802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Promotes Gin's AsyncProvider to GWT, along with a more general (issue1387801)
[+google-...@googlegroups.com] What dependency? DI is a pattern, not a commitment to a particular framework. That said, I agree that taking AsyncProvider from Gin is a bit presumptuous. I meant to include the gin community on this patch, adding them now. What do you think, folks? The goal here is to sever the dependency between com.google.gwt.inject.client and GWT RPC. On Mar 12, 2011 10:36 AM, ara...@gmail.com wrote: I support the move of the AsyncCallback! What is the purpose of adding AsyncProvider to GWT's types? It seems only really useful if using dependency injection (other usages are usually abuses :) and I don't see why GWT would want that kind of dependency? http://gwt-code-reviews.appspot.com/1387801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
LGTM, SGTM, 10-4 On Fri, Mar 11, 2011 at 7:36 AM, j...@google.com wrote: http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java File user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java (right): http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java#newcode31 user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java:31: public class MockMessageCatalogContext implements On 2011/03/11 14:33:53, rjrjr wrote: If this will be useful to non-gwt teams writing their own tests, it should be in user/src/.../i18n/server/testing. Note user/src, not user/test Done. http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Comment on BeanValidation in google-web-toolkit
It's in svn only. We haven't included it in release jars yet, it's too raw. On Mar 9, 2011 4:47 AM, codesite-nore...@google.com wrote: Comment by mail.mic...@googlemail.com: Where is the com.google.gwt.validation package? I cannot find it For more information: http://code.google.com/p/google-web-toolkit/wiki/BeanValidation -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtml Rendering for UiBinder (issue1305801)
Makes sense, but let's file a follow up issue on that rather than block this
patch.
On Mar 9, 2011 9:04 AM, x...@google.com wrote:
This is really great! It pretty much completely removes uibinder out of
the security-relevant codebase.
http://gwt-code-reviews.appspot.com/1305801/diff/55001/user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java
File
user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java
(right):
http://gwt-code-reviews.appspot.com/1305801/diff/55001/user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java#newcode77
user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java:77:
return
uiWriter.tokenForSafeHtmlExpression(messages.declareMessage(message));
Methods in Messages interfaces can themselves be declared to return
SafeHtml
(
http://code.google.com/webtoolkit/doc/latest/DevGuideI18nMessages.html#SafeHtmlMessages
).
I'm wondering if it would work to change MessageWriter#writeDeclaration
to emit declarations of Messages methods that return SafeHtml rather
than String. And if that's done, would that remove the need to use
tokenForSafeHtmlExpression here (in which case that method could be
deleted altogether)?
http://gwt-code-reviews.appspot.com/1305801/diff/55001/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/1305801/diff/55001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode686
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:686: public
String tokenForSafeHtmlExpression(String expression) {
This is the one place where potential HTML unsafety could be introduced
(if this method were called on an expression that is not in fact a safe
constant). The only use in this CL appears to be in
HtmlMessageInterpreter; see a question there if it's possible to avoid
relying on this method. If so, this method could be removed.
http://gwt-code-reviews.appspot.com/1305801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: property fall back value evaluation scheme - enable fall back bindings. (issue1369807)
Does the new IE9 value for user.agent imply yet another permutation? We should really avoid that if we can, and so far it sounds like it might not be needed. Can we introduce IE9 without causing a new hard perm? On Fri, Mar 4, 2011 at 11:07 AM, j...@google.com wrote: Mostly LGTM Needs a unit test for property fallback behavior. http://gwt-code-reviews.appspot.com/1369807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: property fall back value evaluation scheme - enable fall back bindings. (issue1369807)
LGTM On Fri, Mar 4, 2011 at 11:04 AM, jlaba...@google.com wrote: LGTM But please format all files. http://gwt-code-reviews.appspot.com/1369807/diff/1/dev/core/src/com/google/gwt/core/ext/DefaultSelectionProperty.java File dev/core/src/com/google/gwt/core/ext/DefaultSelectionProperty.java (right): http://gwt-code-reviews.appspot.com/1369807/diff/1/dev/core/src/com/google/gwt/core/ext/DefaultSelectionProperty.java#newcode89 dev/core/src/com/google/gwt/core/ext/DefaultSelectionProperty.java:89: extra spaces http://gwt-code-reviews.appspot.com/1369807/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right): http://gwt-code-reviews.appspot.com/1369807/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java#newcode163 dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java:163: spaces http://gwt-code-reviews.appspot.com/1369807/diff/1/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java File dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java (right): http://gwt-code-reviews.appspot.com/1369807/diff/1/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java#newcode137 dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java:137: spaces http://gwt-code-reviews.appspot.com/1369807/diff/1/user/src/com/google/gwt/user/rebind/UserAgentPropertyGenerator.java File user/src/com/google/gwt/user/rebind/UserAgentPropertyGenerator.java (right): http://gwt-code-reviews.appspot.com/1369807/diff/1/user/src/com/google/gwt/user/rebind/UserAgentPropertyGenerator.java#newcode50 user/src/com/google/gwt/user/rebind/UserAgentPropertyGenerator.java:50: // Opera Move // Opera comment down a line. http://gwt-code-reviews.appspot.com/1369807/diff/1/user/src/com/google/gwt/user/rebind/UserAgentPropertyGenerator.java#newcode148 user/src/com/google/gwt/user/rebind/UserAgentPropertyGenerator.java:148: extra spaces http://gwt-code-reviews.appspot.com/1369807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: property fall back value evaluation scheme - enable fall back bindings. (issue1369807)
But we *don't* distinguish ie7 and ie8, and IIRC that was to avoid making a new permutation. At the very least, why don't we collapse the ie9 permutation by default if we can? On Fri, Mar 4, 2011 at 11:35 AM, j...@google.com wrote: A user can already collapse the permutations using softperms, and I don't think we should assume that for them, just like we don't collapse ie6/ie8 using softperms today. http://gwt-code-reviews.appspot.com/1369807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
On Thu, Mar 3, 2011 at 8:12 AM, j...@google.com wrote:
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java
File user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java#newcode1
user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java:1: /*
I'll revert.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/AbstractResource.java
File user/src/com/google/gwt/i18n/rebind/AbstractResource.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/AbstractResource.java#newcode1
user/src/com/google/gwt/i18n/rebind/AbstractResource.java:1: /*
On 2011/03/03 01:07:39, rjrjr wrote:
no real change
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java
File
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java#newcode44
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java:44:
interface MessageCatalogContext {
On 2011/03/03 01:07:39, rjrjr wrote:
Since this is nested, how about just calling it Context?
I think the normal case for someone using it this is just to use
auto-import in their IDE, so the source is likely to jus t have Context,
which seems likely to conflict with other uses and less understandable.
If you still would prefer shortening the name, I am happy to do it.
If you don't mind.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java
File
user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java#newcode219
user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java:219:
String fileName) throws MessageProcessingException {
On 2011/03/03 01:07:39, rjrjr wrote:
not thrown
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageFormVisitor.java
File user/src/com/google/gwt/i18n/server/MessageFormVisitor.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageFormVisitor.java#newcode54
user/src/com/google/gwt/i18n/server/MessageFormVisitor.java:54: * /ul
Yes, just failed to delete it when I added it there.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageFormVisitor.java#newcode63
user/src/com/google/gwt/i18n/server/MessageFormVisitor.java:63: * {@link
#processDefaultMessage(MessageStyle, String)}.
On 2011/03/03 01:07:39, rjrjr wrote:
You're talking about the default message, but it's not used in this
interface at
all.
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java
File user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode27
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:27: *
{@code mv = miv.visitMessage(msg, msgTrans);}
On 2011/03/03 01:07:39, rjrjr wrote:
{@code} is redundant with pre
If you show the types of these visitors (declare them), it would be
easier to
see what the types are. e.g.
{@link MessageVisitor} mv = miv.visitMessage(msg, msgTrans);
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode62
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:62: *
{@code miv.endClass(msgIntf);}
On 2011/03/03 01:07:39, rjrjr wrote:
Don't forget to close that pre section
Done.
I still think the MessageInterfaceVisitor adds noise, not value.
Look at this from
AbstractLocalizableImplCreator.generateToMsgCatFactory(),
which uses MessageCatalogWriter, which has the only implementation of
this
interface.
catWriter = msgCatFactory.getWriter(ctx, catalogName);
msgIntf.accept(catWriter.visitClass());
As a client of the writer I have to know about visitors and the accept
method,
where those could easily be implementation details. It's not as easy
to
understand as:
catWriter = msgCatFactory.getWriter(ctx, catalogName);
catWriter.write(msgIntf);
That removes the ability to visit multiple classes with the same visitor
(admittedly not used at present).
How about retaining the existing API, but
[gwt-contrib] Re: Adding a constructor overload to CellTable that takes a loading indicator widget. For legacy sup... (issue1371805)
LGTM Rietveld seems to be ignoring the binary file. Saw it offline, nice improvement On Thu, Mar 3, 2011 at 9:55 AM, rj...@google.com wrote: Is the new one in a separate patch? I don't see it here. http://gwt-code-reviews.appspot.com/1371805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use RequestContext-local AutoBeanFactory. (issue1368805)
LGTM
On Thu, Mar 3, 2011 at 10:52 AM, b...@google.com wrote:
Reviewers: rjrjr,
Description:
Use RequestContext-local AutoBeanFactory.
Fix unmade change due to branch merge problem.
Patch by: bobv
Review by: rjrjr
Please review this at http://gwt-code-reviews.appspot.com/1368805/
Affected files:
M
user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java
Index:
user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java
===
---
user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java
(revision 9792)
+++
user/src/com/google/gwt/requestfactory/shared/impl/AbstractRequestContext.java
(working copy)
@@ -180,8 +180,8 @@
if (obj == null) {
return LazySplittable.NULL;
} else if (obj.getClass().isEnum()
- getRequestFactory().getAutoBeanFactory() instanceof EnumMap)
{
-value = ValueCodex.encode(((EnumMap)
getRequestFactory().getAutoBeanFactory()).getToken((Enum?) obj));
+ getAutoBeanFactory() instanceof EnumMap) {
+value = ValueCodex.encode(((EnumMap)
getAutoBeanFactory()).getToken((Enum?) obj));
} else if (ValueCodex.canDecode(obj.getClass())) {
value = ValueCodex.encode(obj);
} else {
@@ -525,7 +525,7 @@
if (previous == null) {
// Compare to empty object
Class? proxyClass = stableId(bean).getProxyClass();
-previous =
getRequestFactory().getAutoBeanFactory().create(proxyClass);
+previous = getAutoBeanFactory().create(proxyClass);
}
if (!AutoBeanUtils.diff(previous, bean).isEmpty()) {
return true;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Increase Stack Size for ant tests, to prevent test failures (address issue 6100) (issue1369806)
LGTM
On Thu, Mar 3, 2011 at 11:03 AM, jbrosenb...@google.com wrote:
Reviewers: rjrjr, kjin,
Description:
Increase Stack Size for ant tests, to prevent test failures (address
issue 6100)
Please review this at http://gwt-code-reviews.appspot.com/1369806/
Affected files:
M common.ant.xml
Index: common.ant.xml
===
--- common.ant.xml (revision 9792)
+++ common.ant.xml (working copy)
@@ -210,7 +210,7 @@
junit dir=@{test.out} fork=yes printsummary=yes
failureproperty=junit.failure tempdir=@{test.out}
jvmarg line=-Xmx768m /
-jvmarg line=-Xss512k /
+jvmarg line=-Xss4M /
jvmarg
value=-Demma.coverage.out.file=@{test.emma.coverage}/coverage.emma /
jvmarg value=-Demma.coverage.out.merge=true /
jvmarg value=-Dcom.google.gwt.junit.reportPath=reports /
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added support for a -quirksMode flag to GWTTestCase (via the gwt.args system (issue1374802)
LGTM On Thu, Mar 3, 2011 at 12:17 PM, skybr...@google.com wrote: http://gwt-code-reviews.appspot.com/1374802/diff/1/user/src/com/google/gwt/junit/JUnitShell.java File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/1374802/diff/1/user/src/com/google/gwt/junit/JUnitShell.java#newcode163 user/src/com/google/gwt/junit/JUnitShell.java:163: options.setServletContainerLauncher(shell.new MyJettyLauncher()); On 2011/03/03 18:29:54, rjrjr wrote: Wow. I've never seen that shell.new thing before, or at least I'd forgotten about it. Yeah, I had forgotten about it too, but it's in several other places in this file, which could certainly use some cleanup. I think these could all be fixed by making inner classes static. But this seemed like enough refactoring for one CL. http://gwt-code-reviews.appspot.com/1374802/diff/1/user/src/com/google/gwt/junit/JUnitShell.java#newcode1102 user/src/com/google/gwt/junit/JUnitShell.java:1102: // CHECKSTYLE_OFF On 2011/03/03 18:29:54, rjrjr wrote: what is checkstyle complaining about? Don't know; I just copied it. Maybe the += on a string? Removed, and it seemed to pass. http://gwt-code-reviews.appspot.com/1374802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: More autoformatter tweaks. Allow wrapping on assignments, do not (issue1371802)
On Wed, Mar 2, 2011 at 6:41 AM, zun...@google.com wrote: LGTM: oops, looks like I never sent this comment http://gwt-code-reviews.appspot.com/1371802/diff/1/eclipse/settings/code-style/gwt-format.xml File eclipse/settings/code-style/gwt-format.xml (right): http://gwt-code-reviews.appspot.com/1371802/diff/1/eclipse/settings/code-style/gwt-format.xml#newcode266 eclipse/settings/code-style/gwt-format.xml:266: setting id=org.eclipse.jdt.core.formatter.lineSplit value=100/ I support this change to 100 character line lengths, but note that it will be a change to the public GWT style guide. http://code.google.com/webtoolkit/makinggwtbetter.html#codestyle Already updated. Anyone using the autoformatter will likely reformat a lot of code for existing files. Again, I support it, I just want everyone to go in with that in mind. I'd be willing to go through and autoformat large swaths of code once we consider this change stable if we can get enough buy in from the rest of the team. http://gwt-code-reviews.appspot.com/1371802/ That would be generous of you. I can't imagine anyone complaining. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] ListEditorWrapper frozen autobeans
Patrick, did you file an issue about this?
On Tue, Feb 22, 2011 at 7:15 AM, Patrick Julien pjul...@gmail.com wrote:
http://gwt-code-reviews.appspot.com/1159801/show
Introduces an autobean is frozen issue. The problem is that the
workingCopy variable that is introduced doesn't cause autobeans to
be unfrozen. The scenario that I have is pretty simple:
public class XEditor extends composite implements EditorSomeProxy {
private SomeChildTable pathToChildren;
}
SomeChildTable has a CellTable which uses the HasDataEditor wrapper.
If I remove workingCopy, the various FieldUpdater inside the table are
fine. If I don't, I get an autobeans is frozen exception. I
believe this is because workingCopy is a regular ArrayListT instead
of the generated RequestFactory list wrapper type
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Future of CellTable
Do you mean patches for CellTable? It's part of GWT, so the usual way: http://code.google.com/webtoolkit/makinggwtbetter.html On Fri, Feb 18, 2011 at 3:30 AM, dflorey daniel.flo...@gmail.com wrote: Thanks for the info! I guess I'll wait until 2.3 and will start to port the TreeTable + filter stuff. Is there a way to contribute patches since the incubator is deprecated? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Added validation jars to 'devmode' and 'test.dev' targets in ant webAppCreator templates (issue1352807)
LGTM On Mon, Feb 14, 2011 at 8:34 AM, rchan...@google.com wrote: Reviewers: rjrjr, Nick Chalko, Description: Added validation jars to 'devmode' and 'test.dev' targets in ant webAppCreator templates Fixes Issue 5950. Please review this at http://gwt-code-reviews.appspot.com/1352807/show Affected files: M user/src/com/google/gwt/user/tools/project.ant.xmlsrc Index: user/src/com/google/gwt/user/tools/project.ant.xmlsrc === --- user/src/com/google/gwt/user/tools/project.ant.xmlsrc (revision 9731) +++ user/src/com/google/gwt/user/tools/project.ant.xmlsrc (working copy) @@ -57,6 +57,8 @@ classpath pathelement location=@srcFolder/ path refid=project.class.path/ +pathelement location=@gwtValidationPath / +pathelement location=@gwtValidationSourcesPath / /classpath jvmarg value=-Xmx256M/ arg value=-startupUrl/ @@ -89,6 +91,8 @@ pathelement location=@srcFolder / pathelement location=@testFolder / path refid=project.class.path / +pathelement location=@gwtValidationPath / +pathelement location=@gwtValidationSourcesPath / pathelement location=@junitJar / /classpath batchtest todir=reports/htmlunit.dev @@ -111,6 +115,8 @@ pathelement location=@srcFolder / pathelement location=@testFolder / path refid=project.class.path / +pathelement location=@gwtValidationPath / +pathelement location=@gwtValidationSourcesPath / pathelement location=@junitJar / /classpath batchtest todir=reports/htmlunit.prod -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added missing validation jars to gwt-user.jar. Fixes Issue 5950. (issue1323803)
I'm confused by your last comment, I guess we need to decide..., which points back to this issue. Who are you asking to choose between what? On Fri, Feb 11, 2011 at 8:38 AM, rchan...@google.com wrote: ping On 2011/02/09 15:22:04, rchandia wrote: Removed the hibernate validation implementation as it is not necessary. I guess we need to decide between adding stuff to gwt-user.jar with this approach or to add the respective jars to each gwtc invocation as in: http://gwt-code-reviews.appspot.com/1342803/show On 2011/02/09 15:19:17, rchandia wrote: http://gwt-code-reviews.appspot.com/1323803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Eclipse autoformatter update: allow assignments to wrap. (issue1354803)
I can't find a way to weight the line breaks, to make it resort to assignments last. I'll drop this one. On Fri, Feb 11, 2011 at 10:57 AM, John Tamplin j...@google.com wrote: On Fri, Feb 11, 2011 at 1:51 PM, zun...@google.com wrote: Well, I personally like the way it looks: this.myPackage = StringInterner.get().intern( (myPackage.length() == 0) ? : (myPackage + '.')); becomes: this.myPackage = StringInterner.get().intern( (myPackage.length() == 0) ? : (myPackage + '.')); This is going to cause a lot of churn... I would prefer wrapping assignments only when it is necessary to fit on the line. Ie, this example, the first version is better (and has the advantage of being the way the source is now). The real problem with assignments is when they are really long, such as field initializers, and Eclipse won't break them at all. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added missing validation jars to gwt-user.jar. Fixes Issue 5950. (issue1323803)
If Dave has already LGTM'd one of the approaches as maven friendly, is there any reason not to go with that? What are the trade offs? On Fri, Feb 11, 2011 at 10:39 AM, Rodrigo Chandia rchan...@google.comwrote: El 11 de febrero de 2011 13:19, Nick Chalko ncha...@google.com escribió: On Fri, Feb 11, 2011 at 10:11 AM, Rodrigo Chandia rchan...@google.comwrote: Oh, sorry. I guess I pasted confusingly similar links. So far there are two approaches to fix Issue 5950: http://gwt-code-reviews.appspot.com/1323803/show (This one) http://gwt-code-reviews.appspot.com/1342803/show (The other one) So adding to gwt-user seems like the simplest approach Correct, but doing so could cause problems for someone trying to use their own validation classes (say version 1.0.1-GA with a small bug fix), right? That is why Ray commented above that this approach could bring Maven wrath upon us. I tried our expenses sample with Maven and Eclipse, but that as far as I can go with Maven given my lack of Maven-fu (white belt still). -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added missing validation jars to gwt-user.jar. Fixes Issue 5950. (issue1323803)
I was *convinced* those two numbers were the same. Dueling 803's! I agree, the de-bundled one smells a lot better. LGTM'd it. On Fri, Feb 11, 2011 at 12:11 PM, Rodrigo Chandia rchan...@google.comwrote: El 11 de febrero de 2011 14:48, Ray Ryan rj...@google.com escribió: If Dave has already LGTM'd one of the approaches as maven friendly, is there any reason not to go with that? What are the trade offs? No LGTM from David Chandler yet. But aside from that, from what David and I have discussed, both approaches are Maven friendly. The only remaining points of discussion seem to be maintainability and possible (yet unknown) conflicts with external versions of the validation API jars. Bundling the classes as done here forces every GWT user to compile with our version of javax.validation (so far the only one AFICT, and the user can still use any implementation on the server). The other Issue (1342803) touches more places in our ant build, but keep the validation jars separate and thus allow our users to easily choose to use other validation APIs (as long as the API is backwards compatible with validation-api-1.0.0.GA). I personally believe in modularity and de-bundling, so I lean toward the later option (Issue 1342803). In any case I'd be happy with either an LGTM here (issue1323803) from David or an unopposed LGTM in issue1342803. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Follow on to svn r8671, which made the GWT code style allow whitespace (issue1346803)
The intention was all but the first call on its own line, and was not very
well thought out — I had builders on the brain. There is another option to
have it wrap when it needs to, which we have turned off at the moment. I'll
set it to that and do more spot checks.
On Wed, Feb 9, 2011 at 7:56 AM, Eric Ayers zun...@google.com wrote:
Was the intention to require all expressions to be put on a new line,
or only when the dots are preceeded by whitespace?
On Wed, Feb 9, 2011 at 10:48 AM, Ray Ryan rj...@google.com wrote:
That's pretty bad. I'll tweak. Even if we can't have perfect builders we
can
at least wrap long lines better.
On Feb 9, 2011 7:43 AM, zun...@google.com wrote:
I did a spot check. There is one odd thing that happens with the new
settings on eclipse 3.5 (found in JsCatchScope)
@Override
protected JsName findExistingNameNoRecurse(String ident) {
if (name.getIdent().equals(ident)) {
return name;
}
}
becomes
@Override
protected JsName findExistingNameNoRecurse(String ident) {
if (name.getIdent()
.equals(ident)) {
return name;
}
}
http://gwt-code-reviews.appspot.com/1346803/show
--
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make UiBinder accept IsWidget subinterfaces properly (issue1295806)
On Wed, Feb 9, 2011 at 7:54 AM, jlaba...@google.com wrote: http://gwt-code-reviews.appspot.com/1295806/diff/1/10 File user/src/com/google/gwt/user/client/ui/HTMLTable.java (right): http://gwt-code-reviews.appspot.com/1295806/diff/1/10#newcode1105 user/src/com/google/gwt/user/client/ui/HTMLTable.java:1105: Element td = cleanCell(row, column, true); I guess the answer is... probably. The question is, what should the code below do? table.setText(0, 0, hello world); table.setWidget(0, 0, null); If you think the call to setWidget should clear the text hello world (and I think that makes sense), then yes, cleanCell() should be outside of the conditional. If you think hello world should remain, then no change is needed. Assuming we want to change it, we should add a test case. Do you want me to take care of it? Love those breaking changes for correctness. :-/ This is why lines that start if (foo == null) should require papal dispensation. // null check cert #38828 granted by His Holiness Pope John Paul Secondus 19 Jan 2005 What would happen in this case? table.setWidget(0, 0, new Label(Dude!)); table.setWidget(0, 0, null); If the answer is that the widget will just stay there, it seems like we should make the fix, even on the chance it will cause some surprises for code relying on the unfriendly friendliness. And yes, if you could make that change it would be very kind of you. http://gwt-code-reviews.appspot.com/1295806/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Follow on to svn r8671, which made the GWT code style allow whitespace (issue1346803)
Okay, ready for re-review. Less ambitious now. Allows foo.bar().baz().bang().imagineManyOfThese(hi mom); to wrap as: foo.bar().baz().bang() .imagineManyOfThese(hi mom); instead of what happens now: foo.bar().baz().bang().imagineManyOfThese( hi mom); On Wed, Feb 9, 2011 at 11:21 AM, rj...@google.com wrote: http://gwt-code-reviews.appspot.com/1346803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Follow on to svn r8671, which made the GWT code style allow whitespace (issue1346803)
I did the spot check that John suggested. Most files stay more or less in tact. The ones that do change look a lot more readable in the new style, IMHO. E.g., try using this style on RequestFactoryTest (Bob, you in particular might want to weigh in here). On Wed, Feb 9, 2011 at 12:36 PM, zun...@google.com wrote: LGTM http://gwt-code-reviews.appspot.com/1346803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Added validation jars to all calls to GWTC to provide classes rquired by (issue1342803)
webAppCreator generates a pom.xml? When did that start happening? On Tue, Feb 8, 2011 at 9:28 AM, David Chandler drfibona...@google.comwrote: Just this in the POM. These are required to use RequestFactory, but not required otherwise, so we should probably note that in the POM, too. dependency groupIdorg.json/groupId artifactIdjson/artifactId version20090211/version /dependency dependency groupIdjavax.validation/groupId artifactIdvalidation-api/artifactId version1.0.0.GA/version /dependency dependency groupIdorg.hibernate/groupId artifactIdhibernate-validator/artifactId version4.0.2.GA/version exclusions exclusion groupIdjavax.xml.bind/groupId artifactIdjaxb-api/artifactId /exclusion exclusion groupIdcom.sun.xml.bind/groupId artifactIdjaxb-impl/artifactId /exclusion /exclusions /dependency On Tue, Feb 8, 2011 at 11:48 AM, rchan...@google.com wrote: On 2011/02/07 23:01:25, drfibonacci wrote: From a maven perspective, this is a much better solution than bundling validation classes in gwt-user.jar. However, will the changes to .classpathsrc and/or the gwtc ant target affect the GWT compiler when called from the maven plugin? Nothing in this change modifies the pom.xml produced by webAppCreator. What would we need to add the validation classes and sources to the pom.xml? http://gwt-code-reviews.appspot.com/1342803/show -- David Chandler Developer Programs Engineer, Google Web Toolkit w: http://code.google.com/ b: http://googlewebtoolkit.blogspot.com/ t: @googledevtools -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added validation jars to all calls to GWTC to provide classes rquired by (issue1342803)
Neato. Don't you need to make the same changes to samples/expenses/pom.xml? On Tue, Feb 8, 2011 at 10:55 AM, rchan...@google.com wrote: On 2011/02/08 18:36:31, rjrjr wrote: webAppCreator generates a pom.xml? When did that start happening? Somewhere before GWT 2.1 for Google I/O 2010. Added at revision r8234: http://code.google.com/p/google-web-toolkit/source/detail?spec=svn9690r=8234 http://gwt-code-reviews.appspot.com/1342803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Announcing GPE/GWT 2.2 RC1
It's not so much about missing features (although it will be nice to be allowed to use and emulate features that were added to the language more than four years ago). It's more about being in step with the rest of Google's code base. Having to keep an eye out for 1.6'ism creeping in has been a non-negligible tax on the GWT team, and it's one we would like to stop paying. rjrjr On Mon, Feb 7, 2011 at 11:47 PM, stuckagain david.no...@gmail.com wrote: What features in Java 6 would be so fundamental to GWT that 1.5 becomes deprecated ? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] RequestFactory - EntityProxy AutoBean Category
You can't do these things yet, but we've been discussing them. On Thu, Feb 3, 2011 at 5:08 AM, Krishna krishnacal...@gmail.com wrote: Hi, Does RequestFactory EntityProxy supports AutoBean Category (http:// code.google.com/p/google-web-toolkit/wiki/AutoBean#Categories) ? i. e. Can I define a Category for an EnitityProxy? If yes, how can I do that? It would be nice to define a category direct as an annotated Entity method... If the method requires only getters/setters and other category methods. I know its not easy to restrict. Is it hard to allow Proxy as an abstract class? Thanks! Krishna Caldas -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a new DeckLayoutPanel that displays one of many widgets and transitions between them usin... (issue1340803)
Re: forcing layout, could that turn into a source of slowness in apps, where
we force recalculation that turns out to be redundant?
On Feb 4, 2011 6:41 AM, jlaba...@google.com wrote:
I updated DeckLayoutPanel to implement AcceptsOneWidget, and I moved the
animationDuration down from TabLayoutPanel into DeckLayoutPanel so that
setWidget() will automatically use the default animation time.
I went with AcceptsOneWidget instead of HasOneWidget because its
compatible with ActivityManager, and because I didn't want to add
DeckLayoutPanel.getWidget() (getVisibleWidget is more clear).
Also, setWidget(null) clears the current widget. That seems more
natural than doing a no-op.
http://gwt-code-reviews.appspot.com/1340803/diff/1/12
File user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java
(right):
http://gwt-code-reviews.appspot.com/1340803/diff/1/12#newcode28
user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java:28: * {@link
com.google.gwt.user.client.ui.TabPanel}.
On 2011/02/02 18:41:20, sbrubaker wrote:
Do you mean TabPanel or TabLayoutPanel?
Done.
http://gwt-code-reviews.appspot.com/1340803/diff/1/21
File user/test/com/google/gwt/user/client/ui/DeckLayoutPanelTest.java
(right):
http://gwt-code-reviews.appspot.com/1340803/diff/1/21#newcode32
user/test/com/google/gwt/user/client/ui/DeckLayoutPanelTest.java:32: //
Show widget at index 1, make sure it becomes visible.
On 2011/02/02 18:41:20, sbrubaker wrote:
You may want to note that you're testing both forms of showWidget
(here and
below).
Done.
http://gwt-code-reviews.appspot.com/1340803/diff/1/22
File user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java
(right):
http://gwt-code-reviews.appspot.com/1340803/diff/1/22#newcode269
user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java:269:
assertEquals(inserted text,
Rietveld has weird highlighting. There aren't actually any spaces after
the comma.
http://gwt-code-reviews.appspot.com/1340803/diff/1/22#newcode271
user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java:271:
assertEquals(added text,
same
http://gwt-code-reviews.appspot.com/1340803/diff/1/22#newcode416
user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java:416:
p.forceLayout();
If we don't force layout (synchronously), then it happens in a finally
command (asynchronously) after the current event loop, which would
require me to turn this into an asynchronous test with a bunch of nested
DeferredCommands.
http://gwt-code-reviews.appspot.com/1340803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a new DeckLayoutPanel that displays one of many widgets and transitions between them usin... (issue1340803)
D'oh. Really should look at the code before wading in like that.
On Feb 4, 2011 7:23 AM, John LaBanca jlaba...@google.com wrote:
I was using it in a test case. It isn't required or recommended for the
widget in general.
Thanks,
John LaBanca
jlaba...@google.com
On Fri, Feb 4, 2011 at 10:18 AM, Ray Ryan rj...@google.com wrote:
Re: forcing layout, could that turn into a source of slowness in apps,
where we force recalculation that turns out to be redundant?
On Feb 4, 2011 6:41 AM, jlaba...@google.com wrote:
I updated DeckLayoutPanel to implement AcceptsOneWidget, and I moved
the
animationDuration down from TabLayoutPanel into DeckLayoutPanel so that
setWidget() will automatically use the default animation time.
I went with AcceptsOneWidget instead of HasOneWidget because its
compatible with ActivityManager, and because I didn't want to add
DeckLayoutPanel.getWidget() (getVisibleWidget is more clear).
Also, setWidget(null) clears the current widget. That seems more
natural than doing a no-op.
http://gwt-code-reviews.appspot.com/1340803/diff/1/12
File user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java
(right):
http://gwt-code-reviews.appspot.com/1340803/diff/1/12#newcode28
user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java:28: *
{@link
com.google.gwt.user.client.ui.TabPanel}.
On 2011/02/02 18:41:20, sbrubaker wrote:
Do you mean TabPanel or TabLayoutPanel?
Done.
http://gwt-code-reviews.appspot.com/1340803/diff/1/21
File user/test/com/google/gwt/user/client/ui/DeckLayoutPanelTest.java
(right):
http://gwt-code-reviews.appspot.com/1340803/diff/1/21#newcode32
user/test/com/google/gwt/user/client/ui/DeckLayoutPanelTest.java:32: //
Show widget at index 1, make sure it becomes visible.
On 2011/02/02 18:41:20, sbrubaker wrote:
You may want to note that you're testing both forms of showWidget
(here and
below).
Done.
http://gwt-code-reviews.appspot.com/1340803/diff/1/22
File user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java
(right):
http://gwt-code-reviews.appspot.com/1340803/diff/1/22#newcode269
user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java:269:
assertEquals(inserted text,
Rietveld has weird highlighting. There aren't actually any spaces after
the comma.
http://gwt-code-reviews.appspot.com/1340803/diff/1/22#newcode271
user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java:271:
assertEquals(added text,
same
http://gwt-code-reviews.appspot.com/1340803/diff/1/22#newcode416
user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java:416:
p.forceLayout();
If we don't force layout (synchronously), then it happens in a finally
command (asynchronously) after the current event loop, which would
require me to turn this into an asynchronous test with a bunch of
nested
DeferredCommands.
http://gwt-code-reviews.appspot.com/1340803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Overhaul Editor framework traversal logic to use a visitor pattern. (issue1340802)
LGTM
On Tue, Feb 1, 2011 at 6:40 PM, b...@google.com wrote:
Patch updated.
http://gwt-code-reviews.appspot.com/1340802/diff/1/2
File user/src/com/google/gwt/editor/client/EditorContext.java (right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/2#newcode114
user/src/com/google/gwt/editor/client/EditorContext.java:114: * Traverse
a editor created by
On 2011/02/01 18:52:05, rjrjr wrote:
an editor
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/4
File user/src/com/google/gwt/editor/client/EditorVisitor.java (right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/4#newcode27
user/src/com/google/gwt/editor/client/EditorVisitor.java:27: public T
void endVisit(EditorContextT ctx) {
Since EditorContext allows access to the EditorT it's useful to be
able to say
editorContext.asLeafValueEditor().setValue(editorConext.getFromModel());
and have the types line up.
http://gwt-code-reviews.appspot.com/1340802/diff/1/8
File
user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java
(right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/8#newcode57
user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java:57:
Chain(CompositeEditorT, R, S composedEditor, ClassR
composedElementType) {
On 2011/02/01 18:52:05, rjrjr wrote:
line too long
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/8#newcode118
user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java:118:
public R, S extends EditorR void createChain(ClassR
composedElementType) {
On 2011/02/01 18:52:05, rjrjr wrote:
Should this be protected?
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/15
File user/src/com/google/gwt/editor/client/impl/Initializer.java
(right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/15#newcode35
user/src/com/google/gwt/editor/client/impl/Initializer.java:35: // Tell
the EditorDelegate about the object its responsible for
On 2011/02/01 18:52:05, rjrjr wrote:
it's
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/15#newcode42
user/src/com/google/gwt/editor/client/impl/Initializer.java:42:
HasEditorDelegateQ asDelegate = ctx.asHasEditorDelegate();
On 2011/02/01 18:52:05, rjrjr wrote:
asHasDelegate
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/16
File user/src/com/google/gwt/editor/client/impl/Refresher.java (right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/16#newcode25
user/src/com/google/gwt/editor/client/impl/Refresher.java:25: * the
editor and delegate hiererchy.
On 2011/02/01 18:52:05, rjrjr wrote:
Perhaps you could simplify initializer, and require that it be used in
tandem
with refresher? Or make it wrap a refresher? Get rid of some
redundancy?
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/17
File user/src/com/google/gwt/editor/client/impl/RootEditorContext.java
(right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/17#newcode42
user/src/com/google/gwt/editor/client/impl/RootEditorContext.java:42:
public T checkAssignment(Object value) {
On 2011/02/01 18:52:05, rjrjr wrote:
Document as unsupported?
Changed to always return value, relying on code-gen in the
implementation of AED.setObject() to throw a ClassCastException.
Documented reasoning.
http://gwt-code-reviews.appspot.com/1340802/diff/1/24
File
user/src/com/google/gwt/editor/rebind/AbstractEditorDriverGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/24#newcode44
user/src/com/google/gwt/editor/rebind/AbstractEditorDriverGenerator.java:44:
public abstract class AbstractEditorDriverGenerator extends Generator {
On 2011/02/01 18:52:05, rjrjr wrote:
Here's the punchline, eh? Nice to see all that red in the diff.
What code-gen remains has no conditional expressions. It's basically a
straight-through transform applied to the editor model data.
http://gwt-code-reviews.appspot.com/1340802/diff/1/29
File
user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryEditorDelegate.java
(right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/29#newcode86
user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryEditorDelegate.java:86:
* Must call four-arg version instead.
On 2011/02/01 18:52:05, rjrjr wrote:
Are you sure you want the base initialize method to be public, then?
Done.
http://gwt-code-reviews.appspot.com/1340802/diff/1/36
File user/test/com/google/gwt/editor/client/EditorErrorTest.java
(right):
http://gwt-code-reviews.appspot.com/1340802/diff/1/36#newcode137
user/test/com/google/gwt/editor/client/EditorErrorTest.java:137:
assertEquals(errors.toString(), 2, errors.size());
On 2011/02/01 18:52:05, rjrjr wrote:
That's going to be pretty cryptic when it fails
Done.
http://gwt-code-reviews.appspot.com/1340802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added missing validation jars to gwt-user.jar. Fixes Issue 5950. (issue1323803)
Wait, I'm not sure we can do that. Dave Chandler, is this the kind of thing
that upset the maven community last time around?
On Fri, Jan 28, 2011 at 10:55 AM, rchan...@google.com wrote:
Reviewers: rjrjr, Nick Chalko,
Description:
Added missing validation jars to gwt-user.jar. Fixes Issue 5950.
Please review this at http://gwt-code-reviews.appspot.com/1323803/show
Affected files:
M user/build.xml
Index: user/build.xml
===
--- user/build.xml (revision 9650)
+++ user/build.xml (working copy)
@@ -148,6 +148,12 @@
zipfileset src=${gwt.tools.lib}/tomcat/servlet-api-2.5.jar
excludes=**/*.java/
zipfileset src=${gwt.tools.lib}/w3c/sac/sac-1.3.jar /
zipfileset src=${gwt.tools.lib}/w3c/flute/flute-1.3-gg1.jar /
+ zipfileset
src=${gwt.tools.lib}/javax/validation/validation-api-1.0.0.GA.jar /
+ !-- The source is included so validation is available from client
code --
+ zipfileset
src=${gwt.tools.lib}/javax/validation/validation-api-1.0.0.GA-sources.jar
/
+ !-- Hibernate is included until we can provide the super source as
an third party jar --
+ zipfileset
src=${gwt.tools.lib}/hibernate/validator/hibernate-validator-4.1.0.Final.jar
/
+ zipfileset
src=${gwt.tools.lib}/hibernate/validator/hibernate-validator-4.1.0.Final-sources.jar
/
/gwt.jar
/target
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added missing validation jars to gwt-user.jar. Fixes Issue 5950. (issue1323803)
Looking at it more closely, there are two parts. You're including
javax/validation/validation-api-1.0.0.GA-sources.jar
for the validation api, right? Seems like we tried that before and got push
back. So, @Chandler, if this is not the right way to make these standard
interfaces available to client code, what is?
The other two jars you describe as an interim step. How short an interim are
we talking about? The answer has to be before 2.3 is cut.
On Fri, Jan 28, 2011 at 11:04 AM, Ray Ryan rj...@google.com wrote:
Wait, I'm not sure we can do that. Dave Chandler, is this the kind of thing
that upset the maven community last time around?
On Fri, Jan 28, 2011 at 10:55 AM, rchan...@google.com wrote:
Reviewers: rjrjr, Nick Chalko,
Description:
Added missing validation jars to gwt-user.jar. Fixes Issue 5950.
Please review this at http://gwt-code-reviews.appspot.com/1323803/show
Affected files:
M user/build.xml
Index: user/build.xml
===
--- user/build.xml (revision 9650)
+++ user/build.xml (working copy)
@@ -148,6 +148,12 @@
zipfileset src=${gwt.tools.lib}/tomcat/servlet-api-2.5.jar
excludes=**/*.java/
zipfileset src=${gwt.tools.lib}/w3c/sac/sac-1.3.jar /
zipfileset src=${gwt.tools.lib}/w3c/flute/flute-1.3-gg1.jar /
+ zipfileset
src=${gwt.tools.lib}/javax/validation/validation-api-1.0.0.GA.jar /
+ !-- The source is included so validation is available from client
code --
+ zipfileset
src=${gwt.tools.lib}/javax/validation/validation-api-1.0.0.GA-sources.jar
/
+ !-- Hibernate is included until we can provide the super source as
an third party jar --
+ zipfileset
src=${gwt.tools.lib}/hibernate/validator/hibernate-validator-4.1.0.Final.jar
/
+ zipfileset
src=${gwt.tools.lib}/hibernate/validator/hibernate-validator-4.1.0.Final-sources.jar
/
/gwt.jar
/target
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: AbstractPlaceHistoryMapper doesn't process tokens correctly wrt empty prefixes (issue1316801)
Oops, PlaceHistoryGeneratorContextTest is failing: java.lang.NullPointerException at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.getPrefixForTokenizerType(PlaceHistoryGeneratorContext.java:269) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.initTokenizersWithoutGetters(PlaceHistoryGeneratorContext.java:330) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.ensureInitialized(PlaceHistoryGeneratorContext.java:189) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.getPrefixes(PlaceHistoryGeneratorContext.java:160) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContextTest.testNoFactory(PlaceHistoryGeneratorContextTest.java:141) testWithFactory error in 0.024s java.lang.NullPointerException at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.getPrefixForTokenizerType(PlaceHistoryGeneratorContext.java:269) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.initTokenizersWithoutGetters(PlaceHistoryGeneratorContext.java:330) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.ensureInitialized(PlaceHistoryGeneratorContext.java:189) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContext.getPrefixes(PlaceHistoryGeneratorContext.java:160) at com.google.gwt.place.rebind.PlaceHistoryGeneratorContextTest.testWithFactory(PlaceHistoryGeneratorContextTest.java:187) On Mon, Jan 24, 2011 at 5:34 PM, rj...@google.com wrote: LGTM, submitting http://gwt-code-reviews.appspot.com/1316801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Optimize redundant 'switch' statements (issue1311801)
I think you could reuse the original rietveld issue, if you haven't closed it yet. On Thu, Jan 20, 2011 at 1:17 PM, Daniel Rice (דניאל רייס) r...@google.comwrote: Here's a manual diff. Is there some slick way to upload it to Mondrian or Rietveld that won't make them confused? Dan On Thu, Jan 20, 2011 at 3:52 PM, sco...@google.com wrote: Can you upload a diff relative to what you checked in before you rolled it back? I just want to see what you fixed relative to that. http://gwt-code-reviews.appspot.com/1311801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
Submitted at r9582 On Thu, Jan 20, 2011 at 6:36 PM, larse...@gmail.com wrote: Awesome, thanks for getting this committed. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for mapping ConstraintViolation objects into SimpleBeanEditor. (issue1260801)
Bob, did this land? On Mon, Jan 10, 2011 at 2:45 PM, ncha...@google.com wrote: LGTM http://gwt-code-reviews.appspot.com/1260801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
I take your point about UiBinder support. You can have your invariant and bind it too by updating DialogBoxParser (and DialogBoxParserTest) to optionally handle the new constructor argument. On Tue, Jan 18, 2011 at 8:02 AM, jlaba...@google.com wrote: Also, can you sign a CLA so we can accept patch. If you scroll down to the bottom of the link below, you can sign it electronically. http://code.google.com/legal/individual-cla-v1.0.html http://gwt-code-reviews.appspot.com/1149803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
On Tue, Jan 18, 2011 at 10:29 AM, larse...@gmail.com wrote: http://gwt-code-reviews.appspot.com/1149803/show Thanks so much for reviewing this guys. @Ray, Do you want me to go back to allowing a setter for the caption? I certainly wouldn't want to see both the setter and the constructor. If you want to go for the setter after all, John will need to scrutinize the code that orphans the previous header, and support for setHeader(null), including unit tests — I'll screw it up. And you'll want to give it an @UiChild annotation. If you want to stick with the constructor, the changes to DialogBoxParser and DialogBoxParserTest to make it UiBinder friendly are pretty simple. And if I get to pick, yes, a full blown setter-based implementation is preferable. I'm just trying not to ask for the moon. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
On Tue, Jan 18, 2011 at 11:53 AM, p...@google.com wrote:
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002#newcode42
user/src/com/google/gwt/canvas/client/Canvas.java:42: public static
Canvas createCanvasIfSupported() {
On 2011/01/18 17:46:05, rjrjr wrote:
Naming nit: use of these factory methods might be easier to automate
down the
road if they are consistent, so I'd suggest
Canvas#createIfSupported(). DRY and
all that.
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode45
user/src/com/google/gwt/canvas/client/Canvas.java:45: return null;
On 2011/01/18 18:55:17, rjrjr wrote:
Duplicate code, should call isSupported instead.
I was trying to provide a way for developers to avoid creating two
Canvas elements by re-using the one created during the check for the
actual Canvas creation (calling isSupported() requires creating a Canvas
element). Think that's too hacky?
Oh, I didn't notice that. Objection withdrawn.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode174
user/src/com/google/gwt/canvas/client/Canvas.java:174: }
On 2011/01/18 18:55:17, rjrjr wrote:
Can this be private?
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003#newcode23
user/src/com/google/gwt/dom/client/PartialSupport.java:23: * supported
at runtime.
On 2011/01/18 18:55:17, rjrjr wrote:
What about the factory method? Even if it's not universally needed,
you can
document how it will appear when it's appropriate.
Improved the javadoc two include both methods. Does the new wording
LGTY?
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005
File user/test/com/google/gwt/canvas/client/CanvasTest.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005#newcode33
user/test/com/google/gwt/canvas/client/CanvasTest.java:33: public class
CanvasTest extends GWTTestCase {
On 2011/01/18 18:55:17, rjrjr wrote:
Doesn't test isSupported.
Done.
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move the validation test cases that depend on reflection out of a client-scoped package. (issue1276801)
They are, Rietveld is just lame On Tue, Jan 11, 2011 at 11:09 AM, ncha...@google.com wrote: LGTM, except these should be moves not adds, to preserve history. http://gwt-code-reviews.appspot.com/1276801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Issue 5549: fix is/has support for boolean properties (issue1272801)
Do similar changes need to be made in Editor, and are you up for that?
Bob, are you able to take this review?
On Sun, Jan 9, 2011 at 5:01 AM, t.bro...@gmail.com wrote:
http://gwt-code-reviews.appspot.com/1272801/diff/1/3
File user/src/com/google/gwt/autobean/server/BeanMethod.java (right):
http://gwt-code-reviews.appspot.com/1272801/diff/1/3#newcode52
user/src/com/google/gwt/autobean/server/BeanMethod.java:52: String
inferName(Method method) {
Similar to AutoBeanMethod but using Java reflection. Also, contrary to
AutoBeanMethod, it doesn't decapitalize the name.
Note that actually only the GET.inferName is ever called.
http://gwt-code-reviews.appspot.com/1272801/diff/1/6
File user/src/com/google/gwt/autobean/server/ProxyAutoBean.java (right):
http://gwt-code-reviews.appspot.com/1272801/diff/1/6#newcode66
user/src/com/google/gwt/autobean/server/ProxyAutoBean.java:66: Class?
returnType = method.getReturnType();
Oops, I moved that line before factoring inferName into BeanMethod and
forgot to undo the move afterwards.
http://gwt-code-reviews.appspot.com/1272801/diff/1/8
File
user/src/com/google/gwt/requestfactory/server/ReflectiveServiceLayer.java
(right):
http://gwt-code-reviews.appspot.com/1272801/diff/1/8#newcode248
user/src/com/google/gwt/requestfactory/server/ReflectiveServiceLayer.java:248:
return domainClass.getMethod(get + capitalizedProperty);
Should some kind of caching be added? (MapClass?,MapString,Method)
http://gwt-code-reviews.appspot.com/1272801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: UiBinder. Support for TreeItems. (issue1233803)
Taking this a bit further, if we're going to be playing with interfaces we
might as well go the whole nine yards:
interface IsTreeItem {
TreeItem asTreeItem();
}
interface HasTreeItems {
void addItem(IsTreeItem);
void addItem(Widget);
void addItem(SafeHtml);
/* No addItem(String), it's unsafe */
void removeItem(TreeItem);
void removeItem(IsTreeItem);
void removeItems();
}
class TreeItem implements IsTreeItem, HasTreeItems {
}
class Tree implements HasTreeItems {
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: UiBinder. Support for TreeItems. (issue1233803)
There is an existing overload that accepts a string and interprets it as
HTML, so that's not an option. And asking Konstantin to add a new
addItemText(String) method seems too far outside the scope of this patch.
On Wed, Jan 5, 2011 at 10:02 AM, John Tamplin j...@google.com wrote:
On Wed, Jan 5, 2011 at 12:47 PM, Ray Ryan rj...@google.com wrote:
Taking this a bit further, if we're going to be playing with interfaces we
might as well go the whole nine yards:
interface IsTreeItem {
TreeItem asTreeItem();
}
interface HasTreeItems {
void addItem(IsTreeItem);
void addItem(Widget);
void addItem(SafeHtml);
/* No addItem(String), it's unsafe */
Taking a string is fine, but it should be interpreted as plain text not
HTML..
--
John A. Tamplin
Software Engineer (GWT), Google
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Re-enable XML parse test, which was previously failing in one Safari configuration in Production... (issue1229801)
Please log a buganizer ticket and assign it to flin
On Wed, Jan 5, 2011 at 12:48 PM, Fred Sauer fre...@google.com wrote:
The test completes successfully on Safari 5.0.2 on OSX in web mode,
although it fails in HTMLUnit. I've marked the
test @DoNotRunWith({Platform.HtmlUnitUnknown})
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix)
Hey, Jeff. Happy New Year, and thanks for your patience. I'm looking at this now. On Thu, Dec 9, 2010 at 7:23 AM, Jeff Larsen larse...@gmail.com wrote: bump. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add getLocaleQueryParam and getLocaleCookieName (issue1250801)
On Tue, Jan 4, 2011 at 5:37 PM, j...@google.com wrote: On 2011/01/05 00:14:22, rjrjr wrote: Are you sure you don't want to introduce the widget and the api at the same time? If you like, I can do a simple version now (similar to the one in showcase) and the more complicated one I had in mind later (which would use a generator to build a menu tree based on the number of locales needed and their inheritance tree). I don't have time to do the more complicated one right now, and I would prefer not to have this sit for a long time waiting. How about making the one in Showcase use the new mechanism? Your call though, I didn't mean to hijack your patch. How are you avoiding duplication here? It's not like anything other than this widget will read this config, right? It's more like you're moving it around. The locale property provider also needs this configuration in order to pick up the proper locale to use, and eventually that information will get to the server for server-side selection, so it would be duplicated if not all derived from information from the module. Gotcha http://gwt-code-reviews.appspot.com/1250801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Initial version of HTML5 Audio and Video (issue1195801)
On Wed, Dec 15, 2010 at 5:40 PM, jlaba...@google.com wrote:
LGTM
Don't forget test cases.
By which I'm sure John meant before you submit this.
You guys are doing an awesome job with the HTML5 stuff! I can't wait to
see this stuff in action.
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004
File user/src/com/google/gwt/dom/client/MediaElement.java (right):
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode178
user/src/com/google/gwt/dom/client/MediaElement.java:178: return
this.error;
What does this return if there is no error? Is it null or undefined?
Might be better to do return this.error || null just in case.
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode309
user/src/com/google/gwt/dom/client/MediaElement.java:309: * controls
(e.g., for controlling play./pause, seek position, and volume),
replace e.g with such as. We avoid abbreviations because some
international users don't recognize them.
Also, play./pause should be play/pause.
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode410
user/src/com/google/gwt/dom/client/MediaElement.java:410: * Causes
playback of the resource to be (re)started.
Does it restart or resume if it is already started? How about Causes
playback of the resource to be started or resumed.
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode516
user/src/com/google/gwt/dom/client/MediaElement.java:516: public final
native void setSrc(String url) /*-{
instead of setAttribute(), can we use this.src = url
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4007
File user/src/com/google/gwt/media/client/Audio.java (right):
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4007#newcode68
user/src/com/google/gwt/media/client/Audio.java:68: public AudioElement
getAudioElement() {
Can we just override the return value of getElement()?
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4008
File user/src/com/google/gwt/media/client/Video.java (right):
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4008#newcode68
user/src/com/google/gwt/media/client/Video.java:68: public VideoElement
getVideoElement() {
I suggest overriding the return value of getElement() instead of
creating a new method.
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4010
File user/src/com/google/gwt/media/dom/DOM.gwt.xml (right):
http://gwt-code-reviews.appspot.com/1195801/diff/3001/4010#newcode18
user/src/com/google/gwt/media/dom/DOM.gwt.xml:18: inherits
name=com.google.gwt.media.dom.DOM/
Should inherit com.google.gwt.dom.DOM, not com.google.gwt.media.dom.DOM
http://gwt-code-reviews.appspot.com/1195801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Breaking change proposed: future proofing Activity in 2.1.1
Nnnnevermind. I think it's too late for me to make this not-terribly-popular change. It's already more widely adopted than I realized internally, so I have to assume that's even more true externally. I can't imagine such a break being well received. (Yes, we're making more significant changes to RequestFactory in 2.1.1, but I suspect that has a lower adoption rate so far, and client side the impact is actually fairly minimal, except for the dropped UserInfo stuff.) rjrjr On Wed, Dec 8, 2010 at 3:53 PM, Thomas Broyer t.bro...@gmail.com wrote: On Wednesday, December 8, 2010 8:22:39 PM UTC+1, John A. Tamplin wrote: On Wed, Dec 8, 2010 at 2:17 PM, Ray Ryan rj...@google.com wrote: Basically we don't know exactly how we want to change the thing, but have a feeling something will be needed. Re: composition or delegation, it always happens, but I'm not sure that's a concrete issue yet. We could introduce an IsActivity interface, but I don't see anywhere in the current GWT code we would actually call it. People implement their own ActivityMappers by hand, so they could use that convention themselves. Sounds like there aren't super strong feelings on this, so today for 2.1.1 I'm inclined to - drop the interface - rename AbstractActivity to Activity - document as being forbidden from developing any non-trivial behavior - and perhaps document the intent to retroactively introduce an interface when it's had a chance to settle Last passionate objections? I still feel like there is little cost in having the interface, which is what is used in the API, and a default implementation where any new methods added will get default behavior. Then document that if you implement the interface but don't extend the default implementation, you will be broken by future updates. That lets users decide whether they care more about not being broken by updates or more about not having to extend a base class. +1 Though I'm OK with the proposed (abstract)Activity and SimpleActivity (i.e. just make Activity an abstract class rather than an interface –with all methods being abstract– and rename AbstractActivity into SimpleActivity, rather than just renaming the current AbstractActivity to Activity, with the no-op methods) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add some missing docs for RPC utility class. (issue1207801)
Lgtm
On Dec 9, 2010 3:01 PM, b...@google.com wrote:
Reviewers: rjrjr,
Description:
Add some missing docs for RPC utility class.
Patch by: bobv
Review by: rjrjr
Please review this at http://gwt-code-reviews.appspot.com/1207801/show
Affected files:
M user/src/com/google/gwt/user/server/rpc/RPC.java
Index: user/src/com/google/gwt/user/server/rpc/RPC.java
===
--- user/src/com/google/gwt/user/server/rpc/RPC.java (revision 9400)
+++ user/src/com/google/gwt/user/server/rpc/RPC.java (working copy)
@@ -49,7 +49,9 @@
* h3Advanced Example/h3 The following example shows a more advanced
way of
* using this class to create an adapter between GWT RPC entities and
POJOs.
*
- * {...@example
com.google.gwt.examples.rpc.server.AdvancedExample#doPost(javax.servlet.http.HttpServletRequest,
javax.servlet.http.HttpServletResponse)}
+ * {...@example
+ *
com.google.gwt.examples.rpc.server.AdvancedExample#doPost(javax.servlet.http.
+ * HttpServletRequest, javax.servlet.http.HttpServletResponse)}
*/
public final class RPC {
@@ -241,7 +243,7 @@
// Read the RPC token
rpcToken = (RpcToken)
streamReader.deserializeValue(RpcToken.class);
}
-
+
// Read the name of the RemoteService interface
String serviceIntfName = maybeDeobfuscate(streamReader,
streamReader.readString());
@@ -369,6 +371,37 @@
AbstractSerializationStream.DEFAULT_FLAGS);
}
+ /**
+ * Returns a string that encodes an exception. If method is not
+ * codenull/code, it is an error if the exception is not in the
method's
+ * list of checked exceptions.
+ *
+ * p
+ * If the serializationPolicy parameter is not codenull/code, it is
used
+ * to determine what types can be encoded as part of this response. If
this
+ * parameter is codenull/code, then only subtypes of
+ * {...@link com.google.gwt.user.client.rpc.IsSerializable IsSerializable}
or
+ * types which have custom field serializers may be encoded.
+ * /p
+ *
+ * @param serviceMethod the method that threw the exception, may be
+ * codenull/code
+ * @param cause the {...@link Throwable} that was thrown
+ * @param serializationPolicy determines the serialization policy to be
used
+ * @param flags a bitmask of the flag constants defined in
+ * {...@link
com.google.gwt.user.client.rpc.impl.AbstractSerializationStream}
+ * . The choice of flags is determined by the options with
which the
+ * client was compiled and any optional features in use. In
+ * particular, the use of elided type names or RpcTokens by the
+ * client will require a flag value.
+ * @return a string that encodes the exception
+ *
+ * @throws NullPointerException if the the cause or the
serializationPolicy
+ * are codenull/code
+ * @throws SerializationException if the result cannot be serialized
+ * @throws UnexpectedException if the result was an unexpected exception
(a
+ * checked exception not declared in the serviceMethod's
signature)
+ */
public static String encodeResponseForFailure(Method serviceMethod,
Throwable cause, SerializationPolicy serializationPolicy, int flags)
throws SerializationException {
@@ -442,6 +475,36 @@
AbstractSerializationStream.DEFAULT_FLAGS);
}
+ /**
+ * Returns a string that encodes the object. It is an error to try to
encode
+ * an object that is not assignable to the service method's return type.
+ *
+ * p
+ * If the serializationPolicy parameter is not codenull/code, it is
used
+ * to determine what types can be encoded as part of this response. If
this
+ * parameter is codenull/code, then only subtypes of
+ * {...@link com.google.gwt.user.client.rpc.IsSerializable IsSerializable}
or
+ * types which have custom field serializers may be encoded.
+ * /p
+ *
+ * @param serviceMethod the method whose result we are encoding
+ * @param object the instance that we wish to encode
+ * @param serializationPolicy determines the serialization policy to be
used
+ * @param flags a bitmask of the flag constants defined in
+ * {...@link
com.google.gwt.user.client.rpc.impl.AbstractSerializationStream}
+ * . The choice of flags is determined by the options with
which the
+ * client was compiled and any optional features in use. In
+ * particular, the use of elided type names or RpcTokens by the
+ * client will require a flag value.
+ * @return a string that encodes the object, if the object is compatible
with
+ * the service method's declared return type
+ *
+ * @throws IllegalArgumentException if the result is not assignable to
the
+ * service method's return type
+ * @throws NullPointerException if the serviceMethod or the
+ * serializationPolicy are codenull/code
+ * @throws SerializationException if the result cannot be serialized
+ */
public static String encodeResponseForSuccess(Method serviceMethod,
Object object, SerializationPolicy serializationPolicy, int flags)
throws SerializationException {
--
Re: [gwt-contrib] Re: Breaking change proposed: future proofing Activity in 2.1.1
Basically we don't know exactly how we want to change the thing, but have a feeling something will be needed. Re: composition or delegation, it always happens, but I'm not sure that's a concrete issue yet. We could introduce an IsActivity interface, but I don't see anywhere in the current GWT code we would actually call it. People implement their own ActivityMappers by hand, so they could use that convention themselves. Sounds like there aren't super strong feelings on this, so today for 2.1.1 I'm inclined to - drop the interface - rename AbstractActivity to Activity - document as being forbidden from developing any non-trivial behavior - and perhaps document the intent to retroactively introduce an interface when it's had a chance to settle Last passionate objections? On Tue, Dec 7, 2010 at 8:31 AM, PhilBeaudoin philippe.beaud...@gmail.comwrote: Tell me if I get this right, but the most important advantage of having only an abstract class is that you are guaranteed your user extends the abstract class instead of implementing the interface, which let you easily extend it later (i.e. add methods) without breaking existing user code? On the other hand, it looks like, in a world of unchanging APIs, the interface might be the best way to go, facilitating things like reuse via a composition/delegation pattern. For example, MyPresenter could inherit from BasePresenter class and implement Activity, delegating all the calls to a composed AbstractActivity. Without an interface I would have to refactor BasePresenter to inherit from the Activity abstract class, making it impossible to have non-activity presenters. So it looks to me like it boils down to: 1) How likely is it that the Activity interface evolves in a way that would not be handled by adding subinterfaces or extra interfaces? 2) How frequently are users expected to compose/delegate with Activity? A concluding remark: GWTP went the abstract class way for its hierarchy of Presenter classes. It makes it quite easy to use, but the composition problem has reared its ugly head a couple of times. If I had to do it again I would use interfaces and favor composition of default implementations. Cheers, Philippe On Dec 6, 5:05 am, Jeff Larsen larse...@gmail.com wrote: Personally, I'm a fan of having both. The default implementation can be an abstract class but have that abstract class implement the Activity interface. Developers will be making a conscious choice to use the interface only knowing that they can introduce bugs. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Breaking change proposed: future proofing Activity in 2.1.1
I hope that doesn't come across as having ignored Neil, John et al. I do prefer using interface + abstract class, but I don't really believe that people actually read JavaDoc, and I'm certain we need to mess with this interface just a bit more. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Breaking change proposed: future proofing Activity in 2.1.1
Let me push harder for the abstract class. If the class is documented to
forbid non-trivial default implementations, there would be no need to mock
it, and no chance of breaking people who decide to use the interface
directly for whatever reason.
WRT to the single base class problem, I was initially going to propose that
we also add interface IsActivity { Activity asActivity(); } and make
Activity implement it. But I didn't see any immediate use for it, since
nothing at the moment accepts an Activity as an argument, and nothing
returns one except for ActivityMapper. If such a need appeared it would be
trivial to add.
You buying it? Is the loss of easyMock too much?
On Fri, Dec 3, 2010 at 12:36 AM, Neil Fuller nful...@google.com wrote:
Full disclosure - I am the one who has suggested a possible evolution of
Activity. I was suggested the introduction of a mechanism to allow an
existing Activity to be informed when the Place has changed but the Activity
has not (e.g. when there is a change in Place tokens). I commented about how
difficult it would be to add a method to Activity without introducing a
break and Ray kindly started this thread.
I agree with Amir about the concerns. The AbstractActivity class and the
interface it implements has to have sensible, obvious, do-nothing
implementation otherwise subclasses have to override the behavior they don't
want and it all gets messy. Also, in Java we get to have one base class, so
extending AbstractActivity precludes extending something else. Several APIs
I've worked with have moved away from a design that requires developers to
extend a particular class. e.g. JUnit
Removal of the interface entirely would make mocking with frameworks like
EasyMock more difficult. Although things like the EasyMock classextension
make it possible to mock non-interfaces it's not ideal and obviously isn't
an option in a GWT test case.
Personally I'd like to try to keep the interface for these reasons. How
about the documentation is beefed up on the interface to say You should
extend AbstractActivity rather than implement Activity directly otherwise
you may be broken if it doesn't already, but leave the interface in place
for people who want to take on the risk of a break? I guess it depends on
how much Activity is expected to change in future. In the case I have in
mind to Activity the implementation would probably have an obvious no-op
implementation, but without having talked about that it's hard to be sure.
Luckily, I'm pretty sure that the use of AbstractActivity is what most
people will have done anyway.
I've just started looking at the Activity framework and this particular
wrinkle was the first we hit, but I wonder how many more things the
ActivityManager (and associated classes) would want to call on the Activity
interface so the need to evolve Activity much further may not be required. I
do agree that now would be a good time to do this kind of change, though,
because it's only going to get harder.
Neil.
On 3 December 2010 05:39, Amir Kashani amirkash...@gmail.com wrote:
We've adopted the new MVP framework pretty heavily in a couple of new
projects and at this point, I don't think we've ever not used
AbstractActivity. So, as long as the existing methods in Activity don't
become inaccessible to non-GWT code (i.e. not package protected or final), I
don't see a problem.
My only concern is that while making it an abstract class makes it easier
to evolve, it also make it easier to default behavior that the end-user may
not want. If a mechanism isn't provided to override this behavior, then
someone may end up having to roll their own MVP framework for an otherwise
minor issue. With a something as powerful and complex as this framework,
that'd be a real shame. So, *please please please,* be cautious with
this power: reasonable default behavior and the ability to override.
Thanks!
- Amir
On Thu, Dec 2, 2010 at 7:55 PM, Ray Ryan rj...@google.com wrote:
We're making a few breaking changes in 2.1.1 to the new features
introduced in 2.1. (We're not supposed to do that kind of thing, but are
hoping to get away with it in this quick follow up release before there is
much adoption.)
I'd like to add a change to Activity to that list, in order to allow it
to evolve in later releases when breakage of any kind won't be an option:
I'd like to make Activity an abstract class instead of an interface,
basically rename AbstractActivity.
Any objections?
rjrjr
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
Google UK Limited
Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W
9TQ
Registered in England Number: 3977902
--
Google UK Limited
Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W
9TQ
Registered in England Number: 3977902
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com
Re: [gwt-contrib] Breaking change proposed: future proofing Activity in 2.1.1
Any part of my point is that making sure it remains a trivial class with only no-ops means you don't need to mock it. Is that a reasonable assumption? On Fri, Dec 3, 2010 at 11:11 AM, Patrick Julien pjul...@gmail.com wrote: making it a class instead of an interface means we can't mock it anymore. On Thu, Dec 2, 2010 at 10:55 PM, Ray Ryan rj...@google.com wrote: We're making a few breaking changes in 2.1.1 to the new features introduced in 2.1. (We're not supposed to do that kind of thing, but are hoping to get away with it in this quick follow up release before there is much adoption.) I'd like to add a change to Activity to that list, in order to allow it to evolve in later releases when breakage of any kind won't be an option: I'd like to make Activity an abstract class instead of an interface, basically rename AbstractActivity. Any objections? rjrjr -- 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
Re: [gwt-contrib] Breaking change proposed: future proofing Activity in 2.1.1
Patrick, you're the case in point. Because you don't use the abstract class, if we change the API later we will break your app. Were you unable to use the abstract class? If the Activity interface were documented to encourage you to do so, would you have? When we break your app, will you be okay with that? On Fri, Dec 3, 2010 at 12:13 PM, Patrick Julien pjul...@gmail.com wrote: This is more in line with what we're doing. With what we experienced with the ramp up to 2.1.0, we only use the Activity interface, we don't use the default implementation and instead make our own for common classes of use cases. On Fri, Dec 3, 2010 at 3:02 PM, John Tamplin j...@google.com wrote: On Fri, Dec 3, 2010 at 2:58 PM, Patrick Julien pjul...@gmail.com wrote: I don't know since I don't know what your plans are, will just have to trust you. That being said, the Activity interface is currently really nice and it doesn't tie us down to a single class for inheritance. I have been very happy with the recent cases where I have used an interface for the API but provided a default implementation, with the admonishment that implementing the interface without extending the default implementation is likely to be broken in the future. That way the people that care more about being able to substitute alternate implementations or to use it without having to extend the implementation can implement the interface, and those that care more about not being broken by future updates can extend the default implementation. -- John A. Tamplin Software Engineer (GWT), Google -- 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
Re: [gwt-contrib] Breaking change proposed: future proofing Activity in 2.1.1
One more question for Patrick: would you be better able to use AbstractActivity if the IsActivity interface were available? On Fri, Dec 3, 2010 at 12:18 PM, Ray Ryan rj...@google.com wrote: Patrick, you're the case in point. Because you don't use the abstract class, if we change the API later we will break your app. Were you unable to use the abstract class? If the Activity interface were documented to encourage you to do so, would you have? When we break your app, will you be okay with that? On Fri, Dec 3, 2010 at 12:13 PM, Patrick Julien pjul...@gmail.com wrote: This is more in line with what we're doing. With what we experienced with the ramp up to 2.1.0, we only use the Activity interface, we don't use the default implementation and instead make our own for common classes of use cases. On Fri, Dec 3, 2010 at 3:02 PM, John Tamplin j...@google.com wrote: On Fri, Dec 3, 2010 at 2:58 PM, Patrick Julien pjul...@gmail.com wrote: I don't know since I don't know what your plans are, will just have to trust you. That being said, the Activity interface is currently really nice and it doesn't tie us down to a single class for inheritance. I have been very happy with the recent cases where I have used an interface for the API but provided a default implementation, with the admonishment that implementing the interface without extending the default implementation is likely to be broken in the future. That way the people that care more about being able to substitute alternate implementations or to use it without having to extend the implementation can implement the interface, and those that care more about not being broken by future updates can extend the default implementation. -- John A. Tamplin Software Engineer (GWT), Google -- 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
