On Fri, Jan 16, 2009 at 11:49 AM, <j...@google.com> wrote: > > http://gwt-code-reviews.appspot.com/2205/diff/43/230 > File user/src/com/google/gwt/event/logical/shared/ValueChangeEvent.java > (right): > > http://gwt-code-reviews.appspot.com/2205/diff/43/230#newcode42 > Line 42: if (source.isEventHandled(TYPE)) { > On 2009/01/15 20:50:33, ecc wrote: > >> On 2009/01/15 20:35:56, rjrjr wrote: >> > Why is this fire method more complicated than all the other ones? No >> > one else > >> > bothers with the isEventHandled check. >> >> I think it was from before we made up our minds on whether to check >> > before > >> creating them. >> > > Simplified. There's no reason to do this check, as the event objects > aren't expensive to create. > > http://gwt-code-reviews.appspot.com/2205/diff/43/221 > File user/src/com/google/gwt/event/shared/HandlerManager.java (right): > > http://gwt-code-reviews.appspot.com/2205/diff/43/221#newcode58 > Line 58: return false; > On 2009/01/15 20:50:33, ecc wrote: > >> It has already been removed. >> > > Confirmed after svn up'ing. > > http://gwt-code-reviews.appspot.com/2205/diff/43/222 > File user/src/com/google/gwt/event/shared/HasHandlers.java (right): > > http://gwt-code-reviews.appspot.com/2205/diff/43/222#newcode33 > Line 33: boolean isEventHandled(Type<?> type); > On 2009/01/15 20:35:56, rjrjr wrote: > >> I'd still just as soon see this method go away, but I'm not passionate >> > about it. > > I'd strongly prefer to make it go away as well, but there is essentially > no way to remove it without breaking the WhateverEvent.fire()'s generic > constraints. The whole thing makes me kind of uncomfortable, but I have > been unable to think of a better solution.
I'm missing something, then. How is isEventHandled required for type safety in FooEvent#fire? > > > http://gwt-code-reviews.appspot.com/2205/diff/43/236 > File user/src/com/google/gwt/user/client/ui/ListenerWrapper.java > (right): > > http://gwt-code-reviews.appspot.com/2205/diff/43/236#newcode93 > Line 93: abstract class ListenerWrapper<T> implements EventHandler { > On 2009/01/15 20:50:33, ecc wrote: > >> I think we might need to rename the inner classes of ListenerWrapper >> > before > >> making it public (having eclipse control-shift T "Tree" bring up >> ListenerWrapper.Tree is not ideal), so we might want to wait until >> > after this > >> patch clears. >> >> On 2009/01/15 20:35:56, rjrjr wrote: >> > Elsewhere we said that we would make this class public (and >> > deprecated), as > >> lots >> > of people are in the same boat we are wrt event conversion. Might as >> > well make > >> > that change in this patch. >> >> >> > So I'll leave this alone for now. > > http://gwt-code-reviews.appspot.com/2205/diff/43/236#newcode531 > Line 531: // We know that Widget's implementation of getHandlers() > returns an instance > On 2009/01/15 20:35:56, rjrjr wrote: > >> stale comment >> > > Done. > > http://gwt-code-reviews.appspot.com/2205/diff/43/217 > File user/test/com/google/gwt/user/client/ui/DisclosurePanelTest.java > (right): > > http://gwt-code-reviews.appspot.com/2205/diff/43/217#newcode82 > Line 82: public void testEvents() { > On 2009/01/15 20:35:56, rjrjr wrote: > >> This is not a good test, as getHandlerManager is not a public method. >> > If we're > >> testing that handlers are actually added, we should do so via public >> methods--add a handler, call fireEvent, see that the handler is >> > called. > > If you don't have any strong objection, I'd like to separate these > changes into a separate issue/patch. The only reason this file is > touched is to fix it relative to other changes. > > http://gwt-code-reviews.appspot.com/2205/diff/43/217#newcode130 > Line 130: assertEquals(3, > panel.getHandlerManager().getHandlerCount(OpenEvent.getType())); > On 2009/01/15 20:35:56, rjrjr wrote: > >> This is a test of non-public HandlerManager masquerading as a test of >> DisclosurePanel. Should just get rid of these two lines. >> > > Ditto. > > http://gwt-code-reviews.appspot.com/2205/diff/43/217#newcode147 > Line 147: assertEquals(2, > panel.getHandlerManager().getHandlerCount(OpenEvent.getType())); > On 2009/01/15 20:35:56, rjrjr wrote: > >> ditto >> > > Double ditto. > > > http://gwt-code-reviews.appspot.com/2205 > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
