[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
-~----------~----~----~----~------~----~------~--~---
