A proposal for Widget ownership

[Scott Blum]

Hi everyone,

Adding and removing a widget to/from a panel (or other parent) is a process that has evolved over time, as we've come to realize what the right behavior needs to be.  Up until now, there hasn't been a formal design for doing this correctly.  Rather, we've been converging on a set of behaviors that mostly work, but aren't completely consistent across the board.  It's time we formalized exactly what needs to happen in order to correctly add or remove a widget.  So I've proposed a "template" or order of operations for handlin this correctly.  But first let's talk about the guiding principles behind the design.

--------------------------------------
Guiding Principles

1. onAttach/onDetach should occur only when both the DOM and the Widget structure are in a consistent, valid state.  In practical terms, that means onAttach is the last thing to happen during an add, and onDetach is the first thing to happen during a remove.
2. Fail fast; avoid leaving things in an invalid state.  In practical terms, error checking and other operations should happen first.
3. The template should be evident in any given Panel's add/remove method, to make it easy to maintain, enhance, and copy from.
   

--------------------------------------
The add()/insert() Template (also, SimpelPanel.setWidget() style methods)

There are several important things that must take place in the correct order  to properly add or insert a Widget to a Panel. Not all of these steps will be  relevant to every Panel, but all of the steps should be considered.  

1. Validate: Perform any sanity checks to ensure the Panel can accept a new Widget. Examples: checking for a valid index on insertion; checking that the Panel is not full if there is a max capacity.
2. Account for Reinsertion: Some Panels need special handling for the case where the Widget is already a child of this Panel. Example: when performing a reinsert, the index might need to be adjusted to account for the Widget's removal.
3. Detach New Child: Remove the Widget from its existing parent, if any. Most Panels will simply call Widget.removeFromParent() on the Widget.  It's important to do this before performing any state changes to the Panel because some Widgets cannot be removed from their parents and will throw an IllegalStateException; such exceptions should be allowed to escape to the caller.
4. Remove an Existing Child (setWidget): Some Panels have a limited number of slots with methods whose semantics dictate that some existing child will be removed to make way for the new child.  This should happen immediately after detaching the new child (which could fail) but before updating any state relating to inserting the new child.
   
5. Logical Attach: Any state variables of the Panel should be updated to reflect the addition of the new Widget. Example: the Widget is added to the Panel's WidgetCollection at the appropriate index.
6. Physical Attach: The Widget's Element must be physically attached to the Panel's Element, either directly or indirectly.
7. Adopt: Call adopt(Widget) to finalize the add as the very last step; this sets the child Widget's parent to the Panel and triggers onAttach().
   

--------------------------------------
The remove() Template

There are several important things that must take place in the correct order  to properly remove a Widget from a Panel. Not all of these steps will be  relevant to every Panel, but all of the steps must be considered.  

1. Validate: Make sure this Panel is actually the parent of the child Widget; return false if it is not.
2. Orphan: Call orphan(Widget) first while the child Widget is still attached; this triggers onDetach() and sets the child Widget's parent to null.
3. Physical Detach: Adjust the DOM to account for the removal of the child Widget. The Widget's Element must be physically removed from the DOM, either directly or indirectly.
4. Logical Detach: Update the Panel's state variables to reflect the removal of the child Widget. Example: the Widget is removed from the Panel's WidgetCollection.

--------------------------------------
We arrived at this design partly by considering the core "primitives" of ownership and who should be responsible for them.

Primitive                 Responsibility?
Panel's logical structure Panel
Physical attach/detach    Panel
set parent/null parent    Panel
onAttach/onDetach         Child

--------------------------------------
Backwards compatibility/changes from the existing API

Panel.adopt(Widget, Element) becomes deprecated.  The problem with this method is that it mixes steps 3, 6, & 7 of adding together in an inextricable way -- it does a Widget.removeFromParent(), a DOM appendChild(), and a Widget.setParent(this).  The only way to get the right flow while using this method is to perform step 3 twice in many cases.  The fact that it accepts a null container element is a unfortunate, because in many cases it's just wrong and lets a Panel can cause onAttach() to fire before the Widget is physically attached to the DOM.  Any value gained by making code size slightly smaller is totally lost in making Panel code less readable.  The new method of choice is Panel.adopt(Widget), which explicitly does not mess with the DOM, and just delegates to the package-protected Widget.setParent(this), possibly triggering onAttach().

