LGTM On Tue, Feb 1, 2011 at 6:40 PM, <[email protected]> wrote:
> Patch updated. > > > > http://gwt-code-reviews.appspot.com/1340802/diff/1/2 > File user/src/com/google/gwt/editor/client/EditorContext.java (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/2#newcode114 > user/src/com/google/gwt/editor/client/EditorContext.java:114: * Traverse > a editor created by > On 2011/02/01 18:52:05, rjrjr wrote: > >> an editor >> > > Done. > > > http://gwt-code-reviews.appspot.com/1340802/diff/1/4 > File user/src/com/google/gwt/editor/client/EditorVisitor.java (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/4#newcode27 > user/src/com/google/gwt/editor/client/EditorVisitor.java:27: public <T> > void endVisit(EditorContext<T> ctx) { > Since EditorContext allows access to the Editor<T> it's useful to be > able to say > > editorContext.asLeafValueEditor().setValue(editorConext.getFromModel()); > > and have the types line up. > > http://gwt-code-reviews.appspot.com/1340802/diff/1/8 > File > user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java > (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/8#newcode57 > user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java:57: > Chain(CompositeEditor<T, R, S> composedEditor, Class<R> > composedElementType) { > On 2011/02/01 18:52:05, rjrjr wrote: > >> line too long >> > > Done. > > > http://gwt-code-reviews.appspot.com/1340802/diff/1/8#newcode118 > user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java:118: > public <R, S extends Editor<R>> void createChain(Class<R> > composedElementType) { > On 2011/02/01 18:52:05, rjrjr wrote: > >> Should this be protected? >> > > Done. > > http://gwt-code-reviews.appspot.com/1340802/diff/1/15 > File user/src/com/google/gwt/editor/client/impl/Initializer.java > (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/15#newcode35 > user/src/com/google/gwt/editor/client/impl/Initializer.java:35: // Tell > the EditorDelegate about the object its responsible for > On 2011/02/01 18:52:05, rjrjr wrote: > >> it's >> > > Done. > > > http://gwt-code-reviews.appspot.com/1340802/diff/1/15#newcode42 > user/src/com/google/gwt/editor/client/impl/Initializer.java:42: > HasEditorDelegate<Q> asDelegate = ctx.asHasEditorDelegate(); > On 2011/02/01 18:52:05, rjrjr wrote: > >> asHasDelegate >> > > Done. > > > http://gwt-code-reviews.appspot.com/1340802/diff/1/16 > File user/src/com/google/gwt/editor/client/impl/Refresher.java (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/16#newcode25 > user/src/com/google/gwt/editor/client/impl/Refresher.java:25: * the > editor and delegate hiererchy. > On 2011/02/01 18:52:05, rjrjr wrote: > >> Perhaps you could simplify initializer, and require that it be used in >> > tandem > >> with refresher? Or make it wrap a refresher? Get rid of some >> > redundancy? > > Done. > > http://gwt-code-reviews.appspot.com/1340802/diff/1/17 > File user/src/com/google/gwt/editor/client/impl/RootEditorContext.java > (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/17#newcode42 > user/src/com/google/gwt/editor/client/impl/RootEditorContext.java:42: > public T checkAssignment(Object value) { > On 2011/02/01 18:52:05, rjrjr wrote: > >> Document as unsupported? >> > > Changed to always return value, relying on code-gen in the > implementation of AED.setObject() to throw a ClassCastException. > Documented reasoning. > > http://gwt-code-reviews.appspot.com/1340802/diff/1/24 > File > user/src/com/google/gwt/editor/rebind/AbstractEditorDriverGenerator.java > (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/24#newcode44 > > user/src/com/google/gwt/editor/rebind/AbstractEditorDriverGenerator.java:44: > public abstract class AbstractEditorDriverGenerator extends Generator { > On 2011/02/01 18:52:05, rjrjr wrote: > >> Here's the punchline, eh? Nice to see all that red in the diff. >> > > What code-gen remains has no conditional expressions. It's basically a > straight-through transform applied to the editor model data. > > http://gwt-code-reviews.appspot.com/1340802/diff/1/29 > File > > user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryEditorDelegate.java > (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/29#newcode86 > > user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryEditorDelegate.java:86: > * Must call four-arg version instead. > On 2011/02/01 18:52:05, rjrjr wrote: > >> Are you sure you want the base initialize method to be public, then? >> > > Done. > > http://gwt-code-reviews.appspot.com/1340802/diff/1/36 > File user/test/com/google/gwt/editor/client/EditorErrorTest.java > (right): > > http://gwt-code-reviews.appspot.com/1340802/diff/1/36#newcode137 > user/test/com/google/gwt/editor/client/EditorErrorTest.java:137: > assertEquals(errors.toString(), 2, errors.size()); > On 2011/02/01 18:52:05, rjrjr wrote: > >> That's going to be pretty cryptic when it fails >> > > Done. > > > http://gwt-code-reviews.appspot.com/1340802/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
