Responses below. No code changes yet.
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#newcode41
user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java:41:
public static DefaultEventBusProvider instance() {
Yes, but I need to add the javax.inject package to GWT. I'll put a TODO.
Re: setInstance(), I've generally seen such things cause more harm than
good, especially in test code.
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() {
You don't write a lot of app unit tests, do you?
The expectation is that instanceMaybe() will be used rarely, and only by
library developers. An app either uses the default bus (and so would
call instance), or it doesn't, and so would never refer to this class.
And even then, most app code would receive an eventBus as a constructor
argument, or would ask a Widget for it. If your app is littered with
references to this class, you screwed up. Perhaps the javadoc should say
so.
http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java#newcode68
user/src/com/google/gwt/user/client/ui/DefaultEventBusProvider.java:68:
public EventBus get() {
I'll look at that, thanks.
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.
I've gone back and forth on this. The idea with a string based approach
is that it makes sketching in ui.xml a lot simpler:
<gwt:Button eventSourceName="save">Save</gwt:Button>
<gwt:Button eventSourceName="delete">Delete</gwt:Button>
Elsewhere your app can register handlers for click events with those
names.
And it's not such a big deal to make those strings as unique as you need
them.
Perhaps the thing to do is provide a convenience setter for the string
case?
Object getEventSource()
void setEventSource(Object)
void setEventSourceName(String)
Too confusing?
http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/IsWidgetDriver.java
File user/src/com/google/gwt/user/client/ui/IsWidgetDriver.java (right):
http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/IsWidgetDriver.java#newcode53
user/src/com/google/gwt/user/client/ui/IsWidgetDriver.java:53: public
interface IsWidgetDriver {
I'd rather change the name than add another dimension of indirection to
our world. DrivesWidgets? Ditto for the abstract class. I'd rather
gamble that this is the right api than force clients to deal with an
abstract class. And to that end, I don't think I should include this in
2.4
On 2011/06/14 12:29:25, bobv wrote:
This type name implies there should be a single method
WidgetDriver asWidgetDriver();
WidgetDriver could then be an abstract base class to allow additional
lifecycle
methods to be added in the future.
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.
On 2011/06/14 15:05:48, jlabanca wrote:
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.
These would be protected methods, right?
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.
I don't understand this. What's the point of setting the driver if you
don't want the widget to call its start and stop methods for you? If I
want to take manual control I can override startDriver() and
stopDriver(), no?
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();
I'm also concerned that providing only setNextDriver might be too
inflexible. Should there be a setDriver() as well, which takes effect
immediately? My instinct was to start with only this one and see if
people ask for more.
On 2011/06/14 15:05:48, jlabanca wrote:
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()
}
Nice. Thanks for this.
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();
How about protected boolean isActive() method, with a default
implementation based on attached and visible? This setVisible method
might become:
if ((driver != null) && isActive()) {
startDriver();
}
super.setVisible(visible);
if ((driver != null) && !isActive()) {
stopDriver();
}
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());
Exactly. And calling ResettableEventBus#die turns the fire method into
no-ops, so the driver doesn't need to have a bunch of if(dead) calls in
its async callbacks.
I should bunch of the javadoc to point out that this is particularly
useful to the widget's driver.
On 2011/06/14 15:05:48, jlabanca wrote:
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();
On 2011/06/14 15:05:48, jlabanca wrote:
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(...)
This isn't a public method. spawnEventBus is the only public way to get
one, and that will never be null.
http://gwt-code-reviews.appspot.com/1449817/diff/4001/user/src/com/google/gwt/user/client/ui/Widget.java#newcode580
user/src/com/google/gwt/user/client/ui/Widget.java:580: protected
EventBus requireEventBus() {
Okay, I guess. But as you read this is it clear to you that
getEventBus() and requireEventBus() are protected calls? The only public
way to get an event bus off of a widget is through spawnEventBus, which
calls here.
And I'm missing some important javadoc on setEventBus(). Calling that
with null will not stop the widget from posting events, it will make it
post to the default event bus (if there is one). To actually silence a
widget you have to call setEventBus with something like a NullEventBus
instance.
The goal here is to make it simple for apps to do the event bus thing
without having to completely convert to a DI approach, but at the same
time allow DI apps to do it the "right" way.
On 2011/06/14 12:29:25, bobv wrote:
Instead of having two getEventBusMethods(), what about placing a
boolean flag on
getEventBus() to say "don't return null" or at least delegating from
getEventBus() to getEventBus(boolean) so there's only one method that
actually
makes EventBus objects pop into existence?
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;
On 2011/06/14 15:05:48, jlabanca wrote:
If I call requireEventBus, the next call to getEventBus() still
returns null.
No it doesn't. It returns the default event bus. By calling
DEBP.instance() we force the DEBP singleton into existence. The next
call to getEventBus() will return that.
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 {
Good point, I'll move it.
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#newcode37
user/src/com/google/web/bindery/event/shared/ResettableEventBus.java:37:
public ResettableEventBus(EventBus wrappedBus, String name) {
You okay with the setEventSource(Object) / setEventSourceName(String)
notion to keep uibinder simple?
On 2011/06/14 12:29:25, bobv wrote:
If the eventSourceName is turned into an Object eventSourceTag, it
would allow
users to use enums or other Objects for dispatch instead of descending
into
String-matching hell.
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
Okay.
On 2011/06/14 15:05:48, jlabanca wrote:
We should rename the method to neuter()
http://gwt-code-reviews.appspot.com/1449817/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