Panel.disown() also becomes deprecated.  Again, we find that it mixes up steps 3 & 4 of removal by doing both a Widget.setParent(null) and DOM.removeChild().  While in many Panels this does the right thing, more complex panels have to do additional DOM manipulations.  So while not completely broken, its value simply does not justify the obscurity it brings-- it's better to just have all Panels deal with physical detachment explicitly so the correct pattern is clear.  The new method of choice is Panel.orphan(), which does not mess with the DOM but just delegates to the package-protected Widget.setParent(null), possibly triggering onDetach().

[Reinier Zwitserloot]

We've had discussions before about the very very strange interaction
between the different parts of the (re)attach process, resulting in
some strange bugs in otherwise very simple operations (like inserting
widgets into a flowpanel on a specific location). This analysis looks
good to me - and the clarity should make future work a lot easier.
Well worth creating some @Deprecated methods. The extensive javadoc on
Panel.add/remove should help in efforts to roll your own panels for
your own projects, as well.

The design LGTM, in other words :-P

On Jul 31, 11:35 pm, "Scott Blum" <sco...@google.com> wrote:
> Hi everyone,
>
> Adding and removing a widget to/from a panel (or other parent) is a process
> that has evolved over time, as we've come to realize what the right behavior
> needs to be. Up until now, there hasn't been a formal design for doing this
> correctly. Rather, we've been converging on a set of behaviors that mostly
> work, but aren't completely consistent across the board. It's time we
> formalized exactly what needs to happen in order to correctly add or remove
> a widget. So I've proposed a "template" or order of operations for handlin
> this correctly. But first let's talk about the guiding principles behind
> the design.
>
> --------------------------------------
> Guiding Principles
>

> 1. onAttach/onDetach should occur only when both the DOM and the

> Widget structure are in a consistent, valid state. In practical terms, that
> means onAttach is the last thing to happen during an add, and onDetach is
> the first thing to happen during a remove.

> 2. Fail fast; avoid leaving things in an invalid state. In practical

> terms, error checking and other operations should happen first.

> 3. The template should be evident in any given Panel's add/remove

> method, to make it easy to maintain, enhance, and copy from.
>
> --------------------------------------
> The add()/insert() Template (also, SimpelPanel.setWidget() style methods)
>
> There are several important things that must take place in the correct
> order to properly add or insert a Widget to a Panel. Not all of these steps
> will be relevant to every Panel, but all of the steps should be
> considered.
>

> 1. *Validate:* Perform any sanity checks to ensure the Panel can

> accept a new Widget. Examples: checking for a valid index on insertion;
> checking that the Panel is not full if there is a max capacity.

> 2. *Account for Reinsertion:* Some Panels need special handling for

> the case where the Widget is already a child of this Panel. Example: when
> performing a reinsert, the index might need to be adjusted to account for
> the Widget's removal.

> 3. *Detach New Child:* Remove the Widget from its existing parent, if

> any. Most Panels will simply call Widget.removeFromParent() on the
> Widget. It's important to do this before performing any state changes to
> the Panel because some Widgets cannot be removed from their parents and will
> throw an IllegalStateException; such exceptions should be allowed to escape
> to the caller.

> 4. Remove an Existing Child (setWidget): Some Panels have a limited

> number of slots with methods whose semantics dictate that some existing
> child will be removed to make way for the new child. This should happen
> immediately after detaching the new child (which could fail) but before
> updating any state relating to inserting the new child.

> 5. *Logical Attach:* Any state variables of the Panel should be

> updated to reflect the addition of the new Widget. Example: the Widget is
> added to the Panel's WidgetCollection at the appropriate index.

> 6. *Physical Attach:* The Widget's Element must be physically attached

> to the Panel's Element, either directly or indirectly.

> 7. *Adopt:* Call adopt(Widget) to finalize the add as the very last

> step; this sets the child Widget's parent to the Panel and triggers
> onAttach().
>
> --------------------------------------
> The remove() Template
>
> There are several important things that must take place in the correct
> order to properly remove a Widget from a Panel. Not all of these steps will
> be relevant to every Panel, but all of the steps must be considered.
>

> 1. *Validate:* Make sure this Panel is actually the parent of the

