[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
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#newcode54
user/src/com/google/gwt/app/place/ProxyPlaceToListPlace.java:54: public
ProxyListPlace go(Place place) {
Whats confusing is that it saves the record returned by
proxyPlace().getProxy(), and then I can get the record by calling
getProxy(). Its a stateful class, but I don't understand why. Do you
ever call ProxyPlaceToListPlace.getProxy()?
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#newcode45
user/src/com/google/gwt/event/shared/EventBus.java:45: void
fireEvent(GwtEvent? event);
There are probably a lot of people using it because all HasXXXHandlers
extend it. For example, HasClickHandlers extends it and adds
addBlurHandler.
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(CollectionT values, RendererT render);
Assuming you mean for ImageLoadingCell, I think the renderers had more
to do with being able to customize loading and error messages than
building safe HTML. Either way, we'll need to take a look at how to
integrate the renderers in ImageLoadingCell with SafeHtmlBuilder,
because you can't just return an HTML string and call it unsafe. We
might need a SafeHtmlRenderer that takes a SafeHtmlBuilder instead of
returning a String.
http://gwt-code-reviews.appspot.com/717801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
On Tue, Aug 10, 2010 at 6:40 AM, jlaba...@google.com wrote:
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#newcode54
user/src/com/google/gwt/app/place/ProxyPlaceToListPlace.java:54: public
ProxyListPlace go(Place place) {
Whats confusing is that it saves the record returned by
proxyPlace().getProxy(), and then I can get the record by calling
getProxy(). Its a stateful class, but I don't understand why. Do you
ever call ProxyPlaceToListPlace.getProxy()?
This is an example of the over-complexity of this approach, which comes of
trying to use a mechanism for apps that do one thing at a time for an app
that is doing several at a time.
Remember that the app can only be in one place at a time. The master list at
the top of the screen needs to be shown with no selection when the app goes
to a ProxyListPlace (it's the thing that shows the list of proxies). And it
needs to stay visible and update its selection when the app moves to a
ProxyPlace (to show the details for a single record). The list on the left
side of the screen needs to maintain its selection in the same way.
A ProxyPlaceToListPlace converter is able to tell you what list a particular
proxy belongs to. An EmployeeRecord is in the EmployeeRecord.class list.
When it figures that out for you, it also holds on to the proxy it looked
at, e.g. to allow you to update your selection — getProxy(). You're right,
though, that's a just plain odd side effect. It made more sense in an
earlier, even more complicated version of this patch last week. I'll try to
drop it before submitting.
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#newcode45
user/src/com/google/gwt/event/shared/EventBus.java:45: void
fireEvent(GwtEvent? event);
There are probably a lot of people using it because all HasXXXHandlers
extend it. For example, HasClickHandlers extends it and adds
addBlurHandler.
Oh, okay. Thanks.
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(CollectionT values, RendererT render);
Assuming you mean for ImageLoadingCell, I think the renderers had more
to do with being able to customize loading and error messages than
building safe HTML. Either way, we'll need to take a look at how to
integrate the renderers in ImageLoadingCell with SafeHtmlBuilder,
because you can't just return an HTML string and call it unsafe. We
might need a SafeHtmlRenderer that takes a SafeHtmlBuilder instead of
returning a String.
Yes. The main point is that a Renderer should strictly be used a source of
plain user readable text, not nascent dom.
http://gwt-code-reviews.appspot.com/717801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
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 GwtEventPlaceChangeEvent.Handler {
On 2010/08/09 02:39:26, jlabanca wrote:
Could we replace this class with ValueChangeEventPlace?
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
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
On Sun, Aug 8, 2010 at 2:16 AM, cromwell...@google.com wrote:
Partial review (time for sleep)
http://gwt-code-reviews.appspot.com/717801/diff/8003/32002
File
bikeshed/src/com/google/gwt/sample/expenses/gwt/client/HistoryPlaceIntegration.java
(right):
http://gwt-code-reviews.appspot.com/717801/diff/8003/32002#newcode91
bikeshed/src/com/google/gwt/sample/expenses/gwt/client/HistoryPlaceIntegration.java:91:
if (r.equals(initial)) {
If we're going with a prefix scheme, maybe there should be a prefix -
Tokenizer map. Or perhaps a more flexible scheme would just chain
tokenizers, so that the first, root, tokenizer installed maps to
sub-tokenizers.
Yes, I plan to do exactly that with a GWT code generator. That will probably
be less work than making Roo generate this thing, so I guess I'll have to do
it today.
There won't be any nesting, at least not yet. The place scheme completely
hinges on there being only one place, so that when someone I don't know sets
a new place I automatically get torn down.
http://gwt-code-reviews.appspot.com/717801/diff/8003/32051
File user/src/com/google/gwt/app/place/ProxyPlace.java (right):
http://gwt-code-reviews.appspot.com/717801/diff/8003/32051#newcode45
user/src/com/google/gwt/app/place/ProxyPlace.java:45: return new
ProxyPlace(requests.getProxy(bits[0]),
TODO: avoid Enum.valueOf(), prefer ordinal(). This requires every enum
to carry these string tables around to do the mapping, I'm teaching the
compiler to prune these with special flags. As an added bonus, the token
will be shorter.
I'll do that now.
http://gwt-code-reviews.appspot.com/717801/diff/8003/32051#newcode50
user/src/com/google/gwt/app/place/ProxyPlace.java:50: return
requests.getToken(place.getProxy()) + @ + place.getOperation();
TODO: eventually, place.getOperation.ordinal()
http://gwt-code-reviews.appspot.com/717801/diff/8003/32058
File
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java
(right):
http://gwt-code-reviews.appspot.com/717801/diff/8003/32058#newcode324
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:324:
// write getToken(Class)
Am I imagining things, or is this simply the same as doing
clazz.getName()? getToken(clazz) looks up a RecordSchema in
recordToTypeMap and then invokes getToken() on RecordSchema, which
returns the Record clazz again, on which getName() is invoked.
That's exactly what it's doing, but it's intended to be temporary — I'll add
a TODO. Just like the JSON is sending the entire FQ class name down the wire
but shouldn't be. I want to find some kind of lossless compression scheme
for both to use, probably just a base 36 encoding.
http://gwt-code-reviews.appspot.com/717801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
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 GwtEventPlaceChangeEvent.Handler {
Could we replace this class with ValueChangeEventPlace?
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
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
Still not ready for review, just sharing the progress. Activities no longer have to clean up any event handlers they register, and ActivityManger should be exception proof. http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
Hold off on this. I forgot to make the list on the left history aware, will update soon. On Mon, Jul 26, 2010 at 6:08 PM, rj...@google.com wrote: Reviewers: bobv, Description: Hard coded History integration for the Scaffold app. This is step zero in the search for a general, generatable scheme. It is not intended to be what we actually ship. I do like that it seems to be feature complete without having required any changes to the PlaceController. That's a good sign, right? Review by: robertvaw...@google.com Please review this at http://gwt-code-reviews.appspot.com/717801/show Affected files: A bikeshed/src/com/google/gwt/sample/expenses/gwt/client/HistoryPlaceIntegration.java M bikeshed/src/com/google/gwt/sample/expenses/gwt/client/Scaffold.java M bikeshed/war/WEB-INF/logging.properties M user/src/com/google/gwt/app/client/CellListPlacePickerView.java M user/src/com/google/gwt/app/place/ActivityManager.java M user/src/com/google/gwt/user/client/History.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
Okay, ready now. http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
Nope, still buggy. Back and forth working but refresh messes up the master list on the top of the screen. http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Hard coded History integration for the Scaffold app. This is step zero (issue717801)
No longer buggy, but I think also not to be committed. Bob, don't mind this. On Tue, Jul 27, 2010 at 5:18 PM, rj...@google.com wrote: http://gwt-code-reviews.appspot.com/717801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
