http://gwt-code-reviews.appspot.com/864801/diff/3001/4003 File user/src/com/google/gwt/app/place/Activity.java (right):
http://gwt-code-reviews.appspot.com/864801/diff/3001/4003#newcode58 user/src/com/google/gwt/app/place/Activity.java:58: * {...@link Display#setWidget(IsWidget)}. On 2010/09/12 11:03:49, tbroyer wrote:
s/Display/HasOneWidget/
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4004 File user/src/com/google/gwt/app/place/ActivityManager.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4004#newcode60 user/src/com/google/gwt/app/place/ActivityManager.java:60: public Widget getWidget() { On 2010/09/12 11:03:49, tbroyer wrote:
Which makes me wonder: should HasOneWidget have this getWidget method?
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4009 File user/src/com/google/gwt/user/client/ui/AbsolutePanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4009#newcode99 user/src/com/google/gwt/user/client/ui/AbsolutePanel.java:99: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
And redundant override. Snipped. http://gwt-code-reviews.appspot.com/864801/diff/3001/4009#newcode154 user/src/com/google/gwt/user/client/ui/AbsolutePanel.java:154: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4010 File user/src/com/google/gwt/user/client/ui/ComplexPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4010#newcode46 user/src/com/google/gwt/user/client/ui/ComplexPanel.java:46: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4012 File user/src/com/google/gwt/user/client/ui/FlowPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4012#newcode41 user/src/com/google/gwt/user/client/ui/FlowPanel.java:41: On 2010/09/12 13:43:57, jlabanca wrote:
extra newline
extra method. Snipped. http://gwt-code-reviews.appspot.com/864801/diff/3001/4013 File user/src/com/google/gwt/user/client/ui/HasOneWidget.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4013#newcode21 user/src/com/google/gwt/user/client/ui/HasOneWidget.java:21: public interface HasOneWidget { On 2010/09/12 13:43:57, jlabanca wrote:
Would HasWidget be a better name? The singularity of Widget
implicitly assumes
it has one.
I felt the one letter difference from HasWidget was too easy to misread, and HasOneWidget didn't sound so bad. Now that we're adding AcceptsOneWidget I like it even better. Okay? http://gwt-code-reviews.appspot.com/864801/diff/3001/4013#newcode28 user/src/com/google/gwt/user/client/ui/HasOneWidget.java:28: public Widget getWidget(); On 2010/09/12 13:43:50, bobv wrote:
On 2010/09/12 11:03:49, tbroyer wrote: > The asymmetry between setWidget taking an IsWidget, and getWidget
returning a
> Widget looks weird.
Agreed. I think we've been using "TakesX" to indicate having both a
getter and
setter.
HasOneWidget = Widget getWidget(); AcceptsOneWidget = setWidget(IsWidget w);
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4015 File user/src/com/google/gwt/user/client/ui/HorizontalPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4015#newcode34 user/src/com/google/gwt/user/client/ui/HorizontalPanel.java:34: On 2010/09/12 13:43:57, jlabanca wrote:
extra newline
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4015#newcode35 user/src/com/google/gwt/user/client/ui/HorizontalPanel.java:35: private VerticalAlignmentConstant vertAlign = ALIGN_TOP; On 2010/09/12 13:43:57, jlabanca wrote:
missing newline
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4015#newcode46 user/src/com/google/gwt/user/client/ui/HorizontalPanel.java:46: public void add(IsWidget w) { On 2010/09/12 13:43:57, jlabanca wrote:
missing newline
snipped method http://gwt-code-reviews.appspot.com/864801/diff/3001/4016 File user/src/com/google/gwt/user/client/ui/IndexedPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4016#newcode30 user/src/com/google/gwt/user/client/ui/IndexedPanel.java:30: * /** Gets the child widget at the specified index. On 2010/09/12 13:43:57, jlabanca wrote:
syntax
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4018 File user/src/com/google/gwt/user/client/ui/IsWidget.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4018#newcode27 user/src/com/google/gwt/user/client/ui/IsWidget.java:27: * @return the {...@link Widget} aspect of the rceiver On 2010/09/12 13:43:50, bobv wrote:
rEceiver
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4019 File user/src/com/google/gwt/user/client/ui/Panel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4019#newcode69 user/src/com/google/gwt/user/client/ui/Panel.java:69: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4019#newcode110 user/src/com/google/gwt/user/client/ui/Panel.java:110: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4020 File user/src/com/google/gwt/user/client/ui/PopupPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4020#newcode971 user/src/com/google/gwt/user/client/ui/PopupPanel.java:971: public void setWidget(IsWidget w) { On 2010/09/12 13:43:57, jlabanca wrote:
+1 users may override setWidget(Widget) and expect it to be called. I
think
this method should delegate to setWidget(widget)
It's a bug for it to be here at all, since SimplePanel takes care of the overload. Thank goodness it broke a test. http://gwt-code-reviews.appspot.com/864801/diff/3001/4021 File user/src/com/google/gwt/user/client/ui/SimplePanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4021#newcode47 user/src/com/google/gwt/user/client/ui/SimplePanel.java:47: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4021#newcode117 user/src/com/google/gwt/user/client/ui/SimplePanel.java:117: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4023 File user/src/com/google/gwt/user/client/ui/StackPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4023#newcode51 user/src/com/google/gwt/user/client/ui/StackPanel.java:51: On 2010/09/12 13:43:50, bobv wrote:
This blank line looks misplaced.
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4023#newcode72 user/src/com/google/gwt/user/client/ui/StackPanel.java:72: } On 2010/09/12 13:43:57, jlabanca wrote:
missing newline
Deleted method. http://gwt-code-reviews.appspot.com/864801/diff/3001/4024 File user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4024#newcode264 user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java:264: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. Can't figure out why this keeps happening. Trying out the AnyEdit plugin to clean this stuff up on save. http://andrei.gmxhome.de/eclipse/ http://gwt-code-reviews.appspot.com/864801/diff/3001/4024#newcode277 user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java:277: On 2010/09/12 13:43:57, jlabanca wrote:
extra spaces
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4026 File user/src/com/google/gwt/user/client/ui/VerticalPanel.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4026#newcode33 user/src/com/google/gwt/user/client/ui/VerticalPanel.java:33: On 2010/09/12 13:43:57, jlabanca wrote:
extra newline
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4026#newcode41 user/src/com/google/gwt/user/client/ui/VerticalPanel.java:41: } On 2010/09/12 13:43:57, jlabanca wrote:
missing newline
Bad method. http://gwt-code-reviews.appspot.com/864801/diff/3001/4027 File user/src/com/google/gwt/user/client/ui/Widget.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4027#newcode42 user/src/com/google/gwt/user/client/ui/Widget.java:42: protected static Widget asWidgetOrNull(IsWidget w) { On 2010/09/12 11:03:49, tbroyer wrote:
Making it public would be harmless and could prove useful ;-)
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4027#newcode57 user/src/com/google/gwt/user/client/ui/Widget.java:57: On 2010/09/12 13:43:50, bobv wrote:
Blank line.
Done. http://gwt-code-reviews.appspot.com/864801/diff/3001/4034 File user/test/com/google/gwt/user/client/ui/SimplePanelTest.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4034#newcode35 user/test/com/google/gwt/user/client/ui/SimplePanelTest.java:35: p.setWidget(null); On 2010/09/12 13:43:57, jlabanca wrote:
Is that because Widget extends IsWidget? I assume it uses the
IsWidget version
then, and in general does a runtime check?
Parameter ambiguity is all decided at compile time in Java, based on variable type. Receiver choice is dynamic, overload resolution is static. And actually, the more specific version wins, setWidget(Widget) in this case. http://gwt-code-reviews.appspot.com/864801/diff/3001/4036 File user/test/com/google/gwt/user/client/ui/WidgetTest.java (right): http://gwt-code-reviews.appspot.com/864801/diff/3001/4036#newcode49 user/test/com/google/gwt/user/client/ui/WidgetTest.java:49: final com.google.gwt.event.shared.HandlerManager manager = new com.google.gwt.event.shared.HandlerManager( On 2010/09/12 13:43:57, jlabanca wrote:
Why did you inline the path for HandlerManager?
To avoid a deprecation warning on the import line without having to suppress it for the entire class. Should I just make eclipse turn off that warning instead? Do other do so? http://gwt-code-reviews.appspot.com/864801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
