Feedback implemented. @RayC, I'll look at your patch before answering the question about the @Service validation.
http://gwt-code-reviews.appspot.com/924801/diff/2001/3007 File user/src/com/google/gwt/app/place/AbstractProxyListActivity.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3007#newcode114 user/src/com/google/gwt/app/place/AbstractProxyListActivity.java:114: // Operation.CREATE)); On 2010/10/01 17:49:28, rjrjr wrote:
I'm pretty sure that ID is never used, this is vestigial. Make it null
and I
believe nothing bad will happen.
At the moment this is only exercised by addon-gwt. I don't want to
hold up your
patch getting that fixed, so you'll have to take the fix on faith and
I'll have
to follow up your submit with the banishment patch I've been working
on. Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3008 File user/src/com/google/gwt/app/place/PropertyColumn.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3008#newcode74 user/src/com/google/gwt/app/place/PropertyColumn.java:74: public String getValue(R object) { On 2010/10/01 17:49:28, rjrjr wrote:
Indeed, a smelly hack and a loophole the size of a barn door that
would lead to
a generation of unoptimizable GWT apps.
As we discussed offline, delete this method and have its uses in samples/expenses implement it as needed.
But *don't* eliminate the property name stuff. Despite the DRY
violation, that's
needed downstream by code that fills out the with() clauses when
building
requests
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3010 File user/src/com/google/gwt/editor/client/AutoBean.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3010#newcode21 user/src/com/google/gwt/editor/client/AutoBean.java:21: * PFM. On 2010/10/01 23:31:07, rjrjr wrote:
ahem
It happens to be true. http://gwt-code-reviews.appspot.com/924801/diff/2001/3010#newcode87 user/src/com/google/gwt/editor/client/AutoBean.java:87: * A clone operation only ever produces a shallow copy of its tags, since the On 2010/10/01 23:31:07, rjrjr wrote:
Having trouble squaring this sentence with the deep flag.
Updated the javadoc. All that I really mean to say is newBean.tags = new HashMap(toClone.tags); regardless of deep. http://gwt-code-reviews.appspot.com/924801/diff/2001/3010#newcode98 user/src/com/google/gwt/editor/client/AutoBean.java:98: List<Operation> getOperations(); Removed Operations entirely, since I didn't need them. They were left over from an earlier sketch. http://gwt-code-reviews.appspot.com/924801/diff/2001/3012 File user/src/com/google/gwt/editor/client/AutoBeanUtils.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3012#newcode39 user/src/com/google/gwt/editor/client/AutoBeanUtils.java:39: public static Map<String, Object> diff(AutoBean<?> a, AutoBean<?> b) { Contents of the diff are used in AbstractRequestContext.makePayload(). I need the property names to be able to send them to the server, although going the deRPC route and using Impl.nameOf() would be possible. http://gwt-code-reviews.appspot.com/924801/diff/2001/3016 File user/src/com/google/gwt/editor/rebind/AutoBeanFactoryGenerator.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3016#newcode92 user/src/com/google/gwt/editor/rebind/AutoBeanFactoryGenerator.java:92: { On 2010/10/01 23:31:07, rjrjr wrote:
You're not using these braces for anything, it's all in the for loop
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3023 File user/src/com/google/gwt/editor/rebind/model/ModelUtils.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3023#newcode32 user/src/com/google/gwt/editor/rebind/model/ModelUtils.java:32: public class ModelUtils { On 2010/10/01 23:31:07, rjrjr wrote:
public?
Used by Editor and RF generators. http://gwt-code-reviews.appspot.com/924801/diff/2001/3043 File user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3043#newcode93 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:93: System.out.println(responseText); On 2010/10/01 23:31:07, rjrjr wrote:
oops
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3043#newcode161 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:161: // Really want Class.isInstance() On 2010/10/01 23:31:07, rjrjr wrote:
Hear hear. And isAssignableFrom() while we're at it
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3043#newcode200 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:200: if (receiver != null) { On 2010/10/01 23:31:07, rjrjr wrote:
Should we have a default receiver that throws an RTE on unhandled
violations or
failures? It's awfully easy to forget to provide a callback and then
miss what's
going on when things fail silently.
That's why we previously required the receiver arg in fire, to make
that mistake
much more difficult to make. I don't think I want to bring that back,
but it
seems like we should do something.
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044 File user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode51 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:51: private List<AbstractRequest<?>> invocations = new ArrayList<AbstractRequest<?>>(); On 2010/10/01 23:31:07, rjrjr wrote:
Tastey
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode85 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:85: checkLocked(); On 2010/10/01 23:31:07, rjrjr wrote:
Really do this before checking the seen set? Seems like you'll get nondeterministic fails.
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode94 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:94: if (!bean.isFrozen() && context != null) { On 2010/10/01 23:31:07, rjrjr wrote:
If the bean is frozen and it's a foreigner you're screwed, no?
Created new method to check for this case. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode145 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:145: receiver.onSuccess(null); On 2010/10/01 23:31:07, rjrjr wrote:
NPE on null receiver? (Which problem goes away if you take my default
receiver
suggestion.)
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode148 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:148: locked = true; On 2010/10/01 23:31:07, rjrjr wrote:
Redundant, you locked on fire, but maybe that's fine.
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode152 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:152: receiver.onViolation(errors); On 2010/10/01 23:31:07, rjrjr wrote:
When do you unlock? I thought there was a test to check that I can
re-edit on
violation or failure.
Called from reuse() which is called by AbstractRequest.fail() and AbstractRequest.processViolations(). That test still exists, it caught an error for me. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode170 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:170: return true; On 2010/10/01 23:31:07, rjrjr wrote:
Actually, no please. I used to be able to tell that a newly created
proxy had no
real info added to it, and then let the user cancel without being
nagged. Don't
want to lose that.
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode468 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:468: * Process an arroy of ReturnRecords, delegating to On 2010/10/01 23:31:07, rjrjr wrote:
arroyo
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode472 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:472: WriteOperation... operations) { On 2010/10/01 23:31:07, rjrjr wrote:
I don't think I've seen you pass more than one operation at a time
Done. http://gwt-code-reviews.appspot.com/924801/diff/2001/3051 File user/src/com/google/gwt/requestfactory/client/impl/EntityCodex.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3051#newcode115 user/src/com/google/gwt/requestfactory/client/impl/EntityCodex.java:115: On 2010/10/01 19:45:39, cromwellian wrote:
This doesn't seem like it would handle nulls in collections properly,
since
you'll return "null"
Done. http://gwt-code-reviews.appspot.com/924801/diff/13001/7088 File user/src/com/google/gwt/requestfactory/rebind/model/RequestFactoryModel.java (right): http://gwt-code-reviews.appspot.com/924801/diff/13001/7088#newcode288 user/src/com/google/gwt/requestfactory/rebind/model/RequestFactoryModel.java:288: On 2010/10/02 00:50:20, cromwellian wrote:
This validates that the return type is transportable, but does not
validate that
the parameters are transportable.
Done. http://gwt-code-reviews.appspot.com/924801/diff/13001/7090 File user/src/com/google/gwt/requestfactory/server/FindService.java (right): http://gwt-code-reviews.appspot.com/924801/diff/13001/7090#newcode18 user/src/com/google/gwt/requestfactory/server/FindService.java:18: On 2010/10/02 00:43:03, rjrjr wrote:
Good catch.
Done. http://gwt-code-reviews.appspot.com/924801/diff/13001/7096 File user/src/com/google/gwt/requestfactory/shared/Request.java (right): http://gwt-code-reviews.appspot.com/924801/diff/13001/7096#newcode31 user/src/com/google/gwt/requestfactory/shared/Request.java:31: * Submit this requests. Failures will be reported through the global uncaught On 2010/10/02 00:43:03, rjrjr wrote:
request, singular
Done. http://gwt-code-reviews.appspot.com/924801/diff/13001/7110 File user/test/com/google/gwt/requestfactory/client/EditorTest.java (right): http://gwt-code-reviews.appspot.com/924801/diff/13001/7110#newcode94 user/test/com/google/gwt/requestfactory/client/EditorTest.java:94: private static final int TEST_TIMEOUT = 500000; On 2010/10/02 00:43:03, rjrjr wrote:
oops
We need a way to see if there's a debugger attached and make this automatically bump the timeout. http://gwt-code-reviews.appspot.com/924801/diff/13001/7114 File user/test/com/google/gwt/requestfactory/client/RequestFactoryStringTest.java (right): http://gwt-code-reviews.appspot.com/924801/diff/13001/7114#newcode22 user/test/com/google/gwt/requestfactory/client/RequestFactoryStringTest.java:22: // XXX find a refactoring so this doesn't wind up a copy-and-paste class On 2010/10/02 00:43:03, rjrjr wrote:
When? If not tonight, should hold your nose and leave this working.
Done. http://gwt-code-reviews.appspot.com/924801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
