http://gwt-code-reviews.appspot.com/2205/diff/1/6 File user/src/com/google/gwt/event/Event.gwt.xml (right):
http://gwt-code-reviews.appspot.com/2205/diff/1/6#newcode2 Line 2: <source path="shared"/> On 2009/01/15 14:53:37, ecc wrote: > In almost all other cases in gwt, the module corresponding to the package name > included everything. What about renaming this module to EventInfrastructure or > EventBase if that is no longer true? The way things were before, it was impossible to take only the Event module, without picking up both the dom and logical packages. That would make this entire exercise pointless, because you wouldn't be able to reuse common event infrastructure without picking up a dependency on User. The only solutions to this are (a) what we have here or (b) to get rid of the event package hierarchy, which would leave us with something like: gwt.event.Event gwt.domevent.DomEvent gwt.logicalevent.LogicalEvent I don't care a great deal, but having a structure that makes it impossible to pick out the modules you want is unacceptable. http://gwt-code-reviews.appspot.com/2205/diff/1/8 File user/src/com/google/gwt/event/shared/HandlerCollection.java (right): http://gwt-code-reviews.appspot.com/2205/diff/1/8#newcode23 Line 23: */ On 2009/01/15 14:53:37, ecc wrote: > wording seems slightly awkward, can you rephrase? Something like "Contains a > collection of handlers, exposing the ability ... It doesn't actually *contain* a collection of handlers, but I've reworded it a bit anyway. http://gwt-code-reviews.appspot.com/2205/diff/1/7 File user/src/com/google/gwt/event/shared/HandlerManager.java (right): http://gwt-code-reviews.appspot.com/2205/diff/1/7#newcode307 Line 307: On 2009/01/15 14:53:37, ecc wrote: > would prefer to have this method on a separate util class that is then javadoc'd > to be only used by people implementing new handler collections > (EventManagementUtil maybe?) > Done. http://gwt-code-reviews.appspot.com/2205/diff/1/7#newcode365 Line 365: HandlerManager.this.removeHandler((Type<EventHandler>) type, handler); On 2009/01/14 22:50:06, rjrjr wrote: > Why is this an improvement over DefaultHandlerRegistration? We have arguably > less type safety, and no fewer classes. Not sure I follow -- DefaultHandlerRegistration assumed and required the use of HandlerManager, so you couldn't use them separately. All it appeared to do was implement the semantics of an inner class with a lot more typing (i.e., it had manually written ctor and fields that are just copies of now-final locals (HandlerManager.this, type, handler), and both implementations of removeHandler() contain the exact same typecast. I can't figure out a single reason for separating it into a top-level class. http://gwt-code-reviews.appspot.com/2205/diff/1/24 File user/src/com/google/gwt/user/client/ui/Widget.java (left): http://gwt-code-reviews.appspot.com/2205/diff/1/24#oldcode53 Line 53: public final HandlerManager getHandlers() { On 2009/01/15 15:01:08, rjrjr wrote: > On 2009/01/15 14:53:37, ecc wrote: > > I think that would be a much larger breaking change if we did not override the > > return type here. > > I think it's too soon to think that way with this code. All of its users are > early adopters who haven't even had access to it in an RC, or internal users > whom we're perfectly capable of fixing. > > Narrowing the type is the odd thing to do, and if we don't have a compelling > reason to do it, we shouldn't. IMHO I'm with Ray on this one -- I originally had tightened it down to HandlerManager because it wasn't an implementation of an interface method. But there's no need for the covariance here, because widgets never use it directly. I've loosened the type back to HandlerCollection, which feels a lot cleaner (I had to add one typecast to ListenerWrapper, but that's a temporary, deprecated glue class, so that feels ok to me). BTW, I dropped the javadoc because it's a trivial implementation of an interface method. There's no reason to specialize the documentation, and it will get picked up from the interface anyway. http://gwt-code-reviews.appspot.com/2205/diff/1/21 File user/src/com/google/gwt/user/datepicker/client/DateBox.java (right): http://gwt-code-reviews.appspot.com/2205/diff/1/21#newcode443 Line 443: DateChangeEvent.fireIfNotEqualDates(getHandlers(), this, oldDate, date); On 2009/01/15 14:53:37, ecc wrote: > I think this was a remnant from the previous patch. Thanks. Removed. http://gwt-code-reviews.appspot.com/2205/diff/1/20 File user/src/com/google/gwt/user/datepicker/client/DateChangeEvent.java (right): http://gwt-code-reviews.appspot.com/2205/diff/1/20#newcode42 Line 42: HandlerCollection handlers, S source, Date oldValue, Date newValue) { On 2009/01/15 14:53:37, ecc wrote: > I think this is a remnant from the previous patch. Yup. Removed. http://gwt-code-reviews.appspot.com/2205/diff/1/3 File user/test/com/google/gwt/user/client/WindowTest.java (right): http://gwt-code-reviews.appspot.com/2205/diff/1/3#newcode191 Line 191: ResizeEvent.fire(Window.handlers, Window.handlers, 0, 0); On 2009/01/14 22:50:06, rjrjr wrote: > this won't compile... Whoops, diff update garbage. Removed. http://gwt-code-reviews.appspot.com/2205 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
