Thanks, John.
A question for you: should I get rid of the EventBus interface and
instead just add addHandler() to HasHandlers? In theory it's a breaking
change, but in practice are there really any HasHandler implementations
other than our own?
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);
Nah, I'll just code generate this bastard instead. Stay tuned for one
more update to the patch.
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.
On 2010/08/09 02:39:26, jlabanca wrote:
ReportScaffoldPlace no longer exists.
"Maps Report {ProxyPlace} instance..."
Done.
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:
On 2010/08/09 02:39:26, jlabanca wrote:
Readd newline to separate methods.
Done.
http://gwt-code-reviews.appspot.com/717801/diff/20002/33041#oldcode78
bikeshed/src/com/google/gwt/sample/expenses/gwt/ui/report/ReportEditView.java:78:
On 2010/08/09 02:39:26, jlabanca wrote:
Readd newline to separate methods.
Done.
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) {
On 2010/08/09 02:39:26, jlabanca wrote:
Might want to add a comment explaining when this happens.
// Place changed before current place called showAcitivityWidget.
Done.
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> {
On 2010/08/09 02:39:26, jlabanca wrote:
Could we replace this class with ValueChangeEvent<Place>?
No, because it goes over the app-wide event bus.
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
On 2010/08/09 02:39:26, jlabanca wrote:
We usually use the present tense.
PlaceChangeRequestEvent (consistent with PlaceChangeEvent)
Done.
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);
On 2010/08/09 02:39:26, jlabanca wrote:
onPlaceChangeRequest
Done.
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:
On 2010/08/09 02:39:26, jlabanca wrote:
Removed a newline?
Done.
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
On 2010/08/09 02:39:26, jlabanca wrote:
afiltered => a filtered
Done.
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) {
You found it a lot confusing, bad name. It's not going anywhere in the
place controller sense, it's filtering function. "What ProxyListPlace is
implied by this other place?", e.g. the ProxyListPlace for
EmployeeRecord.class when given the ProxyPlace for EmployeeRecord #25.
Renamed proxyListPlaceFor()
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,
On 2010/08/09 02:39:26, jlabanca wrote:
and holds to hold => and holds
Done.
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 {
On 2010/08/09 02:39:26, jlabanca wrote:
should extend com.google.gwt.event.shared.HasHandlers
Done.
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);
Do you think I should i just add addHandler to HasHandlers and get rid
of this interface?
On 2010/08/09 02:39:26, jlabanca wrote:
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:
On 2010/08/09 02:39:26, jlabanca wrote:
Extra spaced = Auto format.
Done.
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));
The version is a vestigial thing that needed deleting long ago. Nothing
uses it, it'll go away soon.
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);
Done, thanks.
Question: did I confuse you when I made you abuse a renderer when what
you really needed was a SafeHtmlBuilder? (Which, by the way, is what
made me start pushing to get that thing into GWT. We should remember to
revisit your image column when it's available.)
Renderers are fundamentally meant to be used for turning an object into
user visible text. They're the opposite of Parsers. You get that?
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;
On 2010/08/09 02:39:26, jlabanca wrote:
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.
Nice, done. Now there are two constructors, one that takes a renderer
and one that takes a CellList. Thanks.
http://gwt-code-reviews.appspot.com/717801/diff/20002/33071#newcode57
user/src/com/google/gwt/user/client/ui/ValuePicker.java:57: public
ValuePicker() {
On 2010/08/09 02:39:26, jlabanca wrote:
Take the renderer in the constructor.
Done.
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() {
I am SO GLAD YOU SAID THAT! I had actually made that change in this
patch and then backed it out for fear of starting protracted
conversation. I'll definitely follow up with that, it will simplify a
lot.
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) {
On 2010/08/09 02:39:26, jlabanca wrote:
Should add a getPageSize().
Done.
http://gwt-code-reviews.appspot.com/717801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
