http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java
File user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java
(right):
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java#newcode27
user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java:27:
public abstract class AbstractUiRenderer implements UiRenderer {
On 2011/09/02 00:01:48, rjrjr wrote:
This should be in an impl package. I've long regretted not doing that
with
UiBinderUtil (and bonus points if you move it there too).
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java#newcode34
user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java:34:
public abstract static class UiRendererDispatcher<T> implements
HasHandlers {
On 2011/09/02 00:01:48, rjrjr wrote:
Can this be protected?
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java#newcode43
user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java:43:
private int methodIndex;
On 2011/09/02 00:01:48, rjrjr wrote:
Are these going to get clobbered on recursive calls?
Yes, but that is fine. After the UiHandle'd mehtod is called these
values are not used again.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java#newcode72
user/src/com/google/gwt/uibinder/client/AbstractUiRenderer.java:72:
table = UiRendererUtilsImpl.buildDispatchMap(keys, values);
On 2011/09/02 00:01:48, rjrjr wrote:
Are you rebuilding this table for each event dispatch? If so, sounds
slow. Or
does the dom hold on to a pointer to this thing?
Each UiRenderer object holds a reference(s) to its dispatcher(s). Oh! I
forgot to make dispatcher refs static, I see.
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java
File user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java
(right):
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java#newcode29
user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java:29:
public class UiRendererUtilsImpl {
On 2011/09/02 00:01:48, rjrjr wrote:
Can all these public static things instead be protected members on
AbstractUiRenderer?
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java#newcode31
user/src/com/google/gwt/uibinder/client/UiRendererUtilsImpl.java:31:
public static final int NO_HANDLER_FOUND = -1;
On 2011/09/02 00:01:48, rjrjr wrote:
do these constants really need to be public?
Done. This one can be private. The other two are needed for generation
and at run time so they need to be accessible.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode192
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:192:
List<JMethod> methods = new
ArrayList<JMethod>(Arrays.asList(type.getMethods()));
On 2011/09/02 00:01:48, rjrjr wrote:
this won't work with superclasses. use
JClassType.getInheritableMethods(), and
should add a unit test for that case.
Ah, that pesky Java feature, inheritance. :D
Done. It also affected render()'ing.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode244
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:244:
JMethod[] allMethods = type.getMethods();
On 2011/09/02 00:01:48, rjrjr wrote:
ditto. Should do a general search for getMethods
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode414
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:414: throw
new RuntimeException("UiRenderer is not a parametrizable type in " +
binderType);
On 2011/09/02 00:01:48, rjrjr wrote:
parameterizable
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1215
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1215:
formatMethodError(targetType, jMethod), e.getMessage());
On 2011/09/06 16:44:11, rjrjr wrote:
Could you consolidate all these identical die calls into a single
private
method?
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1465
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1465: *
Validates each {@code eventMethod} (e.g. {@code onBrowserEvent()}).
On 2011/09/02 00:01:48, rjrjr wrote:
(e.g. {@code void onBrowserEvent(HandlerType o, NativeEvent e, Element
parent, A
a, B b, ...);})
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1470
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1470: *
(if it has any methods annotated with {@code @UiHandler})
On 2011/09/02 00:01:48, rjrjr wrote:
d/if it has/
Done.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1515
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1515: * <li>
The second parameter must be of type {@link
com.google.gwt.dom.client.Element Element}
On 2011/09/02 00:01:48, rjrjr wrote:
What do you think about making the element type optional too? I'd be
okay with
requiring it to be in place if they want to use other optional
parameters.
SGTM. Done. Now handler params after the first one are optional.
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode2140
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:2140: //
ClickEvent.getType();
On 2011/09/02 00:01:48, rjrjr wrote:
Should this happen lazily the first time, say, computeDispatchEvent is
called?
AbstractUiRenderer could have an abstract initEventTypes() method.
Made it happen right after the dispatch table is initialized. This
happens lazily upon a call to onBrowserEvent
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/test/com/google/gwt/uibinder/test/UiJavaResources.java
File user/test/com/google/gwt/uibinder/test/UiJavaResources.java
(right):
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/test/com/google/gwt/uibinder/test/UiJavaResources.java#newcode69
user/test/com/google/gwt/uibinder/test/UiJavaResources.java:69:
code.append("public class ClickEvent extends DomEvent<ClickHandler>
{\n");
On 2011/09/02 00:01:48, rjrjr wrote:
Oops. Bet that was fun to track down.
Yes! But not as fun as realizing the need to call XEvent#getType() to
initialize type names. :)
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/test/com/google/gwt/uibinder/test/client/UiRendererEventsTest.java
File
user/test/com/google/gwt/uibinder/test/client/UiRendererEventsTest.java
(right):
http://gwt-code-reviews.appspot.com/1534806/diff/1/user/test/com/google/gwt/uibinder/test/client/UiRendererEventsTest.java#newcode233
user/test/com/google/gwt/uibinder/test/client/UiRendererEventsTest.java:233:
private native NativeEvent createMockNativeEvent(Element target, String
type) /*-{
On 2011/09/02 00:01:48, rjrjr wrote:
Have you tested manually, really clicking something in a real browser?
Yes. But I could not figure out how to implement that from a
GwtTestCase.
http://gwt-code-reviews.appspot.com/1534806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
