On Sun, Sep 12, 2010 at 4:03 AM, <[email protected]> wrote: > Are all these ForIsWidget interfaces (and method overloads) really > needed? (and actually useful?) >
I think so. Part of the goal here is to make it simpler to use the IsWidget pattern without having to think about it so hard. Ideally this patch includes nearly every call to asWidget() that will ever need to be written. Although it did have too many in the first patch, due to some foolish copy paste of lots of extra add(IsWidget). (Still working on the new patch.) > For completeness, TabLayoutPanel and TabPanel don't have IsWidget > overloads for their add, getTabWidget and selectTab methods. > They don't need it for add, they inherit it from Panel. Re: the other two (on TabLayoutPanel only), there are also a ton of variations on insert and such. I'm thinking it's just too much to add overloads for every one of them. Ideally I'd just change all their Widget arguments to IsWidget, but that would break anyone who had sub-classed them. (This kind of thing is why I really hate convenience methods. They paint you into corners.) So I guess I'm arbitrarily drawing the line at the characteristic interface methods. Are you guys buying this? I don't think there is much benefit to making Widget extend IsWidget unless the common add and insert methods know about it too, but covering every widget insert point in our api seems too much. > > (if you ask me, I'd only keep HasOneWidget, without the getWidget > method, i.e. just a move of Activity.Display into c.g.g.user.client.ui, > and have it implemented by SimplePanel; and also of course the move of > IsWidget, implemented by Widget; and the Widget#getWidgetOrNull, as a > public method; and that's it) > I've long thought that HasOneWidget was missing as a parallel to HasWidgets, so I'd like to keep it. I'll go with Bob's AcceptsOneWidget notion. I'll respond to inline comments in Rietveld. > > > 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)}. > s/Display/HasOneWidget/ > > 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() { > Which makes me wonder: should HasOneWidget have this getWidget method? > > 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#newcode28 > user/src/com/google/gwt/user/client/ui/HasOneWidget.java:28: public > Widget getWidget(); > The asymmetry between setWidget taking an IsWidget, and getWidget > returning a Widget looks weird. > > Maybe rename the interface to TakesOneWidget and only keep its > setWidget(IsWdiget) method? > ...or maybe add a setWidget(Widget) method. > > 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) { > Shouldn't this be a new method overload, and keep the setWidget(Widget) > override to also maybeUpdateSize? > > 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) { > Making it public would be harmless and could prove useful ;-) > > > http://gwt-code-reviews.appspot.com/864801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
