[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-10-05 Thread Ray Ryan 
Point taken on the view contention front. I think Thomas is right that
setting and clearing the view delegate should happen in start() and stop(),
not in the constructor. Thanks, Thomas.

On Mon, Sep 27, 2010 at 12:46 PM, t.bro...@gmail.com wrote:


 http://gwt-code-reviews.appspot.com/925801/diff/1/2
 File user/src/com/google/gwt/app/place/AbstractProxyListActivity.java
 (right):

 http://gwt-code-reviews.appspot.com/925801/diff/1/2#newcode256
 user/src/com/google/gwt/app/place/AbstractProxyListActivity.java:256: if
 (view == null) {
 On 2010/09/27 18:55:21, rjrjr wrote:

 The activities have never been intended to be re-used.


 Yes, but there's another issue with initializing in the constructor
 (while sharing a singleton view between activity instances); see my
 reply on the issue front page (don't know how you call it).
 I don't have a dev env right now; will try to produce a test case
 tomorrow demonstrating the issue (unless I'm wrong in my reading of the
 code).


  Sounds like the javadoc should say so.


 +1


 http://gwt-code-reviews.appspot.com/925801/show


-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-09-30 Thread jlabanca 

committed as r8886

http://gwt-code-reviews.appspot.com/925801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-09-28 Thread t . broyer 

On 2010/09/27 23:49:02, rjrjr wrote:

Point taken on the view contention front. I think Thomas is right that
setting and clearing the view delegate should happen in start() and

stop(),

not in the constructor. Thanks, Thomas.


You're welcome ;-)


  Sounds like the javadoc should say so.

 +1


How about also adding an assert in the start method?
   assert view != null : Activity cannot be reused;
(in both AbstractProxyListActivity and AbstractProxyEditActivity)

(I'd actually even add a isDead() protected method for the view ==
null check)

http://gwt-code-reviews.appspot.com/925801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-09-27 Thread t . broyer 


http://gwt-code-reviews.appspot.com/925801/diff/1/2
File user/src/com/google/gwt/app/place/AbstractProxyListActivity.java
(right):

http://gwt-code-reviews.appspot.com/925801/diff/1/2#newcode256
user/src/com/google/gwt/app/place/AbstractProxyListActivity.java:256: if
(view == null) {
Isn't the problem that this activity isn't reusable? I mean, it
initializes the view in its constructor (lines 87 to 108) and clears
everything in onStop().
Shouldn't it rather initializes in start()? and keep the view around in
onStop so that it can be re-initialized (re-start()ed)? (and probably
clear the idToRow and idToProxy maps)

This check would then be if (display == null), just like in
AbstractProxyEditActivity.

http://gwt-code-reviews.appspot.com/925801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-09-27 Thread jlabanca 


http://gwt-code-reviews.appspot.com/925801/diff/1/2
File user/src/com/google/gwt/app/place/AbstractProxyListActivity.java
(right):

http://gwt-code-reviews.appspot.com/925801/diff/1/2#newcode256
user/src/com/google/gwt/app/place/AbstractProxyListActivity.java:256: if
(view == null) {
TBH - I didn't put that much thought into it, I just copied the code
from line 137.

@Ray - Does tbroyer's comment sound correct?

http://gwt-code-reviews.appspot.com/925801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-09-27 Thread jlabanca 


http://gwt-code-reviews.appspot.com/925801/diff/1/3
File user/src/com/google/gwt/app/place/AbstractProxyListView.java
(right):

http://gwt-code-reviews.appspot.com/925801/diff/1/3#newcode84
user/src/com/google/gwt/app/place/AbstractProxyListView.java:84:
protected void init(Widget root, CellTableP table, Button newButton,
I wasn't planing on it because the table version adds the columns to the
table, but thats probably not the best idea anyway.  I'll deprecate this
method and change the Scaffold to always use the other version.

http://gwt-code-reviews.appspot.com/925801/diff/1/4
File user/src/com/google/gwt/view/client/SingleSelectionModel.java
(right):

http://gwt-code-reviews.appspot.com/925801/diff/1/4#newcode92
user/src/com/google/gwt/view/client/SingleSelectionModel.java:92: Object
key = (newSelectedObject == null) ? null : getKey(newSelectedObject);
If I wasn't lazy... which I won't be.  Test coming.

http://gwt-code-reviews.appspot.com/925801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes required to make the Scaffold app look like the mocks. Added null checks AbstractProxyLi... (issue925801)

2010-09-27 Thread jlabanca 

http://gwt-code-reviews.appspot.com/925801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors