remove child on PNode fails with NPE

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a PNode
2. Add an event handler that removes that PNode from the root.
3. Fire the event handler multiple times.

What is the expected output? What do you see instead?
Rather than failing gracefully, you get an NPE on every event after the
first one.  In most cases it is safe to return gracefully if there are no
nodes to remove (which is what the corresponding function on JComponent does).

The proposed fix is to check for existence of the node before removal so
the client code does not have to. 

Here is a diff of the change:
Index: src/edu/umd/cs/piccolo/PNode.java
===================================================================
--- src/edu/umd/cs/piccolo/PNode.java   (revision 22)
+++ src/edu/umd/cs/piccolo/PNode.java   (revision 23)
@@ -30,68 +30,38 @@

 package edu.umd.cs.piccolo;

-import java.awt.Color;
-import java.awt.Graphics;
-import java.awt.Graphics2D;
-import java.awt.GraphicsConfiguration;
-import java.awt.GraphicsEnvironment;
-import java.awt.Image;
-import java.awt.Paint;
-import java.awt.Transparency;
-import java.awt.geom.AffineTransform;
-import java.awt.geom.Dimension2D;
-import java.awt.geom.NoninvertibleTransformException;
-import java.awt.geom.Point2D;
-import java.awt.geom.Rectangle2D;
-import java.awt.image.BufferedImage;
-import java.awt.print.Book;
-import java.awt.print.PageFormat;
-import java.awt.print.Paper;
-import java.awt.print.Printable;
-import java.awt.print.PrinterJob;
-import java.beans.PropertyChangeEvent;
-import java.beans.PropertyChangeListener;
-import java.io.ByteArrayInputStream;
-import java.io.IOException;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
-import java.io.Serializable;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Enumeration;
-import java.util.Iterator;
-import java.util.List;
-import java.util.ListIterator;
-
-import javax.swing.event.EventListenerList;
-import javax.swing.event.SwingPropertyChangeSupport;
-import javax.swing.text.MutableAttributeSet;
-import javax.swing.text.SimpleAttributeSet;
-
 import edu.umd.cs.piccolo.activities.PActivity;
 import edu.umd.cs.piccolo.activities.PColorActivity;
 import edu.umd.cs.piccolo.activities.PInterpolatingActivity;
 import edu.umd.cs.piccolo.activities.PTransformActivity;
 import edu.umd.cs.piccolo.event.PInputEventListener;
-import edu.umd.cs.piccolo.util.PAffineTransform;
-import edu.umd.cs.piccolo.util.PBounds;
-import edu.umd.cs.piccolo.util.PNodeFilter;
-import edu.umd.cs.piccolo.util.PObjectOutputStream;
-import edu.umd.cs.piccolo.util.PPaintContext;
-import edu.umd.cs.piccolo.util.PPickPath;
-import edu.umd.cs.piccolo.util.PUtil;
+import edu.umd.cs.piccolo.util.*;

