Makes EventBus available outside of the gwt package, in (issue1394803)

[rj...@google.com]

Reviewers: jlabanca, bobv,

Description:
Makes EventBus available outside of the gwt package, in
com.google.web.bindery.event.shared. Retrofits the old
classes as deprecated wrappers around the new ones.

The new package is not quite an identical API: we use the
excuse to remove the EventHandler and HandlesEvents
interfaces, which serve no discernable purpose. And the new
base event class is named Event, not GwtEvent

Note that the classes used directly by GWT widgets and dom
events (GwtEvent and HandlerManager in particular) are not
deprecated.

Please review this at http://gwt-code-reviews.appspot.com/1394803/

Affected files:
M tools/api-checker/config/gwt22_23userApi.conf
M user/src/com/google/gwt/activity/shared/Activity.java
M user/src/com/google/gwt/activity/shared/ActivityManager.java
M user/src/com/google/gwt/event/Event.gwt.xml
M user/src/com/google/gwt/event/EventBase.gwt.xml
M user/src/com/google/gwt/event/shared/EventBus.java
M user/src/com/google/gwt/event/shared/EventHandler.java
M user/src/com/google/gwt/event/shared/GwtEvent.java
M user/src/com/google/gwt/event/shared/HandlerManager.java
M user/src/com/google/gwt/event/shared/HandlerRegistration.java
A user/src/com/google/gwt/event/shared/LegacyHandlerWrapper.java
M user/src/com/google/gwt/event/shared/ResettableEventBus.java
M user/src/com/google/gwt/event/shared/SimpleEventBus.java
M user/src/com/google/gwt/event/shared/UmbrellaException.java
M user/src/com/google/gwt/event/shared/testing/CountingEventBus.java
M user/src/com/google/gwt/place/shared/PlaceController.java
M user/src/com/google/gwt/place/shared/PlaceHistoryHandler.java
M user/src/com/google/gwt/user/client/ui/Widget.java
A user/src/com/google/web/bindery/event/Event.gwt.xml
A user/src/com/google/web/bindery/event/shared/Event.java
A user/src/com/google/web/bindery/event/shared/EventBus.java
A user/src/com/google/web/bindery/event/shared/HandlerRegistration.java
A user/src/com/google/web/bindery/event/shared/ResettableEventBus.java
A user/src/com/google/web/bindery/event/shared/SimpleEventBus.java
A user/src/com/google/web/bindery/event/shared/UmbrellaException.java
A
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
M user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
M user/test/com/google/gwt/event/shared/HandlerManagerTest.java
M user/test/com/google/gwt/event/shared/ResettableEventBusTest.java
M user/test/com/google/gwt/event/shared/SimpleEventBusTest.java
M user/test/com/google/gwt/user/client/ui/WidgetTest.java
A user/test/com/google/web/bindery/event/shared/BarEvent.java
A user/test/com/google/web/bindery/event/shared/EventBusTestBase.java
A user/test/com/google/web/bindery/event/shared/EventSharedSuite.java
A user/test/com/google/web/bindery/event/shared/FooEvent.java
A
user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java
A user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java

[rj...@google.com]

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/

[Andrés Testi]

Why bindery package is nested in a web package? Are these APIs not
available for non web applications?
Regards.

- Andrés

[Ray Ryan]

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.

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Stephen Haberman]

> 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.

Is it worth moving packages at the current time then? You could tease
out a non-GWT jar with build tricks and go from there.

- Stephen

[John LaBanca]

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?  It seems like "web" could be dropped, saving 4 bytes in a lot of files.

.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?

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
jlab...@google.com

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Ray Ryan]

On Fri, Apr 1, 2011 at 10:38 AM, John LaBanca <jlab...@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.

[John LaBanca]

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 <jlab...@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.

I don't like it.  No good reason, it just doesn't have a nice ring to it.  You guys should start over and come up with a better name, preferably a palindrome.  Maybe com.google.web.rjrjr?

[Ray Ryan]

On Fri, Apr 1, 2011 at 11:16 AM, John LaBanca <jlab...@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 <jlab...@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.

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. 

[Andrés Testi]

> 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."

I really like the GWT event model and want to use it in Guice
applications to raise bussiness rules, dispatch entity lifecycle
events, etc.. I think this events API is useful for more than "web
service" projects.
Regards.

- Andrés

