Emily and I talked over these changes informally, and she convinced me that HasHandlers.getHandlers() is necessary, even though it makes me kind of uncomfortable (I don't have a better answer, so who am I to complain). Here's why. Take a look at at ValueChangeEvent.fire() (the original):
public static <I> void fire(HasValueChangeHandlers<I> source, I value) {
... }
Notice that the 'source' argument's type creates a generic constraint that
ties it to the type of the 'value' argument. When I created my patch, I
added the HandlerCollection as the first argument. Which was cool except
that it obviated the need for 'source' altogether. I failed to notice this,
but had I removed 'source', it would have also removed the generic
constraint, making it possible to pass the wrong type for value, which would
be a Bad Thing.
So I have reluctantly dropped that method back in and remade the patch
(attached). I've also created a rietveld issue here:
http://gwt-code-reviews.appspot.com/2205
Other comments inline.
On Wed, Jan 14, 2009 at 4:36 PM, Ray Ryan <rj...@google.com> wrote:
>
>
> On Wed, Jan 14, 2009 at 1:18 PM, Joel Webber <j...@google.com> wrote:
>
>> All,
>> I've run into a few snags trying to use the new event system with some
>> custom libraries, and would like to propose a few changes that would make it
>> easier to reuse the GwtEvent hierarchy without necessarily using all of the
>> infrastructure in com.google.gwt.event.*
>>
>> When trying to fire GwtEvents without using HandlerManager (don't ask...
>> but it is a legitimate use-case), I hit a couple of brick walls. The first
>> was that GwtEvent.setSource() was package-protected, so that only
>> HandlerManager was intended to be able to call it. This makes it impossible
>> to usefully create one on your own.
>>
>> To solve this first problem, I simply added a static setEventSource()
>> method to HandlerManager. Even though it's in HandlerManager, I can use it
>> without pulling in the rest of it, and still not tempt event users to try
>> setting the event's source.
>>
>> public static void setEventSource(GwtEvent<?> event, Object source) {
>>
>> event.setSource(source);
>>
>> }
>>
>
> I don't buy that this is an improvement over just making the method public.
> How do you figure that users will be less tempted to call this than to use
> the instance method?
>
> If it's that big a concern, let's make source an invariant, and make it a
> constructor argument on GwtEvent.
>
I don't have a strong opinion about this. I would have made it a ctor
invariant, but the ability to setSource() is being used by HandlerManager et
al to recycle old event objects, and to switch out the source when refiring
an event (this comes up in Composites that wrap and refire events).
>
>> The second issue was that HasHandlers had the method getHandlers() that
>> returns a HandlerManager. This effectively tightly couples HandlerManager to
>> the use of GwtEvent. It also exposes all event sources' HandlerManagers as
>> part of their public API, which doesn't seem useful and constrains their
>> ability to evolve implementations in the future.
>>
>> My proposal for this problem is as follows:
>>
>> Create a HandlerCollection interface that HandlerManager implements. This
>> makes the use of HandlerManager an implementation detail rather than an API
>> decision.
>>
>>
>> public interface HandlerCollection {
>>
>> boolean isEventHandled(Type<?> type);
>>
>> void fireEvent(GwtEvent<?> event);
>>
>> }
>>
>> I then removed getHandlers() from HasHandlers (this interface is now
>> empty, and could probably be removed as well).
>>
>
> Can you go ahead and remove it?
>
Never mind, it's no longer empty.
This method was being used by methods such as ResizeEvent.fire(). I propose
>> simply requiring fire() to take the HandlerCollection as an argument
>> (DomEvent.fire() already works this way, so it doesn't seem like much of a
>> stretch). This keeps the fire() methods from having to call
>> source.getHandlers(), and allows us to remove HasHandlers.getHandlers()
>> altogether.
>>
>> public static void fire(HandlerCollection handlers, HasResizeHandlers
>> source,
>>
>> int width, int height) { ... }
>>
>>
>> I've attached a patch that implements this change. It touches a lot of
>> files, but the changes are all pretty simple. I also refactored the Event
>> modules slightly, so that it's possible to include only the shared event
>> infrastructure, and just the logical and/or dom events (I also fixed the
>> fact that the dom event module didn't have an explicit inherits of
>> User.gwt.xml (for the Event class -- mostly a technicality since it will
>> always be included practically speaking).
>>
>
> If you upload the patch to Rietveld, I'll be happy to take the review.
>
Done, thanks. See above.
>> One other quick question: Does Widget.isEventHandled() actually get used
>> anywhere? I can't find any references to it. I can see it perhaps being
>> useful for widget implementors, but so far no one does, and it's public,
>> which seems a bit odd.
>>
>
> Please remove it.
>
>>
I asked Emily about it, and it seems the impetus was to allow Composites to
ask their wrapped widgets whether there are any events of the given type
that it might fire, so that the Composite can optionally skip work. The more
I think about it, though, I'm not sure this is legit, because the Composite
owns the wrapped widget anyway, so it should *know* what events it has added
handlers for.
@ecc: Can you clarify where this would be used in practice? I thought I
understood before, but it seems I didn't :)
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
handlers_1_6_r4446_take2.patch
Description: Binary data