+import javax.swing.event.EventListenerList;
+import javax.swing.event.SwingPropertyChangeSupport;
+import javax.swing.text.MutableAttributeSet;
+import javax.swing.text.SimpleAttributeSet;
+import java.awt.*;
+import java.awt.geom.*;
+import java.awt.image.BufferedImage;
+import java.awt.print.*;
+import java.beans.PropertyChangeEvent;
+import java.beans.PropertyChangeListener;
+import java.io.*;
+import java.util.*;
+import java.util.List;
+
 /**
  * <b>PNode</b> is the central abstraction in Piccolo. All objects that are
  * visible on the screen are instances of the node class. All nodes may have 
  * other "child" nodes added to them.
- * <p>
+ * <p/>
  * See edu.umd.piccolo.examples.NodeExample.java for demonstrations of how
nodes
  * can be used and how new types of nodes can be created.
- * <P>
- * @version 1.0
+ * <p/>
+ *
  * @author Jesse Grosjean
+ * @version 1.0
  */
 public class PNode implements Cloneable, Serializable, Printable {

@@ -177,7 +147,8 @@
     * The property name that identifies a change in the set of this node's
direct children
     * (see {@link #getChildrenReference getChildrenReference}, {@link
#getChildrenIterator getChildrenIterator}).
     * In any property change event the new value will be a reference to this
node's children,
-    * but  old value will always be null. */
+     * but  old value will always be null.
+     */
     public static final String PROPERTY_CHILDREN = "children";
     public static final int PROPERTY_CODE_CHILDREN = 1 << 9;

@@ -203,6 +174,7 @@
     */
    public interface PSceneGraphDelegate {
        public void nodePaintInvalidated(PNode node);
+
        public void nodeFullBoundsInvalidated(PNode node);
    }

@@ -234,7 +206,7 @@

     /**
     * Constructs a new PNode.
-    * <P>
+     * <p/>
     * By default a node's paint is null, and bounds are empty. These values
     * must be set for the node to show up on the screen once it's added to
     * a scene graph. 
@@ -391,6 +363,7 @@
                public void setTransform(AffineTransform aTransform) {
                    PNode.this.setTransform(aTransform);
                }
+
                public void getSourceMatrix(double[] aSource) {
                    PNode.this.getTransformReference(true).getMatrix(aSource);
                }
@@ -425,6 +398,7 @@
                public Color getColor() {
                    return (Color) getPaint();
                }
+
                public void setColor(Color color) {
                    setPaint(color);
                }
@@ -528,11 +502,11 @@

    /**
     * Add an arbitrary key/value to this node.
-    * <p>
+     * <p/>
     * The <code>get/add attribute<code> methods provide access to
     * a small per-instance attribute set. Callers can use get/add attribute
     * to annotate nodes that were created by another module.
-    * <p>
+     * <p/>
     * If value is null this method will remove the attribute.
     */
    public void addAttribute(Object key, Object value) {
@@ -618,9 +592,11 @@
            public boolean hasNext() {
                return enumeration.hasMoreElements();
            }
+
            public Object next() {
                return enumeration.nextElement();
            }
+
            public void remove() {
                throw new UnsupportedOperationException();
            }
@@ -959,6 +935,7 @@
     * The listener is registered for all properties.
     * See the fields in PNode and subclasses that start
     * with PROPERTY_ to find out which properties exist.
+     *
     * @param listener  The PropertyChangeListener to be added
     */
    public void addPropertyChangeListener(PropertyChangeListener listener) {
@@ -973,6 +950,7 @@
     * will be invoked only when a call on firePropertyChange names that
     * specific property. See the fields in PNode and subclasses that start
     * with PROPERTY_ to find out which properties are supported.
+     *
     * @param propertyName  The name of the property to listen on.
     * @param listener  The PropertyChangeListener to be added
     */
@@ -1157,10 +1135,10 @@
    /**
     * Set the bounds of this node to the given value. These bounds 
     * are stored in the local coordinate system of this node.
-    * 
+     * <p/>
     * If the width or height is less then or equal to zero then the bound's
     * emtpy bit will be set to true.
-    * 
+     * <p/>
     * Subclasses must call the super.setBounds() method.
     *  
     * @return true if the bounds changed.
@@ -1188,7 +1166,7 @@
     * Gives nodes a chance to update their internal structure
     * before bounds changed notifications are sent. When this message
     * is recived the nodes bounds field will contain the new value.
-    * 
+     * <p/>
     * See PPath for an example that uses this method.
     */
    protected void internalUpdateBounds(double x, double y, double width,
double height) {
@@ -1945,7 +1923,7 @@
     * the source anchor point coincides with the reference anchor
     * point. This can be useful for layout algorithms as it is
     * straightforward to position one object relative to another.
-    * <p>
+     * <p/>
     * For example, If you have two nodes, A and B, and you call
     * <PRE>
     *     Point2D srcPt = new Point2D.Double(1.0, 0.0);
@@ -1955,6 +1933,7 @@
     * The result is that A will move so that its upper-right corner is at
     * the same place as the upper-left corner of B, and the transition will
     * be smoothly animated over a period of 750 milliseconds.
+     *
     * @param srcPt The anchor point on this transform's node (normalized to
a unit square)
     * @param destPt The anchor point on destination bounds (normalized to a
unit square)
     * @param destBounds The bounds (in global coordinates) used to calculate
this transform's node
@@ -2812,8 +2791,12 @@
     * @return the removed child
     */
    public PNode removeChild(PNode child) {
-       return removeChild(indexOfChild(child));
+        int index = indexOfChild(child);
+        if (index == -1) {
+            return null;
    }
+        return removeChild(index);
+    }

    /**
     * Remove the child at the specified position of this group node's children.
@@ -2824,6 +2807,9 @@
     * @return the removed child
     */
    public PNode removeChild(int index) {
+        if (children == null) {
+            return null;
+        }
        PNode child = (PNode) children.remove(index);

        if (children.size() == 0) {

Original issue reported on code.google.com by steveonjava on 24 Jun 2008 at 9:02

[GoogleCodeExporter]

Original comment by steveonjava on 25 Jun 2008 at 6:35

• Added labels: Milestone-1.3

[GoogleCodeExporter]

Original comment by steveonjava on 2 Jul 2008 at 12:28

• Added labels: Milestone-2.0

[GoogleCodeExporter]

Original comment by steveonjava on 3 Jul 2008 at 5:05

• Added labels: Milestone-1.3

[GoogleCodeExporter]

Committed to trunk, with modifications (did not commit formatting changes).

commit -m "Issue 23 ; check for existence of a node before removal to prevent
NullPointerExceptions"
    Sending        piccolo2d.java-trunk/core/src/main/java/edu/umd/cs/piccolo/PNode.java
    Transmitting file data ...
    Committed revision 397.

Original comment by heue...@gmail.com on 7 Oct 2008 at 8:58

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by mr0...@mro.name on 25 Oct 2009 at 6:46

• Changed state: Verified