LGTM This looks good, but its fairly complicated to reuse in a non-generated GWT app. We might want to revisit this for the next milestone and see if we can simplify the API even further.
I love that we can delete so many classes in the sample. My biggest gripe is with HasValues, which takes a renderer. As noted in the comment, the render should be specific to the Widget, not the HasValues interface. http://gwt-code-reviews.appspot.com/717801/diff/20002/33009 File bikeshed/src/com/google/gwt/sample/expenses/gwt/client/HistoryPlaceIntegration.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33009#newcode90 bikeshed/src/com/google/gwt/sample/expenses/gwt/client/HistoryPlaceIntegration.java:90: String rest = token.substring(2); Might want to add a comment explaining why rest starts at index 2 instead of 1. // Index 1 is a colon separator, so rest starts at index 2. http://gwt-code-reviews.appspot.com/717801/diff/20002/33038 File bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportActivitiesMapper.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33038#newcode25 bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportActivitiesMapper.java:25: * Maps {...@link ReportScaffoldPlace} instances to the {...@link Activity} to run. ReportScaffoldPlace no longer exists. "Maps Report {ProxyPlace} instance..." http://gwt-code-reviews.appspot.com/717801/diff/20002/33041 File bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditView.java (left): http://gwt-code-reviews.appspot.com/717801/diff/20002/33041#oldcode73 bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditView.java:73: Readd newline to separate methods. http://gwt-code-reviews.appspot.com/717801/diff/20002/33041#oldcode78 bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditView.java:78: Readd newline to separate methods. http://gwt-code-reviews.appspot.com/717801/diff/20002/33047 File user/src/com/google/gwt/app/place/ActivityManager.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33047#newcode107 user/src/com/google/gwt/app/place/ActivityManager.java:107: if (startingNext) { Might want to add a comment explaining when this happens. // Place changed before current place called showAcitivityWidget. http://gwt-code-reviews.appspot.com/717801/diff/20002/33052 File user/src/com/google/gwt/app/place/PlaceChangeEvent.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33052#newcode29 user/src/com/google/gwt/app/place/PlaceChangeEvent.java:29: public class PlaceChangeEvent extends GwtEvent<PlaceChangeEvent.Handler> { Could we replace this class with ValueChangeEvent<Place>? http://gwt-code-reviews.appspot.com/717801/diff/20002/33053 File user/src/com/google/gwt/app/place/PlaceChangeRequestedEvent.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33053#newcode31 user/src/com/google/gwt/app/place/PlaceChangeRequestedEvent.java:31: public class PlaceChangeRequestedEvent extends We usually use the present tense. PlaceChangeRequestEvent (consistent with PlaceChangeEvent) http://gwt-code-reviews.appspot.com/717801/diff/20002/33053#newcode40 user/src/com/google/gwt/app/place/PlaceChangeRequestedEvent.java:40: void onPlaceChangeRequested(PlaceChangeRequestedEvent event); onPlaceChangeRequest http://gwt-code-reviews.appspot.com/717801/diff/20002/33054 File user/src/com/google/gwt/app/place/PlaceController.java (left): http://gwt-code-reviews.appspot.com/717801/diff/20002/33054#oldcode49 user/src/com/google/gwt/app/place/PlaceController.java:49: Removed a newline? http://gwt-code-reviews.appspot.com/717801/diff/20002/33060 File user/src/com/google/gwt/app/place/ProxyPlaceToListPlace.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33060#newcode43 user/src/com/google/gwt/app/place/ProxyPlaceToListPlace.java:43: * @return the proxy of afiltered {...@link ProxyPlace}, or null afiltered => a filtered http://gwt-code-reviews.appspot.com/717801/diff/20002/33060#newcode54 user/src/com/google/gwt/app/place/ProxyPlaceToListPlace.java:54: public ProxyListPlace go(Place place) { I find this class a little confusing. You "go" to a place, which remembers the record associated with the place for subsequent calls to getProxy? It seems like the PlaceController should be responsible for keeping track of the current place. http://gwt-code-reviews.appspot.com/717801/diff/20002/33061 File user/src/com/google/gwt/app/place/StopperedEventBus.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33061#newcode28 user/src/com/google/gwt/app/place/StopperedEventBus.java:28: * Wraps an EventBus and holds to hold on to any HandlerRegistrations, and holds to hold => and holds http://gwt-code-reviews.appspot.com/717801/diff/20002/33062 File user/src/com/google/gwt/event/shared/EventBus.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33062#newcode21 user/src/com/google/gwt/event/shared/EventBus.java:21: public interface EventBus { should extend com.google.gwt.event.shared.HasHandlers http://gwt-code-reviews.appspot.com/717801/diff/20002/33062#newcode45 user/src/com/google/gwt/event/shared/EventBus.java:45: void fireEvent(GwtEvent<?> event); Also defined in HasHandlers http://gwt-code-reviews.appspot.com/717801/diff/20002/33065 File user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryJsonImpl.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33065#newcode196 user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryJsonImpl.java:196: Extra spaced = Auto format. http://gwt-code-reviews.appspot.com/717801/diff/20002/33065#newcode204 user/src/com/google/gwt/requestfactory/client/impl/RequestFactoryJsonImpl.java:204: return schema.create(RecordJsoImpl.create(id, -1, schema)); Is the version always -1? http://gwt-code-reviews.appspot.com/717801/diff/20002/33068 File user/src/com/google/gwt/requestfactory/shared/RequestFactory.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33068#newcode47 user/src/com/google/gwt/requestfactory/shared/RequestFactory.java:47: /** add spaces http://gwt-code-reviews.appspot.com/717801/diff/20002/33069 File user/src/com/google/gwt/user/client/History.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33069#newcode82 user/src/com/google/gwt/user/client/History.java:82: @SuppressWarnings("deprecation") I would think that the fact that this method is deprecated means that you don't need to suppress deprecation. Is there an Eclipse warning option for this? I could be wrong. http://gwt-code-reviews.appspot.com/717801/diff/20002/33070 File user/src/com/google/gwt/user/client/ui/HasConstrainedValue.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33070#newcode34 user/src/com/google/gwt/user/client/ui/HasConstrainedValue.java:34: void setValues(Collection<T> values, Renderer<T> render); Passing in the renderer here seems wrong. It should be up to the Widget to determine how the values are rendered. For example, one implementation may show a list, while another shows a table, and another shows images. Can we pass the renderer into the ValuePicker constructor instead. http://gwt-code-reviews.appspot.com/717801/diff/20002/33071 File user/src/com/google/gwt/user/client/ui/ValuePicker.java (right): http://gwt-code-reviews.appspot.com/717801/diff/20002/33071#newcode53 user/src/com/google/gwt/user/client/ui/ValuePicker.java:53: private final CellList<T> cellList; The user has no way to modify the look of the cell. You should either create CellList<T> in a protected method that the user can override, or take a CellList or CellList.Resources as an optional constructor arg. http://gwt-code-reviews.appspot.com/717801/diff/20002/33071#newcode57 user/src/com/google/gwt/user/client/ui/ValuePicker.java:57: public ValuePicker() { Take the renderer in the constructor. http://gwt-code-reviews.appspot.com/717801/diff/20002/33071#newcode75 user/src/com/google/gwt/user/client/ui/ValuePicker.java:75: public ValuePicker<T> asWidget() { Should Widget implement IsWidget? http://gwt-code-reviews.appspot.com/717801/diff/20002/33071#newcode83 user/src/com/google/gwt/user/client/ui/ValuePicker.java:83: public void setPageSize(int size) { Should add a getPageSize(). http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
