public class Test implements EntryPoint {
class TestPanel extends SimplePanel {
protected void onLoad() {
super.onLoad();
add(new Label("label"));
}
}
public void onModuleLoad() {
RootPanel.get().add(new TestPanel()); //exception
}
}
[ERROR] Unable to load module entry point class com.allen_sauer.gwt.test.client.Test (see associated exception for details)
java.lang.IllegalStateException: Should only call onAttach when the widget is detached from the browser's document
at com.google.gwt.user.client.ui.Widget.onAttach(Widget.java:89)
at com.google.gwt.user.client.ui.Panel.onAttach (Panel.java:86)
at com.google.gwt.user.client.ui.Widget.setParent(Widget.java:216)
at com.google.gwt.user.client.ui.Panel.adopt(Panel.java:59)
at com.google.gwt.user.client.ui.ComplexPanel.insert(ComplexPanel.java:107)
at com.google.gwt.user.client.ui.ComplexPanel.add (ComplexPanel.java:70)
at com.google.gwt.user.client.ui.AbsolutePanel.add(AbsolutePanel.java:68)
at com.allen_sauer.gwt.test.client.Test.onModuleLoad(Test.java:22)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:25)
The problem is that the onLoad() method is called in the middle of the
onAttach process. This means the widget is in a weird, possibly
inconsistent state. If you were to override onAttach() and put your
onLoad() logic there it would work fine.
The flow is Panel.onAttach() calls Widget.onAttach() which sets
attached to true and then calls onLoad(). After that is done then
Panel.onAttach() finishes by calling onAttach for all the children of
the panel.
In your case the onLoad() method added the new Label and the label was
"attached" as part of that add. When the Panel.onAttach() finished up
it "attached" the label again.
There are two fixed for this. 1. Don't throw the exception and allow a
double attach behavior or 2. move Widget.onAttach()'s call to onLoad()
to before the widget sets attached to true. The former allows a weak
widget lifecycle and the latter means that children are "attached"
before the panel is.
IMO onLoad() should be @deprecated and the onUnload should be removed
before it's too late.
On 5/30/07, Fred Sauer <fr...@allen-sauer.com> wrote:
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine
The flow is Panel.onAttach() calls Widget.onAttach() which sets
attached to true and then calls onLoad(). After that is done then
Panel.onAttach() finishes by calling onAttach for all the children of
the panel.
Short of special case compiler magic intercepting the first call to
this.onAttach() and then calling this.onLoad() or Widget.onAttach()
queuing a DeferredCommand (an unacceptable solution because of
latency) I don't think onLoad() can be "fixed". The onUnload() is
susceptible to the same kinds of problems, just in reverse.
someWidget.addAttachListener?
Can anyone see an issue with simply moving Panel.onAttach()'s super call to the end of the method, to at least fix the problem at hand? Before we start discussing stuff like addAttachListener(), I want to first pin down the correct ordering of this invocation, and ensure that the one guarantee we make in onLoad (namely that the widget will be attached at that point) is maintained.
This will have the opposite effect. onAttach() will be called leaf to head.
If you have a detached Panel that contains a Label the attaching flow would be:
* Panel.onAttach()
for each child Widget calls:
* Label.onAttach() which would eventually call the Label's onLoad()
exit back to Panel.onAttach() which calls
* super.onAttach()
which sets attached to true, sets event listeners, can calls
* this.onLoad()
This means that contained widgets will be "attached" and "loaded"
before their parents are. If there were an exception during the attach
process you're left with some attached children widgets and an
unattached parent panel.
It seems reasonable to me that:
* parent widgets should be attached before children widgets.
* children widgets should be detached before parent widgets.
Also, we should come up with a standard way of handling DOM resizes
that we cannot catch. For example, the ScrollTable I'm working on
listens for WindowResize events, but in order to handle random DOM
manipulation, I added a Timer that periodically compares the old
dimensions to the current dimensions of the parent element. This
works in testing, but I'm not sure how it would hold up if every
Widget had its own Timer and dimension comparisons.
One option is to have a ResizableWidget class with an abstract
onresize() method, and a non-abstract "redraw" method. onResize will
be called whenever the DOM resizes through some catchable event
(WindowResize, SplitPanel resizing), but the user can call the
redraw() method manually if needed. A static Timer() will
periodically check the dimensions of all ResizableWidget instances,
and if they change, it will call the onresize() method.
On Jun 27, 6:17 pm, "Sandy McArthur" <sandy...@gmail.com> wrote:
> On 6/27/07, Scott Blum <sco...@google.com> wrote:
>
> > On 6/27/07, Joel Webber <j...@google.com> wrote:
> > > Can anyone see an issue with simply moving Panel.onAttach()'s super call
> > to the end of the method, to at least fix the problem at hand? Before we
> > start discussing stuff like addAttachListener(), I want to first pin down
> > the correct ordering of this invocation, and ensure that the one guarantee
> > we make inonLoad(namely that thewidgetwill be attached at that point) is
> > maintained.
>
> > I think this is right. If I understand this correctly, it has the nice
> > effect that onAttach() should run from head to leaf, whileonLoad() should
> I think this is right. If I understand this correctly, it has the nice
> effect that onAttach() should run from head to leaf, while onLoad() should
> run from leaf to head.
This will have the opposite effect. onAttach() will be called leaf to head.
If you have a detached Panel that contains a Label the attaching flow would be:
* Panel.onAttach()
for each child Widget calls:
* Label.onAttach() which would eventually call the Label's onLoad()
exit back to Panel.onAttach() which calls
* super.onAttach()
which sets attached to true, sets event listeners, can calls
* this.onLoad()
I've attached a patch that I believe addresses all the issues we've been discussing. It should ensure that:- onAttach() runs top to bottom, so that the hierarchy can't get in an inconsistent state.- onLoad() runs bottom to top, so that Panels can assume all of their children are already attached in onLoad()- onDetach() runs bottom to top, for the same reason that onAttach() runs the other way.- onUnload() also runs bottom to top, which seems to make sense as well.It does this by splitting Widget.onAttach's implementation into two pieces. It should be fairly clear from the patch.Please let me know if anyone sees any holes in this formulation, so I can put the patch up for formal review.
> attemptToFixOnLoadAndStuff.patch
> 5K Download
The patch name indicates that you understand the problem with onLoad
too, why stick with the more complicated solution when a simpler one
is available. With this patch the requirement to always call
super.onAttach() is now a sometimes requirement and Widget's API
surface has increased.
What happened to "do the simplest thing that could possibly work"
http://www.google.com/search?q=Do+the+simplest+thing+that+could+possibly+work
The simplest thing that meets everybody's needs is to use onAttach.
Trying to support onLoad is adding complexity which will increase the
compiled app's size, decrease it's speed and makes the code a little
harder to understand.
The patch name indicates that you understand the problem with onLoad
too, why stick with the more complicated solution when a simpler one
is available. With this patch the requirement to always call
super.onAttach() is now a sometimes requirement and Widget's API
surface has increased.
What happened to "do the simplest thing that could possibly work"
http://www.google.com/search?q=Do+the+simplest+thing+that+could+possibly+work
The simplest thing that meets everybody's needs is to use onAttach.
Trying to support onLoad is adding complexity which will increase the
compiled app's size, decrease it's speed and makes the code a little
harder to understand.
Tree.onAttach will need to be reworked like Panel.onAttach.
Also I just noticed Composite.onAttach isn't right but for a different
reason (and has been wrong since at least 1.3.3). It shouldn't be
calling super.onAttach because DOM.setEventListener(getElement(),
this); ends up being called twice on the same DOM element, once for
the Composite instance and once for the Composite's wrapped Widget
instance.
I think Composite methods should look like:
protected void onAttach() {
// Call onAttach() on behalf of the contained widget.
widget.onAttach();
onLoad();
}
protected void onDetach() {
// Call onDetach() on behalf of the contained widget.
widget.onDetach();
onUnload();
}
public boolean isAttached() {
return widget.isAttached();
}
Also, while not currently a problem, direct access to the "attached"
field in Widget only happens to work because it defaults the the right
value needed when Composite.initWidget must be called. It would be
prudent to replace the field access with calls to isAttached() to
prevent future bugs.
Also I just noticed Composite.onAttach isn't right but for a different
reason (and has been wrong since at least 1.3.3). It shouldn't be
calling super.onAttach because DOM.setEventListener(getElement(),
this); ends up being called twice on the same DOM element, once for
the Composite instance and once for the Composite's wrapped Widget
instance.
Also, while not currently a problem, direct access to the "attached"
field in Widget only happens to work because it defaults the the right
value needed when Composite.initWidget must be called. It would be
prudent to replace the field access with calls to isAttached() to
prevent future bugs.
All,@knorton: can you have a look at this as well? I want as many eyes on this as possible.I'm attaching a patch that I think will finally clear all this stuff up, and adds a test to keep us honest in the future.
To deal with the weird issues in overriding onAttach/onDetach, I changed their implementations in Widget to call the template methods doAttachChildren/doDetachChildren. If you call your childrens' onAttach/onDetach within this template, it will happen at exactly the right time.For onDetach/onUnload, I settled on having onUnload happen *before* actually detaching, as this is almost always what you need in practice. To give an example, the CheckBox.onUnload() needs to store the state of its 'checked' property *before* being removed from the DOM, because some browsers clobber that value when it's detached (go figure...). I believe the same is true for RichText (though I haven't implemented it yet).
Also I just noticed Composite. onAttach isn't right but for a different
reason (and has been wrong since at least 1.3.3). It shouldn't be
calling super.onAttach because DOM.setEventListener(getElement(),
this); ends up being called twice on the same DOM element, once for
the Composite instance and once for the Composite's wrapped Widget
instance.
Good call. Done.Also, while not currently a problem, direct access to the "attached"
field in Widget only happens to work because it defaults the the right
value needed when Composite.initWidget must be called. It would be
prudent to replace the field access with calls to isAttached() to
prevent future bugs.Also a good call. Done as well.
Thanks again everyone for your patience in helping me get these issues pinned down.joel.
Oops, sorry. I read over this and still missed the fact that my name was on it.
Just a few comments:
Widget.java
@87 & 118 - Intuitively it seems like onAttach and onDetach should be symmetric operations on the tree, so I'm wondering why onDetach isn't:
try {
onUnload();
} finally {
doDetachChildren();
DOM.setEventListener(getElement(), null);
attached = false;
}
@151 & 159 - It seems odd to me that onAttach and doAttachChildren do similar things but have slightly different names. onAttachChildren seems like the obvious choice for consistency, is there something different about those methods to warrant a new auxiliary verb?
@156 - File needs a sort. doAttachChildren is out of order.
Composite.java
@96 - Widget.onDetach guards against exceptions being thrown in the local onUnload, but Composite.onDetach does not? Should it?
Panel.java
@98 - "only once" made me think you were saying something else. I think the same wording used in onUnload's doc will work:
"A Panel's onLoad method will be called after all of its children are attached."
WidgetOnLoadTest
@1 - Needs a header to appease checkstyle.
@7 - Needs a javadoc to appease checkstyle.
Here's my logic -- see if you think it makes sense:- onAttach() is called when the widget *is attached*- doAttachChildren() is a template method giving the subclass the opportunity to do any attachment of children (using the more or less standard doWhatever() naming convention).