I believe I've addressed what should be addressed. What say you?
http://gwt-code-reviews.appspot.com/841804/diff/1/4
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/4#newcode185
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java:185:
void onSelectionChange(@SuppressWarnings("unused") SelectionChangeEvent
event) {
Untrue, this isn't actually a handler method. Removing the unused param,
tweaking the method name.
http://gwt-code-reviews.appspot.com/841804/diff/1/7
File samples/expenses/pom.xml (right):
http://gwt-code-reviews.appspot.com/841804/diff/1/7#newcode10
samples/expenses/pom.xml:10: <roo.version>1.1.0.M4</roo.version>
Accidental, thanks.
http://gwt-code-reviews.appspot.com/841804/diff/1/18
File user/src/com/google/gwt/event/logical/shared/ValueChangeEvent.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/18#newcode106
user/src/com/google/gwt/event/logical/shared/ValueChangeEvent.java:106:
@SuppressWarnings({"unchecked", "rawtypes"})
Sounds like you should upgrade to 3.6?
On 2010/09/09 17:45:48, bobv wrote:
My eclipse does not know about this warning, and therefore will
display a
warning.
http://gwt-code-reviews.appspot.com/841804/diff/1/19
File user/src/com/google/gwt/event/shared/AbstractEventBus.java (right):
http://gwt-code-reviews.appspot.com/841804/diff/1/19#newcode1
user/src/com/google/gwt/event/shared/AbstractEventBus.java:1: package
com.google.gwt.event.shared;
On 2010/09/09 17:45:48, bobv wrote:
Copyright?
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/19#newcode8
user/src/com/google/gwt/event/shared/AbstractEventBus.java:8: public
abstract class AbstractEventBus implements EventBus {
Not crazy about even more boilerplate.
Should I even bother with the interface?
On 2010/09/09 17:45:48, bobv wrote:
Declare unimplemented methods as abstsract so subclasses can use
@Override in
1.5 language.
http://gwt-code-reviews.appspot.com/841804/diff/1/21
File user/src/com/google/gwt/event/shared/EventBus.java (right):
http://gwt-code-reviews.appspot.com/841804/diff/1/21#newcode40
user/src/com/google/gwt/event/shared/EventBus.java:40: * .
On 2010/09/09 17:45:48, bobv wrote:
Dangling period.
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/21#newcode66
user/src/com/google/gwt/event/shared/EventBus.java:66: * source means
that this is a global handler, which shouldreceive all events
On 2010/09/09 17:45:48, bobv wrote:
should receive
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/24
File user/src/com/google/gwt/event/shared/SimpleEventBus.java (right):
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode36
user/src/com/google/gwt/event/shared/SimpleEventBus.java:36: // Add and
remove operations received during dispatch.
On 2010/09/09 17:56:32, bobv wrote:
Javadoc comment?
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode45
user/src/com/google/gwt/event/shared/SimpleEventBus.java:45:
SimpleEventBus(boolean fireInReverseOrder) {
On 2010/09/09 17:56:32, bobv wrote:
Document use?
Wrote small novel.
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode52
user/src/com/google/gwt/event/shared/SimpleEventBus.java:52: assert
handler != null : "Cannot add a null handler";
On 2010/09/09 17:56:32, bobv wrote:
Assertions or IllegalArgumentExceptions for public APIs?
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode66
user/src/com/google/gwt/event/shared/SimpleEventBus.java:66: public <H
extends EventHandler> void removeHandler(
On 2010/09/09 17:56:32, bobv wrote:
Why make this public?
The registration object is basically impossible to use from within the
handler it applies to. The community raised holy hell when we tried to
remove it last time around, and John and I were unable to come up with a
palatable work around.
Short answer: Java still sucks.
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode109
user/src/com/google/gwt/event/shared/SimpleEventBus.java:109:
event.setSource(source);
On 2010/09/09 17:56:32, bobv wrote:
firingDepth could become negative if this throws an NPE.
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode120
user/src/com/google/gwt/event/shared/SimpleEventBus.java:120: for (int i
= count - 1; i >= 0; i--) {
On 2010/09/09 17:56:32, bobv wrote:
Replace the two for loops with a ListIterator?
ListIterator it = isReverseOrder ? list.listIterator(it.size()) :
list.listIterator();
while (isReverseOrder ? it.hasPrevious() : it.hasNext()) {
H handler = isReverseOrder ? it.previous() : it.next();
}
Thanks, couldn't remember where that lives.
http://gwt-code-reviews.appspot.com/841804/diff/1/24#newcode160
user/src/com/google/gwt/event/shared/SimpleEventBus.java:160: + type;
On 2010/09/09 17:56:32, bobv wrote:
This makes it an error to attempt to remove a handler more than once,
which can
be done with the public removeHandler() method.
Fair enough. Can't think of a good reason to be strict about that.
http://gwt-code-reviews.appspot.com/841804/diff/1/25
File user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/25#newcode123
user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java:123:
On 2010/09/09 17:45:48, bobv wrote:
Extra whitespace.
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/26
File user/src/com/google/gwt/requestfactory/client/impl/ProxySchema.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/26#newcode67
user/src/com/google/gwt/requestfactory/client/impl/ProxySchema.java:67:
// Relying on generated code to call only with correct type
On 2010/09/09 17:45:48, bobv wrote:
Can you add an assertion to that effect?
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/28
File
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/28#newcode308
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:308:
method.getName(), interfaceType.getName()));
On 2010/09/09 17:45:48, bobv wrote:
Who isn't running their formatter?
The formatter thinks that's beautiful. Chopped up the string literal for
better flow.
http://gwt-code-reviews.appspot.com/841804/diff/1/29
File
user/src/com/google/gwt/requestfactory/shared/EntityProxyChange.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/29#newcode55
user/src/com/google/gwt/requestfactory/shared/EntityProxyChange.java:55:
TYPE = new Type<EntityProxyChange.Handler<?>>();
On 2010/09/09 17:45:48, bobv wrote:
The rest of the events eagerly initialize the TYPE field so that the
field
reference will inline at the locations where the handlers are added.
You sure? I hope so. I had old info from John L that this lazy
instantiation was necessary to defeat mass clinit()s. Is there perhaps a
problem with public statics or something? Presuming you're right, b/c I
hate the getter.
Most of our events still use this lazy pattern, btw.
http://gwt-code-reviews.appspot.com/841804/diff/1/29#newcode69
user/src/com/google/gwt/requestfactory/shared/EntityProxyChange.java:69:
@SuppressWarnings({"unchecked", "rawtypes"})
nyah nyah
On 2010/09/09 17:45:48, bobv wrote:
rawtypes?
http://gwt-code-reviews.appspot.com/841804/diff/1/29#newcode71
user/src/com/google/gwt/requestfactory/shared/EntityProxyChange.java:71:
public com.google.gwt.event.shared.GwtEvent.Type<Handler<P>>
getAssociatedType() {
On 2010/09/09 17:45:48, bobv wrote:
Use shorter type name here?
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/29#newcode73
user/src/com/google/gwt/requestfactory/shared/EntityProxyChange.java:73:
// field itself does not, so we have to do an unsafe cast here.
On 2010/09/09 17:45:48, bobv wrote:
block comment
Done.
http://gwt-code-reviews.appspot.com/841804/diff/1/36
File
user/test/com/google/gwt/event/logical/shared/LogicalEventsTest.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/36#newcode78
user/test/com/google/gwt/event/logical/shared/LogicalEventsTest.java:78:
@SuppressWarnings("rawtypes") GwtEvent instance) {
On 2010/09/09 17:45:48, bobv wrote:
Where is this coming from?
Eclipse 3.6 warns, and I obey.
http://gwt-code-reviews.appspot.com/841804/diff/1/37
File user/test/com/google/gwt/event/shared/CountingEventBus.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/37#newcode29
user/test/com/google/gwt/event/shared/CountingEventBus.java:29: * for
tests.
On 2010/09/09 17:45:48, bobv wrote:
Put it in a testing package?
Done. Should you do the same w/editor mocks?
http://gwt-code-reviews.appspot.com/841804/diff/1/37#newcode52
user/test/com/google/gwt/event/shared/CountingEventBus.java:52:
counts.put(type, count - 1);
On 2010/09/09 17:45:48, bobv wrote:
This could store values less than one in the map if a handler were
repeatedly
unregistered.
Good, would want to know that in test.
http://gwt-code-reviews.appspot.com/841804/diff/1/38
File user/test/com/google/gwt/event/shared/HandlerManagerTest.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/38#newcode43
user/test/com/google/gwt/event/shared/HandlerManagerTest.java:43:
On 2010/09/09 17:45:48, bobv wrote:
Unnecessary whitespace.
How do I stop the auto formatter from doing that!? Drives me nuts.
http://gwt-code-reviews.appspot.com/841804/diff/1/39
File user/test/com/google/gwt/event/shared/SimpleEventBusTest.java
(right):
http://gwt-code-reviews.appspot.com/841804/diff/1/39#newcode29
user/test/com/google/gwt/event/shared/SimpleEventBusTest.java:29: public
class SimpleEventBusTest extends HandlerTestBase {
On 2010/09/09 17:45:48, bobv wrote:
If a handler is added multiple times, will it receive the same event
more than
once?
Yes, as has always been the case.
http://gwt-code-reviews.appspot.com/841804/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
