[google-web-toolkit] [EMAIL PROTECTED] commented on branch /branches/1_6_clean_events. Details are at http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events
Line-by-line comments: File: /branches/1_6_clean_events/user/src/com/google/gwt/event/logical/shared/BeforeSelectionEvent.java (r3969) =============================================================================== Line 42: * @param item the item ------------------------------------------------------------------------------- Yep, I think we can in general. Line 48: HandlerManager handlers = source.getHandlers(); ------------------------------------------------------------------------------- Nope, it is perfectly legitimate to call fire here with no handlers, will add comment to that effect. Line 77: */ ------------------------------------------------------------------------------- Also used for subclasses though, so I'd hate to lie. How about just "Create a new before selection event. Line 114: @Override ------------------------------------------------------------------------------- The problem is actually not the H type, that one works perfectly and prevents anyone from adding a handler receiver of the wrong type, which is good because we use an unsafe cast to dispatch it. The problem is the actual selected item wild car type. I'll try to add a better comment. File: /branches/1_6_clean_events/user/src/com/google/gwt/event/logical/shared/SelectionEvent.java (r3969) =============================================================================== Line 38: * @param source the source of the handlers. Must have selection handlers and ------------------------------------------------------------------------------- yep. Line 67: ------------------------------------------------------------------------------- Not long term, as selection events may also be recycled. File: /branches/1_6_clean_events/user/src/com/google/gwt/event/shared/DefaultHandlerRegistration.java (r3987) =============================================================================== Line 24: ------------------------------------------------------------------------------- We figured we could optimize this later, as we'd still want to give people a default version that is not an inner class. Line 27: private Type<?> type; ------------------------------------------------------------------------------- Yep. File: /branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java (r4006) =============================================================================== Line 41: return event.isLive(); ------------------------------------------------------------------------------- So people can create aux. event management systems. However, the consumers for the events should not care about isLive() so the method is hidden from them. Line 50: if (event.isLive() == false) { ------------------------------------------------------------------------------- See above comment for why make the public version exists, same logic. Line 60: } ------------------------------------------------------------------------------- With the two registries moving inside the handler manager, does this still bother you? Line 65: // Only one of JsHandlerRegistry and JavaHandlerRegistry are live at once. ------------------------------------------------------------------------------- yes, they are parts. Only reason they are not in the class is because of size, we can move them. Line 77: ------------------------------------------------------------------------------- Yep, so either we need full command objects or copy on concurrent write. Line 87: } else { ------------------------------------------------------------------------------- It seemed to be removing it when I inspected the code, but that may be fragile, so I agree that we should not touch it instead. Line 134: if (useJs) { ------------------------------------------------------------------------------- widgets used to be able to simply throw away their listener collections away. Reproducing that functionality here. Line 136: } else { ------------------------------------------------------------------------------- This clears the handlers of a single type, not all handlers in the system. Line 151: revive(event); ------------------------------------------------------------------------------- revive can be overridden for event specific clearing, this is a speed optimization. Line 228: final H handler) { ------------------------------------------------------------------------------- Not really possible because it would make the listener wrappers a lot more inefficient. File: /branches/1_6_clean_events/user/src/com/google/gwt/i18n/rebind/AbstractLocalizableImplCreator.java (r3963) =============================================================================== Line 19: ------------------------------------------------------------------------------- From the 1.6 release branch. File: /branches/1_6_clean_events/user/src/com/google/gwt/user/client/ListenerWrapper.java (r4020) =============================================================================== Line 37: * @param <T> listener type ------------------------------------------------------------------------------- Nope, thanks! Line 41: public static class HistoryChange extends ListenerWrapper<HistoryListener> implements ------------------------------------------------------------------------------- The reason I avoided ListenerAdaptor was just because I was worried it would be confused with are ClickListenerAdaptor etc. which is unrelated, but darn it is a better name! In terms of removing HandlerManager.removeHandler, unfortunately not easily without making the wrapper much less memory efficient and harder for others to convert their listener classes. The problem is that for listeners, we have no idea when someone will call removeListener, so the only way to do this would be to keep all removers in the system. Respond to these comments at http://code.google.com/p/google-web-toolkit/source/branch?spec=issue3083&branch=%2Fbranches%2F1_6_clean_events -- You received this message because you starred this review, or because your project has directed all notifications to a mailing list that you subscribe to. You may adjust your review notification preferences at: http://code.google.com/hosting/settings --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---