Another sanity check, still sane. I haven't looked in as much depth as I
should, but I'm not scared.


http://gwt-code-reviews.appspot.com/767801/diff/14001/2014
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java
(right):

http://gwt-code-reviews.appspot.com/767801/diff/14001/2014#newcode101
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:101:
topLevelDelegate.initialize(null, personEditor);
This is the EditorDelegate, right? We need a better name for that thing.
Maybe Editor itself is wrong. Don't have alternatives in mind, but let's
keep thinking.

http://gwt-code-reviews.appspot.com/767801/diff/14001/2014#newcode112
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:112:
topLevelDelegate.edit(requestFactory.personRequest().persist(person));
I always imagined the delegate being an implementation detail of the top
level editor, and that setValue() would make this edit() call. If that's
impractical (you said it interferes with composability, right?), that's
what makes me talk about better naming.

The delegate already knows about personEditor, right? Should edit() call
setValue, rather than making the user code do both?

http://gwt-code-reviews.appspot.com/767801/diff/14001/2014#newcode116
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:116:
personEditor.setDelegate(topLevelDelegate);
Odd that I have to point them at each other:

topLevelDelegate.initialize(null, personEditor);
personEditor.setDelegate(topLevelDelegate);

http://gwt-code-reviews.appspot.com/767801/diff/14001/2024
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AddressEditorDelegate.java
(right):

http://gwt-code-reviews.appspot.com/767801/diff/14001/2024#newcode31
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/AddressEditorDelegate.java:31:
public HandlerRegistration subscribe(AddressProxy person) {
Need to be careful with this. If I'm actually editing a copy of the
thing, I don't necessarily want my work clobbered by stray update events
to the canonical one.

http://gwt-code-reviews.appspot.com/767801/diff/14001/2026
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/PersonRequestFactoryDelegate.java
(right):

http://gwt-code-reviews.appspot.com/767801/diff/14001/2026#newcode67
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/gen/PersonRequestFactoryDelegate.java:67:
checkDelegate();
could make this ensureDelegate() and have it return the delegate, or
just getDelegate(). Slightly prettier: return
ensureDelegate().getPath().

http://gwt-code-reviews.appspot.com/767801/diff/14001/2050
File
user/src/com/google/gwt/editor/client/RequestFactoryEditorDelegate.java
(right):

http://gwt-code-reviews.appspot.com/767801/diff/14001/2050#newcode37
user/src/com/google/gwt/editor/client/RequestFactoryEditorDelegate.java:37:
* instance.flush().fire(new Receiver {...});
Is RFED really in the fire() business?

http://gwt-code-reviews.appspot.com/767801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to