[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
LGTM http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
Exactly, that's my hope too :-) Hopefully the numbers will be enough to convince us the work is worth doing. On Wed, May 4, 2011 at 2:57 PM, Ray Ryan wrote: > Once we've validated the work, seems like a lot of the Attachable support > should be baked into UiObject, Widget and Panel in some kind of opt-in > manner. > > On Wed, May 4, 2011 at 10:20 AM, Rafael Castro wrote: > >> Liked it. With the stuff I added to our subclass of AttachableHTMLPanel, >> this already works pretty well. I have to review some other tricky cases >> (like if you add a non-attachable widget to an Attachable panel before >> finishing the initialization), but we're pretty close. The other cases that >> could trigger this is calling some UIObject method that we haven't yet >> @Override (like we did for setStyleName). Those call getElement() and hurt >> us. >> >> >> On Wed, May 4, 2011 at 2:15 PM, wrote: >> >>> How's that? >>> >>> Is the bit I wrote about "after adding it to a panel" accurate? Seems >>> like we're trying to get to a world where the add would be fine, and the >>> wrap call wouldn't happen until the parent is wrapped — are we there >>> already? >>> >>> >>> >>> http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java >>> >>> File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java >>> (right): >>> >>> >>> http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode211 >>> user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:211: >>> throw new IllegalStateException( >>> wrapElement() cannot be called twice, or after a call to getElement() >>> has forced the widget to render itself (e.g. after adding it to a panel) >>> >>> >>> http://gwt-code-reviews.appspot.com/1427812/ >>> >> >> -- >> http://groups.google.com/group/Google-Web-Toolkit-Contributors > > > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
Once we've validated the work, seems like a lot of the Attachable support should be baked into UiObject, Widget and Panel in some kind of opt-in manner. On Wed, May 4, 2011 at 10:20 AM, Rafael Castro wrote: > Liked it. With the stuff I added to our subclass of AttachableHTMLPanel, > this already works pretty well. I have to review some other tricky cases > (like if you add a non-attachable widget to an Attachable panel before > finishing the initialization), but we're pretty close. The other cases that > could trigger this is calling some UIObject method that we haven't yet > @Override (like we did for setStyleName). Those call getElement() and hurt > us. > > > On Wed, May 4, 2011 at 2:15 PM, wrote: > >> How's that? >> >> Is the bit I wrote about "after adding it to a panel" accurate? Seems >> like we're trying to get to a world where the add would be fine, and the >> wrap call wouldn't happen until the parent is wrapped — are we there >> already? >> >> >> >> http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java >> >> File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java >> (right): >> >> >> http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode211 >> user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:211: >> throw new IllegalStateException( >> wrapElement() cannot be called twice, or after a call to getElement() >> has forced the widget to render itself (e.g. after adding it to a panel) >> >> >> http://gwt-code-reviews.appspot.com/1427812/ >> > > -- > http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
Done. I didn't know how verbose I should be on the error message. Please
advise :-)
On Wed, May 4, 2011 at 2:04 PM, Ray Ryan wrote:
> I'd be inclined to start with a) and see what happens.
>
>
> On Wed, May 4, 2011 at 10:02 AM, wrote:
>
>>
>>
>> http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
>> File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
>> (right):
>>
>>
>> http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode205
>> user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:205: if
>> (isFullyInitialized()) {
>> On 2011/05/04 16:47:17, rjrjr wrote:
>>
>>> This seems pretty unexpected. I would have thought it would either be
>>>
>> an error
>>
>>> (RuntimeException), or else that I would do the opposite of this:
>>>
>> replace my
>>
>>> existing element with the new one.
>>>
>>
>> I'd be totally fine with a RuntimeError, it'll probably be m ore
>> manageable in the long-term. I was trying to support this because even
>> though it's less efficient, we can still make it work. The scenario here
>> is:
>>
>> 1-) Build Attachable tree. This includes calling all the render() stuff
>> and so on. Let's assume that this particular AttachableHTMLPanel is in
>> the middle of the tree.
>>
>> 2-) For some reason do something to this particular panel (that's in the
>> middle of the tree) that calls its getElement() [let's say attach it to
>> the document]. This triggers the process of building the widget tree as
>> if this panel was the root (i.e., hidden div, set innerHTML, getting
>> elements for all children and initializing them). At this point
>> everything works
>>
>> 3-) Now you go and attach the "real" root of the tree, an ancestor of
>> this panel. We can do one of 3 things:
>> (a) throw an error. You probably didn't mean to do this as it's less
>> efficient
>> (b) replace the element that our parent assigned us (along with its
>> subtree) with the subtree we already built in step 2
>> (c) re-do everything in step 2, ignoring the fact that we already
>> initialized all children widget
>>
>> I think (a) would be ideal, specially for debugging purposes. Honestly I
>> implemented (b) because I was a little afraid and wanted to be as
>> backwards-compatible as possible. I don't think (c) would work
>> out-of-the-box, as we'd have to support initializing Widgets twice
>> (i.e., re-setting the element, etc.)
>>
>> Makes sense?
>>
>>
>> http://gwt-code-reviews.appspot.com/1427812/
>>
>
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
Liked it. With the stuff I added to our subclass of AttachableHTMLPanel, this already works pretty well. I have to review some other tricky cases (like if you add a non-attachable widget to an Attachable panel before finishing the initialization), but we're pretty close. The other cases that could trigger this is calling some UIObject method that we haven't yet @Override (like we did for setStyleName). Those call getElement() and hurt us. On Wed, May 4, 2011 at 2:15 PM, wrote: > How's that? > > Is the bit I wrote about "after adding it to a panel" accurate? Seems > like we're trying to get to a world where the add would be fine, and the > wrap call wouldn't happen until the parent is wrapped — are we there > already? > > > > http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java > > File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java > (right): > > > http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode211 > user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:211: > throw new IllegalStateException( > wrapElement() cannot be called twice, or after a call to getElement() > has forced the widget to render itself (e.g. after adding it to a panel) > > > http://gwt-code-reviews.appspot.com/1427812/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
How's that? Is the bit I wrote about "after adding it to a panel" accurate? Seems like we're trying to get to a world where the add would be fine, and the wrap call wouldn't happen until the parent is wrapped — are we there already? http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java (right): http://gwt-code-reviews.appspot.com/1427812/diff/6003/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode211 user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:211: throw new IllegalStateException( wrapElement() cannot be called twice, or after a call to getElement() has forced the widget to render itself (e.g. after adding it to a panel) http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
I'd be inclined to start with a) and see what happens.
On Wed, May 4, 2011 at 10:02 AM, wrote:
>
>
> http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
> File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
> (right):
>
>
> http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode205
> user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:205: if
> (isFullyInitialized()) {
> On 2011/05/04 16:47:17, rjrjr wrote:
>
>> This seems pretty unexpected. I would have thought it would either be
>>
> an error
>
>> (RuntimeException), or else that I would do the opposite of this:
>>
> replace my
>
>> existing element with the new one.
>>
>
> I'd be totally fine with a RuntimeError, it'll probably be m ore
> manageable in the long-term. I was trying to support this because even
> though it's less efficient, we can still make it work. The scenario here
> is:
>
> 1-) Build Attachable tree. This includes calling all the render() stuff
> and so on. Let's assume that this particular AttachableHTMLPanel is in
> the middle of the tree.
>
> 2-) For some reason do something to this particular panel (that's in the
> middle of the tree) that calls its getElement() [let's say attach it to
> the document]. This triggers the process of building the widget tree as
> if this panel was the root (i.e., hidden div, set innerHTML, getting
> elements for all children and initializing them). At this point
> everything works
>
> 3-) Now you go and attach the "real" root of the tree, an ancestor of
> this panel. We can do one of 3 things:
> (a) throw an error. You probably didn't mean to do this as it's less
> efficient
> (b) replace the element that our parent assigned us (along with its
> subtree) with the subtree we already built in step 2
> (c) re-do everything in step 2, ignoring the fact that we already
> initialized all children widget
>
> I think (a) would be ideal, specially for debugging purposes. Honestly I
> implemented (b) because I was a little afraid and wanted to be as
> backwards-compatible as possible. I don't think (c) would work
> out-of-the-box, as we'd have to support initializing Widgets twice
> (i.e., re-setting the element, etc.)
>
> Makes sense?
>
>
> http://gwt-code-reviews.appspot.com/1427812/
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
(right):
http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode205
user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:205: if
(isFullyInitialized()) {
On 2011/05/04 16:47:17, rjrjr wrote:
This seems pretty unexpected. I would have thought it would either be
an error
(RuntimeException), or else that I would do the opposite of this:
replace my
existing element with the new one.
I'd be totally fine with a RuntimeError, it'll probably be m ore
manageable in the long-term. I was trying to support this because even
though it's less efficient, we can still make it work. The scenario here
is:
1-) Build Attachable tree. This includes calling all the render() stuff
and so on. Let's assume that this particular AttachableHTMLPanel is in
the middle of the tree.
2-) For some reason do something to this particular panel (that's in the
middle of the tree) that calls its getElement() [let's say attach it to
the document]. This triggers the process of building the widget tree as
if this panel was the root (i.e., hidden div, set innerHTML, getting
elements for all children and initializing them). At this point
everything works
3-) Now you go and attach the "real" root of the tree, an ancestor of
this panel. We can do one of 3 things:
(a) throw an error. You probably didn't mean to do this as it's less
efficient
(b) replace the element that our parent assigned us (along with its
subtree) with the subtree we already built in step 2
(c) re-do everything in step 2, ignoring the fact that we already
initialized all children widget
I think (a) would be ideal, specially for debugging purposes. Honestly I
implemented (b) because I was a little afraid and wanted to be as
backwards-compatible as possible. I don't think (c) would work
out-of-the-box, as we'd have to support initializing Widgets twice
(i.e., re-setting the element, etc.)
Makes sense?
http://gwt-code-reviews.appspot.com/1427812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
http://gwt-code-reviews.appspot.com/1427812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix error in usage of newly-creted helper method in AttachableHTMLPanel, correct double-calling ... (issue1427812)
Sorry Rafa, forgot to hit send on this yesterday.
http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
File user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java
(right):
http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode205
user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:205: if
(isFullyInitialized()) {
This seems pretty unexpected. I would have thought it would either be an
error (RuntimeException), or else that I would do the opposite of this:
replace my existing element with the new one.
http://gwt-code-reviews.appspot.com/1427812/diff/1/user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java#newcode208
user/src/com/google/gwt/user/client/ui/AttachableHTMLPanel.java:208: //
we already built.
/* for long comments please
http://gwt-code-reviews.appspot.com/1427812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
