The general idea of decoupling the handler manager from the actual events
seems like a great goal, and I think we are on the right track, a couple of
problems with the current implementation...
-------------------------------
Removing getHandlers() unforutnately makes firing most logical events
type-unsafe. We can decouple the handler manager by making getHandlers()
return the HandlerCollection interface however in order to avoid this
problem.
------------------------------
As a user, within my handler, I expect to be able to safely call all methods
on my event. The user, however, cannot safely call setSource and have it
work, but it will "appear" to work in many circumstances, therefore setting
him/her up for problems later. So having a public setSource() on event seems
like a very bad idea.
For example:
...
MyEvent myEvent = new MyEvent();
*myEvent.setSource(childWidget);*
* delegateEvent(childWidget, myEvent)*
seems like valid code but is actually incorrect and prevents us from killing
the event at the correct time.
Making the source an invariant would mean we could no longer cache all of
our dom events and would have to remove the currently supported ability to
delegate events from one widget to another.
Now, personally, I would prefer to have the access method on another class
altogether, as it is still a bit too easy to find on HandlerManager.
However, I would have absolutely no sympathy for a user who tried to
something like this and used a method off of a EventDispatchUtil class.
--------------------------------
Assume we have a logical event that is expensive to create (maybe it needs
server information) named ExpensiveEvent.
A composite widget may/may not have to create an ExpensiveEvent on some user
state change, depending upon whether it or any of its children have an
ExpensiveHandler.
isEventHandled() allows the widget to know if it needs to create an
expensive event or not.
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.
>
>>
>> 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?
>
>
>> 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.
>
>>
>> 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.
>
>>
>> The patch is not 100% tested yet, but I wanted to get feedback before
>> taking it any further. This may seem like a pedantic exercise, but I think
>> it's important to start taking steps to carefully decouple modules sooner
>> rather than later.
>>
>> Cheers,
>> joel.
>>
>>
>
--
"There are only 10 types of people in the world: Those who understand
binary, and those who don't"
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
