I have a lot of feedback here, but I think you should go ahead and submit it as an excellent check point rather than gating it on a protracted design discussion.
http://gwt-code-reviews.appspot.com/767801/diff/37001/38010 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/FavoritesManager.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38010#newcode28 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/FavoritesManager.java:28: * Manages client-side favorites. TODO: track favorites on the server? http://gwt-code-reviews.appspot.com/767801/diff/37001/38013 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38013#newcode80 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:80: private PersonEditorWorkflow(DynaTableRequestFactory requestFactory, I've never understood the Workflow name. Session? Dialog? The only problem here (maybe not for this CL) is that this class is not JRE test friendly. A Wave-style MVP treatment would be: /** View class, passthrough implementation not worth testing */ interface PersonEditorDialog extends IsWidget { interface Delegate { void saveClicked(); void setFavorite(boolean b); // etc. } void hide(); void center(); void setDelegate(Delegate d); // etc. } /** app logic, needs speedy unit test */ PersonEditorSession implements PersonEditorDialog.Delegate { // Fires off requests and events } PES might have a constructor to accept a view and a delegate, and GWT.create them by default. http://gwt-code-reviews.appspot.com/767801/diff/37001/38019 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/events/EditPersonEvent.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38019#newcode25 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/events/EditPersonEvent.java:25: * TODO: Make this an Activity. eh? http://gwt-code-reviews.appspot.com/767801/diff/37001/38019#newcode49 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/events/EditPersonEvent.java:49: public com.google.gwt.event.shared.GwtEvent.Type<Handler> getAssociatedType() { format, and unneeded fqn http://gwt-code-reviews.appspot.com/767801/diff/37001/38022 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AbstractRequestFactoryDriver.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38022#newcode53 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AbstractRequestFactoryDriver.java:53: public void flush() { assert valueAwareEditor != null ? http://gwt-code-reviews.appspot.com/767801/diff/37001/38022#newcode57 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AbstractRequestFactoryDriver.java:57: object = ensureMutable(object); JavaDoc said that this flush is called at the end of the edit session, but if this is when we're making the thing mutable that sounds wrong. http://gwt-code-reviews.appspot.com/767801/diff/37001/38023 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AddressEditorDelegate.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38023#newcode50 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AddressEditorDelegate.java:50: PrimitiveValueEditor<String> valueEditor = (PrimitiveValueEditor<String>) editor.getEditorForPath(STREET); If we're code generating this, do we really need the string based path api and the casts? http://gwt-code-reviews.appspot.com/767801/diff/37001/38025 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/PersonRequestFactoryDriver.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38025#newcode36 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/PersonRequestFactoryDriver.java:36: RequestFactoryEditorDriver<DynaTableRequestFactory, PersonProxy> { I still think you want interfaces for RequestFactory and RequestObject to implement. With those, this code is completely re-usable w/o request factory. If you're not comfortable making RF aware of Editor, put the interfaces outside of either of them. So: interface com.google.gwt.something.Mutator { <T> T ensureMutable(T t); } RequestObject extends Mutator { // maybe rename ensureMutable() to edit() } For that matter, you don't seem to be using the factory at all. But presuming you will, it too could implement an interface found in the something package. Although I *still* think it would be just fine knowing about editor, or perhaps just editor.interfaces http://gwt-code-reviews.appspot.com/767801/diff/37001/38032 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/FavoritesWidget.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38032#newcode40 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/FavoritesWidget.java:40: public class FavoritesWidget extends Composite { Another candidate for MVP separation. Although given how small it is, and that this is a sample, would that be overkill? http://gwt-code-reviews.appspot.com/767801/diff/37001/38034 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/NameLabel.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38034#newcode61 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/NameLabel.java:61: public Editor<?> getEditorForPath(String path) { Seems like code generated EditorDriver could simply calculate its way through all of this, provided it had access to appropriate getters or fields. What justifies the runtime api? Seems very out of character ;-) I'm not going to push hard on this point, as I suspect you have a real world use case in mind, just would like it articulated. http://gwt-code-reviews.appspot.com/767801/diff/37001/38036 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/PersonEditor.ui.xml (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38036#newcode3 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/PersonEditor.ui.xml:3: <ui:style src="../common.css"> you sure ../ will work in prod? http://gwt-code-reviews.appspot.com/767801/diff/37001/38037 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38037#newcode49 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java:49: * A paging table with summaries of all known people. Another spot that really should be MVP separated. I'm thinking this MVP pass might be my job, if it happens. Might only do that in the expenses sample. http://gwt-code-reviews.appspot.com/767801/diff/37001/38039 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Address.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38039#newcode66 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Address.java:66: * already if we forget to update this comment). This is already true. You can delete any of these persist methods that you're not actually using. Note that we *will not* be providing a generalized persist mechanism. It remains a developer responsibility to figure out just how they make things stick to the server. http://gwt-code-reviews.appspot.com/767801/diff/37001/38040 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38040#newcode31 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java:31: * should get more flexible soon. yes. http://gwt-code-reviews.appspot.com/767801/diff/37001/38040#newcode97 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java:97: public void persist() { Again, can delete if it's not being used. Even if it is being used, should lose the comment. E.g., if you want to implement a cascading persist, you can do so. http://gwt-code-reviews.appspot.com/767801/diff/37001/38061 File user/src/com/google/gwt/editor/client/ValueAwareEditor.java (right): http://gwt-code-reviews.appspot.com/767801/diff/37001/38061#newcode28 user/src/com/google/gwt/editor/client/ValueAwareEditor.java:28: public interface ValueAwareEditor<T> extends Editor<T> { ValueEditor would be more concise... http://gwt-code-reviews.appspot.com/767801/diff/37001/38061#newcode32 user/src/com/google/gwt/editor/client/ValueAwareEditor.java:32: * to flush their sub-editors. But what should they do when it is called? http://gwt-code-reviews.appspot.com/767801/diff/37001/38061#newcode45 user/src/com/google/gwt/editor/client/ValueAwareEditor.java:45: void setDelegate(EditorDelegate<T> delegate); ...so that this could be a ValueEditorDelegate. http://gwt-code-reviews.appspot.com/767801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
