See comments. I think we can give users more control over when drivers are started/stopped and make the flow a little more obtainable in the process.
http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java File user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java (right): http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java#newcode54 user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java:54: public static DefaultEventBusProvider instanceMaybe() { Instead of making event sources do null checks, you could add a static fireEvent/fireEventFromSource methods that do the null check for the event source. http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/IsEventSource.java File user/src/com/google/gwt/user/client/ui/IsEventSource.java (right): http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/IsEventSource.java#newcode28 user/src/com/google/gwt/user/client/ui/IsEventSource.java:28: * event bus. On 2011/06/14 12:29:25, bobv wrote:
Why a String and not the general Object that Events can use as a
source? +1 Makes it hard to distinguish between multiple instances of the same Widget, such as when used in a table column. http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java File user/src/com/google/gwt/user/client/ui/Widget.java (right): http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode252 user/src/com/google/gwt/user/client/ui/Widget.java:252: * particular, hiding a parent does not stop the drivers of its children. This sounds like its going to cause problems. If I detach a Panel, ondetach will bubble down and stop the child drivers. But if I hide a Panel, that does not happen. That inconsistency is confusing and hard to work around. start() doesn't have enough information to know if I'm invisible or if I'm detached, and I only need to descend in one of those cases. Instead, we could introduce two new methods to Widget: startDriver() and stopDriver(). The contract of both methods should be that they descend into child widgets. By default, we call these methods from onAttach/onDetach/setVisible. Users should be able to set a boolean so they can manually control the driver (possibly as an optional arg to setNextDriver). If they take manual control, then the user is responsible for calling startDriver/stopDriver. This is particularly useful if you plan to attach/detach to widget often and don't necessarily want to kill the driver. Its also useful if you don't want to disable the driver when the widget is invisible. http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode268 user/src/com/google/gwt/user/client/ui/Widget.java:268: driver.stop(); What if I set the driver within driver.stop()? For example, maybe I have a two drivers: one when the widget is active, one when its inactive, and I swap them back and forth. This should be more resilient: IsWidgetDriver oldDriver = driver; IsWidgetDriver newDriver = nextDriver; driver = nextDriver; if (...) {oldDriver.stop();} // If setNextDriver was called in stop, newDriver is unused. // Need to wait for updateDriver to execute asynchronously. if (newDriver == nextDriver && ...) { nextDriver = null; newDriver.start() } http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode320 user/src/com/google/gwt/user/client/ui/Widget.java:320: driver.start(); I'm worried that start/stop can get out of sync if the user overrides setVisible(). Would it be better to maintain a boolean indicating that the driver has been started? If you take my suggestion for startDriver/stopDriver, its an even better idea. http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode364 user/src/com/google/gwt/user/client/ui/Widget.java:364: return new ResettableEventBus(requireEventBus(), getEventSourceName()); Is like a temporary event bus? Is the idea that a driver will spawn an event bus and use it until its stopped? http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode434 user/src/com/google/gwt/user/client/ui/Widget.java:434: DefaultEventBusProvider ebProvider = DefaultEventBusProvider.instanceMaybe(); This might be a bit too smart. getEventBus() may or may not return null depending on when the user adds a handler to DefaultEventBusProvider.instance(). DefaultEventBusProvider.instance().eventBus().addHandler(...) myWidget.getEventBus().addHandler(); // Works myWidget.getEventBus().addHandler(); // NPE DefaultEventBusProvider.instance().eventBus().addHandler(...) http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode583 user/src/com/google/gwt/user/client/ui/Widget.java:583: return specifiedEventBus; If I call requireEventBus, the next call to getEventBus() still returns null. http://gwt-code-reviews.appspot.com/1449817/diff/4001/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/1449817/diff/4001/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode37 user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:37: public class Null implements HandlerRegistration { Or maybe NOOP? Null is a reserved word with a specific meaning. This might be better as a private class in NullEventBus. I don't thinks its useful anywhere else. On 2011/06/14 12:29:25, bobv wrote:
Do you ever need more than one instance of this type?
HandlerRegistration.NULL
might be a better choice.
http://gwt-code-reviews.appspot.com/1449817/diff/4001/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/1449817/diff/4001/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java#newcode55 user/src/com/google/web/bindery/event/shared/ResettableEventBus.java:55: * Remove all handlers that have been added through this wrapper, and neuter We should rename the method to neuter() http://gwt-code-reviews.appspot.com/1449817/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
