http://gwt-code-reviews.appspot.com/1299801/diff/1/2
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/PersonEditor.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/2#newcode59
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/PersonEditor.java:59:
public PersonEditor(DynaTableRequestFactory factory) {
On 2011/01/21 02:54:31, rjrjr wrote:
If we're following the DI pattern, ScheduleEditor should be a
constructor arg
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/4
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/ScheduleEditor.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/4#newcode41
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/ScheduleEditor.java:41:
public ScheduleEditor(DynaTableRequestFactory factory) {
On 2011/01/21 02:54:31, rjrjr wrote:
timeSlots should be a constructor arg
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/5
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/ScheduleEditor.ui.xml
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/5#newcode6
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/ScheduleEditor.ui.xml:6:
/* Add CSS here. See the GWT docs on UI Binder for more details */
On 2011/01/21 02:54:31, rjrjr wrote:
This comment is noise, I think.
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/5#newcode7
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/ScheduleEditor.ui.xml:7:
.important {
On 2011/01/21 02:54:31, rjrjr wrote:
Does anything use this style?
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/7
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/7#newcode64
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.java:64:
class ScheduleRow {
On 2011/01/21 02:54:31, rjrjr wrote:
why not private?
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/7#newcode76
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.java:76:
for (TimeSlotProxy timeSlot : backing) {
On 2011/01/21 02:54:31, rjrjr wrote:
This is kind of nasty. Could you make an immutable TimeSlotKey object
that
implemented this comparison in its equals, and keep a map of
TimeSlotKey to
TimeSlotProxy?
Done. Also kept track of requested TimeSlotProxys to minimize server
requests.
http://gwt-code-reviews.appspot.com/1299801/diff/1/7#newcode96
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.java:96:
context.createTimeSlot(day.ordinal(), hour * 60, hour * 60 +
50).fire(new Receiver<TimeSlotProxy>() {
On 2011/01/21 02:54:31, rjrjr wrote:
What happens on redundant request, if the user clicks a few times
before the
response is received?
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/7#newcode118
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.java:118:
SATURDAY("S");
On 2011/01/21 02:54:31, rjrjr wrote:
Two character short names would avoid ambiguity
Done. Also improved formatting for the hour column.
http://gwt-code-reviews.appspot.com/1299801/diff/1/8
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.ui.xml
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/8#newcode6
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.ui.xml:6:
/* Add CSS here. See the GWT docs on UI Binder for more details */
On 2011/01/21 02:54:31, rjrjr wrote:
ditto
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/8#newcode7
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/widgets/TimeSlotListWidget.ui.xml:7:
.important {
On 2011/01/21 02:54:31, rjrjr wrote:
ditto
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/9
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/9#newcode154
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Person.java:154:
* to supply methods required by RequestFactory.
On 2011/01/21 02:54:31, rjrjr wrote:
This comment was screaming out "delete me!" If the persist method is
no longer
used, just kill it. If it is still used, please get rid of the "When
this was
written..." stuff, nobody cares about our history.
And the "See...to supply methods required..." stuff won't make sense
here since
this method is not a requirement.
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/10
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Schedule.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/10#newcode25
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Schedule.java:25:
* {@code getVersion()} nor {@code findSchedule()}. Those methods
On 2011/01/21 02:54:31, rjrjr wrote:
This entity does not follow the usual pattern of providing getId(),
getVersion()
and findSchedule() methods for RequestFactory's use. ScheduleLocator
handles
those responsibilities instead.
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/10#newcode26
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Schedule.java:26:
* are provided by {@link ScheduleLocator} instead.
On 2011/01/21 02:54:31, rjrjr wrote:
This link won't work, it's not in the same package.
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/10#newcode85
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/domain/Schedule.java:85:
this.timeSlots = timeSlots;
On 2011/01/21 02:54:31, rjrjr wrote:
should make a defensive copy
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/12
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/PersonFuzzer.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/12#newcode30
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/PersonFuzzer.java:30:
static final int MAX_PEOPLE = 100;
On 2011/01/21 02:54:31, rjrjr wrote:
Might as well be public
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/12#newcode75
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/PersonFuzzer.java:75:
if (rnd.nextInt(STUDENTS_PER_PROF) == 0) {
Comparing to 1 is arguably a minor bug as the random number goes from 0
to N - 1. But you are right in that I did not fix the problem
completely. What we want is numbers from 0 to STUDENTS_PER_PROF,
inclusive.
On 2011/01/21 02:54:31, rjrjr wrote:
Why?
http://gwt-code-reviews.appspot.com/1299801/diff/1/14
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleFuzzer.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/14#newcode2
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleFuzzer.java:2:
* Copyright 2007 Google Inc.
On 2011/01/21 02:54:31, rjrjr wrote:
2011
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/15
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleLocator.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/15#newcode2
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleLocator.java:2:
* Copyright 2010 Google Inc.
On 2011/01/21 02:54:31, rjrjr wrote:
2011
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/15#newcode28
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleLocator.java:28:
* A Schedule locator that interfaces between DTOs and Request Factory.
On 2011/01/21 02:54:31, rjrjr wrote:
This class serves as an example of implementing a Locator to allow
RequestFactory to work with entities that don't conform to its
expectations of
static find*() methods, and getId() and getVersion() methods. In a
production
application such a Locator might be the bridge to your dependency
injection
framework, or a data access object.
<p>
There is a reference to this class in a {@literal @}Service annotation
in {@link
...DynaTableRequestFactory}
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/15#newcode30
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleLocator.java:30:
public class ScheduleLocator extends Locator<Schedule, String> {
On 2011/01/21 02:54:31, rjrjr wrote:
I think the example would be clearer if you refactored a
ScheduleSource out of
the ScheduleLocator, something analogous to PersonSource.
ScheduleCalendarService would know about ScheduleSource, not
ScheduleLocator.
The locator would be a very simple thing, one step up from a
configuration file.
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/16
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleService.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/16#newcode21
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleService.java:21:
* Service object for Schedule entities.
On 2011/01/21 02:54:31, rjrjr wrote:
, used to demonstrate the use of non-static service objects with
RequestFactory.
RequestFactory finds this service via the {@link
ScheduleServiceLocator}.
Done.
http://gwt-code-reviews.appspot.com/1299801/diff/1/17
File
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleServiceLocator.java
(right):
http://gwt-code-reviews.appspot.com/1299801/diff/1/17#newcode21
samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/server/ScheduleServiceLocator.java:21:
* Locator for {@link Schedule} service objects.
On 2011/01/21 02:54:31, rjrjr wrote:
This class provides an example of implementing a ServiceLocator to
allow
RequestFactory to work with instances of service objects, instead of
its default
behavior of mapping service calls to static methods.
There is a reference to this class in an {@literal @}Service
annotation in
{@link ...DynaTableRequestFactory}
Done.
http://gwt-code-reviews.appspot.com/1299801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
