Forgot to send these comments earlier. Ignore if they're too late.
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.
It makes it difficult to know whose listening to a specific widget. If
you remove a widget, you have to do a text search over the codebase to
find out who was listening to that specific widget. Whereas if you
force people to get a handle on the widget instance (or the presenter
instance), then you can easily search for references.
On 2011/06/14 18:34:41, rjrjr wrote:
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/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 18:34:41, rjrjr wrote:
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?
The methods descend into the widget hierarchy, which is convenient. If
users take manual control, then they would also have to descend into the
hierarchy of drivers, starting and stopping them.
They could probably be made public, with a public
setSyncDriversToAttachState(boolean) method to enable/disable the
default implementation in onAttach/onDetach().
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();
On 2011/06/14 18:34:41, rjrjr wrote:
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.
I agree that setNextDriver() is fine for now, and I see why its
important that it is asynchronous.
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();
So now we have isActive(), why not just add start/stopDriver(), which
are the equivalent of setActive(boolean)?
On 2011/06/14 18:34:41, rjrjr wrote:
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/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
