Being able to pick out only base events is crucial for me, as trying to package the DOM classes in some environments leads to breakage (e.g. Android)-Ray
On Thu, Jan 15, 2009 at 10:45 AM, <j...@google.com> wrote: > > > 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 -~----------~----~----~----~------~----~------~--~---