[jlab...@google.com]

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/activity/shared/Activity.java
File user/src/com/google/gwt/activity/shared/Activity.java (right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/activity/shared/Activity.java#newcode19
user/src/com/google/gwt/activity/shared/Activity.java:19: import
com.google.gwt.user.client.ui.IsWidget;
This looks like an unused import (only used in JavaDoc). Might be
better to inline the full path in the JavaDoc to avoid the warning.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/Event.gwt.xml
File user/src/com/google/gwt/event/Event.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/Event.gwt.xml#newcode2
user/src/com/google/gwt/event/Event.gwt.xml:2: <inherits
name="com.google.gwt.event.EventBase" />
Remove tabs from all lines or revert.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/EventBase.gwt.xml
File user/src/com/google/gwt/event/EventBase.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/EventBase.gwt.xml#newcode5
user/src/com/google/gwt/event/EventBase.gwt.xml:5: <source path="shared"
/>
remove tabs from all lines.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/Event.gwt.xml
File user/src/com/google/web/bindery/event/Event.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/Event.gwt.xml#newcode17
user/src/com/google/web/bindery/event/Event.gwt.xml:17: <source
path="client"/>
The client path is not actually present. Can we remove it? Is it here
for future support?

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/Event.gwt.xml#newcode18
user/src/com/google/web/bindery/event/Event.gwt.xml:18: <source
path="shared"/>
extra spaces or tabs

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/Event.java
File user/src/com/google/web/bindery/event/shared/Event.java (right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/Event.java#newcode2
user/src/com/google/web/bindery/event/shared/Event.java:2: * Copyright
2009 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/EventBus.java
File user/src/com/google/web/bindery/event/shared/EventBus.java (right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/EventBus.java#newcode2
user/src/com/google/web/bindery/event/shared/EventBus.java:2: *
Copyright 2010 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java
File
user/src/com/google/web/bindery/event/shared/HandlerRegistration.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode2
user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:2:
* Copyright 2008 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode20
user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:20:
* {@link EventBus#addHandler}, used to deregister.
Close parathesis from (e.g....):

(e.g. via {@linkEventBus#addHandler})

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode22
user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:22:
* A tip: to make a handler deregister itself, the following works:
Phrasing. How about the following:

NOTE: The following code creates a handler that deregisters itself the
first time it handles an event.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode35
user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:35:
* Deregisters a handler.
The API should call out how to handle multiple calls or calls to
handlers that are already removed.

Deregisters the handler associated with this registration object if the
handler is still attached to the event source. If the handler is no
longer attached to the event source, this is a no-op.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java
File
user/src/com/google/web/bindery/event/shared/ResettableEventBus.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java#newcode2
user/src/com/google/web/bindery/event/shared/ResettableEventBus.java:2:
* Copyright 2010 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java#newcode27
user/src/com/google/web/bindery/event/shared/ResettableEventBus.java:27:
public class ResettableEventBus extends EventBus {
Make sure you incorporate http://gwt-code-reviews.appspot.com/1388804/
before submitting.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java
File user/src/com/google/web/bindery/event/shared/SimpleEventBus.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode2
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:2: *
Copyright 2010 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode37
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:37:
private interface Command {
interfaces go above fields.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode63
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:63: *
protected because it is a bad idea to rely upon the order of
It isn't package protected, but see next comment.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode69
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:69:
protected SimpleEventBus(boolean fireInReverseOrder) {
I suggest that you remove this method and add a package protected
setFireInReverseOrder method. Then, you can call it from
c.g.g.event.shared.SimpleEventBus using the violator pattern and keep
this API clean.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode96
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:96: }
You can move the type and handler checks to doAdd(). Slight reduction
in code size and protects against future uses of doAdd().

The source==null check will have to stay in this method.

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode116
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:116: }
Ditto. Move event==null check into doFire, keep the source==null check
in this method.

Also, there are a lot of if(null) checks. You could add a
assertNotNull(Object toCheck, String message)

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode124
user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:124:
protected <H> void doRemove(Event.Type<H> type, Object source, H
handler) {
I don't like adding deprecated APIs. Can you use the violator pattern
to keep the deprecated methods out of here?

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java
File user/src/com/google/web/bindery/event/shared/UmbrellaException.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java#newcode2
user/src/com/google/web/bindery/event/shared/UmbrellaException.java:2: *
Copyright 2010 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java#newcode22
user/src/com/google/web/bindery/event/shared/UmbrellaException.java:22:
* {@link Throwable}s together. Typically thrown after loop, with all of
the
thrown after loop => thrown after a loop

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
File
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode2
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:2:
* Copyright 2010 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode77
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:77:
counts.put(type, count - 1);
maybe just getCount(type) - 1

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode85
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:85:
counts.put(type, count + 1);
getCount(type) + 1

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/gwt/event/shared/SimpleEventBusTest.java
File user/test/com/google/gwt/event/shared/SimpleEventBusTest.java
(left):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/gwt/event/shared/SimpleEventBusTest.java#oldcode35
user/test/com/google/gwt/event/shared/SimpleEventBusTest.java:35: public
void testAddAndRemoveHandlers() {
Why remove all of these tests?

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/EventBusTestBase.java
File user/test/com/google/web/bindery/event/shared/EventBusTestBase.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/EventBusTestBase.java#newcode2
user/test/com/google/web/bindery/event/shared/EventBusTestBase.java:2: *
Copyright 2008 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java
File
user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java#newcode2
user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java:2:
* Copyright 2010 Google Inc.
2011

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java
File
user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java
(right):

http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java#newcode2
user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java:2:
* Copyright 2010 Google Inc.
2011

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