> child Widget; return false if it is not.

> 2. *Orphan:* Call orphan(Widget) first while the child Widget is still

> attached; this triggers onDetach() and sets the child Widget's parent to
> null.

> 3. *Physical Detach:* Adjust the DOM to account for the removal of the

> child Widget. The Widget's Element must be physically removed from the DOM,
> either directly or indirectly.

> 4. *Logical Detach:* Update the Panel's state variables to reflect the

> widget-add-remove-SOLID-SNAKE-r1282.patch
> 79KDownload

[mP]

I believe before a widget is inserted it should be formally removed.
Inserting somewhere else should not automatically remove it from the
previous parent. This sort of thing just encourages laziness,
developers should be actively aware that a widget is being removed
from one parent and being adopted by another.

[Ray Cromwell]

My concern is with steps 6/7. How the completion of this step is defined is very important. For example, for my purposes, DOM.appendChild()  returning successfully is only a neccessary, but not sufficient, definition of physical attachment.

The problems occurs with attaching elements which are not immediately ready for operations. Consider the case of a FlashCanvas widget, whose physical attachment consists of attaching a EMBED or OBJECT element. If a client were to place code into an onAttach() method which invoked methods on the OBJECT element before it was fully initialized, the result will either be a) inconsistent (depending on caching/speed of initialization or b) a browser crash (as is the case with Silverlight and the Java Plugin)

So I strongly favor Child responsibility for invoking onAttach, because in the case of Flash wrappers, it needs to be done asynchronously.

The other question is, when does the parent Panel call onAttach()? It seems like you have the following cases (for an unattached panel being attached)

1) the Panel has no children, so it calls onAttach after steps 6/7
2) the Panel has existing children, all of which are already attached, so it calls onAttach after steps 6/7
3) the Panel has existing children, some of which are in the process of attaching themselves, so does it call onAttach as soon as the Panel is added to the DOM,
or does it wait for all children currently unattached to become attached?

-Ray

[Joel Webber]

@mP: I agree that perhaps it would be better to disallow adding a widget to a panel before it is removed from its current parent. But unfortunately, this would be a breaking change for existing code.

@Ray:

On the question of onAttach/onLoad for embed/object tags that need to defer their initialization: I encountered a similar problem with RichTextArea on some browsers. You can't usually set the iframe to designMode='on' until sometime after it's formally attached. In onLoad(), it simply sets a timeout (or an onload handler in some instances) that finishes the initialization later.

As to when the parent panel calls onAttach(), it actually doesn't do so directly. Panel.adopt() just calls child.setParent(this), which can trigger the onAttach() method to be called, but only if the parent itself is already attached. This guarantees that a widget's onAttach() is called precisely when there is an unbroken chain of parents all the way to a RootPanel. So in the three cases you mention:

1) No worries.

2) If a panel's children are attached, then it must already be, so setParent() will not call onAttach() again.

3) A panel's children will always be either attached or unattached, so this can't occur.

I hope this clears things up a bit, and please let me know if we've missed anything. In particular, I want to make sure that the flash/silverlight/applet cases can be dealt with properly.

Thanks,

joel.

[Fred Sauer]

Scott,

I'm very happy the whole insert/remove situation is being thoroughly reviewed. The end result will certainly make my life easier.


Two minor notes on the patch to start with:

1. The patch is missing LocalServletRequest.java (I realize files not yet in SVN fall out pretty easily)
2. The javadoc on @domInsert for ComplexPanel#insert(Widget, Element, int, boolean) has been inverted


As far as backwards compatibility, I do have code out there that would break with this patch, but I'm quite happy to live with that if the current APIs can be improved. It would be a compile time breakage, which personally I prefer, because that is so much easier to locate and fix.

Thanks
Fred

On 7/31/07, Scott Blum <sco...@google.com> wrote:

--
Fred Sauer
fr...@allen-sauer.com

[Scott Blum]

On 7/31/07, Fred Sauer <fr...@allen-sauer.com> wrote:

I'm very happy the whole insert/remove situation is being thoroughly reviewed. The end result will certainly make my life easier.

Thanks.

Two minor notes on the patch to start with:

1. The patch is missing LocalServletRequest.java (I realize files not yet in SVN fall out pretty easily)

Oh, the stray changes to RemoteServiceServlet were just in my local copy and were not supposed to be in the patch.  Clean patch file attached.

