Re: missing change in changelog / additional questions for code contribution

[John LaBanca]

+gwt-contrib for open discussion

Thanks,
John LaBanca
jlab...@google.com

2010/2/4 Sven Brunken <sven.b...@googlemail.com>

Sorry for the delay:

http://gwt-code-reviews.appspot.com/139801/show
http://gwt-code-reviews.appspot.com/138801/show

Sven Brunken


Am 04.02.2010 21:04, schrieb John LaBanca:

I'd suggest that Sven upload his patch to rietveid,  and let gwt-contrib users will be able to review and comment on it.  Or just start a gwt-contrib thread if you'd rather get feedback before doing the work.

Thanks,
John LaBanca
jlab...@google.com

2010/2/4 Joel Webber <j...@google.com>

It would be great if we could move this conversation to gwt-contrib to get everyone's feedback. I don't doubt that there are some modifications we could make to HandlerManager and its use in Widget that would make everyone's lives easier :)

2010/2/4 Sven Brunken <sven.b...@googlemail.com>

Am 04.02.2010 20:02, schrieb John LaBanca:

There is no way to change the default HandlerManager in an easy, java way. You could hack it with a JSNI call. At the moment it always creates the default instance.

Do you have a user case for replacing it?   We could add a protected createHandlerManager() method that would allow users to subclass widget and provide there own, but in my opinion its too rare a use case to support it.

I have a more complex HandlerManager with a couple of more features like event buffering, delayed events etc. Also the normal HandlerManager always creates the DefaultHandlerRegistration. I have a custom HandlerRegistration that has a setSuspended(boolean suspended) method that can suspend this handler temporary.

I have a couple of own widgets that are far more complex and fire many more events than the GWT widgets. The changes needed are minimal. I can publish a small patchfile how i did it to the review system and than you can have a look.

An other one is inside the RichTextAreaImpl, it links directly to the RichTextArea widget, while it should link to the HasInitializeHandlers interface, so you can reuse it everywhere.

RichTextAreaImpl is highly customized for RichTextArea.  Impl classes are considered off limits, and we don't encourage people to reuse them.

The hookEvents method of the RichTextAreaImpl subclasses links to the Widget class, here you could better link the the EventListener interface that defines onBrowserEvent method.

Again, impl classes aren't meant to be reusable.

The change to support this is minimal. I am using this Impl class for an own RichTextArea. The Impl class has all the features you need to write an own widget that supports all this. I think they should be made reusable. FormPanelImpl is doing what i am looking for. It is using the FormPanelImplHost interface instead of directly FormPanel. That is why it was easy for me to use the already finished Impl class with an "own" FormPanel. I will also open a review for this.

I also have fixes for a couple of other issues that are already listed in the issuetracker. All changes are backward compatible.

Bug fixes are always welcome!  You can upload them to rietveld and assign either jgw or jlabanca as the reviewer, and we'll take a look.

http://gwt-code-reviews.appspot.com/

Great, i will add them.

Thanks,
John LaBanca
jlab...@google.com

2010/2/4 Miguel Méndez <mme...@google.com>

Thanks for the heads up on the breaking change Sven.  I've added someone to this thread that should be able to answer the Widget question.

We love to get external contributions!  The process for contributing code is outlined in Making GWT Better.

Cheers,

On Wed, Feb 3, 2010 at 8:42 AM, Sven Brunken <sven.b...@googlemail.com> wrote:

Hello Miguel,

sorry for contacting you directly.

I wanted to inform you, that your changelog for GWT 2.0.1 is missing one small, but braking, change. In revision 7424 the only contructor of ImageResourcePrototype was changed (http://code.google.com/p/google-web-toolkit/source/diff?spec=svn7523&r=7424&format=side&path=/trunk/user/src/com/google/gwt/resources/client/impl/ImageResourcePrototype.java). This change leads to braking code, if your code was calling this contructor. However the fix is quite easy.
I am using this class because i modified the ImageResourceGenerator to not generate transparent PNG24 files for IE6. I already raised the issue with using transparent PNG files in IE6 on the contributer mailinglist, but also after a long discussion, the changes were minimal. The only good news was that IE7 is no longer using the alpha filters.


I also have a couple of small design issues with the Widget class. There is no way to change the default HandlerManager in an easy, java way. You could hack it with a JSNI call. At the moment it always creates the default instance. An other one is inside the RichTextAreaImpl, it links directly to the RichTextArea widget, while it should link to the HasInitializeHandlers interface, so you can reuse it everywhere. The hookEvents method of the RichTextAreaImpl subclasses links to the Widget class, here you could better link the the EventListener interface that defines onBrowserEvent method.
I also have fixes for a couple of other issues that are already listed in the issuetracker. All changes are backward compatible.

I would love to contribute them back to you. Are you interested in the patchfiles? And if so, where should a best publish them for review, on the tracker, the contributer mailinglist or in the code review system? Do i need to sign the CLA before publishing them?

I hope you find some minutes to come back to me.


Sven Brunken

--
Miguel