To be clear, all these comments are nits. If you need to submit and
follow up to get to them, that's cool.
On 2010/09/24 21:48:40, rjrjr wrote:
LGTM
As we discussed, putting the new JSO in the master value store feels
skeevey but
should do no harm for this patch. Landing this lets us proceed.
We have uncovered a pretty serious problem with the api, the fact that
requests
are not responsible for their own creates. I'll log a follow up issue
on that.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9002
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/events/EditPersonEvent.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9002#newcode41
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/events/EditPersonEvent.java:41:
public EditPersonEvent(PersonProxy person) {
A bit squirrely that this event is carrying a proxy rather than a
proxyid. Yes,
yes, more async code, more server hits. We will get caching sooner or
later, and
we'd like this app to serve as an example of thorough decoupling.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9006
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9006#newcode185
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java:185:
PersonProxy person = (PersonProxy) response;
@Override, and don't need the cast. Did eclipse not warn?
http://gwt-code-reviews.appspot.com/921801/diff/8001/9006#newcode216
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.java:216:
// XXX add back filter
NC-17
http://gwt-code-reviews.appspot.com/921801/diff/8001/9007
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.ui.xml
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9007#newcode6
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.ui.xml:6:
display: inline;
tab
http://gwt-code-reviews.appspot.com/921801/diff/8001/9007#newcode10
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/SummaryWidget.ui.xml:10:
width: 30%
tab
http://gwt-code-reviews.appspot.com/921801/diff/8001/9009
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9009#newcode71
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java:71:
true, true, true, true, true, true, true};
Storing this per user is pretty funny. Not pushing back — it's a
demo, and we
need to exercise the feature. Merely sharing my amusement.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9009#newcode151
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java:151:
this.address.copyFrom(address);
// TODO: odd to copy an entity this, but we're waiting for value
object support.
Convert when they're ready
http://gwt-code-reviews.appspot.com/921801/diff/8001/9014
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/SchoolCalendarService.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9014#newcode50
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/SchoolCalendarService.java:50:
* XXX Remove this once primitive lists work.
XXX is even worse than TODO?
Thought they work now?
http://gwt-code-reviews.appspot.com/921801/diff/8001/9014#newcode75
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/SchoolCalendarService.java:75:
* XXX private due to inability to add method overloads.
Is there a bug on that? I can't see any excuse for it.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9018
File user/src/com/google/gwt/editor/client/ValueAwareEditor.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9018#newcode41
user/src/com/google/gwt/editor/client/ValueAwareEditor.java:41: *
ValueAwareEditors should preferentially use sub-editors to alter the
They don't have a choice any more, due they?
http://gwt-code-reviews.appspot.com/921801/diff/8001/9021
File
user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9021#newcode202
user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java:202:
protected <Q> Q ensureMutable(Q object) {
javadoc
http://gwt-code-reviews.appspot.com/921801/diff/8001/9026
File
user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9026#newcode114
user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java:114:
private final Map<EntityProxyId<?>, WriteOperation> operations = new
HashMap<EntityProxyId<?>, WriteOperation>();
Thank you. I couldn't get this to work.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9026#newcode218
user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java:218:
ProxyJsoImpl proxy = creates.get(record.stableId());
nit: I've been trying to use the proxyJso name in any var that points
to one.
Newcomers are very confused by the ProxyImpl-wraps-ProxyJsoImpl thing.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9026#newcode237
user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java:237:
ProxyJsoImpl proxy = creates.get(record.stableId());
proxyJso
http://gwt-code-reviews.appspot.com/921801/diff/8001/9026#newcode329
user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java:329:
if (!proxy.unpersisted()) {
I would not be offended if this were reversed and named persisted.
http://gwt-code-reviews.appspot.com/921801/diff/8001/9030
File user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9030#newcode76
user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java:76:
* and they
are from the same Request object.
Should document this on EntityProxy.java
http://gwt-code-reviews.appspot.com/921801/diff/8001/9030#newcode79
user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java:79:
public
boolean equals(Object o) {
need null check on o. Does instanceof take care of that?
http://gwt-code-reviews.appspot.com/921801/diff/8001/9030#newcode197
user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java:197:
this.request = request;
Why have pointers to both the request and the dvs? They're 1:1, no?
http://gwt-code-reviews.appspot.com/921801/diff/8001/9031
File
user/src/com/google/gwt/requestfactory/client/impl/ProxyJsoImpl.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9031#newcode251
user/src/com/google/gwt/requestfactory/client/impl/ProxyJsoImpl.java:251:
return
(V) toReturn;
it's a shame you couldn't fit in a few more calls to split
http://gwt-code-reviews.appspot.com/921801/diff/8001/9033
File
user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryJsonImpl.java
(right):
http://gwt-code-reviews.appspot.com/921801/diff/8001/9033#newcode120
user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryJsonImpl.java:120:
for (Object proxy : requestData.getProxyParameters()) {
Did you delete the dummy object support code in JsonRequestProcessor?
If you
can't do it now, lmk and I'll file a task.
http://gwt-code-reviews.appspot.com/921801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