2. The javadoc on @domInsert for ComplexPanel#insert(Widget, Element, int, boolean) has been inverted

Thanks, good catch. 

As far as backwards compatibility, I do have code out there that would break with this patch, but I'm quite happy to live with that if the current APIs can be improved. It would be a compile time breakage, which personally I prefer, because that is so much easier to locate and fix.

Compile time breaks, eth?  I'm interested to hear specifically what's breaking I didnt realize I was breaking code.

Scott

[Scott Blum]

Helps if I actually attach it, doesn't it?

[Miroslav Pokorny]

I had a quick think and i believe the parent should be responsible for calling all the on** methods on the child. The child should only be responsible for itself that is maintaining its internal state.  The same concept applies to a parent ( a panel ). A child belongs to it so its the parents responsibility to maintain its internal state, which in this case is its children.

[Joel Webber]

On 8/1/07, Miroslav Pokorny <miroslav...@gmail.com> wrote:

I had a quick think and i believe the parent should be responsible for calling all the on** methods on the child. The child should only be responsible for itself that is maintaining its internal state.  The same concept applies to a parent ( a panel ). A child belongs to it so its the parents responsibility to maintain its internal state, which in this case is its children.

This isn't too far off what actually happens:

- parent.add() calls adopt(child)

- adopt() calls child.setParent(this)

- child.setParent () notices that its new parent is attached and calls this.onAttach()

 

This way, the parent only needs to call child.setParent(), and onAttach/onLoad/onDetach/onUnload will get called as appropriate, and in a way that is identical for all widgets. If Panel.adopt() were responsible for both setting its child's parent and setting its attachment state, two things would result:

- We would have to expose setAttached() as a public method, which would open up the possibility of attached state corruption, and

- All implementors of HasWidgets would have to duplicate the code in adopt() (because not all HasWidgets' are Panels). This is true currently as well, but there's only one method call in it, so it's a bit simpler.

Make sense?

[Fred Sauer]

Scott,

On 8/1/07, Scott Blum <sco...@google.com> wrote:

As far as backwards compatibility, I do have code out there that would break with this patch, but I'm quite happy to live with that if the current APIs can be improved. It would be a compile time breakage, which personally I prefer, because that is so much easier to locate and fix.

Compile time breaks, eth?  I'm interested to hear specifically what's breaking I didnt realize I was breaking code.

I thought you might be. Here's the class in question:
  http://gwt-dnd.googlecode.com/svn/trunk/DragDrop/src/com/allen_sauer/gwt/dragdrop/client/temp/IndexedFlowPanel.java@277

As you can tell by the package name, this class wasn't intended to drink from the fountain of youth forever. Actually, I would argue that hacked implementations like this make a good case for the current refactoring effort being undertaken. This implementation breaks with the slightest modification of the underlying implementations. It also is also far from exemplary.

Thanks--
Fred Sauer
fr...@allen-sauer.com

[Scott Blum]

Ok, this is what I was hoping it was.  When I made that change, I thought that ComplexPanel.insert(3) method had been added in 1.4.10.  But I checked 1.3.3 and it was in at that time.

I think the right thing to do is add back the 3 arg version that delegates to the 4 arg version, but also deprecate the 3 arg version.  Thoughts?

Scott

[Joel Webber]

SGTM.

[Fred Sauer]

Scott,

On 8/1/07, Scott Blum <sco...@google.com> wrote:

Ok, this is what I was hoping it was.  When I made that change, I thought that ComplexPanel.insert(3) method had been added in 1.4.10.  But I checked 1.3.3 and it was in at that time.

I think the right thing to do is add back the 3 arg version that delegates to the 4 arg version, but also deprecate the 3 arg version.  Thoughts?

Agreed. I believe we have the responsibility to do everything possible to maintain the 1.3.3 API and method implementation behavior. Deprecation is the perfect way to let application developers update their code when it is convenient for them, while providing a path forward for the API.

The 3 arg method would need a bit of work though, for example, to deal with the case where the second argument is null.

Thanks
Fred

[Scott Blum]

On 8/1/07, Fred Sauer <fr...@allen-sauer.com> wrote:

On 8/1/07, Scott Blum <sco...@google.com > wrote:

Ok, this is what I was hoping it was.  When I made that change, I thought that ComplexPanel.insert(3) method had been added in 1.4.10.  But I checked 1.3.3 and it was in at that time.

I think the right thing to do is add back the 3 arg version that delegates to the 4 arg version, but also deprecate the 3 arg version.  Thoughts?

Agreed. I believe we have the responsibility to do everything possible to maintain the 1.3.3 API and method implementation behavior. Deprecation is the perfect way to let application developers update their code when it is convenient for them, while providing a path forward for the API.

Excellent.

The 3 arg method would need a bit of work though, for example, to deal with the case where the second argument is null.

Hmmm.... your point is well taken.  The problem, though, is that calling add/insert with a null container gives you an execution order that cannot possibly be right .  I think it's extremely unfortunate that a null was ever allowed.  Personally I'm willing to break backwards compatibility for code that never could have worked correctly.  The NPE will at least force the developer to come back and address the issue.  FWIW, the doc for those methods never stated that null was a legal value for the container.

But I'll demur to Joel or Kelly to make the call on this.

Scott

[Fred Sauer]

Scott,

I'm perfectly happy with the NPE. And, in general, I would agree that it better to break previous hacks, than to continue to silently support something that cannot possibly be right :). I little note in the javadoc to mention that null is no longer allowed might be helpful to others out there.

I wonder though if other uses of the old 3 arg method are potentially incorrect as well. If it turns out to be reasonable to expect that a lot of existing 3 arg uses would be incorrect, then maybe a NotImplementedException is in order for the method. There be dragons....


A final recommendation for this refactoring/cleanup for 1.4 would be to really beef up all the assertions in methods that affect insertion/attachment/detachment so that the bad client code (new or existing) can get caught red handed. In the 3 arg method above I imagine a sprinkling of assertions could catch the misuse even before a NPE is thrown. That would mitigate the need for a special mention on the null parameter in the docs.

Fred

[Scott Blum]

What I've done in my local copy, pending Joel's approval, is to have the 3-argument version explicitly null-check container and thrown a direct NPE with a helpful message.  So I definitely think we're on the same page.  My intent was definitely to put assert statements in the most useful places I could think of, so if you spot more places that could benefit, definitely point them out.

Scott

[Fred Sauer]

On 8/1/07, Scott Blum <sco...@google.com> wrote:

What I've done in my local copy, pending Joel's approval, is to have the 3-argument version explicitly null-check container and thrown a direct NPE with a helpful message.  So I definitely think we're on the same page.  My intent was definitely to put assert statements in the most useful places I could think of, so if you spot more places that could benefit, definitely point them out.

Scott

Sounds good. I'll keep an eye out.


I guess what I was trying to state in a round about way was this:

• Good or bad, 1.3.3 had implicit contracts for what each method did
• With the 1.4 cleanup, many of these contracts change and/or become explicit
• Assertions provide I nice way to stop any client code which depends on the 1.3.3 contracts, at least in those cases where backward compatibility is not desirable because the old behavior was simply incorrect
  

Fred

 

On 8/1/07, Fred Sauer <fr...@allen-sauer.com> wrote:

I'm perfectly happy with the NPE. And, in general, I would agree that it better to break previous hacks, than to continue to silently support something that cannot possibly be right :). I little note in the javadoc to mention that null is no longer allowed might be helpful to others out there.

I wonder though if other uses of the old 3 arg method are potentially incorrect as well. If it turns out to be reasonable to expect that a lot of existing 3 arg uses would be incorrect, then maybe a NotImplementedException is in order for the method. There be dragons....

A final recommendation for this refactoring/cleanup for 1.4 would be to really beef up all the assertions in methods that affect insertion/attachment/detachment so that the bad client code (new or existing) can get caught red handed. In the 3 arg method above I imagine a sprinkling of assertions could catch the misuse even before a NPE is thrown. That would mitigate the need for a special mention on the null parameter in the docs.

--
Fred Sauer
fr...@allen-sauer.com

[Miroslav Pokorny]

Currently the emualted types have lots of jsni but in the future when the rest of the java.lang.* and java.util.* classes get emulated these most probably will not contain jsni and should be tested against the real counterparts. Perhaps with this in mind does my suggestion make sense.

[mP]

Doh server got mixed up this post ended up in the wrong thread.